unreferenced-used-static: run test everywhere #127115
Conversation
rustbot
added
S-waiting-on-review
| // This is a non-regression test for issue #127052 where unreferenced `#[used]` statics in the | ||
| // binary crate couldn't be found by the MSVC linker, causing linking errors. |
There was a problem hiding this comment.
What do you think of the following?
| // This is a non-regression test for issue #127052 where unreferenced `#[used]` statics in the | |
| // binary crate couldn't be found by the MSVC linker, causing linking errors. | |
| // This is a non-regression test for issue #127052 where unreferenced `#[used]` statics | |
| // were forcefully exported and included in the final binary, causing linking errors on MSVC. |
There was a problem hiding this comment.
They were not included though, wasn't that the issue? If they had been included the linker would not have complained about missing symbols.
There was a problem hiding this comment.
I meant the symbol was added to the list of exported symbols.
There was a problem hiding this comment.
Actually maybe just steal #126938 (comment)
There was a problem hiding this comment.
That is also confusing IMO, since "forcing them to be included in the final binary" sounds like what we want.
My understanding is that we are on the one hand telling the linker to export the symbol, but on the other hand the symbol is not actually included in the binary, and that leads to the error. But I may be misunderstanding, this is all very strange to me.
Cc @ChrisDenton
There was a problem hiding this comment.
Yeah basically. As far as I'm aware we don't have any tests that ensure FOO actually ends up in the final binary. It won't, at least on msvc, because we define it as a COMDAT (i.e. we tell the linker it's allowed to remove it).
This is technically in keeping with the documented behaviour which only says the compiler considers it used, and not the linker. But I think that was intended as a hedge and we should still make a best effort to ensure #[used] means it's used.
There was a problem hiding this comment.
There's also used(linker) which I guess is meant to tell the linker to keep it -- I don't understand why that is a separate flag though. 🤷
There was a problem hiding this comment.
I meant the symbol was added to the list of exported symbols.
So what about the current wording?
// This is a non-regression test for issue #127052 where unreferenced `#[used]` statics in the
// binary crate would be marked as "exported", but not be present in the binary, causing linking
// errors with the MSVC linker.
There was a problem hiding this comment.
whatever Chris prefers, r? @ChrisDenton
There was a problem hiding this comment.
I'm finding it a bit hard to summarise without giving a full tutorial on linking. But this explanation seems accurate enough and the issue link helps if anyone wants to explore further. So, yep, this looks good to me.
RalfJung
force-pushed
the
unreferenced-used-static
branch
from
3b4ac01
to
cf6f6ca
Compare
|
@bors r=lqd,ChrisDenton |
bors
added
S-waiting-on-bors
Rollup of 8 pull requests Successful merges: - rust-lang#123588 (Stabilize `hint::assert_unchecked`) - rust-lang#126403 (Actually report normalization-based type errors correctly for alias-relate obligations in new solver) - rust-lang#126917 (Disable rmake test `inaccessible-temp-dir` on riscv64) - rust-lang#127115 (unreferenced-used-static: run test everywhere) - rust-lang#127204 (Stabilize atomic_bool_fetch_not) - rust-lang#127239 (remove unnecessary ignore-endian-big from stack-overflow-trait-infer …) - rust-lang#127245 (Add a test for `generic_const_exprs`) - rust-lang#127246 (Give remote-test-client a longer timeout) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#127115 - RalfJung:unreferenced-used-static, r=lqd,ChrisDenton unreferenced-used-static: run test everywhere Follow-up to rust-lang#127099.
Follow-up to #127099.