-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
x86_64-pc-windows-gnu can't link libtest with LTO=thin #104852
Comments
- CI failing - <https://github.com/rust-lang/cargo/actions/runs/3542820690/jobs/5948722067> - <https://github.com/rust-lang/cargo/actions/runs/3535399031/jobs/5933377592> - Tracking in <rust-lang/rust#104852> - Discussing in <https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/windows.20gnu.20LTO.20CI.20error>
temporarily disable test `lto::test_profile` - CI failing - <https://github.com/rust-lang/cargo/actions/runs/3542820690/jobs/5948722067> - <https://github.com/rust-lang/cargo/actions/runs/3535399031/jobs/5933377592> - Tracking in <rust-lang/rust#104852> - Discussing in <https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/windows.20gnu.20LTO.20CI.20error>
This pretty much sucks since it seems like a rustc bug preventing a (admittedly imperfect) soundness fix, but @nikic (#101368 (comment)) and @eddyb (#101368 (comment)) seemed to have a other ideas for how to prevent that issue from happening, so maybe we take the revert here and wait til something can try one of those. CC @ChrisDenton |
As I said on the revert, I think we do need a proper fix to the underlying issue rather than attempting libs workarounds. |
Hmm, I wonder if we could just do it for |
Maybe. My concern now is that we maybe end up essentially pushing the miscompilation around. So we workaround one problem but maybe we cause another, harder to diagnose, problem somewhere else. That said, I'm not totally against the idea. Perhaps its a trade off we should make. It doesn't sit particularly well with me though I guess there aren't a lot of good options here. |
Hm, well, thinking about it after a cup of coffee... maybe it's the best trade off. But rather than #[cfg_attr(not(windows), inline)]
#[cfg_attr(all(windows, target_thread_local), inline(never))] Though I'm still not entirely comfortable with all this. |
Yeah, good idea. |
I'll wait on PRing that until I can test the patch in a few different build configurations (to avoid a repeat). That will probably be later this weekend. |
Why would it be compiler bug? unsafe fn __getit(
init: $crate::option::Option<&mut $crate::option::Option<$t>>,
) -> $crate::option::Option<&'static $t> { to: #[inline]
unsafe fn __getit(
init: $crate::option::Option<&mut $crate::option::Option<$t>>,
) -> $crate::option::Option<&'static $t> { By the way looks like this could use some unit test to avoid regressions in the future. |
Using the |
Without |
But that's a compiler issue. If the gnu linker cannot handle the inlining, then surely gnu toolchains should not be inlining it? The
|
x86_64-pc-windows-gnu is unable to build a test binary with LTO=thin.
Reproduction:
touch foo.rs
rustc +nightly-x86_64-pc-windows-gnu -Clto=thin --test foo.rs
Error:
Regression started with #101368.
Meta
rustc --version --verbose
:The text was updated successfully, but these errors were encountered: