Skip to content
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

Regression in #[used] attribute on Windows msvc #127052

Closed
ehuss opened this issue Jun 27, 2024 · 16 comments · Fixed by #127099
Closed

Regression in #[used] attribute on Windows msvc #127052

ehuss opened this issue Jun 27, 2024 · 16 comments · Fixed by #127099
Labels
A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. O-windows Operating system: Windows O-windows-msvc Toolchain: MSVC, Operating system: Windows regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.

Comments

@ehuss
Copy link
Contributor

ehuss commented Jun 27, 2024

Code

I tried this code:

#[used]
static FOO: u32 = 0;
fn main() {}

I expected to see this happen: Compiles successfully.

Instead, this happened: Fails with this error:

  = note: symbols.o : error LNK2001: unresolved external symbol _ZN3foo3FOO17h3b276d94f7182e74E
          D:\Temp\foo\target\debug\deps\foo.exe : fatal error LNK1120: 1 unresolved externals

Version it worked on

nightly-2024-06-26 and earlier.

Version with regression

nightly-2024-06-27 and later, starting with #126938.
See #126938 (comment) for a discussion of the issue.

rustc --version --verbose:

rustc 1.81.0-nightly (4bc39f028 2024-06-26)
binary: rustc
commit-hash: 4bc39f028d14c24b04dd17dc425432c6ec354536
commit-date: 2024-06-26
host: x86_64-pc-windows-msvc
release: 1.81.0-nightly
LLVM version: 18.1.7

cc @RalfJung @ChrisDenton

@ehuss ehuss added A-linkage Area: linking into static, shared libraries and binaries O-windows Operating system: Windows regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. O-windows-msvc Toolchain: MSVC, Operating system: Windows C-bug Category: This is a bug. labels Jun 27, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jun 27, 2024
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jun 27, 2024
@ChrisDenton
Copy link
Member

To elaborate, the difference is in symbols.o.

Here's a dump of symbols.o before the change:

COFF SYMBOL TABLE
000 00000000 ABS    notype       Static       | @feat.00
001 00000000 UNDEF  notype ()    External     | main
002 00000000 UNDEF  notype ()    External     | rust_eh_personality
003 00000000 UNDEF  notype ()    External     | rust_begin_unwind
004 00000000 UNDEF  notype ()    External     | rust_eh_personality
005 00000000 UNDEF  notype ()    External     | __rust_alloc
006 00000000 UNDEF  notype ()    External     | __rust_dealloc
007 00000000 UNDEF  notype ()    External     | __rust_realloc
008 00000000 UNDEF  notype ()    External     | __rust_alloc_zeroed

And here's after:

COFF SYMBOL TABLE
000 00000000 ABS    notype       Static       | @feat.00
001 00000000 UNDEF  notype       External     | _ZN3foo3FOO17hff4b2214a6243215E
002 00000000 UNDEF  notype ()    External     | main
003 00000000 UNDEF  notype ()    External     | rust_eh_personality
004 00000000 UNDEF  notype ()    External     | rust_begin_unwind
005 00000000 UNDEF  notype ()    External     | rust_eh_personality
006 00000000 UNDEF  notype ()    External     | __rust_alloc
007 00000000 UNDEF  notype ()    External     | __rust_dealloc
008 00000000 UNDEF  notype ()    External     | __rust_realloc
009 00000000 UNDEF  notype ()    External     | __rust_alloc_zeroed

Spot the difference. Spoiler: _ZN3foo3FOO17hff4b2214a6243215E is now present.

@RalfJung
Copy link
Member

RalfJung commented Jun 27, 2024 via email

@ChrisDenton
Copy link
Member

Well that's not a problem per se but if we want to do that then we have to do it properly. _ZN3foo3FOO17hff4b2214a6243215E is defined as a STATIC in foo.eopdl1t2ep4vx19njs4kilajt.rcgu.o (on my machine). This is linked directly and not in a lib file. So it's already being included. There's no reason for symbols.o to include it.

The only issue is that FOO is being emitted as a comdat, meaning the linker is still allowed to remove it. We could just not do that (or add a /include linker directive). But that's a pre-existing issue.

@RalfJung
Copy link
Member

RalfJung commented Jun 27, 2024 via email

@ChrisDenton
Copy link
Member

I don't know enough about the Rust compiler to say. My complete guess would be that used was silently broken before but now it's nosily broken.

@RalfJung
Copy link
Member

Hm, who would understand our Windows linking story? @bjorn3 ?

@tmiasko
Copy link
Contributor

tmiasko commented Jun 28, 2024

After CGU partitioning, we try to internalize symbols when possible. What is surprising to me is that this process only looks at export level, ignoring used attribute (in the contrast to say the internalization during fat LTO).

Consequently we have a situation where synthetic object file references a symbol that is no longer exported (on Linux that does not result in a linking error, even though reference is unresolved).

Anyway, if the consequence of #126938 would be to export those symbols for no reason, that seems rather counter productive to me.

@ChrisDenton
Copy link
Member

Ah, the behaviour was working to spec before: https://doc.rust-lang.org/reference/abi.html?highlight=Used#the-used-attribute

This attribute forces the compiler to keep the variable in the output object file (.o, .rlib, etc. excluding final binaries) even if the variable is not used, or referenced, by any other item in the crate. However, the linker is still free to remove such an item.

So I think I'd agree that #126938 is the wrong approach and should be reverted because this currently ends up conflating different things. Maybe changes elsewhere would fix this but I don't want to speculate.

@tmiasko
Copy link
Contributor

tmiasko commented Jun 28, 2024

@ChrisDenton the separate issue you pointed out earlier in #127052 (comment) is presumably still relevant for #[used(linker)]?

@tgross35
Copy link
Contributor

Could this explain recent CI flakiness? The failure at #127066 (comment) and a couple other failed merges (linked on that thread, also the retries) have been msvc linker errors.

@RalfJung
Copy link
Member

RalfJung commented Jun 28, 2024

That's unfortunate. :/ Our reachable symbol code is such a spaghetti mess... I guess I'll just have to copy-paste more things into Miri then. 🤷

I am traveling right now, I'd appreciate if someone else could do the revert.

@tmiasko
Copy link
Contributor

tmiasko commented Jun 28, 2024

The following patch should fix the incorrect internalization I mentioned earlier:

diff --git a/compiler/rustc_codegen_ssa/src/back/symbol_export.rs b/compiler/rustc_codegen_ssa/src/back/symbol_export.rs
index 6abe4fa1c38..71b8cb9c559 100644
--- a/compiler/rustc_codegen_ssa/src/back/symbol_export.rs
+++ b/compiler/rustc_codegen_ssa/src/back/symbol_export.rs
@@ -159,7 +159,7 @@ fn is_reachable_non_generic_provider_local(tcx: TyCtxt<'_>, def_id: LocalDefId)
     let export_threshold = threshold(tcx);
 
     if let Some(&info) = tcx.reachable_non_generics(LOCAL_CRATE).get(&def_id.to_def_id()) {
-        info.level.is_below_threshold(export_threshold)
+        info.level.is_below_threshold(export_threshold) || info.used
     } else {
         false
     }

As for the question whether those symbols should be exported in the first place, it might be necessary for COFF and #[linker(used)] . The following code suggests that llvm.used would not have the desired effect otherwise https://github.com/rust-lang/llvm-project/blob/5a5152f653959d14d68613a3a8a033fb65eec021/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp#L1898-L1910

I don't have access to windows environment at the moment to verify that, so I don't plan to work on this myself.

bors added a commit to rust-lang-ci/rust that referenced this issue Jun 28, 2024
Revert "miri: make sure we can find link_section statics even for the local crate"

This PR reverts rust-lang#126938 as [requested by its author](rust-lang#127052 (comment)), to fix the rust-lang#127052 regression.

r? ghost

fixes rust-lang#127052

We should probably improve the [`used` rmake test(s)](https://github.com/rust-lang/rust/blob/57931e50409f9365791f7923a68f9ae1ec301c4c/tests/run-make/used/rmake.rs#L7) in the future, though this should do for now.

(Opening as draft to run tests on a windows env)

try-job: x86_64-msvc
@matthiaskrgr
Copy link
Member

Ooh this is also the stuff that also broke several ci-runs already ...

@bors bors closed this as completed in 9ed2ab3 Jun 29, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jun 29, 2024
Revert "miri: make sure we can find link_section statics even for the local crate"

This PR reverts #126938 as [requested by its author](rust-lang/rust#127052 (comment)), to fix the #127052 regression.

Fixes #127052

We should probably improve the [`used` rmake test(s)](https://github.com/rust-lang/rust/blob/57931e50409f9365791f7923a68f9ae1ec301c4c/tests/run-make/used/rmake.rs#L7) in the future, but this should do for now.
@matthiaskrgr
Copy link
Member

Looks like this did not fully fix the ci failures 🫠

@lqd
Copy link
Member

lqd commented Jun 29, 2024

Makes sense: some CI failures look non-deterministic, while we'd expect this error not to be.

@RalfJung
Copy link
Member

Alternative approach for Miri: rust-lang/miri#3723

@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. O-windows Operating system: Windows O-windows-msvc Toolchain: MSVC, Operating system: Windows regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants