Message-Id: <DCYCVV212A7X.U2ZDJDY18LQX@kernel.org>
Date: Sun, 21 Sep 2025 11:03:59 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Joel Fernandes" <joelagnelf@...dia.com>
Cc: <linux-kernel@...r.kernel.org>, "Miguel Ojeda" <ojeda@...nel.org>, "Alex
 Gaynor" <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary
 Guo" <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Danilo Krummrich" <dakr@...nel.org>, <acourbot@...dia.com>, "Alistair
 Popple" <apopple@...dia.com>, "Timur Tabi" <ttabi@...dia.com>,
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH] rust: print: Fix issue with rust_build_error

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

Powered by blists - more mailing lists