[PATCH v3] rust: check type of `$ptr` in `container_of!`

[PATCH v3] rust: check type of `$ptr` in `container_of!`

[Tamir Duberstein]

Add a compile-time check that `*$ptr` is of the type of `$type->$($f)*`.
Rename those placeholders for clarity.

Given the incorrect usage:

> diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs
> index 8d978c896747..6a7089149878 100644
> --- a/rust/kernel/rbtree.rs
> +++ b/rust/kernel/rbtree.rs
> @@ -329,7 +329,7 @@ fn raw_entry(&mut self, key: &K) -> RawEntry<'_, K, V> {
>          while !(*child_field_of_parent).is_null() {
>              let curr = *child_field_of_parent;
>              // SAFETY: All links fields we create are in a `Node<K, V>`.
> -            let node = unsafe { container_of!(curr, Node<K, V>, links) };
> +            let node = unsafe { container_of!(curr, Node<K, V>, key) };
>
>              // SAFETY: `node` is a non-null node so it is valid by the type invariants.
>              match key.cmp(unsafe { &(*node).key }) {

this patch produces the compilation error:

> error[E0308]: mismatched types
>    --> rust/kernel/lib.rs:207:25
>     |
> 207 |             [field_ptr, container_field_ptr]; // typeof(`$field_ptr`) == typeof(`$Container.$($fields)*`)
>     |                         ^^^^^^^^^^^^^^^^^^^ expected `*mut rb_node`, found `*mut K`
>     |
>    ::: rust/kernel/rbtree.rs:270:6
>     |
> 270 | impl<K, V> RBTree<K, V>
>     |      - found this type parameter
> ...
> 332 |             let node = unsafe { container_of!(curr, Node<K, V>, key) };
>     |                                 ------------------------------------ in this macro invocation
>     |
>     = note: expected raw pointer `*mut bindings::rb_node`
>                found raw pointer `*mut K`
>     = note: this error originates in the macro `container_of` (in Nightly builds, run with -Z macro-backtrace for more info)
>
> error: aborting due to 1 previous error

Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Suggested-by: Alice Ryhl <aliceryhl@google.com>
Link: https://lore.kernel.org/all/CAH5fLgh6gmqGBhPMi2SKn7mCmMWfOSiS0WP5wBuGPYh9ZTAiww@mail.gmail.com/
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
I also considered an implementation using a function, but the resulting
compilation error was not as concise:

> error[E0308]: mismatched types
>    --> rust/kernel/lib.rs:207:42
>     |
> 207 |             assert_same_type($field_ptr, field_ptr);
>     |             ----------------             ^^^^^^^^^ expected `*const rb_node`, found `*mut K`
>     |             |
>     |             arguments to this function are incorrect
>     |
>    ::: rust/kernel/rbtree.rs:270:6
>     |
> 270 | impl<K, V> RBTree<K, V>
>     |      - found this type parameter
> ...
> 332 |             let node = unsafe { container_of!(curr, Node<K, V>, key) };
>     |                                 ------------------------------------ in this macro invocation
>     |
>     = note: expected raw pointer `*const bindings::rb_node`
>                found raw pointer `*mut K`
> note: function defined here
>    --> rust/kernel/lib.rs:205:16
>     |
> 205 |             fn assert_same_type<T>(_: *const T, _: *const T) {}
>     |                ^^^^^^^^^^^^^^^^                 -----------
>     |
>    ::: rust/kernel/rbtree.rs:332:33
>     |
> 332 |             let node = unsafe { container_of!(curr, Node<K, V>, key) };
>     |                                 ------------------------------------ in this macro invocation
>     = note: this error originates in the macro `container_of` (in Nightly builds, run with -Z macro-backtrace for more info)
> 
> error: aborting due to 1 previous error
---
Changes in v3:
- Fix comment typo.
- s/^;/   / in commit message and cover letter. (Miguel Ojeda)
- Evaluate $ptr only once. (Alice Ryhl)
- Link to v2: https://lore.kernel.org/r/20250412-b4-container-of-type-check-v2-1-f3cc9934c160@gmail.com

Changes in v2:
- Wrap in `if false` to improve unoptimized codegen. (Alice Ryhl)
- Shrink implementation using an array literal instead of a function.
- Link to v1: https://lore.kernel.org/r/20250411-b4-container-of-type-check-v1-1-08262ef67c95@gmail.com
---
 rust/kernel/lib.rs | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 1df11156302a..d14ed86efb68 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -198,9 +198,15 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
 /// ```
 #[macro_export]
 macro_rules! container_of {
-    ($ptr:expr, $type:ty, $($f:tt)*) => {{
-        let offset: usize = ::core::mem::offset_of!($type, $($f)*);
-        $ptr.byte_sub(offset).cast::<$type>()
+    ($field_ptr:expr, $Container:ty, $($fields:tt)*) => {{
+        let offset: usize = ::core::mem::offset_of!($Container, $($fields)*);
+        let field_ptr = $field_ptr;
+        let container_ptr = field_ptr.byte_sub(offset).cast::<$Container>();
+        if false {
+            let container_field_ptr = ::core::ptr::addr_of!((*container_ptr).$($fields)*).cast_mut();
+            [field_ptr, container_field_ptr]; // typeof(`$field_ptr`) == typeof(`$Container.$($fields)*`)
+        }
+        container_ptr
     }}
 }
 

---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250411-b4-container-of-type-check-06af1c204f59
prerequisite-change-id: 20250409-container-of-mutness-b153dab4388d:v1
prerequisite-patch-id: 53d5889db599267f87642bb0ae3063c29bc24863

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

Re: [PATCH v3] rust: check type of `$ptr` in `container_of!`

[Alice Ryhl]

On Wed, Apr 23, 2025 at 01:40:10PM -0400, Tamir Duberstein wrote:
> Add a compile-time check that `*$ptr` is of the type of `$type->$($f)*`.
> Rename those placeholders for clarity.
> 
> Given the incorrect usage:
> 
> > diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs
> > index 8d978c896747..6a7089149878 100644
> > --- a/rust/kernel/rbtree.rs
> > +++ b/rust/kernel/rbtree.rs
> > @@ -329,7 +329,7 @@ fn raw_entry(&mut self, key: &K) -> RawEntry<'_, K, V> {
> >          while !(*child_field_of_parent).is_null() {
> >              let curr = *child_field_of_parent;
> >              // SAFETY: All links fields we create are in a `Node<K, V>`.
> > -            let node = unsafe { container_of!(curr, Node<K, V>, links) };
> > +            let node = unsafe { container_of!(curr, Node<K, V>, key) };
> >
> >              // SAFETY: `node` is a non-null node so it is valid by the type invariants.
> >              match key.cmp(unsafe { &(*node).key }) {
> 
> this patch produces the compilation error:
> 
> > error[E0308]: mismatched types
> >    --> rust/kernel/lib.rs:207:25
> >     |
> > 207 |             [field_ptr, container_field_ptr]; // typeof(`$field_ptr`) == typeof(`$Container.$($fields)*`)
> >     |                         ^^^^^^^^^^^^^^^^^^^ expected `*mut rb_node`, found `*mut K`
> >     |
> >    ::: rust/kernel/rbtree.rs:270:6
> >     |
> > 270 | impl<K, V> RBTree<K, V>
> >     |      - found this type parameter
> > ...
> > 332 |             let node = unsafe { container_of!(curr, Node<K, V>, key) };
> >     |                                 ------------------------------------ in this macro invocation
> >     |
> >     = note: expected raw pointer `*mut bindings::rb_node`
> >                found raw pointer `*mut K`
> >     = note: this error originates in the macro `container_of` (in Nightly builds, run with -Z macro-backtrace for more info)
> >
> > error: aborting due to 1 previous error
> 
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Link: https://lore.kernel.org/all/CAH5fLgh6gmqGBhPMi2SKn7mCmMWfOSiS0WP5wBuGPYh9ZTAiww@mail.gmail.com/
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>

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

>  rust/kernel/lib.rs | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 1df11156302a..d14ed86efb68 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -198,9 +198,15 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
>  /// ```
>  #[macro_export]
>  macro_rules! container_of {
> -    ($ptr:expr, $type:ty, $($f:tt)*) => {{
> -        let offset: usize = ::core::mem::offset_of!($type, $($f)*);
> -        $ptr.byte_sub(offset).cast::<$type>()
> +    ($field_ptr:expr, $Container:ty, $($fields:tt)*) => {{

It's rather unusual to use an uppercase C in the name of this parameter.

Alice

> +        let offset: usize = ::core::mem::offset_of!($Container, $($fields)*);
> +        let field_ptr = $field_ptr;
> +        let container_ptr = field_ptr.byte_sub(offset).cast::<$Container>();
> +        if false {
> +            let container_field_ptr = ::core::ptr::addr_of!((*container_ptr).$($fields)*).cast_mut();
> +            [field_ptr, container_field_ptr]; // typeof(`$field_ptr`) == typeof(`$Container.$($fields)*`)
> +        }
> +        container_ptr
>      }}
>  }
>  
> 
> ---
> base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
> change-id: 20250411-b4-container-of-type-check-06af1c204f59
> prerequisite-change-id: 20250409-container-of-mutness-b153dab4388d:v1
> prerequisite-patch-id: 53d5889db599267f87642bb0ae3063c29bc24863
> 
> Best regards,
> -- 
> Tamir Duberstein <tamird@gmail.com>
> 

Re: [PATCH v3] rust: check type of `$ptr` in `container_of!`

[Tamir Duberstein]

On Thu, Apr 24, 2025 at 7:53 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Wed, Apr 23, 2025 at 01:40:10PM -0400, Tamir Duberstein wrote:
> > Add a compile-time check that `*$ptr` is of the type of `$type->$($f)*`.
> > Rename those placeholders for clarity.
> >
> > Given the incorrect usage:
> >
> > > diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs
> > > index 8d978c896747..6a7089149878 100644
> > > --- a/rust/kernel/rbtree.rs
> > > +++ b/rust/kernel/rbtree.rs
> > > @@ -329,7 +329,7 @@ fn raw_entry(&mut self, key: &K) -> RawEntry<'_, K, V> {
> > >          while !(*child_field_of_parent).is_null() {
> > >              let curr = *child_field_of_parent;
> > >              // SAFETY: All links fields we create are in a `Node<K, V>`.
> > > -            let node = unsafe { container_of!(curr, Node<K, V>, links) };
> > > +            let node = unsafe { container_of!(curr, Node<K, V>, key) };
> > >
> > >              // SAFETY: `node` is a non-null node so it is valid by the type invariants.
> > >              match key.cmp(unsafe { &(*node).key }) {
> >
> > this patch produces the compilation error:
> >
> > > error[E0308]: mismatched types
> > >    --> rust/kernel/lib.rs:207:25
> > >     |
> > > 207 |             [field_ptr, container_field_ptr]; // typeof(`$field_ptr`) == typeof(`$Container.$($fields)*`)
> > >     |                         ^^^^^^^^^^^^^^^^^^^ expected `*mut rb_node`, found `*mut K`
> > >     |
> > >    ::: rust/kernel/rbtree.rs:270:6
> > >     |
> > > 270 | impl<K, V> RBTree<K, V>
> > >     |      - found this type parameter
> > > ...
> > > 332 |             let node = unsafe { container_of!(curr, Node<K, V>, key) };
> > >     |                                 ------------------------------------ in this macro invocation
> > >     |
> > >     = note: expected raw pointer `*mut bindings::rb_node`
> > >                found raw pointer `*mut K`
> > >     = note: this error originates in the macro `container_of` (in Nightly builds, run with -Z macro-backtrace for more info)
> > >
> > > error: aborting due to 1 previous error
> >
> > Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> > Suggested-by: Alice Ryhl <aliceryhl@google.com>
> > Link: https://lore.kernel.org/all/CAH5fLgh6gmqGBhPMi2SKn7mCmMWfOSiS0WP5wBuGPYh9ZTAiww@mail.gmail.com/
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>
> >  rust/kernel/lib.rs | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 1df11156302a..d14ed86efb68 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -198,9 +198,15 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
> >  /// ```
> >  #[macro_export]
> >  macro_rules! container_of {
> > -    ($ptr:expr, $type:ty, $($f:tt)*) => {{
> > -        let offset: usize = ::core::mem::offset_of!($type, $($f)*);
> > -        $ptr.byte_sub(offset).cast::<$type>()
> > +    ($field_ptr:expr, $Container:ty, $($fields:tt)*) => {{
>
> It's rather unusual to use an uppercase C in the name of this parameter.

I took the parameter name from `offset_of`:
https://doc.rust-lang.org/stable/std/mem/macro.offset_of.html.

Re: [PATCH v3] rust: check type of `$ptr` in `container_of!`

[Alice Ryhl]

On Thu, Apr 24, 2025 at 09:47:52AM -0400, Tamir Duberstein wrote:
> > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > > index 1df11156302a..d14ed86efb68 100644
> > > --- a/rust/kernel/lib.rs
> > > +++ b/rust/kernel/lib.rs
> > > @@ -198,9 +198,15 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
> > >  /// ```
> > >  #[macro_export]
> > >  macro_rules! container_of {
> > > -    ($ptr:expr, $type:ty, $($f:tt)*) => {{
> > > -        let offset: usize = ::core::mem::offset_of!($type, $($f)*);
> > > -        $ptr.byte_sub(offset).cast::<$type>()
> > > +    ($field_ptr:expr, $Container:ty, $($fields:tt)*) => {{
> >
> > It's rather unusual to use an uppercase C in the name of this parameter.
> 
> I took the parameter name from `offset_of`:
> https://doc.rust-lang.org/stable/std/mem/macro.offset_of.html.

https://github.com/rust-lang/rust/pull/140285

Re: [PATCH v3] rust: check type of `$ptr` in `container_of!`

[Miguel Ojeda]

On Wed, Apr 23, 2025 at 7:40 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> +        if false {
> +            let container_field_ptr = ::core::ptr::addr_of!((*container_ptr).$($fields)*).cast_mut();
> +            [field_ptr, container_field_ptr]; // typeof(`$field_ptr`) == typeof(`$Container.$($fields)*`)

If I understand correctly, we keep the `// typeof ...` in the same
line so that it appears in the error message and thus it is clearer to
the user, right?

In that case, could we nevertheless please clarify things a bit at the
top of the `if false` block, i.e. something like:

    // Ensure that both types are equal while avoiding codegen .....
    // ... i.e. effectively compare `typeof(...) == ...`.
    if false {
        ...

etc.?

Or, perhaps even better, we move this into its own macro, so that we
document it there and why we chose this particular approach, assuming
the error message still prints the right thing.

Speaking of magic, to be honest, is this approach worth it? I liked v1
quite more. The error seems concise enough, and the first line that
the compiler points out is `assert_same_type` which makes it super
clear, and showed the actual expressions involved without using a
comment.

With v1, we could also just put `assert_same_type` outside as a
utility for others to use, i.e. in the `kernel` crate, which
simplifies things and makes the error a bit shorter. Moving the
function out makes the error slightly shorter, would also allow us to
document its usage, including the suggestion to use `if false` in an
example.

Regarding the `if false`, the kernel is always built with at least
-O2. Benno mentioned debug performance -- was that related to
something like debug assertions being enabled or just optimization
level? Either way, even with the assertions enabled, I don't see it in
codegen.

Am I missing something?

Thanks!

Cheers,
Miguel

Re: [PATCH v3] rust: check type of `$ptr` in `container_of!`

[Tamir Duberstein]

On Thu, Apr 24, 2025 at 8:48 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Wed, Apr 23, 2025 at 7:40 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > +        if false {
> > +            let container_field_ptr = ::core::ptr::addr_of!((*container_ptr).$($fields)*).cast_mut();
> > +            [field_ptr, container_field_ptr]; // typeof(`$field_ptr`) == typeof(`$Container.$($fields)*`)
>
> If I understand correctly, we keep the `// typeof ...` in the same
> line so that it appears in the error message and thus it is clearer to
> the user, right?
>
> In that case, could we nevertheless please clarify things a bit at the
> top of the `if false` block, i.e. something like:
>
>     // Ensure that both types are equal while avoiding codegen .....
>     // ... i.e. effectively compare `typeof(...) == ...`.
>     if false {
>         ...
>
> etc.?
>
> Or, perhaps even better, we move this into its own macro, so that we
> document it there and why we chose this particular approach, assuming
> the error message still prints the right thing.
>
> Speaking of magic, to be honest, is this approach worth it? I liked v1
> quite more. The error seems concise enough, and the first line that
> the compiler points out is `assert_same_type` which makes it super
> clear, and showed the actual expressions involved without using a
> comment.
>
> With v1, we could also just put `assert_same_type` outside as a
> utility for others to use, i.e. in the `kernel` crate, which
> simplifies things and makes the error a bit shorter. Moving the
> function out makes the error slightly shorter, would also allow us to
> document its usage, including the suggestion to use `if false` in an
> example.
>
> Regarding the `if false`, the kernel is always built with at least
> -O2. Benno mentioned debug performance -- was that related to
> something like debug assertions being enabled or just optimization
> level? Either way, even with the assertions enabled, I don't see it in
> codegen.
>
> Am I missing something?

I have no strong opinions here, I'm just trying to keep everyone
happy. I'm happy to go back to v1 with or without `assert_same_type`
moved out of the macro body.

Re: [PATCH v3] rust: check type of `$ptr` in `container_of!`

[John Hubbard]

On 4/23/25 10:40 AM, Tamir Duberstein wrote:
...
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 1df11156302a..d14ed86efb68 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -198,9 +198,15 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
>   /// ```
>   #[macro_export]
>   macro_rules! container_of {
> -    ($ptr:expr, $type:ty, $($f:tt)*) => {{
> -        let offset: usize = ::core::mem::offset_of!($type, $($f)*);
> -        $ptr.byte_sub(offset).cast::<$type>()
> +    ($field_ptr:expr, $Container:ty, $($fields:tt)*) => {{
> +        let offset: usize = ::core::mem::offset_of!($Container, $($fields)*);
> +        let field_ptr = $field_ptr;
> +        let container_ptr = field_ptr.byte_sub(offset).cast::<$Container>();
> +        if false {

This jumped out at me. It's something that I'd like to recommend NOT
doing, here or anywhere else, because:

     a) Anything of the form "if false" will get removed by any compiler
        worthy of the name, especially in kernel builds.

     b) It is a "magic trick", in that the code is on the face of it,
     unnecessary. So that's not something that you would pick as your
     first choice anyway. But as I see now that Miguel has also pointed
     out, the -O2 optimization level that we build at makes it either
     unreliable or broken, so it's Bad Magic. :)

Anyway, I don't know where this pattern came from, but it's not a good
one for kernel builds.


thanks,
-- 
John Hubbard

Re: [PATCH v3] rust: check type of `$ptr` in `container_of!`

[Alice Ryhl]

On Sun, Apr 27, 2025 at 03:59:48PM -0700, John Hubbard wrote:
> On 4/23/25 10:40 AM, Tamir Duberstein wrote:
> ...
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 1df11156302a..d14ed86efb68 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -198,9 +198,15 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
> >   /// ```
> >   #[macro_export]
> >   macro_rules! container_of {
> > -    ($ptr:expr, $type:ty, $($f:tt)*) => {{
> > -        let offset: usize = ::core::mem::offset_of!($type, $($f)*);
> > -        $ptr.byte_sub(offset).cast::<$type>()
> > +    ($field_ptr:expr, $Container:ty, $($fields:tt)*) => {{
> > +        let offset: usize = ::core::mem::offset_of!($Container, $($fields)*);
> > +        let field_ptr = $field_ptr;
> > +        let container_ptr = field_ptr.byte_sub(offset).cast::<$Container>();
> > +        if false {
> 
> This jumped out at me. It's something that I'd like to recommend NOT
> doing, here or anywhere else, because:
> 
>     a) Anything of the form "if false" will get removed by any compiler
>        worthy of the name, especially in kernel builds.

The `if false` branch is used to trigger a compilation failure when the
macro is used incorrectly. The intent is that the compiler should
optimize it out. I don't think there's anything wrong with that pattern.

Alice

Re: [PATCH v3] rust: check type of `$ptr` in `container_of!`

[John Hubbard]

On 4/28/25 2:40 AM, Alice Ryhl wrote:
> On Sun, Apr 27, 2025 at 03:59:48PM -0700, John Hubbard wrote:
>> On 4/23/25 10:40 AM, Tamir Duberstein wrote:
>> ...
>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>>> index 1df11156302a..d14ed86efb68 100644
>>> --- a/rust/kernel/lib.rs
>>> +++ b/rust/kernel/lib.rs
>>> @@ -198,9 +198,15 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
>>>   /// ```
>>>   #[macro_export]
>>>   macro_rules! container_of {
>>> -    ($ptr:expr, $type:ty, $($f:tt)*) => {{
>>> -        let offset: usize = ::core::mem::offset_of!($type, $($f)*);
>>> -        $ptr.byte_sub(offset).cast::<$type>()
>>> +    ($field_ptr:expr, $Container:ty, $($fields:tt)*) => {{
>>> +        let offset: usize = ::core::mem::offset_of!($Container, $($fields)*);
>>> +        let field_ptr = $field_ptr;
>>> +        let container_ptr = field_ptr.byte_sub(offset).cast::<$Container>();
>>> +        if false {
>>
>> This jumped out at me. It's something that I'd like to recommend NOT
>> doing, here or anywhere else, because:
>>
>>     a) Anything of the form "if false" will get removed by any compiler
>>        worthy of the name, especially in kernel builds.
> 
> The `if false` branch is used to trigger a compilation failure when the
> macro is used incorrectly. The intent is that the compiler should
> optimize it out. I don't think there's anything wrong with that pattern.

OK...probably best to either encapsulate that, or at least comment
it. I'm accustomed to seeing that pattern in cases where people
expected the code to *not* get optimized out, so it triggers me. :)

thanks,
-- 
John Hubbard

Re: [PATCH v3] rust: check type of `$ptr` in `container_of!`

[Alice Ryhl]

On Mon, Apr 28, 2025 at 12:54:19PM -0700, John Hubbard wrote:
> On 4/28/25 2:40 AM, Alice Ryhl wrote:
> > On Sun, Apr 27, 2025 at 03:59:48PM -0700, John Hubbard wrote:
> >> On 4/23/25 10:40 AM, Tamir Duberstein wrote:
> >> ...
> >>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> >>> index 1df11156302a..d14ed86efb68 100644
> >>> --- a/rust/kernel/lib.rs
> >>> +++ b/rust/kernel/lib.rs
> >>> @@ -198,9 +198,15 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
> >>>   /// ```
> >>>   #[macro_export]
> >>>   macro_rules! container_of {
> >>> -    ($ptr:expr, $type:ty, $($f:tt)*) => {{
> >>> -        let offset: usize = ::core::mem::offset_of!($type, $($f)*);
> >>> -        $ptr.byte_sub(offset).cast::<$type>()
> >>> +    ($field_ptr:expr, $Container:ty, $($fields:tt)*) => {{
> >>> +        let offset: usize = ::core::mem::offset_of!($Container, $($fields)*);
> >>> +        let field_ptr = $field_ptr;
> >>> +        let container_ptr = field_ptr.byte_sub(offset).cast::<$Container>();
> >>> +        if false {
> >>
> >> This jumped out at me. It's something that I'd like to recommend NOT
> >> doing, here or anywhere else, because:
> >>
> >>     a) Anything of the form "if false" will get removed by any compiler
> >>        worthy of the name, especially in kernel builds.
> > 
> > The `if false` branch is used to trigger a compilation failure when the
> > macro is used incorrectly. The intent is that the compiler should
> > optimize it out. I don't think there's anything wrong with that pattern.
> 
> OK...probably best to either encapsulate that, or at least comment
> it. I'm accustomed to seeing that pattern in cases where people
> expected the code to *not* get optimized out, so it triggers me. :)

Okay ... why exactly would people do that? I can't imagine what purpose
that would serve.

Alice

Re: [PATCH v3] rust: check type of `$ptr` in `container_of!`

[John Hubbard]

On 4/29/25 1:20 AM, Alice Ryhl wrote:
> On Mon, Apr 28, 2025 at 12:54:19PM -0700, John Hubbard wrote:
...
>>> The `if false` branch is used to trigger a compilation failure when the
>>> macro is used incorrectly. The intent is that the compiler should
>>> optimize it out. I don't think there's anything wrong with that pattern.
>>
>> OK...probably best to either encapsulate that, or at least comment
>> it. I'm accustomed to seeing that pattern in cases where people
>> expected the code to *not* get optimized out, so it triggers me. :)
> 
> Okay ... why exactly would people do that? I can't imagine what purpose
> that would serve.
> 

lol I know, right? Two things, neither of which gives me joy to recall:

a) Configuration games, in which the dead code is available as an
   option that is not configured right now.

b) Binary patching games, sort of the same as above.

And at a slightly higher level, my concern is that when one reads
code like "if false", it sets off "omg, someone is either confused
or doing something questionable...actually, why choose? Probably
both." :)


thanks,
-- 
John Hubbard