Ignore vtables in {Rc, Arc, Weak}::ptr_eq

[andersk]

We sometimes generate duplicate vtables depending on the number of codegen units (#46139), so we need to exclude the vtable part when comparing fat pointers.

This is safe for the case of Rc, Arc, and Weak because these always point to an entire allocation. These methods can’t be used to compare, e.g., a struct with one of its fields, a slice with one of its subslices, or consecutive ZST fields.

Fixes #63021.

[rust-highfive]

r? @shepmaster

(rust-highfive has picked a reviewer for you, use r? to override)

[pickfire]

Should we add tests for this to prevent regression?

[RalfJung]

This is wrong, I am afraid -- it will also ignore the slice length when comparing [T] pointers.

[andersk]

@RalfJung I know that’s a problem for arbitrary pointer comparisons, but can that be a problem for Rc, Arc, and Weak specifically? AFAIK safe code can’t get Rc<[T; 3]> and Rc<[T; 5]> to point to the same address.

[andersk]

Indeed, even unsafely converting Rc<[T; 5]> to Rc<[T; 3]> with the same address would result an undefined deallocation of the wrong size if the Rc<[T; 5]> were to be dropped before the Rc<[T; 3]>—so that can’t possibly be a valid thing to do, which justifies the assumption that Rc<[T]> of different length must have different addresses.

[RalfJung]

Hm, that's an interesting argument -- Rc necessarily points to a "whole object", never to some part (or subslice) of it.

However, making ptr_eq behave differently than ref1 as *const _ == ref2 as *const _ also sounds like a potentially bad idea (see #46139). This is an ad-hoc fix without really solving the underlying problem. As such I am not convinced that it is a good idea.

We'll have to see what the team thinks.

[m-ou-se]

behave differently

It'd behave the same if the vtable issue was fixed, right? (That is, this change makes ptr_eq work correctly, but it'd behave differently than ptr::eq because of an issue with ptr::eq/vtables.) Or is there any other difference that would remain?

[burdges]

I'd think some new method could provide this, accompanied by the explanation of (a) why this works for reference counted pointers, and (b) how one should compare Weak with Rc or Arc (unimportant for Rc but as Weak for Arc I guess).

We've previously discussed providing Mutex and RwLock types that only worked when pinned. I wonder if one could provide an interface here that exposes pinned equality? I suppose no since one could create a pinned type for which shrinking it made sense.

[RalfJung]

It'd behave the same if the vtable issue was fixed, right? (That is, this change makes ptr_eq work correctly, but it'd behave differently than ptr::eq because of an issue with ptr::eq/vtables.)

I have not seen consensus for what the "correct" behavior of ptr_eq is, or if there even is a bug.

Note that vtables are not the only things that have "strange" equality -- the same is true for function pointers.

[andersk]

The same argument applies to function pointers in Rc, Arc, and Weak: different functions must be in different Rc allocations and therefore have different addresses. The documented behavior of Rc::ptr_eq is “true if the two Rcs point to the same allocation”, and in general, two Rc allocations will always have different addresses.

We could make it clear that fat pointer casting quirks are not relevant here if we instead compared the addresses of the RcBox.strong fields?

[matklad]

Another interesting exalple here is #48814 (comment)

use std::{rc::Rc, mem};

struct A(u8);
#[repr(transparent)]
struct B(A);

trait T {
    fn f(&self);
}
impl T for A {
    fn f(&self) { println!("A") }
}
impl T for B {
    fn f(&self) { println!("B") }
}


fn main() {
    let b = Rc::new(B(A(92)));
    let a = unsafe {
        mem::transmute::<Rc<B>, Rc<A>>(b.clone())
    };
    
    let a: Rc<dyn T> = a;
    let b: Rc<dyn T> = b;
    println!("{}", Rc::ptr_eq(&a, &b));
}

That said, I feel that the current behavior of ptr_eq is broken for !Sized types (it could be return false). We should at least document it.

[SimonSapin]

ptr_eq uses ==, it only exits as a convenience so one does not need to extract the correct *const T pointers. I’m opposed to changing this.

If we’re considering changing the semantics of comparison of wide pointers to trait objects to ignore the vtable address, let’s do it uniformly including for the == operator.

[burdges]

This behavior only seems correct for Arc and Rc and their Weaks. It'll create issues like #46139 (comment) if == ignores the vtable pointer for *const dyn Trait. It's even problematic comparing Weak<dyn Trait>, et al. vs *const dyn Trait this way.

I do think some convenience method for this on Arc and Rc and their Weaks makes sense though, even if it cannot be called ptr_eq.

[shepmaster]

r? @m-ou-se

[SimonSapin]

Review: this PR correctly implements the goal stated in its title and description. However that goal is flawed in the first place since it makes the modified methods inconsistent with std::ptr:eq, as well as PartialEq / the == operator for *const, *mut, and NonNull. I think this PR should be closed, and possibility of (uniformly) changing wide pointer comparison semantics should be proposed to the Lang Team: https://lang-team.rust-lang.org/proposing_a_project.html

[andersk]

@SimonSapin Your review doesn’t seem to acknowledge the above counterarguments:

• The documentation does not suggest that Rc::ptr_eq needs to be equivalent to ptr::eq; it says “Returns true if the two Rcs point to the same allocation (in a vein similar to ptr::eq)”. The ptr::eq documentation makes no mention of allocations, nor should it, because general pointers need not point to an entire allocation.

• This fix is only possible for Rc, Arc, and Weak. Uniformly changing wide pointer comparison semantics is not a possibility, as can be demonstrated entirely in safe Rust:

  trait T { fn f(&self) -> bool; }
  struct A(u32);
  impl T for A { fn f(&self) -> bool { false } }
  struct B(A);
  impl T for B { fn f(&self) -> bool { true } }
  
  fn main() {
      let b = B(A(17));
      let p: &dyn T = &b.0;
      let q: &dyn T = &b;
      assert_eq!(p as *const dyn T as *const u8, q as *const dyn T as *const u8);
      assert_ne!(p.f(), q.f());
  }

  This problem cannot arise for Rc except via unsafe transmute between Rc<A> and Rc<B>. (Hence one of the implicit questions here is whether there is or should be some special case in the unsafe code guidelines that allows transmute between Rc<A> and Rc<B>. It’s definitely not allowed to transmute between general Foo<A> and Foo<B> even with #[repr(transparent)]; for example, the layout might depend on an associated type.)

[SimonSapin]

• Documentation is not an immutable source of absolute truth. This one was changed in Rc: value -> allocation #65505 from "same value", which in turn was added in Add pub fn ptr_eq(this: &Self, other: &Self) -> bool to Rc and Arc #35992 without considering DSTs at all.

  (Source: I wrote that, mentioning a project that uses Rc<Node> where Node: Sized.)

  So I don’t think this precise choice of phrasing should be an argument for changing the behavior.

• Right, always ignoring vtables in pointer comparisons may not be desirable. But that doesn’t make inconsistency desirable in methods whose names imply equivalence.

I agree with RalfJung’s assessment

This is an ad-hoc fix without really solving the underlying problem

and with @burdges’s suggestion to make this a new method if the best-we-can-do-for-now ad-hoc fix for reference-counted things only is still wanted.

[burdges]

I suppose transmute::<Rc<A>,Rc<B>> makes sense sometimes too, so maybe some trait that promises no transmutes elsewhere.

pub unsafe trait NoRcTransmutes {}

impl<T: NoRcTransmutes> Rc<T> {
    pub fn data_ptr_eq(&self, other: Rc<impl NoRcTransmutes>) { .. }
    pub fn data_ptr_eq_weak(&self, other: Weak<impl NoRcTransmutes>) { .. }
}
impl<T: NoRcTransmutes> Weak<T> {
    pub fn data_ptr_eq(&self, other: Rc<impl NoRcTransmutes>) { .. }
}

Could one instead just add some trick that deduplicates the vtables during codegen without doing -C codegen-units=1? All codegen units could output their vtables seperately for some later merger stage? If I understand, all these repeated vtables waste considerable space, not because vtable take much space, but because they imply redundent monomorpized code, yes? If so, then merging vtables should permit the linker to strip all this redundent code too.

[andersk]

This is exactly what LLVM linkonce (or linkonce_odr) linkage is for, right? I kind of assumed that #46139 wouldn’t have been open for three years if it were that simple to fix, but maybe I’m wrong.

[burdges]

If vtables cannot be inlined then linkonce should not have any downsides, right?

[m-ou-se]

We've discussed this issue in the library team meeting a few days ago. Those of use that attended the meeting agreed with @andersk: The documentation specifies "same allocation", and it makes sense for this function to effectively compare the address of the counters, as opposed to that of the value itself. (Just like owner-based comparison for shared_ptr/weak_ptr in C++.)

That'd mean that the example in #80505 (comment) should return true: It's the same allocation, 'owned' by the same counters.

@SimonSapin @RalfJung What do you think?

[m-ou-se]

This is misleading, as this is not just because of that issue. If we're deciding now that ptr_eq compares the allocation (like the documentation says), we will not change this back if that issue ever gets solved. This change will make the example in #80505 (comment) return true on purpose. (Which would also make a good test case.)

[andersk]

My understanding was that we’re deeming that example an incorrect use of transmute (as per #80505 (comment)), and thus making no guarantee whether it returns true or false?

[Dylan-DPC-zz]

@m-ou-se the author needs help, after that we can move this ahead

[burdges]

@m-ou-se As I understand it, your statement is incorrect. There are no other reasons besides #46139 for doing this.

In fact, after a better solution to #46139 appears then reverting this change might reduce bugs.

[JohnCSimon]

triage:
@andersk - assigning to reviewer

@rustbot label: +S-waiting-on-review -S-waiting-on-author

[burdges]

I still disagree that transmuting Rc and Arc should be discarded since they often contain large data. We've no idea what's out there in the ecosystem, but even if we did then doing this still requires a name that encourages reading documentation, like data_ptr_eq or whatever.

[Dylan-DPC-zz]

@m-ou-se this is ready for review

[Dylan-DPC-zz]

given that the team isn't in favour of this change in the current form, I'm going to close this PR. If you can find a better way to solve this, feel free to open a new PR and we can take this forward from there. Thanks for taking the time to contribute.

[andersk]

I’m confused. What has changed since #80505 (comment)?

This PR fixes a bug, and there seems to be agreement that it should be fixed. There’s an apparent controversy about whether we should document additional guarantees about the behavior under transmute::<Rc<A>, Rc<B>>. But since we currently make no guarantees that such a transmute can ever be valid, that would constitute a larger expansion of the language semantics, not just a library bug fix. Shouldn’t we fix the bug and then perhaps consider those additional guarantees separately? Is my interpretation wrong?

[rib]

I'm also a bit confused that this was closed - as @andersk noted this is surely a bug fix (seemingly quite a serious one at that even)?

As mentioned earlier the documentation for Arc::ptr_eq concisely states:

Returns true if the two Arcs point to the same allocation (in a vein similar to ptr::eq).

Those are the semantics I've relied on in multiple projects but its only since I recently ran clippy on one of those projects that I discovered that I've shot myself in the foot here because that's not what Arc::ptr_eq does if the Arc holds a dynamically sized type.

Being able to reliably tell whether two Arcs point back to the same shared memory allocation is surely a really important/basic test that many projects might reasonably expect to trust ptr_eq for? (especially since that's precisely what the documentation for Arc::ptr_eq states that it does)

[RalfJung]

ptr::eq of dyn trait raw pointers also compares the vtables. So ignoring the vtables here would actually be against the documentation.

[rib]

ignoring the vtables doesn't seem like it's against the docs for Arc::ptr_eq

Saying that the comparison is done in a "vein similar to ptr::eq" reads to me as only implying a superficial similarity, without setting any strict expectation about semantics. "vein similar" is very weak, non-assertive language.

On the other hand saying that it "Returns true if the two Arcs point to the same allocation" seems like a very clear and precise description of what to expect.

Personally I interpreted it as enough of a similarity that the two functions both deal with pointer comparisons, and would expect to read the docs for each separately if I wanted to check the specific semantics.

[kpreid]

Another consideration: Rust may have dyn upcasting in the future. This means that it will be possible to take two Arc/Rc of different types that point to the same allocation (say, some sort of “sender” and “receiver”), convert them to the same type, and compare them. This is a straightforward and meaningful thing to do, but in order to do it in Rust today, you either have to hope the compiler does both coercions in the same codegen unit so they share a vtable (plausible, for a single function, but not specified to be so), or ignore Rc::ptr_eq and play with pointers directly, missing the convenience and documentation ofRc::ptr_eq. On this basis, I'd argue that if Rc::ptr_eq is not to have this semantics, then another function should exist that does.

#![feature(trait_upcasting)]
use std::rc::Rc;

trait A {}
trait B: A {}
trait C: A {}
impl A for i32 {}
impl B for i32 {}
impl C for i32 {}

fn main() {
    let x = Rc::new(1);
    
    let y: Rc<dyn B> = x.clone();
    let z: Rc<dyn C> = x.clone();
    
    // This assertion is true in practice but is not currently guaranteed to be,
    // and would be false sometimes if in a function that accepted `Rc<dyn A>`s as parameters.
    assert!(Rc::ptr_eq(&(y as Rc<dyn A>), &(z as Rc<dyn A>)));
}

[Edit, correction/clarification: It's also possible to do this in Rust today without trait upcasting, when one Rc is a concrete type and the other is dyn Something. Trait upcasting will merely offer more situations in which this is a thing one could think of trying to do.]

[RalfJung]

FWIW, based on the discussion above, the libs team seems to agree that comparing the pointer without its metadata is usually what one wants. The problem is that it is really surprising if Rc::ptr_eq(x, y) is different from ptr::eq(Rc::as_ptr(x), Rc::as_ptr(y)), and it is also surprising if ptr::eq(x, y) is different from x == y. Changing the behavior of == to ignore metadata is probably out of the question since there will be code relying on its current behavior. So far nobody has made a good suggestion for how to resolve this conflict.

Either way, a closed PR is also a bad place to discuss anything. I'd suggest opening a new issue for this, possibly a Zulip thread, and/or a libs-abi team ACP.