Fix deduplication mismatches in vtables leading to upcasting unsoundness #135318
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred to the CTFE machinery cc @rust-lang/wg-const-eval |
This comment has been minimized.
This comment has been minimized.
compiler-errors
force-pushed
the
vtable-fixes
branch
from
04fb1f2 to
c9ae1a0
Compare
|
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred in compiler/rustc_codegen_gcc |
RalfJung
reviewed
Jan 10, 2025
We do have assertions that are intended to ensure that looking up the method via the offset leads to the same result as the type-driven "direct" lookup. If those did not catch this problem, something seems odd. Note that these are debug assertions so you won't see this in |
lcnr
reviewed
Jan 10, 2025
|
vibeck 👍 can talk about it regardless, but this feels very reasonable to me modulo the nit above wrt to hr-trait objects, the important part is that all trait object args are invariant so you can't subtype types in the args: going from |
compiler-errors
force-pushed
the
vtable-fixes
branch
from
c9ae1a0 to
dfe3084
Compare
|
The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes. |
|
I've reworked the
I've also removed the |
|
I've also changed a bunch of |
|
I've tested out the new behavior (before the force-push, but it sounds like that wasn't changing anything about it) a little bit, and it seems quite convincing to me :-) |
|
didnt change anything behaviorally since the rebase |
RalfJung
reviewed
Jan 11, 2025
|
FWIW running the test from #135315 in Miri with debug assertions does ICE, so the existing check do in fact enable Miri to catch problems like this: Same for the test in #135316. Maybe we should just always enable these checks for Miri? That makes virtual calls slightly more expensive, but it seems hard to imagine that becoming a bottleneck. |
RalfJung
reviewed
Jan 11, 2025
tests/ui/traits/trait-upcasting/multiple-supertraits-modulo-binder.rs
Outdated
Show resolved
Hide resolved
tests/ui/traits/trait-upcasting/multiple-supertraits-modulo-normalization.rs
Outdated
Show resolved
Hide resolved
Yeah, my first reaction was to test it in miri and not it triggering did give me a false impression that miri was not affected here :) I'll change the debug assertion to be a regular assertion. |
|
Miri is indeed not affected, but it'd still be nice to have Miri catch issues that only affect code using the actual vtable entries. |
lukas-code
reviewed
Jan 11, 2025
compiler-errors
force-pushed
the
vtable-fixes
branch
from
dfe3084 to
305a3e0
Compare
|
The Miri subtree was changed cc @rust-lang/miri |
compiler-errors
force-pushed
the
vtable-fixes
branch
from
305a3e0 to
1543bb4
Compare
RalfJung
reviewed
Jan 11, 2025
RalfJung
reviewed
Jan 11, 2025
I haven't looked too closely at the implementation, but the changes make sense to me 💚 |
|
Here’s a new ICE on this PR #![feature(trait_upcasting)]
trait Supertrait<T> {
fn method(&self) {}
}
impl<T> Supertrait<T> for () {}
trait WithAssoc {
type Assoc;
}
trait Trait<P: WithAssoc>: Supertrait<P::Assoc> + Supertrait<()> {}
fn upcast<P>(x: &dyn Trait<P>) -> &dyn Supertrait<()> {
x
}
fn main() {
println!("{:p}", upcast::<()> as *const ());
}Error |
|
@steffahn: Different bug, probably shouldn't block this PR. I'll open a different PR exploring a strategy to fix that one. |
|
I’ve commented it here, because I haven’t quite found a repro yet for this particular ICE without this PR. |
compiler-errors
force-pushed
the
vtable-fixes
branch
from
1543bb4 to
9a7d1a4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We currently have two cases where subtleties in supertraits can trigger disagreements in the vtable layout, e.g. leading to a different vtable layout being accessed at a callsite compared to what was prepared during unsizing. Namely:
#135315
In this example, we were not normalizing supertraits when preparing vtables. In the example,
When we prepare
dyn Trait, we see a supertrait ofMiddle<<() as Identity>::Selff>, which itself has two supertraitsSupertrait<()>andSupertrait<<() as Identity>::Selff>. These two supertraits are identical, but they are not duplicated because we were using structural equality and not considering normalization. This leads to a vtable layout with two trait pointers.When we upcast to
dyn Middle<()>, those two supertraits are now the same, leading to a vtable layout with only one trait pointer. This leads to an offset error, and we call the wrong method.#135316
This one is a bit more interesting, and is the bulk of the changes in this PR. It's a bit similar, except it uses binder equality instead of normalization to make the compiler get confused about two vtable layouts. In the example,
When we prepare the vtable for
dyn for<'a> Trait<&'static (), &'a ()>, we currently consider the PolyTraitRef of the vtable as the key for a supertrait. This leads two two supertraits --Supertrait<&'static ()>andfor<'a> Supertrait<&'a ()>.However, we can upcast1 without offsetting the vtable from
dyn for<'a> Trait<&'static (), &'a ()>todyn Trait<&'static (), &'static ()>. This is just instantiating the principal trait ref for a specific'a = 'static. However, when considering those supertraits, we now have only one distinct supertrait --Supertrait<&'static ()>(which is deduplicated since there are two supertraits with the same substitutions). This leads to similar offsetting issues, leading to the wrong method being called.The solution here is to recognize that a vtable isn't really meaningfully higher ranked, and to just treat a vtable as corresponding to a
TraitRefso we can do this deduplication more faithfully. That is to say, the vtable fordyn for<'a> Tr<'a>anddyn Tr<'x>are always identical, since they both would correspond to a set of free regions on an impl... Do note thatTr<for<'a> fn(&'a ())>andTr<fn(&'static ())>are still distinct.There's a bit more that can be cleaned up. In codegen, we can stop using
PolyExistentialTraitRefbasically everywhere. We can also fix SMIR to stop storingPolyExistentialTraitRefin its vtable allocations.As for testing, it's difficult to actually turn this into something that can be tested with
rustc_dump_vtable, since having multiple supertraits that are identical is a recipe for ambiguity errors. Maybe someone else is more creative with getting that attr to work, since the tests I added being run-pass tests is a bit unsatisfying. Miri also doesn't help here, since it doesn't really generate vtables that are offset by an index in the same way as codegen.r? @lcnr for the vibe check? Or reassign, idk. Maybe let's talk about whether this makes sense.
(I guess an alternative would also be to not do any deduplication of vtable supertraits (or only a really conservative subset) rather than trying to normalize and deduplicate more faithfully here. Not sure if that works and is sufficient tho.)
cc @steffahn -- ty for the minimizations
cc @WaffleLapkin -- since you're overseeing the feature stabilization :3
Fixes #135315
Fixes #135316
Footnotes
I say upcast but this is a cast that is allowed on stable, since it's not changing the vtable at all, just instantiating the binder of the principal trait ref for some lifetime. ↩