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

On Tue Jun 24, 2025 at 6:35 PM CEST, Boqun Feng wrote:
> On Tue, Jun 24, 2025 at 01:27:38AM +0200, Benno Lossin wrote:
>> On Mon Jun 23, 2025 at 9:09 PM CEST, Boqun Feng wrote:
>> > On Mon, Jun 23, 2025 at 07:30:19PM +0100, Gary Guo wrote:
>> >> 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?
>> 
>> If `get_mut` transmutes the integer into a pointer, then it will have
>> the wrong provenance (it will just have plain invalid provenance).
>> 
>
> The key topic Gary and I have been discussing is whether we should
> define Atomic<T> as:
>
> (my current implementation)
>
>     pub struct Atomic<T: AllowAtomic>(Opaque<T>);
>
> or
>
> (Gary's suggestion)
>
>     pub struct Atomic<T: AllowAtomic>(Opaque<T::Repr>);
>
> `T::Repr` is guaranteed to be the same size and alignment of `T`, and
> per our discussion, it makes sense to further require that `transmute<T,
> T::Repr>()` should also be safe (as the safety requirement of
> `AllowAtomic`), or we can say `T` bit validity can be preserved by
> `T::Repr`: a valid bit combination `T` can be transumated to `T::Repr`,
> and if transumated back, it's the same bit combination.
>
> Now as I pointed out, if we use `Opaque<T::Repr>`, then `.get_mut()`
> would be unsound for `Atomic<*mut T>`. And Gary's concern is that in
> the current implementation, we directly cast a `*mut T` (from
> `Opaque::get()`) into a `*mut T::Repr`, and pass it directly into C/asm
> atomic primitives. However, I think with the additional safety
> requirement above, this shouldn't be a problem: because the C/asm atomic
> primitives would just pass the address to an asm block, and that'll be
> out of Rust abstract machine, and as long as the C/primitives atomic
> primitives are implemented correctly, the bit representation of `T`
> remains valid after asm blocks.
>
> So I think the current implementation still works and is better.

I don't think there is a big difference between `Opaque<T>` and
`Opaque<T::Repr>` if we have the transmute equivalence between the two.
From a safety perspective, you don't gain or lose anything by using the
first over the second one. They both require the invariant that they are
valid (as `Opaque` removes that... we should really be using
`UnsafeCell` here instead... why aren't we doing that?).

Where their differences do play a role is in the implementation of the
various operations on the atomic. If you need to pass `*mut T::Repr` to
the C side, it's better if you store `Opaque<T::Repr>` and if you want
to give `&mut T` back to the user, then it's better to
store `Opaque<T>`.

I would choose the one that results in overall less code. It's probably
going to be `Opaque<T::Repr>`, since we will have more operations that
need `*mut T::Repr` than `*mut T`.

Now I don't understand why you value `Opaque<T>` over `Opaque<T::Repr>`,
they are (up to transmute-equivalence) the same.

I think that you said at one point that `Opaque<T>` makes more sense
from a conceptual view, since we're building `Atomic<T>`. I think that
doesn't really matter, since it's implementation detail. The same
argument could be made about casing `u64` to `i64` for implementing the
atomics: just implement atomics in C also for `u64` and then use that
instead...

---
Cheers,
Benno