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

Revert recent change to avoid using Arc:ptr_eq #29

Merged
merged 1 commit into from
Dec 20, 2022
Merged

Revert recent change to avoid using Arc:ptr_eq #29

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
32 changes: 8 additions & 24 deletions src/verifier.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,31 +57,15 @@ struct VerifierClosure {
impl Eq for VerifierClosure {}
impl PartialEq for VerifierClosure {
fn eq(&self, other: &Self) -> bool {
// WARNING:
// The libs team has resolved to update Arc::ptr_eq so that it doesn't compare
// meta data for the wrapped value (such as a dyn trait vtable):
// https://github.com/rust-lang/rust/issues/103763
//
// `Arc::ptr_eq(&self.func, &other.func)` is effectively implemented as:
//
// `Arc::as_ptr(&arc_a) == Arc::as_ptr(&arc_b)`
//
// which will notably result in comparing the vtable pointer for (wide) trait object pointers
// and does _not_ match the API documentation that states:
//
// > Returns true if the two Arcs point to the same allocation (in a vein similar to ptr::eq).
//
// (which is what we need here)
//
// See: https://github.com/rust-lang/rust/pull/80505
//
// To ensure we are comparing the data pointer component of any wide pointer then we
// additionally cast `Arc::as_ptr(&arc)` to a thin pointer to discard the vtable
//
// This avoid getting a `clippy::vtable_address_comparisons` error
//
// Ref: https://github.com/rust-lang/rust-clippy/issues/6524

let a = Arc::as_ptr(&self.func) as *const u8;
let b = Arc::as_ptr(&other.func) as *const u8;
std::ptr::eq(a, b)
// In the meantime we can silence this clippy suggestion since the vtable is
// benign in this case anyway:
// https://github.com/rust-lang/rust-clippy/issues/6524
#[allow(clippy::vtable_address_comparisons)]
Copy link

Choose a reason for hiding this comment

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

You can drop this now, the fix seems to have landed in 1.72 😬

rust-lang/rust#106450

Copy link
Owner Author

Choose a reason for hiding this comment

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

heh, small world.

thanks for the update.

Copy link

Choose a reason for hiding this comment

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

Thank you for backlinking the whole thing, I ran into this for an unrelated (non-Android 😉) project and wouldn't have otherwise found that it was addressed in Rust 1.72 without existing clippy issues being updated.

Arc::ptr_eq(&self.func, &other.func)
}
}
impl std::fmt::Debug for VerifierClosure {
Expand Down