Disable building rustc with (Thin)LTO on Windows
#113433
Conversation
|
r? @clubby789 (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
src/bootstrap/compile.rs
Outdated
| @@ -817,6 +817,12 @@ impl Step for Rustc { | |||
| if compiler.stage != 0 { | |||
| match builder.config.rust_lto { | |||
| RustcLto::Thin | RustcLto::Fat => { | |||
| if target.contains("windows") { | |||
There was a problem hiding this comment.
(we know windows-msvc has the miscompile but it could be fine on windows-gnu.)
There was a problem hiding this comment.
Good point. I suppose that we only definitely know about miscompiles in x86_64-pc-windows-msvc, right? So maybe we should just warn against this single target.
There was a problem hiding this comment.
Yeah, for sure on x86_64-pc-windows-msvc, on the others there have been no reports yet I don't think, but it may just be that they're less commonly used. But now that we know a few ways to trigger them, it may be possible to test there. (maybe even on CI depending on the Tier)
There was a problem hiding this comment.
Hm, do the current reports affect out-of-the-box msvc or just when using lld?
There was a problem hiding this comment.
#109114 (if we don't use lld on CI) coupled with #113425 (which involves lld) together mean that building rustc with rust.lto = "thin" will result in a miscompiled rustc.
We're not building rustc with ThinLTO on windows because of these issues, and those -msvc miscompilations look serious enough that people probably shouldn't do so either, and instead be made aware of that?
So I'm not sure what you mean by "maybe a bit heavy handed to disable thinlto for x86_64-pc-windows-msvc" ? This PR is not disabling ThinLTO, it's about bootstrap preventing building rustc with -Zdylib-lto, to avoid miscompilations.
There was a problem hiding this comment.
We can (hopefully) test for issues like #109114 in CI. But the same is not true for lld, no?
There was a problem hiding this comment.
#109114 => Rustc compiled with LLD=false, ThinLTO=true => produces miscompilations
#112946 (comment) => Rustc compiled with LLD=true, ThinLTO=true => produces miscompilations
So it seems that when we compile rustc with ThinLTO, it then produces miscompilations regardless whether it was itself compiled with LLD or not. Therefore we want to ban ThinLTO when you build Rustc for Windows for the moment.
Note 1: This has nothing to do with applying LTO to Rust programs. Rustc can still perform LTO when building Rust code on Windows.
Note 2: Rustc on Windows is currently not optimized with LTO because of the miscompilations. This PR doesn't change that.
There was a problem hiding this comment.
We could instead emit a scary warning, if an error would disturb your use-cases.
There was a problem hiding this comment.
FWIW I recall seeing many MSVC + LTO + LLD miscompilation issues over the years but strangely they didn't reproduce on windows-gnu when I tested them, so I'd agree it makes sense to apply this limit on windows-msvc only.
Kobzol
force-pushed
the
bootstrap-window-thinlto-error
branch
from
2091be1 to
292cafe
Compare
|
☔ The latest upstream changes (presumably #116196) made this pull request unmergeable. Please resolve the merge conflicts. |
We know that
rustccurrently produces miscompilations when it is compiled with (Thin)LTO on Windows (#109067). We have therefore disabled LTO fordistwindows builds. However, other people who build their ownrustcmight now know this, and this leads to them having miscompilations in the wild (#112946 (comment)).I think that we should give a loud warning that this build configuration produces known miscompilations.