Re: [PATCH v2 1/3] rust: add UnsafePinned type

["Benno Lossin" <lossin@kernel.org>]

On Wed Apr 30, 2025 at 7:30 PM CEST, Christian Schrefl wrote:
> On 30.04.25 11:45 AM, Benno Lossin wrote:
>> On Wed Apr 30, 2025 at 10:36 AM CEST, Christian Schrefl wrote:
>>> +/// This implementation works because of the "`!Unpin` hack" in rustc, which allows (some kinds of)
>>> +/// mutual aliasing of `!Unpin` types. This hack might be removed at some point, after which only
>>> +/// the `core::pin::UnsafePinned` type will allow this behavior. In order to simplify the migration
>>> +/// to future rust versions only this polyfill of this type should be used when this behavior is
>>> +/// required.
>>> +///
>>> +/// In order to disable niche optimizations this implementation uses [`UnsafeCell`] internally,
>>> +/// the upstream version however will not. So the fact that [`UnsafePinned`] contains an
>>> +/// [`UnsafeCell`] must not be relied on (Other than the niche blocking).
>> 
>> I would make this last paragraph a normal comment, I don't think we
>> should expose it in the docs.
>
> I added this as docs since I wanted it to be a bit more visible,
> but I can replace the comment text (about `UnsafeCell`) with this paragraph
> and drop it from the docs if you want.

I think we shouldn't talk about these implementation details in the
docs.

>>> +// As opposed to the upstream Rust type this contains a `PhantomPinned`` and `UnsafeCell<T>`
>>> +// - `PhantomPinned` to avoid needing a `impl<T> !Unpin for UnsafePinned<T>`
>>> +//      Required to use the `!Unpin hack`.
>>> +// - `UnsafeCell<T>` instead of T to disallow niche optimizations,
>>> +//     which is handled in the compiler in upstream Rust
>>> +#[repr(transparent)]
>>> +pub struct UnsafePinned<T: ?Sized> {
>>> +    _ph: PhantomPinned,
>>> +    value: UnsafeCell<T>,
>>> +}
>>> +
>>> +impl<T> UnsafePinned<T> {
>>> +    /// Constructs a new instance of [`UnsafePinned`] which will wrap the specified value.
>>> +    ///
>>> +    /// All access to the inner value through `&UnsafePinned<T>` or `&mut UnsafePinned<T>` or
>>> +    /// `Pin<&mut UnsafePinned<T>>` requires `unsafe` code.
>>> +    #[inline(always)]
>>> +    #[must_use]
>>> +    pub const fn new(value: T) -> Self {
>>> +        UnsafePinned {
>>> +            value: UnsafeCell::new(value),
>>> +            _ph: PhantomPinned,
>>> +        }
>>> +    }
>>> +}
>>> +impl<T: ?Sized> UnsafePinned<T> {
>>> +    /// Get read-only access to the contents of a shared `UnsafePinned`.
>>> +    ///
>>> +    /// Note that `&UnsafePinned<T>` is read-only if `&T` is read-only. This means that if there is
>>> +    /// mutation of the `T`, future reads from the `*const T` returned here are UB! Use
>>> +    /// [`UnsafeCell`] if you also need interior mutability.
>> 
>> I agree with copy-pasting the docs from upstream, even though our
>> implementation already wraps the value in `UnsafeCell`, but I think we
>> should include a comment at the top of this doc that mentions this
>> difference. So something along the lines "In order to make replacing
>> this type with the upstream one, we want to have as little API
>> divergence as possible. Thus we don't mention the implementation detail
>> of `UnsafeCell` and people have to use `UnsafePinned<UnsafeCell<T>>`
>> instead of just `UnsafePinned<T>`." feel free to modify.
>> 
>
> I already wrote about this in comments (and documentation in this version)
> on the `UnsafePinned` type definition.
>
> I'm not sure where exactly we want to have this, but I think having it
> at the top of the file and on the type definition is a bit redundant.

Sure.

>>> +    /// Gets a mutable pointer to the wrapped value.
>>> +    ///
>>> +    /// The difference from `get_mut_pinned` and `get_mut_unchecked` is that this function
>>> +    /// accepts a raw pointer, which is useful to avoid the creation of temporary references.
>> 
>> You did not include the `get_mut_{pinned,unchecked}` methods, so
>> mentioning them here in the docs might confuse people. Do we want to
>> have those methods?
>
> I only included the functions that we needed for `Opaque` and my
> `miscdevice' patches. I think these functions should only be added
> once they have a user. That's why I wrote the next sentence in the
> documents.
>
> Should I handle this differently?
>
> It should be a really simple patch to add these functions and I can
> do that if someone needs them or I can just include them in this
> patch set.

Then I'd remove the sentence referencing the functions you don't add.

---
Cheers,
Benno