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

[Boqun Feng]

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()?


So something like this for `AllowAtomic`, implementation-wise, no need
to change.

    /// Atomics that support basic atomic operations.
    ///
    /// Implementers must guarantee that `into_repr()` and `from_repr()` provide the same results as
    /// transmute between [`Self`] and [`Self::Repr`].
    ///
    /// TODO: Currently the [`AllowAtomic`] types are restricted within basic integer types (and their
    /// transparent new types). In the future, we could extend the scope to more data types when there
    /// is a clear and meaningful usage, but for now, [`AllowAtomic`] should only be implemented inside
    /// atomic mod for the restricted types mentioned above.
    ///
    /// # Safety
    ///
    /// - [`Self`] must have the same size and alignment as [`Self::Repr`].
    /// - Any value of [`Self`] must be safe to [`transmute()`] to a [`Self::Repr`], this also means
    ///   that a pointer to [`Self`] can be treated as a pointer to [`Self::Repr`].
    /// - If a value of [`Self::Repr`] is a result a [`transmute()`] from a [`Self`], it must be safe
    ///   to [`transmute()`] the value back to a [`Self`].
    ///   that a pointer to [`Self`] can be treated as a pointer to [`Self::Repr`].
    /// - The implementer must guarantee it's safe to transfer ownership from one execution context to
    ///   another, this means it has to be a [`Send`], but because `*mut T` is not [`Send`] and that's
    ///   the basic type needs to support atomic operations, so this safety requirement is added to
    ///   [`AllowAtomic`] trait. This safety requirement is automatically satisfied if the type is a
    ///   [`Send`].
    ///
    /// [`transmute()`]: core::mem::transmute
    pub unsafe trait AllowAtomic: Sized + Copy {

Thoughts?

Regards,
Boqun

> #[repr(transparent)]
> struct MyWeirdI32(pub i32);
> 
> impl AllowAtomic for MyWeirdI32 {
>     type Repr = i32;
> 
>     fn into_repr(self) -> Self::Repr {
>         !self
>     }
> 
>     fn from_repr(repr: Self::Repr) -> Self {
>         !self
>     }
> }
> 
> Then `Atomic<MyWeirdI32>::new(MyWeirdI32(0)).load(Relaxed)` will give me
> `MyWeirdI32(-1)`.
> 
> Alternatively, we should remove `into_repr`/`from_repr` and always cast
> instead. In such case, `AllowAtomic` needs to have the transmutability
> as a safety precondition.
> 
[...]