[kpreid]

View all comments

(These changes are all intended to be “it obviously should be this”, not “let’s choose to allow this”. If we don’t want to allow one of these things, I will be happy to revise this text to be stricter.)

• Allow FnPtr::from_ptr() to be used for FFI/linking applications and not just Rust-to-Rust type erasure.

• Specify that calling FnPtr::from_ptr() with an invalid pointer is not immediate UB.

  (T-opsem decided the relevant function pointer validity at Decide on the validity of function pointers unsafe-code-guidelines#440)

Tracking issue for fn_static: #148768

@rustbot label +A-docs +T-opsem +T-libs

[rustbot]

r? @clarfonthey

rustbot has assigned @clarfonthey.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from JohnTitor, Mark-Simulacrum, clarfonthey

[theemathas]

cc @rust-lang/opsem, due to guaranteeing new things in user-facing documentation, although in an unstable API.

[asquared31415]

Should we perhaps also link to the docs talking about the hazards of function pointers? In particular, NonNull<Code> implements PartialEq, but it has the same problems as "normal" function pointers fn() with regards to comparisons and identity.

(also the wrong lint fires when you try to equate NonNull<Code>, the wide pointer lint fires instead of the fn pointer comparison lint, but that should be fixed in a different PR)

View changes since the review

[clarfonthey]

That seems fair. Potentially something like, in general, you should not expect to glean any useful information from this pointer besides converting it back into a function, and to link fn_addr_eq to explain pitfalls.

(whose docs I don't think have been updated yet, but will be)

[RalfJung]

ABI-compatible with what?

View changes since the review

[RalfJung]

I am not sure if this is correct. Self here may be e.g. fn(i32) -> i32, right? In that case the constructor needs to require that this is a safe function with that signature. Being ABI-compatible is not enough.

[clarfonthey]

Yeah, I'm not entirely sure this is correct either. It feels like we're back into the fn_addr_eq discussion again, where you really can't be sure that functions are compatible because the compiler might insert invisible nonsense in the prelude.

[RalfJung]

I don't think this has anything to do with fn_addr_eq.

It's just that the safety invariant for fn(i32) -> i32 is "when this function is invoked with an i32, it will run without causing UB and if it returns, the result is an i32". The comment that's here right now is not strong enough to justify this property.

[RalfJung]

Suggested change

	/// everything else about the type except except the fact that it was a function pointer.
	/// everything else about the function `self` points to.

NonNull<Code> can be safely constructed to point to non-functions so arguably this forgets everything.

View changes since the review

[RalfJung]

Suggested change

	/// Returns the address of the function `self` points to.

View changes since the review

[RalfJung]

Suggested change

	/// `ptr` must have been obtained from calling [`FnPtr::as_ptr`] on a function pointer type
	/// `ptr` must have been obtained by calling [`FnPtr::as_ptr`] on a function pointer type

View changes since the review

[RalfJung]

cc @rust-lang/opsem, due to guaranteeing new things in user-facing documentation, although in an unstable API.

I am not sure what the opsem part of this is, this is mostly changing the docs on a trait whose invariant is library-defined, no?

[clarfonthey]

I was under the impression that the machinery of FnPtr is firmly in opsem territory even though the library is free to further restrict the API out of caution. So, effectively, the upper bound on guarantees is controlled by opsem, but that libs may choose to further lower that bound.

[RalfJung]

When the trait is implemented for fn types, then from the opsem side any non-null value is fine for from_ptr. That's the validity invariant. But from a soundness side that's obviously not enough, the safety invariant is stronger than that. But safety invariants aren't really opsem any more, they are lang/types at least as much as they are opsem (in terms of who gets to decide about them). Though opsem folks tend to know about them so we can still help. :)

I have no idea which other types the trait is or will be implemented for.

[theemathas]

@RalfJung In the initial version of this PR, there was a new guarantee is that FnPtr::from_ptr() on any arbitrary non-null pointer is not immediately UB. I'm not sure if the current version has any new guarantees.

[clarfonthey]

In the initial version of this PR, there was a new guarantee is that FnPtr::from_ptr() on any arbitrary non-null pointer is not immediately UB. I'm not sure if the current version has any new guarantees.

Yeah, from a libs perspective that's just incorrect. There is the strong boundary of language UB where the compiler is immediately allowed to eat your laundry, but in the standard library we like to make stronger guarantees so that safe code can't invoke UB. Even if it's not "immediately" UB, having a function pointer that cannot be called is a liability that we don't want to offer.

[clarfonthey]

@rustbot author

(since there are still open questions)

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[theemathas]

There is the strong boundary of language UB where the compiler is immediately allowed to eat your laundry, but in the standard library we like to make stronger guarantees so that safe code can't invoke UB.

@clarfonthey Turns out there's already precedent for making a distinction between "not immediately UB, but may cause UB later" and "all bets are immediately off" in the standard library. See #134598

[RalfJung]

I don't see such a distinction there? That is just spelling out more carefully what may or may not later be UB.

[clarfonthey]

The precedent I was referring to is specifically in the assert_unsafe_precondition macro and we're choosing to loosen the requirements in that location specifically after a long and arduous debate. I don't think we should do the same here.

[RalfJung]

It's long discussion but assert_unsafe_precondition does not seem to be mentioned anywhere in the PR or the diff. So I have no idea what you are referring to.

[clarfonthey]

I was referring to the context of the quote @theemathas made:

There is the strong boundary of language UB where the compiler is immediately allowed to eat your laundry, but in the standard library we like to make stronger guarantees so that safe code can't invoke UB.

This was me referring to the current distinction which is most well codified in the assert_unsafe_precondition macro, between language and library UB. Language UB checks are turned off for miri (which does its own checking) but library UB checks are still turned on in all cases.

[RalfJung]

Turns out there's already precedent for making a distinction between "not immediately UB, but may cause UB later" and "all bets are immediately off" in the standard library. See #134598

So I still don't understand what the PR has to do with that but &str is an example of that, where we document that UTF-8 is a library/safety invariant, not a validity invariant. When working with Pin it is also quite common to have &mut references that are not actually satisfying their full safety invariant. There is a number of situations in Rust where one may have to hold a value that's "tainted" by having some of its key properties broken, but a few key operations are still possible in this "tainted" state. We are currently terrible at documenting this, but it's not something we can just pretend won't happen.

[clarfonthey]

Right, and I guess my main issue communicating here is that I don't think that we should perform the exception here, where the distinction between when UB happens should be part of a guarantee.

In general, the library team likes to guarantee as few possible exceptions to invariants as possible, and str is one case where these exceptions are useful. FnPtr is not only not one of those cases where it's useful to make this distinction, but additionally, there is the potential for just outright making this a case where immediate language UB occurs (e.g., niches requiring certain values) and thus libs wouldn't even have a say in that case. It's just always UB.