Regression in `#[used]` attribute on Windows msvc about rust HOT 16 CLOSED

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.

from rust.

from rust.

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.

from rust.

from rust.

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.

from rust.

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

from rust.

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.

from rust.

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.

from rust.

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

from rust.

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.

from rust.

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.

from rust.

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.

from rust.

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

from rust.

Looks like this did not fully fix the ci failures 🫠

from rust.

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

from rust.

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

from rust.