Consider warning when comparing wide pointers with vtables (as their address identity is unstable)

[jdm]

vtable addresses may differ cross codegen units. To mitigate this, it would be good to have a lint that warns against comparing wide pointers with vtables.

Original report

This is a regression from the 2/27 to 2/28 nightly.

pub trait Trait {}
impl Trait for i32 {}

pub struct Obj<'a, T: 'static + Trait> {
    ptr: &'a T,
    trait_ptr: *const dyn Trait,
}

impl<'a, T: Trait + 'static> Drop for Obj<'a, T> {
    fn drop(&mut self) {
        assert_eq!(self.trait_ptr, self.ptr as *const dyn Trait);
    }
}

fn main() {
    let ptr = 5;
    let _name = Obj {
        ptr: &ptr,
        trait_ptr: &ptr,
    };
}

When this program is built with rustc main.rs, it runs without any trouble. When it's built with rustc main.rs -C incremental=, I receive the following output:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `0x7ffee7d23864`,
 right: `0x7ffee7d23864`', main.rs:11:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

From the regression window, I suspect https://github.com/rust-lang/rust/pull/67332.

[RalfJung]

Something is odd about your example, you are asserting them to be equal and then the error is that they are equal? When I run your code on playground, it works fine on nightly.

But also, note that there is no guarantee that all vtables for the same type get the same address, or that they get different addresses. Vtable address identity is an unstable implementation detail (similar to, for example, address identity of functions, or of promoteable references). In other words, I see no bug here. Could you describe the expected behavior and how that differs from the actual behavior?

[Centril]

@RalfJung From my reading of the OP, it seems like there's something particular about incremental compilation here, so maybe it's not reproducible on the playground?

[RalfJung]

I guess one question is whether wide ptr equality should even compare vtables, given that they are "unstable" in a sense. I am not sure if it does, but your tests indicate they do?

FWIW, the example on playground also works on stable, and in release mode. Looks like playground cannot reproduce the problem.

[RalfJung]

Ah, I missed that incremental compilation is involved. Incremental compilation having an effect on vtable identity seems odd... I am not sure if we want to consider that a bug or not.^^

[RalfJung]

@jdm could you adjust your example to print not just the data address but also the vtable address? That might also explain why the output looks so strange.^^ (You seem to have already diagnosed that this is about different vtable pointers, but do not explain how you arrived at that conclusion?)

Here's a totally not supported way to do that:

fn fmt_with_vtable(ptr: *const dyn Trait) -> String {
    let (ptr, vtable): (*const u8, *const u8) = unsafe { std::mem::transmute(ptr) };
    format!("{:?} (vtable: {:?})", ptr, vtable)
}

[jonas-schievink]

Incremental may result in different codegen unit partitioning, and each codegen unit might get its own vtable, so that part seems entirely expected to me. However the assertion failure seems to show the same address here?

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `0x7ffee7d23864`,
 right: `0x7ffee7d23864`', main.rs:11:9

[RalfJung]

My guess is that the main problem @jdm has is that comparing wide pointers also compares their metadata. For trait objects, this basically means equal pointers might "randomly" not be equal any more due to vtable address differences.

So either @jdm could cast the raw ptr to a thin ptr in their codebase to avoid relying on vtable identities, or else comparing wide raw pointers could ignore metadata in general -- though I expect for slices we might want it to compare the length?

[jonas-schievink]

Oh, right, the vtable pointer just isn't printed

[nox]

@RalfJung Do you mean that as a temporary workaround? It is definitely a regression that this broke. How codegen units are generated shouldn't influence whether two identical trait objects are indeed identical.

[nox]

Or should PartialEq be implemented for *const T and *mut T only if T: Sized?

[tmiasko]

See also #46139, #48795 and #63021.

[nox]

I have no idea how this never broke Servo before then.

Is there a lint somewhere to complain loudly about equality checks for unsized pointer types?

[RalfJung]

Given there is ample precedence for "vtable identity is unstable and allowed to change with codegen units" (and there are three open issues about it so we don't need a 4th), should this issue be turned into one of the following discussions?

• Better linting to prevent people from running into this.
• Changing pointer equality test to not compare vtables any more when comparing wide pointers.
• Making an effort to actually make vtables unique across the entire compilation unit.

[nox]

Changing pointer equality test to not compare vtables any more when comparing wide pointers.

That seems to be the worst solution (among the three you suggested I mean). In Servo, there are two different tracing objects for a given T, depending on whether the T has been fully initialised or not. Granted, we can add safety guards on our side, but if we naively assumed that the vtable pointers were indeed inspected for equality, we could end up removing the tracing object for a fully initialised T when we meant to remove the older one, for when the T was not initialised yet.

[RalfJung]

@nox that also sounds like the lint would have a high risk of false positives, though.

[nox]

@RalfJung Why? Our problem in Servo does not involve any generic T, it's all about *const dyn JSTraceable comparisons.

[RalfJung]

It just sounded like you actually do want to compare vtables in that case?

I was imagining the lint would propose to cast the pointer the *const u8 before comparing if vtables ought to be ignored, at least that part seems not to be what you want.

[nox]

I was imagining the lint would propose to cast the pointer the *const u8 before comparing if vtables ought to be ignored, at least that part seems not to be what you want.

My reply was assuming that when you said "changing pointer equality test to not compare vtables any more when comparing wide pointers" you meant to change the test in the very implementationn of PartialEq in the standard library, which would lead us to not even notice there was a problem.

If you meant in our code in Servo, yeah, we should probably do that.

[RalfJung]

If you meant in our code in Servo, yeah, we should probably do that.

Wait didn't you just say that in Server you don't want to ignore the vtable? I am confused. Or did you want to say that Servo is being careful enough but hypothetically it would be easy to get wrong?

But anyway I just take you word for it that ignoring vtables would be a bad idea. There are other arguments for this as well (not least being implementation difficulty, as for slices we likely do want to compare the metadata).

[nox]

Let's walk back a bit:

• In Servo we currently rely on the vtable pointers being stable.
• I thought you were saying you want PartialEq to ignore the vtable pointers.
• That would lead to issues in current Servo because the vtable pointers are currently important, and ignoring the vtables in PartialEq itself would be a silent change for us.

So I'm saying that we should lint against trait object comparisons, and that in Servo we should stop relying on vtable pointers being stable and change the thing I mentioned earlier.

[nox]

For the record you convinced me earlier that we need to change the code in Servo anyway because those vtable pointers were never stable, so I made the necessary changes etc in Servo. I guess this issue could be repurposed as needing a lint to report such comparisons.

[RalfJung]

All right I edited the issue title and description accordingly.

[tmiasko]

There is also an opposite issue: addresses of virtual tables associated with different impls might compare equal, since they are marked with unnamed_addr.

It is possible to produce such cases after repeating some optimization passes. First all methods from vtables are merged together, and then vtables themselves are.

[redradist]

From my point of view this issue should be fixed, because it is completely not obvious that the same trait object has different fat pointers ... :(

https://users.rust-lang.org/t/rwlock-hashmap-no-entry-found/42428/15

[Aaron1011]

If we don't want to guarantee that vtables are unique, then I think a lint would need to effectively "deprecate" the Eq/PartialEq impls for *[const/mut] T and [&/&mut] T when T: !Sized (assuming that we want to lint on something like HashSet<*const T> where T: !Sized). The only alternative I could see would be to do some kind of post-monomorphization check, which I think could lead to very confusing error messages.

I'm not sure how noisy this would end up being, or how difficult it would be to implement.

[redradist]

@Aaron1011

If we don't want to guarantee that vtables are unique, then I think a lint would need to effectively "deprecate" the Eq/PartialEq impls for *[const/mut] T and [&/&mut] T when T: !Sized (assuming that we want to lint on something like HashSet<*const T> where T: !Sized). The only alternative I could see would be to do some kind of post-monomorphization check, which I think could lead to very confusing error messages.

I'm not sure how noisy this would end up being, or how difficult it would be to implement.

I am not sure if rust should to have different vtables for implementation of trait for the same object because it does not seems like Zero-Overhead Abstraction !! We have two vtables instead of one and for lots of traits it increases the binary size !!

I think the best that could be done is to stabilize trait object vtable address for the same object ...

[RalfJung]

I think this has been closed by https://github.com/rust-lang/rust/pull/117758, or is there anything left to do?