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

interpret: dyn trait metadata check: equate traits in a proper way #126232

Merged
merged 3 commits into from
Jun 12, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jun 10, 2024

Hopefully fixes rust-lang/miri#3541... unfortunately we don't have a testcase.

The first commit is just a refactor without functional change.

r? @oli-obk

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

rustbot commented Jun 10, 2024

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@rustbot
Copy link
Collaborator

rustbot commented Jun 10, 2024

The Miri subtree was changed

cc @rust-lang/miri

@RalfJung
Copy link
Member Author

@lcnr turns out that using the trait system here is not sufficient, the test still fails with this PR...

@RalfJung
Copy link
Member Author

Ah never mind, I missed one place where we are using !=.

@RalfJung
Copy link
Member Author

... but even if I fix that, the issue remains. These two do not seem to be considered equal:

Binder { value: std::ops::FnMut<(<[std::boxed::Box<i32>] as std::ptr::Pointee>::Metadata,)>, bound_vars: [Region(BrNamed(DefId(0:9 ~ issue_miri_3541_dyn_vtable_trait_normalization[e6e3]::EmplacerFn::'b), 'b))] }
Binder { value: std::ops::FnMut<(usize,)>, bound_vars: [] }

the error is

error: there were 1 unmatched diagnostics
  --> tests/pass/issues/issue-miri-3541-dyn-vtable-trait-normalization.rs:23:24
   |
23 |         unsafe { &mut *((emplacer_fn as *mut EmplacerFn<'a, T>) as *mut Self) }
   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Error: Undefined Behavior: constructing invalid value: wrong trait in wide pointer vtable: expected `std::ops::FnMut(<[std::boxed::Box<i32>] as std::ptr::Pointee>::Metadata)`, but encountered `std::ops::FnMut<(usize,)>`
   |

@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the dyn-trait-equality branch 2 times, most recently from 5043f61 to 5578e8c Compare June 10, 2024 15:26
We can check that the vtable is for the right trait very early, and then just pass the type around.
@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

RalfJung commented Jun 10, 2024 via email

@lcnr
Copy link
Contributor

lcnr commented Jun 10, 2024

So just normalizing and then using == does not work?

If the types are fully concrete, then they should be structurally equal after normalizing, anonymizing bound vars, and erasing all other regions. Breaking this assumption can result in linker errors. In general, you still need to handle types whose bound vars have a different order however.

While I can't think of any other causes for types to be semantically equal but not structurally rn, it still feels wrong to not use proper equality here.

@RalfJung
Copy link
Member Author

Makes sense. This seems to work, thanks!

@rustbot ready

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 12, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jun 12, 2024

📌 Commit 3757136 has been approved by oli-obk

It is now in the queue for this repository.

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

Rollup of 9 pull requests

Successful merges:

 - rust-lang#126039 (Promote `arm64ec-pc-windows-msvc` to tier 2)
 - rust-lang#126075 (Remove `DebugWithInfcx` machinery)
 - rust-lang#126228 (Provide correct parent for nested anon const)
 - rust-lang#126232 (interpret: dyn trait metadata check: equate traits in a proper way)
 - rust-lang#126242 (Simplify provider api to improve llvm ir)
 - rust-lang#126294 (coverage: Replace the old span refiner with a single function)
 - rust-lang#126295 (No uninitalized report in a pre-returned match arm)
 - rust-lang#126312 (Update `rustc-perf` submodule)
 - rust-lang#126322 (Follow up to splitting core's PanicInfo and std's PanicInfo)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 51a58c5 into rust-lang:master Jun 12, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 12, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 12, 2024
Rollup merge of rust-lang#126232 - RalfJung:dyn-trait-equality, r=oli-obk

interpret: dyn trait metadata check: equate traits in a proper way

Hopefully fixes rust-lang/miri#3541... unfortunately we don't have a testcase.

The first commit is just a refactor without functional change.

r? `@oli-obk`
@RalfJung RalfJung deleted the dyn-trait-equality branch June 13, 2024 07:53
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.

"constructing invalid value: wrong trait in wide pointer vtable" for seemingly identical traits
6 participants