Re: [PATCH v3 2/2] rust: types: `Opaque` doc: Add some destructor description

[Boqun Feng <boqun.feng@gmail.com>]

On Thu, Mar 06, 2025 at 10:45:00AM +0000, Benno Lossin wrote:
> On Thu Mar 6, 2025 at 10:42 AM CET, Andreas Hindborg wrote:
> > "Boqun Feng" <boqun.feng@gmail.com> writes:
> >
> >> On Wed, Mar 05, 2025 at 10:07:33PM +0100, Andreas Hindborg wrote:
> >>> "Boqun Feng" <boqun.feng@gmail.com> writes:
> >>>
> >> [...]
> >>> >> >> >  rust/kernel/types.rs | 26 +++++++++++++++++++++-----
> >>> >> >> >  1 file changed, 21 insertions(+), 5 deletions(-)
> >>> >> >> >
> >>> >> >> > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> >>> >> >> > index af30e9c0ebccb..f370cdb48a648 100644
> >>> >> >> > --- a/rust/kernel/types.rs
> >>> >> >> > +++ b/rust/kernel/types.rs
> >>> >> >> > @@ -271,17 +271,33 @@ fn drop(&mut self) {
> >>> >> >> >
> >>> >> >> >  /// Stores an opaque value.
> >>> >> >> >  ///
> >>> >> >> > -/// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
> >>> >> >> > +/// [`Opaque<T>`] opts out of the following Rust language invariants for the contained `T`
> >>> >> >> > +/// by using [`UnsafeCell`]:
> >>> >> >> >  ///
> >>> >> >> > -/// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
> >>> >> >> > -/// It gets rid of all the usual assumptions that Rust has for a value:
> >>> >> >> > +/// * Initialization invariant - the contained value is allowed to be uninitialized and
> >>> >> >> > +///   contain invalid bit patterns.
> >>> >> >> > +/// * Immutability invariant - [`Opaque<T>`] allows interior mutability.
> >>> >> >> > +/// * Uniqueness invariant - [`Opaque<T>`] allows aliasing of shared references.
> >>> >> >>
> >>> >> >> This last one is wrong (I know it's probably my fault, sorry). It should be:
> >>> >> >>
> >>> >> >>   /// * Uniqueness invariant - [`Opaque<T>`] allows aliasing of **exclusive** references.
> >>> >> >>
> >>> >> >
> >>> >> > Hmm... are we trying to say "`&mut Opaque<T>` still cannot mean
> >>> >> > noalias" here? If so I feel the wording might be confusing. I would
> >>> >> > suggest we don't use "uniqueness" here. Maybe something like:
> >>> >> >
> >>> >> > * Always aliased invariant - `&mut` [`Opaque<T>`] is not necessarily a
> >>> >> >   unique pointer, and thus the compiler cannot just make aliasing
> >>> >> >   assumptions.
> >>> >> >
> >>> >> > (I use the wording from UnsafePinned[1], maybe there could be better
> >>> >> > wording)
> >>> >> >
> >>> >> > [1]: https://rust-lang.github.io/rfcs/3467-unsafe-pinned.html#summary
> >>> >>
> >>> >> I like my wording better. It says the same with fewer words, and we have
> >>> >> a more wordy section below anyway. We could combine:
> >>> >>
> >>> >>  * Uniqueness invariant - [`Opaque<T>`] allows aliasing of **exclusive**
> >>> >>    references. That is, `&mut` [`Opaque<T>`] is not necessarily a unique
> >>> >>    pointer.
> >>> >>
> >>> >>
> >>> >> How is that?
> >>> >>
> >>> >
> >>> > I think my biggest problem is the word "allows", the phrase "allows
> >>> > aliasing" sounds like Opaque<T> provide something *optional*, however,
> >>> > as I understand it, Opaque<T> here just disallows certain optimization
> >>> > from compiler, in other words, programmers want to use `Opaque<T>` to
> >>> > forbid something, hence "allows" may not be the good word to use here?
> >>> >
> >>> > How about:
> >>> >
> >>> >   * Uniqueness invariant - [`Opaque<T>`] disallow alias assumptions
> >>> >     made by compiler on an *exclusive* references.
> >>>
> >>> I don't object, but I feel like we should be able to define this within
> >>> the context of the Rust language, without going into details about
> >>> compiler internals.
> >>>
> >>> Without `Opaque` it is illegal to have aliased `&mut T`. With `Opaque`,
> >>> it is legal to have aliased `&mut Opaque<T>`. Or at least that is my
> >>> understanding.
> >>>
> >>
> >> Oh, I see. That's why you used "allows", however, allow me to explain
> >> why it looks confusing to me. For `&mut T`, we can still have an
> >> aliasing pointer (a pointer that points to the same variable as the
> >> mutable reference):
> >>
> >> 	let mut x = ...;
> >>
> >> 	let mut_ref = &mut x;
> >> 	let aliased_ptr = mut_ref as *mut _;
> >>
> >> it's just that if we dereference the pointer while the `&mut T` exists,
> >> it's UB, e.g.
> >>
> >> 	unsafe { *aliased_ptr = ... };
> >
> > Right.
> >
> >>
> >> and `Opaque<T>` allows above to be not a UB (there still are rules like
> >> we cannot have data races, violating these rules would still be UB).
> >
> > Yes.
> >
> >> So to me "aliasing" can mean "having an aliasing pointer", and that as
> >> shown above can be provided by both non-Opaque and Opaque cases. I
> >> wonder whether "allow aliased/aliasing accesses" is better than "allow
> >> aliasing" here.
> 
> I disagree, to me "aliasing pointer" means having two pointers that
> point to the same value and that you can use in an interleaved fashion
> for writes and reads.
> 
> > Yes, I think that would be better.
> >
> > I had another thing in mind when I wrote the text, and I am wondering if
> > it is incorrect. As far as I know it is normally illegal to have more
> > than one `&mut T` to the same `T`, even if no access occur through these
> > references.
> 
> The rules aren't completely set and IIRC you *can* create multiple
> references as long as the value they point to is not read or written to
> while they exist. But I wouldn't bet anything on that...
> 
> > With `Opaque<T>`, is it still illegal to have two `&mut Opaque<T>` to
> > the same `T` in existence? I thought it was not illegal in this case.
> 
> Yes it is legal to have multiple `&mut Opaque<T>` pointing to the same
> value (and writing/reading through the `get` method or directly by
> having another `Opaque<T>` value).
> 
> > At any rate, your wording is probably the most accurate:
> >
> >   * Uniqueness invariant - [`Opaque<T>`] disallow alias assumptions
> >     made by compiler on an *exclusive* references.
> >
> > Alternatively, if we do not want to refer to compiler assumptions:
> >
> >   * Uniqueness invariant - [`Opaque<T>`] makes aliasing accesses to the
> >     underlying `T` well defined.
> 
> This wording doesn't sound right to me, since it's still illegal to have
> multiple `&mut T` that point to a value inside of an `Opaque<T>`.
> 

Which wording proposed so far would sound right to you? Or you have a
proposal that could describe this rigorously?

Regards,
Boqun

> ---
> Cheers,
> Benno
>