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

Boqun Feng
2025-06-23T05:19:50Z

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.
> …