rust-lld: fallback to rustc's sysroot if there's no path to the linker in the target sysroot #125263

lqd · 2024-05-18T23:14:46Z

As seen in #125246, some sysroots don't expect to contain rust-lld and want to keep it that way, so we fallback to the default rustc sysroot if there is no path to the linker in any of the sysroot tools search paths. This is how we locate codegen-backends' dylibs already.

People also have requested an error if none of these search paths contain the self-contained linker directory, so there's also an error in that case.

r? @petrochenkov cc @ehuss @RalfJung

I'm not sure where we check for rust-lld's existence on the targets where we use it by default, and if we just ignore it when missing or emit a warning (as I assume we don't emit an error), so I just checked for the existence of gcc-ld, where cc will look for the lld-wrapper binaries.

_{Feel free to point out better ways to do this, it's the middle of the night here.}

Fixes #125246

bjorn3 · 2024-05-19T12:18:36Z

For the codegen backends we search in the sysroot containing rustc itself in addition to the sysroot specified using --sysroot.

lqd · 2024-05-21T10:26:05Z

For the codegen backends we search in the sysroot containing rustc itself in addition to the sysroot specified using --sysroot.

Is that an FYI, or what you are asking me to do?

bjorn3 · 2024-05-21T10:29:45Z

I feel like doing the same sysroot fallback method here would be nicer than falling back to the default linker, but if you prefer falling back to the default linker that is fine by me too.

Kobzol · 2024-05-21T10:32:25Z

I would personally prefer erroring out if lld was not found, rather than silently dropping back to the default linker. If the target uses lld by default, and it is not available, it should be an error, and the user should use the corresponding linker features flag (ideally suggested by the compiler as a hint) to explicitly opt out of lld.

RalfJung · 2024-05-21T11:58:39Z

So like this?

first search --sysroot
then search the rustc location
then error

lqd · 2024-05-21T14:10:48Z

I agree an error would be preferable if we were talking about an opt-in flag; for a change of default values with an opt-out, however, it probably would be less desirable than a fallback to "things work like they used to when they can't work like we now want them too": an unwanted error from cc is what happened to Ralf and Eric, and it seems unlikely that just having a different error message would have made it acceptable.

Kobzol · 2024-05-21T14:20:14Z

Defaults might eventually change, and this is hardly the last one, e.g. I can imagine us switching to Cranelift in the future by default for debug builds, and in such case it will also start being required in the toolchain.

I don't like silent fallbacks, because they can make it more difficult to debug issues ("wait, how are you hitting this issue..? oh, your rustc silently fell back to a completely different linker/codegen backend/etc.").

Of course, we should make sure that the default case works for the default toolchain, but if someone does something custom, then it might be needed for them to e.g. configure the compilation of rustc to add the required stuff to the toolchain, or opt out of this functionality.

RalfJung · 2024-05-21T14:27:55Z

an unwanted error from cc is what happened to Ralf and Eric, and it seems unlikely that just having a different error message would have made it acceptable.

The proposed search order would fix the error for my case, and I think also for Erik's case.

lqd · 2024-05-21T14:37:25Z

an unwanted error from cc is what happened to Ralf and Eric, and it seems unlikely that just having a different error message would have made it acceptable.

The proposed search order would fix the error for my case, and I think also for Erik's case.

Right, because in this case we can mitigate the error. Asking for an error, for rust-lld now or cg-clif in the future, in the general case would have applied here if we weren't able to do the mitigation, getting us back to square 1. There are too many trade-offs and unknowns for us to completely figure this out in a random PR ^^.

RalfJung · 2024-05-21T14:47:43Z

it seems unlikely that just having a different error message would have made it acceptable.

A different error message would have helped a lot, because I first thought "wtf why does it not find the ld binary which is clearly there". It's a gcc bug (assuming they are providing cc here) that the error is so bad, but we can still do something about it.

Given that the proposed search order is what we are already doing for codegen backends (right?), it seems like a reasonable starting point for now.

Silent fallbacks should be a last resort IMO, I don't know that we are already so desperate as to having to go there.

bjorn3 · 2024-05-21T15:33:38Z

I can imagine us switching to Cranelift in the future by default for debug builds, and in such case it will also start being required in the toolchain.

That already works fine with custom sysroots as we fallback to the default sysroot of rustc for locating codegen backends if the custom sysroot doesn't have the specified codegen backend.

lqd · 2024-05-21T18:47:34Z

Ok it's christmas time, fallback search path for some, error if there's no self-contained linker path for the others, presents for everybody 🎅.

@RalfJung is the following how things are expected to work without your sysroot changes, on 0.4.1 IIUC?

$ cargo install cargo-careful --version 0.4.1
  Installing cargo-careful v0.4.1
    Updating crates.io index
     Locking 53 packages to latest compatible versions
      Adding cargo-careful v0.4.1 (latest: v0.4.2)
      Adding linux-raw-sys v0.4.14 (latest: v0.6.4)
      Adding rustc-build-sysroot v0.4.7 (latest: v0.5.2)
      Adding wasi v0.11.0+wasi-snapshot-preview1 (latest: v0.13.1+wasi-0.2.0)
      Adding windows-sys v0.48.0 (latest: v0.52.0)
      Adding windows-targets v0.48.5 (latest: v0.52.5)
      Adding windows_aarch64_gnullvm v0.48.5 (latest: v0.52.5)
      Adding windows_aarch64_msvc v0.48.5 (latest: v0.52.5)
      Adding windows_i686_gnu v0.48.5 (latest: v0.52.5)
      Adding windows_i686_msvc v0.48.5 (latest: v0.52.5)
      Adding windows_x86_64_gnu v0.48.5 (latest: v0.52.5)
      Adding windows_x86_64_gnullvm v0.48.5 (latest: v0.52.5)
      Adding windows_x86_64_msvc v0.48.5 (latest: v0.52.5)
   Compiling rustix v0.38.34
   Compiling semver v1.0.23
   Compiling libc v0.2.155
   Compiling serde v1.0.202
   Compiling linux-raw-sys v0.4.14
   Compiling anyhow v1.0.86
   Compiling bitflags v2.5.0
   Compiling cfg-if v1.0.0
   Compiling same-file v1.0.6
   Compiling fastrand v2.1.0
   Compiling serde_json v1.0.117
   Compiling option-ext v0.2.0
   Compiling ryu v1.0.18
   Compiling itoa v1.0.11
   Compiling walkdir v2.5.0
   Compiling rustc_version v0.4.0
   Compiling dirs-sys v0.4.1
   Compiling directories v5.0.1
   Compiling tempfile v3.10.1
   Compiling rustc-build-sysroot v0.4.7
   Compiling cargo-careful v0.4.1
    Finished `release` profile [optimized] target(s) in 3.88s
  Installing /home/lqd/.cargo/bin/cargo-careful
   Installed package `cargo-careful v0.4.1` (executable `cargo-careful`)

$ cargo +stage1 careful run -q
Preparing a careful sysroot (target: x86_64-unknown-linux-gnu)... done
The value is 513!
thread 'main' panicked at /home/lqd/rust/lqd-rust/library/core/src/panicking.rs:219:5:
unsafe precondition(s) violated: ptr::read requires that the pointer argument is aligned and non-null
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread caused non-unwinding panic. aborting.
Aborted (core dumped)

$ ls ~/.cache/cargo-careful
lib

lqd · 2024-05-22T20:20:52Z

@RalfJung I was editing #125263 (comment) while you were reading it I think. I was adding my results with cargo-careful 0.4.1 that I'm not sure you saw?

RalfJung · 2024-05-22T20:38:49Z

Yes that looks right, the test crate is deliberately buggy and the bug is detected by the debug assertions.

lqd · 2024-05-22T20:42:08Z

Cool, thank you.

compiler/rustc_codegen_ssa/src/back/link.rs

petrochenkov · 2024-05-22T20:53:44Z

compiler/rustc_codegen_ssa/src/back/link.rs

+        if !linker_path_exists {
+            // As a sanity check, we emit an error if none of these paths exist: we want
+            // self-contained linking and have no linker.
+            sess.dcx().emit_fatal(errors::SelfContainedLinkerMissing);


What if a distro wants to drop Rust lld binaries and rely on system lld?

Then the corresponding self-contained directories either won't exist or will be empty.
If we just push the -B options without checking then gcc will go through them and then proceed searching and finding lld on the system without errors.

If we report an error here, then such distros will also need to reset self-contained flags in target specs to false, probably through env vars like the current CFG_USE_SELF_CONTAINED_LINKER.
This will have a negative effect on portability of options like -Clink-self-contained=+linker between distros (if we need such portability).

But I'm fine with reporting a conservative error here for now, it can always be relaxed later.
At least we'll be able see what breaks.

Another reason why I preferred the fallback compared to the error :3, because falling back to system lld is what we currently do in the case you mention of course.

But one can also say having the self-contained linker enabled is an opt-in for the toolchain maker, it goes hand in hand with bootstrap building lld and putting in the sysroot. Not having both can be seen as a mismatch, and that situation would be properly modeled as -Clink-self-contained=-linker -Clinker-features=+lld.

Note that I only check for the existence of the gcc-ld folder, so in theory an empty directory with no lld binaries would pass this test 🤡

One problem with the fallback is that if there is no system lld and we try to use it anyway, the error is really bad:

= note: collect2: fatal error: cannot find 'ld' compilation terminated.

It doesn't even say that it was trying to find lld, mentioning ld instead which is not what it was looking for.

What if a distro wants to drop Rust lld binaries and rely on system lld?

Sounds to me like they should then disable self-contained linking in their rustc builds.

It's definitely not going to be winning an oscar of the best error message, but also it's in gcc, a non-zero number of people are used to it, and look for the -fuse-ld option that is in the command just above that note. (Maybe it also depends on the gcc version and newer ones have improved it 🤔)

But it's neither here nor there, the PR currently emits an error and we'll see what happens with distros. Right now, they'll be fine, and in the future they just may need to learn how to have the system lld fallback which is how you and I describe, quite sensible imho.

a non-zero number of people are used to it, and look for the -fuse-ld option that is in the command just above that note. (Maybe it also depends on the gcc version and newer ones have improved it 🤔)

I dare say most Rust users are not used to it. I certainly was entirely flabbergasted, and I know more than the average Rust programmer about how the compiler works. If an error is too obscure for me I think we can safely assume that it is useless-or-worse for the majority of our users.

That should obviously be reported against gcc, I just didn't have the time for that. But in general we try to work around weaknesses of other tools we employ when we are aware of them.

But as you say indeed for this PR this isn't relevant. I was just replying to @petrochenkov's arguments.

That should obviously be reported against gcc, I just didn't have the time for that. But in general we try to work around weaknesses of other tools we employ when we are aware of them.

I think we could actually help with this issue in rustc by intervening into the linker error and emitting an extra note.
Like in #125417, but without retrying.

compiler/rustc_codegen_ssa/src/back/link.rs

compiler/rustc_session/src/session.rs

…ently located

petrochenkov · 2024-05-23T16:17:39Z

@bors r+

bors · 2024-05-23T16:17:41Z

📌 Commit d64a8bd has been approved by petrochenkov

It is now in the queue for this repository.

…llaumeGomez Rollup of 6 pull requests Successful merges: - rust-lang#125263 (rust-lld: fallback to rustc's sysroot if there's no path to the linker in the target sysroot) - rust-lang#125345 (rustc_codegen_llvm: add support for writing summary bitcode) - rust-lang#125362 (Actually use TAIT instead of emulating it) - rust-lang#125412 (Don't suggest adding the unexpected cfgs to the build-script it-self) - rust-lang#125445 (Migrate `run-make/rustdoc-with-short-out-dir-option` to `rmake.rs`) - rust-lang#125452 (Cleanup check-cfg handling in core and std) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#125263 - lqd:lld-fallback, r=petrochenkov rust-lld: fallback to rustc's sysroot if there's no path to the linker in the target sysroot As seen in rust-lang#125246, some sysroots don't expect to contain `rust-lld` and want to keep it that way, so we fallback to the default rustc sysroot if there is no path to the linker in any of the sysroot tools search paths. This is how we locate codegen-backends' dylibs already. People also have requested an error if none of these search paths contain the self-contained linker directory, so there's also an error in that case. r? `@petrochenkov` cc `@ehuss` `@RalfJung` I'm not sure where we check for `rust-lld`'s existence on the targets where we use it by default, and if we just ignore it when missing or emit a warning (as I assume we don't emit an error), so I just checked for the existence of `gcc-ld`, where `cc` will look for the lld-wrapper binaries. <sub>*Feel free to point out better ways to do this, it's the middle of the night here.*</sub> Fixes rust-lang#125246

rustbot assigned petrochenkov May 18, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 18, 2024

RalfJung mentioned this pull request May 19, 2024

rustc can't find the linker when --sysroot is set to a local build of the sysroot #125246

Closed

lqd mentioned this pull request May 21, 2024

Cross compilation fails on Rust nightly when using proc macro crates due to GCC not understanding -fuse-ld=lld #125330

Closed

add helper to target bin path

6867d64

lqd force-pushed the lld-fallback branch from f2611aa to 91cf429 Compare May 21, 2024 18:42

lqd force-pushed the lld-fallback branch 2 times, most recently from df12b88 to d8284a3 Compare May 21, 2024 18:57

lqd changed the title ~~rust-lld: fallback to the default linker if there's no path to the linker in the sysroot~~ rust-lld: fallback to rustc's sysroot if there's no path to the linker in the target sysroot May 21, 2024

petrochenkov reviewed May 22, 2024

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 22, 2024

petrochenkov reviewed May 23, 2024

View reviewed changes

compiler/rustc_codegen_ssa/src/back/link.rs Outdated Show resolved Hide resolved

compiler/rustc_codegen_ssa/src/back/link.rs Outdated Show resolved Hide resolved

compiler/rustc_session/src/session.rs Outdated Show resolved Hide resolved

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2024

lqd added 2 commits May 23, 2024 15:47

rust-lld: fallback to the default default sysroot where rustc is curr…

fab28f2

…ently located

emit an error if we can't find a path to the self-contained linker

d64a8bd

lqd force-pushed the lld-fallback branch from bac5344 to d64a8bd Compare May 23, 2024 16:11

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 23, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2024

GuillaumeGomez mentioned this pull request May 23, 2024

Rollup of 6 pull requests #125463

Merged

bors merged commit d6a1f1d into rust-lang:master May 24, 2024
6 checks passed

rustbot added this to the 1.80.0 milestone May 24, 2024

lqd deleted the lld-fallback branch May 24, 2024 06:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rust-lld: fallback to rustc's sysroot if there's no path to the linker in the target sysroot #125263

rust-lld: fallback to rustc's sysroot if there's no path to the linker in the target sysroot #125263

lqd commented May 18, 2024 •

edited

Loading

bjorn3 commented May 19, 2024

lqd commented May 21, 2024

bjorn3 commented May 21, 2024

Kobzol commented May 21, 2024 •

edited

Loading

RalfJung commented May 21, 2024

lqd commented May 21, 2024

Kobzol commented May 21, 2024

RalfJung commented May 21, 2024

lqd commented May 21, 2024 •

edited

Loading

RalfJung commented May 21, 2024 •

edited

Loading

bjorn3 commented May 21, 2024

lqd commented May 21, 2024 •

edited

Loading

lqd commented May 22, 2024

RalfJung commented May 22, 2024 •

edited

Loading

lqd commented May 22, 2024

petrochenkov May 22, 2024

petrochenkov May 22, 2024

lqd May 22, 2024

lqd May 22, 2024

RalfJung May 22, 2024

RalfJung May 22, 2024

lqd May 22, 2024 •

edited

Loading

RalfJung May 23, 2024

petrochenkov May 23, 2024

petrochenkov commented May 23, 2024

bors commented May 23, 2024

rust-lld: fallback to rustc's sysroot if there's no path to the linker in the target sysroot #125263

rust-lld: fallback to rustc's sysroot if there's no path to the linker in the target sysroot #125263

Conversation

lqd commented May 18, 2024 • edited Loading

bjorn3 commented May 19, 2024

lqd commented May 21, 2024

bjorn3 commented May 21, 2024

Kobzol commented May 21, 2024 • edited Loading

RalfJung commented May 21, 2024

lqd commented May 21, 2024

Kobzol commented May 21, 2024

RalfJung commented May 21, 2024

lqd commented May 21, 2024 • edited Loading

RalfJung commented May 21, 2024 • edited Loading

bjorn3 commented May 21, 2024

lqd commented May 21, 2024 • edited Loading

lqd commented May 22, 2024

RalfJung commented May 22, 2024 • edited Loading

lqd commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqd May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented May 23, 2024

bors commented May 23, 2024

lqd commented May 18, 2024 •

edited

Loading

Kobzol commented May 21, 2024 •

edited

Loading

lqd commented May 21, 2024 •

edited

Loading

RalfJung commented May 21, 2024 •

edited

Loading

lqd commented May 21, 2024 •

edited

Loading

RalfJung commented May 22, 2024 •

edited

Loading

lqd May 22, 2024 •

edited

Loading