2024-05-06 09:55:41

by Alice Ryhl

[permalink] [raw]
Subject: [PATCH v2 4/9] rust: list: add macro for implementing ListItem

Adds a macro for safely implementing the ListItem trait. As part of the
implementation of the macro, we also provide a HasListLinks trait
similar to the workqueue's HasWorkItem trait.

The HasListLinks trait is only necessary if you are implementing
ListItem using the impl_list_item macro.

Signed-off-by: Alice Ryhl <[email protected]>
---
rust/kernel/list.rs | 3 ++
rust/kernel/list/impl_list_item_mod.rs | 99 ++++++++++++++++++++++++++++++++++
2 files changed, 102 insertions(+)

diff --git a/rust/kernel/list.rs b/rust/kernel/list.rs
index b5cfbb96a392..f2eca542e090 100644
--- a/rust/kernel/list.rs
+++ b/rust/kernel/list.rs
@@ -8,6 +8,9 @@
use crate::types::Opaque;
use core::ptr;

+mod impl_list_item_mod;
+pub use self::impl_list_item_mod::{impl_has_list_links, impl_list_item, HasListLinks};
+
mod arc;
pub use self::arc::{
impl_list_arc_safe, AtomicListArcTracker, ListArc, ListArcSafe, TryNewListArc,
diff --git a/rust/kernel/list/impl_list_item_mod.rs b/rust/kernel/list/impl_list_item_mod.rs
new file mode 100644
index 000000000000..3ff483be89d1
--- /dev/null
+++ b/rust/kernel/list/impl_list_item_mod.rs
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! Helpers for implementing list traits safely.
+
+use crate::list::ListLinks;
+
+/// Declares that this type has a `ListLinks<ID>` field at a fixed offset.
+///
+/// This trait is only used to help implement `ListItem` safely. If `ListItem` is implemented
+/// manually, then this trait is not needed.
+///
+/// # Safety
+///
+/// All values of this type must have a `ListLinks<ID>` field at the given offset.
+pub unsafe trait HasListLinks<const ID: u64 = 0> {
+ /// The offset of the `ListLinks` field.
+ const OFFSET: usize;
+
+ /// Returns a pointer to the [`ListLinks<T, ID>`] field.
+ ///
+ /// # Safety
+ ///
+ /// The provided pointer must point at a valid struct of type `Self`.
+ ///
+ /// [`ListLinks<T, ID>`]: ListLinks
+ // We don't really need this method, but it's necessary for the implementation of
+ // `impl_has_work!` to be correct.
+ #[inline]
+ unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut ListLinks<ID> {
+ // SAFETY: The caller promises that the pointer is valid. The implementer promises that the
+ // `OFFSET` constant is correct.
+ unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut ListLinks<ID> }
+ }
+}
+
+/// Implements the [`HasListLinks`] trait for the given type.
+#[macro_export]
+macro_rules! impl_has_list_links {
+ ($(impl$(<$($implarg:ident),*>)?
+ HasListLinks$(<$id:tt>)?
+ for $self:ident $(<$($selfarg:ty),*>)?
+ { self$(.$field:ident)* }
+ )*) => {$(
+ // SAFETY: The implementation of `raw_get_list_links` only compiles if the field has the
+ // right type.
+ unsafe impl$(<$($implarg),*>)? $crate::list::HasListLinks$(<$id>)? for
+ $self $(<$($selfarg),*>)?
+ {
+ const OFFSET: usize = ::core::mem::offset_of!(Self, $($field).*) as usize;
+
+ #[inline]
+ unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut $crate::list::ListLinks$(<$id>)? {
+ // SAFETY: The caller promises that the pointer is not dangling.
+ unsafe {
+ ::core::ptr::addr_of_mut!((*ptr)$(.$field)*)
+ }
+ }
+ }
+ )*};
+}
+pub use impl_has_list_links;
+
+/// Implements the [`ListItem`] trait for the given type.
+///
+/// Assumes that the type implements [`HasListLinks`].
+///
+/// [`ListItem`]: crate::list::ListItem
+#[macro_export]
+macro_rules! impl_list_item {
+ (
+ impl$({$($generics:tt)*})? $crate::list::ListItem<$num:tt> for $t:ty {
+ using ListLinks;
+ } $($rest:tt)*
+ ) => {
+ unsafe impl$(<$($generics)*>)? $crate::list::ListItem<$num> for $t {
+ unsafe fn view_links(me: *const Self) -> *mut $crate::list::ListLinks<$num> {
+ unsafe {
+ <Self as $crate::list::HasListLinks<$num>>::raw_get_list_links(me.cast_mut())
+ }
+ }
+
+ unsafe fn view_value(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
+ let offset = <Self as $crate::list::HasListLinks<$num>>::OFFSET;
+ unsafe { (me as *const u8).sub(offset) as *const Self }
+ }
+
+ unsafe fn prepare_to_insert(me: *const Self) -> *mut $crate::list::ListLinks<$num> {
+ unsafe { <Self as $crate::list::ListItem<$num>>::view_links(me) }
+ }
+
+ unsafe fn post_remove(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
+ unsafe { <Self as $crate::list::ListItem<$num>>::view_value(me) }
+ }
+ }
+ };
+}
+pub use impl_list_item;

--
2.45.0.rc1.225.g2a3ae87e7f-goog



2024-05-27 10:17:48

by Benno Lossin

[permalink] [raw]
Subject: Re: [PATCH v2 4/9] rust: list: add macro for implementing ListItem

On 06.05.24 11:53, Alice Ryhl wrote:
> Adds a macro for safely implementing the ListItem trait. As part of the
> implementation of the macro, we also provide a HasListLinks trait
> similar to the workqueue's HasWorkItem trait.
>
> The HasListLinks trait is only necessary if you are implementing
> ListItem using the impl_list_item macro.
>
> Signed-off-by: Alice Ryhl <[email protected]>
> ---
> rust/kernel/list.rs | 3 ++
> rust/kernel/list/impl_list_item_mod.rs | 99 ++++++++++++++++++++++++++++++++++
> 2 files changed, 102 insertions(+)
>
> diff --git a/rust/kernel/list.rs b/rust/kernel/list.rs
> index b5cfbb96a392..f2eca542e090 100644
> --- a/rust/kernel/list.rs
> +++ b/rust/kernel/list.rs
> @@ -8,6 +8,9 @@
> use crate::types::Opaque;
> use core::ptr;
>
> +mod impl_list_item_mod;
> +pub use self::impl_list_item_mod::{impl_has_list_links, impl_list_item, HasListLinks};
> +
> mod arc;
> pub use self::arc::{
> impl_list_arc_safe, AtomicListArcTracker, ListArc, ListArcSafe, TryNewListArc,
> diff --git a/rust/kernel/list/impl_list_item_mod.rs b/rust/kernel/list/impl_list_item_mod.rs
> new file mode 100644
> index 000000000000..3ff483be89d1
> --- /dev/null
> +++ b/rust/kernel/list/impl_list_item_mod.rs
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! Helpers for implementing list traits safely.
> +
> +use crate::list::ListLinks;
> +
> +/// Declares that this type has a `ListLinks<ID>` field at a fixed offset.
> +///
> +/// This trait is only used to help implement `ListItem` safely. If `ListItem` is implemented
> +/// manually, then this trait is not needed.
> +///
> +/// # Safety
> +///
> +/// All values of this type must have a `ListLinks<ID>` field at the given offset.
> +pub unsafe trait HasListLinks<const ID: u64 = 0> {
> + /// The offset of the `ListLinks` field.
> + const OFFSET: usize;
> +
> + /// Returns a pointer to the [`ListLinks<T, ID>`] field.
> + ///
> + /// # Safety
> + ///
> + /// The provided pointer must point at a valid struct of type `Self`.
> + ///
> + /// [`ListLinks<T, ID>`]: ListLinks
> + // We don't really need this method, but it's necessary for the implementation of
> + // `impl_has_work!` to be correct.

You could also check for correctness within the expansion of
`impl_has_list_links` (by creating an invisible function there), so you
don't need this function. It still could be useful for `dyn Trait`
though.

> + #[inline]
> + unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut ListLinks<ID> {
> + // SAFETY: The caller promises that the pointer is valid. The implementer promises that the
> + // `OFFSET` constant is correct.
> + unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut ListLinks<ID> }
> + }
> +}
> +
> +/// Implements the [`HasListLinks`] trait for the given type.
> +#[macro_export]
> +macro_rules! impl_has_list_links {
> + ($(impl$(<$($implarg:ident),*>)?
> + HasListLinks$(<$id:tt>)?
> + for $self:ident $(<$($selfarg:ty),*>)?
> + { self$(.$field:ident)* }
> + )*) => {$(
> + // SAFETY: The implementation of `raw_get_list_links` only compiles if the field has the
> + // right type.
> + unsafe impl$(<$($implarg),*>)? $crate::list::HasListLinks$(<$id>)? for
> + $self $(<$($selfarg),*>)?
> + {
> + const OFFSET: usize = ::core::mem::offset_of!(Self, $($field).*) as usize;

AFAIK only a single field is supported on stable.

> +
> + #[inline]
> + unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut $crate::list::ListLinks$(<$id>)? {
> + // SAFETY: The caller promises that the pointer is not dangling.
> + unsafe {
> + ::core::ptr::addr_of_mut!((*ptr)$(.$field)*)

Note that this runs into the `Box` issue, but `offset_of` will fail to
compile in that case, so it is fine.
Do we want to note this in a comment?

> + }
> + }
> + }
> + )*};
> +}
> +pub use impl_has_list_links;
> +
> +/// Implements the [`ListItem`] trait for the given type.
> +///
> +/// Assumes that the type implements [`HasListLinks`].
> +///
> +/// [`ListItem`]: crate::list::ListItem
> +#[macro_export]
> +macro_rules! impl_list_item {
> + (
> + impl$({$($generics:tt)*})? $crate::list::ListItem<$num:tt> for $t:ty {
> + using ListLinks;
> + } $($rest:tt)*
> + ) => {
> + unsafe impl$(<$($generics)*>)? $crate::list::ListItem<$num> for $t {

Missing SAFETY comment.

---
Cheers,
Benno

> + unsafe fn view_links(me: *const Self) -> *mut $crate::list::ListLinks<$num> {
> + unsafe {
> + <Self as $crate::list::HasListLinks<$num>>::raw_get_list_links(me.cast_mut())
> + }
> + }
> +
> + unsafe fn view_value(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
> + let offset = <Self as $crate::list::HasListLinks<$num>>::OFFSET;
> + unsafe { (me as *const u8).sub(offset) as *const Self }
> + }
> +
> + unsafe fn prepare_to_insert(me: *const Self) -> *mut $crate::list::ListLinks<$num> {
> + unsafe { <Self as $crate::list::ListItem<$num>>::view_links(me) }
> + }
> +
> + unsafe fn post_remove(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
> + unsafe { <Self as $crate::list::ListItem<$num>>::view_value(me) }
> + }
> + }
> + };
> +}
> +pub use impl_list_item;
>
> --
> 2.45.0.rc1.225.g2a3ae87e7f-goog
>