-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Generalize configuring LLD as the default linker in bootstrap #147157
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
base: master
Are you sure you want to change the base?
Conversation
This PR modifies If appropriate, please update This PR modifies If appropriate, please update These commits modify compiler targets. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was supposed to be linux_gnu.rs
, not linux_musl.rs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, the files are too similar 😆 I actually wanted to put this into linux.rs
directly, as the override should also work for MUSL targets. Unless you think we should only allow it for linux-gnu
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, but if we put it into linux.rs
, then musl would override link_self_contained: LinkSelfContainedDefault::InferredForMusl
, that's not ideal. Ideally we should use the override at the very end of the config, not at the beginning, I guess.
9ed9ef8
to
1d1b183
Compare
Do you mean this is disabling the lld default for x64 linux and contributors have to change their local configs to keep it? |
Currently yes. I realize now that this is probably not a good idea, as we want to have as much coverage of LLD as possible. Then what I would do it configure this to be enabled by default in bootstrap for this specific target (either in code or bootstrap.toml default profiles), and then allow distros to opt-out. But enabling it by default would require us to also set rust.lld to true, or only override rust-lld when rust.lld is enabled, which is essentially what happened before and what I wanted to get rid off =D Ok then I would propose:
Thus essentially the other way around than it was before. |
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Updated the PR to apply the override by default, with the possibility to opt out. Updated the PR descripton. |
This PR generalizes the previous hardcoded logic for setting self-contained LLD to be used as the default Linker on Linux GNU targets. The changes made:
rust.lld
now only controls whether LLD is built and included in the sysroot. Ifrust.lld
is set tofalse
, it will disable the default opt into LLD on x64 Linux.rust-lld
throughcc
can now be applied to all Linux targets. It is configured throughtarget.<target>.default-linker-linux-override = "self-contained-lld-cc"/"off"
Here is how it behaves:
default-linker-linux-override = "self-contained-lld-cc"
and alsorust.lld = true
.rust.lld = false
, you disable this overridetarget.x86_64-unknown-linux-gnu.default-linux-linker-override = "off"
, you disable this overridetarget.x86_64-unknown-linux-gnu.llvm-config = ...
, you disable this overrideNote that in contrast to what I described in #146640, I didn't bother moving
rust.lld
tollvm.lld
. We can do that separately later, but I don't think it's worth the churn. I realized thatrust
is perhaps a better place for this, because it not only controls whether LLD is built, but also if it is included in the compiler sysroot. So to truly distinguish these two, we'd need bothllvm.lld
andrust.lld
, which doesn't seem worth it. Note thatrust.codegen-backends = ["llvm"]
works in the same way, it tells bootstrap to build LLVM and include it in the sysroot. So it is consistent withrust.lld
Related to: #146640
r? @jieyouxu