Add zig cc support for LLVM flag --no-undefined-version

[DhashS]

I'm pretty sure this is not all I need to do, so would appreciate some guidance from the broader zig community.

This issue is what i'm trying to tackle #16855 by punching that flag through to LLVM.

I've also seen it invoked as -Wl,--no-undefined-version, so i'm wondering if to parse it in the .Wl section as well. I didn't initially since it seems like there's a catch-all append call that gets parsed where I specified.

[andrewrk]

Thanks for working on this.

What does this do? The ld --help menu isn't particularly helpful:

  --undefined-version         Allow undefined version
  --no-undefined-version      Disallow undefined version

[motiejus]

I'm pretty sure this is not all I need to do, so would appreciate some guidance from the broader zig community.

You will need to add a case in src/link/Elf.zig. See a833bdc for a simple isolated example.

I've also seen it invoked as -Wl,--no-undefined-version, so i'm wondering if to parse it in the .Wl section as well. I didn't initially since it seems like there's a catch-all append call that gets parsed where I specified.

Looking at the man pages of gcc and clang, I think this is the only valid way to call it (via -Wl,). And seems like you did it correctly for that case.

After you've built zig, you can poke at it:

$ ZIG_VERBOSE_LINK=1 zig cc -Wl,--no-undefined-version <...>

... and then observe that the flag is present in the line that starts with ld (then it's calling the underlying linker).

Thanks for working on this.

What does this do? The ld --help menu isn't particularly helpful:

  --undefined-version         Allow undefined version
  --no-undefined-version      Disallow undefined version

from gcc man page:

       Normally when a symbol has an undefined version, the linker will ignore it. This option disallows symbols with
       undefined version and a fatal error will be issued instead.

Also a relevant lld commit: https://reviews.llvm.org/D135402

[motiejus]

Also add a comment in approx https://github.com/ziglang/zig/pull/17326/files#diff-d924ca21d81d7d5a59eb9e10d3e2689c4c58a7e28e6ecce6a064701666f5a730L481

[DhashS]

Thanks for the pointer to that commit @motiejus i've done as requested

[motiejus]

I am leaving the suggestion "strange" with double-negatives (i.e. no_undefined_version instead of undefined_version), because that's the exact flag that man ld offers.

[DhashS]

makes sense, yeah i can't imagine a case where you're specifying both of them and it's correct.
looks like you left the parsing logic in though, so it'll handle both flags.

[DhashS]

Cool, now that i know what you're looking for, i'll give this PR a once over in the morning to make sure it's decent given the back and forth.

[kubkon]

In addition to my code suggestions, it is also necessary to bump the hash version, and check that everywhere we hit a compile error we have correctly added the flag to relevant caches. Notably here and here.

[kubkon]

I am also thinking that I might start requiring folks enabling new linker features to come up with linker tests and them here. This will help me in the long run as I will be able to simply change the test case from use_lld = true to use_lld = false and see if my implementation passes the test.

[DhashS]

In my understanding, things that y'all would like

• use the LLD docstring
• make sure caches are happy
• make build.zig (zig build system) has test coverage for new code paths

[DhashS]

okay, not sure how to do those last two, cc @motiejus @kubkon

[motiejus]

In my understanding, things that y'all would like

* [x]  use the LLD docstring

* [ ]  make sure caches are happy

You are very close. What's missing:

• Add those linker flags to the cache, exactly like Jakub mentioned in Add zig cc support for LLVM flag --no-undefined-version  #17326 (comment) ;
• Bump the constant link_hash_implementation_version.

* [ ]  make build.zig (zig build system) has test coverage for new code paths

@kubkon I totally see why this would make sense, but I think it's unfair to ask to write such test for newcomers: @DhashS already is shaving the n'th yak by coming from from hermetic_cc_toolchain, because his builds stopped working after Rust upgrade (when it started using this new flag).

Even though I feel like I know something in Zig by now (both the language and how it's wired up), I would certainly spend more than a day writing such test. So it would be nice to have, but perhaps too early or much to ask.

[DhashS]

sorry, took me a second to figure out what y'all meant.

[scanterog]

We're hitting the same issue. Thanks for the fix @DhashS :) @motiejus @kubkon would you be able to review/merge/release this one 🙏 ?

[motiejus]

Looks good to me, but I am not the one deciding what goes to master and when. :)

Anyhow, looks like it is failing CI checks due to compile errors.

[scanterog]

yeah I think I've spotted why CI is failing cc @DhashS

[behos]

👋 @kubkon it looks like there are some updates in the PR which might solve the CI issues.
Is there something else blocking this?

[DhashS]

would really like to get this merged so that we can bump our rust version :)

[motiejus]

All linux tests are failing, it is easier to get attention with a working CI.

[behos]

@DhashS looks like a formatting error

diff --git a/src/main.zig b/src/main.zig
index a4d7ae93f..9ef095ef6 100644
--- a/src/main.zig
+++ b/src/main.zig
@@ -2379,7 +2379,7 @@ fn buildOutputType(
                     linker_allow_undefined_version = true;
                 } else if (mem.eql(u8, arg, "--no-undefined-version")) {
                     linker_allow_undefined_version = false;
-                }  else if (mem.eql(u8, arg, "--version")) {
+                } else if (mem.eql(u8, arg, "--version")) {
                     try std.io.getStdOut().writeAll("zig ld " ++ build_options.version ++ "\n");
                     process.exit(0);
                 } else {

I think this is what's picked up in CI. You can run zig fmt on the changed files.

[behos]

@DhashS since only some formatting seems to be blocking this PR and it's been pending for a while now, if you don't have time to update it I can create another fork from your branch and reopen it after running zig fmt. Your commits will be left intact.