[PATCH] rust: print: Fix issue with rust_build_error

[PATCH] rust: print: Fix issue with rust_build_error

[Joel Fernandes]

When printing just before calling io.write32(), modpost fails due to
build_assert's missing rust_build_error symbol. The issue is that, the
printk arguments are passed as reference in bindings code, thus Rust
cannot trust its value and fails to optimize away the build_assert.

The issue can be reproduced with the following simple snippet:
  let offset = 0;
  pr_err!("{}", offset);
  io.write32(base, offset);

Fix it by just using a closure to call printk. Rust captures the
arguments into the closure's arguments thus breaking the dependency.
This can be fixed by simply creating a variable alias for each variable
however the closure is a simple and concise fix.

Another approach with using const-generics for the io.write32 API was
investigated, but it cannot work with code that dynamically calculates
the write offset.

Disassembly of users of pr_err!() with/without patch shows identical
code generation, thus the fix has no difference in the final binary.

Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
 rust/kernel/print.rs | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
index 2d743d78d220..5847942195a7 100644
--- a/rust/kernel/print.rs
+++ b/rust/kernel/print.rs
@@ -143,34 +143,38 @@ pub fn call_printk_cont(args: fmt::Arguments<'_>) {
 #[expect(clippy::crate_in_macro_def)]
 macro_rules! print_macro (
     // The non-continuation cases (most of them, e.g. `INFO`).
-    ($format_string:path, false, $($arg:tt)+) => (
+    ($format_string:path, false, $($arg:tt)+) => ({
         // To remain sound, `arg`s must be expanded outside the `unsafe` block.
         // Typically one would use a `let` binding for that; however, `format_args!`
         // takes borrows on the arguments, but does not extend the scope of temporaries.
         // Therefore, a `match` expression is used to keep them around, since
         // the scrutinee is kept until the end of the `match`.
-        match $crate::prelude::fmt!($($arg)+) {
-            // SAFETY: This hidden macro should only be called by the documented
-            // printing macros which ensure the format string is one of the fixed
-            // ones. All `__LOG_PREFIX`s are null-terminated as they are generated
-            // by the `module!` proc macro or fixed values defined in a kernel
-            // crate.
-            args => unsafe {
-                $crate::print::call_printk(
-                    &$format_string,
-                    crate::__LOG_PREFIX,
-                    args,
-                );
+        (|| {
+            match $crate::prelude::fmt!($($arg)+) {
+                // SAFETY: This hidden macro should only be called by the documented
+                // printing macros which ensure the format string is one of the fixed
+                // ones. All `__LOG_PREFIX`s are null-terminated as they are generated
+                // by the `module!` proc macro or fixed values defined in a kernel
+                // crate.
+                args => unsafe {
+                    $crate::print::call_printk(
+                        &$format_string,
+                        crate::__LOG_PREFIX,
+                        args,
+                    );
+                }
             }
-        }
-    );
+        })();
+    });
 
     // The `CONT` case.
-    ($format_string:path, true, $($arg:tt)+) => (
-        $crate::print::call_printk_cont(
-            $crate::prelude::fmt!($($arg)+),
-        );
-    );
+    ($format_string:path, true, $($arg:tt)+) => ({
+        (|| {
+            $crate::print::call_printk_cont(
+                $crate::prelude::fmt!($($arg)+),
+            );
+        })();
+    });
 );
 
 /// Stub for doctests
-- 
2.34.1

Re: [PATCH] rust: print: Fix issue with rust_build_error

[Boqun Feng]

On Sat, Sep 20, 2025 at 12:19:58PM -0400, Joel Fernandes wrote:
> When printing just before calling io.write32(), modpost fails due to
> build_assert's missing rust_build_error symbol. The issue is that, the
> printk arguments are passed as reference in bindings code, thus Rust
> cannot trust its value and fails to optimize away the build_assert.
> 

I think "cannot trust" is a bit vague and misleading here, for this
kind of "workaround", we want the reason to be a bit clear. @Gary, could
you help explain it a bit more?

> The issue can be reproduced with the following simple snippet:
>   let offset = 0;
>   pr_err!("{}", offset);
>   io.write32(base, offset);
> 
> Fix it by just using a closure to call printk. Rust captures the
> arguments into the closure's arguments thus breaking the dependency.
> This can be fixed by simply creating a variable alias for each variable
> however the closure is a simple and concise fix.
> 

Similar here, "breaking the dependency" and "creating a variable alias"
can be described more accurately, e.g. the latter can be "creating a new
binding".

All in all, we need to establish a wide understanding of the issue and
agree on a reasonable fix. But anyway, thank Joel for doing this ;-)

Regards,
Boqun

> Another approach with using const-generics for the io.write32 API was
> investigated, but it cannot work with code that dynamically calculates
> the write offset.
> 
> Disassembly of users of pr_err!() with/without patch shows identical
> code generation, thus the fix has no difference in the final binary.
> 
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> ---
>  rust/kernel/print.rs | 44 ++++++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
> index 2d743d78d220..5847942195a7 100644
> --- a/rust/kernel/print.rs
> +++ b/rust/kernel/print.rs
> @@ -143,34 +143,38 @@ pub fn call_printk_cont(args: fmt::Arguments<'_>) {
>  #[expect(clippy::crate_in_macro_def)]
>  macro_rules! print_macro (
>      // The non-continuation cases (most of them, e.g. `INFO`).
> -    ($format_string:path, false, $($arg:tt)+) => (
> +    ($format_string:path, false, $($arg:tt)+) => ({
>          // To remain sound, `arg`s must be expanded outside the `unsafe` block.
>          // Typically one would use a `let` binding for that; however, `format_args!`
>          // takes borrows on the arguments, but does not extend the scope of temporaries.
>          // Therefore, a `match` expression is used to keep them around, since
>          // the scrutinee is kept until the end of the `match`.
> -        match $crate::prelude::fmt!($($arg)+) {
> -            // SAFETY: This hidden macro should only be called by the documented
> -            // printing macros which ensure the format string is one of the fixed
> -            // ones. All `__LOG_PREFIX`s are null-terminated as they are generated
> -            // by the `module!` proc macro or fixed values defined in a kernel
> -            // crate.
> -            args => unsafe {
> -                $crate::print::call_printk(
> -                    &$format_string,
> -                    crate::__LOG_PREFIX,
> -                    args,
> -                );
> +        (|| {
> +            match $crate::prelude::fmt!($($arg)+) {
> +                // SAFETY: This hidden macro should only be called by the documented
> +                // printing macros which ensure the format string is one of the fixed
> +                // ones. All `__LOG_PREFIX`s are null-terminated as they are generated
> +                // by the `module!` proc macro or fixed values defined in a kernel
> +                // crate.
> +                args => unsafe {
> +                    $crate::print::call_printk(
> +                        &$format_string,
> +                        crate::__LOG_PREFIX,
> +                        args,
> +                    );
> +                }
>              }
> -        }
> -    );
> +        })();
> +    });
>  
>      // The `CONT` case.
> -    ($format_string:path, true, $($arg:tt)+) => (
> -        $crate::print::call_printk_cont(
> -            $crate::prelude::fmt!($($arg)+),
> -        );
> -    );
> +    ($format_string:path, true, $($arg:tt)+) => ({
> +        (|| {
> +            $crate::print::call_printk_cont(
> +                $crate::prelude::fmt!($($arg)+),
> +            );
> +        })();
> +    });
>  );
>  
>  /// Stub for doctests
> -- 
> 2.34.1
> 

Re: [PATCH] rust: print: Fix issue with rust_build_error

[Joel Fernandes]

Hi Boqun,

On Sat, Sep 20, 2025 at 12:34:04PM -0700, Boqun Feng wrote:
> On Sat, Sep 20, 2025 at 12:19:58PM -0400, Joel Fernandes wrote:
> > When printing just before calling io.write32(), modpost fails due to
> > build_assert's missing rust_build_error symbol. The issue is that, the
> > printk arguments are passed as reference in bindings code, thus Rust
> > cannot trust its value and fails to optimize away the build_assert.
> > 
> 
> I think "cannot trust" is a bit vague and misleading here, for this
> kind of "workaround", we want the reason to be a bit clear. @Gary, could
> you help explain it a bit more?

I dumped the MIR and LLVM IRs here. It appears in the case of calling an
external function, it creates a pointer:

MIR:

bb0: {
    _1 = const 42_i32;           // offset = 42
    _3 = &raw const _1;          // Address is taken causing an escape I think.
    _2 = some_func(move _3);
}

I then dumped the LLVM IR and saw that it is infact reloading the pointer
after some_func is called.

Where as otherwise, you don't see the 'raw const' thing and no reloading
(build check is optimized away).

But I freely admit to be not yet too knowledgeable about the Rust compiler
and I only arrived at this patch with trial and error.

> 
> > The issue can be reproduced with the following simple snippet:
> >   let offset = 0;
> >   pr_err!("{}", offset);
> >   io.write32(base, offset);
> > 
> > Fix it by just using a closure to call printk. Rust captures the
> > arguments into the closure's arguments thus breaking the dependency.
> > This can be fixed by simply creating a variable alias for each variable
> > however the closure is a simple and concise fix.
> > 
> 
> Similar here, "breaking the dependency" and "creating a variable alias"
> can be described more accurately, e.g. the latter can be "creating a new
> binding".

Yeah sorry, "breaking the dependency" is indeed vague. I meant there is a
dependency between the external function modifying the data pointed at, and
the code following external function call using the data. Due to this, the
pointer is reloaded. I'll explain it better.

> 
> All in all, we need to establish a wide understanding of the issue and
> agree on a reasonable fix. But anyway, thank Joel for doing this ;-)

Absolutely, please consider this patch as a starting point for an
investigation. Worst case, maybe if I fixed the closure issue, it can be a
temporary workaround to alleviate the pain felt by some real users.

thanks,

 - Joel


> 
> Regards,
> Boqun
> 
> > Another approach with using const-generics for the io.write32 API was
> > investigated, but it cannot work with code that dynamically calculates
> > the write offset.
> > 
> > Disassembly of users of pr_err!() with/without patch shows identical
> > code generation, thus the fix has no difference in the final binary.
> > 
> > Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> > ---
> >  rust/kernel/print.rs | 44 ++++++++++++++++++++++++--------------------
> >  1 file changed, 24 insertions(+), 20 deletions(-)
> > 
> > diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
> > index 2d743d78d220..5847942195a7 100644
> > --- a/rust/kernel/print.rs
> > +++ b/rust/kernel/print.rs
> > @@ -143,34 +143,38 @@ pub fn call_printk_cont(args: fmt::Arguments<'_>) {
> >  #[expect(clippy::crate_in_macro_def)]
> >  macro_rules! print_macro (
> >      // The non-continuation cases (most of them, e.g. `INFO`).
> > -    ($format_string:path, false, $($arg:tt)+) => (
> > +    ($format_string:path, false, $($arg:tt)+) => ({
> >          // To remain sound, `arg`s must be expanded outside the `unsafe` block.
> >          // Typically one would use a `let` binding for that; however, `format_args!`
> >          // takes borrows on the arguments, but does not extend the scope of temporaries.
> >          // Therefore, a `match` expression is used to keep them around, since
> >          // the scrutinee is kept until the end of the `match`.
> > -        match $crate::prelude::fmt!($($arg)+) {
> > -            // SAFETY: This hidden macro should only be called by the documented
> > -            // printing macros which ensure the format string is one of the fixed
> > -            // ones. All `__LOG_PREFIX`s are null-terminated as they are generated
> > -            // by the `module!` proc macro or fixed values defined in a kernel
> > -            // crate.
> > -            args => unsafe {
> > -                $crate::print::call_printk(
> > -                    &$format_string,
> > -                    crate::__LOG_PREFIX,
> > -                    args,
> > -                );
> > +        (|| {
> > +            match $crate::prelude::fmt!($($arg)+) {
> > +                // SAFETY: This hidden macro should only be called by the documented
> > +                // printing macros which ensure the format string is one of the fixed
> > +                // ones. All `__LOG_PREFIX`s are null-terminated as they are generated
> > +                // by the `module!` proc macro or fixed values defined in a kernel
> > +                // crate.
> > +                args => unsafe {
> > +                    $crate::print::call_printk(
> > +                        &$format_string,
> > +                        crate::__LOG_PREFIX,
> > +                        args,
> > +                    );
> > +                }
> >              }
> > -        }
> > -    );
> > +        })();
> > +    });
> >  
> >      // The `CONT` case.
> > -    ($format_string:path, true, $($arg:tt)+) => (
> > -        $crate::print::call_printk_cont(
> > -            $crate::prelude::fmt!($($arg)+),
> > -        );
> > -    );
> > +    ($format_string:path, true, $($arg:tt)+) => ({
> > +        (|| {
> > +            $crate::print::call_printk_cont(
> > +                $crate::prelude::fmt!($($arg)+),
> > +            );
> > +        })();
> > +    });
> >  );
> >  
> >  /// Stub for doctests
> > -- 
> > 2.34.1
> > 

Re: [PATCH] rust: print: Fix issue with rust_build_error

[Benno Lossin]

On Sat Sep 20, 2025 at 6:19 PM CEST, Joel Fernandes wrote:
> When printing just before calling io.write32(), modpost fails due to
> build_assert's missing rust_build_error symbol. The issue is that, the
> printk arguments are passed as reference in bindings code, thus Rust
> cannot trust its value and fails to optimize away the build_assert.
>
> The issue can be reproduced with the following simple snippet:
>   let offset = 0;
>   pr_err!("{}", offset);
>   io.write32(base, offset);

I took a long time to understand this and I think I got it now, but
maybe I'm still wrong: passing `offset` to `printk` via a reference
results in the compiler thinking that the value of `offset` might be
changed (even though its a shared reference I assume). For this reason
the `build_assert!` used by `io.write32` cannot be optimized away.

> Fix it by just using a closure to call printk. Rust captures the
> arguments into the closure's arguments thus breaking the dependency.
> This can be fixed by simply creating a variable alias for each variable
> however the closure is a simple and concise fix.

I don't think this is the fix we want to have. In fact it already
doesn't compile all of the existing code:

    error[E0277]: the `?` operator can only be used in a closure that returns `Result` or `Option` (or another type that implements `FromResidual`)
        --> rust/doctests_kernel_generated.rs:3446:70
         |
    3446 |     pr_info!("The frequency at index 0 is: {:?}\n", table.freq(index)?);
         |     -----------------------------------------------------------------^-
         |     |                                                                |
         |     |                                                                cannot use the `?` operator in a closure that returns `()`
         |     this function should return `Result` or `Option` to accept `?`

(originating from `rust/kernel/cpufreq.rs:217`)

Can't we just mark the pointer properly as read-only?

---
Cheers,
Benno

> Another approach with using const-generics for the io.write32 API was
> investigated, but it cannot work with code that dynamically calculates
> the write offset.
>
> Disassembly of users of pr_err!() with/without patch shows identical
> code generation, thus the fix has no difference in the final binary.
>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>

Re: [PATCH] rust: print: Fix issue with rust_build_error

[Joel Fernandes]

On Sat, Sep 20, 2025 at 10:09:30PM +0200, Benno Lossin wrote:
> On Sat Sep 20, 2025 at 6:19 PM CEST, Joel Fernandes wrote:
> > When printing just before calling io.write32(), modpost fails due to
> > build_assert's missing rust_build_error symbol. The issue is that, the
> > printk arguments are passed as reference in bindings code, thus Rust
> > cannot trust its value and fails to optimize away the build_assert.
> >
> > The issue can be reproduced with the following simple snippet:
> >   let offset = 0;
> >   pr_err!("{}", offset);
> >   io.write32(base, offset);
> 
> I took a long time to understand this and I think I got it now, but
> maybe I'm still wrong: passing `offset` to `printk` via a reference
> results in the compiler thinking that the value of `offset` might be
> changed (even though its a shared reference I assume). For this reason
> the `build_assert!` used by `io.write32` cannot be optimized away.

Yes, that's correct and that's my understanding. I in fact also dumped the IR
and see that the compiler is trying to reload the pointer to the offset. I
feel this is a compiler bug, and for immutable variables, the compiler should
not be reloading them even if they are passed to external code? Because if
external code is modifying immutable variables, that is buggy anyway.

> > Fix it by just using a closure to call printk. Rust captures the
> > arguments into the closure's arguments thus breaking the dependency.
> > This can be fixed by simply creating a variable alias for each variable
> > however the closure is a simple and concise fix.
> 
> I don't think this is the fix we want to have.

Yeah its ugly, though a small workaround. IMO ideally the fix should be in
the compiler. I want to send a proposal for this in fact. I looked at the MIR
and I believe with my limited understanding, that the MIR should be issuing a
copy before making a pointer of the immutable variable if pointers to
immutable variables are being passed to external functions.

If I were to send a proposal to the Rust language to start a discussion
leading to a potential RFC stage, would you mind guiding me on doing so?

> In fact it already
> doesn't compile all of the existing code:

Oh gosh, I should have run doctests. I just did a normal build. Let me see
if I can fix this closure issue.

> 
>     error[E0277]: the `?` operator can only be used in a closure that returns `Result` or `Option` (or another type that implements `FromResidual`)
>         --> rust/doctests_kernel_generated.rs:3446:70
>          |
>     3446 |     pr_info!("The frequency at index 0 is: {:?}\n", table.freq(index)?);
>          |     -----------------------------------------------------------------^-
>          |     |                                                                |
>          |     |                                                                cannot use the `?` operator in a closure that returns `()`
>          |     this function should return `Result` or `Option` to accept `?`
> 
> (originating from `rust/kernel/cpufreq.rs:217`)
> 
> Can't we just mark the pointer properly as read-only?

I found no way yet to do that. If there is something you'd like me to try,
let me know. But even if the pointer is a C const pointer, LLVM seems to
always want to reload it.

Thanks!

 - Joel


> 
> ---
> Cheers,
> Benno
> 
> > Another approach with using const-generics for the io.write32 API was
> > investigated, but it cannot work with code that dynamically calculates
> > the write offset.
> >
> > Disassembly of users of pr_err!() with/without patch shows identical
> > code generation, thus the fix has no difference in the final binary.
> >
> > Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>

Re: [PATCH] rust: print: Fix issue with rust_build_error

[Benno Lossin]

On Sun Sep 21, 2025 at 2:45 AM CEST, Joel Fernandes wrote:
> On Sat, Sep 20, 2025 at 10:09:30PM +0200, Benno Lossin wrote:
>> On Sat Sep 20, 2025 at 6:19 PM CEST, Joel Fernandes wrote:
>> > When printing just before calling io.write32(), modpost fails due to
>> > build_assert's missing rust_build_error symbol. The issue is that, the
>> > printk arguments are passed as reference in bindings code, thus Rust
>> > cannot trust its value and fails to optimize away the build_assert.
>> >
>> > The issue can be reproduced with the following simple snippet:
>> >   let offset = 0;
>> >   pr_err!("{}", offset);
>> >   io.write32(base, offset);
>> 
>> I took a long time to understand this and I think I got it now, but
>> maybe I'm still wrong: passing `offset` to `printk` via a reference
>> results in the compiler thinking that the value of `offset` might be
>> changed (even though its a shared reference I assume). For this reason
>> the `build_assert!` used by `io.write32` cannot be optimized away.
>
> Yes, that's correct and that's my understanding. I in fact also dumped the IR
> and see that the compiler is trying to reload the pointer to the offset. I
> feel this is a compiler bug, and for immutable variables, the compiler should
> not be reloading them even if they are passed to external code? Because if
> external code is modifying immutable variables, that is buggy anyway.

It would be great if you could add all of the extra info that you looked
at into the commit message here. So all of the code to reproduce the
issue, the IR you looked at...

>> > Fix it by just using a closure to call printk. Rust captures the
>> > arguments into the closure's arguments thus breaking the dependency.
>> > This can be fixed by simply creating a variable alias for each variable
>> > however the closure is a simple and concise fix.
>> 
>> I don't think this is the fix we want to have.
>
> Yeah its ugly, though a small workaround. IMO ideally the fix should be in
> the compiler. I want to send a proposal for this in fact. I looked at the MIR
> and I believe with my limited understanding, that the MIR should be issuing a
> copy before making a pointer of the immutable variable if pointers to
> immutable variables are being passed to external functions.

... and definitely the MIR.

> If I were to send a proposal to the Rust language to start a discussion
> leading to a potential RFC stage, would you mind guiding me on doing so?

I agree that this should be fixed in the compiler if we aren't missing
some attribute on one of our functions.

Note that this probably doesn't require an RFC, since it's not a big
feature and I imagine that there isn't much controversy about it. We can
mention this in our sync meeting on Wednesday with the Rust folks & see
what they have to say about it. After that we probably want to open an
issue about it, I'll let you know.

>> In fact it already
>> doesn't compile all of the existing code:
>
> Oh gosh, I should have run doctests. I just did a normal build. Let me see
> if I can fix this closure issue.

We might just want to live with this until we get the compiler fixed. Or
is there a different workaround like this:

    let offset = 0;
    let offset_pr = offset;
    pr_err!("{}", offset_pr);
    io.write32(base, offset);

In that case, we might want to prefer that over using a closure... Let's
see how some others respond to this.

>>     error[E0277]: the `?` operator can only be used in a closure that returns `Result` or `Option` (or another type that implements `FromResidual`)
>>         --> rust/doctests_kernel_generated.rs:3446:70
>>          |
>>     3446 |     pr_info!("The frequency at index 0 is: {:?}\n", table.freq(index)?);
>>          |     -----------------------------------------------------------------^-
>>          |     |                                                                |
>>          |     |                                                                cannot use the `?` operator in a closure that returns `()`
>>          |     this function should return `Result` or `Option` to accept `?`
>> 
>> (originating from `rust/kernel/cpufreq.rs:217`)
>> 
>> Can't we just mark the pointer properly as read-only?
>
> I found no way yet to do that. If there is something you'd like me to try,
> let me know. But even if the pointer is a C const pointer, LLVM seems to
> always want to reload it.

Might be an LLVM bug then? Wouldn't be the first that Rust found :)

---
Cheers,
Benno

Re: [PATCH] rust: print: Fix issue with rust_build_error

[Benno Lossin]

On Sun Sep 21, 2025 at 9:12 AM CEST, Benno Lossin wrote:
> On Sun Sep 21, 2025 at 2:45 AM CEST, Joel Fernandes wrote:
>> On Sat, Sep 20, 2025 at 10:09:30PM +0200, Benno Lossin wrote:
>>> On Sat Sep 20, 2025 at 6:19 PM CEST, Joel Fernandes wrote:
>>> > When printing just before calling io.write32(), modpost fails due to
>>> > build_assert's missing rust_build_error symbol. The issue is that, the
>>> > printk arguments are passed as reference in bindings code, thus Rust
>>> > cannot trust its value and fails to optimize away the build_assert.
>>> >
>>> > The issue can be reproduced with the following simple snippet:
>>> >   let offset = 0;
>>> >   pr_err!("{}", offset);
>>> >   io.write32(base, offset);
>>> 
>>> I took a long time to understand this and I think I got it now, but
>>> maybe I'm still wrong: passing `offset` to `printk` via a reference
>>> results in the compiler thinking that the value of `offset` might be
>>> changed (even though its a shared reference I assume). For this reason
>>> the `build_assert!` used by `io.write32` cannot be optimized away.
>>
>> Yes, that's correct and that's my understanding. I in fact also dumped the IR
>> and see that the compiler is trying to reload the pointer to the offset. I
>> feel this is a compiler bug, and for immutable variables, the compiler should
>> not be reloading them even if they are passed to external code? Because if
>> external code is modifying immutable variables, that is buggy anyway.
>
> It would be great if you could add all of the extra info that you looked
> at into the commit message here. So all of the code to reproduce the
> issue, the IR you looked at...
>
>>> > Fix it by just using a closure to call printk. Rust captures the
>>> > arguments into the closure's arguments thus breaking the dependency.
>>> > This can be fixed by simply creating a variable alias for each variable
>>> > however the closure is a simple and concise fix.
>>> 
>>> I don't think this is the fix we want to have.
>>
>> Yeah its ugly, though a small workaround. IMO ideally the fix should be in
>> the compiler. I want to send a proposal for this in fact. I looked at the MIR
>> and I believe with my limited understanding, that the MIR should be issuing a
>> copy before making a pointer of the immutable variable if pointers to
>> immutable variables are being passed to external functions.
>
> ... and definitely the MIR.
>
>> If I were to send a proposal to the Rust language to start a discussion
>> leading to a potential RFC stage, would you mind guiding me on doing so?
>
> I agree that this should be fixed in the compiler if we aren't missing
> some attribute on one of our functions.
>
> Note that this probably doesn't require an RFC, since it's not a big
> feature and I imagine that there isn't much controversy about it. We can
> mention this in our sync meeting on Wednesday with the Rust folks & see
> what they have to say about it. After that we probably want to open an
> issue about it, I'll let you know.

I have created a simpler example than using `io.write32` and given lots
of info below, I'll show this to the Rust folks & you can add it to the
commit message if you want :)

Printing and our build assert doesn't work together:

```rust
let x = 0;
pr_info!("{}", x);
build_assert!(x == 0);
```

Macros expanded:

```rust
let x = 0;
match format_args!("{0}", x) {
    args => unsafe {
        ::kernel::print::call_printk(
            &::kernel::print::format_strings::INFO,
            crate::__LOG_PREFIX,
            args,
        );
    },
};
{
    if !(x == 0) {
        ::kernel::build_assert::build_error("assertion failed: x == 0");
    }
};
```

where `build_error` & `call_printk` are defined as:

```rust
#[inline(never)]
#[cold]
#[export_name = "rust_build_error"]
#[track_caller]
pub const fn build_error(msg: &'static str) -> ! {
    panic!("{}", msg);
}

pub unsafe fn call_printk(
    format_string: &[u8; format_strings::LENGTH],
    module_name: &[u8],
    args: fmt::Arguments<'_>,
) {
    unsafe {
        bindings::_printk(
            format_string.as_ptr(),
            module_name.as_ptr(),
            core::ptr::from_ref(&args).cast::<c_void>(),
        );
    }
}
// bindings:
extern "C" {
    pub fn _printk(fmt: *const ffi::c_char, ...) -> ffi::c_int;
}
```

```c
int _printk(const char *fmt, ...);
```

When compiling, we won't have the `rust_build_error` symbol defined, so if the
linker sees this, it will fail with a (very unergonomic) error:

```text
ld.lld: error: undefined symbol: rust_build_error
>>> referenced by usercopy_64.c
>>>               vmlinux.o:(print_build_assert_test)
make[2]: *** [scripts/Makefile.vmlinux:91: vmlinux.unstripped] Error 1
make[1]: *** [Makefile:1244: vmlinux] Error 2
make: *** [Makefile:248: __sub-make] Error 2
```

The LLVM IR looks like this:

```llvm
  %args1 = alloca [16 x i8], align 8
  %args = alloca [48 x i8], align 8
  %x = alloca [4 x i8], align 4
  call void @llvm.lifetime.start.p0(i64 4, ptr nonnull %x)
  store i32 0, ptr %x, align 4
  call void @llvm.lifetime.start.p0(i64 16, ptr nonnull %args1)
  store ptr %x, ptr %args1, align 8
  %_4.sroa.4.0..sroa_idx = getelementptr inbounds nuw i8, ptr %args1, i64 8
  store ptr @_RNvXs5_NtNtNtCs1h9PaebWwla_4core3fmt3num3implNtB9_7Display3fmt, ptr %_4.sroa.4.0..sroa_idx, align 8
  store ptr @alloc_eab5d04767146d7d9b93b60d28ef530a, ptr %args, align 8
  %0 = getelementptr inbounds nuw i8, ptr %args, i64 8
  store i64 1, ptr %0, align 8
  %1 = getelementptr inbounds nuw i8, ptr %args, i64 32
  store ptr null, ptr %1, align 8
  %2 = getelementptr inbounds nuw i8, ptr %args, i64 16
  store ptr %args1, ptr %2, align 8
  %3 = getelementptr inbounds nuw i8, ptr %args, i64 24
  store i64 1, ptr %3, align 8
; call kernel::print::call_printk
  call void @_RNvNtCsetEQUy9amV6_6kernel5print11call_printk(ptr noalias noundef nonnull readonly align 1 dereferenceable(10) @_RNvNtNtCsetEQUy9amV6_6kernel5print14format_strings4INFO, ptr noalias noundef nonnull readonly align 1 @alloc_9e327419e36c421bc1658af3b68806c2, i64 noundef 13, ptr noalias nocapture noundef nonnull align 8 dereferenceable(48) %args) #8
  call void @llvm.lifetime.end.p0(i64 16, ptr nonnull %args1)
  %_7 = load i32, ptr %x, align 4, !noundef !7
  ;     ^^^^^^^^^^^^^^^^ we load the value of `x` again after calling `call_printk`,
  ;                      even though it was a shared reference!
  %4 = icmp eq i32 %_7, 0
  br i1 %4, label %bb2, label %bb3, !prof !11

bb2:                                              ; preds = %start
  call void @llvm.lifetime.end.p0(i64 4, ptr nonnull %x)
  ret void

bb3:                                              ; preds = %start
  call void @rust_build_error(ptr noalias noundef nonnull readonly align 1 @alloc_7f598d51153ed56ec9aa560e64562fc0, i64 noundef 24, ptr noalias noundef nonnull readonly align 8 dereferenceable(24) @alloc_4255a8c4673dfae2bb72b50834aecf9e) #9
  unreachable
```

The MIR looks like this:

```text
let mut _0: ();
let _1: i32;
let mut _2: core::fmt::Arguments<'_>;
let mut _4: core::fmt::rt::Argument<'_>;
let _5: &[core::fmt::rt::Argument<'_>; 1];
let _6: ();
let mut _7: i32;
let _8: !;
scope 1 {
    debug x => _1;
    let _3: [core::fmt::rt::Argument<'_>; 1];
    scope 2 {
        debug args => _3;
        scope 9 (inlined core::fmt::rt::<impl Arguments<'_>>::new_v1::<1, 1>) {
            let mut _15: &[&str];
            let mut _16: &[core::fmt::rt::Argument<'_>];
        }
    }
    scope 3 {
        debug args => _2;
    }
    scope 4 (inlined core::fmt::rt::Argument::<'_>::new_display::<i32>) {
        let mut _9: core::fmt::rt::ArgumentType<'_>;
        let mut _10: core::ptr::NonNull<()>;
        let mut _11: for<'a, 'b> unsafe fn(core::ptr::NonNull<()>, &'a mut core::fmt::Formatter<'b>) -> core::result::Result<(), core::fmt::Error>;
        let _12: for<'a, 'b, 'c> fn(&'a i32, &'b mut core::fmt::Formatter<'c>) -> core::result::Result<(), core::fmt::Error>;
        scope 5 {
        }
        scope 6 (inlined NonNull::<i32>::from_ref) {
            let mut _13: *const i32;
        }
        scope 7 (inlined NonNull::<i32>::cast::<()>) {
            let mut _14: *const ();
            scope 8 (inlined NonNull::<i32>::as_ptr) {
            }
        }
    }
}

bb0: {
    StorageLive(_1);
    _1 = const 0_i32;
    StorageLive(_3);
    StorageLive(_4);
    StorageLive(_12);
    StorageLive(_13);
    StorageLive(_9);
    StorageLive(_10);
    _13 = &raw const _1;
    StorageLive(_14);
    _14 = copy _13 as *const () (PtrToPtr);
    _10 = NonNull::<()> { pointer: move _14 };
    StorageDead(_14);
    StorageLive(_11);
    _12 = <i32 as core::fmt::Display>::fmt as for<'a, 'b, 'c> fn(&'a i32, &'b mut core::fmt::Formatter<'c>) -> core::result::Result<(), core::fmt::Error> (PointerCoercion(ReifyFnPointer, Implicit));
    _11 = copy _12 as for<'a, 'b> unsafe fn(core::ptr::NonNull<()>, &'a mut core::fmt::Formatter<'b>) -> core::result::Result<(), core::fmt::Error> (Transmute);
    _9 = core::fmt::rt::ArgumentType::<'_>::Placeholder { value: move _10, formatter: move _11, _lifetime: const PhantomData::<&()> };
    StorageDead(_11);
    StorageDead(_10);
    _4 = core::fmt::rt::Argument::<'_> { ty: move _9 };
    StorageDead(_9);
    StorageDead(_13);
    StorageDead(_12);
    _3 = [move _4];
    StorageDead(_4);
    _5 = &_3;
    StorageLive(_15);
    _15 = const print_build_assert_test::promoted[0] as &[&str] (PointerCoercion(Unsize, Implicit));
    StorageLive(_16);
    _16 = copy _5 as &[core::fmt::rt::Argument<'_>] (PointerCoercion(Unsize, Implicit));
    _2 = Arguments::<'_> { pieces: move _15, fmt: const Option::<&[core::fmt::rt::Placeholder]>::None, args: move _16 };
    StorageDead(_16);
    StorageDead(_15);
    _6 = call_printk(const <static(DefId(2:3562 ~ kernel[a8a3]::print::format_strings::INFO))>, const __LOG_PREFIX, move _2) -> [return: bb1, unwind unreachable];
}

bb1: {
    StorageDead(_3);
    StorageLive(_7);
    _7 = copy _1;
    //   ^^^^^^^^ this is the problematic part, it shouldn't prompt a re-read of
    //            the location, but rather just know that the value is 0
    switchInt(move _7) -> [0: bb2, otherwise: bb3];
}

bb2: {
    StorageDead(_7);
    StorageDead(_1);
    return;
}

bb3: {
    StorageDead(_7);
    _8 = build_error(const "assertion failed: x == 0") -> unwind unreachable;
}
```

---
Cheers,
Benno

Re: [PATCH] rust: print: Fix issue with rust_build_error

[Gary Guo]

On Sun, 21 Sep 2025 09:12:45 +0200
"Benno Lossin" <lossin@kernel.org> wrote:

> On Sun Sep 21, 2025 at 2:45 AM CEST, Joel Fernandes wrote:
> > On Sat, Sep 20, 2025 at 10:09:30PM +0200, Benno Lossin wrote:  
> >> On Sat Sep 20, 2025 at 6:19 PM CEST, Joel Fernandes wrote:  
> >> > When printing just before calling io.write32(), modpost fails due to
> >> > build_assert's missing rust_build_error symbol. The issue is that, the
> >> > printk arguments are passed as reference in bindings code, thus Rust
> >> > cannot trust its value and fails to optimize away the build_assert.
> >> >
> >> > The issue can be reproduced with the following simple snippet:
> >> >   let offset = 0;
> >> >   pr_err!("{}", offset);
> >> >   io.write32(base, offset);  
> >> 
> >> I took a long time to understand this and I think I got it now, but
> >> maybe I'm still wrong: passing `offset` to `printk` via a reference
> >> results in the compiler thinking that the value of `offset` might be
> >> changed (even though its a shared reference I assume). For this reason
> >> the `build_assert!` used by `io.write32` cannot be optimized away.  
> >
> > Yes, that's correct and that's my understanding. I in fact also dumped the IR
> > and see that the compiler is trying to reload the pointer to the offset. I
> > feel this is a compiler bug, and for immutable variables, the compiler should
> > not be reloading them even if they are passed to external code? Because if
> > external code is modifying immutable variables, that is buggy anyway.  
> 
> It would be great if you could add all of the extra info that you looked
> at into the commit message here. So all of the code to reproduce the
> issue, the IR you looked at...
> 
> >> > Fix it by just using a closure to call printk. Rust captures the
> >> > arguments into the closure's arguments thus breaking the dependency.
> >> > This can be fixed by simply creating a variable alias for each variable
> >> > however the closure is a simple and concise fix.  
> >> 
> >> I don't think this is the fix we want to have.  
> >
> > Yeah its ugly, though a small workaround. IMO ideally the fix should be in
> > the compiler. I want to send a proposal for this in fact. I looked at the MIR
> > and I believe with my limited understanding, that the MIR should be issuing a
> > copy before making a pointer of the immutable variable if pointers to
> > immutable variables are being passed to external functions.  
> 
> ... and definitely the MIR.
> 
> > If I were to send a proposal to the Rust language to start a discussion
> > leading to a potential RFC stage, would you mind guiding me on doing so?  
> 
> I agree that this should be fixed in the compiler if we aren't missing
> some attribute on one of our functions.
> 
> Note that this probably doesn't require an RFC, since it's not a big
> feature and I imagine that there isn't much controversy about it. We can
> mention this in our sync meeting on Wednesday with the Rust folks & see
> what they have to say about it. After that we probably want to open an
> issue about it, I'll let you know.
> 
> >> In fact it already
> >> doesn't compile all of the existing code:  
> >
> > Oh gosh, I should have run doctests. I just did a normal build. Let me see
> > if I can fix this closure issue.  
> 
> We might just want to live with this until we get the compiler fixed. Or
> is there a different workaround like this:
> 
>     let offset = 0;
>     let offset_pr = offset;
>     pr_err!("{}", offset_pr);
>     io.write32(base, offset);

I suggested to Joel during Kangrejos that just

	pr_err!("{}", {offset});

should work, as it would create a new copy.

Best,
Gary

Re: [PATCH] rust: print: Fix issue with rust_build_error

[Miguel Ojeda]

On Mon, Sep 22, 2025 at 12:29 PM Gary Guo <gary@garyguo.net> wrote:
>
> I suggested to Joel during Kangrejos that just
>
>         pr_err!("{}", {offset});
>
> should work, as it would create a new copy.

This one (or Benno's) sounds simple enough (assuming `rustfmt` keeps
it reasonable) -- I would welcome a patch adding the explanation of
the issue and the workaround(s) to e.g.
`Documentation/rust/coding-guidelines.rst`.

Thanks!

Cheers,
Miguel

Re: [PATCH] rust: print: Fix issue with rust_build_error

[Miguel Ojeda]

On Sun, Sep 21, 2025 at 2:45 AM Joel Fernandes <joelagnelf@nvidia.com> wrote:
>
> But even if the pointer is a C const pointer, LLVM seems to
> always want to reload it.

What do you mean by this? I think I mentioned in the other thread that
a C pointer to const still allows the callee to change the value.

Cheers,
Miguel

Re: [PATCH] rust: print: Fix issue with rust_build_error

[Joel Fernandes]

On Sun, Sep 21, 2025 at 11:13:11AM +0200, Miguel Ojeda wrote:
> On Sun, Sep 21, 2025 at 2:45 AM Joel Fernandes <joelagnelf@nvidia.com> wrote:
> >
> > But even if the pointer is a C const pointer, LLVM seems to
> > always want to reload it.
> 
> What do you mean by this? I think I mentioned in the other thread that
> a C pointer to const still allows the callee to change the value.

Apologies, indeed a const pointer in C does not mean the pointee cannot be
modified.

I think I somewhat understand the issue but still not fully. MIR optimization
is supposed to optimize away the dead code in build_assert. This is what I
see for "good" cases when things work.

But the information that the data being printed is an immutable reference, is
lost somehow during MIR optimization phase when a printk is involved.  Per the
github issue [1], there is likely some provenance information in the immutable
reference to the data, that gets lost during "MIR inlining" optimization. In
other words, this is not an LLVM problem as I was pointing out, but an MIR
optimization problem.  Benno/Gary correct me anything I said is wrong.

I guess I still have a few more questions:

1. What is being inlined when we talk about MIR inlining? The print
statement? Constructor to the Argument object? Something else?

2. What does 'noalias' mean in the github report [1], and why would that effect
'MIR inlining'?

3. Why does LLVM inlining still succeed when MIR inlining is disabled? This
has something to do with a new llvm feature Niki referred to in the report.

thanks,

 - Joel
[1] https://github.com/rust-lang/rust/issues/146844

Re: [PATCH] rust: print: Fix issue with rust_build_error

[Alice Ryhl]

On Sat, Sep 20, 2025 at 6:20 PM Joel Fernandes <joelagnelf@nvidia.com> wrote:
>
> When printing just before calling io.write32(), modpost fails due to
> build_assert's missing rust_build_error symbol. The issue is that, the
> printk arguments are passed as reference in bindings code, thus Rust
> cannot trust its value and fails to optimize away the build_assert.
>
> The issue can be reproduced with the following simple snippet:
>   let offset = 0;
>   pr_err!("{}", offset);
>   io.write32(base, offset);
>
> Fix it by just using a closure to call printk. Rust captures the
> arguments into the closure's arguments thus breaking the dependency.
> This can be fixed by simply creating a variable alias for each variable
> however the closure is a simple and concise fix.
>
> Another approach with using const-generics for the io.write32 API was
> investigated, but it cannot work with code that dynamically calculates
> the write offset.
>
> Disassembly of users of pr_err!() with/without patch shows identical
> code generation, thus the fix has no difference in the final binary.
>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>

The actual bug is that write32 uses build_error!. Trying to change the
printing macros is just a band-aid. Someone already mentioned that it
breaks the ? operator. I think this change is a bad idea. We should
fix the actual problem, rather than making random changes to other
parts of the kernel to work around build_error!'s inherent fragility.

Alice

Re: [PATCH] rust: print: Fix issue with rust_build_error

[Joel Fernandes]

On Sun, Sep 21, 2025 at 12:46:26PM +0200, Alice Ryhl wrote:
> On Sat, Sep 20, 2025 at 6:20 PM Joel Fernandes <joelagnelf@nvidia.com> wrote:
> >
> > When printing just before calling io.write32(), modpost fails due to
> > build_assert's missing rust_build_error symbol. The issue is that, the
> > printk arguments are passed as reference in bindings code, thus Rust
> > cannot trust its value and fails to optimize away the build_assert.
> >
> > The issue can be reproduced with the following simple snippet:
> >   let offset = 0;
> >   pr_err!("{}", offset);
> >   io.write32(base, offset);
> >
> > Fix it by just using a closure to call printk. Rust captures the
> > arguments into the closure's arguments thus breaking the dependency.
> > This can be fixed by simply creating a variable alias for each variable
> > however the closure is a simple and concise fix.
> >
> > Another approach with using const-generics for the io.write32 API was
> > investigated, but it cannot work with code that dynamically calculates
> > the write offset.
> >
> > Disassembly of users of pr_err!() with/without patch shows identical
> > code generation, thus the fix has no difference in the final binary.
> >
> > Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> 
> The actual bug is that write32 uses build_error!.

I don't think that is the issue, I spoke with Gary and he educated me that
failure of the compiler to do simple compiler optimizations (I am guessing in
this case, dead-code elimination) is a compiler bug. Even in the case where
the write offset is dynamic at runtime, since the caller of write should be
using something like try_write or checking for the offset bounds, the
build_error should be optimized out. Right?

> Trying to change the printing macros is just a band-aid. Someone already
> mentioned that it breaks the ? operator. I think this change is a bad idea.
> We should fix the actual problem, rather than making random changes to
> other parts of the kernel to work around build_error!'s inherent fragility.

I don't think the fragility is in build_error since the direction here is the
bug is in the compiler. So if the compiler optimizes things correctly, we may
conclude that build_error is the correct thing to call. Or is there some
other reason you think build_error is fragile?

As for this patch (and its being a bad idea), I think it was already
mentioned that this patch was not intended for a permanent solution but
rather as a starting point for an investigation. I am probably to take the
blame for not tagging it as 'RFC' though. I encourage people personally to
send patches (whether good or bad ideas) to code I maintain. Lets have mercy
on ideas, they may be bad sometimes but every once in a while, they may
happen to turn out to be good too. How would one know if they didn't try? ;-)

thanks,

 - Joel

Re: [PATCH] rust: print: Fix issue with rust_build_error

[kernel test robot]

Hi Joel,

kernel test robot noticed the following build errors:

[auto build test ERROR on rust/rust-next]
[also build test ERROR on linus/master v6.17-rc6 next-20250919]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Joel-Fernandes/rust-print-Fix-issue-with-rust_build_error/20250921-002215
base:   https://github.com/Rust-for-Linux/linux rust-next
patch link:    https://lore.kernel.org/r/20250920161958.2079105-1-joelagnelf%40nvidia.com
patch subject: [PATCH] rust: print: Fix issue with rust_build_error
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250922/202509220423.4d1foVAO-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250922/202509220423.4d1foVAO-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509220423.4d1foVAO-lkp@intel.com/

All errors (new ones prefixed by >>):

>> error[E0277]: the `?` operator can only be used in a closure that returns `Result` or `Option` (or another type that implements `FromResidual`)
   --> rust/doctests_kernel_generated.rs:3446:70
   |
   3446 |     pr_info!("The frequency at index 0 is: {:?}n", table.freq(index)?);
   |     -----------------------------------------------------------------^-
   |     |                                                                |
   |     |                                                                cannot use the `?` operator in a closure that returns `()`
   |     this function should return `Result` or `Option` to accept `?`

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki