Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stabilize -Zgcc-ld=lld as -Clink-self-contained=linker -Clinker-flavor=gcc-lld #510

Closed
1 of 3 tasks
lqd opened this issue Apr 14, 2022 · 16 comments
Closed
1 of 3 tasks
Labels
disposition-merge The FCP starter wants to merge this finished-final-comment-period The FCP has finished, action upon the disposition label needs to be taken major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@lqd
Copy link
Member

lqd commented Apr 14, 2022

Proposal

In the context of improvements to compile-times, we'd like to make progress on enabling LLD by default. Since this is a broad topic, with known issues and multiple stakeholders, the goal is to first focus on an achievable subset -- enabling LLD by default on linux to start -- and to make incremental progress towards that in multiple steps.

This MCP is proposing such a first step, stabilizing the -Zgcc-ld=lld flag to be able to use rust-lld (avoiding the known lld issues for now, allowing to focus on a smaller scope and make progress), and then other follow-up tasks towards achieving the goal.

The system's lld can already be used on stable, and we build and distribute rust-lld in rustup. Using rust-lld would alleviate the need to install lld, and possibly avoid some incompatibilities between the system version and the LLVM version used by rustc (or the need to keep them in sync). On some targets the wrappers and executable can be used directly, but it's not the case by default on linux, and that's where -Zgcc-ld=lld currently helps.

-Zgcc-ld=lld uses lld-wrappers to call rust-lld. They are compiled as ld and ld64 binaries, and distributed in a gcc-ld folder in the sysroot next to rust-lld. The flags makes sure the wrappers exist, and adds arguments to the Command used to do the linking.


Stabilizing -Zgcc-ld=lld as -Clink-self-contained=linker -Clinker-flavor=gcc-lld

The details of this proposal were discussed in this zulip thread and the discussion there has settled to stabilize a way to use rust-lld by splitting the 2 things that -Zgcc-ld=lld does more cleanly:

  • it requests using lld. We propose that this is done via a dedicated -C linker-flavor=gcc-lld option that would itself only add the -fuse-ld=lld link argument. (Or as a colon-separated gcc:lld, allowing for extensions such as passing anything after the : separator straight through -fuse-ld= and work out of the box for gold and mold if supported by the installed version of GCC)
  • it uses rust-lld instead, by passing the path needed to find the lld-wrappers within the sysroot. We propose this is done via a new option to an existing flag, a -Clink-self-contained=linker option. This flags currently only targets linking our CRT objects on a few targets.

Currently, the same enums are used internally to handle the CLI's codegen flags, by the target specs themselves, as well as all linking related code. This can cause issues of duplication, and be error-prone: it seems some lld flavor variants have been created for CLI use, but they must be handled the same way internally. We propose splitting the surface enums used for CLI, so that the changes to the flag values don't change the internal linking code or target specs.

Testing

Looking at the issues in the rust repo, I couldn't find some related to -Zgcc-ld=lld itself. There are a few about rust-lld, but on other targets.

I have tested enabling it on x86_64-unknown-linux-gnu with the 800 most popular crates on crates.io (most of them are libraries so linking and executing only happens for build scripts and proc macros) and a dozen popular binaries (cargo, ripgrep, nushell, tokei, etc) without obvious problems. It's possible there are still issues, in addition to the known issues about using lld in general: it's unclear whether this flag is well-known and used in the community, so a build-and-test crater run would be at least reassuring. I've opened PR 96025 to do a crater run with -Zgcc-ld=lld enabled.

Of note
  • distro builds don't bundle rust-lld, so -Clink-self-contained=linker should either be a no-op there, or produce a warning. This could be requested with a config.toml flag, either a new one or the existing rust.lld flag.
  • to allow for some experimentation and testing on nightly, the new flag values will be requiring -Z unstable-options in the beginning.

Follow-up tasks

These follow-up tasks could be next steps towards using rust-lld as the default linker on linux:

  1. Tracking known issues when using lld, finding ways to fix them or have workarounds.

There's an issue with stack traces generated when using perf, detected in flamegraph-rs. This one doesn't seem to be tracked in the rust or LLVM repositories. The impact in practice is still a bit unclear (if it's not limited to the flamegraph use-case), and it doesn't seem to affect e.g. cachegrind. It probably should at least be tracked in our issues, since it could be decided to be a blocker. It seems unlikely, as there is a workaround (the --no-rosegment link arg) and we could imagine using it by default when rust-lld is enabled.

There is one issue related to coverage on the musl target, but this could be avoided and fixed later, by focusing on enabling it only for x86_64-unknown-linux-gnu. This specific use-case would then use the existing default. (The workaround for the previous issue doesn't seem work in this case)

Having LLVM/LLD experts look at these would be good. They could know whether these are issues in lld or our use, their severity, etc.

  1. Likely publicize the new flag, so that users can try it out and report issues.

  2. Eventually, depending on the results of the previous two tasks, discussing switching the default to rust-lld.

Mentors or Reviewers

Maybe @petrochenkov, since they've reviewed the PR adding -Zgcc-ld ?

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@lqd lqd added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Apr 14, 2022
@rustbot
Copy link
Collaborator

rustbot commented Apr 14, 2022

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Apr 14, 2022
@lqd
Copy link
Member Author

lqd commented Apr 14, 2022

Notable people to cc:

@Kmeakin
Copy link

Kmeakin commented Apr 19, 2022

We (Arm) tested lld on aarch64 and found a few issues, but nothing arm-specific. It should be easy to adopt lld as the default on aarch64 if and when it is also adopted as the default for x86_64:

We ran a crater experiment building all crates from crates.io on aarch64-unknown-linux-gnu with -Clink-args=-fuse-ld=lld. In total, 2 crates failed to link with lld. We filed bug reports with the LLVM project (llvm/llvm-project#54700 and llvm/llvm-project#54701), but they were rejected as intended behavior. Neither issue was Aarch64 specific (both also failed on x86_64-unknown-linux-gnu). We will file issues against the crates that failed to fix their build.rs scripts.

@pnkfelix
Copy link
Member

pnkfelix commented Apr 19, 2022

Should issue rust-lang/rust#81408 block this proposed stabilization?

On the one hand, we strive to not let our own progress be blocked by LLVM upstream bugs (depending on what their severity and relative risk are), which is an argument for not letting #81408 block this.

But on the other hand rust-lang/rust#81408 is pretty subtle and its hard to put bounds on its potential impact.

@roblabla
Copy link

Should issue rust-lang/rust#81408 block this proposed stabilization?

It's already possible to hit rust-lang/rust#81408 on stable rust by specifying the rust-lld linker in the .cargo/config, so I don't think this issue should block stabilization. Stabilizing this feature will make the situation no better or worse.

But on the other hand rust-lang/rust#81408 is pretty subtle and its hard to put bounds on its potential impact.

At least in potential impact, it only affects windows (presumably due to a bug in the generation of thread-local-storage sections of PE/.exe files), and I was only able to trigger it with LTO enabled (ever since disabling LTO on my project, I haven't had any issue).

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 21, 2022
@pnkfelix
Copy link
Member

At least in potential impact, it only affects windows (presumably due to a bug in the generation of thread-local-storage sections of PE/.exe files), and I was only able to trigger it with LTO enabled (ever since disabling LTO on my project, I haven't had any issue).

Yeah, I don't doubt that its solely Windows + LTO.

The kinds of bounds I was talking about were things like "when this goes wrong, it will always manifest itself as an explicitly signaled runtime error" versus "the LTO thinks there's undefined behavior (even though, AFAICT, there isn't UB) and the optimizations performed imply that anything can happen."

Anyway I do think we're in agreement that we can mitigate risk here, e.g. by focusing attention on lld atop Linux.

@pnkfelix
Copy link
Member

pnkfelix commented May 6, 2022

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented May 6, 2022

Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP has been started, cast your votes and raise concerns disposition-merge The FCP starter wants to merge this labels May 6, 2022
@pnkfelix pnkfelix changed the title Stabilize -Zgcc-ld=lld Stabilize -Zgcc-ld=lld as -Clink-self-contained=linker -Clinker-flavor=gcc-lld May 6, 2022
@petrochenkov
Copy link

I'm on board with the high-level -Clink-self-contained=linker -Clinker-flavor=gcc-lld strategy, but I'd prefer to sign off on the stabilization only when it's actually implemented, reviewed in detail, and merged.

I'm in particularly interested in what can go after -Clink-self-contained and what meaning it has on different platforms.
I suspect that we may need to switch it to a +/- list like -Clink-self-contained=+foo,-bar similar to target features and linking modifiers.

@pnkfelix
Copy link
Member

pnkfelix commented May 6, 2022

Oh, certainly: We'd still have to have an FCP as part of the stabilization. An FCP merge at this stage doesn't change that.

(I was openly questioning in today's steering meeting about whether requiring an FCP at this stage makes sense, given that we will still need another FCP before stabilization. I hadn't considered the detail that doing an FCP now might make people think that another one wouldn't happen later...)

And also, I'm on board for a +/- style list, at least as an optional way to be very explicit about one's intent. We can discuss that on the zulip topic thread

@Firstyear
Copy link

Hey there, I'm the maintainer of Rust in OpenSUSE and SUSE, and we're really interested in this change. If you want assistance to test, let us know :)

@rfcbot rfcbot added final-comment-period The FCP has started, most (if not all) team members are in agreement and removed proposed-final-comment-period An FCP has been started, cast your votes and raise concerns labels Jul 21, 2022
@rfcbot
Copy link

rfcbot commented Jul 21, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The FCP has finished, action upon the disposition label needs to be taken and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Jul 31, 2022
@rfcbot
Copy link

rfcbot commented Jul 31, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jul 31, 2022
@bstrie
Copy link

bstrie commented Aug 9, 2022

Can this be merged?

@apiraino
Copy link
Contributor

apiraino commented May 4, 2023

@rustbot label -final-comment-period +major-change-accepted

(I think this was meant to be closed long ago)

@apiraino apiraino closed this as completed May 4, 2023
@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting labels May 4, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 4, 2023
@AlexTMjugador
Copy link

To make it easier for interested people to trace the mentioned unstable features, I'd like to mention that -Zgcc-ld=lld was subsequently removed on rust-lang/rust#116514.

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 9, 2023
…arsan68,petrochenkov

Generalize LLD usage in bootstrap

The current usage of using LLD (`rust.use-lld = true`) in bootstrap is a bit messy. What it claimed:

> Indicates whether LLD will be used to link Rust crates during bootstrap on
> supported platforms. The LLD from the bootstrap distribution will be used
> and not the LLD compiled during the bootstrap.

What it did:
1) On MSVC, it did indeed use the snapshot compiler's `rust-lld`, but at the same time it was invoking a global `lld` binary (since rust-lang#102101), therefore it wouldn't work if `lld` wasn't available.
2) On other targets, it was just straight up using a global `lld` linker. If it wasn't available, it would fail.

This PR (hopefully) cleans up handling of LLD in bootstrap. It introduces a new enum called `LldMode`, which explicitly distinguishes between no LLD, external LLD and self-contained LLD. Since it's non-trivial to provide a custom path to LLD, if an external `lld` is used, the linker binary has to be named exactly `lld` and it has to be available in PATH.

In addition, this PR also dog-foods [MCP510](rust-lang/compiler-team#510) in bootstrap.

To keep backwards compatibility somewhat, I kept the original `use-lld` flag and mapped the `true` value to `"external"`, which is how it behaved before on Linux and other non-MSVC targets.

Having the option to use an external `lld` on Linux should come in handy for testing on CI once MCP510 sets the default linker on Linux to `lld`.

Note that thanks to MCP510, currently "self-contained" means that `lld` is used from the stage N-1 compiler (before, we always used `lld` from the snapshot/stage0 compiler).

Best reviewed commit by commit.

CC `@petrochenkov`
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 10, 2023
…arsan68,petrochenkov

Generalize LLD usage in bootstrap

The current usage of using LLD (`rust.use-lld = true`) in bootstrap is a bit messy. What it claimed:

> Indicates whether LLD will be used to link Rust crates during bootstrap on
> supported platforms. The LLD from the bootstrap distribution will be used
> and not the LLD compiled during the bootstrap.

What it did:
1) On MSVC, it did indeed use the snapshot compiler's `rust-lld`, but at the same time it was invoking a global `lld` binary (since rust-lang#102101), therefore it wouldn't work if `lld` wasn't available.
2) On other targets, it was just straight up using a global `lld` linker. If it wasn't available, it would fail.

This PR (hopefully) cleans up handling of LLD in bootstrap. It introduces a new enum called `LldMode`, which explicitly distinguishes between no LLD, external LLD and self-contained LLD. Since it's non-trivial to provide a custom path to LLD, if an external `lld` is used, the linker binary has to be named exactly `lld` and it has to be available in PATH.

In addition, this PR also dog-foods [MCP510](rust-lang/compiler-team#510) in bootstrap.

To keep backwards compatibility somewhat, I kept the original `use-lld` flag and mapped the `true` value to `"external"`, which is how it behaved before on Linux and other non-MSVC targets.

Having the option to use an external `lld` on Linux should come in handy for testing on CI once MCP510 sets the default linker on Linux to `lld`.

Note that thanks to MCP510, currently "self-contained" means that `lld` is used from the stage N-1 compiler (before, we always used `lld` from the snapshot/stage0 compiler).

Best reviewed commit by commit.

CC `@petrochenkov`
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 10, 2023
…arsan68,petrochenkov

Generalize LLD usage in bootstrap

The current usage of using LLD (`rust.use-lld = true`) in bootstrap is a bit messy. What it claimed:

> Indicates whether LLD will be used to link Rust crates during bootstrap on
> supported platforms. The LLD from the bootstrap distribution will be used
> and not the LLD compiled during the bootstrap.

What it did:
1) On MSVC, it did indeed use the snapshot compiler's `rust-lld`, but at the same time it was invoking a global `lld` binary (since rust-lang#102101), therefore it wouldn't work if `lld` wasn't available.
2) On other targets, it was just straight up using a global `lld` linker. If it wasn't available, it would fail.

This PR (hopefully) cleans up handling of LLD in bootstrap. It introduces a new enum called `LldMode`, which explicitly distinguishes between no LLD, external LLD and self-contained LLD. Since it's non-trivial to provide a custom path to LLD, if an external `lld` is used, the linker binary has to be named exactly `lld` and it has to be available in PATH.

In addition, this PR also dog-foods [MCP510](rust-lang/compiler-team#510) in bootstrap.

To keep backwards compatibility somewhat, I kept the original `use-lld` flag and mapped the `true` value to `"external"`, which is how it behaved before on Linux and other non-MSVC targets.

Having the option to use an external `lld` on Linux should come in handy for testing on CI once MCP510 sets the default linker on Linux to `lld`.

Note that thanks to MCP510, currently "self-contained" means that `lld` is used from the stage N-1 compiler (before, we always used `lld` from the snapshot/stage0 compiler).

Best reviewed commit by commit.

CC `@petrochenkov`
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Dec 12, 2023
…etrochenkov

Generalize LLD usage in bootstrap

The current usage of using LLD (`rust.use-lld = true`) in bootstrap is a bit messy. What it claimed:

> Indicates whether LLD will be used to link Rust crates during bootstrap on
> supported platforms. The LLD from the bootstrap distribution will be used
> and not the LLD compiled during the bootstrap.

What it did:
1) On MSVC, it did indeed use the snapshot compiler's `rust-lld`, but at the same time it was invoking a global `lld` binary (since rust-lang/rust#102101), therefore it wouldn't work if `lld` wasn't available.
2) On other targets, it was just straight up using a global `lld` linker. If it wasn't available, it would fail.

This PR (hopefully) cleans up handling of LLD in bootstrap. It introduces a new enum called `LldMode`, which explicitly distinguishes between no LLD, external LLD and self-contained LLD. Since it's non-trivial to provide a custom path to LLD, if an external `lld` is used, the linker binary has to be named exactly `lld` and it has to be available in PATH.

In addition, this PR also dog-foods [MCP510](rust-lang/compiler-team#510) in bootstrap.

To keep backwards compatibility somewhat, I kept the original `use-lld` flag and mapped the `true` value to `"external"`, which is how it behaved before on Linux and other non-MSVC targets.

Having the option to use an external `lld` on Linux should come in handy for testing on CI once MCP510 sets the default linker on Linux to `lld`.

Note that thanks to MCP510, currently "self-contained" means that `lld` is used from the stage N-1 compiler (before, we always used `lld` from the snapshot/stage0 compiler).

Best reviewed commit by commit.

CC `@petrochenkov`
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
…etrochenkov

Generalize LLD usage in bootstrap

The current usage of using LLD (`rust.use-lld = true`) in bootstrap is a bit messy. What it claimed:

> Indicates whether LLD will be used to link Rust crates during bootstrap on
> supported platforms. The LLD from the bootstrap distribution will be used
> and not the LLD compiled during the bootstrap.

What it did:
1) On MSVC, it did indeed use the snapshot compiler's `rust-lld`, but at the same time it was invoking a global `lld` binary (since rust-lang/rust#102101), therefore it wouldn't work if `lld` wasn't available.
2) On other targets, it was just straight up using a global `lld` linker. If it wasn't available, it would fail.

This PR (hopefully) cleans up handling of LLD in bootstrap. It introduces a new enum called `LldMode`, which explicitly distinguishes between no LLD, external LLD and self-contained LLD. Since it's non-trivial to provide a custom path to LLD, if an external `lld` is used, the linker binary has to be named exactly `lld` and it has to be available in PATH.

In addition, this PR also dog-foods [MCP510](rust-lang/compiler-team#510) in bootstrap.

To keep backwards compatibility somewhat, I kept the original `use-lld` flag and mapped the `true` value to `"external"`, which is how it behaved before on Linux and other non-MSVC targets.

Having the option to use an external `lld` on Linux should come in handy for testing on CI once MCP510 sets the default linker on Linux to `lld`.

Note that thanks to MCP510, currently "self-contained" means that `lld` is used from the stage N-1 compiler (before, we always used `lld` from the snapshot/stage0 compiler).

Best reviewed commit by commit.

CC `@petrochenkov`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
…etrochenkov

Generalize LLD usage in bootstrap

The current usage of using LLD (`rust.use-lld = true`) in bootstrap is a bit messy. What it claimed:

> Indicates whether LLD will be used to link Rust crates during bootstrap on
> supported platforms. The LLD from the bootstrap distribution will be used
> and not the LLD compiled during the bootstrap.

What it did:
1) On MSVC, it did indeed use the snapshot compiler's `rust-lld`, but at the same time it was invoking a global `lld` binary (since rust-lang/rust#102101), therefore it wouldn't work if `lld` wasn't available.
2) On other targets, it was just straight up using a global `lld` linker. If it wasn't available, it would fail.

This PR (hopefully) cleans up handling of LLD in bootstrap. It introduces a new enum called `LldMode`, which explicitly distinguishes between no LLD, external LLD and self-contained LLD. Since it's non-trivial to provide a custom path to LLD, if an external `lld` is used, the linker binary has to be named exactly `lld` and it has to be available in PATH.

In addition, this PR also dog-foods [MCP510](rust-lang/compiler-team#510) in bootstrap.

To keep backwards compatibility somewhat, I kept the original `use-lld` flag and mapped the `true` value to `"external"`, which is how it behaved before on Linux and other non-MSVC targets.

Having the option to use an external `lld` on Linux should come in handy for testing on CI once MCP510 sets the default linker on Linux to `lld`.

Note that thanks to MCP510, currently "self-contained" means that `lld` is used from the stage N-1 compiler (before, we always used `lld` from the snapshot/stage0 compiler).

Best reviewed commit by commit.

CC `@petrochenkov`
Erk- added a commit to twilight-rs/http-proxy that referenced this issue Jun 15, 2024
-Zgcc-lld was stabilized in
rust-lang/compiler-team#510 under a
different name.
Erk- added a commit to twilight-rs/http-proxy that referenced this issue Jun 15, 2024
-Zgcc-lld was stabilized in
rust-lang/compiler-team#510 under a
different name.
Erk- added a commit to twilight-rs/http-proxy that referenced this issue Jun 15, 2024
-Zgcc-ld=lld was stabilized in
rust-lang/compiler-team#510 under a
different name.
Erk- added a commit to twilight-rs/http-proxy that referenced this issue Jun 15, 2024
* Don't run CI twice in PR's

* fix ci after change in nightly

-Zgcc-ld=lld was stabilized in
rust-lang/compiler-team#510 under a
different name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge The FCP starter wants to merge this finished-final-comment-period The FCP has finished, action upon the disposition label needs to be taken major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests