-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Generalize LLD usage in bootstrap #116278
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp. |
#116276 has been merged, so I could try using MCP510 instead of the linker flags directly in this PR. @petrochenkov let me know if you want me to do that, or if we should first redesign LLD usage while keeping the old flags (this PR), and then try to move it to MCP510 to dogfood it in a later PR. |
If it simplifies the logic, rather than complicates it, then let's maybe do it in this PR. |
3d50aee
to
72e5ff2
Compare
This PR changes This PR changes |
I switched it to MCP510. Seems to work for me locally, and it indeed simplifies the code. |
This comment has been minimized.
This comment has been minimized.
What's the difference between Is it that this only affects "host" binaries, and not "target" ones which happen to be built for the same target as the host? |
The difference is that setting the linker to lld on other targets than MSVC doesn't work :( If you want to use lld on MSVC, you use lld as the linker. |
Oh, we're using clang/clang++ as the linker but yep that checks out. |
Yeah, sorry, I meant some compiler driver binary in general. |
I spoke to @onur-ozkan and it looks like it might not be so simple to reuse the code between the |
…ozkan,petrochenkov Pass rustc shim flags using environment variable This PR implements a generalized way of passing of host flags to the `rustc` shim in bootstrap, as proposed [here](rust-lang#116278 (comment)). I tried to implement the bootstrap side using `OsString`, but then I realized that the shim code was using `env::var` before anyway, instead of `env::var_os`, so I just settled on a `String`. The shim side is still general and uses `env::vars_os` now. I'm not sure if we actually need to do something with the `rustdoc` shim. It *seems* to me that the env. vars passed to it (`RUSTDOC_LINKER`) and (`RUSTDOC_LLD_NO_THREADS`) could just be passed to cargo directly (or rather, the commands that they invoke in the shim could be passed directly). I'm not sure why are they set by the shim. r? `@onur-ozkan` CC `@petrochenkov`
…ozkan,petrochenkov Pass rustc shim flags using environment variable This PR implements a generalized way of passing of host flags to the `rustc` shim in bootstrap, as proposed [here](rust-lang#116278 (comment)). I tried to implement the bootstrap side using `OsString`, but then I realized that the shim code was using `env::var` before anyway, instead of `env::var_os`, so I just settled on a `String`. The shim side is still general and uses `env::vars_os` now. I'm not sure if we actually need to do something with the `rustdoc` shim. It *seems* to me that the env. vars passed to it (`RUSTDOC_LINKER`) and (`RUSTDOC_LLD_NO_THREADS`) could just be passed to cargo directly (or rather, the commands that they invoke in the shim could be passed directly). I'm not sure why are they set by the shim. r? `@onur-ozkan` CC `@petrochenkov`
This comment was marked as resolved.
This comment was marked as resolved.
…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`
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
d8a8486
to
53031b2
Compare
Sigh, unclosed HTML tag in documentation. Next try. @bors r=albertlarsan68,petrochenkov |
☀️ Test successful - checks-actions |
Finished benchmarking commit (befd1eb): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 668.621s -> 668.664s (0.01%) |
if matches!(lld_threads, LldThreads::No) { | ||
args.push(format!( | ||
"-Clink-arg=-Wl,{}", | ||
lld_flag_no_threads(builder.config.lld_mode, target.is_msvc()) |
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.
lld_flag_no_threads(builder.config.lld_mode, target.is_msvc()) | |
lld_flag_no_threads(builder.config.lld_mode, target.contains("windows")) |
@Kobzol
This change broke the build on windows-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.
Sorry for that, I missed that during one of the rebases. #118906 should fix it.
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.
I don't think it will work the way you expect it to work.
LLD in MinGW mode will accept --threads
not /threads
in version 18: llvm/llvm-project@0b51e64#diff-65b7b7830ff6769240f3cd2cb084e19282d9c1b9d70ffceaf7e87a7e15721151R376
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 is just about keeping the previous behaviour. If LLD on MinGW changes it's behaviour, we need to modify bootstrap once we switch to LLVM 18 (or rather once people start using LLD 18 with bootstrap on MinGW)
…rochenkov Fix LLD thread flags in bootstrap on Windows Fixes [this comment](rust-lang#116278 (comment)). r? `@petrochenkov`
Rollup merge of rust-lang#118906 - Kobzol:bootstrap-is-windows, r=petrochenkov Fix LLD thread flags in bootstrap on Windows Fixes [this comment](rust-lang#116278 (comment)). r? `@petrochenkov`
Update Rust toolchain from nightly-2023-12-10 to nightly-2023-12-11 without any other source changes. This is an automatically generated pull request. If any of the CI checks fail, manual intervention is required. In such a case, review the changes at https://github.com/rust-lang/rust from rust-lang@06e02d5 up to rust-lang@d86d65b. The log for this commit range is: rust-lang@d86d65bbc1 Auto merge of rust-lang#118368 - GuillaumeGomez:env-flag, r=Nilstrieb rust-lang@ec4176167b Auto merge of rust-lang#118703 - Kobzol:bootstrap-config-unused, r=onur-ozkan rust-lang@f1c5558edc Add ChangeInfo record rust-lang@ccbd88dc83 Remove unused run_dsymutil and gpg_password_file config values rust-lang@6badc0d840 Destructure TOML configs rust-lang@7e452c123c Auto merge of rust-lang#118791 - saethlin:use-immediate-type, r=nikic rust-lang@b9068315db Auto merge of rust-lang#116952 - compiler-errors:lifetime_capture_rules_2024, r=TaKO8Ki rust-lang@befd1eb4ec Auto merge of rust-lang#116278 - Kobzol:bootstrap-lld-mode, r=albertlarsan68,petrochenkov rust-lang@dc2f77aad6 Add (unstable) documentation for `--env` command line option rust-lang@d2b1f94f05 Add feature gate test for `--env` flag rust-lang@37d68093da Add ui tests for `--env` option rust-lang@486e55e547 Implement `--env` compiler flag rust-lang@53031b264e Review fixes rust-lang@84f6130fe3 Auto merge of rust-lang#118692 - surechen:remove_unused_imports, r=petrochenkov rust-lang@4750e9de47 Produce an explicit error when using a self-contained lld, but we don't add it to sysroot rust-lang@ea769dbeb7 Add change tracker entry rust-lang@cbfe6327a1 Do not invoke external lld to figure out thread flags in self-contained mode rust-lang@50865745e1 Update `config.example.toml` rust-lang@40c3d351ad Use MCP510 rust-lang@48c1607bc6 Introduce `LldMode` and generalize parsing of `use-lld` rust-lang@d9f9e67bc1 Refactor rust(do)c linker flags rust-lang@b3c9ffdc77 Add `is_msvc` function rust-lang@c1a3919378 Auto merge of rust-lang#118792 - naglis:fix-mutex-doc-typo, r=workingjubilee rust-lang@42dfac5e08 Auto merge of rust-lang#118788 - compiler-errors:const-pretty, r=fee1-dead rust-lang@61afc9c928 Auto merge of rust-lang#116949 - hamza1311:stablize-arc_unwrap_or_clone, r=dtolnay rust-lang@c71c246876 Auto merge of rust-lang#118550 - cjgillot:filecheck-const-prop, r=Mark-Simulacrum rust-lang@40ae34194c remove redundant imports rust-lang@f7253f2317 Auto merge of rust-lang#118787 - GuillaumeGomez:rollup-fj5wr3q, r=GuillaumeGomez rust-lang@7d50a39763 Fix typo in `std::sync::Mutex` example rust-lang@8cd8d31369 Auto merge of rust-lang#118069 - onur-ozkan:bypass_bootstrap_lock, r=Mark-Simulacrum rust-lang@b0a580112b Use immediate_backend_type when reading from a const alloc rust-lang@afa35e90ef Print constness in TraitPredPrintModifiersAndPath rust-lang@7467c3a45c s/const_effect/host_effect rust-lang@f1bf874fb1 Don't print host effect param in pretty path_generic_args rust-lang@6860654d82 allow bypassing the build directory lock rust-lang@dd234696ed Rollup merge of rust-lang#118782 - jyn514:powershell, r=ChrisDenton rust-lang@034d73d6d7 Rollup merge of rust-lang#118775 - Young-Flash:fix, r=compiler-errors rust-lang@5b9e917b64 Rollup merge of rust-lang#118774 - lcnr:region-refactor-uwu, r=compiler-errors rust-lang@cc821d3ae6 Rollup merge of rust-lang#118747 - Urgau:check-cfg-freebsd-cleanup, r=onur-ozkan rust-lang@83e814f88c Rollup merge of rust-lang#117966 - lxy19980601:safe_compilation_options, r=Mark-Simulacrum rust-lang@2cf54e9f99 use `&` instead of start-process in x.ps1 rust-lang@ef796db5f0 add test for inductive cycle hangs rust-lang@cb6984217f chore: add test case for type with generic rust-lang@0f40b6545d Remove extra check-cfg handled by libc directly rust-lang@803772e81d Enable new capture rules by default on edition 2024 rust-lang@acba7efe1b Add test for implicitly capturing late-bound var with new capture rules rust-lang@0ad160a585 Add lifetime_capture_rules_2024 rust-lang@3f8487a099 Add safe compilation options rust-lang@30a95b7c0a FileCheck while_let_loops. rust-lang@c00068e49f FileCheck tuple_literal_propagation. rust-lang@87522d0007 FileCheck return_place. rust-lang@a12027e128 FileCheck switch_int. rust-lang@19767eb7a6 FileCheck slice_len. rust-lang@3e90c1b434 FileCheck scalar_literal_propagation. rust-lang@f3743aec51 FileCheck repeat. rust-lang@343ef6a9cb FileCheck reify_fn_ptr. rust-lang@6baec3ccc2 FileCheck ref_deref. rust-lang@c8c9207e4c FileCheck read_immutable_static. rust-lang@45dd5d6bf3 FileCheck mutable_variable_unprop_assign. rust-lang@6a8eea8f5b FileCheck mutable_variable_aggregate_partial_read. rust-lang@d91bb5074e FileCheck mutable_variable_no_prop. rust-lang@3e169abc1b FileCheck mutable_variable_aggregate_mut_ref. rust-lang@03c5ad1549 FileCheck mutable_variable_aggregate. rust-lang@ea9f968333 FileCheck mutable_variable. rust-lang@902a3e2e75 FileCheck mult_by_zero. rust-lang@8e9b912c4c FileCheck issue_67019. rust-lang@ce9b1e23a5 FileCheck issue_66971. rust-lang@218d8ccf43 FileCheck inherit_overflow. rust-lang@6086dd6766 FileCheck indirect. rust-lang@bf5d114da8 FileCheck discriminant. rust-lang@043d29b58a FileCheck and rename const_prop_fails_gracefully. rust-lang@7f328d2a44 FileCheck checked_add. rust-lang@e6a1b77cd1 FileCheck cast. rust-lang@b8f2f63931 FileCheck boxes. rust-lang@3fc03948a8 FileCheck boolean_identities. rust-lang@e8e35c8127 FileCheck bad_op_unsafe_oob_for_slices. rust-lang@97f03cb898 FileCheck bad_op_mod_by_zero. rust-lang@0d5bc872a9 FileCheck bad_op_div_by_zero. rust-lang@9f01d9d1b6 FileCheck array_index. rust-lang@6564bac532 FileCheck aggregate. rust-lang@378abbc604 FileCheck address_of_pair. rust-lang@540921e468 Stablize arc_unwrap_or_clone Co-authored-by: celinval <[email protected]>
The current usage of using LLD (
rust.use-lld = true
) in bootstrap is a bit messy. What it claimed:What it did:
rust-lld
, but at the same time it was invoking a globallld
binary (since check lld version to choose correct option to disable multi-threading in tests #102101), therefore it wouldn't work iflld
wasn't available.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 externallld
is used, the linker binary has to be named exactlylld
and it has to be available in PATH.In addition, this PR also dog-foods MCP510 in bootstrap.
To keep backwards compatibility somewhat, I kept the original
use-lld
flag and mapped thetrue
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 tolld
.Note that thanks to MCP510, currently "self-contained" means that
lld
is used from the stage N-1 compiler (before, we always usedlld
from the snapshot/stage0 compiler).Best reviewed commit by commit.
If you're coming here for the
change-id
change:lld
installed as a global program, useuse-lld = true
oruse-lld = "external"
lld
, useuse-lld = "self-contained"
CC @petrochenkov