* [PATCH v2 01/12] rust: init: consolidate init macros
2023-07-19 14:20 [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
@ 2023-07-19 14:20 ` Benno Lossin
2023-07-19 18:37 ` Martin Rodriguez Reboredo
2023-07-20 13:09 ` Alice Ryhl
2023-07-19 14:20 ` [PATCH v2 02/12] rust: add derive macro for `Zeroable` Benno Lossin
` (11 subsequent siblings)
12 siblings, 2 replies; 43+ messages in thread
From: Benno Lossin @ 2023-07-19 14:20 UTC (permalink / raw)
To: Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, Andreas Hindborg, rust-for-linux, linux-kernel
Merges the implementations of `try_init!` and `try_pin_init!`. These two
macros are very similar, but use different traits. The new macro
`__init_internal!` that is now the implementation for both takes these
traits as parameters.
This change does not affect any users, as no public API has been
changed, but it should simplify maintaining the init macros.
Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
v1 -> v2:
* added Reviewed-by from Björn
rust/kernel/init.rs | 388 +++----------------------------------
rust/kernel/init/macros.rs | 237 +++++++++++++++++++++-
2 files changed, 259 insertions(+), 366 deletions(-)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index e05563aad2ed..d431d0b153a2 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -548,11 +548,14 @@ macro_rules! pin_init {
($(&$this:ident in)? $t:ident $(::<$($generics:ty),* $(,)?>)? {
$($fields:tt)*
}) => {
- $crate::try_pin_init!(
+ $crate::__init_internal!(
@this($($this)?),
@typ($t $(::<$($generics),*>)?),
@fields($($fields)*),
@error(::core::convert::Infallible),
+ @data(PinData, use_data),
+ @has_data(HasPinData, __pin_data),
+ @construct_closure(pin_init_from_closure),
)
};
}
@@ -601,205 +604,29 @@ macro_rules! try_pin_init {
($(&$this:ident in)? $t:ident $(::<$($generics:ty),* $(,)?>)? {
$($fields:tt)*
}) => {
- $crate::try_pin_init!(
+ $crate::__init_internal!(
@this($($this)?),
@typ($t $(::<$($generics),*>)? ),
@fields($($fields)*),
@error($crate::error::Error),
+ @data(PinData, use_data),
+ @has_data(HasPinData, __pin_data),
+ @construct_closure(pin_init_from_closure),
)
};
($(&$this:ident in)? $t:ident $(::<$($generics:ty),* $(,)?>)? {
$($fields:tt)*
}? $err:ty) => {
- $crate::try_pin_init!(
+ $crate::__init_internal!(
@this($($this)?),
@typ($t $(::<$($generics),*>)? ),
@fields($($fields)*),
@error($err),
+ @data(PinData, use_data),
+ @has_data(HasPinData, __pin_data),
+ @construct_closure(pin_init_from_closure),
)
};
- (
- @this($($this:ident)?),
- @typ($t:ident $(::<$($generics:ty),*>)?),
- @fields($($fields:tt)*),
- @error($err:ty),
- ) => {{
- // 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
- // no possibility of returning without `unsafe`.
- struct __InitOk;
- // Get the pin data from the supplied type.
- let data = unsafe {
- use $crate::init::__internal::HasPinData;
- $t$(::<$($generics),*>)?::__pin_data()
- };
- // Ensure that `data` really is of type `PinData` and help with type inference:
- let init = $crate::init::__internal::PinData::make_closure::<_, __InitOk, $err>(
- data,
- move |slot| {
- {
- // Shadow the structure so it cannot be used to return early.
- struct __InitOk;
- // 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) };)?
- // Initialize every field.
- $crate::try_pin_init!(init_slot:
- @data(data),
- @slot(slot),
- @munch_fields($($fields)*,),
- );
- // We use unreachable code to ensure that all fields have been mentioned exactly
- // once, this struct initializer will still be type-checked and complain with a
- // very natural error message if a field is forgotten/mentioned more than once.
- #[allow(unreachable_code, clippy::diverging_sub_expression)]
- if false {
- $crate::try_pin_init!(make_initializer:
- @slot(slot),
- @type_name($t),
- @munch_fields($($fields)*,),
- @acc(),
- );
- }
- // Forget all guards, since initialization was a success.
- $crate::try_pin_init!(forget_guards:
- @munch_fields($($fields)*,),
- );
- }
- Ok(__InitOk)
- }
- );
- let init = move |slot| -> ::core::result::Result<(), $err> {
- init(slot).map(|__InitOk| ())
- };
- let init = unsafe { $crate::init::pin_init_from_closure::<_, $err>(init) };
- init
- }};
- (init_slot:
- @data($data:ident),
- @slot($slot:ident),
- @munch_fields($(,)?),
- ) => {
- // Endpoint of munching, no fields are left.
- };
- (init_slot:
- @data($data:ident),
- @slot($slot:ident),
- // In-place initialization syntax.
- @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
- ) => {
- let $field = $val;
- // Call the initializer.
- //
- // SAFETY: `slot` is valid, because we are inside of an initializer closure, we
- // 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.
- //
- // We only give access to `&DropGuard`, so it cannot be forgotten via safe code.
- //
- // SAFETY: We forget the guard later when initialization has succeeded.
- let $field = &unsafe {
- $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
- };
-
- $crate::try_pin_init!(init_slot:
- @data($data),
- @slot($slot),
- @munch_fields($($rest)*),
- );
- };
- (init_slot:
- @data($data:ident),
- @slot($slot:ident),
- // Direct value init, this is safe for every field.
- @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
- ) => {
- $(let $field = $val;)?
- // Initialize the field.
- //
- // SAFETY: The memory at `slot` is uninitialized.
- 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.
- //
- // SAFETY: We forget the guard later when initialization has succeeded.
- let $field = &unsafe {
- $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
- };
-
- $crate::try_pin_init!(init_slot:
- @data($data),
- @slot($slot),
- @munch_fields($($rest)*),
- );
- };
- (make_initializer:
- @slot($slot:ident),
- @type_name($t:ident),
- @munch_fields($(,)?),
- @acc($($acc:tt)*),
- ) => {
- // Endpoint, nothing more to munch, create the initializer.
- // Since we are in the `if false` branch, this will never get executed. We abuse `slot` to
- // get the correct type inference here:
- unsafe {
- ::core::ptr::write($slot, $t {
- $($acc)*
- });
- }
- };
- (make_initializer:
- @slot($slot:ident),
- @type_name($t:ident),
- @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
- @acc($($acc:tt)*),
- ) => {
- $crate::try_pin_init!(make_initializer:
- @slot($slot),
- @type_name($t),
- @munch_fields($($rest)*),
- @acc($($acc)* $field: ::core::panic!(),),
- );
- };
- (make_initializer:
- @slot($slot:ident),
- @type_name($t:ident),
- @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
- @acc($($acc:tt)*),
- ) => {
- $crate::try_pin_init!(make_initializer:
- @slot($slot),
- @type_name($t),
- @munch_fields($($rest)*),
- @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::try_pin_init!(forget_guards:
- @munch_fields($($rest)*),
- );
- };
- (forget_guards:
- @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
- ) => {
- unsafe { $crate::init::__internal::DropGuard::forget($field) };
-
- $crate::try_pin_init!(forget_guards:
- @munch_fields($($rest)*),
- );
- };
}
/// Construct an in-place initializer for `struct`s.
@@ -824,11 +651,14 @@ macro_rules! init {
($(&$this:ident in)? $t:ident $(::<$($generics:ty),* $(,)?>)? {
$($fields:tt)*
}) => {
- $crate::try_init!(
+ $crate::__init_internal!(
@this($($this)?),
@typ($t $(::<$($generics),*>)?),
@fields($($fields)*),
@error(::core::convert::Infallible),
+ @data(InitData, /*no use_data*/),
+ @has_data(HasInitData, __init_data),
+ @construct_closure(init_from_closure),
)
}
}
@@ -871,199 +701,29 @@ macro_rules! try_init {
($(&$this:ident in)? $t:ident $(::<$($generics:ty),* $(,)?>)? {
$($fields:tt)*
}) => {
- $crate::try_init!(
+ $crate::__init_internal!(
@this($($this)?),
@typ($t $(::<$($generics),*>)?),
@fields($($fields)*),
@error($crate::error::Error),
+ @data(InitData, /*no use_data*/),
+ @has_data(HasInitData, __init_data),
+ @construct_closure(init_from_closure),
)
};
($(&$this:ident in)? $t:ident $(::<$($generics:ty),* $(,)?>)? {
$($fields:tt)*
}? $err:ty) => {
- $crate::try_init!(
+ $crate::__init_internal!(
@this($($this)?),
@typ($t $(::<$($generics),*>)?),
@fields($($fields)*),
@error($err),
+ @data(InitData, /*no use_data*/),
+ @has_data(HasInitData, __init_data),
+ @construct_closure(init_from_closure),
)
};
- (
- @this($($this:ident)?),
- @typ($t:ident $(::<$($generics:ty),*>)?),
- @fields($($fields:tt)*),
- @error($err:ty),
- ) => {{
- // 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
- // no possibility of returning without `unsafe`.
- struct __InitOk;
- // Get the init data from the supplied type.
- let data = unsafe {
- use $crate::init::__internal::HasInitData;
- $t$(::<$($generics),*>)?::__init_data()
- };
- // Ensure that `data` really is of type `InitData` and help with type inference:
- let init = $crate::init::__internal::InitData::make_closure::<_, __InitOk, $err>(
- data,
- move |slot| {
- {
- // Shadow the structure so it cannot be used to return early.
- struct __InitOk;
- // 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) };)?
- // Initialize every field.
- $crate::try_init!(init_slot:
- @slot(slot),
- @munch_fields($($fields)*,),
- );
- // We use unreachable code to ensure that all fields have been mentioned exactly
- // once, this struct initializer will still be type-checked and complain with a
- // very natural error message if a field is forgotten/mentioned more than once.
- #[allow(unreachable_code, clippy::diverging_sub_expression)]
- if false {
- $crate::try_init!(make_initializer:
- @slot(slot),
- @type_name($t),
- @munch_fields($($fields)*,),
- @acc(),
- );
- }
- // Forget all guards, since initialization was a success.
- $crate::try_init!(forget_guards:
- @munch_fields($($fields)*,),
- );
- }
- Ok(__InitOk)
- }
- );
- let init = move |slot| -> ::core::result::Result<(), $err> {
- init(slot).map(|__InitOk| ())
- };
- let init = unsafe { $crate::init::init_from_closure::<_, $err>(init) };
- init
- }};
- (init_slot:
- @slot($slot:ident),
- @munch_fields( $(,)?),
- ) => {
- // Endpoint of munching, no fields are left.
- };
- (init_slot:
- @slot($slot:ident),
- @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
- ) => {
- let $field = $val;
- // Call the initializer.
- //
- // 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.
- //
- // We only give access to `&DropGuard`, so it cannot be accidentally forgotten.
- //
- // SAFETY: We forget the guard later when initialization has succeeded.
- let $field = &unsafe {
- $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
- };
-
- $crate::try_init!(init_slot:
- @slot($slot),
- @munch_fields($($rest)*),
- );
- };
- (init_slot:
- @slot($slot:ident),
- // Direct value init.
- @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
- ) => {
- $(let $field = $val;)?
- // Call the initializer.
- //
- // SAFETY: The memory at `slot` is uninitialized.
- 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.
- //
- // SAFETY: We forget the guard later when initialization has succeeded.
- let $field = &unsafe {
- $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
- };
-
- $crate::try_init!(init_slot:
- @slot($slot),
- @munch_fields($($rest)*),
- );
- };
- (make_initializer:
- @slot($slot:ident),
- @type_name($t:ident),
- @munch_fields( $(,)?),
- @acc($($acc:tt)*),
- ) => {
- // Endpoint, nothing more to munch, create the initializer.
- // Since we are in the `if false` branch, this will never get executed. We abuse `slot` to
- // get the correct type inference here:
- unsafe {
- ::core::ptr::write($slot, $t {
- $($acc)*
- });
- }
- };
- (make_initializer:
- @slot($slot:ident),
- @type_name($t:ident),
- @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
- @acc($($acc:tt)*),
- ) => {
- $crate::try_init!(make_initializer:
- @slot($slot),
- @type_name($t),
- @munch_fields($($rest)*),
- @acc($($acc)*$field: ::core::panic!(),),
- );
- };
- (make_initializer:
- @slot($slot:ident),
- @type_name($t:ident),
- @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
- @acc($($acc:tt)*),
- ) => {
- $crate::try_init!(make_initializer:
- @slot($slot),
- @type_name($t),
- @munch_fields($($rest)*),
- @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::try_init!(forget_guards:
- @munch_fields($($rest)*),
- );
- };
- (forget_guards:
- @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
- ) => {
- unsafe { $crate::init::__internal::DropGuard::forget($field) };
-
- $crate::try_init!(forget_guards:
- @munch_fields($($rest)*),
- );
- };
}
/// A pin-initializer for the type `T`.
diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
index 00aa4e956c0a..fbaebd34f218 100644
--- a/rust/kernel/init/macros.rs
+++ b/rust/kernel/init/macros.rs
@@ -1,10 +1,12 @@
// SPDX-License-Identifier: Apache-2.0 OR MIT
//! This module provides the macros that actually implement the proc-macros `pin_data` and
-//! `pinned_drop`.
+//! `pinned_drop`. It also contains `__init_internal` the implementation of the `{try_}{pin_}init!`
+//! macros.
//!
//! These macros should never be called directly, since they expect their input to be
-//! in a certain format which is internal. Use the proc-macros instead.
+//! in a certain format which is internal. If used incorrectly, these macros can lead to UB even in
+//! safe code! Use the public facing macros instead.
//!
//! This architecture has been chosen because the kernel does not yet have access to `syn` which
//! would make matters a lot easier for implementing these as proc-macros.
@@ -980,3 +982,234 @@ impl<$($impl_generics)*> $pin_data<$($ty_generics)*>
}
};
}
+
+/// The internal init macro. Do not call manually!
+///
+/// This is called by the `{try_}{pin_}init!` macros with various inputs.
+///
+/// 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.
+/// - `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 {
+ (
+ @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),
+ ) => {{
+ // 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
+ // no possibility of returning without `unsafe`.
+ struct __InitOk;
+ // Get the data about fields from the supplied type.
+ let data = unsafe {
+ use $crate::init::__internal::$has_data;
+ $t$(::<$($generics),*>)?::$get_data()
+ };
+ // Ensure that `data` really is of type `$data` and help with type inference:
+ let init = $crate::init::__internal::$data::make_closure::<_, __InitOk, $err>(
+ data,
+ move |slot| {
+ {
+ // Shadow the structure so it cannot be used to return early.
+ struct __InitOk;
+ // 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) };)?
+ // Initialize every field.
+ $crate::__init_internal!(init_slot($($use_data)?):
+ @data(data),
+ @slot(slot),
+ @munch_fields($($fields)*,),
+ );
+ // We use unreachable code to ensure that all fields have been mentioned exactly
+ // once, this struct initializer will still be type-checked and complain with a
+ // very natural error message if a field is forgotten/mentioned more than once.
+ #[allow(unreachable_code, clippy::diverging_sub_expression)]
+ if false {
+ $crate::__init_internal!(make_initializer:
+ @slot(slot),
+ @type_name($t),
+ @munch_fields($($fields)*,),
+ @acc(),
+ );
+ }
+ // Forget all guards, since initialization was a success.
+ $crate::__init_internal!(forget_guards:
+ @munch_fields($($fields)*,),
+ );
+ }
+ Ok(__InitOk)
+ }
+ );
+ let init = move |slot| -> ::core::result::Result<(), $err> {
+ init(slot).map(|__InitOk| ())
+ };
+ let init = unsafe { $crate::init::$construct_closure::<_, $err>(init) };
+ init
+ }};
+ (init_slot($($use_data:ident)?):
+ @data($data:ident),
+ @slot($slot:ident),
+ @munch_fields($(,)?),
+ ) => {
+ // Endpoint of munching, no fields are left.
+ };
+ (init_slot($use_data:ident): // use_data is present, so we use the `data` to init fields.
+ @data($data:ident),
+ @slot($slot:ident),
+ // In-place initialization syntax.
+ @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
+ ) => {
+ let $field = $val;
+ // Call the initializer.
+ //
+ // SAFETY: `slot` is valid, because we are inside of an initializer closure, we
+ // 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.
+ //
+ // We only give access to `&DropGuard`, so it cannot be forgotten via safe code.
+ //
+ // SAFETY: We forget the guard later when initialization has succeeded.
+ let $field = &unsafe {
+ $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
+ };
+
+ $crate::__init_internal!(init_slot($use_data):
+ @data($data),
+ @slot($slot),
+ @munch_fields($($rest)*),
+ );
+ };
+ (init_slot(): // no use_data, so we use `Init::__init` directly.
+ @data($data:ident),
+ @slot($slot:ident),
+ // In-place initialization syntax.
+ @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
+ ) => {
+ let $field = $val;
+ // Call the initializer.
+ //
+ // 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.
+ //
+ // We only give access to `&DropGuard`, so it cannot be forgotten via safe code.
+ //
+ // SAFETY: We forget the guard later when initialization has succeeded.
+ let $field = &unsafe {
+ $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
+ };
+
+ $crate::__init_internal!(init_slot():
+ @data($data),
+ @slot($slot),
+ @munch_fields($($rest)*),
+ );
+ };
+ (init_slot($($use_data:ident)?):
+ @data($data:ident),
+ @slot($slot:ident),
+ // Init by-value.
+ @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
+ ) => {
+ $(let $field = $val;)?
+ // Initialize the field.
+ //
+ // SAFETY: The memory at `slot` is uninitialized.
+ 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.
+ //
+ // SAFETY: We forget the guard later when initialization has succeeded.
+ let $field = &unsafe {
+ $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
+ };
+
+ $crate::__init_internal!(init_slot($($use_data)?):
+ @data($data),
+ @slot($slot),
+ @munch_fields($($rest)*),
+ );
+ };
+ (make_initializer:
+ @slot($slot:ident),
+ @type_name($t:ident),
+ @munch_fields($(,)?),
+ @acc($($acc:tt)*),
+ ) => {
+ // Endpoint, nothing more to munch, create the initializer.
+ // Since we are in the `if false` branch, this will never get executed. We abuse `slot` to
+ // get the correct type inference here:
+ unsafe {
+ ::core::ptr::write($slot, $t {
+ $($acc)*
+ });
+ }
+ };
+ (make_initializer:
+ @slot($slot:ident),
+ @type_name($t:ident),
+ @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
+ @acc($($acc:tt)*),
+ ) => {
+ $crate::__init_internal!(make_initializer:
+ @slot($slot),
+ @type_name($t),
+ @munch_fields($($rest)*),
+ @acc($($acc)* $field: ::core::panic!(),),
+ );
+ };
+ (make_initializer:
+ @slot($slot:ident),
+ @type_name($t:ident),
+ @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
+ @acc($($acc:tt)*),
+ ) => {
+ $crate::__init_internal!(make_initializer:
+ @slot($slot),
+ @type_name($t),
+ @munch_fields($($rest)*),
+ @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)*),
+ );
+ };
+}
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 01/12] rust: init: consolidate init macros
2023-07-19 14:20 ` [PATCH v2 01/12] rust: init: consolidate init macros Benno Lossin
@ 2023-07-19 18:37 ` Martin Rodriguez Reboredo
2023-07-20 13:09 ` Alice Ryhl
1 sibling, 0 replies; 43+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-07-19 18:37 UTC (permalink / raw)
To: Benno Lossin, Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel
On 7/19/23 11:20, Benno Lossin wrote:
> Merges the implementations of `try_init!` and `try_pin_init!`. These two
> macros are very similar, but use different traits. The new macro
> `__init_internal!` that is now the implementation for both takes these
> traits as parameters.
>
> This change does not affect any users, as no public API has been
> changed, but it should simplify maintaining the init macros.
>
> Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 01/12] rust: init: consolidate init macros
2023-07-19 14:20 ` [PATCH v2 01/12] rust: init: consolidate init macros Benno Lossin
2023-07-19 18:37 ` Martin Rodriguez Reboredo
@ 2023-07-20 13:09 ` Alice Ryhl
1 sibling, 0 replies; 43+ messages in thread
From: Alice Ryhl @ 2023-07-20 13:09 UTC (permalink / raw)
To: benno.lossin
Cc: alex.gaynor, aliceryhl, bjorn3_gh, boqun.feng, gary,
linux-kernel, nmi, ojeda, rust-for-linux, wedsonaf
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="yes", Size: 596 bytes --]
Benno Lossin <benno.lossin@proton.me> writes:
> Merges the implementations of `try_init!` and `try_pin_init!`. These two
> macros are very similar, but use different traits. The new macro
> `__init_internal!` that is now the implementation for both takes these
> traits as parameters.
>
> This change does not affect any users, as no public API has been
> changed, but it should simplify maintaining the init macros.
>
> Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 02/12] rust: add derive macro for `Zeroable`
2023-07-19 14:20 [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
2023-07-19 14:20 ` [PATCH v2 01/12] rust: init: consolidate init macros Benno Lossin
@ 2023-07-19 14:20 ` Benno Lossin
2023-07-19 18:42 ` Martin Rodriguez Reboredo
2023-07-20 13:13 ` Alice Ryhl
2023-07-19 14:20 ` [PATCH v2 03/12] rust: init: make guards in the init macros hygienic Benno Lossin
` (10 subsequent siblings)
12 siblings, 2 replies; 43+ messages in thread
From: Benno Lossin @ 2023-07-19 14:20 UTC (permalink / raw)
To: Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, Andreas Hindborg, rust-for-linux, linux-kernel,
Asahi Lina
Add a derive proc-macro for the `Zeroable` trait. The macro supports
structs where every field implements the `Zeroable` trait. This way
`unsafe` implementations can be avoided.
The macro is split into two parts:
- a proc-macro to parse generics into impl and ty generics,
- a declarative macro that expands to the impl block.
Suggested-by: Asahi Lina <lina@asahilina.net>
Reviewed-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
v1 -> v2:
* fix Zeroable path
* add Reviewed-by from Gary and Björn
rust/kernel/init/macros.rs | 28 ++++++++++++++++++++++++++++
rust/kernel/prelude.rs | 2 +-
rust/macros/lib.rs | 20 ++++++++++++++++++++
rust/macros/quote.rs | 6 ++++++
rust/macros/zeroable.rs | 25 +++++++++++++++++++++++++
5 files changed, 80 insertions(+), 1 deletion(-)
create mode 100644 rust/macros/zeroable.rs
diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
index fbaebd34f218..c50429173fc7 100644
--- a/rust/kernel/init/macros.rs
+++ b/rust/kernel/init/macros.rs
@@ -1213,3 +1213,31 @@ macro_rules! __init_internal {
);
};
}
+
+#[doc(hidden)]
+#[macro_export]
+macro_rules! __derive_zeroable {
+ (parse_input:
+ @sig(
+ $(#[$($struct_attr:tt)*])*
+ $vis:vis struct $name:ident
+ $(where $($whr:tt)*)?
+ ),
+ @impl_generics($($impl_generics:tt)*),
+ @ty_generics($($ty_generics:tt)*),
+ @body({
+ $(
+ $(#[$($field_attr:tt)*])*
+ $field:ident : $field_ty:ty
+ ),* $(,)?
+ }),
+ ) => {
+ // SAFETY: every field type implements `Zeroable` and padding bytes may be zero.
+ #[automatically_derived]
+ unsafe impl<$($impl_generics)*> $crate::init::Zeroable for $name<$($ty_generics)*>
+ where
+ $($field_ty: $crate::init::Zeroable,)*
+ $($($whr)*)?
+ {}
+ };
+}
diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index c28587d68ebc..ae21600970b3 100644
--- a/rust/kernel/prelude.rs
+++ b/rust/kernel/prelude.rs
@@ -18,7 +18,7 @@
pub use alloc::{boxed::Box, vec::Vec};
#[doc(no_inline)]
-pub use macros::{module, pin_data, pinned_drop, vtable};
+pub use macros::{module, pin_data, pinned_drop, vtable, Zeroable};
pub use super::build_assert;
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index b4bc44c27bd4..fd7a815e68a8 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -11,6 +11,7 @@
mod pin_data;
mod pinned_drop;
mod vtable;
+mod zeroable;
use proc_macro::TokenStream;
@@ -343,3 +344,22 @@ pub fn paste(input: TokenStream) -> TokenStream {
paste::expand(&mut tokens);
tokens.into_iter().collect()
}
+
+/// Derives the [`Zeroable`] trait for the given struct.
+///
+/// This can only be used for structs where every field implements the [`Zeroable`] trait.
+///
+/// # Examples
+///
+/// ```rust
+/// #[derive(Zeroable)]
+/// pub struct DriverData {
+/// id: i64,
+/// buf_ptr: *mut u8,
+/// len: usize,
+/// }
+/// ```
+#[proc_macro_derive(Zeroable)]
+pub fn derive_zeroable(input: TokenStream) -> TokenStream {
+ zeroable::derive(input)
+}
diff --git a/rust/macros/quote.rs b/rust/macros/quote.rs
index dddbb4e6f4cb..b76c198a4ed5 100644
--- a/rust/macros/quote.rs
+++ b/rust/macros/quote.rs
@@ -124,6 +124,12 @@ macro_rules! quote_spanned {
));
quote_spanned!(@proc $v $span $($tt)*);
};
+ (@proc $v:ident $span:ident ; $($tt:tt)*) => {
+ $v.push(::proc_macro::TokenTree::Punct(
+ ::proc_macro::Punct::new(';', ::proc_macro::Spacing::Alone)
+ ));
+ quote_spanned!(@proc $v $span $($tt)*);
+ };
(@proc $v:ident $span:ident $id:ident $($tt:tt)*) => {
$v.push(::proc_macro::TokenTree::Ident(::proc_macro::Ident::new(stringify!($id), $span)));
quote_spanned!(@proc $v $span $($tt)*);
diff --git a/rust/macros/zeroable.rs b/rust/macros/zeroable.rs
new file mode 100644
index 000000000000..cddb866c44ef
--- /dev/null
+++ b/rust/macros/zeroable.rs
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use crate::helpers::{parse_generics, Generics};
+use proc_macro::TokenStream;
+
+pub(crate) fn derive(input: TokenStream) -> TokenStream {
+ let (
+ Generics {
+ impl_generics,
+ ty_generics,
+ },
+ mut rest,
+ ) = parse_generics(input);
+ // This should be the body of the struct `{...}`.
+ let last = rest.pop();
+ quote! {
+ ::kernel::__derive_zeroable!(
+ parse_input:
+ @sig(#(#rest)*),
+ @impl_generics(#(#impl_generics)*),
+ @ty_generics(#(#ty_generics)*),
+ @body(#last),
+ );
+ }
+}
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 02/12] rust: add derive macro for `Zeroable`
2023-07-19 14:20 ` [PATCH v2 02/12] rust: add derive macro for `Zeroable` Benno Lossin
@ 2023-07-19 18:42 ` Martin Rodriguez Reboredo
2023-07-20 13:13 ` Alice Ryhl
1 sibling, 0 replies; 43+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-07-19 18:42 UTC (permalink / raw)
To: Benno Lossin, Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel, Asahi Lina
On 7/19/23 11:20, Benno Lossin wrote:
> Add a derive proc-macro for the `Zeroable` trait. The macro supports
> structs where every field implements the `Zeroable` trait. This way
> `unsafe` implementations can be avoided.
>
> The macro is split into two parts:
> - a proc-macro to parse generics into impl and ty generics,
> - a declarative macro that expands to the impl block.
>
> Suggested-by: Asahi Lina <lina@asahilina.net>
> Reviewed-by: Gary Guo <gary@garyguo.net>
> Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 02/12] rust: add derive macro for `Zeroable`
2023-07-19 14:20 ` [PATCH v2 02/12] rust: add derive macro for `Zeroable` Benno Lossin
2023-07-19 18:42 ` Martin Rodriguez Reboredo
@ 2023-07-20 13:13 ` Alice Ryhl
1 sibling, 0 replies; 43+ messages in thread
From: Alice Ryhl @ 2023-07-20 13:13 UTC (permalink / raw)
To: benno.lossin
Cc: alex.gaynor, aliceryhl, bjorn3_gh, boqun.feng, gary, lina,
linux-kernel, nmi, ojeda, rust-for-linux, wedsonaf
Benno Lossin <benno.lossin@proton.me> writes:
> Add a derive proc-macro for the `Zeroable` trait. The macro supports
> structs where every field implements the `Zeroable` trait. This way
> `unsafe` implementations can be avoided.
>
> The macro is split into two parts:
> - a proc-macro to parse generics into impl and ty generics,
> - a declarative macro that expands to the impl block.
>
> Suggested-by: Asahi Lina <lina@asahilina.net>
> Reviewed-by: Gary Guo <gary@garyguo.net>
> Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 03/12] rust: init: make guards in the init macros hygienic
2023-07-19 14:20 [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
2023-07-19 14:20 ` [PATCH v2 01/12] rust: init: consolidate init macros Benno Lossin
2023-07-19 14:20 ` [PATCH v2 02/12] rust: add derive macro for `Zeroable` Benno Lossin
@ 2023-07-19 14:20 ` Benno Lossin
2023-07-19 19:04 ` Martin Rodriguez Reboredo
2023-07-20 13:16 ` Alice Ryhl
2023-07-19 14:20 ` [PATCH v2 04/12] rust: init: wrap type checking struct initializers in a closure Benno Lossin
` (9 subsequent siblings)
12 siblings, 2 replies; 43+ messages in thread
From: Benno Lossin @ 2023-07-19 14:20 UTC (permalink / raw)
To: Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, Andreas Hindborg, rust-for-linux, linux-kernel,
Asahi Lina
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>
---
v1 -> v2:
* use Gary's `paste!` macro to create the guard hygiene
rust/kernel/init.rs | 1 -
rust/kernel/init/__internal.rs | 25 ++-----
rust/kernel/init/macros.rs | 116 +++++++++++++++------------------
3 files changed, 56 insertions(+), 86 deletions(-)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index d431d0b153a2..0120674b451e 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -216,7 +216,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 c50429173fc7..c5f977f52d0c 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,28 @@ 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.
- //
- // We only give access to `&DropGuard`, so it cannot be forgotten via safe code.
+ // Create the drop guard:
//
- // SAFETY: We forget the guard later when initialization has succeeded.
- let $field = &unsafe {
- $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
- };
+ // We rely on macro hygiene to make it impossible for users to access this local variable.
+ // 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 {
+ $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
+ };
- $crate::__init_internal!(init_slot($use_data):
- @data($data),
- @slot($slot),
- @munch_fields($($rest)*),
- );
+ $crate::__init_internal!(init_slot($use_data):
+ @data($data),
+ @slot($slot),
+ @guards([<$field>], $($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 +1111,28 @@ 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.
- //
- // We only give access to `&DropGuard`, so it cannot be forgotten via safe code.
+ // Create the drop guard:
//
- // SAFETY: We forget the guard later when initialization has succeeded.
- let $field = &unsafe {
- $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
- };
+ // We rely on macro hygiene to make it impossible for users to access this local variable.
+ // 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 {
+ $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
+ };
- $crate::__init_internal!(init_slot():
- @data($data),
- @slot($slot),
- @munch_fields($($rest)*),
- );
+ $crate::__init_internal!(init_slot():
+ @data($data),
+ @slot($slot),
+ @guards([<$field>], $($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,18 +1143,21 @@ 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.
- //
- // SAFETY: We forget the guard later when initialization has succeeded.
- let $field = &unsafe {
- $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
- };
+ // We rely on macro hygiene to make it impossible for users to access this local variable.
+ // 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 {
+ $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
+ };
- $crate::__init_internal!(init_slot($($use_data)?):
- @data($data),
- @slot($slot),
- @munch_fields($($rest)*),
- );
+ $crate::__init_internal!(init_slot($($use_data)?):
+ @data($data),
+ @slot($slot),
+ @guards([<$field>], $($guards,)*),
+ @munch_fields($($rest)*),
+ );
+ }
};
(make_initializer:
@slot($slot:ident),
@@ -1189,29 +1200,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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 03/12] rust: init: make guards in the init macros hygienic
2023-07-19 14:20 ` [PATCH v2 03/12] rust: init: make guards in the init macros hygienic Benno Lossin
@ 2023-07-19 19:04 ` Martin Rodriguez Reboredo
2023-07-20 13:16 ` Alice Ryhl
1 sibling, 0 replies; 43+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-07-19 19:04 UTC (permalink / raw)
To: Benno Lossin, Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel, Asahi Lina
On 7/19/23 11:20, Benno Lossin 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: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 03/12] rust: init: make guards in the init macros hygienic
2023-07-19 14:20 ` [PATCH v2 03/12] rust: init: make guards in the init macros hygienic Benno Lossin
2023-07-19 19:04 ` Martin Rodriguez Reboredo
@ 2023-07-20 13:16 ` Alice Ryhl
1 sibling, 0 replies; 43+ messages in thread
From: Alice Ryhl @ 2023-07-20 13:16 UTC (permalink / raw)
To: benno.lossin
Cc: alex.gaynor, aliceryhl, bjorn3_gh, boqun.feng, gary, lina,
linux-kernel, nmi, ojeda, rust-for-linux, wedsonaf
Benno Lossin <benno.lossin@proton.me> writes:
> 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: Alice Ryhl <aliceryhl@google.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 04/12] rust: init: wrap type checking struct initializers in a closure
2023-07-19 14:20 [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
` (2 preceding siblings ...)
2023-07-19 14:20 ` [PATCH v2 03/12] rust: init: make guards in the init macros hygienic Benno Lossin
@ 2023-07-19 14:20 ` Benno Lossin
2023-07-19 19:05 ` Martin Rodriguez Reboredo
2023-07-20 13:17 ` Alice Ryhl
2023-07-19 14:20 ` [PATCH v2 05/12] rust: init: make initializer values inaccessible after initializing Benno Lossin
` (8 subsequent siblings)
12 siblings, 2 replies; 43+ messages in thread
From: Benno Lossin @ 2023-07-19 14:20 UTC (permalink / raw)
To: Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, Andreas Hindborg, rust-for-linux, linux-kernel
In the implementation of the init macros there is a `if false` statement
that type checks the initializer to ensure every field is initialized.
Since the next patch has a stack variable to store the struct, the
function might allocate too much memory on debug builds. Putting the
struct into a closure that is never executed ensures that even in debug
builds no stack overflow error is caused. In release builds this was not
a problem since the code was optimized away due to the `if false`.
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
v1 -> v2:
* do not call the created closure
rust/kernel/init/macros.rs | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
index c5f977f52d0c..160b95fc03c9 100644
--- a/rust/kernel/init/macros.rs
+++ b/rust/kernel/init/macros.rs
@@ -1038,14 +1038,14 @@ macro_rules! __init_internal {
// once, this struct initializer will still be type-checked and complain with a
// very natural error message if a field is forgotten/mentioned more than once.
#[allow(unreachable_code, clippy::diverging_sub_expression)]
- if false {
+ let _ = || {
$crate::__init_internal!(make_initializer:
@slot(slot),
@type_name($t),
@munch_fields($($fields)*,),
@acc(),
);
- }
+ };
}
Ok(__InitOk)
}
@@ -1166,8 +1166,8 @@ macro_rules! __init_internal {
@acc($($acc:tt)*),
) => {
// Endpoint, nothing more to munch, create the initializer.
- // Since we are in the `if false` branch, this will never get executed. We abuse `slot` to
- // get the correct type inference here:
+ // Since we are in the closure that is never called, this will never get executed.
+ // We abuse `slot` to get the correct type inference here:
unsafe {
::core::ptr::write($slot, $t {
$($acc)*
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 04/12] rust: init: wrap type checking struct initializers in a closure
2023-07-19 14:20 ` [PATCH v2 04/12] rust: init: wrap type checking struct initializers in a closure Benno Lossin
@ 2023-07-19 19:05 ` Martin Rodriguez Reboredo
2023-07-20 13:17 ` Alice Ryhl
1 sibling, 0 replies; 43+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-07-19 19:05 UTC (permalink / raw)
To: Benno Lossin, Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel
On 7/19/23 11:20, Benno Lossin wrote:
> In the implementation of the init macros there is a `if false` statement
> that type checks the initializer to ensure every field is initialized.
> Since the next patch has a stack variable to store the struct, the
> function might allocate too much memory on debug builds. Putting the
> struct into a closure that is never executed ensures that even in debug
> builds no stack overflow error is caused. In release builds this was not
> a problem since the code was optimized away due to the `if false`.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 04/12] rust: init: wrap type checking struct initializers in a closure
2023-07-19 14:20 ` [PATCH v2 04/12] rust: init: wrap type checking struct initializers in a closure Benno Lossin
2023-07-19 19:05 ` Martin Rodriguez Reboredo
@ 2023-07-20 13:17 ` Alice Ryhl
1 sibling, 0 replies; 43+ messages in thread
From: Alice Ryhl @ 2023-07-20 13:17 UTC (permalink / raw)
To: benno.lossin
Cc: alex.gaynor, aliceryhl, bjorn3_gh, boqun.feng, gary,
linux-kernel, nmi, ojeda, rust-for-linux, wedsonaf
> In the implementation of the init macros there is a `if false` statement
> that type checks the initializer to ensure every field is initialized.
> Since the next patch has a stack variable to store the struct, the
> function might allocate too much memory on debug builds. Putting the
> struct into a closure that is never executed ensures that even in debug
> builds no stack overflow error is caused. In release builds this was not
> a problem since the code was optimized away due to the `if false`.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 05/12] rust: init: make initializer values inaccessible after initializing
2023-07-19 14:20 [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
` (3 preceding siblings ...)
2023-07-19 14:20 ` [PATCH v2 04/12] rust: init: wrap type checking struct initializers in a closure Benno Lossin
@ 2023-07-19 14:20 ` Benno Lossin
2023-07-19 19:07 ` Martin Rodriguez Reboredo
2023-07-20 13:19 ` Alice Ryhl
2023-07-19 14:20 ` [PATCH v2 06/12] rust: init: add `..Zeroable::zeroed()` syntax for zeroing all missing fields Benno Lossin
` (7 subsequent siblings)
12 siblings, 2 replies; 43+ messages in thread
From: Benno Lossin @ 2023-07-19 14:20 UTC (permalink / raw)
To: Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, Andreas Hindborg, rust-for-linux, linux-kernel
Previously the init macros would create a local variable with the name
and hygiene of the field that is being initialized to store the value of
the field. This would override any user defined variables. For example:
```
struct Foo {
a: usize,
b: usize,
}
let a = 10;
let foo = init!(Foo{
a: a + 1, // This creates a local variable named `a`.
b: a, // This refers to that variable!
});
let foo = Box::init!(foo)?;
assert_eq!(foo.a, 11);
assert_eq!(foo.b, 11);
```
This patch changes this behavior, so the above code would panic at the
last assertion, since `b` would have value 10.
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
rust/kernel/init/macros.rs | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
index 160b95fc03c9..5de939e0801f 100644
--- a/rust/kernel/init/macros.rs
+++ b/rust/kernel/init/macros.rs
@@ -1073,13 +1073,13 @@ macro_rules! __init_internal {
// In-place initialization syntax.
@munch_fields($field:ident <- $val:expr, $($rest:tt)*),
) => {
- let $field = $val;
+ let init = $val;
// Call the initializer.
//
// SAFETY: `slot` is valid, because we are inside of an initializer closure, we
// 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)? };
+ unsafe { $data.$field(::core::ptr::addr_of_mut!((*$slot).$field), init)? };
// Create the drop guard:
//
// We rely on macro hygiene to make it impossible for users to access this local variable.
@@ -1105,12 +1105,12 @@ macro_rules! __init_internal {
// In-place initialization syntax.
@munch_fields($field:ident <- $val:expr, $($rest:tt)*),
) => {
- let $field = $val;
+ let init = $val;
// Call the initializer.
//
// 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))? };
+ unsafe { $crate::init::Init::__init(init, ::core::ptr::addr_of_mut!((*$slot).$field))? };
// Create the drop guard:
//
// We rely on macro hygiene to make it impossible for users to access this local variable.
@@ -1136,11 +1136,13 @@ macro_rules! __init_internal {
// Init by-value.
@munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
) => {
- $(let $field = $val;)?
- // Initialize the field.
- //
- // SAFETY: The memory at `slot` is uninitialized.
- unsafe { ::core::ptr::write(::core::ptr::addr_of_mut!((*$slot).$field), $field) };
+ {
+ $(let $field = $val;)?
+ // Initialize the field.
+ //
+ // SAFETY: The memory at `slot` is uninitialized.
+ unsafe { ::core::ptr::write(::core::ptr::addr_of_mut!((*$slot).$field), $field) };
+ }
// Create the drop guard:
//
// We rely on macro hygiene to make it impossible for users to access this local variable.
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 05/12] rust: init: make initializer values inaccessible after initializing
2023-07-19 14:20 ` [PATCH v2 05/12] rust: init: make initializer values inaccessible after initializing Benno Lossin
@ 2023-07-19 19:07 ` Martin Rodriguez Reboredo
2023-07-20 13:19 ` Alice Ryhl
1 sibling, 0 replies; 43+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-07-19 19:07 UTC (permalink / raw)
To: Benno Lossin, Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel
On 7/19/23 11:20, Benno Lossin wrote:
> Previously the init macros would create a local variable with the name
> and hygiene of the field that is being initialized to store the value of
> the field. This would override any user defined variables. For example:
> ```
> struct Foo {
> a: usize,
> b: usize,
> }
> let a = 10;
> let foo = init!(Foo{
> a: a + 1, // This creates a local variable named `a`.
> b: a, // This refers to that variable!
> });
> let foo = Box::init!(foo)?;
> assert_eq!(foo.a, 11);
> assert_eq!(foo.b, 11);
> ```
>
> This patch changes this behavior, so the above code would panic at the
> last assertion, since `b` would have value 10.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 05/12] rust: init: make initializer values inaccessible after initializing
2023-07-19 14:20 ` [PATCH v2 05/12] rust: init: make initializer values inaccessible after initializing Benno Lossin
2023-07-19 19:07 ` Martin Rodriguez Reboredo
@ 2023-07-20 13:19 ` Alice Ryhl
1 sibling, 0 replies; 43+ messages in thread
From: Alice Ryhl @ 2023-07-20 13:19 UTC (permalink / raw)
To: benno.lossin
Cc: alex.gaynor, aliceryhl, bjorn3_gh, boqun.feng, gary,
linux-kernel, nmi, ojeda, rust-for-linux, wedsonaf
> Previously the init macros would create a local variable with the name
> and hygiene of the field that is being initialized to store the value of
> the field. This would override any user defined variables. For example:
> ```
> struct Foo {
> a: usize,
> b: usize,
> }
> let a = 10;
> let foo = init!(Foo{
> a: a + 1, // This creates a local variable named `a`.
> b: a, // This refers to that variable!
> });
> let foo = Box::init!(foo)?;
> assert_eq!(foo.a, 11);
> assert_eq!(foo.b, 11);
> ```
>
> This patch changes this behavior, so the above code would panic at the
> last assertion, since `b` would have value 10.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 06/12] rust: init: add `..Zeroable::zeroed()` syntax for zeroing all missing fields
2023-07-19 14:20 [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
` (4 preceding siblings ...)
2023-07-19 14:20 ` [PATCH v2 05/12] rust: init: make initializer values inaccessible after initializing Benno Lossin
@ 2023-07-19 14:20 ` Benno Lossin
2023-07-20 13:25 ` Alice Ryhl
2023-07-20 13:51 ` Martin Rodriguez Reboredo
2023-07-19 14:20 ` [PATCH v2 07/12] rust: init: Add functions to create array initializers Benno Lossin
` (6 subsequent siblings)
12 siblings, 2 replies; 43+ messages in thread
From: Benno Lossin @ 2023-07-19 14:20 UTC (permalink / raw)
To: Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, Andreas Hindborg, rust-for-linux, linux-kernel,
Asahi Lina
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@asahilina.net>
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
v1 -> v2:
* fix doctest imports
* fix doctest examples
* fix `Zeroable` path in the `__init_internal` macro
* rename `is_zeroable` -> `assert_zeroable`
* add missing `{}` to the case when `..Zeroable::zeroed()` is present
* add `allow(unused_assignments)` in the type-checked struct initializer
rust/kernel/init.rs | 16 +++++-
rust/kernel/init/macros.rs | 115 ++++++++++++++++++++++++++++++++++++-
2 files changed, 129 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 0120674b451e..460f808ebf84 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -517,13 +517,17 @@ 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::{macros::pin_data, pin_init};
+/// # use kernel::{macros::{Zeroable, pin_data}, pin_init};
/// # use core::{ptr::addr_of_mut, marker::PhantomPinned};
/// #[pin_data]
+/// #[derive(Zeroable)]
/// struct Buf {
/// // `ptr` points into `buf`.
/// ptr: *mut u8,
@@ -536,6 +540,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
@@ -555,6 +563,7 @@ macro_rules! pin_init {
@data(PinData, use_data),
@has_data(HasPinData, __pin_data),
@construct_closure(pin_init_from_closure),
+ @munch_fields($($fields)*),
)
};
}
@@ -611,6 +620,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),* $(,)?>)? {
@@ -624,6 +634,7 @@ macro_rules! try_pin_init {
@data(PinData, use_data),
@has_data(HasPinData, __pin_data),
@construct_closure(pin_init_from_closure),
+ @munch_fields($($fields)*),
)
};
}
@@ -658,6 +669,7 @@ macro_rules! init {
@data(InitData, /*no use_data*/),
@has_data(HasInitData, __init_data),
@construct_closure(init_from_closure),
+ @munch_fields($($fields)*),
)
}
}
@@ -708,6 +720,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),* $(,)?>)? {
@@ -721,6 +734,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 5de939e0801f..f5d7f0943f60 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 assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
+ // Ensure that the struct is indeed `Zeroable`.
+ assert_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.
+ })?
// 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) };)?
@@ -1060,7 +1148,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.
@@ -1161,6 +1249,31 @@ macro_rules! __init_internal {
);
}
};
+ (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:
+ #[allow(unused_assignments)]
+ unsafe {
+ let mut zeroed = ::core::mem::zeroed();
+ // We have to use type inference here to make zeroed have the correct type. This does
+ // 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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 06/12] rust: init: add `..Zeroable::zeroed()` syntax for zeroing all missing fields
2023-07-19 14:20 ` [PATCH v2 06/12] rust: init: add `..Zeroable::zeroed()` syntax for zeroing all missing fields Benno Lossin
@ 2023-07-20 13:25 ` Alice Ryhl
2023-07-20 13:51 ` Martin Rodriguez Reboredo
1 sibling, 0 replies; 43+ messages in thread
From: Alice Ryhl @ 2023-07-20 13:25 UTC (permalink / raw)
To: benno.lossin
Cc: alex.gaynor, aliceryhl, bjorn3_gh, boqun.feng, gary, lina,
linux-kernel, nmi, ojeda, rust-for-linux, wedsonaf
Benno Lossin <benno.lossin@proton.me> writes:
> 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@asahilina.net>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> + (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:
Didn't you just change it to a closure rather than an `if else`?
Regardless, I'm happy with this change.
Alice
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 06/12] rust: init: add `..Zeroable::zeroed()` syntax for zeroing all missing fields
2023-07-19 14:20 ` [PATCH v2 06/12] rust: init: add `..Zeroable::zeroed()` syntax for zeroing all missing fields Benno Lossin
2023-07-20 13:25 ` Alice Ryhl
@ 2023-07-20 13:51 ` Martin Rodriguez Reboredo
1 sibling, 0 replies; 43+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-07-20 13:51 UTC (permalink / raw)
To: Benno Lossin, Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel, Asahi Lina
On 7/19/23 11:20, Benno Lossin 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@asahilina.net>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 07/12] rust: init: Add functions to create array initializers
2023-07-19 14:20 [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
` (5 preceding siblings ...)
2023-07-19 14:20 ` [PATCH v2 06/12] rust: init: add `..Zeroable::zeroed()` syntax for zeroing all missing fields Benno Lossin
@ 2023-07-19 14:20 ` Benno Lossin
2023-07-20 13:28 ` Alice Ryhl
2023-07-20 13:59 ` Martin Rodriguez Reboredo
2023-07-19 14:21 ` [PATCH v2 08/12] rust: init: add support for arbitrary paths in init macros Benno Lossin
` (5 subsequent siblings)
12 siblings, 2 replies; 43+ messages in thread
From: Benno Lossin @ 2023-07-19 14:20 UTC (permalink / raw)
To: Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, Andreas Hindborg, rust-for-linux, linux-kernel,
Asahi Lina
Add two functions `pin_init_array_from_fn` and `init_array_from_fn` that
take a function that generates initializers for `T` from usize, the added
functions then return an initializer for `[T; N]` where every element is
initialized by an element returned from the generator function.
Suggested-by: Asahi Lina <lina@asahilina.net>
Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
v1 -> v2:
* fix warnings and errors in doctests
* replace dropping loop with `drop_in_place` and `slice_from_raw_parts_mut`
inside of `{pin_}init_array_from_fn` functions
rust/kernel/init.rs | 86 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 86 insertions(+)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 460f808ebf84..fa1ebdbf5f4b 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -875,6 +875,92 @@ pub fn uninit<T, E>() -> impl Init<MaybeUninit<T>, E> {
unsafe { init_from_closure(|_| Ok(())) }
}
+/// Initializes an array by initializing each element via the provided initializer.
+///
+/// # Examples
+///
+/// ```rust
+/// use kernel::{error::Error, init::init_array_from_fn};
+/// let array: Box<[usize; 1_000_000_000]>= Box::init::<Error>(init_array_from_fn(|i| i)).unwrap();
+/// pr_info!("{array:?}");
+/// ```
+pub fn init_array_from_fn<I, const N: usize, T, E>(
+ mut make_init: impl FnMut(usize) -> I,
+) -> impl Init<[T; N], E>
+where
+ I: Init<T, E>,
+{
+ let init = move |slot: *mut [T; N]| {
+ let slot = slot.cast::<T>();
+ for i in 0..N {
+ let init = make_init(i);
+ // SAFETY: since 0 <= `i` < N, it is still in bounds of `[T; N]`.
+ let ptr = unsafe { slot.add(i) };
+ // SAFETY: The pointer is derived from `slot` and thus satisfies the `__init`
+ // requirements.
+ match unsafe { init.__init(ptr) } {
+ Ok(()) => {}
+ Err(e) => {
+ // We now free every element that has been initialized before:
+ // SAFETY: The loop initialized exactly the values from 0..i and since we
+ // return `Err` below, the caller will consider the memory at `slot` as
+ // uninitialized.
+ unsafe { ptr::drop_in_place(ptr::slice_from_raw_parts_mut(slot, i)) };
+ return Err(e);
+ }
+ }
+ }
+ Ok(())
+ };
+ // SAFETY: The initializer above initializes every element of the array. On failure it drops
+ // any initialized elements and returns `Err`.
+ unsafe { init_from_closure(init) }
+}
+
+/// Initializes an array by initializing each element via the provided initializer.
+///
+/// # Examples
+///
+/// ```rust
+/// use kernel::{sync::{Arc, Mutex}, init::pin_init_array_from_fn, new_mutex};
+/// let array: Arc<[Mutex<usize>; 1_000_000_000]>=
+/// Arc::pin_init(pin_init_array_from_fn(|i| new_mutex!(i))).unwrap();
+/// pr_info!("{}", array.len());
+/// ```
+pub fn pin_init_array_from_fn<I, const N: usize, T, E>(
+ mut make_init: impl FnMut(usize) -> I,
+) -> impl PinInit<[T; N], E>
+where
+ I: PinInit<T, E>,
+{
+ let init = move |slot: *mut [T; N]| {
+ let slot = slot.cast::<T>();
+ for i in 0..N {
+ let init = make_init(i);
+ // SAFETY: since 0 <= `i` < N, it is still in bounds of `[T; N]`.
+ let ptr = unsafe { slot.add(i) };
+ // SAFETY: The pointer is derived from `slot` and thus satisfies the `__pinned_init`
+ // requirements.
+ match unsafe { init.__pinned_init(ptr) } {
+ Ok(()) => {}
+ Err(e) => {
+ // We now have to free every element that has been initialized before, since we
+ // have to abide by the drop guarantee.
+ // SAFETY: The loop initialized exactly the values from 0..i and since we
+ // return `Err` below, the caller will consider the memory at `slot` as
+ // uninitialized.
+ unsafe { ptr::drop_in_place(ptr::slice_from_raw_parts_mut(slot, i)) };
+ return Err(e);
+ }
+ }
+ }
+ Ok(())
+ };
+ // SAFETY: The initializer above initializes every element of the array. On failure it drops
+ // any initialized elements and returns `Err`.
+ unsafe { pin_init_from_closure(init) }
+}
+
// SAFETY: Every type can be initialized by-value.
unsafe impl<T, E> Init<T, E> for T {
unsafe fn __init(self, slot: *mut T) -> Result<(), E> {
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 07/12] rust: init: Add functions to create array initializers
2023-07-19 14:20 ` [PATCH v2 07/12] rust: init: Add functions to create array initializers Benno Lossin
@ 2023-07-20 13:28 ` Alice Ryhl
2023-07-20 13:59 ` Martin Rodriguez Reboredo
1 sibling, 0 replies; 43+ messages in thread
From: Alice Ryhl @ 2023-07-20 13:28 UTC (permalink / raw)
To: benno.lossin
Cc: alex.gaynor, aliceryhl, bjorn3_gh, boqun.feng, gary, lina,
linux-kernel, nmi, ojeda, rust-for-linux, wedsonaf
> Add two functions `pin_init_array_from_fn` and `init_array_from_fn` that
> take a function that generates initializers for `T` from usize, the added
> functions then return an initializer for `[T; N]` where every element is
> initialized by an element returned from the generator function.
>
> Suggested-by: Asahi Lina <lina@asahilina.net>
> Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 07/12] rust: init: Add functions to create array initializers
2023-07-19 14:20 ` [PATCH v2 07/12] rust: init: Add functions to create array initializers Benno Lossin
2023-07-20 13:28 ` Alice Ryhl
@ 2023-07-20 13:59 ` Martin Rodriguez Reboredo
1 sibling, 0 replies; 43+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-07-20 13:59 UTC (permalink / raw)
To: Benno Lossin, Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel, Asahi Lina
On 7/19/23 11:20, Benno Lossin wrote:
> Add two functions `pin_init_array_from_fn` and `init_array_from_fn` that
> take a function that generates initializers for `T` from usize, the added
> functions then return an initializer for `[T; N]` where every element is
> initialized by an element returned from the generator function.
>
> Suggested-by: Asahi Lina <lina@asahilina.net>
> Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> [...]
> +/// Initializes an array by initializing each element via the provided initializer.
> +///
> +/// # Examples
> +///
> +/// ```rust
> +/// use kernel::{error::Error, init::init_array_from_fn};
> +/// let array: Box<[usize; 1_000_000_000]>= Box::init::<Error>(init_array_from_fn(|i| i)).unwrap();
> +/// pr_info!("{array:?}");
> +/// ```
Rather than debug printing the array I'd suggest to assert the struct
size or its elements instead.
> [...]
> +
> +/// Initializes an array by initializing each element via the provided initializer.
> +///
> +/// # Examples
> +///
> +/// ```rust
> +/// use kernel::{sync::{Arc, Mutex}, init::pin_init_array_from_fn, new_mutex};
> +/// let array: Arc<[Mutex<usize>; 1_000_000_000]>=
> +/// Arc::pin_init(pin_init_array_from_fn(|i| new_mutex!(i))).unwrap();
> +/// pr_info!("{}", array.len());
> +/// ```
> [...]
Same as above.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 08/12] rust: init: add support for arbitrary paths in init macros
2023-07-19 14:20 [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
` (6 preceding siblings ...)
2023-07-19 14:20 ` [PATCH v2 07/12] rust: init: Add functions to create array initializers Benno Lossin
@ 2023-07-19 14:21 ` Benno Lossin
2023-07-20 13:30 ` Alice Ryhl
2023-07-20 14:02 ` Martin Rodriguez Reboredo
2023-07-19 14:21 ` [PATCH v2 09/12] rust: init: implement Zeroable for Opaque<T> Benno Lossin
` (4 subsequent siblings)
12 siblings, 2 replies; 43+ messages in thread
From: Benno Lossin @ 2023-07-19 14:21 UTC (permalink / raw)
To: Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, Andreas Hindborg, rust-for-linux, linux-kernel,
Asahi Lina
Previously only `ident` and generic types were supported in the
`{try_}{pin_}init!` macros. This patch allows arbitrary path fragments,
so for example `Foo::Bar` but also very complex paths such as
`<Foo as Baz>::Bar::<0, i32>`.
Internally this is accomplished by using `path` fragments. Due to some
peculiar declarative macro limitations, we have to "forget" certain
additional parsing information in the token trees. This is achieved by
using the `paste!` proc macro. It does not actually modify the input,
since no `[< >]` will be present in the input, so it just strips the
information held by declarative macros. For example, if a declarative
macro takes `$t:path` as its input, it cannot sensibly propagate this to
a macro that takes `$($p:tt)*` as its input, since the `$t` token will
only be considered one `tt` token for the second macro. If we first pipe
the tokens through `paste!`, then it parses as expected.
Suggested-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
v1 -> v2:
* use Gary's `paste!` macro instead of `retokenize`
* remove the retokenize macro
rust/kernel/init/macros.rs | 54 ++++++++++++++++++++++++--------------
1 file changed, 35 insertions(+), 19 deletions(-)
diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
index f5d7f0943f60..345cfc0e6d37 100644
--- a/rust/kernel/init/macros.rs
+++ b/rust/kernel/init/macros.rs
@@ -998,7 +998,7 @@ impl<$($impl_generics)*> $pin_data<$($ty_generics)*>
macro_rules! __init_internal {
(
@this($($this:ident)?),
- @typ($t:ident $(::<$($generics:ty),*>)?),
+ @typ($t:path),
@fields($($fields:tt)*),
@error($err:ty),
// Either `PinData` or `InitData`, `$use_data` should only be present in the `PinData`
@@ -1012,7 +1012,7 @@ macro_rules! __init_internal {
) => {
$crate::__init_internal!(with_update_parsed:
@this($($this)?),
- @typ($t $(::<$($generics),*>)? ),
+ @typ($t),
@fields($($fields)*),
@error($err),
@data($data, $($use_data)?),
@@ -1023,7 +1023,7 @@ macro_rules! __init_internal {
};
(
@this($($this:ident)?),
- @typ($t:ident $(::<$($generics:ty),*>)?),
+ @typ($t:path),
@fields($($fields:tt)*),
@error($err:ty),
// Either `PinData` or `InitData`, `$use_data` should only be present in the `PinData`
@@ -1037,7 +1037,7 @@ macro_rules! __init_internal {
) => {
$crate::__init_internal!(with_update_parsed:
@this($($this)?),
- @typ($t $(::<$($generics),*>)? ),
+ @typ($t),
@fields($($fields)*),
@error($err),
@data($data, $($use_data)?),
@@ -1048,7 +1048,7 @@ macro_rules! __init_internal {
};
(
@this($($this:ident)?),
- @typ($t:ident $(::<$($generics:ty),*>)?),
+ @typ($t:path),
@fields($($fields:tt)*),
@error($err:ty),
// Either `PinData` or `InitData`, `$use_data` should only be present in the `PinData`
@@ -1062,7 +1062,7 @@ macro_rules! __init_internal {
) => {
$crate::__init_internal!(
@this($($this)?),
- @typ($t $(::<$($generics),*>)? ),
+ @typ($t),
@fields($($fields)*),
@error($err),
@data($data, $($use_data)?),
@@ -1073,7 +1073,7 @@ macro_rules! __init_internal {
};
(with_update_parsed:
@this($($this:ident)?),
- @typ($t:ident $(::<$($generics:ty),*>)?),
+ @typ($t:path),
@fields($($fields:tt)*),
@error($err:ty),
// Either `PinData` or `InitData`, `$use_data` should only be present in the `PinData`
@@ -1092,7 +1092,11 @@ macro_rules! __init_internal {
// Get the data about fields from the supplied type.
let data = unsafe {
use $crate::init::__internal::$has_data;
- $t$(::<$($generics),*>)?::$get_data()
+ // Here we abuse `paste!` to retokenize `$t`. Declarative macros have some internal
+ // information that is associated to already parsed fragments, so a path fragment
+ // cannot be used in this position. Doing the retokenization results in valid rust
+ // code.
+ ::kernel::macros::paste!($t::$get_data())
};
// Ensure that `data` really is of type `$data` and help with type inference:
let init = $crate::init::__internal::$data::make_closure::<_, __InitOk, $err>(
@@ -1251,7 +1255,7 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
};
(make_initializer:
@slot($slot:ident),
- @type_name($t:ident),
+ @type_name($t:path),
@munch_fields(..Zeroable::zeroed() $(,)?),
@acc($($acc:tt)*),
) => {
@@ -1268,15 +1272,21 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
// not get executed, so it has no effect.
::core::ptr::write($slot, zeroed);
zeroed = ::core::mem::zeroed();
- ::core::ptr::write($slot, $t {
- $($acc)*
- ..zeroed
- });
+ // Here we abuse `paste!` to retokenize `$t`. Declarative macros have some internal
+ // information that is associated to already parsed fragments, so a path fragment
+ // cannot be used in this position. Doing the retokenization results in valid rust
+ // code.
+ ::kernel::macros::paste!(
+ ::core::ptr::write($slot, $t {
+ $($acc)*
+ ..zeroed
+ });
+ );
}
};
(make_initializer:
@slot($slot:ident),
- @type_name($t:ident),
+ @type_name($t:path),
@munch_fields($(,)?),
@acc($($acc:tt)*),
) => {
@@ -1284,14 +1294,20 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
// Since we are in the closure that is never called, this will never get executed.
// We abuse `slot` to get the correct type inference here:
unsafe {
- ::core::ptr::write($slot, $t {
- $($acc)*
- });
+ // Here we abuse `paste!` to retokenize `$t`. Declarative macros have some internal
+ // information that is associated to already parsed fragments, so a path fragment
+ // cannot be used in this position. Doing the retokenization results in valid rust
+ // code.
+ ::kernel::macros::paste!(
+ ::core::ptr::write($slot, $t {
+ $($acc)*
+ });
+ );
}
};
(make_initializer:
@slot($slot:ident),
- @type_name($t:ident),
+ @type_name($t:path),
@munch_fields($field:ident <- $val:expr, $($rest:tt)*),
@acc($($acc:tt)*),
) => {
@@ -1304,7 +1320,7 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
};
(make_initializer:
@slot($slot:ident),
- @type_name($t:ident),
+ @type_name($t:path),
@munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
@acc($($acc:tt)*),
) => {
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 08/12] rust: init: add support for arbitrary paths in init macros
2023-07-19 14:21 ` [PATCH v2 08/12] rust: init: add support for arbitrary paths in init macros Benno Lossin
@ 2023-07-20 13:30 ` Alice Ryhl
2023-07-20 14:02 ` Martin Rodriguez Reboredo
1 sibling, 0 replies; 43+ messages in thread
From: Alice Ryhl @ 2023-07-20 13:30 UTC (permalink / raw)
To: benno.lossin
Cc: alex.gaynor, aliceryhl, bjorn3_gh, boqun.feng, gary, lina,
linux-kernel, nmi, ojeda, rust-for-linux, wedsonaf
Benno Lossin <benno.lossin@proton.me> writes:
> Previously only `ident` and generic types were supported in the
> `{try_}{pin_}init!` macros. This patch allows arbitrary path fragments,
> so for example `Foo::Bar` but also very complex paths such as
> `<Foo as Baz>::Bar::<0, i32>`.
>
> Internally this is accomplished by using `path` fragments. Due to some
> peculiar declarative macro limitations, we have to "forget" certain
> additional parsing information in the token trees. This is achieved by
> using the `paste!` proc macro. It does not actually modify the input,
> since no `[< >]` will be present in the input, so it just strips the
> information held by declarative macros. For example, if a declarative
> macro takes `$t:path` as its input, it cannot sensibly propagate this to
> a macro that takes `$($p:tt)*` as its input, since the `$t` token will
> only be considered one `tt` token for the second macro. If we first pipe
> the tokens through `paste!`, then it parses as expected.
>
> Suggested-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 08/12] rust: init: add support for arbitrary paths in init macros
2023-07-19 14:21 ` [PATCH v2 08/12] rust: init: add support for arbitrary paths in init macros Benno Lossin
2023-07-20 13:30 ` Alice Ryhl
@ 2023-07-20 14:02 ` Martin Rodriguez Reboredo
1 sibling, 0 replies; 43+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-07-20 14:02 UTC (permalink / raw)
To: Benno Lossin, Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel, Asahi Lina
On 7/19/23 11:21, Benno Lossin wrote:
> Previously only `ident` and generic types were supported in the
> `{try_}{pin_}init!` macros. This patch allows arbitrary path fragments,
> so for example `Foo::Bar` but also very complex paths such as
> `<Foo as Baz>::Bar::<0, i32>`.
>
> Internally this is accomplished by using `path` fragments. Due to some
> peculiar declarative macro limitations, we have to "forget" certain
> additional parsing information in the token trees. This is achieved by
> using the `paste!` proc macro. It does not actually modify the input,
> since no `[< >]` will be present in the input, so it just strips the
> information held by declarative macros. For example, if a declarative
> macro takes `$t:path` as its input, it cannot sensibly propagate this to
> a macro that takes `$($p:tt)*` as its input, since the `$t` token will
> only be considered one `tt` token for the second macro. If we first pipe
> the tokens through `paste!`, then it parses as expected.
>
> Suggested-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 09/12] rust: init: implement Zeroable for Opaque<T>
2023-07-19 14:20 [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
` (7 preceding siblings ...)
2023-07-19 14:21 ` [PATCH v2 08/12] rust: init: add support for arbitrary paths in init macros Benno Lossin
@ 2023-07-19 14:21 ` Benno Lossin
2023-07-20 13:34 ` Alice Ryhl
2023-07-20 14:03 ` Martin Rodriguez Reboredo
2023-07-19 14:21 ` [PATCH v2 10/12] rust: init: make `PinInit<T, E>` a supertrait of `Init<T, E>` Benno Lossin
` (3 subsequent siblings)
12 siblings, 2 replies; 43+ messages in thread
From: Benno Lossin @ 2023-07-19 14:21 UTC (permalink / raw)
To: Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, Andreas Hindborg, rust-for-linux, linux-kernel
Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
bit pattern for that type.
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
rust/kernel/types.rs | 2 ++
1 file changed, 2 insertions(+)
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index d849e1979ac7..f0f540578d0f 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -11,6 +11,7 @@
ops::{Deref, DerefMut},
ptr::NonNull,
};
+use macros::Zeroable;
/// Used to transfer ownership to and from foreign (non-Rust) languages.
///
@@ -251,6 +252,7 @@ fn drop(&mut self) {
///
/// This is meant to be used with FFI objects that are never interpreted by Rust code.
#[repr(transparent)]
+#[derive(Zeroable)]
pub struct Opaque<T> {
value: UnsafeCell<MaybeUninit<T>>,
_pin: PhantomPinned,
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 09/12] rust: init: implement Zeroable for Opaque<T>
2023-07-19 14:21 ` [PATCH v2 09/12] rust: init: implement Zeroable for Opaque<T> Benno Lossin
@ 2023-07-20 13:34 ` Alice Ryhl
2023-07-24 14:16 ` Benno Lossin
2023-07-20 14:03 ` Martin Rodriguez Reboredo
1 sibling, 1 reply; 43+ messages in thread
From: Alice Ryhl @ 2023-07-20 13:34 UTC (permalink / raw)
To: benno.lossin
Cc: alex.gaynor, aliceryhl, bjorn3_gh, boqun.feng, gary,
linux-kernel, nmi, ojeda, rust-for-linux, wedsonaf
Benno Lossin <benno.lossin@proton.me> writes:
> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
> bit pattern for that type.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> ///
> /// This is meant to be used with FFI objects that are never interpreted by Rust code.
> #[repr(transparent)]
> +#[derive(Zeroable)]
> pub struct Opaque<T> {
> value: UnsafeCell<MaybeUninit<T>>,
> _pin: PhantomPinned,
> }
Does this actually work? I don't think we implement Zeroable for
UnsafeCell.
I suggest you instead add Opaque to the `impl_zeroable!` macro in
`rust/kernel/init.rs`.
Alice
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 09/12] rust: init: implement Zeroable for Opaque<T>
2023-07-20 13:34 ` Alice Ryhl
@ 2023-07-24 14:16 ` Benno Lossin
2023-07-25 11:57 ` Alice Ryhl
0 siblings, 1 reply; 43+ messages in thread
From: Benno Lossin @ 2023-07-24 14:16 UTC (permalink / raw)
To: Alice Ryhl
Cc: alex.gaynor, bjorn3_gh, boqun.feng, gary, linux-kernel, nmi,
ojeda, rust-for-linux, wedsonaf
On 20.07.23 15:34, Alice Ryhl wrote:
> Benno Lossin <benno.lossin@proton.me> writes:
>> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
>> bit pattern for that type.
>>
>> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
>> ---
>> ///
>> /// This is meant to be used with FFI objects that are never interpreted by Rust code.
>> #[repr(transparent)]
>> +#[derive(Zeroable)]
>> pub struct Opaque<T> {
>> value: UnsafeCell<MaybeUninit<T>>,
>> _pin: PhantomPinned,
>> }
>
> Does this actually work? I don't think we implement Zeroable for
> UnsafeCell.
Good catch, this does compile, but only because the current
implementation of the derive expands to (modulo correct paths):
```
impl<T> Zeroable for Opaque<T>
where
UnsafeCell<MaybeUninit<T>>: Zeroable,
PhantomPinned: Zeroable,
{}
```
This implementation is of course useless, since `UnsafeCell` is never
`Zeroable` at the moment. We could of course implement that and then this
should work, but the question is if this is actually the desired output in
general. I thought before that this would be a good idea, but I forgot that
if the bounds are never satisfied it would silently compile.
Do you think that we should have this expanded output instead?
```
impl<T: Zeroable> Zeroable for Foo<T> {}
const _: () = {
fn assert_zeroable<T: Zeroable>() {}
fn ensure_zeroable<T: Zeroable>() {
assert_zeroable::<Field1>();
assert_zeroable::<Field2>();
}
};
```
If the input was
```
#[derive(Zeroable)]
struct Foo<T> {
field1: Field1,
field2: Field2,
}
```
> I suggest you instead add Opaque to the `impl_zeroable!` macro in
> `rust/kernel/init.rs`.
We would have to do this when using the alternative approach from
above, since we do not want the `Zeroable` bound on `T` for `Opaque`.
I wanted to avoid the `SAFETY` comment, since that is needed for the
`impl_zeroable` macro (as it has an unsafe block inside).
--
Cheers,
Benno
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 09/12] rust: init: implement Zeroable for Opaque<T>
2023-07-24 14:16 ` Benno Lossin
@ 2023-07-25 11:57 ` Alice Ryhl
2023-07-29 4:11 ` Benno Lossin
0 siblings, 1 reply; 43+ messages in thread
From: Alice Ryhl @ 2023-07-25 11:57 UTC (permalink / raw)
To: benno.lossin
Cc: alex.gaynor, aliceryhl, bjorn3_gh, boqun.feng, gary,
linux-kernel, nmi, ojeda, rust-for-linux, wedsonaf
Benno Lossin <benno.lossin@proton.me> writes:
> On 20.07.23 15:34, Alice Ryhl wrote:
>> Benno Lossin <benno.lossin@proton.me> writes:
>>> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
>>> bit pattern for that type.
>>>
>>> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
>>> ---
>>> ///
>>> /// This is meant to be used with FFI objects that are never interpreted by Rust code.
>>> #[repr(transparent)]
>>> +#[derive(Zeroable)]
>>> pub struct Opaque<T> {
>>> value: UnsafeCell<MaybeUninit<T>>,
>>> _pin: PhantomPinned,
>>> }
>>
>> Does this actually work? I don't think we implement Zeroable for
>> UnsafeCell.
>
> Good catch, this does compile, but only because the current
> implementation of the derive expands to (modulo correct paths):
> ```
> impl<T> Zeroable for Opaque<T>
> where
> UnsafeCell<MaybeUninit<T>>: Zeroable,
> PhantomPinned: Zeroable,
> {}
> ```
> This implementation is of course useless, since `UnsafeCell` is never
> `Zeroable` at the moment. We could of course implement that and then this
> should work, but the question is if this is actually the desired output in
> general. I thought before that this would be a good idea, but I forgot that
> if the bounds are never satisfied it would silently compile.
>
> Do you think that we should have this expanded output instead?
> ```
> impl<T: Zeroable> Zeroable for Foo<T> {}
> const _: () = {
> fn assert_zeroable<T: Zeroable>() {}
> fn ensure_zeroable<T: Zeroable>() {
> assert_zeroable::<Field1>();
> assert_zeroable::<Field2>();
> }
> };
> ```
> If the input was
> ```
> #[derive(Zeroable)]
> struct Foo<T> {
> field1: Field1,
> field2: Field2,
> }
> ```
Yeah. The way that these macros usually expand is by adding `where T:
Zeroable` to the impl for each generic parameter, and failing
compilation if that is not enough to ensure that all of the fields are
`Zeroable`.
You might want to consider this expansion instead:
```
impl<T: Zeroable> Zeroable for Foo<T> {}
const _: () = {
fn assert_zeroable<T: Zeroable>(arg: &T) {}
fn ensure_zeroable<T: Zeroable>(arg: &Foo<T>) {
assert_zeroable(&arg.field1);
assert_zeroable(&arg.field2);
}
};
```
>> I suggest you instead add Opaque to the `impl_zeroable!` macro in
>> `rust/kernel/init.rs`.
>
> We would have to do this when using the alternative approach from
> above, since we do not want the `Zeroable` bound on `T` for `Opaque`.
> I wanted to avoid the `SAFETY` comment, since that is needed for the
> `impl_zeroable` macro (as it has an unsafe block inside).
Indeed, if we expand the derive macro in the standard way, then it
doesn't work for `Opaque` due to the extra unnecessary bound.
Alice
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 09/12] rust: init: implement Zeroable for Opaque<T>
2023-07-25 11:57 ` Alice Ryhl
@ 2023-07-29 4:11 ` Benno Lossin
2023-07-29 8:14 ` Alice Ryhl
0 siblings, 1 reply; 43+ messages in thread
From: Benno Lossin @ 2023-07-29 4:11 UTC (permalink / raw)
To: Alice Ryhl
Cc: alex.gaynor, bjorn3_gh, boqun.feng, gary, linux-kernel, nmi,
ojeda, rust-for-linux, wedsonaf
On 25.07.23 13:57, Alice Ryhl wrote:
> Benno Lossin <benno.lossin@proton.me> writes:
>> On 20.07.23 15:34, Alice Ryhl wrote:
>>> Benno Lossin <benno.lossin@proton.me> writes:
>>>> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
>>>> bit pattern for that type.
>>>>
>>>> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
>>>> ---
>>>> ///
>>>> /// This is meant to be used with FFI objects that are never interpreted by Rust code.
>>>> #[repr(transparent)]
>>>> +#[derive(Zeroable)]
>>>> pub struct Opaque<T> {
>>>> value: UnsafeCell<MaybeUninit<T>>,
>>>> _pin: PhantomPinned,
>>>> }
>>>
>>> Does this actually work? I don't think we implement Zeroable for
>>> UnsafeCell.
>>
>> Good catch, this does compile, but only because the current
>> implementation of the derive expands to (modulo correct paths):
>> ```
>> impl<T> Zeroable for Opaque<T>
>> where
>> UnsafeCell<MaybeUninit<T>>: Zeroable,
>> PhantomPinned: Zeroable,
>> {}
>> ```
>> This implementation is of course useless, since `UnsafeCell` is never
>> `Zeroable` at the moment. We could of course implement that and then this
>> should work, but the question is if this is actually the desired output in
>> general. I thought before that this would be a good idea, but I forgot that
>> if the bounds are never satisfied it would silently compile.
>>
>> Do you think that we should have this expanded output instead?
>> ```
>> impl<T: Zeroable> Zeroable for Foo<T> {}
>> const _: () = {
>> fn assert_zeroable<T: Zeroable>() {}
>> fn ensure_zeroable<T: Zeroable>() {
>> assert_zeroable::<Field1>();
>> assert_zeroable::<Field2>();
>> }
>> };
>> ```
>> If the input was
>> ```
>> #[derive(Zeroable)]
>> struct Foo<T> {
>> field1: Field1,
>> field2: Field2,
>> }
>> ```
>
> Yeah. The way that these macros usually expand is by adding `where T:
> Zeroable` to the impl for each generic parameter, and failing
> compilation if that is not enough to ensure that all of the fields are
> `Zeroable`.
>
> You might want to consider this expansion instead:
> ```
> impl<T: Zeroable> Zeroable for Foo<T> {}
> const _: () = {
> fn assert_zeroable<T: Zeroable>(arg: &T) {}
> fn ensure_zeroable<T: Zeroable>(arg: &Foo<T>) {
> assert_zeroable(&arg.field1);
> assert_zeroable(&arg.field2);
> }
> };
> ```
Is there a specific reason you think that I should us references here
instead of the expansion from above (where I just use the types and
not the fields themselves)?
--
Cheers,
Benno
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 09/12] rust: init: implement Zeroable for Opaque<T>
2023-07-29 4:11 ` Benno Lossin
@ 2023-07-29 8:14 ` Alice Ryhl
0 siblings, 0 replies; 43+ messages in thread
From: Alice Ryhl @ 2023-07-29 8:14 UTC (permalink / raw)
To: Benno Lossin
Cc: alex.gaynor, bjorn3_gh, boqun.feng, gary, linux-kernel, nmi,
ojeda, rust-for-linux, wedsonaf, Alice Ryhl
I suggested it because it seemed less fragile.
That said, after seeing what #[derive(Eq)] expands to, I'm not so sure.
I'd probably investigate why the Eq macro has to expand to what it does.
On 7/29/23 06:11, Benno Lossin wrote:
> On 25.07.23 13:57, Alice Ryhl wrote:
>> Benno Lossin <benno.lossin@proton.me> writes:
>>> On 20.07.23 15:34, Alice Ryhl wrote:
>>>> Benno Lossin <benno.lossin@proton.me> writes:
>>>>> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
>>>>> bit pattern for that type.
>>>>>
>>>>> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
>>>>> ---
>>>>> ///
>>>>> /// This is meant to be used with FFI objects that are never interpreted by Rust code.
>>>>> #[repr(transparent)]
>>>>> +#[derive(Zeroable)]
>>>>> pub struct Opaque<T> {
>>>>> value: UnsafeCell<MaybeUninit<T>>,
>>>>> _pin: PhantomPinned,
>>>>> }
>>>>
>>>> Does this actually work? I don't think we implement Zeroable for
>>>> UnsafeCell.
>>>
>>> Good catch, this does compile, but only because the current
>>> implementation of the derive expands to (modulo correct paths):
>>> ```
>>> impl<T> Zeroable for Opaque<T>
>>> where
>>> UnsafeCell<MaybeUninit<T>>: Zeroable,
>>> PhantomPinned: Zeroable,
>>> {}
>>> ```
>>> This implementation is of course useless, since `UnsafeCell` is never
>>> `Zeroable` at the moment. We could of course implement that and then this
>>> should work, but the question is if this is actually the desired output in
>>> general. I thought before that this would be a good idea, but I forgot that
>>> if the bounds are never satisfied it would silently compile.
>>>
>>> Do you think that we should have this expanded output instead?
>>> ```
>>> impl<T: Zeroable> Zeroable for Foo<T> {}
>>> const _: () = {
>>> fn assert_zeroable<T: Zeroable>() {}
>>> fn ensure_zeroable<T: Zeroable>() {
>>> assert_zeroable::<Field1>();
>>> assert_zeroable::<Field2>();
>>> }
>>> };
>>> ```
>>> If the input was
>>> ```
>>> #[derive(Zeroable)]
>>> struct Foo<T> {
>>> field1: Field1,
>>> field2: Field2,
>>> }
>>> ```
>>
>> Yeah. The way that these macros usually expand is by adding `where T:
>> Zeroable` to the impl for each generic parameter, and failing
>> compilation if that is not enough to ensure that all of the fields are
>> `Zeroable`.
>>
>> You might want to consider this expansion instead:
>> ```
>> impl<T: Zeroable> Zeroable for Foo<T> {}
>> const _: () = {
>> fn assert_zeroable<T: Zeroable>(arg: &T) {}
>> fn ensure_zeroable<T: Zeroable>(arg: &Foo<T>) {
>> assert_zeroable(&arg.field1);
>> assert_zeroable(&arg.field2);
>> }
>> };
>> ```
>
> Is there a specific reason you think that I should us references here
> instead of the expansion from above (where I just use the types and
> not the fields themselves)?
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 09/12] rust: init: implement Zeroable for Opaque<T>
2023-07-19 14:21 ` [PATCH v2 09/12] rust: init: implement Zeroable for Opaque<T> Benno Lossin
2023-07-20 13:34 ` Alice Ryhl
@ 2023-07-20 14:03 ` Martin Rodriguez Reboredo
1 sibling, 0 replies; 43+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-07-20 14:03 UTC (permalink / raw)
To: Benno Lossin, Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel
On 7/19/23 11:21, Benno Lossin wrote:
> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
> bit pattern for that type.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 10/12] rust: init: make `PinInit<T, E>` a supertrait of `Init<T, E>`
2023-07-19 14:20 [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
` (8 preceding siblings ...)
2023-07-19 14:21 ` [PATCH v2 09/12] rust: init: implement Zeroable for Opaque<T> Benno Lossin
@ 2023-07-19 14:21 ` Benno Lossin
2023-07-20 13:36 ` Alice Ryhl
2023-07-20 14:07 ` Martin Rodriguez Reboredo
2023-07-19 14:21 ` [PATCH v2 11/12] rust: init: add `{pin_}chain` functions to `{Pin}Init<T, E>` Benno Lossin
` (2 subsequent siblings)
12 siblings, 2 replies; 43+ messages in thread
From: Benno Lossin @ 2023-07-19 14:21 UTC (permalink / raw)
To: Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, Andreas Hindborg, rust-for-linux, linux-kernel
Remove the blanket implementation of `PinInit<T, E> for I where I:
Init<T, E>`. This blanket implementation prevented custom types that
implement `PinInit`.
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
rust/kernel/init.rs | 20 +++++++-------------
rust/kernel/init/__internal.rs | 12 ++++++++++++
2 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index fa1ebdbf5f4b..3c7cd36a424b 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -805,7 +805,7 @@ pub unsafe trait PinInit<T: ?Sized, E = Infallible>: Sized {
///
/// [`Arc<T>`]: crate::sync::Arc
#[must_use = "An initializer must be used in order to create its value."]
-pub unsafe trait Init<T: ?Sized, E = Infallible>: Sized {
+pub unsafe trait Init<T: ?Sized, E = Infallible>: PinInit<T, E> {
/// Initializes `slot`.
///
/// # Safety
@@ -816,18 +816,6 @@ pub unsafe trait Init<T: ?Sized, E = Infallible>: Sized {
unsafe fn __init(self, slot: *mut T) -> Result<(), E>;
}
-// SAFETY: Every in-place initializer can also be used as a pin-initializer.
-unsafe impl<T: ?Sized, E, I> PinInit<T, E> for I
-where
- I: Init<T, E>,
-{
- unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
- // SAFETY: `__init` meets the same requirements as `__pinned_init`, except that it does not
- // require `slot` to not move after init.
- unsafe { self.__init(slot) }
- }
-}
-
/// Creates a new [`PinInit<T, E>`] from the given closure.
///
/// # Safety
@@ -968,6 +956,12 @@ unsafe fn __init(self, slot: *mut T) -> Result<(), E> {
Ok(())
}
}
+// SAFETY: Every type can be initialized by-value. `__pinned_init` calls `__init`.
+unsafe impl<T, E> PinInit<T, E> for T {
+ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
+ unsafe { self.__init(slot) }
+ }
+}
/// Smart pointer that can initialize memory in-place.
pub trait InPlaceInit<T>: Sized {
diff --git a/rust/kernel/init/__internal.rs b/rust/kernel/init/__internal.rs
index 7abd1fb65e41..12e195061525 100644
--- a/rust/kernel/init/__internal.rs
+++ b/rust/kernel/init/__internal.rs
@@ -32,6 +32,18 @@ unsafe fn __init(self, slot: *mut T) -> Result<(), E> {
}
}
+// SAFETY: While constructing the `InitClosure`, the user promised that it upholds the
+// `__pinned_init` invariants.
+unsafe impl<T: ?Sized, F, E> PinInit<T, E> for InitClosure<F, T, E>
+where
+ F: FnOnce(*mut T) -> Result<(), E>,
+{
+ #[inline]
+ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
+ (self.0)(slot)
+ }
+}
+
/// This trait is only implemented via the `#[pin_data]` proc-macro. It is used to facilitate
/// the pin projections within the initializers.
///
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 10/12] rust: init: make `PinInit<T, E>` a supertrait of `Init<T, E>`
2023-07-19 14:21 ` [PATCH v2 10/12] rust: init: make `PinInit<T, E>` a supertrait of `Init<T, E>` Benno Lossin
@ 2023-07-20 13:36 ` Alice Ryhl
2023-07-20 14:07 ` Martin Rodriguez Reboredo
1 sibling, 0 replies; 43+ messages in thread
From: Alice Ryhl @ 2023-07-20 13:36 UTC (permalink / raw)
To: benno.lossin
Cc: alex.gaynor, aliceryhl, bjorn3_gh, boqun.feng, gary,
linux-kernel, nmi, ojeda, rust-for-linux, wedsonaf
Benno Lossin <benno.lossin@proton.me> writes:
> Remove the blanket implementation of `PinInit<T, E> for I where I:
> Init<T, E>`. This blanket implementation prevented custom types that
> implement `PinInit`.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 10/12] rust: init: make `PinInit<T, E>` a supertrait of `Init<T, E>`
2023-07-19 14:21 ` [PATCH v2 10/12] rust: init: make `PinInit<T, E>` a supertrait of `Init<T, E>` Benno Lossin
2023-07-20 13:36 ` Alice Ryhl
@ 2023-07-20 14:07 ` Martin Rodriguez Reboredo
1 sibling, 0 replies; 43+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-07-20 14:07 UTC (permalink / raw)
To: Benno Lossin, Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel
On 7/19/23 11:21, Benno Lossin wrote:
> Remove the blanket implementation of `PinInit<T, E> for I where I:
> Init<T, E>`. This blanket implementation prevented custom types that
> implement `PinInit`.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> [...]
> @@ -968,6 +956,12 @@ unsafe fn __init(self, slot: *mut T) -> Result<(), E> {
> Ok(())
> }
> }
I'd put an empty line here, so to separate each block.
> +// SAFETY: Every type can be initialized by-value. `__pinned_init` calls `__init`.
> +unsafe impl<T, E> PinInit<T, E> for T {
> + unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
> + unsafe { self.__init(slot) }
> + }
> +}
>
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 11/12] rust: init: add `{pin_}chain` functions to `{Pin}Init<T, E>`
2023-07-19 14:20 [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
` (9 preceding siblings ...)
2023-07-19 14:21 ` [PATCH v2 10/12] rust: init: make `PinInit<T, E>` a supertrait of `Init<T, E>` Benno Lossin
@ 2023-07-19 14:21 ` Benno Lossin
2023-07-21 0:23 ` Martin Rodriguez Reboredo
2023-07-19 14:21 ` [PATCH v2 12/12] rust: init: update expanded macro explanation Benno Lossin
2023-07-19 15:23 ` [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
12 siblings, 1 reply; 43+ messages in thread
From: Benno Lossin @ 2023-07-19 14:21 UTC (permalink / raw)
To: Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, Andreas Hindborg, rust-for-linux, linux-kernel,
Asahi Lina
The `{pin_}chain` functions extend an initializer: it not only
initializes the value, but also executes a closure taking a reference to
the initialized value. This allows to do something with a value directly
after initialization.
Suggested-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
rust/kernel/init.rs | 138 +++++++++++++++++++++++++++++++++
rust/kernel/init/__internal.rs | 2 +-
2 files changed, 139 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 3c7cd36a424b..3b0df839f64c 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -773,6 +773,77 @@ pub unsafe trait PinInit<T: ?Sized, E = Infallible>: Sized {
/// deallocate.
/// - `slot` will not move until it is dropped, i.e. it will be pinned.
unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E>;
+
+ /// First initializes the value using `self` then calls the function `f` with the initialized
+ /// value.
+ ///
+ /// # Examples
+ ///
+ /// ```rust
+ /// # #![allow(clippy::disallowed_names)]
+ /// use kernel::{types::Opaque, init::pin_init_from_closure};
+ /// #[repr(C)]
+ /// struct RawFoo([u8; 16]);
+ /// extern {
+ /// fn init_foo(_: *mut RawFoo);
+ /// }
+ ///
+ /// #[pin_data]
+ /// struct Foo {
+ /// #[pin]
+ /// raw: Opaque<RawFoo>,
+ /// }
+ ///
+ /// impl Foo {
+ /// fn setup(self: Pin<&mut Self>) {
+ /// pr_info!("Setting up foo");
+ /// }
+ /// }
+ ///
+ /// let foo = pin_init!(Foo {
+ /// raw <- unsafe {
+ /// Opaque::ffi_init(|s| {
+ /// init_foo(s);
+ /// })
+ /// },
+ /// }).pin_chain(|foo| {
+ /// foo.setup();
+ /// Ok(())
+ /// });
+ /// ```
+ fn pin_chain<F>(self, f: F) -> ChainPinInit<Self, F, T, E>
+ where
+ F: FnOnce(Pin<&mut T>) -> Result<(), E>,
+ {
+ ChainPinInit(self, f, PhantomData)
+ }
+}
+
+/// An initializer returned by [`PinInit::pin_chain`].
+pub struct ChainPinInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant<(E, Box<T>)>);
+
+// SAFETY: the `__pinned_init` function is implemented such that it
+// - returns `Ok(())` on successful initialization,
+// - returns `Err(err)` on error and in this case `slot` will be dropped.
+// - considers `slot` pinned.
+unsafe impl<T: ?Sized, E, I, F> PinInit<T, E> for ChainPinInit<I, F, T, E>
+where
+ I: PinInit<T, E>,
+ F: FnOnce(Pin<&mut T>) -> Result<(), E>,
+{
+ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
+ // SAFETY: all requirements fulfilled since this function is `__pinned_init`.
+ unsafe { self.0.__pinned_init(slot)? };
+ // SAFETY: The above call initialized `slot` and we still have unique access.
+ let val = unsafe { &mut *slot };
+ // SAFETY: `slot` is considered pinned
+ let val = unsafe { Pin::new_unchecked(val) };
+ (self.1)(val).map_err(|e| {
+ // SAFETY: `slot` was initialized above.
+ unsafe { core::ptr::drop_in_place(slot) };
+ e
+ })
+ }
}
/// An initializer for `T`.
@@ -814,6 +885,73 @@ pub unsafe trait Init<T: ?Sized, E = Infallible>: PinInit<T, E> {
/// - the caller does not touch `slot` when `Err` is returned, they are only permitted to
/// deallocate.
unsafe fn __init(self, slot: *mut T) -> Result<(), E>;
+
+ /// First initializes the value using `self` then calls the function `f` with the initialized
+ /// value.
+ ///
+ /// # Examples
+ ///
+ /// ```rust
+ /// # #![allow(clippy::disallowed_names)]
+ /// use kernel::{types::Opaque, init::{self, init_from_closure}};
+ /// struct Foo {
+ /// buf: [u8; 1_000_000],
+ /// }
+ ///
+ /// impl Foo {
+ /// fn setup(&mut self) {
+ /// pr_info!("Setting up foo");
+ /// }
+ /// }
+ ///
+ /// let foo = init!(Foo {
+ /// buf <- init::zeroed()
+ /// }).chain(|foo| {
+ /// foo.setup();
+ /// Ok(())
+ /// });
+ /// ```
+ fn chain<F>(self, f: F) -> ChainInit<Self, F, T, E>
+ where
+ F: FnOnce(&mut T) -> Result<(), E>,
+ {
+ ChainInit(self, f, PhantomData)
+ }
+}
+
+/// An initializer returned by [`Init::chain`].
+pub struct ChainInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant<(E, Box<T>)>);
+
+// SAFETY: the `__init` function is implemented such that it
+// - returns `Ok(())` on successful initialization,
+// - returns `Err(err)` on error and in this case `slot` will be dropped.
+unsafe impl<T: ?Sized, E, I, F> Init<T, E> for ChainInit<I, F, T, E>
+where
+ I: Init<T, E>,
+ F: FnOnce(&mut T) -> Result<(), E>,
+{
+ unsafe fn __init(self, slot: *mut T) -> Result<(), E> {
+ // SAFETY: all requirements fulfilled since this function is `__init`.
+ unsafe { self.0.__pinned_init(slot)? };
+ // SAFETY: The above call initialized `slot` and we still have unique access.
+ (self.1)(unsafe { &mut *slot }).map_err(|e| {
+ // SAFETY: `slot` was initialized above.
+ unsafe { core::ptr::drop_in_place(slot) };
+ e
+ })
+ }
+}
+
+// SAFETY: `__pinned_init` behaves exactly the same as `__init`.
+unsafe impl<T: ?Sized, E, I, F> PinInit<T, E> for ChainInit<I, F, T, E>
+where
+ I: Init<T, E>,
+ F: FnOnce(&mut T) -> Result<(), E>,
+{
+ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
+ // SAFETY: `__init` has less strict requirements compared to `__pinned_init`.
+ unsafe { self.__init(slot) }
+ }
}
/// Creates a new [`PinInit<T, E>`] from the given closure.
diff --git a/rust/kernel/init/__internal.rs b/rust/kernel/init/__internal.rs
index 12e195061525..db3372619ecd 100644
--- a/rust/kernel/init/__internal.rs
+++ b/rust/kernel/init/__internal.rs
@@ -13,7 +13,7 @@
///
/// [nomicon]: https://doc.rust-lang.org/nomicon/subtyping.html
/// [this table]: https://doc.rust-lang.org/nomicon/phantom-data.html#table-of-phantomdata-patterns
-type Invariant<T> = PhantomData<fn(*mut T) -> *mut T>;
+pub(super) type Invariant<T> = PhantomData<fn(*mut T) -> *mut T>;
/// This is the module-internal type implementing `PinInit` and `Init`. It is unsafe to create this
/// type, since the closure needs to fulfill the same safety requirement as the
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 11/12] rust: init: add `{pin_}chain` functions to `{Pin}Init<T, E>`
2023-07-19 14:21 ` [PATCH v2 11/12] rust: init: add `{pin_}chain` functions to `{Pin}Init<T, E>` Benno Lossin
@ 2023-07-21 0:23 ` Martin Rodriguez Reboredo
2023-07-24 14:08 ` Benno Lossin
0 siblings, 1 reply; 43+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-07-21 0:23 UTC (permalink / raw)
To: Benno Lossin, Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel, Asahi Lina
On 7/19/23 11:21, Benno Lossin wrote:
> The `{pin_}chain` functions extend an initializer: it not only
> initializes the value, but also executes a closure taking a reference to
> the initialized value. This allows to do something with a value directly
> after initialization.
>
> Suggested-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> rust/kernel/init.rs | 138 +++++++++++++++++++++++++++++++++
> rust/kernel/init/__internal.rs | 2 +-
> 2 files changed, 139 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> index 3c7cd36a424b..3b0df839f64c 100644
> --- a/rust/kernel/init.rs
> +++ b/rust/kernel/init.rs
> @@ -773,6 +773,77 @@ pub unsafe trait PinInit<T: ?Sized, E = Infallible>: Sized {
> /// deallocate.
> /// - `slot` will not move until it is dropped, i.e. it will be pinned.
> unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E>;
> +
> + /// First initializes the value using `self` then calls the function `f` with the initialized
> + /// value.
> + ///
> + /// # Examples
> + ///
> + /// ```rust
> + /// # #![allow(clippy::disallowed_names)]
> + /// use kernel::{types::Opaque, init::pin_init_from_closure};
> + /// #[repr(C)]
> + /// struct RawFoo([u8; 16]);
> + /// extern {
> + /// fn init_foo(_: *mut RawFoo);
> + /// }
> + ///
> + /// #[pin_data]
> + /// struct Foo {
> + /// #[pin]
> + /// raw: Opaque<RawFoo>,
> + /// }
> + ///
> + /// impl Foo {
> + /// fn setup(self: Pin<&mut Self>) {
> + /// pr_info!("Setting up foo");
> + /// }
> + /// }
> + ///
> + /// let foo = pin_init!(Foo {
> + /// raw <- unsafe {
> + /// Opaque::ffi_init(|s| {
> + /// init_foo(s);
> + /// })
> + /// },
> + /// }).pin_chain(|foo| {
> + /// foo.setup();
> + /// Ok(())
> + /// });
> + /// ```
> + fn pin_chain<F>(self, f: F) -> ChainPinInit<Self, F, T, E>
> + where
> + F: FnOnce(Pin<&mut T>) -> Result<(), E>,
> + {
> + ChainPinInit(self, f, PhantomData)
> + }
> +}
> +
> +/// An initializer returned by [`PinInit::pin_chain`].
> +pub struct ChainPinInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant<(E, Box<T>)>);
> +
> +// SAFETY: the `__pinned_init` function is implemented such that it
> +// - returns `Ok(())` on successful initialization,
> +// - returns `Err(err)` on error and in this case `slot` will be dropped.
> +// - considers `slot` pinned.
> +unsafe impl<T: ?Sized, E, I, F> PinInit<T, E> for ChainPinInit<I, F, T, E>
> +where
> + I: PinInit<T, E>,
> + F: FnOnce(Pin<&mut T>) -> Result<(), E>,
> +{
> + unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
> + // SAFETY: all requirements fulfilled since this function is `__pinned_init`.
> + unsafe { self.0.__pinned_init(slot)? };
> + // SAFETY: The above call initialized `slot` and we still have unique access.
> + let val = unsafe { &mut *slot };
> + // SAFETY: `slot` is considered pinned
> + let val = unsafe { Pin::new_unchecked(val) };
> + (self.1)(val).map_err(|e| {
> + // SAFETY: `slot` was initialized above.
> + unsafe { core::ptr::drop_in_place(slot) };
> + e
I might stumble upon an error like EAGAIN if I call `pin_chain` but that
means `slot` will be dropped. So my recommendation is to either not drop
the value or detail in `pin_chain`'s doc comment that the closure will
drop on error.
> + })
> + }
> }
>
> /// An initializer for `T`.
> @@ -814,6 +885,73 @@ pub unsafe trait Init<T: ?Sized, E = Infallible>: PinInit<T, E> {
> /// - the caller does not touch `slot` when `Err` is returned, they are only permitted to
> /// deallocate.
> unsafe fn __init(self, slot: *mut T) -> Result<(), E>;
> +
> + /// First initializes the value using `self` then calls the function `f` with the initialized
> + /// value.
> + ///
> + /// # Examples
> + ///
> + /// ```rust
> + /// # #![allow(clippy::disallowed_names)]
> + /// use kernel::{types::Opaque, init::{self, init_from_closure}};
> + /// struct Foo {
> + /// buf: [u8; 1_000_000],
> + /// }
> + ///
> + /// impl Foo {
> + /// fn setup(&mut self) {
> + /// pr_info!("Setting up foo");
> + /// }
> + /// }
> + ///
> + /// let foo = init!(Foo {
> + /// buf <- init::zeroed()
> + /// }).chain(|foo| {
> + /// foo.setup();
> + /// Ok(())
> + /// });
> + /// ```
> + fn chain<F>(self, f: F) -> ChainInit<Self, F, T, E>
> + where
> + F: FnOnce(&mut T) -> Result<(), E>,
> + {
> + ChainInit(self, f, PhantomData)
> + }
> +}
> +
> +/// An initializer returned by [`Init::chain`].
> +pub struct ChainInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant<(E, Box<T>)>);
> +
> +// SAFETY: the `__init` function is implemented such that it
> +// - returns `Ok(())` on successful initialization,
> +// - returns `Err(err)` on error and in this case `slot` will be dropped.
> +unsafe impl<T: ?Sized, E, I, F> Init<T, E> for ChainInit<I, F, T, E>
> +where
> + I: Init<T, E>,
> + F: FnOnce(&mut T) -> Result<(), E>,
> +{
> + unsafe fn __init(self, slot: *mut T) -> Result<(), E> {
> + // SAFETY: all requirements fulfilled since this function is `__init`.
> + unsafe { self.0.__pinned_init(slot)? };
> + // SAFETY: The above call initialized `slot` and we still have unique access.
> + (self.1)(unsafe { &mut *slot }).map_err(|e| {
> + // SAFETY: `slot` was initialized above.
> + unsafe { core::ptr::drop_in_place(slot) };
> + e
> + })
> + }
> +}
> +
> [...]
Same as above.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 11/12] rust: init: add `{pin_}chain` functions to `{Pin}Init<T, E>`
2023-07-21 0:23 ` Martin Rodriguez Reboredo
@ 2023-07-24 14:08 ` Benno Lossin
2023-07-24 16:07 ` Martin Rodriguez Reboredo
0 siblings, 1 reply; 43+ messages in thread
From: Benno Lossin @ 2023-07-24 14:08 UTC (permalink / raw)
To: Martin Rodriguez Reboredo, Miguel Ojeda, Wedson Almeida Filho,
Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel, Asahi Lina
On 21.07.23 02:23, Martin Rodriguez Reboredo wrote:
> On 7/19/23 11:21, Benno Lossin wrote:
>> +/// An initializer returned by [`PinInit::pin_chain`].
>> +pub struct ChainPinInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant<(E, Box<T>)>);
>> +
>> +// SAFETY: the `__pinned_init` function is implemented such that it
>> +// - returns `Ok(())` on successful initialization,
>> +// - returns `Err(err)` on error and in this case `slot` will be dropped.
>> +// - considers `slot` pinned.
>> +unsafe impl<T: ?Sized, E, I, F> PinInit<T, E> for ChainPinInit<I, F, T, E>
>> +where
>> + I: PinInit<T, E>,
>> + F: FnOnce(Pin<&mut T>) -> Result<(), E>,
>> +{
>> + unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
>> + // SAFETY: all requirements fulfilled since this function is `__pinned_init`.
>> + unsafe { self.0.__pinned_init(slot)? };
>> + // SAFETY: The above call initialized `slot` and we still have unique access.
>> + let val = unsafe { &mut *slot };
>> + // SAFETY: `slot` is considered pinned
>> + let val = unsafe { Pin::new_unchecked(val) };
>> + (self.1)(val).map_err(|e| {
>> + // SAFETY: `slot` was initialized above.
>> + unsafe { core::ptr::drop_in_place(slot) };
>> + e
>
> I might stumble upon an error like EAGAIN if I call `pin_chain` but that
> means `slot` will be dropped. So my recommendation is to either not drop
> the value or detail in `pin_chain`'s doc comment that the closure will
> drop on error.
This is a bit confusing to me, because dropping the value on returning `Err`
is a safety requirement of `PinInit`. Could you elaborate why this is
surprising? I can of course add it to the documentation, but I do not see
how it could be implemented differently. Since if you do not drop the value
here, nobody would know that it is still initialized.
--
Cheers,
Benno
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 11/12] rust: init: add `{pin_}chain` functions to `{Pin}Init<T, E>`
2023-07-24 14:08 ` Benno Lossin
@ 2023-07-24 16:07 ` Martin Rodriguez Reboredo
2023-07-24 21:55 ` Benno Lossin
0 siblings, 1 reply; 43+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-07-24 16:07 UTC (permalink / raw)
To: Benno Lossin, Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel, Asahi Lina
On 7/24/23 11:08, Benno Lossin wrote:
> On 21.07.23 02:23, Martin Rodriguez Reboredo wrote:
>> On 7/19/23 11:21, Benno Lossin wrote:
>>> +/// An initializer returned by [`PinInit::pin_chain`].
>>> +pub struct ChainPinInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant<(E, Box<T>)>);
>>> +
>>> +// SAFETY: the `__pinned_init` function is implemented such that it
>>> +// - returns `Ok(())` on successful initialization,
>>> +// - returns `Err(err)` on error and in this case `slot` will be dropped.
>>> +// - considers `slot` pinned.
>>> +unsafe impl<T: ?Sized, E, I, F> PinInit<T, E> for ChainPinInit<I, F, T, E>
>>> +where
>>> + I: PinInit<T, E>,
>>> + F: FnOnce(Pin<&mut T>) -> Result<(), E>,
>>> +{
>>> + unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
>>> + // SAFETY: all requirements fulfilled since this function is `__pinned_init`.
>>> + unsafe { self.0.__pinned_init(slot)? };
>>> + // SAFETY: The above call initialized `slot` and we still have unique access.
>>> + let val = unsafe { &mut *slot };
>>> + // SAFETY: `slot` is considered pinned
>>> + let val = unsafe { Pin::new_unchecked(val) };
>>> + (self.1)(val).map_err(|e| {
>>> + // SAFETY: `slot` was initialized above.
>>> + unsafe { core::ptr::drop_in_place(slot) };
>>> + e
>>
>> I might stumble upon an error like EAGAIN if I call `pin_chain` but that
>> means `slot` will be dropped. So my recommendation is to either not drop
>> the value or detail in `pin_chain`'s doc comment that the closure will
>> drop on error.
>
> This is a bit confusing to me, because dropping the value on returning `Err`
> is a safety requirement of `PinInit`. Could you elaborate why this is
> surprising? I can of course add it to the documentation, but I do not see
> how it could be implemented differently. Since if you do not drop the value
> here, nobody would know that it is still initialized.
I knew about the requirement of dropping on `Err`, but what has caught my
attention is that `{pin_}chain` might not abide with it per the doc
comment as it says that `self` is initialized before calling `f`...
/// First initializes the value using `self` then calls the function
/// `f` with the initialized value.
But one can not know what would happen when `f` fails, specially if
such failure can be ignored or it's only temporarily.
So then, the best course IMO is to mention that in some way the value is
still being initialized, kinda setting it up, and that it will be dropped
when an error is returned. WDYT?
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 11/12] rust: init: add `{pin_}chain` functions to `{Pin}Init<T, E>`
2023-07-24 16:07 ` Martin Rodriguez Reboredo
@ 2023-07-24 21:55 ` Benno Lossin
0 siblings, 0 replies; 43+ messages in thread
From: Benno Lossin @ 2023-07-24 21:55 UTC (permalink / raw)
To: Martin Rodriguez Reboredo, Miguel Ojeda, Wedson Almeida Filho,
Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel, Asahi Lina
On 7/24/23 18:07, Martin Rodriguez Reboredo wrote:
> On 7/24/23 11:08, Benno Lossin wrote:
>> This is a bit confusing to me, because dropping the value on returning `Err`
>> is a safety requirement of `PinInit`. Could you elaborate why this is
>> surprising? I can of course add it to the documentation, but I do not see
>> how it could be implemented differently. Since if you do not drop the value
>> here, nobody would know that it is still initialized.
>
> I knew about the requirement of dropping on `Err`, but what has caught my
> attention is that `{pin_}chain` might not abide with it per the doc
> comment as it says that `self` is initialized before calling `f`...
>
> /// First initializes the value using `self` then calls the function
> /// `f` with the initialized value.
>
> But one can not know what would happen when `f` fails, specially if
> such failure can be ignored or it's only temporarily.
>
> So then, the best course IMO is to mention that in some way the value is
> still being initialized, kinda setting it up, and that it will be dropped
> when an error is returned. WDYT?
I see, then I will just expand the documentation.
--
Cheers,
Benno
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 12/12] rust: init: update expanded macro explanation
2023-07-19 14:20 [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
` (10 preceding siblings ...)
2023-07-19 14:21 ` [PATCH v2 11/12] rust: init: add `{pin_}chain` functions to `{Pin}Init<T, E>` Benno Lossin
@ 2023-07-19 14:21 ` Benno Lossin
2023-07-21 0:24 ` Martin Rodriguez Reboredo
2023-07-19 15:23 ` [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
12 siblings, 1 reply; 43+ messages in thread
From: Benno Lossin @ 2023-07-19 14:21 UTC (permalink / raw)
To: Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, Andreas Hindborg, rust-for-linux, linux-kernel
The previous patches changed the internals of the macros resulting in
the example expanded code being outdated. This patch updates the example
and only changes documentation.
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
rust/kernel/init/macros.rs | 126 ++++++++++++++++++++-----------------
1 file changed, 69 insertions(+), 57 deletions(-)
diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
index 345cfc0e6d37..1d492c4bab69 100644
--- a/rust/kernel/init/macros.rs
+++ b/rust/kernel/init/macros.rs
@@ -45,7 +45,7 @@
//! #[pinned_drop]
//! impl PinnedDrop for Foo {
//! fn drop(self: Pin<&mut Self>) {
-//! println!("{self:p} is getting dropped.");
+//! pr_info!("{self:p} is getting dropped.");
//! }
//! }
//!
@@ -170,8 +170,10 @@
//! t: T,
//! }
//! #[doc(hidden)]
-//! impl<'__pin, T>
-//! ::core::marker::Unpin for Bar<T> where __Unpin<'__pin, T>: ::core::marker::Unpin {}
+//! impl<'__pin, T> ::core::marker::Unpin for Bar<T>
+//! where
+//! __Unpin<'__pin, T>: ::core::marker::Unpin,
+//! {}
//! // Now we need to ensure that `Bar` does not implement `Drop`, since that would give users
//! // access to `&mut self` inside of `drop` even if the struct was pinned. This could lead to
//! // UB with only safe code, so we disallow this by giving a trait implementation error using
@@ -188,8 +190,9 @@
//! // for safety, but a good sanity check, since no normal code calls `PinnedDrop::drop`.
//! #[allow(non_camel_case_types)]
//! trait UselessPinnedDropImpl_you_need_to_specify_PinnedDrop {}
-//! impl<T: ::kernel::init::PinnedDrop>
-//! UselessPinnedDropImpl_you_need_to_specify_PinnedDrop for T {}
+//! impl<
+//! T: ::kernel::init::PinnedDrop,
+//! > UselessPinnedDropImpl_you_need_to_specify_PinnedDrop for T {}
//! impl<T> UselessPinnedDropImpl_you_need_to_specify_PinnedDrop for Bar<T> {}
//! };
//! ```
@@ -219,7 +222,7 @@
//! // return type and shadow it later when we insert the arbitrary user code. That way
//! // there will be no possibility of returning without `unsafe`.
//! struct __InitOk;
-//! // Get the pin-data type from the initialized type.
+//! // Get the data about fields from the supplied type.
//! // - the function is unsafe, hence the unsafe block
//! // - we `use` the `HasPinData` trait in the block, it is only available in that
//! // scope.
@@ -227,8 +230,7 @@
//! use ::kernel::init::__internal::HasPinData;
//! Self::__pin_data()
//! };
-//! // Use `data` to help with type inference, the closure supplied will have the type
-//! // `FnOnce(*mut Self) -> Result<__InitOk, Infallible>`.
+//! // Ensure that `data` really is of type `PinData` and help with type inference:
//! let init = ::kernel::init::__internal::PinData::make_closure::<
//! _,
//! __InitOk,
@@ -236,71 +238,75 @@
//! >(data, move |slot| {
//! {
//! // Shadow the structure so it cannot be used to return early. If a user
-//! // tries to write `return Ok(__InitOk)`, then they get a type error, since
-//! // that will refer to this struct instead of the one defined above.
+//! // tries to write `return Ok(__InitOk)`, then they get a type error,
+//! // since that will refer to this struct instead of the one defined
+//! // above.
//! struct __InitOk;
//! // This is the expansion of `t,`, which is syntactic sugar for `t: t,`.
-//! unsafe { ::core::ptr::write(::core::addr_of_mut!((*slot).t), t) };
-//! // Since initialization could fail later (not in this case, since the 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. We make a reference to it, so users
-//! // cannot `mem::forget` it from the initializer, since the name is the same
-//! // as the field (including hygiene).
-//! let t = &unsafe {
-//! ::kernel::init::__internal::DropGuard::new(
-//! ::core::addr_of_mut!((*slot).t),
-//! )
+//! {
+//! unsafe { ::core::ptr::write(::core::addr_of_mut!((*slot).t), t) };
+//! }
+//! // Since initialization could fail later (not in this case, since the
+//! // 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 {
+//! ::pinned_init::__internal::DropGuard::new(::core::addr_of_mut!((*slot).t))
//! };
//! // Expansion of `x: 0,`:
-//! // Since this can be an arbitrary expression we cannot place it inside of
-//! // the `unsafe` block, so we bind it here.
-//! let x = 0;
-//! unsafe { ::core::ptr::write(::core::addr_of_mut!((*slot).x), x) };
+//! // Since this can be an arbitrary expression we cannot place it inside
+//! // of the `unsafe` block, so we bind it here.
+//! {
+//! let x = 0;
+//! unsafe { ::core::ptr::write(::core::addr_of_mut!((*slot).x), x) };
+//! }
//! // We again create a `DropGuard`.
-//! let x = &unsafe {
-//! ::kernel::init::__internal::DropGuard::new(
-//! ::core::addr_of_mut!((*slot).x),
-//! )
+//! let x = 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);
//! // 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.
-//! // Additionally we abuse `slot` to automatically infer the correct type for
-//! // the struct. This is also another check that every field is accessible
-//! // from this scope.
+//! // Additionally we abuse `slot` to automatically infer the correct type
+//! // for the struct. This is also another check that every field is
+//! // accessible from this scope.
//! #[allow(unreachable_code, clippy::diverging_sub_expression)]
-//! if false {
+//! let _ = || {
//! unsafe {
//! ::core::ptr::write(
//! slot,
//! Self {
-//! // We only care about typecheck finding every field here,
-//! // the expression does not matter, just conjure one using
-//! // `panic!()`:
+//! // We only care about typecheck finding every field
+//! // here, the expression does not matter, just conjure
+//! // one using `panic!()`:
//! t: ::core::panic!(),
//! x: ::core::panic!(),
//! },
//! );
//! };
-//! }
-//! // Since initialization has successfully completed, we can now forget the
-//! // guards. This is not `mem::forget`, since we only have `&DropGuard`.
-//! unsafe { ::kernel::init::__internal::DropGuard::forget(t) };
-//! unsafe { ::kernel::init::__internal::DropGuard::forget(x) };
+//! };
//! }
//! // We leave the scope above and gain access to the previously shadowed
//! // `__InitOk` that we need to return.
//! Ok(__InitOk)
//! });
//! // Change the return type from `__InitOk` to `()`.
-//! let init = move |slot| -> ::core::result::Result<(), ::core::convert::Infallible> {
+//! let init = move |
+//! slot,
+//! | -> ::core::result::Result<(), ::core::convert::Infallible> {
//! init(slot).map(|__InitOk| ())
//! };
//! // Construct the initializer.
//! let init = unsafe {
-//! ::kernel::init::pin_init_from_closure::<_, ::core::convert::Infallible>(init)
+//! ::kernel::init::pin_init_from_closure::<
+//! _,
+//! ::core::convert::Infallible,
+//! >(init)
//! };
//! init
//! }
@@ -374,7 +380,10 @@
//! b: Bar<u32>,
//! }
//! #[doc(hidden)]
-//! impl<'__pin> ::core::marker::Unpin for Foo where __Unpin<'__pin>: ::core::marker::Unpin {}
+//! impl<'__pin> ::core::marker::Unpin for Foo
+//! where
+//! __Unpin<'__pin>: ::core::marker::Unpin,
+//! {}
//! // Since we specified `PinnedDrop` as the argument to `#[pin_data]`, we expect `Foo` to
//! // implement `PinnedDrop`. Thus we do not need to prevent `Drop` implementations like
//! // before, instead we implement `Drop` here and delegate to `PinnedDrop`.
@@ -403,7 +412,7 @@
//! #[pinned_drop]
//! impl PinnedDrop for Foo {
//! fn drop(self: Pin<&mut Self>) {
-//! println!("{self:p} is getting dropped.");
+//! pr_info!("{self:p} is getting dropped.");
//! }
//! }
//! ```
@@ -414,7 +423,7 @@
//! // `unsafe`, full path and the token parameter are added, everything else stays the same.
//! unsafe impl ::kernel::init::PinnedDrop for Foo {
//! fn drop(self: Pin<&mut Self>, _: ::kernel::init::__internal::OnlyCallFromDrop) {
-//! println!("{self:p} is getting dropped.");
+//! pr_info!("{self:p} is getting dropped.");
//! }
//! }
//! ```
@@ -449,18 +458,21 @@
//! >(data, move |slot| {
//! {
//! struct __InitOk;
-//! unsafe { ::core::ptr::write(::core::addr_of_mut!((*slot).a), a) };
-//! let a = &unsafe {
+//! {
+//! unsafe { ::core::ptr::write(::core::addr_of_mut!((*slot).a), a) };
+//! }
+//! let a = unsafe {
//! ::kernel::init::__internal::DropGuard::new(::core::addr_of_mut!((*slot).a))
//! };
-//! let b = Bar::new(36);
+//! let init = Bar::new(36);
//! unsafe { data.b(::core::addr_of_mut!((*slot).b), b)? };
-//! let b = &unsafe {
+//! let b = unsafe {
//! ::kernel::init::__internal::DropGuard::new(::core::addr_of_mut!((*slot).b))
//! };
-//!
+//! ::core::mem::forget(b);
+//! ::core::mem::forget(a);
//! #[allow(unreachable_code, clippy::diverging_sub_expression)]
-//! if false {
+//! let _ = || {
//! unsafe {
//! ::core::ptr::write(
//! slot,
@@ -470,13 +482,13 @@
//! },
//! );
//! };
-//! }
-//! unsafe { ::kernel::init::__internal::DropGuard::forget(a) };
-//! unsafe { ::kernel::init::__internal::DropGuard::forget(b) };
+//! };
//! }
//! Ok(__InitOk)
//! });
-//! let init = move |slot| -> ::core::result::Result<(), ::core::convert::Infallible> {
+//! let init = move |
+//! slot,
+//! | -> ::core::result::Result<(), ::core::convert::Infallible> {
//! init(slot).map(|__InitOk| ())
//! };
//! let init = unsafe {
--
2.41.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 12/12] rust: init: update expanded macro explanation
2023-07-19 14:21 ` [PATCH v2 12/12] rust: init: update expanded macro explanation Benno Lossin
@ 2023-07-21 0:24 ` Martin Rodriguez Reboredo
0 siblings, 0 replies; 43+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-07-21 0:24 UTC (permalink / raw)
To: Benno Lossin, Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Alice Ryhl,
Andreas Hindborg, rust-for-linux, linux-kernel
On 7/19/23 11:21, Benno Lossin wrote:
> The previous patches changed the internals of the macros resulting in
> the example expanded code being outdated. This patch updates the example
> and only changes documentation.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 00/12] Quality of life improvements for pin-init
2023-07-19 14:20 [PATCH v2 00/12] Quality of life improvements for pin-init Benno Lossin
` (11 preceding siblings ...)
2023-07-19 14:21 ` [PATCH v2 12/12] rust: init: update expanded macro explanation Benno Lossin
@ 2023-07-19 15:23 ` Benno Lossin
12 siblings, 0 replies; 43+ messages in thread
From: Benno Lossin @ 2023-07-19 15:23 UTC (permalink / raw)
To: Miguel Ojeda, Wedson Almeida Filho, Alex Gaynor
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Alice Ryhl, Andreas Hindborg, rust-for-linux, linux-kernel
On 19.07.23 16:20, Benno Lossin wrote:
> This patch series adds several improvements to the pin-init api:
> - a derive macro for the `Zeroable` trait,
> - makes hygiene of fields in initializers behave like normal struct
> initializers would behave,
> - prevent stackoverflow without optimizations
> - add `..Zeroable::zeroed()` syntax to zero missing fields.
> - support arbitrary paths in initializer macros
>
> This is the second version of this patch series.
> * v1: https://lore.kernel.org/rust-for-linux/20230624092330.157338-1-benno.lossin@proton.me/
>
> Changes not present on modified commits:
> v1 -> v2:
> - implement `Zeroable` for `Opaque`,
> - remove blanket impl of `PinInit` for `Init` and make it a supertrait
> instead,
> - add `{pin_}chain` functions to execute code after initialization,
> - update the example macro expansion
I forgot to mention that this patch series is based on rust-dev [1],
as it depends on Gary's `paste!` macro.
[1]: https://github.com/Rust-for-Linux/linux/tree/rust-dev
Repository: https://github.com/Rust-for-Linux/linux.git rust-dev
--
Cheers,
Benno
^ permalink raw reply [flat|nested] 43+ messages in thread