Re: [PATCH v6 2/9] rust: sync: Add basic atomic operation mapping framework

On Thu, Jul 10, 2025 at 05:46:56PM +0200, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 5:12 PM CEST, Boqun Feng wrote:
> > On Thu, Jul 10, 2025 at 01:04:38PM +0200, Benno Lossin wrote:
> >> On Thu Jul 10, 2025 at 8:00 AM CEST, Boqun Feng wrote:
> >> > +declare_and_impl_atomic_methods!(
> >> > +    AtomicHasBasicOps ("Basic atomic operations") {
> >> > +        read[acquire](ptr: *mut Self) -> Self {
> >> > +            call(ptr.cast())
> >> > +        }
> >> > +
> >> > +        set[release](ptr: *mut Self, v: Self) {
> >> > +            call(ptr.cast(), v)
> >> > +        }
> >> > +    }
> >> 
> >> I think this would look a bit better:
> >> 
> >>     /// Basic atomic operations.
> >>     pub trait AtomicHasBasicOps {
> >>         unsafe fn read[acquire](ptr: *mut Self) -> Self {
> >>             bindings::#call(ptr.cast())
> >>         }
> >> 
> >>         unsafe fn set[release](ptr: *mut Self, v: Self) {
> >>             bindings::#call(ptr.cast(), v)
> >>         }
> >>     }
> >> 
> >
> > Make sense, I've made `pub trait`, `bindings::#` and `unsafe fn`
> > hard-coded:
> >
> > macro_rules! declare_and_impl_atomic_methods {
> >     (#[doc = $doc:expr] pub trait $ops:ident {
> 
> You should allow any kind of attribute (and multiple), that makes it
> much simpler.
> 

I didn't know I could do that, updated:

    ($(#[$attr:meta])* pub trait $ops:ident {
        $(
            unsafe fn $func:ident [$($variant:ident),*]($($arg_sig:tt)*) $( -> $ret:ty)? {
                bindings::#call($($arg:tt)*)
            }
        )*
    }) => {
        $(#[$attr])*

Thanks!

> >         $(
> >             unsafe fn $func:ident [$($variant:ident),*]($($arg_sig:tt)*) $( -> $ret:ty)? {
> >                 bindings::#call($($arg:tt)*)
> >             }
> >         )*
> >     }) => {
> >
> > It shouldn't be very hard to make use of the actual visibility or
> > unsafe, but we currently don't have other visibility or safe function,
> > so it's simple to keep it as it is.
[..]
> >> I'm not sure if this is worth it, but for reading the definitions of
> >> these operations directly in the code this is going to be a lot more
> >> readable. I don't think it's too bad to duplicate it.
> >> 
> >> I'm also not fully satisfied with the safety comment on
> >> `bindings::#call`...
> >> 
> >
> > Based on the above, I'm not going to do the change (i.e. duplicating
> > the safe comments and improve them), and I would make an issue tracking
> > it, and we can revisit it when we have time. Sounds good?
> 
> Sure, I feel like some kind of method duplication macro might be much
> better here, like:
> 
>     multi_functions! {
>         pub trait AtomicHasBasicOps {
>             /// Atomic read
>             ///
>             /// # Safety
>             /// - Any pointer passed to the function has to be a valid pointer
>             /// - Accesses must not cause data races per LKMM:
>             ///   - Atomic read racing with normal read, normal write or atomic write is not a data race.
>             ///   - Atomic write racing with normal read or normal write is a data race, unless the
>             ///     normal access is done from the C side and considered immune to data races, e.g.
>             ///     `CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC`.
>             unsafe fn [<read, read_acquire>](ptr: *mut Self) -> Self;
> 
>             // ...
>         }
>     }
> 
> And then also allow it on impls. I don't really like the idea of
> duplicating and thus hiding the safety docs... But I also see that just

At least the rustdoc has safety section for each function. ;-)

> copy pasting them everywhere isn't really a good solution either...
> 

Yeah, perhaps there is no immediate resolution, but open to any
suggestion.

Regards,
Boqun

> ---
> Cheers,
> Benno