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

Strange Arc::ptr_eq behaviour (duplicate vtables?) #63021

Closed
TrionProg opened this issue Jul 26, 2019 · 10 comments
Closed

Strange Arc::ptr_eq behaviour (duplicate vtables?) #63021

TrionProg opened this issue Jul 26, 2019 · 10 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@TrionProg
Copy link

TrionProg commented Jul 26, 2019

Hello. This issue is simmilar to #48795 and #46139 but have more strange behaviour and suggestion.

The problem is located when we clone Arc to Arc<Trait> in one function, return Arc and then clone Arc to Arc<Trait> in "parent" function, but Arc::ptr_eq says, that Arc<Trait>(called function) != Arc<Trait>(parent function).

Where is example of this problem, but Arc::ptr_eq works!
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=95eef8742b08ab94f3fbf3048dc514b2

We see, that data pointer is equal, but vtables are different, but implementations are correct.

In my case I have more traits, etc, project in 14k lines, and Arc::ptr_eq do not works, but this objects/traits are located in same crate. Note this problem is still occured in build --release cause, and implementations of vtables eat memory.

So..

  1. Why Arc::ptr_eq match pointers to vtable, but I just need to know, points Arc to object that I search. This behaviour looks strange for user and may cause problems, when project will be large, as in my case (previously it worked correctly).
  2. How should I match Arc-s?
let ptr_equal = unsafe {
                    let (a,_):(usize, usize) = std::mem::transmute_copy(&node.widget_instance);
                    let (b,_):(usize, usize) = std::mem::transmute_copy(widget);

                    a==b // Addresses of ArcInner
                };

or

impl WidgetInstanceTrait {
    pub fn ref_eq(&self, other:&Arc<WidgetTrait>) -> bool {
        self.ptr_eq(&**other)
    }

    pub fn ptr_eq(&self, other:&WidgetTrait) -> bool {
        unsafe {
            let (a,_):(usize, usize) = std::mem::transmute_copy(&self);
            let (b,_):(usize, usize) = std::mem::transmute_copy(&other);

            a==b // Addresses of data (self)
        }
    }
}

and

if node.widget_instance.ptr_eq(&**widget) { ... }
if node.widget_instance.ptr_eq(widget) { ... }
@sfackler
Copy link
Member

vtables are not unique. There can be, for example, a duplicate vtables in each codegen unit that needs one.

@Centril
Copy link
Contributor

Centril commented Jul 26, 2019

Note also that the layout of trait objects is not specified.

@Diggsey
Copy link
Contributor

Diggsey commented Jul 26, 2019

vtables are not unique. There can be, for example, a duplicate vtables in each codegen unit that needs one.

Seems odd to ever use the vtable pointer as part of a comparison then...

@sfackler
Copy link
Member

The documentation for ptr::eq covers why vtable pointers are incorporated in one of the examples: https://doc.rust-lang.org/std/ptr/fn.eq.html.

@jonas-schievink jonas-schievink added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools and removed T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 28, 2019
@Diggsey
Copy link
Contributor

Diggsey commented Jul 29, 2019

The net result is C++-level foot-gunnery. The documentation there says that different implementations may compare differently, but the problem here is that the same implementation is comparing differently depending on which compilation unit converted the concrete type to a trait object.

What's worse is that this exposes an implementation detail. Let's say I have this code:

fn identity(x: Box<dyn Trait>) -> Box<dyn Trait> { x }

If the compiler is able to prove that the function is only called with Box<Foo>, then the compiler would be free to return TraitRepr { ptr: x.ptr(), vtable: MyFooVtable }, even if it was passed a different vtable.

It's difficult to see how ptr::eq could ever be used effectively on a trait object given the lack of any defined semantics?

@Mark-Simulacrum Mark-Simulacrum added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Aug 10, 2019
@Mark-Simulacrum
Copy link
Member

Relabeling as T-lang as this does seem like a bit of an unfortunate result of compiler internals, ideally we'd not create different vtables for different codegen units. Maybe there's some sort of linker or LLVM attribute we could set to deduplicate those?

@vojtechkral
Copy link
Contributor

Hi. We've just ran into this problem (cf. the bug referenced), it seems that, for example, turning off/on incremental compilation is enough to trigger a difference. This is definitely a footgun.

IMO at least the documentation should be updated, it doesn't seem to mention this problem and the documentation for Arc::ptr_eq seems to be simply wrong. On a more positive note, I'm willing to help if need be :)

@RalfJung
Copy link
Member

RalfJung commented Mar 6, 2020

To avoid having many issues open that are all about the same thing, should this be closed in favor of #46139 or #48795 or #69757?

@vojtechkral
Copy link
Contributor

vojtechkral commented Mar 6, 2020

@RalfJung IMO #48795 describes the problem most succintly / without imvolving other not-so-imporant specifics. (So, yeah.)

@RalfJung
Copy link
Member

RalfJung commented Mar 7, 2020

Okay let's close it... but I feel #46139 actually is more to the point.

Closing as duplicate of #46139.

@RalfJung RalfJung closed this as completed Mar 7, 2020
andersk added a commit to andersk/rust that referenced this issue Jan 24, 2021
We sometimes generate duplicate vtables depending on the number of
codegen units (rust-lang#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 rust-lang#63021.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants