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

add note on comparing vtables / function pointers #120880

Merged
merged 2 commits into from
Feb 12, 2024
Merged

add note on comparing vtables / function pointers #120880

merged 2 commits into from
Feb 12, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Feb 10, 2024 •
edited
Loading

Fixes #99388
Fixes #117047

@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2024

r? @cuviper

rustbot has assigned @cuviper.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 10, 2024
cuviper
cuviper reviewed Feb 11, 2024
View reviewed changes
library/core/src/primitive_docs.rs
/// pointers to the same function can compare inequal (because functions are duplicated in multiple
/// codegen units), and pointers to *different* functions can compare equal (since identical
/// functions can be deduplicated within a codegen unit).
///
/// [`Hash`]: hash::Hash
Copy link
Member

Choose a reason for hiding this comment

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

I hadn't considered before -- are we then violating the Hash-Eq contract? Or is it ok because the separate origins of pointers to the "same" function will at least be self-consistent in hashing, and pointers to different deduplicated functions will also hash the same.

That is, if you use function pointers as a HashMap key, they will behave correctly with respect to the address of actual codegen functions, but not their original function in the source code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hash and PartialEq use the same source data, so that contract is maintained.

That is, if you use function pointers as a HashMap key, they will behave correctly with respect to the address of actual codegen functions, but not their original function in the source code.

Correct.

@cuviper
Copy link
Member

cuviper commented Feb 11, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 11, 2024

📌 Commit 1383657 has been approved by cuviper

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2024
@RalfJung RalfJung mentioned this pull request Feb 11, 2024
Closed
@RalfJung
Copy link
Member Author

@bors r-
RawWakerVTable's PartialEq also involves function pointers, seems worth adding the note there as well (as these are private fields and hence the fact that its equality is "odd" is otherwise hidden).

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 11, 2024
@RalfJung
Copy link
Member Author

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 11, 2024
@RalfJung

This comment was marked as duplicate.

Sign in to view
@cuviper
Copy link
Member

cuviper commented Feb 11, 2024

Sure, sounds good.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 11, 2024

📌 Commit aaa6d3b has been approved by cuviper

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 11, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 12, 2024
@bors
Auto merge of rust-lang#120951 - matthiaskrgr:rollup-0nnm7dv, r=matth…
084ce5b 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#110483 (Create try_new function for ThinBox)
 - rust-lang#120740 (Make cmath.rs a single file)
 - rust-lang#120872 (hir: Refactor getters for HIR parents)
 - rust-lang#120880 (add note on comparing vtables / function pointers)
 - rust-lang#120885 (interpret/visitor: ensure we only see normalized types)
 - rust-lang#120888 (assert_unsafe_precondition cleanup)
 - rust-lang#120897 (Encode `coroutine_for_closure` for foreign crates)
 - rust-lang#120937 ([docs] Update armv6k-nintendo-3ds platform docs for outdated info)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3d7d709 into rust-lang:master Feb 12, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 12, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 12, 2024
@rust-timer
Unrolled build for rust-lang#120880
f02e458 
Rollup merge of rust-lang#120880 - RalfJung:vtable-fnptr-partialeq, r=cuviper

add note on comparing vtables / function pointers

Fixes rust-lang#99388
Fixes rust-lang#117047
@RalfJung RalfJung deleted the vtable-fnptr-partialeq branch February 12, 2024 07:15
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 26, 2024
@bors
Auto merge of rust-lang#120951 - matthiaskrgr:rollup-0nnm7dv, r=matth…
036d00b 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#110483 (Create try_new function for ThinBox)
 - rust-lang#120740 (Make cmath.rs a single file)
 - rust-lang#120872 (hir: Refactor getters for HIR parents)
 - rust-lang#120880 (add note on comparing vtables / function pointers)
 - rust-lang#120885 (interpret/visitor: ensure we only see normalized types)
 - rust-lang#120888 (assert_unsafe_precondition cleanup)
 - rust-lang#120897 (Encode `coroutine_for_closure` for foreign crates)
 - rust-lang#120937 ([docs] Update armv6k-nintendo-3ds platform docs for outdated info)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cuviper cuviper cuviper left review comments

Assignees

@cuviper cuviper

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.78.0
4 participants
@RalfJung @rustbot @cuviper @bors