Message-ID: <we7PHkrpBV6kIgFZojiBFerqlXtSJB9HWkj129OGUWUVyVFwtuoirr3gVybgLzW2hmpUqqSAAQUPsdfL9QC2JyNKOLRNX0mSTfgD8llSJKE=@protonmail.com>
Date:   Sun, 25 Jun 2023 12:56:29 +0000
From:   Björn Roy Baron <bjorn3_gh@...tonmail.com>
To:     Benno Lossin <benno.lossin@...ton.me>
Cc:     Miguel Ojeda <ojeda@...nel.org>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Alice Ryhl <aliceryhl@...gle.com>,
        Andreas Hindborg <nmi@...aspace.dk>,
        rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
        patches@...ts.linux.dev, Asahi Lina <lina@...hilina.net>
Subject: Re: [PATCH 5/7] rust: init: add `..Zeroable::zeroed()` syntax for zeroing all missing fields

On Saturday, June 24th, 2023 at 23:14, Benno Lossin <benno.lossin@...ton.me> wrote:

> On 6/24/23 17:11, Björn Roy Baron wrote:
> > On Saturday, June 24th, 2023 at 11:25, Benno Lossin <benno.lossin@...ton.me> wrote:
> >
> >> Add the struct update syntax to the init macros, but only for
> >> `..Zeroable::zeroed()`. Adding this at the end of the struct initializer
> >> allows one to omit fields from the initializer, these fields will be
> >> initialized with 0x00 set to every byte. Only types that implement the
> >> `Zeroable` trait can utilize this.
> >>
> >> Suggested-by: Asahi Lina <lina@...hilina.net>
> >> Signed-off-by: Benno Lossin <benno.lossin@...ton.me>
> >> ---
> >>   rust/kernel/init.rs        |  16 +++++-
> >>   rust/kernel/init/macros.rs | 114 ++++++++++++++++++++++++++++++++++++-
> >>   2 files changed, 128 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> >> index ecf6a4bd0ce4..44bc3e77419a 100644
> >> --- a/rust/kernel/init.rs
> >> +++ b/rust/kernel/init.rs
> >> @@ -508,14 +508,18 @@ macro_rules! stack_try_pin_init {
> >>   /// - Fields that you want to initialize in-place have to use `<-` instead of `:`.
> >>   /// - In front of the initializer you can write `&this in` to have access to a [`NonNull<Self>`]
> >>   ///   pointer named `this` inside of the initializer.
> >> +/// - Using struct update syntax one can place `..Zeroable::zeroed()` at the very end of the
> >> +///   struct, this initializes every field with 0 and then runs all initializers specified in the
> >> +///   body. This can only be done if [`Zeroable`] is implemented for the struct.
> >>   ///
> >>   /// For instance:
> >>   ///
> >>   /// ```rust
> >>   /// # use kernel::pin_init;
> >> -/// # use macros::pin_data;
> >> +/// # use macros::{pin_data, Zeroable};
> >>   /// # use core::{ptr::addr_of_mut, marker::PhantomPinned};
> >>   /// #[pin_data]
> >> +/// #[derive(Zeroable)]
> >>   /// struct Buf {
> >>   ///     // `ptr` points into `buf`.
> >>   ///     ptr: *mut u8,
> >> @@ -528,6 +532,10 @@ macro_rules! stack_try_pin_init {
> >>   ///     ptr: unsafe { addr_of_mut!((*this.as_ptr()).buf).cast() },
> >>   ///     pin: PhantomPinned,
> >>   /// });
> >> +/// pin_init!(Buf {
> >> +///     buf: [1; 64],
> >> +///     ..Zeroable::zeroed(),
> >> +/// });
> >>   /// ```
> >>   ///
> >>   /// [`try_pin_init!`]: kernel::try_pin_init
> >> @@ -547,6 +555,7 @@ macro_rules! pin_init {
> >>               @data(PinData, use_data),
> >>               @has_data(HasPinData, __pin_data),
> >>               @construct_closure(pin_init_from_closure),
> >> +            @munch_fields($($fields)*),
> >>           )
> >>       };
> >>   }
> >> @@ -603,6 +612,7 @@ macro_rules! try_pin_init {
> >>               @data(PinData, use_data),
> >>               @has_data(HasPinData, __pin_data),
> >>               @construct_closure(pin_init_from_closure),
> >> +            @munch_fields($($fields)*),
> >>           )
> >>       };
> >>       ($(&$this:ident in)? $t:ident $(::<$($generics:ty),* $(,)?>)? {
> >> @@ -616,6 +626,7 @@ macro_rules! try_pin_init {
> >>               @data(PinData, use_data),
> >>               @has_data(HasPinData, __pin_data),
> >>               @construct_closure(pin_init_from_closure),
> >> +            @munch_fields($($fields)*),
> >>           )
> >>       };
> >>   }
> >> @@ -650,6 +661,7 @@ macro_rules! init {
> >>               @data(InitData, /*no use_data*/),
> >>               @has_data(HasInitData, __init_data),
> >>               @construct_closure(init_from_closure),
> >> +            @munch_fields($($fields)*),
> >>           )
> >>       }
> >>   }
> >> @@ -700,6 +712,7 @@ macro_rules! try_init {
> >>               @data(InitData, /*no use_data*/),
> >>               @has_data(HasInitData, __init_data),
> >>               @construct_closure(init_from_closure),
> >> +            @munch_fields($($fields)*),
> >>           )
> >>       };
> >>       ($(&$this:ident in)? $t:ident $(::<$($generics:ty),* $(,)?>)? {
> >> @@ -713,6 +726,7 @@ macro_rules! try_init {
> >>               @data(InitData, /*no use_data*/),
> >>               @has_data(HasInitData, __init_data),
> >>               @construct_closure(init_from_closure),
> >> +            @munch_fields($($fields)*),
> >>           )
> >>       };
> >>   }
> >> diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
> >> index 1e0c4aca055a..5dcb2e513f26 100644
> >> --- a/rust/kernel/init/macros.rs
> >> +++ b/rust/kernel/init/macros.rs
> >> @@ -989,6 +989,7 @@ impl<$($impl_generics)*> $pin_data<$($ty_generics)*>
> >>   ///
> >>   /// This macro has multiple internal call configurations, these are always the very first ident:
> >>   /// - nothing: this is the base case and called by the `{try_}{pin_}init!` macros.
> >> +/// - `with_update_parsed`: when the `..Zeroable::zeroed()` syntax has been handled.
> >>   /// - `init_slot`: recursively creates the code that initializes all fields in `slot`.
> >>   /// - `make_initializer`: recursively create the struct initializer that guarantees that every
> >>   ///   field has been initialized exactly once.
> >> @@ -1007,6 +1008,82 @@ macro_rules! __init_internal {
> >>           @has_data($has_data:ident, $get_data:ident),
> >>           // `pin_init_from_closure` or `init_from_closure`.
> >>           @construct_closure($construct_closure:ident),
> >> +        @munch_fields(),
> >> +    ) => {
> >> +        $crate::__init_internal!(with_update_parsed:
> >> +            @this($($this)?),
> >> +            @typ($t $(::<$($generics),*>)? ),
> >> +            @fields($($fields)*),
> >> +            @error($err),
> >> +            @data($data, $($use_data)?),
> >> +            @has_data($has_data, $get_data),
> >> +            @construct_closure($construct_closure),
> >> +            @zeroed(), // nothing means default behavior.
> >> +        )
> >> +    };
> >> +    (
> >> +        @this($($this:ident)?),
> >> +        @typ($t:ident $(::<$($generics:ty),*>)?),
> >> +        @fields($($fields:tt)*),
> >> +        @error($err:ty),
> >> +        // Either `PinData` or `InitData`, `$use_data` should only be present in the `PinData`
> >> +        // case.
> >> +        @data($data:ident, $($use_data:ident)?),
> >> +        // `HasPinData` or `HasInitData`.
> >> +        @has_data($has_data:ident, $get_data:ident),
> >> +        // `pin_init_from_closure` or `init_from_closure`.
> >> +        @construct_closure($construct_closure:ident),
> >> +        @munch_fields(..Zeroable::zeroed()),
> >> +    ) => {
> >> +        $crate::__init_internal!(with_update_parsed:
> >> +            @this($($this)?),
> >> +            @typ($t $(::<$($generics),*>)? ),
> >> +            @fields($($fields)*),
> >> +            @error($err),
> >> +            @data($data, $($use_data)?),
> >> +            @has_data($has_data, $get_data),
> >> +            @construct_closure($construct_closure),
> >> +            @zeroed(()), // `()` means zero all fields not mentioned.
> >> +        )
> >> +    };
> >> +    (
> >> +        @this($($this:ident)?),
> >> +        @typ($t:ident $(::<$($generics:ty),*>)?),
> >> +        @fields($($fields:tt)*),
> >> +        @error($err:ty),
> >> +        // Either `PinData` or `InitData`, `$use_data` should only be present in the `PinData`
> >> +        // case.
> >> +        @data($data:ident, $($use_data:ident)?),
> >> +        // `HasPinData` or `HasInitData`.
> >> +        @has_data($has_data:ident, $get_data:ident),
> >> +        // `pin_init_from_closure` or `init_from_closure`.
> >> +        @construct_closure($construct_closure:ident),
> >> +        @munch_fields($ignore:tt $($rest:tt)*),
> >> +    ) => {
> >> +        $crate::__init_internal!(
> >> +            @this($($this)?),
> >> +            @typ($t $(::<$($generics),*>)? ),
> >> +            @fields($($fields)*),
> >> +            @error($err),
> >> +            @data($data, $($use_data)?),
> >> +            @has_data($has_data, $get_data),
> >> +            @construct_closure($construct_closure),
> >> +            @munch_fields($($rest)*),
> >> +        )
> >> +    };
> >> +    (with_update_parsed:
> >> +        @this($($this:ident)?),
> >> +        @typ($t:ident $(::<$($generics:ty),*>)?),
> >> +        @fields($($fields:tt)*),
> >> +        @error($err:ty),
> >> +        // Either `PinData` or `InitData`, `$use_data` should only be present in the `PinData`
> >> +        // case.
> >> +        @data($data:ident, $($use_data:ident)?),
> >> +        // `HasPinData` or `HasInitData`.
> >> +        @has_data($has_data:ident, $get_data:ident),
> >> +        // `pin_init_from_closure` or `init_from_closure`.
> >> +        @construct_closure($construct_closure:ident),
> >> +        @zeroed($($init_zeroed:expr)?),
> >>       ) => {{
> >>           // We do not want to allow arbitrary returns, so we declare this type as the `Ok` return
> >>           // type and shadow it later when we insert the arbitrary user code. That way there will be
> >> @@ -1024,6 +1101,17 @@ macro_rules! __init_internal {
> >>                   {
> >>                       // Shadow the structure so it cannot be used to return early.
> >>                       struct __InitOk;
> >> +                    // If `$init_zeroed` is present we should zero the slot now and not emit an
> >> +                    // error when fields are missing (since they will be zeroed). We also have to
> >> +                    // check that the type actually implements `Zeroable`.
> >> +                    $(
> >> +                        fn is_zeroable<T: Zeroable>(ptr: *mut T) {}
> >
> > Maybe call this assert_zeroable?
> 
> Sure.
> 
> >
> >> +                        // Ensure that the struct is indeed `Zeroable`.
> >> +                        is_zeroable(slot);
> >> +                        // SAFETY:  The type implements `Zeroable` by the check above.
> >> +                        unsafe { ::core::ptr::write_bytes(slot, 0, 1) };
> >> +                        $init_zeroed // this will be `()` if set.
> >
> > How does this work? Shouldn't there be a ; after $init_zeroed to consume the () value?
> 
> It is the last expression of a block and since it is `()` it is ok
> (adding a ; would also be ok, but it is not necessary).

I'm surprised it is considered the last expression of a block. Unlike with {} using $()? will still
allow variables defined inside this as if they were outside of it. Also I can't reproduce this
behavior with:

    macro_rules! foo {
        ($($a:expr)?) => {
            $($a)?
            bar();
        }
    }

    fn main() {
        foo!(());
    }

Is there something I'm missing?

Cheers,
Björn

> 
> >
> >> +                    )?
> >>                       // Create the `this` so it can be referenced by the user inside of the
> >>                       // expressions creating the individual fields.
> >>                       $(let $this = unsafe { ::core::ptr::NonNull::new_unchecked(slot) };)?
> >> @@ -1064,7 +1152,7 @@ macro_rules! __init_internal {
> >>           @data($data:ident),
> >>           @slot($slot:ident),
> >>           @guards($($guards:ident,)*),
> >> -        @munch_fields($(,)?),
> >> +        @munch_fields($(..Zeroable::zeroed())? $(,)?),
> >>       ) => {
> >>           // Endpoint of munching, no fields are left. If execution reaches this point, all fields
> >>           // have been initialized. Therefore we can now dismiss the guards by forgetting them.
> >> @@ -1157,6 +1245,30 @@ macro_rules! __init_internal {
> >>               @munch_fields($($rest)*),
> >>           );
> >>       };
> >> +    (make_initializer:
> >> +        @slot($slot:ident),
> >> +        @type_name($t:ident),
> >> +        @munch_fields(..Zeroable::zeroed() $(,)?),
> >> +        @acc($($acc:tt)*),
> >> +    ) => {
> >> +        // Endpoint, nothing more to munch, create the initializer. Since the users specified
> >> +        // `..Zeroable::zeroed()`, the slot will already have been zeroed and all field that have
> >> +        // not been overwritten are thus zero and initialized. We still check that all fields are
> >> +        // actually accessible by using the struct update syntax ourselves.
> >> +        // Since we are in the `if false` branch, this will never get executed. We abuse `slot` to
> >> +        // get the correct type inference here:
> >> +        unsafe {
> >> +            let mut zeroed = ::core::mem::zeroed();
> >> +            // We have to use type inference her to make zeroed have the correct type. This does
> >
> > *here
> 
> Will fix.
> 
> --
> Cheers,
> Benno
> 
> >
> >> +            // not get executed, so it has no effect.
> >> +            ::core::ptr::write($slot, zeroed);
> >> +            zeroed = ::core::mem::zeroed();
> >> +            ::core::ptr::write($slot, $t {
> >> +                $($acc)*
> >> +                ..zeroed
> >> +            });
> >> +        }
> >> +    };
> >>       (make_initializer:
> >>           @slot($slot:ident),
> >>           @type_name($t:ident),
> >> --
> >> 2.41.0
> >
> > Cheers,
> > Björn

Powered by blists - more mailing lists