core: compare callsite::Identifier data pointers only
#749
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Clippy [now warns us][1] that comparing trait object pointers with `ptr::eq` is bad, because vtable addresses are not guaranteed to be unique due to the compiler merging vtables, and vtables for the same trait impl may have different addresses in different compilation units. In practice, I don't believe this actually effects `tracing-core`'s use case for this comparison. Because callsites must be static, the data component of the trait object wide pointer will always be unique, even if the compiler merges the vtables for all the identical generated `Callsite` impls (which it may very well do!). Although this is probably not an issue in practice, I still thought it was good to fix the clippy warning, and to be more explicit that it's the data pointer comparison that's load-bearing here. I've updated the `PartialEq` impl for `callsite::Identifier` to cast to `*const ()` to extract the data address from the wide pointer. [1]: https://rust-lang.github.io/rust-clippy/master/index.html#vtable_address_comparisons Signed-off-by: Eliza Weisman <eliza@buoyant.io>
tobz
approved these changes
Jun 12, 2020
ericjheinz
pushed a commit
to ericjheinz/tracing
that referenced
this pull request
Jun 12, 2020
## Motivation Clippy [now warns us][1] that comparing trait object pointers with `ptr::eq` is bad, because vtable addresses are not guaranteed to be unique due to the compiler merging vtables, and vtables for the same trait impl may have different addresses in different compilation units. In practice, I don't believe this actually effects `tracing-core`'s use case for this comparison. Because callsites must be static, the data component of the trait object wide pointer will always be unique, even if the compiler merges the vtables for all the identical generated `Callsite` impls (which it may very well do!). ## Solution Although this is probably not an issue in practice, I still thought it was good to fix the clippy warning, and to be more explicit that it's the data pointer comparison that's load-bearing here. I've updated the `PartialEq` impl for `callsite::Identifier` to cast to `*const ()` to extract the data address from the wide pointer. [1]: https://rust-lang.github.io/rust-clippy/master/index.html#vtable_address_comparisons Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Jul 8, 2020
Changed - Replaced use of `inner_local_macros` with `$crate::` (#729) Added - `must_use` warning to guards returned by `dispatcher::set_default` (#686) - `fmt::Debug` impl to `dyn Value`s (#696) - Functions to convert between `span::Id` and `NonZeroU64` (#770) - More obvious warnings in documentation (#769) Fixed - Compiler error when `tracing-core/std` feature is enabled but `tracing/std` is not (#760) - Clippy warning on vtable address comparison in `callsite::Identifier` (#749) - Documentation formatting issues (#715, #771) Thanks to @bkchr, @majecty, @taiki-e, @nagisa, and @nqvz for contributing to this release!
hawkw
added a commit
that referenced
this pull request
Jul 8, 2020
### Changed - Replaced use of `inner_local_macros` with `$crate::` (#729) ### Added - `must_use` warning to guards returned by `dispatcher::set_default` (#686) - `fmt::Debug` impl to `dyn Value`s (#696) - Functions to convert between `span::Id` and `NonZeroU64` (#770) - More obvious warnings in documentation (#769) ### Fixed - Compiler error when `tracing-core/std` feature is enabled but `tracing/std` is not (#760) - Clippy warning on vtable address comparison in `callsite::Identifier` (#749) - Documentation formatting issues (#715, #771) Thanks to @bkchr, @majecty, @taiki-e, @nagisa, and @nvzqz for contributing to this release!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Clippy now warns us that comparing trait object pointers with
ptr::eqis bad, because vtable addresses are not guaranteed to beunique due to the compiler merging vtables, and vtables for the same
trait impl may have different addresses in different compilation units.
In practice, I don't believe this actually effects
tracing-core's usecase for this comparison. Because callsites must be static, the data
component of the trait object wide pointer will always be unique, even
if the compiler merges the vtables for all the identical generated
Callsiteimpls (which it may very well do!).Solution
Although this is probably not an issue in practice, I still thought it
was good to fix the clippy warning, and to be more explicit that it's
the data pointer comparison that's load-bearing here. I've updated the
PartialEqimpl forcallsite::Identifierto cast to*const ()toextract the data address from the wide pointer.
Signed-off-by: Eliza Weisman eliza@buoyant.io