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-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)

compiler/rustc_codegen_llvm/src/consts.rs

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

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

bors · 2024-11-04T04:18:39Z

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

Zoxc · 2025-03-04T11:12:49Z

It would be nice to get this merged so Bevy doesn't keep running into this issue.

r? @ChrisDenton

lqd · 2025-03-04T11:28:40Z

We've landed a non-regression test for #81408, does it pass with this PR, I guess so, right? If this lands, please reopen 81408.

Did you see the question about bevy_quickstart in #122790 (comment)?

Zoxc · 2025-03-04T12:22:22Z

#129079 fixes #81408, so I don't see why we'd want to reopen it?

For bevy_quickstart to work, we need both this PR landed and rust-lang/cargo#14575 fixed.

lqd · 2025-03-04T12:31:22Z

Ah right.

For bevy_quickstart to work, we need both this PR landed and rust-lang/cargo#14575 fixed.

Ah, unfortunate. Do you know if this is the main source for people encountering this issue or are you mentioning other bevy use cases?

I assume we will hit asserts preventing ThinLTO + dylib-lto on windows if people tried to enable it again on the dist workflows.

Otherwise this also looks good to me, but let’s see what chris thinks.

Zoxc · 2025-03-04T13:23:12Z

I opened #137998 which would help with rust-lang/cargo#14575.

Do you know if this is the main source for people encountering this issue or are you mentioning other bevy use cases?

Not quite sure what you're asking here.

I assume we will hit asserts preventing ThinLTO + dylib-lto on windows if people tried to enable it again on the dist workflows.

No. That should just work without issue if this PR lands, unless something changed since last year. I did test that it worked locally.

ChrisDenton · 2025-03-08T16:02:23Z

Thanks for tackling this! I agree with your reasoning and I've tested locally. There's still plenty of time before this reaches stable if wesley or someone else does find this breaks in some cases but I fairly strongly suspect this won't cause regressions.

@bors r+

bors · 2025-03-08T16:02:26Z

📌 Commit cc39e5f has been approved by ChrisDenton

It is now in the queue for this repository.

Apply dllimport in ThinLTO 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` --- Edit: This PR is changed to just generally revert rust-lang#103353.

Rollup of 16 pull requests Successful merges: - rust-lang#122790 (Apply dllimport in ThinLTO) - rust-lang#136127 (Allow `*const W<dyn A> -> *const dyn A` ptr cast) - rust-lang#136968 (Turn order dependent trait objects future incompat warning into a hard error) - rust-lang#137147 (Add exclude to config.toml) - rust-lang#137319 (Stabilize `const_vec_string_slice`) - rust-lang#137885 (tidy: add triagebot checks) - rust-lang#138040 (compiler: Use `size_of` from the prelude instead of imported) - rust-lang#138052 (strip `-Wlinker-messages` wrappers from `rust-lld` rmake test) - rust-lang#138084 (Use workspace lints for crates in `compiler/`) - rust-lang#138158 (Move more layouting logic to `rustc_abi`) - rust-lang#138160 (depend more on attr_data_structures and move find_attr! there) - rust-lang#138192 (crashes: couple more tests) - rust-lang#138216 (bootstrap: Fix stack printing when a step cycle is detected) - rust-lang#138232 (Reduce verbosity of GCC build log) - rust-lang#138233 (Windows: Don't link std (and run-make) against advapi32, except on win7) - rust-lang#138242 (Revert "Don't test new error messages with the stage 0 compiler") r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#122790 (Apply dllimport in ThinLTO) - rust-lang#137650 (Move `fs` into `sys`) - rust-lang#138228 (Use `disjoint_bitor` inside `borrowing_sub`) - rust-lang#138233 (Windows: Don't link std (and run-make) against advapi32, except on win7) - rust-lang#138253 (Continue to check attr if meet empty repr for adt) - rust-lang#138263 (Fix `repr128-dwarf` test) - rust-lang#138276 (Lazy load NtOpenFile for UWP) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#122790 - Zoxc:dllimp-rev, r=ChrisDenton Apply dllimport in ThinLTO 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`` --- Edit: This PR is changed to just generally revert rust-lang#103353.

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

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

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

Zoxc force-pushed the dllimp-rev branch from cd09504 to 8794ca2 Compare September 22, 2024 10:11

Kobzol mentioned this pull request Jan 23, 2025

Disable building rustc with (Thin)LTO on Windows #113433

Closed

0not mentioned this pull request Jan 31, 2025

.add_systems DONT WORK bevyengine/bevy#17130

Open

Apply dllimport in ThinLTO

cc39e5f

Zoxc force-pushed the dllimp-rev branch from 8794ca2 to cc39e5f Compare March 3, 2025 13:02

janhohenheim mentioned this pull request Mar 3, 2025

It's possible to hit unreachable code of the standard library when compiling with dynamic_linking and release on Windows in certain cases bevyengine/bevy#14396

Open

rustbot assigned ChrisDenton and unassigned wesleywiser Mar 4, 2025

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2025

jieyouxu mentioned this pull request Mar 9, 2025

Rollup of 16 pull requests #138255

Closed

matthiaskrgr mentioned this pull request Mar 9, 2025

Rollup of 7 pull requests #138279

Merged

bors merged commit 827bb5e into rust-lang:master Mar 9, 2025
6 checks passed

rustbot added this to the 1.87.0 milestone Mar 9, 2025

Zoxc deleted the dllimp-rev branch March 9, 2025 21:43

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 24, 2024

bors commented Sep 20, 2024

bors commented Nov 4, 2024

Zoxc commented Mar 4, 2025

lqd commented Mar 4, 2025 •

edited

Loading

Zoxc commented Mar 4, 2025

lqd commented Mar 4, 2025

Zoxc commented Mar 4, 2025

ChrisDenton commented Mar 8, 2025

bors commented Mar 8, 2025

Apply dllimport in ThinLTO #122790

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 24, 2024

bors commented Sep 20, 2024

bors commented Nov 4, 2024

Zoxc commented Mar 4, 2025

lqd commented Mar 4, 2025 • edited Loading

Zoxc commented Mar 4, 2025

lqd commented Mar 4, 2025

Zoxc commented Mar 4, 2025

ChrisDenton commented Mar 8, 2025

bors commented Mar 8, 2025

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

lqd commented Mar 4, 2025 •

edited

Loading