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

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

On Wed, Mar 05, 2025 at 08:47:42AM +0100, Andreas Hindborg wrote:
> "Dirk Behme" <dirk.behme@de.bosch.com> writes:
> 
> > In the discussion [1] some `Opaque` documentation updates have
> > been proposed. In that discussion it was clarified that `Opaque`
> > is intended to be used for (partial) uninitialized or changing C
> > structs. And which consequences this has for using raw pointers
> > or the destruction. Improve the `Opaque` documentation by adding
> > these conclusions from that discussion.
> >
> > Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
> > Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/ [1]
> > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> >
> > ---
> > Changes in v3:
> >    * Add the "terse" proposals.
> >    * Move the non-linkage artifact from patch 1/2 to here.
> >
> >  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

Regards,
Boqun

> > +///
> > +/// Further, [`Opaque<T>`] is `!Unpin` and will not run the drop method of the contained `T`
> > +/// when dropped.
> 
> Could you link `Unpin` here?
> 
> >  ///
> > -/// * The value is allowed to be uninitialized (for example have invalid bit patterns: `3` for a
> > -///   [`bool`]).
> > +/// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
> > +/// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
> > +/// This is useful for C structs that are not fully initialized (yet) or might change their
> > +/// content from C side at runtime. [`Opaque<T>`] gets rid of all the usual assumptions that
> > +/// Rust has for a value of type `T`:
> > +///
> > +/// * The value is allowed to be uninitialized or invalid (for example have invalid bit patterns:
> > +///   `3` for a [`bool`]).
> > +/// * By dereferencing a raw pointer to the value you are unsafely asserting that the value is
> > +///   valid *right now*.
> >  /// * The value is allowed to be mutated, when a `&Opaque<T>` exists on the Rust side.
> >  /// * No uniqueness for mutable references: it is fine to have multiple `&mut Opaque<T>` point to
> >  ///   the same value.
> >  /// * The value is not allowed to be shared with other threads (i.e. it is `!Sync`).
> 
> Link `Sync` here. You might need a [`!Sync`]: `Sync` at the bottom for
> it to work.
> 
> > +/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
> > +///   be uninitialized, as described above.
> 
> 
> Best regards,
> Andreas Hindborg
> 
> 
>