Apply dllimport in ThinLTO #122790

Zoxc · 2024-03-20T19:13:05Z

This partially reverts #103353 by properly applying dllimport if -Z dylib-lto is passed. That PR should probably fully be reverted as it looks quite sketchy. We don't know locally if the entire crate graph would be statically linked.

This should hopefully be sufficient to make ThinLTO work for rustc on Windows.

r? @wesleywiser

Edit: This PR is changed to just generally revert #103353.

compiler/rustc_codegen_llvm/src/consts.rs

lqd · 2024-03-20T20:28:41Z

We can make try builds also run some tests now, so we could test this PR with it:

re-apply https://github.com/rust-lang/rust/pull/103591/files
set DIST_TRY_BUILD to 0
switch the try builder to x64-msvc
launch a try build and see what happens

lqd · 2024-03-21T06:24:04Z

@bors try

Apply dllimport in ThinLTO for -Z dylib-lto This partially reverts rust-lang#103353 by properly applying `dllimport` if `-Z dylib-lto` is passed. That PR should probably fully be reverted as it looks quite sketchy. We don't know locally if the entire crate graph would be statically linked. This should hopefully be sufficient to make ThinLTO work for rustc on Windows. r? `@wesleywiser`

bors · 2024-03-21T06:25:13Z

⌛ Trying commit ea72d12 with merge af61c39...

bors · 2024-03-21T08:31:54Z

☀️ Try build successful - checks-actions
Build commit: af61c39 (af61c395e5842831b05bff9ab29b95ebd08a2b34)

Zoxc · 2024-08-11T16:35:37Z

I'm a bit confused what you meant about underlying issues in #128947?

lqd · 2024-08-11T16:43:43Z

In recent t-compiler meetings, Wesley said he'd be looking at this PR soon and testing with the original internal repro.

I think #81408, #109114, #103353, and this PR are all pointing at unknown issues, with dllimports on our side that I guess are more or less "tracked" via #27438, and/or lld bugs on msvc (or the converse with link.exe, #109114 (comment) seems to show a damned if we do damned if we don't kind of deal).

On top of this, the fact that we're using debug_asserts, that we don't enable anywhere, to do CLI validation is concerning. We also only CLI validate -Clinker-plugin-lto while #103353's workaround also looks at -Clto. This would likely have caught #109067/#109114.

It doesn't look like we have tests for #81408's behavior, only for what the workaround is expected to produce. So we don't know if the lld issue would reproduce or has been fixed, if we land this PR.

lqd · 2024-08-11T17:07:49Z

To be clear, I want us to land the PR. I'm just not sure we understand the issues, workarounds, and what other miscompilations we could uncover, don't have sufficient test coverage, and can't even crater any of this on windows, and last time this happened no one noticed before hitting stable.

tl;dr: we’re in a tough spot to make changes confidently imho

So with all of the above and previous comment, are you 100% confident this is fine as-is?

Zoxc · 2024-08-11T19:45:50Z

@lqd Just to clarify, you're just talking about this PR and not applying ThinLTO to the compiler on msvc right?

Zoxc · 2024-08-12T01:10:05Z

It looks like #103353 also causes #127979 so it might make sense to fully revert it instead. I suspect Bevy is not using -Z dylib-lto.

tgross35 · 2024-08-12T02:19:47Z

On top of this, the fact that we're using debug_asserts, that we don't enable anywhere, to do CLI validation is concerning.

What is this referring to? I assume not #127979 (comment) since many of these were changed in #127995.

lqd · 2024-08-12T14:16:47Z

What is this referring to? I assume not #127979 (comment)

That was the one yes, the change is recent and didn't show in the github UI in this older PR until it was rebased yesterday. The assertion is still not checking -Clto on master. But that may be moot if we land this full revert.

@lqd Just to clarify, you're just talking about this PR and not applying ThinLTO to the compiler on msvc right?

The latter is scarier, but #109114 (comment) seems to be saying that the same situation can happen on both lld and msvc's link, and that fixing one breaks the other (and that may be what happened with #127979). So I wished that we had a more in-depth look at the entire situation rather than a back and forth of regressions.

I wish to know whether this comment is still accurate. And if we're indeed "disallowing dynamic linking" with -Clinker-plugin-lto but not -Clto=thin.

                // ThinLTO can't handle this workaround in all cases, so we don't
                // emit the attrs. Instead we make them unnecessary by disallowing
                // dynamic linking when linker plugin based LTO is enabled.

I also wish for some involvement from people more familiar with the situation like @wesleywiser and @michaelwoerister or other windows experts.

It looks like #103353 also causes #127979 so it might make sense to fully revert it instead. I suspect Bevy is not using -Z dylib-lto.

Right, they probably don't (but do recommend -Zthreads=0 which is audacious :).

What happens with this PR on the repros in #81408? A revert is just preferring to accept #81408 instead of #127979, and both were encountered in the wild. Bevy seems to use lld on windows anyways, so their users turning on ThinLTO can easily encounter either issue. Not great.

The good thing out of #127979 is that we have a minimal repro to make a test.

Zoxc · 2024-08-12T20:28:37Z

I tested a reproducer in #81408 with this PR applied. It crashed with lld, but not Microsoft's linker, so that's likely just a lld bug.

A revert is just preferring to accept #81408 instead of #127979

One is a lld bug, the other is a rustc bug introduced in #103353 and thus affects all linkers. It's pretty clear what should be fixed there.

lqd · 2024-08-13T21:55:36Z

Ok then until wesley/michael can take a look, it'll be worthwhile to have tests in this PR that reproduce the issue on CI for sure.

FWIW I tested bash's repro of #127979, and it reproduced. I then updated VS build tools to test with this PR (because of linker errors on rustc-driver with download-ci-llvm, and it didn't even help; but stage1 built when compiling llvm).

With these build tools v16.11.38, bash's repro doesn't trigger the bug anymore, with link.exe or rust-lld, in release or debug, with this PR or master.

I also tested the original bevy quickstart repo where the issue appeared, with this PR. It crashed when using rust-lld, but doesn't work with link.exe either (doesn't crash, just that "nothing happens", unlike in debug mode). And things were the same on nightly with link.exe.

🔥🔥🔥🔥🔥🔥🔥🔥
🔥 🐶 this is fine 🔥
🔥🔥🔥🔥🔥🔥🔥🔥

Zoxc · 2024-08-14T03:38:00Z

bash's repro doesn't trigger the bug anymore

Apparently after #128469 (which is in this branch now) it no longer reproduces, assume that's why you couldn't reproduce it? That doesn't seem to contain a fix though.

lqd · 2024-08-14T12:47:00Z

assume that's why you couldn't reproduce it?

Ah yes:

it reproduces on the nightly that precedes the rollup (nightly-2024-07-31) and on its parent commit
it reproduces on master with std: implement the once_wait feature #127567 from that rollup reverted
it doesn't reproduce with this PR + std: implement the once_wait feature #127567 reverted

As for bevy_quickstart:

"nothing happens" unexpectedly on yesterday's nightly (91376f4)
"entered unreachable code" ICE on master with std: implement the once_wait feature #127567 reverted
"entered unreachable code" ICE with this PR + std: implement the once_wait feature #127567 reverted. Are you able to reproduce this?

lqd · 2024-08-14T19:01:04Z

In case it helps, here's a non-regression test for #81408. It will pass on master and with #129079, but will fail with this PR of course.

janhohenheim · 2024-08-24T12:17:55Z

@lqd regarding your earlier comment: any recommendations for making Bevy's set of recommended compiler settings less audacious? I'd be ready to open a PR there to fix them up :)
Most of these were not chosen with careful deliberation but more of an "I chose this and that and them benchmarked" approach.

lqd · 2024-08-24T12:28:09Z

Just that -Zthreads is known to have issues, deadlocks and ICEs — but I guess test driving it and opening reproducible issues should they appear, could also be worthwhile. (And it also can work fine)

RalfJung · 2024-09-02T16:38:14Z

compiler/rustc_codegen_llvm/src/consts.rs

@@ -327,8 +326,7 @@ impl<'ll> CodegenCx<'ll, '_> {
                // ThinLTO can't handle this workaround in all cases, so we don't
                // emit the attrs. Instead we make them unnecessary by disallowing
                // dynamic linking when linker plugin based LTO is enabled.
-                !self.tcx.sess.opts.cg.linker_plugin_lto.enabled() &&
-                self.tcx.sess.lto() != Lto::Thin;


Given the history of this, is it worth explaining that we explicitly do not want to check sess.lto() here?

bors · 2024-09-20T14:58:59Z

☔ The latest upstream changes (presumably #130506) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned wesleywiser Mar 20, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 20, 2024

lqd reviewed Mar 20, 2024

View reviewed changes

compiler/rustc_codegen_llvm/src/consts.rs Outdated Show resolved Hide resolved

Zoxc force-pushed the dllimp-rev branch from 3d83ca4 to 2694c65 Compare March 20, 2024 20:25

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Mar 21, 2024

Zoxc force-pushed the dllimp-rev branch from ea72d12 to 2694c65 Compare March 21, 2024 08:38

Zoxc mentioned this pull request Aug 10, 2024

Perform ThinLTO on x86_64-pc-windows-msvc dist builder #128947

Open

Zoxc force-pushed the dllimp-rev branch from 2694c65 to c8d81f1 Compare August 12, 2024 01:21

Zoxc changed the title ~~Apply dllimport in ThinLTO for -Z dylib-lto~~ Apply dllimport in ThinLTO Aug 12, 2024

This comment has been minimized.

Sign in to view

Zoxc mentioned this pull request Aug 12, 2024

unreachable!("state is never set to invalid values") is reached #127979

Open

Apply dllimport in ThinLTO

cd09504

Zoxc force-pushed the dllimp-rev branch from c8d81f1 to cd09504 Compare August 12, 2024 02:01

RalfJung reviewed Sep 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply dllimport in ThinLTO #122790

Apply dllimport in ThinLTO #122790

Zoxc commented Mar 20, 2024 •

edited

Loading

lqd commented Mar 20, 2024

lqd commented Mar 21, 2024

bors commented Mar 21, 2024

bors commented Mar 21, 2024

Zoxc commented Aug 11, 2024

lqd commented Aug 11, 2024 •

edited

Loading

lqd commented Aug 11, 2024 •

edited

Loading

Zoxc commented Aug 11, 2024

Zoxc commented Aug 12, 2024

This comment has been minimized.

tgross35 commented Aug 12, 2024

lqd commented Aug 12, 2024 •

edited

Loading

Zoxc commented Aug 12, 2024

lqd commented Aug 13, 2024 •

edited

Loading

Zoxc commented Aug 14, 2024

lqd commented Aug 14, 2024

lqd commented Aug 14, 2024

janhohenheim commented Aug 24, 2024 •

edited

Loading

lqd commented Aug 24, 2024

RalfJung Sep 2, 2024

bors commented Sep 20, 2024

Apply dllimport in ThinLTO #122790

Are you sure you want to change the base?

Apply dllimport in ThinLTO #122790

Conversation

Zoxc commented Mar 20, 2024 • edited Loading

lqd commented Mar 20, 2024

lqd commented Mar 21, 2024

bors commented Mar 21, 2024

bors commented Mar 21, 2024

Zoxc commented Aug 11, 2024

lqd commented Aug 11, 2024 • edited Loading

lqd commented Aug 11, 2024 • edited Loading

Zoxc commented Aug 11, 2024

Zoxc commented Aug 12, 2024

This comment has been minimized.

tgross35 commented Aug 12, 2024

lqd commented Aug 12, 2024 • edited Loading

Zoxc commented Aug 12, 2024

lqd commented Aug 13, 2024 • edited Loading

Zoxc commented Aug 14, 2024

lqd commented Aug 14, 2024

lqd commented Aug 14, 2024

janhohenheim commented Aug 24, 2024 • edited Loading

lqd commented Aug 24, 2024

RalfJung Sep 2, 2024

Choose a reason for hiding this comment

bors commented Sep 20, 2024

Zoxc commented Mar 20, 2024 •

edited

Loading

lqd commented Aug 11, 2024 •

edited

Loading

lqd commented Aug 11, 2024 •

edited

Loading

lqd commented Aug 12, 2024 •

edited

Loading

lqd commented Aug 13, 2024 •

edited

Loading

janhohenheim commented Aug 24, 2024 •

edited

Loading