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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions library/alloc/src/rc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1002,7 +1002,9 @@ impl<T: ?Sized> Rc<T> {
///
/// [`ptr::eq`]: core::ptr::eq
pub fn ptr_eq(this: &Self, other: &Self) -> bool {
this.ptr.as_ptr() == other.ptr.as_ptr()
// The *const u8 cast discards the vtable to work around
// https://github.com/rust-lang/rust/issues/46139
Comment on lines +1005 to +1006
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@burdges burdges Mar 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

this.ptr.as_ptr() as *const u8 == other.ptr.as_ptr() as *const u8
}
}

Expand Down Expand Up @@ -2136,7 +2138,9 @@ impl<T: ?Sized> Weak<T> {
#[inline]
#[stable(feature = "weak_ptr_eq", since = "1.39.0")]
pub fn ptr_eq(&self, other: &Self) -> bool {
self.ptr.as_ptr() == other.ptr.as_ptr()
// The *const u8 cast discards the vtable to work around
// https://github.com/rust-lang/rust/issues/46139
self.ptr.as_ptr() as *const u8 == other.ptr.as_ptr() as *const u8
}
}

Expand Down
8 changes: 6 additions & 2 deletions library/alloc/src/sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1066,7 +1066,9 @@ impl<T: ?Sized> Arc<T> {
///
/// [`ptr::eq`]: core::ptr::eq
pub fn ptr_eq(this: &Self, other: &Self) -> bool {
this.ptr.as_ptr() == other.ptr.as_ptr()
// The *const u8 cast discards the vtable to work around
// https://github.com/rust-lang/rust/issues/46139
this.ptr.as_ptr() as *const u8 == other.ptr.as_ptr() as *const u8
}
}

Expand Down Expand Up @@ -1936,7 +1938,9 @@ impl<T: ?Sized> Weak<T> {
#[inline]
#[stable(feature = "weak_ptr_eq", since = "1.39.0")]
pub fn ptr_eq(&self, other: &Self) -> bool {
self.ptr.as_ptr() == other.ptr.as_ptr()
// The *const u8 cast discards the vtable to work around
// https://github.com/rust-lang/rust/issues/46139
self.ptr.as_ptr() as *const u8 == other.ptr.as_ptr() as *const u8
}
}

Expand Down
17 changes: 17 additions & 0 deletions library/alloc/tests/arc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,3 +195,20 @@ fn shared_from_iter_trustedlen_no_fuse() {
assert_trusted_len(&iter);
assert_eq!(&[Box::new(42), Box::new(24)], &*iter.collect::<Rc<[_]>>());
}

mod other_mod {
use std::sync::Arc;

pub fn cast(r: Arc<()>) -> Arc<dyn Send> {
r
}
}

#[test]
fn ptr_eq_ignores_duplicated_vtable() {
let a = Arc::new(());
let b = other_mod::cast(Arc::clone(&a));
let c: Arc<dyn Send> = a;
assert!(Arc::ptr_eq(&b, &c));
assert!(Weak::ptr_eq(&Arc::downgrade(&b), &Arc::downgrade(&c)));
}
17 changes: 17 additions & 0 deletions library/alloc/tests/rc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,3 +191,20 @@ fn shared_from_iter_trustedlen_no_fuse() {
assert_trusted_len(&iter);
assert_eq!(&[Box::new(42), Box::new(24)], &*iter.collect::<Rc<[_]>>());
}

mod other_mod {
use std::rc::Rc;

pub fn cast(r: Rc<()>) -> Rc<dyn Send> {
r
}
}

#[test]
fn ptr_eq_ignores_duplicated_vtable() {
let a = Rc::new(());
let b = other_mod::cast(Rc::clone(&a));
let c: Rc<dyn Send> = a;
assert!(Rc::ptr_eq(&b, &c));
assert!(Weak::ptr_eq(&Rc::downgrade(&b), &Rc::downgrade(&c)));
}