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

On Thu, Jun 26, 2025 at 03:54:24PM +0200, Benno Lossin wrote:
> 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?).
> 

I need the `UnsafePinned`-like behavior of `Atomic<*mut T>` to support
Rcu<T>, and I will replace it with `UnsafePinned`, once that's is
available.

Maybe that also means `UnsafePinned<T>` make more sense? Because if `T`
is a pointer, it's easy to prove the provenance is there. (Note a
`&Atomic<*mut T>` may come from a `*mut *mut T`, may be a field in C
struct)

Regards,
Boqun