[3/7] rust: init: make guards in the init macros hygienic

Message ID 20230624092330.157338-3-benno.lossin@proton.me
State New
Headers
Series [1/7] rust: init: consolidate init macros |

Commit Message

Benno Lossin June 24, 2023, 9:25 a.m. UTC
  Use hygienic identifiers for the guards instead of the field names. This
makes the init macros feel more like normal struct initializers, since
assigning identifiers with the name of a field does not create
conflicts.
Also change the internals of the guards, no need to make the `forget`
function `unsafe`, since users cannot access the guards anyways. Now the
guards are carried directly on the stack and have no extra `Cell<bool>`
field that marks if they have been forgotten or not, instead they are
just forgotten via `mem::forget`.

Suggested-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
 rust/kernel/init.rs            |  1 -
 rust/kernel/init/__internal.rs | 25 +++------------
 rust/kernel/init/macros.rs     | 56 ++++++++++++----------------------
 3 files changed, 23 insertions(+), 59 deletions(-)

--
2.41.0
  

Comments

Björn Roy Baron June 24, 2023, 2:58 p.m. UTC | #1
On Saturday, June 24th, 2023 at 11:25, Benno Lossin <benno.lossin@proton.me> wrote:

> Use hygienic identifiers for the guards instead of the field names. This
> makes the init macros feel more like normal struct initializers, since
> assigning identifiers with the name of a field does not create
> conflicts.
> Also change the internals of the guards, no need to make the `forget`
> function `unsafe`, since users cannot access the guards anyways. Now the
> guards are carried directly on the stack and have no extra `Cell<bool>`
> field that marks if they have been forgotten or not, instead they are
> just forgotten via `mem::forget`.
> 
> Suggested-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>

Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com>

> ---
>  rust/kernel/init.rs            |  1 -
>  rust/kernel/init/__internal.rs | 25 +++------------
>  rust/kernel/init/macros.rs     | 56 ++++++++++++----------------------
>  3 files changed, 23 insertions(+), 59 deletions(-)
> 
> diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> index d9a91950cba2..ecf6a4bd0ce4 100644
> --- a/rust/kernel/init.rs
> +++ b/rust/kernel/init.rs
> @@ -206,7 +206,6 @@
>  use alloc::boxed::Box;
>  use core::{
>      alloc::AllocError,
> -    cell::Cell,
>      convert::Infallible,
>      marker::PhantomData,
>      mem::MaybeUninit,
> diff --git a/rust/kernel/init/__internal.rs b/rust/kernel/init/__internal.rs
> index 44751fb62b51..7abd1fb65e41 100644
> --- a/rust/kernel/init/__internal.rs
> +++ b/rust/kernel/init/__internal.rs
> @@ -174,7 +174,6 @@ pub fn init<E>(self: Pin<&mut Self>, init: impl PinInit<T, E>) -> Result<Pin<&mu
>  /// Can be forgotten to prevent the drop.
>  pub struct DropGuard<T: ?Sized> {
>      ptr: *mut T,
> -    do_drop: Cell<bool>,
>  }
> 
>  impl<T: ?Sized> DropGuard<T> {
> @@ -190,32 +189,16 @@ impl<T: ?Sized> DropGuard<T> {
>      /// - will not be dropped by any other means.
>      #[inline]
>      pub unsafe fn new(ptr: *mut T) -> Self {
> -        Self {
> -            ptr,
> -            do_drop: Cell::new(true),
> -        }
> -    }
> -
> -    /// Prevents this guard from dropping the supplied pointer.
> -    ///
> -    /// # Safety
> -    ///
> -    /// This function is unsafe in order to prevent safe code from forgetting this guard. It should
> -    /// only be called by the macros in this module.
> -    #[inline]
> -    pub unsafe fn forget(&self) {
> -        self.do_drop.set(false);
> +        Self { ptr }
>      }
>  }
> 
>  impl<T: ?Sized> Drop for DropGuard<T> {
>      #[inline]
>      fn drop(&mut self) {
> -        if self.do_drop.get() {
> -            // SAFETY: A `DropGuard` can only be constructed using the unsafe `new` function
> -            // ensuring that this operation is safe.
> -            unsafe { ptr::drop_in_place(self.ptr) }
> -        }
> +        // SAFETY: A `DropGuard` can only be constructed using the unsafe `new` function
> +        // ensuring that this operation is safe.
> +        unsafe { ptr::drop_in_place(self.ptr) }
>      }
>  }
> 
> diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
> index e8165ff53a94..df4281743175 100644
> --- a/rust/kernel/init/macros.rs
> +++ b/rust/kernel/init/macros.rs
> @@ -992,7 +992,6 @@ impl<$($impl_generics)*> $pin_data<$($ty_generics)*>
>  /// - `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.
> -/// - `forget_guards`: recursively forget the drop guards for every field.
>  #[doc(hidden)]
>  #[macro_export]
>  macro_rules! __init_internal {
> @@ -1032,6 +1031,7 @@ macro_rules! __init_internal {
>                      $crate::__init_internal!(init_slot($($use_data)?):
>                          @data(data),
>                          @slot(slot),
> +                        @guards(),
>                          @munch_fields($($fields)*,),
>                      );
>                      // We use unreachable code to ensure that all fields have been mentioned exactly
> @@ -1046,10 +1046,6 @@ macro_rules! __init_internal {
>                              @acc(),
>                          );
>                      }
> -                    // Forget all guards, since initialization was a success.
> -                    $crate::__init_internal!(forget_guards:
> -                        @munch_fields($($fields)*,),
> -                    );
>                  }
>                  Ok(__InitOk)
>              }
> @@ -1063,13 +1059,17 @@ macro_rules! __init_internal {
>      (init_slot($($use_data:ident)?):
>          @data($data:ident),
>          @slot($slot:ident),
> +        @guards($($guards:ident,)*),
>          @munch_fields($(,)?),
>      ) => {
> -        // Endpoint of munching, no fields are left.
> +        // 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.
> +        $(::core::mem::forget($guards);)*
>      };
>      (init_slot($use_data:ident): // use_data is present, so we use the `data` to init fields.
>          @data($data:ident),
>          @slot($slot:ident),
> +        @guards($($guards:ident,)*),
>          // In-place initialization syntax.
>          @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
>      ) => {
> @@ -1080,24 +1080,26 @@ macro_rules! __init_internal {
>          // return when an error/panic occurs.
>          // We also use the `data` to require the correct trait (`Init` or `PinInit`) for `$field`.
>          unsafe { $data.$field(::core::ptr::addr_of_mut!((*$slot).$field), $field)? };
> -        // Create the drop guard.
> +        // Create the drop guard:
>          //
> -        // We only give access to `&DropGuard`, so it cannot be forgotten via safe code.
> +        // We rely on macro hygiene to make it impossible for users to access this local variable.
>          //
>          // SAFETY: We forget the guard later when initialization has succeeded.
> -        let $field = &unsafe {
> +        let guard = unsafe {
>              $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
>          };
> 
>          $crate::__init_internal!(init_slot($use_data):
>              @data($data),
>              @slot($slot),
> +            @guards(guard, $($guards,)*),
>              @munch_fields($($rest)*),
>          );
>      };
>      (init_slot(): // no use_data, so we use `Init::__init` directly.
>          @data($data:ident),
>          @slot($slot:ident),
> +        @guards($($guards:ident,)*),
>          // In-place initialization syntax.
>          @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
>      ) => {
> @@ -1107,24 +1109,26 @@ macro_rules! __init_internal {
>          // SAFETY: `slot` is valid, because we are inside of an initializer closure, we
>          // return when an error/panic occurs.
>          unsafe { $crate::init::Init::__init($field, ::core::ptr::addr_of_mut!((*$slot).$field))? };
> -        // Create the drop guard.
> +        // Create the drop guard:
>          //
> -        // We only give access to `&DropGuard`, so it cannot be forgotten via safe code.
> +        // We rely on macro hygiene to make it impossible for users to access this local variable.
>          //
>          // SAFETY: We forget the guard later when initialization has succeeded.
> -        let $field = &unsafe {
> +        let guard = unsafe {
>              $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
>          };
> 
>          $crate::__init_internal!(init_slot():
>              @data($data),
>              @slot($slot),
> +            @guards(guard, $($guards,)*),
>              @munch_fields($($rest)*),
>          );
>      };
>      (init_slot($($use_data:ident)?):
>          @data($data:ident),
>          @slot($slot:ident),
> +        @guards($($guards:ident,)*),
>          // Init by-value.
>          @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
>      ) => {
> @@ -1135,16 +1139,17 @@ macro_rules! __init_internal {
>          unsafe { ::core::ptr::write(::core::ptr::addr_of_mut!((*$slot).$field), $field) };
>          // Create the drop guard:
>          //
> -        // We only give access to `&DropGuard`, so it cannot be accidentally forgotten.
> +        // We rely on macro hygiene to make it impossible for users to access this local variable.
>          //
>          // SAFETY: We forget the guard later when initialization has succeeded.
> -        let $field = &unsafe {
> +        let guard = unsafe {
>              $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
>          };
> 
>          $crate::__init_internal!(init_slot($($use_data)?):
>              @data($data),
>              @slot($slot),
> +            @guards(guard, $($guards,)*),
>              @munch_fields($($rest)*),
>          );
>      };
> @@ -1189,29 +1194,6 @@ macro_rules! __init_internal {
>              @acc($($acc)* $field: ::core::panic!(),),
>          );
>      };
> -    (forget_guards:
> -        @munch_fields($(,)?),
> -    ) => {
> -        // Munching finished.
> -    };
> -    (forget_guards:
> -        @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
> -    ) => {
> -        unsafe { $crate::init::__internal::DropGuard::forget($field) };
> -
> -        $crate::__init_internal!(forget_guards:
> -            @munch_fields($($rest)*),
> -        );
> -    };
> -    (forget_guards:
> -        @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
> -    ) => {
> -        unsafe { $crate::init::__internal::DropGuard::forget($field) };
> -
> -        $crate::__init_internal!(forget_guards:
> -            @munch_fields($($rest)*),
> -        );
> -    };
>  }
> 
>  #[doc(hidden)]
> --
> 2.41.0
  
Gary Guo June 25, 2023, 8:54 p.m. UTC | #2
On Sat, 24 Jun 2023 09:25:10 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> Use hygienic identifiers for the guards instead of the field names. This
> makes the init macros feel more like normal struct initializers, since
> assigning identifiers with the name of a field does not create
> conflicts.
> Also change the internals of the guards, no need to make the `forget`
> function `unsafe`, since users cannot access the guards anyways. Now the
> guards are carried directly on the stack and have no extra `Cell<bool>`
> field that marks if they have been forgotten or not, instead they are
> just forgotten via `mem::forget`.

The code LGTM, so:

Reviewed-by: Gary Guo <gary@garyguo.net>

Although this will cause the new expansion we have to be no longer
compatible with a totally-proc-macro impl, if we want to do everything
in proc macro in the future.

If we have the paste macro upstream (
https://github.com/nbdd0121/linux/commit/fff00461b0be7fd3ec218dcc428f25886b5ec04a
) then we can replace the `guard` with `paste!([<$field>])` and keep
the expansion identical.

> 
> Suggested-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
>  rust/kernel/init.rs            |  1 -
>  rust/kernel/init/__internal.rs | 25 +++------------
>  rust/kernel/init/macros.rs     | 56 ++++++++++++----------------------
>  3 files changed, 23 insertions(+), 59 deletions(-)
> 
> diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> index d9a91950cba2..ecf6a4bd0ce4 100644
> --- a/rust/kernel/init.rs
> +++ b/rust/kernel/init.rs
> @@ -206,7 +206,6 @@
>  use alloc::boxed::Box;
>  use core::{
>      alloc::AllocError,
> -    cell::Cell,
>      convert::Infallible,
>      marker::PhantomData,
>      mem::MaybeUninit,
> diff --git a/rust/kernel/init/__internal.rs b/rust/kernel/init/__internal.rs
> index 44751fb62b51..7abd1fb65e41 100644
> --- a/rust/kernel/init/__internal.rs
> +++ b/rust/kernel/init/__internal.rs
> @@ -174,7 +174,6 @@ pub fn init<E>(self: Pin<&mut Self>, init: impl PinInit<T, E>) -> Result<Pin<&mu
>  /// Can be forgotten to prevent the drop.
>  pub struct DropGuard<T: ?Sized> {
>      ptr: *mut T,
> -    do_drop: Cell<bool>,
>  }
> 
>  impl<T: ?Sized> DropGuard<T> {
> @@ -190,32 +189,16 @@ impl<T: ?Sized> DropGuard<T> {
>      /// - will not be dropped by any other means.
>      #[inline]
>      pub unsafe fn new(ptr: *mut T) -> Self {
> -        Self {
> -            ptr,
> -            do_drop: Cell::new(true),
> -        }
> -    }
> -
> -    /// Prevents this guard from dropping the supplied pointer.
> -    ///
> -    /// # Safety
> -    ///
> -    /// This function is unsafe in order to prevent safe code from forgetting this guard. It should
> -    /// only be called by the macros in this module.
> -    #[inline]
> -    pub unsafe fn forget(&self) {
> -        self.do_drop.set(false);
> +        Self { ptr }
>      }
>  }
> 
>  impl<T: ?Sized> Drop for DropGuard<T> {
>      #[inline]
>      fn drop(&mut self) {
> -        if self.do_drop.get() {
> -            // SAFETY: A `DropGuard` can only be constructed using the unsafe `new` function
> -            // ensuring that this operation is safe.
> -            unsafe { ptr::drop_in_place(self.ptr) }
> -        }
> +        // SAFETY: A `DropGuard` can only be constructed using the unsafe `new` function
> +        // ensuring that this operation is safe.
> +        unsafe { ptr::drop_in_place(self.ptr) }
>      }
>  }
> 
> diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
> index e8165ff53a94..df4281743175 100644
> --- a/rust/kernel/init/macros.rs
> +++ b/rust/kernel/init/macros.rs
> @@ -992,7 +992,6 @@ impl<$($impl_generics)*> $pin_data<$($ty_generics)*>
>  /// - `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.
> -/// - `forget_guards`: recursively forget the drop guards for every field.
>  #[doc(hidden)]
>  #[macro_export]
>  macro_rules! __init_internal {
> @@ -1032,6 +1031,7 @@ macro_rules! __init_internal {
>                      $crate::__init_internal!(init_slot($($use_data)?):
>                          @data(data),
>                          @slot(slot),
> +                        @guards(),
>                          @munch_fields($($fields)*,),
>                      );
>                      // We use unreachable code to ensure that all fields have been mentioned exactly
> @@ -1046,10 +1046,6 @@ macro_rules! __init_internal {
>                              @acc(),
>                          );
>                      }
> -                    // Forget all guards, since initialization was a success.
> -                    $crate::__init_internal!(forget_guards:
> -                        @munch_fields($($fields)*,),
> -                    );
>                  }
>                  Ok(__InitOk)
>              }
> @@ -1063,13 +1059,17 @@ macro_rules! __init_internal {
>      (init_slot($($use_data:ident)?):
>          @data($data:ident),
>          @slot($slot:ident),
> +        @guards($($guards:ident,)*),
>          @munch_fields($(,)?),
>      ) => {
> -        // Endpoint of munching, no fields are left.
> +        // 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.
> +        $(::core::mem::forget($guards);)*
>      };
>      (init_slot($use_data:ident): // use_data is present, so we use the `data` to init fields.
>          @data($data:ident),
>          @slot($slot:ident),
> +        @guards($($guards:ident,)*),
>          // In-place initialization syntax.
>          @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
>      ) => {
> @@ -1080,24 +1080,26 @@ macro_rules! __init_internal {
>          // return when an error/panic occurs.
>          // We also use the `data` to require the correct trait (`Init` or `PinInit`) for `$field`.
>          unsafe { $data.$field(::core::ptr::addr_of_mut!((*$slot).$field), $field)? };
> -        // Create the drop guard.
> +        // Create the drop guard:
>          //
> -        // We only give access to `&DropGuard`, so it cannot be forgotten via safe code.
> +        // We rely on macro hygiene to make it impossible for users to access this local variable.
>          //
>          // SAFETY: We forget the guard later when initialization has succeeded.
> -        let $field = &unsafe {
> +        let guard = unsafe {
>              $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
>          };
> 
>          $crate::__init_internal!(init_slot($use_data):
>              @data($data),
>              @slot($slot),
> +            @guards(guard, $($guards,)*),
>              @munch_fields($($rest)*),
>          );
>      };
>      (init_slot(): // no use_data, so we use `Init::__init` directly.
>          @data($data:ident),
>          @slot($slot:ident),
> +        @guards($($guards:ident,)*),
>          // In-place initialization syntax.
>          @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
>      ) => {
> @@ -1107,24 +1109,26 @@ macro_rules! __init_internal {
>          // SAFETY: `slot` is valid, because we are inside of an initializer closure, we
>          // return when an error/panic occurs.
>          unsafe { $crate::init::Init::__init($field, ::core::ptr::addr_of_mut!((*$slot).$field))? };
> -        // Create the drop guard.
> +        // Create the drop guard:
>          //
> -        // We only give access to `&DropGuard`, so it cannot be forgotten via safe code.
> +        // We rely on macro hygiene to make it impossible for users to access this local variable.
>          //
>          // SAFETY: We forget the guard later when initialization has succeeded.
> -        let $field = &unsafe {
> +        let guard = unsafe {
>              $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
>          };
> 
>          $crate::__init_internal!(init_slot():
>              @data($data),
>              @slot($slot),
> +            @guards(guard, $($guards,)*),
>              @munch_fields($($rest)*),
>          );
>      };
>      (init_slot($($use_data:ident)?):
>          @data($data:ident),
>          @slot($slot:ident),
> +        @guards($($guards:ident,)*),
>          // Init by-value.
>          @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
>      ) => {
> @@ -1135,16 +1139,17 @@ macro_rules! __init_internal {
>          unsafe { ::core::ptr::write(::core::ptr::addr_of_mut!((*$slot).$field), $field) };
>          // Create the drop guard:
>          //
> -        // We only give access to `&DropGuard`, so it cannot be accidentally forgotten.
> +        // We rely on macro hygiene to make it impossible for users to access this local variable.
>          //
>          // SAFETY: We forget the guard later when initialization has succeeded.
> -        let $field = &unsafe {
> +        let guard = unsafe {
>              $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
>          };
> 
>          $crate::__init_internal!(init_slot($($use_data)?):
>              @data($data),
>              @slot($slot),
> +            @guards(guard, $($guards,)*),
>              @munch_fields($($rest)*),
>          );
>      };
> @@ -1189,29 +1194,6 @@ macro_rules! __init_internal {
>              @acc($($acc)* $field: ::core::panic!(),),
>          );
>      };
> -    (forget_guards:
> -        @munch_fields($(,)?),
> -    ) => {
> -        // Munching finished.
> -    };
> -    (forget_guards:
> -        @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
> -    ) => {
> -        unsafe { $crate::init::__internal::DropGuard::forget($field) };
> -
> -        $crate::__init_internal!(forget_guards:
> -            @munch_fields($($rest)*),
> -        );
> -    };
> -    (forget_guards:
> -        @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
> -    ) => {
> -        unsafe { $crate::init::__internal::DropGuard::forget($field) };
> -
> -        $crate::__init_internal!(forget_guards:
> -            @munch_fields($($rest)*),
> -        );
> -    };
>  }
> 
>  #[doc(hidden)]
> --
> 2.41.0
> 
>
  
Benno Lossin June 28, 2023, 11:41 a.m. UTC | #3
On 25.06.23 22:54, Gary Guo wrote:
> On Sat, 24 Jun 2023 09:25:10 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>> Use hygienic identifiers for the guards instead of the field names. This
>> makes the init macros feel more like normal struct initializers, since
>> assigning identifiers with the name of a field does not create
>> conflicts.
>> Also change the internals of the guards, no need to make the `forget`
>> function `unsafe`, since users cannot access the guards anyways. Now the
>> guards are carried directly on the stack and have no extra `Cell<bool>`
>> field that marks if they have been forgotten or not, instead they are
>> just forgotten via `mem::forget`.
> 
> The code LGTM, so:
> 
> Reviewed-by: Gary Guo <gary@garyguo.net>
> 
> Although this will cause the new expansion we have to be no longer
> compatible with a totally-proc-macro impl, if we want to do everything
> in proc macro in the future.
> 
> If we have the paste macro upstream (
> https://github.com/nbdd0121/linux/commit/fff00461b0be7fd3ec218dcc428f25886b5ec04a
> ) then we can replace the `guard` with `paste!([<$field>])` and keep
> the expansion identical.
> 

I tried it and it seems to work, but I am not sure why the hygiene is
set correctly. Could you maybe explain why this works?
```
        $crate::__internal::paste!{
            let [<$field>] = unsafe {
                $crate::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
            };
            $crate::__init_internal!(init_slot($use_data):
                @data($data),
                @slot($slot),
                @guards([<$field>], $($guards,)*),
                @munch_fields($($rest)*),
            );
        }
```

i.e. why can't a user access the guard? I think it is because the hygiene of the `[<>]`
is used, but not sure why that works.
  
Gary Guo June 28, 2023, 4:48 p.m. UTC | #4
On Wed, 28 Jun 2023 11:41:59 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 25.06.23 22:54, Gary Guo wrote:
> > On Sat, 24 Jun 2023 09:25:10 +0000
> > Benno Lossin <benno.lossin@proton.me> wrote:
> >   
> >> Use hygienic identifiers for the guards instead of the field names. This
> >> makes the init macros feel more like normal struct initializers, since
> >> assigning identifiers with the name of a field does not create
> >> conflicts.
> >> Also change the internals of the guards, no need to make the `forget`
> >> function `unsafe`, since users cannot access the guards anyways. Now the
> >> guards are carried directly on the stack and have no extra `Cell<bool>`
> >> field that marks if they have been forgotten or not, instead they are
> >> just forgotten via `mem::forget`.  
> > 
> > The code LGTM, so:
> > 
> > Reviewed-by: Gary Guo <gary@garyguo.net>
> > 
> > Although this will cause the new expansion we have to be no longer
> > compatible with a totally-proc-macro impl, if we want to do everything
> > in proc macro in the future.
> > 
> > If we have the paste macro upstream (
> > https://github.com/nbdd0121/linux/commit/fff00461b0be7fd3ec218dcc428f25886b5ec04a
> > ) then we can replace the `guard` with `paste!([<$field>])` and keep
> > the expansion identical.
> >   
> 
> I tried it and it seems to work, but I am not sure why the hygiene is
> set correctly. Could you maybe explain why this works?
> ```
>         $crate::__internal::paste!{
>             let [<$field>] = unsafe {
>                 $crate::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
>             };
>             $crate::__init_internal!(init_slot($use_data):
>                 @data($data),
>                 @slot($slot),
>                 @guards([<$field>], $($guards,)*),
>                 @munch_fields($($rest)*),
>             );
>         }
> ```
> 
> i.e. why can't a user access the guard? I think it is because the hygiene of the `[<>]`
> is used, but not sure why that works.

Yes, by default the hygiene of pasted macro is that of the group,
unless explicitly overriden.

Best,
Gary
  

Patch

diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index d9a91950cba2..ecf6a4bd0ce4 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -206,7 +206,6 @@ 
 use alloc::boxed::Box;
 use core::{
     alloc::AllocError,
-    cell::Cell,
     convert::Infallible,
     marker::PhantomData,
     mem::MaybeUninit,
diff --git a/rust/kernel/init/__internal.rs b/rust/kernel/init/__internal.rs
index 44751fb62b51..7abd1fb65e41 100644
--- a/rust/kernel/init/__internal.rs
+++ b/rust/kernel/init/__internal.rs
@@ -174,7 +174,6 @@  pub fn init<E>(self: Pin<&mut Self>, init: impl PinInit<T, E>) -> Result<Pin<&mu
 /// Can be forgotten to prevent the drop.
 pub struct DropGuard<T: ?Sized> {
     ptr: *mut T,
-    do_drop: Cell<bool>,
 }

 impl<T: ?Sized> DropGuard<T> {
@@ -190,32 +189,16 @@  impl<T: ?Sized> DropGuard<T> {
     /// - will not be dropped by any other means.
     #[inline]
     pub unsafe fn new(ptr: *mut T) -> Self {
-        Self {
-            ptr,
-            do_drop: Cell::new(true),
-        }
-    }
-
-    /// Prevents this guard from dropping the supplied pointer.
-    ///
-    /// # Safety
-    ///
-    /// This function is unsafe in order to prevent safe code from forgetting this guard. It should
-    /// only be called by the macros in this module.
-    #[inline]
-    pub unsafe fn forget(&self) {
-        self.do_drop.set(false);
+        Self { ptr }
     }
 }

 impl<T: ?Sized> Drop for DropGuard<T> {
     #[inline]
     fn drop(&mut self) {
-        if self.do_drop.get() {
-            // SAFETY: A `DropGuard` can only be constructed using the unsafe `new` function
-            // ensuring that this operation is safe.
-            unsafe { ptr::drop_in_place(self.ptr) }
-        }
+        // SAFETY: A `DropGuard` can only be constructed using the unsafe `new` function
+        // ensuring that this operation is safe.
+        unsafe { ptr::drop_in_place(self.ptr) }
     }
 }

diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
index e8165ff53a94..df4281743175 100644
--- a/rust/kernel/init/macros.rs
+++ b/rust/kernel/init/macros.rs
@@ -992,7 +992,6 @@  impl<$($impl_generics)*> $pin_data<$($ty_generics)*>
 /// - `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.
-/// - `forget_guards`: recursively forget the drop guards for every field.
 #[doc(hidden)]
 #[macro_export]
 macro_rules! __init_internal {
@@ -1032,6 +1031,7 @@  macro_rules! __init_internal {
                     $crate::__init_internal!(init_slot($($use_data)?):
                         @data(data),
                         @slot(slot),
+                        @guards(),
                         @munch_fields($($fields)*,),
                     );
                     // We use unreachable code to ensure that all fields have been mentioned exactly
@@ -1046,10 +1046,6 @@  macro_rules! __init_internal {
                             @acc(),
                         );
                     }
-                    // Forget all guards, since initialization was a success.
-                    $crate::__init_internal!(forget_guards:
-                        @munch_fields($($fields)*,),
-                    );
                 }
                 Ok(__InitOk)
             }
@@ -1063,13 +1059,17 @@  macro_rules! __init_internal {
     (init_slot($($use_data:ident)?):
         @data($data:ident),
         @slot($slot:ident),
+        @guards($($guards:ident,)*),
         @munch_fields($(,)?),
     ) => {
-        // Endpoint of munching, no fields are left.
+        // 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.
+        $(::core::mem::forget($guards);)*
     };
     (init_slot($use_data:ident): // use_data is present, so we use the `data` to init fields.
         @data($data:ident),
         @slot($slot:ident),
+        @guards($($guards:ident,)*),
         // In-place initialization syntax.
         @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
     ) => {
@@ -1080,24 +1080,26 @@  macro_rules! __init_internal {
         // return when an error/panic occurs.
         // We also use the `data` to require the correct trait (`Init` or `PinInit`) for `$field`.
         unsafe { $data.$field(::core::ptr::addr_of_mut!((*$slot).$field), $field)? };
-        // Create the drop guard.
+        // Create the drop guard:
         //
-        // We only give access to `&DropGuard`, so it cannot be forgotten via safe code.
+        // We rely on macro hygiene to make it impossible for users to access this local variable.
         //
         // SAFETY: We forget the guard later when initialization has succeeded.
-        let $field = &unsafe {
+        let guard = unsafe {
             $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
         };

         $crate::__init_internal!(init_slot($use_data):
             @data($data),
             @slot($slot),
+            @guards(guard, $($guards,)*),
             @munch_fields($($rest)*),
         );
     };
     (init_slot(): // no use_data, so we use `Init::__init` directly.
         @data($data:ident),
         @slot($slot:ident),
+        @guards($($guards:ident,)*),
         // In-place initialization syntax.
         @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
     ) => {
@@ -1107,24 +1109,26 @@  macro_rules! __init_internal {
         // SAFETY: `slot` is valid, because we are inside of an initializer closure, we
         // return when an error/panic occurs.
         unsafe { $crate::init::Init::__init($field, ::core::ptr::addr_of_mut!((*$slot).$field))? };
-        // Create the drop guard.
+        // Create the drop guard:
         //
-        // We only give access to `&DropGuard`, so it cannot be forgotten via safe code.
+        // We rely on macro hygiene to make it impossible for users to access this local variable.
         //
         // SAFETY: We forget the guard later when initialization has succeeded.
-        let $field = &unsafe {
+        let guard = unsafe {
             $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
         };

         $crate::__init_internal!(init_slot():
             @data($data),
             @slot($slot),
+            @guards(guard, $($guards,)*),
             @munch_fields($($rest)*),
         );
     };
     (init_slot($($use_data:ident)?):
         @data($data:ident),
         @slot($slot:ident),
+        @guards($($guards:ident,)*),
         // Init by-value.
         @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
     ) => {
@@ -1135,16 +1139,17 @@  macro_rules! __init_internal {
         unsafe { ::core::ptr::write(::core::ptr::addr_of_mut!((*$slot).$field), $field) };
         // Create the drop guard:
         //
-        // We only give access to `&DropGuard`, so it cannot be accidentally forgotten.
+        // We rely on macro hygiene to make it impossible for users to access this local variable.
         //
         // SAFETY: We forget the guard later when initialization has succeeded.
-        let $field = &unsafe {
+        let guard = unsafe {
             $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
         };

         $crate::__init_internal!(init_slot($($use_data)?):
             @data($data),
             @slot($slot),
+            @guards(guard, $($guards,)*),
             @munch_fields($($rest)*),
         );
     };
@@ -1189,29 +1194,6 @@  macro_rules! __init_internal {
             @acc($($acc)* $field: ::core::panic!(),),
         );
     };
-    (forget_guards:
-        @munch_fields($(,)?),
-    ) => {
-        // Munching finished.
-    };
-    (forget_guards:
-        @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
-    ) => {
-        unsafe { $crate::init::__internal::DropGuard::forget($field) };
-
-        $crate::__init_internal!(forget_guards:
-            @munch_fields($($rest)*),
-        );
-    };
-    (forget_guards:
-        @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
-    ) => {
-        unsafe { $crate::init::__internal::DropGuard::forget($field) };
-
-        $crate::__init_internal!(forget_guards:
-            @munch_fields($($rest)*),
-        );
-    };
 }

 #[doc(hidden)]