[PATCH] rust: init: change the generated name of guard variables

[PATCH] rust: init: change the generated name of guard variables

[Benno Lossin]

The initializers created by the `[try_][pin_]init!` macros utilize the
guard pattern to drop already initialized fields, when initialization
fails mid-way. These guards are generated to have the same name as the
field that they handle. To prevent namespacing issues when the field
name is the same as e.g. a constant name, add `__` as a prefix and
`_guard` as the suffix.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
 rust/kernel/init/macros.rs | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
index cb6e61b6c50b..93bf4c3080f9 100644
--- a/rust/kernel/init/macros.rs
+++ b/rust/kernel/init/macros.rs
@@ -250,7 +250,7 @@
 //!                     // error type is `Infallible`) we will need to drop this field if there
 //!                     // is an error later. This `DropGuard` will drop the field when it gets
 //!                     // dropped and has not yet been forgotten.
-//!                     let t = unsafe {
+//!                     let __t_guard = unsafe {
 //!                         ::pinned_init::__internal::DropGuard::new(::core::addr_of_mut!((*slot).t))
 //!                     };
 //!                     // Expansion of `x: 0,`:
@@ -261,14 +261,14 @@
 //!                         unsafe { ::core::ptr::write(::core::addr_of_mut!((*slot).x), x) };
 //!                     }
 //!                     // We again create a `DropGuard`.
-//!                     let x = unsafe {
+//!                     let __x_guard = unsafe {
 //!                         ::kernel::init::__internal::DropGuard::new(::core::addr_of_mut!((*slot).x))
 //!                     };
 //!                     // Since initialization has successfully completed, we can now forget
 //!                     // the guards. This is not `mem::forget`, since we only have
 //!                     // `&DropGuard`.
-//!                     ::core::mem::forget(x);
-//!                     ::core::mem::forget(t);
+//!                     ::core::mem::forget(__x_guard);
+//!                     ::core::mem::forget(__t_guard);
 //!                     // Here we use the type checker to ensure that every field has been
 //!                     // initialized exactly once, since this is `if false` it will never get
 //!                     // executed, but still type-checked.
@@ -461,16 +461,16 @@
 //!             {
 //!                 unsafe { ::core::ptr::write(::core::addr_of_mut!((*slot).a), a) };
 //!             }
-//!             let a = unsafe {
+//!             let __a_guard = unsafe {
 //!                 ::kernel::init::__internal::DropGuard::new(::core::addr_of_mut!((*slot).a))
 //!             };
 //!             let init = Bar::new(36);
 //!             unsafe { data.b(::core::addr_of_mut!((*slot).b), b)? };
-//!             let b = unsafe {
+//!             let __b_guard = unsafe {
 //!                 ::kernel::init::__internal::DropGuard::new(::core::addr_of_mut!((*slot).b))
 //!             };
-//!             ::core::mem::forget(b);
-//!             ::core::mem::forget(a);
+//!             ::core::mem::forget(__b_guard);
+//!             ::core::mem::forget(__a_guard);
 //!             #[allow(unreachable_code, clippy::diverging_sub_expression)]
 //!             let _ = || {
 //!                 unsafe {
@@ -1192,14 +1192,14 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
         // We use `paste!` to create new hygiene for `$field`.
         ::kernel::macros::paste! {
             // SAFETY: We forget the guard later when initialization has succeeded.
-            let [<$field>] = unsafe {
+            let [< __ $field _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([<$field>], $($guards,)*),
+                @guards([< __ $field _guard >], $($guards,)*),
                 @munch_fields($($rest)*),
             );
         }
@@ -1223,14 +1223,14 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
         // We use `paste!` to create new hygiene for `$field`.
         ::kernel::macros::paste! {
             // SAFETY: We forget the guard later when initialization has succeeded.
-            let [<$field>] = unsafe {
+            let [< __ $field _guard >] = unsafe {
                 $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
             };
 
             $crate::__init_internal!(init_slot():
                 @data($data),
                 @slot($slot),
-                @guards([<$field>], $($guards,)*),
+                @guards([< __ $field _guard >], $($guards,)*),
                 @munch_fields($($rest)*),
             );
         }
@@ -1255,14 +1255,14 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
         // We use `paste!` to create new hygiene for `$field`.
         ::kernel::macros::paste! {
             // SAFETY: We forget the guard later when initialization has succeeded.
-            let [<$field>] = unsafe {
+            let [< __ $field _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([<$field>], $($guards,)*),
+                @guards([< __ $field _guard >], $($guards,)*),
                 @munch_fields($($rest)*),
             );
         }

base-commit: 9ffe2a730313f27cebd0859ea856247ac59c576c
-- 
2.44.0

Re: [PATCH] rust: init: change the generated name of guard variables

[Boqun Feng]

On Wed, Apr 03, 2024 at 07:43:37PM +0000, Benno Lossin wrote:
> The initializers created by the `[try_][pin_]init!` macros utilize the
> guard pattern to drop already initialized fields, when initialization
> fails mid-way. These guards are generated to have the same name as the
> field that they handle. To prevent namespacing issues when the field

Do you have an example of this kind of issues?

Regards,
Boqun

> name is the same as e.g. a constant name, add `__` as a prefix and
> `_guard` as the suffix.
> 
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
>  rust/kernel/init/macros.rs | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
> index cb6e61b6c50b..93bf4c3080f9 100644
> --- a/rust/kernel/init/macros.rs
> +++ b/rust/kernel/init/macros.rs
> @@ -250,7 +250,7 @@
>  //!                     // error type is `Infallible`) we will need to drop this field if there
>  //!                     // is an error later. This `DropGuard` will drop the field when it gets
>  //!                     // dropped and has not yet been forgotten.
> -//!                     let t = unsafe {
> +//!                     let __t_guard = unsafe {
>  //!                         ::pinned_init::__internal::DropGuard::new(::core::addr_of_mut!((*slot).t))
>  //!                     };
>  //!                     // Expansion of `x: 0,`:
> @@ -261,14 +261,14 @@
>  //!                         unsafe { ::core::ptr::write(::core::addr_of_mut!((*slot).x), x) };
>  //!                     }
>  //!                     // We again create a `DropGuard`.
> -//!                     let x = unsafe {
> +//!                     let __x_guard = unsafe {
>  //!                         ::kernel::init::__internal::DropGuard::new(::core::addr_of_mut!((*slot).x))
>  //!                     };
>  //!                     // Since initialization has successfully completed, we can now forget
>  //!                     // the guards. This is not `mem::forget`, since we only have
>  //!                     // `&DropGuard`.
> -//!                     ::core::mem::forget(x);
> -//!                     ::core::mem::forget(t);
> +//!                     ::core::mem::forget(__x_guard);
> +//!                     ::core::mem::forget(__t_guard);
>  //!                     // Here we use the type checker to ensure that every field has been
>  //!                     // initialized exactly once, since this is `if false` it will never get
>  //!                     // executed, but still type-checked.
> @@ -461,16 +461,16 @@
>  //!             {
>  //!                 unsafe { ::core::ptr::write(::core::addr_of_mut!((*slot).a), a) };
>  //!             }
> -//!             let a = unsafe {
> +//!             let __a_guard = unsafe {
>  //!                 ::kernel::init::__internal::DropGuard::new(::core::addr_of_mut!((*slot).a))
>  //!             };
>  //!             let init = Bar::new(36);
>  //!             unsafe { data.b(::core::addr_of_mut!((*slot).b), b)? };
> -//!             let b = unsafe {
> +//!             let __b_guard = unsafe {
>  //!                 ::kernel::init::__internal::DropGuard::new(::core::addr_of_mut!((*slot).b))
>  //!             };
> -//!             ::core::mem::forget(b);
> -//!             ::core::mem::forget(a);
> +//!             ::core::mem::forget(__b_guard);
> +//!             ::core::mem::forget(__a_guard);
>  //!             #[allow(unreachable_code, clippy::diverging_sub_expression)]
>  //!             let _ = || {
>  //!                 unsafe {
> @@ -1192,14 +1192,14 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
>          // We use `paste!` to create new hygiene for `$field`.
>          ::kernel::macros::paste! {
>              // SAFETY: We forget the guard later when initialization has succeeded.
> -            let [<$field>] = unsafe {
> +            let [< __ $field _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([<$field>], $($guards,)*),
> +                @guards([< __ $field _guard >], $($guards,)*),
>                  @munch_fields($($rest)*),
>              );
>          }
> @@ -1223,14 +1223,14 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
>          // We use `paste!` to create new hygiene for `$field`.
>          ::kernel::macros::paste! {
>              // SAFETY: We forget the guard later when initialization has succeeded.
> -            let [<$field>] = unsafe {
> +            let [< __ $field _guard >] = unsafe {
>                  $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
>              };
>  
>              $crate::__init_internal!(init_slot():
>                  @data($data),
>                  @slot($slot),
> -                @guards([<$field>], $($guards,)*),
> +                @guards([< __ $field _guard >], $($guards,)*),
>                  @munch_fields($($rest)*),
>              );
>          }
> @@ -1255,14 +1255,14 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
>          // We use `paste!` to create new hygiene for `$field`.
>          ::kernel::macros::paste! {
>              // SAFETY: We forget the guard later when initialization has succeeded.
> -            let [<$field>] = unsafe {
> +            let [< __ $field _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([<$field>], $($guards,)*),
> +                @guards([< __ $field _guard >], $($guards,)*),
>                  @munch_fields($($rest)*),
>              );
>          }
> 
> base-commit: 9ffe2a730313f27cebd0859ea856247ac59c576c
> -- 
> 2.44.0
> 
> 
> 

Re: [PATCH] rust: init: change the generated name of guard variables

[Benno Lossin]

On 03.04.24 23:20, Boqun Feng wrote:
> On Wed, Apr 03, 2024 at 07:43:37PM +0000, Benno Lossin wrote:
>> The initializers created by the `[try_][pin_]init!` macros utilize the
>> guard pattern to drop already initialized fields, when initialization
>> fails mid-way. These guards are generated to have the same name as the
>> field that they handle. To prevent namespacing issues when the field
> 
> Do you have an example of this kind of issues?

https://lore.kernel.org/rust-for-linux/1e8a2a1f-abbf-44ba-8344-705a9cbb1627@proton.me/

-- 
Cheers,
Benno

Re: [PATCH] rust: init: change the generated name of guard variables

[Boqun Feng]

On Wed, Apr 03, 2024 at 10:09:49PM +0000, Benno Lossin wrote:
> On 03.04.24 23:20, Boqun Feng wrote:
> > On Wed, Apr 03, 2024 at 07:43:37PM +0000, Benno Lossin wrote:
> >> The initializers created by the `[try_][pin_]init!` macros utilize the
> >> guard pattern to drop already initialized fields, when initialization
> >> fails mid-way. These guards are generated to have the same name as the
> >> field that they handle. To prevent namespacing issues when the field
> > 
> > Do you have an example of this kind of issues?
> 
> https://lore.kernel.org/rust-for-linux/1e8a2a1f-abbf-44ba-8344-705a9cbb1627@proton.me/
> 

Ok, so a new binding cannot shadow the name of a constant, that's a bit
surprising, but seems makes sense.

The solution is not ideal (for example, a constant can have the name
"__something_guard"), but nothing better we could I guess.

FWIW:

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> -- 
> Cheers,
> Benno
> 
> 

Re: [PATCH] rust: init: change the generated name of guard variables

[Benno Lossin]

On 04.04.24 00:38, Boqun Feng wrote:
> On Wed, Apr 03, 2024 at 10:09:49PM +0000, Benno Lossin wrote:
>> On 03.04.24 23:20, Boqun Feng wrote:
>>> On Wed, Apr 03, 2024 at 07:43:37PM +0000, Benno Lossin wrote:
>>>> The initializers created by the `[try_][pin_]init!` macros utilize the
>>>> guard pattern to drop already initialized fields, when initialization
>>>> fails mid-way. These guards are generated to have the same name as the
>>>> field that they handle. To prevent namespacing issues when the field
>>>
>>> Do you have an example of this kind of issues?
>>
>> https://lore.kernel.org/rust-for-linux/1e8a2a1f-abbf-44ba-8344-705a9cbb1627@proton.me/
>>
> 
> Ok, so a new binding cannot shadow the name of a constant, that's a bit
> surprising, but seems makes sense.
> 
> The solution is not ideal (for example, a constant can have the name
> "__something_guard"), but nothing better we could I guess.

Yeah that would also be problematic. I think the problem from the thread
is best solved by making the constant there upper case, so it would also
avoid this issue.
But debugging the error in the macro is a bit annoying, so if someone
encounters this again, they don't have to deal with that.

-- 
Cheers,
Benno

Re: [PATCH] rust: init: change the generated name of guard variables

[Gary Guo]

On Wed, 03 Apr 2024 22:09:49 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 03.04.24 23:20, Boqun Feng wrote:
> > On Wed, Apr 03, 2024 at 07:43:37PM +0000, Benno Lossin wrote:  
> >> The initializers created by the `[try_][pin_]init!` macros utilize the
> >> guard pattern to drop already initialized fields, when initialization
> >> fails mid-way. These guards are generated to have the same name as the
> >> field that they handle. To prevent namespacing issues when the field  
> > 
> > Do you have an example of this kind of issues?  
> 
> https://lore.kernel.org/rust-for-linux/1e8a2a1f-abbf-44ba-8344-705a9cbb1627@proton.me/
> 

Here's the simplified example:

```
macro_rules! f {
    () => {
        let a = 1;
        let _: u32 = a;
    }
}

const a: u64 = 1;

fn main() {
    f!();
}
```

The `a` in `f` have a different hygiene so normally it is scoped to the
macro expansion and wouldn't escape. Interestingly a constant is still
preferred despite the hygiene so constants escaped into the macro,
leading to the error.

Would your change regress error message when `pin_init!` is used
wrongly? Personally I would say this kind of error is niche enough
(given the casing of constants and variables differ) that we probably
don't really need to care. So if error message would be affected then
we'd better off not making the change.

Best,
Gary

Re: [PATCH] rust: init: change the generated name of guard variables

[Benno Lossin]

On 17.04.24 17:06, Gary Guo wrote:
> On Wed, 03 Apr 2024 22:09:49 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>> On 03.04.24 23:20, Boqun Feng wrote:
>>> On Wed, Apr 03, 2024 at 07:43:37PM +0000, Benno Lossin wrote:
>>>> The initializers created by the `[try_][pin_]init!` macros utilize the
>>>> guard pattern to drop already initialized fields, when initialization
>>>> fails mid-way. These guards are generated to have the same name as the
>>>> field that they handle. To prevent namespacing issues when the field
>>>
>>> Do you have an example of this kind of issues?
>>
>> https://lore.kernel.org/rust-for-linux/1e8a2a1f-abbf-44ba-8344-705a9cbb1627@proton.me/
>>
> 
> Here's the simplified example:
> 
> ```
> macro_rules! f {
>      () => {
>          let a = 1;
>          let _: u32 = a;
>      }
> }
> 
> const a: u64 = 1;
> 
> fn main() {
>      f!();
> }
> ```
> 
> The `a` in `f` have a different hygiene so normally it is scoped to the
> macro expansion and wouldn't escape. Interestingly a constant is still
> preferred despite the hygiene so constants escaped into the macro,
> leading to the error.
> 
> Would your change regress error message when `pin_init!` is used
> wrongly? Personally I would say this kind of error is niche enough
> (given the casing of constants and variables differ) that we probably
> don't really need to care. So if error message would be affected then
> we'd better off not making the change.

For all the tested error messages (see [1]) there is absolutely no
difference in the diagnostic.

[1]: https://github.com/Rust-for-Linux/pinned-init/tree/main/tests/ui/compile-fail

-- 
Cheers,
Benno

Re: [PATCH] rust: init: change the generated name of guard variables

[Alice Ryhl]

On Wed, Apr 3, 2024 at 9:43 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> The initializers created by the `[try_][pin_]init!` macros utilize the
> guard pattern to drop already initialized fields, when initialization
> fails mid-way. These guards are generated to have the same name as the
> field that they handle. To prevent namespacing issues when the field
> name is the same as e.g. a constant name, add `__` as a prefix and
> `_guard` as the suffix.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH] rust: init: change the generated name of guard variables

[Miguel Ojeda]

On Wed, Apr 3, 2024 at 9:43 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> The initializers created by the `[try_][pin_]init!` macros utilize the
> guard pattern to drop already initialized fields, when initialization
> fails mid-way. These guards are generated to have the same name as the
> field that they handle. To prevent namespacing issues when the field
> name is the same as e.g. a constant name, add `__` as a prefix and
> `_guard` as the suffix.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>

[ Added Benno's link and Gary's simplified example. - Miguel ]

Applied to `rust-next` -- thanks everyone!

Cheers,
Miguel