Re: [PATCH v4 4/4] rust: Add warn_on macro

[FUJITA Tomonori]

On Tue, 8 Apr 2025 15:55:38 +0200
Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:

> On Wed, Mar 5, 2025 at 12:10 PM FUJITA Tomonori
> <fujita.tomonori@xxxxxxxxx> wrote:
>>
>> Add warn_on macro, uses the BUG/WARN feature (lib/bug.c) via assembly
>> for x86_64/arm64/riscv.
>>
>> The current Rust code simply wraps BUG() macro but doesn't provide the
>> proper debug information. The BUG/WARN feature can only be used from
>> assembly.
>>
>> This uses the assembly code exported by the C side via ARCH_WARN_ASM
>> macro. To avoid duplicating the assembly code, this approach follows
>> the same strategy as the static branch code: it generates the assembly
>> code for Rust using the C preprocessor at compile time.
>>
>> Similarly, ARCH_WARN_REACHABLE is also used at compile time to
>> generate the assembly code; objtool's reachable anotation code. It's
>> used for only architectures that use objtool.
>>
>> For now, Loongarch just uses a wrapper for WARN macro.
>>
>> UML doesn't use the assembly BUG/WARN feature; just wrapping generic
>> BUG/WARN functions implemented in C works.
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxx>
> 
> Overall looks reasonable. A few nits below:

Thanks!

>> +#[macro_export]
>> +#[doc(hidden)]
>> +#[cfg(all(CONFIG_BUG, not(CONFIG_UML), not(CONFIG_LOONGARCH)))]
>> +#[cfg(CONFIG_DEBUG_BUGVERBOSE)]
>> +macro_rules! warn_flags {
>> +    ($flags:expr) => {
>> +        const FLAGS: u32 = $crate::bindings::BUGFLAG_WARNING | $flags;
>> +        const _FILE: &[u8] = file!().as_bytes();
>> +        // Plus one for null-terminator.
>> +        static FILE: [u8; _FILE.len() + 1] = {
>> +            let mut bytes = [0; _FILE.len() + 1];
>> +            let mut i = 0;
>> +            while i < _FILE.len() {
>> +                bytes[i] = _FILE[i];
>> +                i += 1;
>> +            }
>> +            bytes
>> +        };
>> +        // SAFETY: Just an FFI call.
>> +        unsafe {
>> +            $crate::asm!(concat!(
>> +                "/* {size} */",
>> +                include!(concat!(env!("OBJTREE"), "/rust/kernel/generated_arch_warn_asm.rs")),
>> +                include!(concat!(env!("OBJTREE"), "/rust/kernel/generated_arch_reachable_asm.rs")));
>> +            file = sym FILE,
>> +            line = const line!(),
>> +            flags = const FLAGS,
>> +            size = const ::core::mem::size_of::<$crate::bindings::bug_entry>(),
> 
> The indentation here is odd. Shouldn't the arguments be indented one more?

Yeah, fixed.


>> +#[macro_export]
>> +#[doc(hidden)]
>> +#[cfg(all(CONFIG_BUG, not(CONFIG_UML), not(CONFIG_LOONGARCH)))]
>> +#[cfg(not(CONFIG_DEBUG_BUGVERBOSE))]
>> +macro_rules! warn_flags {
>> +    ($flags:expr) => {
>> +        const FLAGS: u32 = $crate::bindings::BUGFLAG_WARNING | $flags;
>> +        // SAFETY: Just an FFI call.
>> +        unsafe {
>> +            $crate::asm!(
>> +            concat!(
>> +                "/* {size} */",
>> +                include!(concat!(env!("OBJTREE"), "/rust/kernel/generated_arch_warn_asm.rs")),
>> +                include!(concat!(env!("OBJTREE"), "/rust/kernel/generated_arch_reachable_asm.rs")));
>> +            flags = const FLAGS,
>> +            size = const ::core::mem::size_of::<$crate::bindings::bug_entry>(),
> 
> Same thing here.

Fixed.

>> +            );
>> +        }
>> +    }
>> +}
>> +
>> +#[macro_export]
>> +#[doc(hidden)]
>> +#[cfg(all(CONFIG_BUG, CONFIG_UML))]
>> +macro_rules! warn_flags {
>> +    ($flags:expr) => {
>> +        // SAFETY: Just an FFI call.
>> +        unsafe {
>> +            $crate::bindings::warn_slowpath_fmt(
>> +                $crate::c_str!(::core::file!()).as_ptr() as *const ::core::ffi::c_uchar,
>> +                line!() as i32,
>> +                $flags as u32,
>> +                ::core::ptr::null() as *const ::core::ffi::c_uchar,
> 
> Why unsigned char? I think this should be ::kernel::ffi::c_char to
> utilize the custom integer type definitions in kernel rather than the
> faulty ones in core.
> 
> (Maybe $crate rather than kernel)

Oops, replaced them with $crate::ffi::c_char