Re: [PATCH v3 5/5] rust: Add warn_on and warn_on_once

[Alice Ryhl <aliceryhl@google.com>]

On Wed, Mar 05, 2025 at 07:24:03PM +0900, FUJITA Tomonori wrote:
> On Wed, 5 Mar 2025 08:42:57 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
> > On Thu, Feb 13, 2025 at 10:57:59PM +0900, FUJITA Tomonori wrote:
> >> Add warn_on and warn_on_once macros. Wrapping the C's WARN_* and BUG_*
> >> macros doesn't work so this uses the assembly code exported by the C
> >> side via ARCH_WARN_ASM macro. Like the static branch code, this
> >> generates the assembly code for rust at compile time by using the C
> >> preprocessor.
> >> 
> >> file()! macro doesn't work for the Rust inline assembly in the same
> >> way as __FILE__ for the C inline assembly. So the code to handle a
> >> file name is different from the C assembly code (similar to the
> >> arm64/loongarch assembly).
> > 
> > Nit: Should be file!() not file()!.
> 
> Ops, thanks.
> 
> Actually, the above comment is obsolete. With your solution in the
> previous mail, I can remove the asm code for the file name. I'll
> remove the comment.
> 
> 
> >> diff --git a/rust/kernel/.gitignore b/rust/kernel/.gitignore
> >> index 6ba39a178f30..f1d7f4225332 100644
> >> --- a/rust/kernel/.gitignore
> >> +++ b/rust/kernel/.gitignore
> >> @@ -1,3 +1,5 @@
> >>  # SPDX-License-Identifier: GPL-2.0
> >>  
> >>  /generated_arch_static_branch_asm.rs
> >> +/generated_arch_warn_asm.rs
> >> +/generated_arch_reachable_asm.rs
> >> \ No newline at end of file
> > 
> > There should be a newline.
> 
> Ah, I'll fix.
> 
> >> +++ b/rust/kernel/bug.rs
> >> @@ -0,0 +1,100 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +// Copyright (C) 2024 FUJITA Tomonori
> > 
> > 2025?
> 
> I'll add.
> 
> >> +#[macro_export]
> >> +#[doc(hidden)]
> >> +#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))]
> >> +macro_rules! warn_flags {
> >> +    ($flags:expr) => {
> >> +        const FLAGS: u32 = $crate::bindings::BUGFLAG_WARNING | $flags;
> >> +        // SAFETY: Just an FFI call.
> >> +        #[cfg(CONFIG_DEBUG_BUGVERBOSE)]
> >> +        unsafe {
> >> +            $crate::asm!(concat!(
> >> +                "/* {size} */",
> >> +                ".pushsection .rodata.str1.1, \"aMS\",@progbits, 1\n",
> >> +                "111:\t .string ", "\"", file!(), "\"\n",
> >> +                ".popsection\n",
> >> +                include!(concat!(env!("OBJTREE"), "/rust/kernel/generated_arch_warn_asm.rs")),
> >> +                include!(concat!(env!("OBJTREE"), "/rust/kernel/generated_arch_reachable_asm.rs")));
> >> +            line = const line!(),
> >> +            flags = const FLAGS,
> >> +            size = const ::core::mem::size_of::<$crate::bindings::bug_entry>(),
> >> +            );
> >> +        }
> >> +        // SAFETY: Just an FFI call.
> >> +        #[cfg(not(CONFIG_DEBUG_BUGVERBOSE))]
> >> +        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>(),
> >> +            );
> >> +        }
> > 
> > I generally prefer to have the cfgs on the macro rather in its
> > expansion. That avoids emitting a lot of code that is not actually used.
> 
> You prefer the following?
> 
> #[cfg(all(CONFIG_BUG, CONFIG_DEBUG_BUGVERBOSE, not(CONFIG_UML)))]
> macro_rules! warn_flags {
> ...
> }
> 
> #[cfg(all(CONFIG_BUG, not(CONFIG_DEBUG_BUGVERBOSE), not(CONFIG_UML)))]
> macro_rules! warn_flags {
> ...
> }

In this case it probably reads better as

#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))]
#[cfg(CONFIG_DEBUG_BUGVERBOSE)]
macro_rules! warn_flags {
...
}

#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))]
#[cfg(not(CONFIG_DEBUG_BUGVERBOSE))]
macro_rules! warn_flags {
...
}

but yes.

> >> +#[doc(hidden)]
> >> +#[macro_export]
> >> +macro_rules! bugflag_taint {
> >> +    ($taint:expr) => {
> >> +        $taint << 8
> >> +    };
> >> +}
> > 
> > This could just be a const fn.
> 
> Yeah, would a const fn be preferable?

Yes, I think a constant or const fn is preferable over a macro whenever
possible.

> >> +/// Report a warning only once.
> >> +#[macro_export]
> >> +macro_rules! warn_on_once {
> >> +    ($cond:expr) => {
> >> +        if $cond {
> >> +            $crate::warn_flags!(
> >> +                $crate::bindings::BUGFLAG_ONCE
> >> +                    | $crate::bugflag_taint!($crate::bindings::TAINT_WARN)
> > 
> > Or maybe a constant?
> > 
> > const WARN_ON_ONCE_FLAGS: u32 = bindings::BUGFLAG_ONCE | (bindings::TAINT_WARN << 8);
> 
> Ok, but you prefer "<< 8" than using const fn bugflag_taint()?

I'm also happy with
const WARN_ON_ONCE_FLAGS: u32 = bindings::BUGFLAG_ONCE | bugflag_taint(bindings::TAINT_WARN);

Up to you.

Alice