Skip to content

Use target-agnostic LLD flags in bootstrap for use-lld #139378

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

Merged
merged 1 commit into from
Apr 5, 2025

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Apr 4, 2025

Before, I hardcoded LLD flags that pretty much only worked on GNU. The right way is to use -Zlinker-features instead though.

I think that this should also make this work on Windows mingw, and thus @petrochenkov's workaround is no longer necessary.

Fixes: #139372
Closes: #139375

r? @lqd

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 4, 2025
@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 4, 2025

After this fix, we might want to revert 3d69dd1, although I'm not sure if it was a good idea to pass LLD flags to compiletest to use for compiling the test programs in the first place...

@rust-log-analyzer

This comment has been minimized.

@Kobzol Kobzol changed the title Use target agnostic LLD flags in bootstrap for use-lld Use target-agnostic LLD flags in bootstrap for use-lld Apr 4, 2025
@petrochenkov
Copy link
Contributor

although I'm not sure if it was a good idea to pass LLD flags to compiletest to use for compiling the test programs in the first place...

Compiletest builds thousands of small programs, for which linking is a large part of build time, LLD was supposed to speedup that process.

@petrochenkov
Copy link
Contributor

I think that this should also make this work on Windows mingw, and thus @petrochenkov's workaround is no longer necessary.

No, -Zlinker-features=+lld will still trigger the incorrect response file escaping mode in bootstrap rustc.
But the workaround should go away in a few days? The new stable rustc was released, so the bootstrap compiler is also going to be bumped soon.

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 4, 2025

We didn't find any perf. differences on Linux in #135001 though (AFAIK, compiletest should use targetflags even when host == target, unless the test specifies //@ force-host). But maybe there are larger differences on other platforms?

No, -Zlinker-features=+lld will still trigger the incorrect response file escaping mode in bootstrap rustc.
But the workaround should go away in a few days? The new stable rustc was released, so the bootstrap compiler is also going to be bumped soon.

Ok, in that case I will keep the workaround, and based on what gets merged first we'll resolve the merge conflict.

@Kobzol Kobzol force-pushed the bootstrap-use-lld-fix branch from a09620b to 5297c89 Compare April 4, 2025 16:28
@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 4, 2025
@rustbot

This comment has been minimized.

@Kobzol Kobzol force-pushed the bootstrap-use-lld-fix branch from 5297c89 to 76e1302 Compare April 4, 2025 16:29
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 4, 2025
@petrochenkov
Copy link
Contributor

We didn't find any perf. differences on Linux in #135001 though (AFAIK, compiletest should use targetflags even when host == target, unless the test specifies //@ force-host). But maybe there are larger differences on other platforms?

I'll try to re-measure (on windows-gnu).

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 4, 2025

r? @petrochenkov

@rustbot rustbot assigned petrochenkov and unassigned lqd Apr 4, 2025
@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 4, 2025

📌 Commit 76e1302 has been approved by petrochenkov

It is now in the queue for this repository.

@bors 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 Apr 4, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 4, 2025
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#139041 (Remove `rustc_middle::ty::util::ExplicitSelf`.)
 - rust-lang#139328 (Fix 2024 edition doctest panic output)
 - rust-lang#139339 (unstable book: document tait)
 - rust-lang#139348 (AsyncDestructor: replace fields with impl_did)
 - rust-lang#139353 (Fix `Debug` impl for `LateParamRegionKind`.)
 - rust-lang#139366 (ToSocketAddrs: fix typo)
 - rust-lang#139374 (Use the span of the whole bound when the diagnostic talks about a bound)
 - rust-lang#139378 (Use target-agnostic LLD flags in bootstrap for `use-lld`)
 - rust-lang#139384 (Add `compiletest` adhoc_group for `r? compiletest`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bad6b7b into rust-lang:master Apr 5, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 5, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 5, 2025
Rollup merge of rust-lang#139378 - Kobzol:bootstrap-use-lld-fix, r=petrochenkov

Use target-agnostic LLD flags in bootstrap for `use-lld`

[Before](rust-lang#135001), I hardcoded LLD flags that pretty much only worked on GNU. The right way is to use `-Zlinker-features` instead though.

I *think* that this should also make this work on Windows mingw, and thus `@petrochenkov's` workaround is no longer necessary.

Fixes: rust-lang#139372
Closes: rust-lang#139375

r? `@lqd`
@Kobzol Kobzol deleted the bootstrap-use-lld-fix branch April 5, 2025 06:11
@petrochenkov
Copy link
Contributor

We didn't find any perf. differences on Linux in #135001 though (AFAIK, compiletest should use targetflags even when host == target, unless the test specifies //@ force-host). But maybe there are larger differences on other platforms?

I'll try to re-measure (on windows-gnu).

time ./x t tests/ui on x86_64-pc-windows-gnu.
Without lld:

2m25.104s
2m17.629s
2m15.590s

With lld:

2m21.616s
2m19.646s
2m17.501s

So pretty much the same.
Also, 27 tests are failing with lld now.

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 6, 2025

Thanks for the benchmark! Yeah, it seems like for the tests it doesn't really matter. use-lld is quite a bit faster on Linux for rebuilding the compiler itself though.

Could you please post the test failures? I tried it on Linux and I think that everything succeeded there.

@petrochenkov
Copy link
Contributor

Could you please post the test failures? I tried it on Linux and I think that everything succeeded there.

    [ui] tests\ui\debuginfo\windows_gnu_split_debuginfo_packed.rs#i686_g
    [ui] tests\ui\debuginfo\windows_gnu_split_debuginfo_packed.rs#i686_gl
    [ui] tests\ui\debuginfo\windows_gnu_split_debuginfo_packed.rs#aarch64_gl
    [ui] tests\ui\debuginfo\windows_gnu_split_debuginfo_packed.rs#x86_64_g
    [ui] tests\ui\debuginfo\windows_gnu_split_debuginfo_packed.rs#x86_64_gl
    [ui] tests\ui\debuginfo\windows_gnu_split_debuginfo_packed.rs#i686_uwp_g
    [ui] tests\ui\debuginfo\windows_gnu_split_debuginfo_packed.rs#x86_64_uwp_g
    [ui] tests\ui\debuginfo\windows_gnu_split_debuginfo_unpacked.rs#i686_g
    [ui] tests\ui\debuginfo\windows_gnu_split_debuginfo_unpacked.rs#aarch64_gl
    [ui] tests\ui\debuginfo\windows_gnu_split_debuginfo_unpacked.rs#i686_gl
    [ui] tests\ui\debuginfo\windows_gnu_split_debuginfo_unpacked.rs#i686_uwp_g
    [ui] tests\ui\debuginfo\windows_gnu_split_debuginfo_unpacked.rs#x86_64_g
    [ui] tests\ui\debuginfo\windows_gnu_split_debuginfo_unpacked.rs#x86_64_gl
    [ui] tests\ui\debuginfo\windows_gnu_split_debuginfo_unpacked.rs#x86_64_uwp_g
    [ui] tests\ui\feature-gates\env-flag.rs
    [ui] tests\ui\feature-gates\feature-gate-native_link_modifiers_as_needed.rs#in_flag
    [ui] tests\ui\linkage-attr\unstable-flavor.rs#bpf
    [ui] tests\ui\linkage-attr\unstable-flavor.rs#ptx
    [ui] tests\ui\linkage-attr\issue-10755.rs
    [ui] tests\ui\linkage-attr\linkage-attr-does-not-panic-llvm-issue-33992.rs
    [ui] tests\ui\print-request\stability.rs#check_cfg
    [ui] tests\ui\print-request\stability.rs#all_target_specs_json
    [ui] tests\ui\print-request\stability.rs#crate_root_lint_levels
    [ui] tests\ui\print-request\stability.rs#supported_crate_types
    [ui] tests\ui\print-request\stability.rs#target_spec_json
    [ui] tests\ui\symbol-mangling-version\unstable.rs#hashed
    [ui] tests\ui\symbol-mangling-version\unstable.rs#legacy

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 8, 2025

Thank you. All of these are either green or ignored on x64 Linux, so that should be fine regarding LLD on x64.

@lqd
Copy link
Member

lqd commented Apr 8, 2025

Some of these seem surprising to fail for linker reasons, apart from a failure about lld itself (but then I don't understand why it's on so few tests). What's the link between the linker and tests\ui\linkage-attr\unstable-flavor.rs#ptx for example? How's windows-gnu different from linux in that regard compared to a gcc + -fuse-ld 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to build wasm32-unknown-unknown target with use-lld=true
6 participants