Skip to content

Allow using self-contained LLD in bootstrap #135001

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 4, 2025
+14 −2
Merged

Allow using self-contained LLD in bootstrap #135001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab5446c
Actually use self-contained lld in bootstrap when `use-lld = "self-co…
Kobzol Jan 1, 2025
3d69dd1
Do not pass LLD flags to targets flags of compiletest
Kobzol Jan 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/bootstrap/src/core/build_steps/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1901,7 +1901,6 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the

let mut targetflags = flags;
targetflags.push(format!("-Lnative={}", builder.test_helpers_out(target).display()));
targetflags.extend(linker_flags(builder, compiler.host, LldThreads::No));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we design linker_flags in a way that preserves the old logic for tests instead of removing it entirely?

Copy link
Contributor Author

@Kobzol Kobzol Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw only very minimal gains with LLD vs the default Linux linker on UI tests, but it's true that on some other platforms the difference might be larger.

The problem with the old approach is that it cannot respect the "self-contained" mode. So when you use lld = "self-contained", the old approach ignores that and just uses the global lld for the tests.

I guess that a compromise could be to use the linker flag with use-lld = true and the new compiler flag with use-lld = "self-contained", but I would eventually like to set use-lld = "self-contained" to be the default on CI; with this approach, some tests would simply fail..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that a compromise could be to use the linker flag with use-lld = true and the new compiler flag with use-lld = "self-contained", but I would eventually like to set use-lld = "self-contained" to be the default on CI; with this approach, some tests would simply fail..

This sounds reasonable I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there's still the "some tests would fail" part 😆 Another alternative would be to actually modify the target configuration of the compiler when use-lld is enabled, but this means that the compiler itself would be modified and would use lld by default; right now, we just enable lld in a hacky way in bootstrap.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You said "eventually", I am not sure why we should concern on that part right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not just about CI. Right now, use-lld is quite useful to make local compiler builds faster. But if we actually make self-contained working as intended, and keep the compiletest target flags, then use-lld = "self-contained" will also cause UI tests to fail for people that use this config value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. In this case I am fine with landing it as is. We can create an issue to track that part.

for flag in targetflags {
cmd.arg("--target-rustcflags").arg(flag);
}
Expand Down
15 changes: 14 additions & 1 deletion src/bootstrap/src/utils/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,20 @@ pub fn linker_flags(
) -> Vec<String> {
let mut args = vec![];
if !builder.is_lld_direct_linker(target) && builder.config.lld_mode.is_used() {
args.push(String::from("-Clink-arg=-fuse-ld=lld"));
match builder.config.lld_mode {
LldMode::External => {
args.push("-Clinker-flavor=gnu-lld-cc".to_string());
// FIXME(kobzol): remove this flag once MCP510 gets stabilized
args.push("-Zunstable-options".to_string());
}
LldMode::SelfContained => {
args.push("-Clinker-flavor=gnu-lld-cc".to_string());
args.push("-Clink-self-contained=+linker".to_string());
// FIXME(kobzol): remove this flag once MCP510 gets stabilized
args.push("-Zunstable-options".to_string());
}
LldMode::Unused => unreachable!(),
};

if matches!(lld_threads, LldThreads::No) {
args.push(format!(
Expand Down
Loading