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

miri: make sure we can find link_section statics even for the local crate #126938

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jun 25, 2024

Miri needs some way to iterate all the exported functions and "used" statics of all crates. For dependency crates, this already works fine since we can overwrite the query resonsible for computing exported_symbols, but it turns out for local binary crates this does not work: for binaries, reachable_set skips a lot of its logic and only checks contains_extern_indicator() and RUSTC_STD_INTERNAL_SYMBOL. Other flags like CodegenFnAttrFlags::USED are entirely ignored.

This PR proposes to use the same check, has_custom_linkage, in binaries that we already use to drive the main workqueue of the reachability recursive traversal. I have no idea why binaries used a slightly different check that ignores USED -- was that deliberate or does it just not matter most of the time?

@rustbot
Copy link
Collaborator

rustbot commented Jun 25, 2024

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot 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 Jun 25, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 25, 2024

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@BoxyUwU
Copy link
Member

BoxyUwU commented Jun 25, 2024

r? compiler

what's a linker

@rustbot rustbot assigned compiler-errors and unassigned BoxyUwU Jun 25, 2024
@RalfJung
Copy link
Member Author

@compiler-errors can I take this as an r=you? I am a bit confused why you didn't r+ if you approved the PR.^^

@compiler-errors
Copy link
Member

because i forgot lol

@bors r+

@bors
Copy link
Contributor

bors commented Jun 25, 2024

📌 Commit d9a3423 has been approved by compiler-errors

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 25, 2024

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors 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 Jun 25, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 26, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#126724 (Fix a span in `parse_ty_bare_fn`.)
 - rust-lang#126812 (Rename `tcx` to `cx` in new solver generic code)
 - rust-lang#126879 (fix Drop items getting leaked in Filter::next_chunk)
 - rust-lang#126925 (Change E0369 to give note informations for foreign items.)
 - rust-lang#126938 (miri: make sure we can find link_section statics even for the local crate)
 - rust-lang#126954 (resolve: Tweak some naming around import ambiguities)
 - rust-lang#126964 (Migrate `lto-empty`, `invalid-so` and `issue-20626` `run-make` tests to rmake.rs)
 - rust-lang#126968 (Don't ICE during RPITIT refinement checking for resolution errors after normalization)
 - rust-lang#126971 (Bump black, ruff and platformdirs)
 - rust-lang#126973 (Fix bad replacement for unsafe extern block suggestion)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5c4ede8 into rust-lang:master Jun 26, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 26, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 26, 2024
Rollup merge of rust-lang#126938 - RalfJung:link_section, r=compiler-errors

miri: make sure we can find link_section statics even for the local crate

Miri needs some way to iterate all the exported functions and "used" statics of all crates. For dependency crates, this already works fine since we can overwrite the query resonsible for computing `exported_symbols`, but it turns out for local binary crates this does not work: for binaries, `reachable_set` skips a lot of its logic and only checks `contains_extern_indicator()` and `RUSTC_STD_INTERNAL_SYMBOL`. Other flags like `CodegenFnAttrFlags::USED` are entirely ignored.

This PR proposes to use the same check, `has_custom_linkage`, in binaries that we already use to drive the main workqueue of the reachability recursive traversal. I have no idea why binaries used a slightly different check that ignores `USED` -- was that deliberate or does it just not matter most of the time?
@ehuss
Copy link
Contributor

ehuss commented Jun 26, 2024

@RalfJung This seems to have caused a regression on Windows (at least the -msvc variant).

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

This now generates a linking error:

  = note: symbols.o : error LNK2001: unresolved external symbol _ZN1a3FOO17haec9c1cdc2806380E
          C:\Temp\z20\a\target\debug\deps\a-f995ecc6e2b1e691.exe : fatal error LNK1120: 1 unresolved externals

@RalfJung
Copy link
Member Author

RalfJung commented Jun 26, 2024 via email

@ChrisDenton
Copy link
Member

ChrisDenton commented Jun 26, 2024

Looks weird.

EDIT: That symbol does seem to be in foo.foo.b0bca38e44b57c9a-cgu.0.rcgu.o in my build but the link still fails.

@ChrisDenton
Copy link
Member

ChrisDenton commented Jun 26, 2024

Oh, now that I've sat down to look at it properly the difference is obvious. We're adding #[used] items to symbols.o, thereby forcing them to be included in the final binary.

Before we only exported symbols from the binary if #[no_mangle] or #[export_name="..."] was used.

@ehuss
Copy link
Contributor

ehuss commented Jun 27, 2024

I opened #127052 to track this.

lqd added a commit to lqd/rust that referenced this pull request Jun 28, 2024
…ompiler-errors"

This reverts commit 5c4ede8, reversing
changes made to 95332b8.
bors added a commit to rust-lang-ci/rust that referenced this pull request 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
bors added a commit to rust-lang-ci/rust that referenced this pull request 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.

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, but this should do for now.
@RalfJung RalfJung deleted the link_section branch June 29, 2024 10:01
bors added a commit to rust-lang/miri that referenced this pull request Jun 29, 2024
iter_exported_symbols: also walk used statics in local crate

Since rust-lang/rust#126938 got reverted, we need a different approach.

Fixes #3722
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Jun 29, 2024
iter_exported_symbols: also walk used statics in local crate

Since rust-lang#126938 got reverted, we need a different approach.

Fixes rust-lang/miri#3722
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants