-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
fn_addr_eq: we actually can guarantee basically nothing #162140
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2542,47 +2542,46 @@ pub fn addr_eq<T: PointeeSized, U: PointeeSized>(p: *const T, q: *const U) -> bo | |
| /// This is the same as `f == g`, but using this function makes clear that the potentially | ||
| /// surprising semantics of function pointer comparison are involved. | ||
| /// | ||
| /// There are **very few guarantees** about how functions are compiled and they have no intrinsic | ||
| /// There are **basically no guarantees** about how functions are compiled as they have no intrinsic | ||
| /// “identity”; in particular, this comparison: | ||
| /// | ||
| /// * May return `true` unexpectedly, in cases where functions are equivalent. | ||
| /// * May return `true` unexpectedly, even in cases where functions are not logically equivalent, | ||
| /// as long as they compile to the same machine code. | ||
| /// | ||
| /// For example, the following program is likely (but not guaranteed) to print `(true, true)` | ||
| /// when compiled with optimization: | ||
| /// | ||
| /// ``` | ||
| /// use std::hint::assert_unchecked; | ||
| /// | ||
| /// let f: fn(i32) -> i32 = |x| x; | ||
| /// let g: fn(i32) -> i32 = |x| x + 0; // different closure, different body | ||
| /// let h: fn(u32) -> u32 = |x| x + 0; // different signature too | ||
| /// dbg!(std::ptr::fn_addr_eq(f, g), std::ptr::fn_addr_eq(f, h)); // not guaranteed to be equal | ||
| /// let g: fn(u32) -> u32 = |x| x + 0; // different type, different body | ||
| /// let h: unsafe fn(i32) -> i32 = |x| unsafe { assert_unchecked(x != 0); x }; // different unsafe preconditions | ||
| /// // These three functions are allowed but not guaranteed to compare equal. | ||
| /// dbg!(std::ptr::fn_addr_eq(f, g), std::ptr::fn_addr_eq(f, h)); | ||
| /// ``` | ||
| /// | ||
| /// In other words, functions that compare equal can have different behavior! Even if | ||
| /// `fn_addr_eq(f, h)` is true, it is still undefined behavior to call `h(0)`, while `f(0)` is | ||
| /// safe to call. | ||
| /// | ||
| /// The explanation for this is that function pointers have [provenance][crate::ptr#provenance]. | ||
| /// If functions have the same assembly and get merged, their provenance decides which variant of | ||
| /// the function will be called. `f` and `h` therefore end up with the same address but different | ||
| /// provenance, and `fn_addr_eq` only compares the address. | ||
| /// | ||
| /// * May return `false` in any case. | ||
| /// | ||
| /// This is particularly likely with generic functions but may happen with any function. | ||
| /// (From an implementation perspective, this is possible because functions may sometimes be | ||
| /// processed more than once by the compiler, resulting in duplicate machine code.) | ||
| /// | ||
| /// Despite these false positives and false negatives, this comparison can still be useful. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One potentially remaining use we were able to come up with is a set of callbacks. If deduplicating additions to the set by "the runtime behavior should happen at least once" works for you then this should work fine.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That seems incredibly niche. Would you like to see it spelled out in the docs?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, the more general idea was that it might be helpful to also have a list of the few things that it can be used for, it might help users to figure out which side they fall on. That's just the only practical thing that came to mind.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't cases like the "different unsafe preconditions" example above cause incorrect results for this kind of use case? Equality returning true does not guarantee that they have the same behavior, so deduplicating in that scenario is not correct.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the callbacks are all required to be safe to call, then calling fewer of them due to deduplication cannot introduce UB. There is some sense in which equal functions have the same behavior -- they have the same asm code, after all. We might be able to give a guarantee of the sort "if neither function causes UB for the given arguments, then calling Cc @rust-lang/opsem
This comment was marked as resolved.
Sorry, something went wrong.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok, having reread that... I guess that depends on whether the comparison lets the function addresses escape like black_box would? Then the optimizer can't remove their bodies. I think even whole-program optimization wouldn't allow it since something could temporarily encrypt the bits of the pointers in some way.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The example perfectly demonstrates my argument from #162140 (comment). Those functions are never called. Their bodies could be optimized out as unreachable, function merged, and function pointer would compare equal.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm... I don't think LLVM considers this a legal transformation today, but I'm also not sure how to forbid it in the spec. I have removed the example.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
LLVM won't do that transform today, but I think it would be legal? Comparison only escapes the address, not the provenance, so the function cannot be called, and optimizing it to unreachable is allowed. |
||
| /// Specifically, if | ||
| /// | ||
| /// * `T` is the same type as `U`, `T` is a [subtype] of `U`, or `U` is a [subtype] of `T`, and | ||
| /// * `ptr::fn_addr_eq(f, g)` returns true, | ||
| /// | ||
| /// then calling `f` and calling `g` will be equivalent. | ||
| /// | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ``` | ||
| /// use std::ptr; | ||
| /// | ||
| /// fn a() { println!("a"); } | ||
| /// fn b() { println!("b"); } | ||
| /// assert!(!ptr::fn_addr_eq(a as fn(), b as fn())); | ||
| /// ``` | ||
| /// | ||
| /// [subtype]: https://doc.rust-lang.org/reference/subtyping.html | ||
| /// These limitations imply that comparing function pointers (via this function or via `==`) is only | ||
| /// useful in extremely niche circumstances. For instance, if a library maintains a set of | ||
| /// callbacks, and doesn't guarantee which of them are actually called (i.e., callbacks can be | ||
| /// spuriously skipped), then it can be legitimate to deduplicate callbacks based on `fn_addr_eq`. | ||
| /// This can never introduce undefined behavior because it only skips executing some of the | ||
| /// registered callbacks. | ||
| #[stable(feature = "ptr_fn_addr_eq", since = "1.85.0")] | ||
|
Comment on lines
+2579
to
2585
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe that this is not legitimate. Suppose that one of the callbacks exposes the provenance of one pointer, while the other callback doesn't, and they have the same machine code. This deduplication can cause a pointer's provenance to not be exposed, even though it would have been exposed without the deduplication. This can technically increase the amount of UB.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the library just says "I will call some of these callbacks but not all of them", that's legitimate.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar issues can happen without exposed provenance if one of the callbacks mutates some byte by adding provenance to it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to understand how that might cause problems where you run fewer copies of a callback than otherwise expected, though. Like, if the provenance is needed and isn't provided.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So this amounts to the library basically saying "callbacks may spuriously just not be run for arbitrary reasons". I don't know when that is ever useful, but it's a spec one can write and deduplication with I have edited the text to make this more clear. I'm still not sure this example is worth actually spelling out, but t-libs seems to prefer having an extremely artificial example over not having any example.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be clear, I'm not 100% sure that's a blocking opinion from libs, just, we really struggled to find examples and wanted to justify this function's existence somehow for a reason other than just documenting how bad function equality is. It does still feel worth having some kind of attribute like Since, well, the main issue is that we know that if we don't offer this function, people are going to try and find a way to hack in its functionality anyway, e.g. casting to non-function pointers. So, we might as well offer a better way if it's already possible, but still heavily discourage it.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't disagree, but I'm not signing up for being the one to implement that.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe just keep:
...and cut the rest? |
||
| #[inline(always)] | ||
| #[must_use = "function pointer comparison produces a value"] | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.