Make [rust] use-lld=true work on windows
#100464
Conversation
rustbot
added
the
T-bootstrap
|
r? @jyn514 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
src/bootstrap/compile.rs
Outdated
| // is already on by default in Windows optimized builds, which is interpreted as --icf=all: | ||
| // https://github.com/llvm/llvm-project/blob/3329cec2f79185bafd678f310fafadba2a8c76d2/lld/COFF/Driver.cpp#L1746 | ||
| // https://github.com/rust-lang/rust/blob/f22819bcce4abaff7d1246a56eec493418f9f4ee/compiler/rustc_codegen_ssa/src/back/linker.rs#L827 | ||
| #[cfg(not(windows))] |
There was a problem hiding this comment.
This is the platform of the host we are building on. But it's possible to cross compile from Unix to Windows, in which case I think we still use the MSVC toolchain.
Also, I think this needs to only special case MSVC - I think pc-windows-gnu should support these flags (@workingjubilee @Kobzol can you confirm?).
As a general rule, the only platform-specific cfg code in bootstrap should be related to filesystem stuff, everything else should be based off compiler.target.
There was a problem hiding this comment.
Ah right, that makes sense!
I am very unsure of myself, but I think that I should be checking compiler.host.contains("msvc") rather than target.contains("msvc") - the comment at the top of this function says "This will build the compiler for a particular stage of the build using the compiler targeting the target architecture", so I think we should be using the flag format for compiler rather than target. Let me know if I'm wrong!
There was a problem hiding this comment.
so, you're right that the distinction matters, but unfortunately bootstrap is really bad at naming things 😅 and compiler is "the compiler building Rustc", not "the compiler generated by the Rustc step". so target is correct.
If it helps, I double-checked this for myself by looking at what platform we pass to cargo, and it is target, not compiler.host:
Line 650 in dcead65
There was a problem hiding this comment.
ok it turns out you are correct after all, we use the linker of the host and not the target. sorry for the confusion.
jyn514
added
S-waiting-on-author
Before, it would fail with "error: ignoring unknown argument '-Wl,--icf=all'"
khyperia
force-pushed
the
lld-icf-on-windows
branch
from
c5fce50
to
dcead65
Compare
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
…mpiler-errors Rollup of 11 pull requests Successful merges: - rust-lang#100355 (rustdoc: Rename ``@has` FILE PATTERN` to ``@hasraw` FILE PATTERN`) - rust-lang#100407 (avoid some int2ptr casts in thread_local_key tests) - rust-lang#100434 (Fix HIR pretty printing of let else) - rust-lang#100438 (Erase regions better in `promote_candidate`) - rust-lang#100445 (adapt test for msan message change) - rust-lang#100447 (Remove more Clean trait implementations) - rust-lang#100464 (Make `[rust] use-lld=true` work on windows) - rust-lang#100475 (Give a helpful diagnostic when the next struct field has an attribute) - rust-lang#100490 (wf: correctly `shallow_resolve` consts) - rust-lang#100501 (nicer Miri backtraces for from_exposed_addr) - rust-lang#100509 (merge two test directories that mean the same thing) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Before, it would fail with "error: ignoring unknown argument '-Wl,--icf=all'"
This option was introduced in #99062 (well, technically #99680)
See zulip thread: https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/rust-lld.3A.20error.3A.20ignoring.20unknown.20argument.20'-Wl.2C--icf.3Dall'