bintools-wrapper: support lld's -flavor argument

[rrbutani]

Description of changes

lld supports specifying the linker flavor with -flavor however this must be the first argument passed. This PR updates bintools-wrapper'd ld-wrapper to hoist -flavor before NIX_LDFLAGS_BEFORE/extraBefore when passed as the first argument to the wrapped linker.

---

Note: I'm pretty sure this patch does not interact correctly with cc-wrapper's use of ld-wrapper; IIUC cc-wrapper will handle NIX_LDFLAGS_BEFORE in lieu of ld-wrapper which makes it so that we cannot distinguish between "injected" linker args and pre-existing linker args within ld-wrapper.

For the use case that prompted this PR (#231951) this isn't an issue (we were only trying to work around linker hardening flags which always get handled by linker-wrapper I believe) but this still seems like something we should fix.

If we're okay "incorrectly" hoisting -flavor (i.e. when not passed as the first argument) then having link-wrapper scan params would be an easy fix for this.

I'm still waiting for stuff to rebuild locally so I can test this; marking this PR as a draft for now.

---

cc @RaitoBezarius

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.05 Release Notes (or backporting 22.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[ghost]

lld cares about the order in which flags are passed? Other than the obvious later-flags-override-earlier flags? Why? That sounds crazy.

Also,

Does not inspire confidence. I don't think LLVM wants us to be adding new uses of this. If they did, they would fix the wacky flag-ordering constraint.

[alyssais]

lld cares about the order in which flags are passed? Other than the obvious later-flags-override-earlier flags? Why? That sounds crazy.

It makes sense to me — it completely changes how argument parsing works. In Windows-style linking, "-flavor" wouldn't even look like a flag, right?

[RaitoBezarius]

lld cares about the order in which flags are passed? Other than the obvious later-flags-override-earlier flags? Why? That sounds crazy.

Correct, because this is the Windows driver for LLD which switches the whole parsing in a Windows-style.

Also,

Does not inspire confidence. I don't think LLVM wants us to be adding new uses of this. If they did, they would fix the wacky flag-ordering constraint.

We can invoke lld-link instead directly. The whole problem is that the whole universe (maybe just Rust?) expects to use that API: https://github.com/rust-lang/rust/blob/0bcfd2d96efe7a2cb5205c3af1b9eea17423fe65/src/tools/lld-wrapper/src/main.rs#L70-L85

I'm not sure this is tractable for the near future to fix everyone doing it wrong at the moment.

[alyssais]

The rustc documentation also mentions that it always uses -flavor with lld.

[ghost]

It makes sense to me — it completely changes how argument parsing works.

I mean then it isn't really a flag, is it? It sounds like this flavor business is really part of argv[0]. Or should be. Can't you folks just make four different lld-${flavor} wrappers for the four approved flavors?

[rrbutani]

It sounds like this flavor business is really part of argv[0]. Or should be.

argv[0] is indeed the other mechanism lld provides for specifying the linker flavor.

Can't you folks just make four different lld-${flavor} wrappers for the four approved flavors?

We went down this route first but my understanding is that rustc makes this a little more annoying to use; i.e. you have to specify -C linker and -C linker-flavor because (AIUI) there's no way to nudge rustc into using lld-link via -C linker-flavor (as mentioned in the docs Alyssa linked).

[alyssais]

> Can't you folks just make four different `lld-${flavor}` wrappers for the four approved flavors? We went down this route first but my understanding is that `rustc` makes this a little more annoying to use; i.e. you have to specify `-C linker` and `-C linker-flavor` because (AIUI) there's no way to nudge `rustc` into using `lld-link` via `-C linker-flavor` (as mentioned in the [docs Alyssa linked](https://doc.rust-lang.org/nightly/rustc/codegen-options/index.html#linker-flavor)).

Yeah, I don't like this interface either, but I don't like lots of interfaces that are out there in the world. We don't get to decide which interfaces get adopted, and this one sadly has been.

[ghost]

Can't you folks just make four different lld-${flavor} wrappers for the four approved flavors?

We went down this route first but my understanding is that rustc makes this a little more annoying to use; i.e. you have to specify -C linker and -C linker-flavor because (AIUI) there's no way to nudge rustc into using lld-link via -C linker-flavor (as mentioned in the docs Alyssa linked).

I'm not quite understanding this.

If that flag needs to go first, and affects all subsequent flags, then it's really part of argv[0] and we should just tell LLVM that the linker is this script:

#!${runtimeShell}
exec lld -C linker-flavor=windows-slashy-flags "$@"

Now lld is guaranteed to always see -C linker-flavor=windows-slashy-flags as the first two arguments if invoked via this wrapper.

What goes wrong if you simply tell rustc that the script above is "the linker"?

[RaitoBezarius]

Can't you folks just make four different lld-${flavor} wrappers for the four approved flavors?

We went down this route first but my understanding is that rustc makes this a little more annoying to use; i.e. you have to specify -C linker and -C linker-flavor because (AIUI) there's no way to nudge rustc into using lld-link via -C linker-flavor (as mentioned in the docs Alyssa linked).

I'm not quite understanding this.

If that flag needs to go first, and affects all subsequent flags, then it's really part of argv[0] and we should just tell LLVM that the linker is this script:

#!${runtimeShell}
exec lld -C linker-flavor=windows-slashy-flags "$@"

You are confusing rustc flags with lld flags, you meant -flavor xxx.

Now lld is guaranteed to always see -C linker-flavor=windows-slashy-flags as the first two arguments if invoked via this wrapper.

What goes wrong if you simply tell rustc that the script above is "the linker"?

This would be a new "linker-flavor" which would not be "lld" per se I believe.
From the doc @alyssais linked above:

This flag controls the linker flavor used by rustc. If a linker is given with the -C linker flag, then the linker flavor is inferred from the value provided. If no linker is given then the linker flavor is used to determine the linker to use. Every rustc target defaults to some linker flavor. Valid options are:

So if we say -C linker-flavor=lld-link -C linker=themodifiedlinker, it will do themodifiedlinker -flavor link $*, therefore passing -flavor link twice.

We would need to fix rustc to make it understand that our linker is already flavored, I'm not sure how easy this is.
Maybe using -C linker-flavor=msvc would work here because the modified linker should be somewhat a "LLVM" LINK.EXE, but uncertain about this.

[ghost]

You are confusing rustc flags with lld flags, you meant -flavor xxx.

Thanks, updated:

#!${runtimeShell}
exec lld -flavor=link "$@"

What goes wrong if you simply tell rustc that the script above is "the linker"?

So if we say -C linker-flavor=lld-link -C linker=themodifiedlinker, it will do themodifiedlinker -flavor link $*, therefore passing -flavor link twice.

Simply have the script check for an extra flag and drop it. Since it's guaranteed to be the first flag this is easy to check for. Unlike this PR that would:

1. impose no extra complexity burden on unflavored linkers or the rest of nixpkgs.
2. not need to parse and scan the entire list of arguments, since it already knows where the argument-in-need of fixing is (see code comment below).

[ghost]

See previous comment.

[RaitoBezarius]

We should re-evaluate this now in the context of rust-lang/rust#112910.

[RaitoBezarius]

You are confusing rustc flags with lld flags, you meant -flavor xxx.

Thanks, updated:

#!${runtimeShell}
exec lld -flavor=link "$@"

What goes wrong if you simply tell rustc that the script above is "the linker"?

So if we say -C linker-flavor=lld-link -C linker=themodifiedlinker, it will do themodifiedlinker -flavor link $*, therefore passing -flavor link twice.

Simply have the script check for an extra flag and drop it. Since it's guaranteed to be the first flag this is easy to check for. Unlike this PR that would:

1. impose no extra complexity burden on unflavored linkers or the rest of nixpkgs.
2. not need to parse and scan the entire list of arguments, since it already knows where the argument-in-need of fixing is (see code comment below).

Unfortunately, even if we do this and this works for the ld-wrapper, I am not exactly sure this will work for Rust bootstrap.

As you suggested me in #231951 I went in the rabbit-hole and my understanding is that bootstrapping Rust for the UEFI target will induce the same issue where we need rustc to understand what is the linker flavor to properly produce the libcompiler_builtins.rlib in COFF format, otherwise, it will produce ELF, making all the bootstrap process fails.

We have few knobs here, we usually pass:

  # We need rust to build rust. If we don't provide it, configure will try to download it.
  # Reference: https://github.com/rust-lang/rust/blob/master/src/bootstrap/configure.py
  configureFlags = let
    setBuild  = "--set=target.${rust.toRustTarget stdenv.buildPlatform}";
    setHost   = "--set=target.${rust.toRustTarget stdenv.hostPlatform}";
    setTarget = "--set=target.${rust.toRustTarget stdenv.targetPlatform}";
    ccForBuild  = "${pkgsBuildBuild.targetPackages.stdenv.cc}/bin/${pkgsBuildBuild.targetPackages.stdenv.cc.targetPrefix}cc";
    cxxForBuild = "${pkgsBuildBuild.targetPackages.stdenv.cc}/bin/${pkgsBuildBuild.targetPackages.stdenv.cc.targetPrefix}c++";
    ccForHost  = "${pkgsBuildHost.targetPackages.stdenv.cc}/bin/${pkgsBuildHost.targetPackages.stdenv.cc.targetPrefix}cc";
    cxxForHost = "${pkgsBuildHost.targetPackages.stdenv.cc}/bin/${pkgsBuildHost.targetPackages.stdenv.cc.targetPrefix}c++";
    ccForTarget  = "${pkgsBuildTarget.targetPackages.stdenv.cc}/bin/${pkgsBuildTarget.targetPackages.stdenv.cc.targetPrefix}cc";
    cxxForTarget = "${pkgsBuildTarget.targetPackages.stdenv.cc}/bin/${pkgsBuildTarget.targetPackages.stdenv.cc.targetPrefix}c++";
    lldForTarget = "${pkgsBuildTarget.targetPackages.stdenv.cc}/bin/${pkgsBuildTarget.targetPackages.stdenv.cc.targetPrefix}ld.lld";
  in [
    "--release-channel=stable"
    "--set=build.rustc=${rustc}/bin/rustc"
    "--set=build.cargo=${cargo}/bin/cargo"
    "--enable-rpath"
    "--enable-vendor"
    "--build=${rust.toRustTargetSpec stdenv.buildPlatform}"
    "--host=${rust.toRustTargetSpec stdenv.hostPlatform}"
    # std is built for all platforms in --target.
    "--target=${concatStringsSep "," ([
      (rust.toRustTargetSpec stdenv.targetPlatform)

    # (build!=target): When cross-building a compiler we need to add
    # the build platform as well so rustc can compile build.rs
    # scripts.
    ] ++ optionals (stdenv.buildPlatform != stdenv.targetPlatform) [
      (rust.toRustTargetSpec stdenv.buildPlatform)

    # (host!=target): When building a cross-targeting compiler we
    # need to add the host platform as well so rustc can compile
    # build.rs scripts.
    ] ++ optionals (stdenv.hostPlatform != stdenv.targetPlatform) [
      (rust.toRustTargetSpec stdenv.hostPlatform)
    ])}"

    "${setBuild}.cc=${ccForBuild}"
    "${setHost}.cc=${ccForHost}"
    "${setTarget}.cc=${ccForTarget}"

    "${setBuild}.linker=${ccForBuild}"
    "${setHost}.linker=${ccForHost}"
     ${setTarget}.linker=${ccForTarget}")

    "${setBuild}.cxx=${cxxForBuild}"
    "${setHost}.cxx=${cxxForHost}"
    "${setTarget}.cxx=${cxxForTarget}"

...

So the linkers for:

build=x86_64-unknown-gnu-linux
host=x86_64-unknown-gnu-linux
target=x86_64-unknown--uefi

will be, e.g. build=gcc cc-wrapped, host=llvm cc-wrapped, target=llvm's cc-wrapped, which all uses ld under the hood, which won't work for my purposes here.

I am not knowledgeable enough again about cc-wrappers to understand how I can swap the linker backend under the hood of the cc-wrapper and pass it back, otherwise, I will have to hack platform-specifics inside the rustc.nix derivation to build a modified linker with ld.lld, it seems undesirable because the domain knowledge should not really end there I believe.

The easiest seems again to get a ld-wrapper that knows about -flavor, I just build the right CC from the get-go with a ld.lld linker rather than binutils's ld, and… this should work, I imagine now.

I spent more almost 10 hours non-stop on that issue, reading the source code of Rust, asking Rust developers and so on and so forth, I just don't know what to do anymore. :)

[ghost]

Unfortunately, even if we do this and this works for the ld-wrapper, I am not exactly sure this will work for Rust bootstrap.

It seems like you're moving the goalposts a bit here. The stated motivation for this PR is to get ld-wrapper to handle and pass the -flavor argument in spite of its bizarre and weird property of changing the parsing (and lexing!) of all subsequent flags.

I suggested a simple and straightforward way to accomplish this, which would impose no ongoing maintenance burden on the rest of Nixpkgs.

You've posted about how you're struggling with getting Rust to bootstrap itself for a COFF target. I don't doubt that's difficult, but I think it's a bit unreasonable to use that as a justification for pushing this workaround all the way down into ld-wrapper (which, by the way, triggers a full stdenv build anytime you touch it).

Compilers' built-in bootstrapping routines are often inflexible; we moved away from using gcc's for this reason. You may need to apply some small patches to Rust's bootstrap routine if you're committed to letting it (instead of Nix) drive the bootstrap. Don't limit yourselves to "the knobs" you see in front of you!

I will have to hack platform-specifics inside the rustc.nix derivation to build a modified linker with ld.lld, it seems undesirable because the domain knowledge should not really end there I believe.

rustc.nix might not be the ideal place for this goal-specific hack, but ld-wrapper is certainly even less ideal!

I have some advice specific to your problem which I've posted over in #231951.

Let's try to keep this PR on track as dealing with ld-wrapper in a general way.

[RaitoBezarius]

OK, I apologize for my "despair" reply, you're right. I am solving slowly the issues without this PR. We will see when I arrive at the ld-wrapper issue with the tool. :)