| Message ID | 20241210001802.228725-6-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rust: Add bug/warn abstractions | expand |
On Tue, Dec 10, 2024 at 1:19 AM FUJITA Tomonori <fujita.tomonori@gmail.com> 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 dynamically 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). > > ASM_REACHABLE definition works in the same way to get objtool's > reachable asm code. The architectures which use objtool (x86 and > loongarch) needs it. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > +#[macro_export] > +#[doc(hidden)] > +#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))] > +#[cfg(any(target_arch = "x86_64", target_arch = "riscv64"))] > +#[macro_export] > +#[doc(hidden)] > +#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))] > +#[cfg(any(target_arch = "aarch64", target_arch = "loongarch64"))] What's the reason for this arch-specific code? The file!()/line!() invocations? Could they be passed as an argument to the asm instead so that we don't need target_arch cfgs? I understand that they don't work exactly the same way, but maybe it could still work? > +#[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_char, > + line!() as i32, > + $flags as u32, > + ::core::ptr::null() as *const ::core::ffi::c_char, I wonder if this could be written to utilize Location::caller() instead so that `#[track_caller]` works? Alice
On Tue, 10 Dec 2024 10:05:07 +0100 Alice Ryhl <aliceryhl@google.com> wrote: > On Tue, Dec 10, 2024 at 1:19 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> 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 dynamically 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). >> >> ASM_REACHABLE definition works in the same way to get objtool's >> reachable asm code. The architectures which use objtool (x86 and >> loongarch) needs it. >> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > >> +#[macro_export] >> +#[doc(hidden)] >> +#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))] >> +#[cfg(any(target_arch = "x86_64", target_arch = "riscv64"))] > >> +#[macro_export] >> +#[doc(hidden)] >> +#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))] >> +#[cfg(any(target_arch = "aarch64", target_arch = "loongarch64"))] > > What's the reason for this arch-specific code? The file!()/line!() > invocations? Could they be passed as an argument to the asm instead so > that we don't need target_arch cfgs? I understand that they don't work > exactly the same way, but maybe it could still work? Because of "error: named argument never used" in Rust inline assembly: All the archs define ARCH_WARN_ASM macro in the same way: #define ARCH_WARN_ASM(file, line, flags, size) However, only x86 and risc-v asm code use the size argument. Without the cfgs, I'll get the following on arm64/loongarch: error: named argument never used --> /home/fujita/git/linux-rust/drivers/block/rnull.rs:54:9 | 54 | warn_on!(true); | ^^^^^^^^^^^^^^ named argument never used | = help: if this argument is intentionally unused, consider using it in an asm comment: `"/* {size} */"` = note: this error originates in the macro `$crate::warn_flags` which comes from the expansion of the macro `warn_on` (in Nightly builds, run with -Z macro-backtrace for more info) Any way to make the compiler to ignore this? >> +#[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_char, >> + line!() as i32, >> + $flags as u32, >> + ::core::ptr::null() as *const ::core::ffi::c_char, > > I wonder if this could be written to utilize Location::caller() > instead so that `#[track_caller]` works? You meant that we could make warn_flags() function instead of macro with Location::caller()? If so, we need to add cfgs to warn_on and warn_on_once because both macro and function of warn_flags are necessary?
On Wed, Dec 11, 2024 at 1:41 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > On Tue, 10 Dec 2024 10:05:07 +0100 > Alice Ryhl <aliceryhl@google.com> wrote: > > > On Tue, Dec 10, 2024 at 1:19 AM FUJITA Tomonori > > <fujita.tomonori@gmail.com> 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 dynamically 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). > >> > >> ASM_REACHABLE definition works in the same way to get objtool's > >> reachable asm code. The architectures which use objtool (x86 and > >> loongarch) needs it. > >> > >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > > > >> +#[macro_export] > >> +#[doc(hidden)] > >> +#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))] > >> +#[cfg(any(target_arch = "x86_64", target_arch = "riscv64"))] > > > >> +#[macro_export] > >> +#[doc(hidden)] > >> +#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))] > >> +#[cfg(any(target_arch = "aarch64", target_arch = "loongarch64"))] > > > > What's the reason for this arch-specific code? The file!()/line!() > > invocations? Could they be passed as an argument to the asm instead so > > that we don't need target_arch cfgs? I understand that they don't work > > exactly the same way, but maybe it could still work? > > Because of "error: named argument never used" in Rust inline assembly: > > All the archs define ARCH_WARN_ASM macro in the same way: > > #define ARCH_WARN_ASM(file, line, flags, size) > > However, only x86 and risc-v asm code use the size argument. Without > the cfgs, I'll get the following on arm64/loongarch: > > error: named argument never used > --> /home/fujita/git/linux-rust/drivers/block/rnull.rs:54:9 > | > 54 | warn_on!(true); > | ^^^^^^^^^^^^^^ named argument never used > | > = help: if this argument is intentionally unused, consider using it in an asm comment: `"/* {size} */"` > = note: this error originates in the macro `$crate::warn_flags` which comes from the expansion of the macro `warn_on` (in Nightly builds, run with -Z macro-backtrace for more info) > > > Any way to make the compiler to ignore this? The error message suggests adding an asm comment. Does that not work? You could even add the comment unconditionally on the Rust side. It's not like the comment hurts on the platforms that *do* use the size parameter. > >> +#[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_char, > >> + line!() as i32, > >> + $flags as u32, > >> + ::core::ptr::null() as *const ::core::ffi::c_char, > > > > I wonder if this could be written to utilize Location::caller() > > instead so that `#[track_caller]` works? > > You meant that we could make warn_flags() function instead of macro > with Location::caller()? > > If so, we need to add cfgs to warn_on and warn_on_once because both macro > and function of warn_flags are necessary? Well, I'm not sure! I don't know if it's feasible at all, since using Location::caller() would mean that the file/line is not a compile-time constant. But if we can, then I think #[track_caller] support would be nice. Alice
On Wed, 11 Dec 2024 10:48:27 +0100 Alice Ryhl <aliceryhl@google.com> wrote: >> >> +#[macro_export] >> >> +#[doc(hidden)] >> >> +#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))] >> >> +#[cfg(any(target_arch = "x86_64", target_arch = "riscv64"))] >> > >> >> +#[macro_export] >> >> +#[doc(hidden)] >> >> +#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))] >> >> +#[cfg(any(target_arch = "aarch64", target_arch = "loongarch64"))] >> > >> > What's the reason for this arch-specific code? The file!()/line!() >> > invocations? Could they be passed as an argument to the asm instead so >> > that we don't need target_arch cfgs? I understand that they don't work >> > exactly the same way, but maybe it could still work? >> >> Because of "error: named argument never used" in Rust inline assembly: >> >> All the archs define ARCH_WARN_ASM macro in the same way: >> >> #define ARCH_WARN_ASM(file, line, flags, size) >> >> However, only x86 and risc-v asm code use the size argument. Without >> the cfgs, I'll get the following on arm64/loongarch: >> >> error: named argument never used >> --> /home/fujita/git/linux-rust/drivers/block/rnull.rs:54:9 >> | >> 54 | warn_on!(true); >> | ^^^^^^^^^^^^^^ named argument never used >> | >> = help: if this argument is intentionally unused, consider using it in an asm comment: `"/* {size} */"` >> = note: this error originates in the macro `$crate::warn_flags` which comes from the expansion of the macro `warn_on` (in Nightly builds, run with -Z macro-backtrace for more info) >> >> >> Any way to make the compiler to ignore this? > > The error message suggests adding an asm comment. Does that not work? Ah, works! I didn't understand what "an asm comment" is.. > You could even add the comment unconditionally on the Rust side. It's > not like the comment hurts on the platforms that *do* use the size > parameter. Yeah, I confirmed that this also works for the architectures that do use the size. I'll remove the target_arch cfg in v2. Thanks! >> >> +#[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_char, >> >> + line!() as i32, >> >> + $flags as u32, >> >> + ::core::ptr::null() as *const ::core::ffi::c_char, >> > >> > I wonder if this could be written to utilize Location::caller() >> > instead so that `#[track_caller]` works? >> >> You meant that we could make warn_flags() function instead of macro >> with Location::caller()? >> >> If so, we need to add cfgs to warn_on and warn_on_once because both macro >> and function of warn_flags are necessary? > > Well, I'm not sure! I don't know if it's feasible at all, since using > Location::caller() would mean that the file/line is not a compile-time > constant. But if we can, then I think #[track_caller] support would be > nice. I think that using Location::caller() makes the code complicated (needs more cfgs for UML). All the callers (warn_* and bug_*) of warn_flags and warn_flags itself must be macro for all the archs execpt for UML. We don't want to have the UML version of all the callers.
diff --git a/rust/Makefile b/rust/Makefile index 9da9042fd627..80c9c6b3e491 100644 --- a/rust/Makefile +++ b/rust/Makefile @@ -37,6 +37,9 @@ obj-$(CONFIG_RUST_KERNEL_DOCTESTS) += doctests_kernel_generated.o obj-$(CONFIG_RUST_KERNEL_DOCTESTS) += doctests_kernel_generated_kunit.o always-$(subst y,$(CONFIG_RUST),$(CONFIG_JUMP_LABEL)) += kernel/generated_arch_static_branch_asm.rs +ifndef CONFIG_UML +always-$(subst y,$(CONFIG_RUST),$(CONFIG_BUG)) += kernel/generated_arch_warn_asm.rs kernel/generated_arch_reachable_asm.rs +endif # Avoids running `$(RUSTC)` for the sysroot when it may not be available. ifdef CONFIG_RUST @@ -436,5 +439,10 @@ $(obj)/kernel.o: $(src)/kernel/lib.rs $(obj)/build_error.o \ ifdef CONFIG_JUMP_LABEL $(obj)/kernel.o: $(obj)/kernel/generated_arch_static_branch_asm.rs endif +ifndef CONFIG_UML +ifdef CONFIG_BUG +$(obj)/kernel.o: $(obj)/kernel/generated_arch_warn_asm.rs $(obj)/kernel/generated_arch_reachable_asm.rs +endif +endif endif # CONFIG_RUST 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 diff --git a/rust/kernel/bug.rs b/rust/kernel/bug.rs new file mode 100644 index 000000000000..8959a2e94971 --- /dev/null +++ b/rust/kernel/bug.rs @@ -0,0 +1,131 @@ +// SPDX-License-Identifier: GPL-2.0 + +// Copyright (C) 2024 FUJITA Tomonori + +//! Support for BUG_* and WARN_* functionality. +//! +//! C header: [`include/asm-generic/bug.h`](srctree/include/asm-generic/bug.h) + +#[macro_export] +#[doc(hidden)] +#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))] +#[cfg(any(target_arch = "x86_64", target_arch = "riscv64"))] +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!( + ".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!( + 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>(), + ); + } + } +} + +#[macro_export] +#[doc(hidden)] +#[cfg(all(CONFIG_BUG, not(CONFIG_UML)))] +#[cfg(any(target_arch = "aarch64", target_arch = "loongarch64"))] +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!( + ".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, + ); + } + // SAFETY: Just an FFI call. + #[cfg(not(CONFIG_DEBUG_BUGVERBOSE))] + unsafe { + $crate::asm!(concat!( + 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, + ); + } + } +} + +#[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_char, + line!() as i32, + $flags as u32, + ::core::ptr::null() as *const ::core::ffi::c_char, + ); + } + }; +} + +#[macro_export] +#[doc(hidden)] +#[cfg(not(CONFIG_BUG))] +macro_rules! warn_flags { + ($flags:expr) => {}; +} + +#[doc(hidden)] +#[macro_export] +macro_rules! bugflag_taint { + ($taint:expr) => { + $taint << 8 + }; +} + +/// 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) + ); + } + $cond + }; +} + +/// Report a warning. +#[macro_export] +macro_rules! warn_on { + ($cond:expr) => { + if $cond { + $crate::warn_flags!($crate::bugflag_taint!($crate::bindings::TAINT_WARN)); + } + $cond + }; +} diff --git a/rust/kernel/generated_arch_reachable_asm.rs.S b/rust/kernel/generated_arch_reachable_asm.rs.S new file mode 100644 index 000000000000..4e65525e4464 --- /dev/null +++ b/rust/kernel/generated_arch_reachable_asm.rs.S @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#include <linux/objtool.h> + +// Cut here. + +::kernel::concat_literals!(ASM_REACHABLE) diff --git a/rust/kernel/generated_arch_warn_asm.rs.S b/rust/kernel/generated_arch_warn_asm.rs.S new file mode 100644 index 000000000000..8f239b431aa4 --- /dev/null +++ b/rust/kernel/generated_arch_warn_asm.rs.S @@ -0,0 +1,7 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#include <linux/bug.h> + +// Cut here. + +::kernel::concat_literals!(ARCH_WARN_ASM("111b", "{line}", "{flags}", "{size}")) diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index e1065a7551a3..913f3a5b2a98 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -32,6 +32,7 @@ pub mod alloc; #[cfg(CONFIG_BLOCK)] pub mod block; +pub mod bug; mod build_assert; pub mod cred; pub mod device;
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 dynamically 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). ASM_REACHABLE definition works in the same way to get objtool's reachable asm code. The architectures which use objtool (x86 and loongarch) needs it. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/Makefile | 8 ++ rust/kernel/.gitignore | 2 + rust/kernel/bug.rs | 131 ++++++++++++++++++ rust/kernel/generated_arch_reachable_asm.rs.S | 7 + rust/kernel/generated_arch_warn_asm.rs.S | 7 + rust/kernel/lib.rs | 1 + 6 files changed, 156 insertions(+) create mode 100644 rust/kernel/bug.rs create mode 100644 rust/kernel/generated_arch_reachable_asm.rs.S create mode 100644 rust/kernel/generated_arch_warn_asm.rs.S