Re: [PATCH v5 04/10] rust: sync: atomic: Add generic atomics

On Mon, Jun 23, 2025 at 07:30:19PM +0100, Gary Guo wrote:
> On Sun, 22 Jun 2025 22:19:44 -0700
> Boqun Feng <[email protected]> wrote:
> 
> > On Sat, Jun 21, 2025 at 12:32:12PM +0100, Gary Guo wrote:
> > [...]
> > > > +#[repr(transparent)]
> > > > +pub struct Atomic<T: AllowAtomic>(Opaque<T>);  
> > > 
> > > This should store `Opaque<T::Repr>` instead.
> > >   
> > 
> > "should" is a strong word ;-) If we still use `into_repr`/`from_repr`
> > it's a bit impossible, because Atomic::new() wants to be a const
> > function, so it requires const_trait_impl I believe.
> > 
> > If we require transmutability as a safety requirement for `AllowAtomic`,
> > then either `T` or `T::Repr` is fine.
> > 
> > > The implementation below essentially assumes that this is
> > > `Opaque<T::Repr>`:
> > > * atomic ops cast this to `*mut T::Repr`
> > > * load/store operates on `T::Repr` then converts to `T` with
> > >   `T::from_repr`/`T::into_repr`.
> > >   
> > 
> > Note that we only require one direction of strong transmutability, that
> > is: for every `T`, it must be able to safe transmute to a `T::Repr`, for
> > `T::Repr` -> `T` transmutation, only if it's a result of a `transmute<T,
> > T::Repr>()`. This is mostly due to potential support for unit-only enum.  
> > E.g. using an atomic variable to represent a finite state.
> > 
> > > Note tha the transparent new types restriction on `AllowAtomic` is not
> > > sufficient for this, as I can define
> > >   
> > 
> > Nice catch! I do agree we should disallow `MyWeirdI32`, and I also agree
> > that we should put transmutability as safety requirement for
> > `AllowAtomic`. However, I would suggest we still keep
> > `into_repr`/`from_repr`, and require the implementation to make them
> > provide the same results as transmute(), as a correctness precondition
> > (instead of a safety precondition), in other words, you can still write
> > a `MyWeirdI32`, and it won't cause safety issues, but it'll be
> > incorrect.
> > 
> > The reason why I think we should keep `into_repr`/`from_repr` but add
> > a correctness precondition is that they are easily to implement as safe
> > code for basic types, so it'll be better than a transmute() call. Also
> > considering `Atomic<*mut T>`, would transmuting between integers and
> > pointers act the same as expose_provenance() and
> > from_exposed_provenance()?
> 
> Okay, this is more problematic than I thought then. For pointers, you

Welcome to my nightmare ;-)

> cannot just transmute between from pointers to usize (which is its
> Repr):
> * Transmuting from pointer to usize discards provenance
> * Transmuting from usize to pointer gives invalid provenance
> 
> We want neither behaviour, so we must store `usize` directly and
> always call into repr functions.
> 

If we store `usize`, how can we support the `get_mut()` then? E.g.

    static V: i32 = 32;

    let mut x = Atomic::new(&V as *const i32 as *mut i32);
    // ^ assume we expose_provenance() in new().

    let ptr: &mut *mut i32 = x.get_mut(); // which is `&mut self.0.get()`.

    let ptr_val = *ptr; // Does `ptr_val` have the proper provenance?

> To make things cost I guess you would need an extra trait to indicate
> that transmuting is fine.

Could you maybe provide an example?

Regards,
Boqun

> 
> Best,
> Gary