Re: [PATCH v3 4/5] rust: print: add dynamic debug support to pr_debug!

[Zhi Wang <zhiw@nvidia.com>]

On Fri, 16 Oct 2026 08:50:44 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

Hey Alice,

Thanks for the detailed review. My comments are below.

> On Thu, Oct 01, 2026 at 05:59:39PM +0300, Zhi Wang wrote:
> > +/// A dynamic debug callsite descriptor.
> > +///
> > +/// The [`crate::dynamic_debug!`] macro places descriptors at
> > stable addresses in `__dyndbg`. +#[repr(transparent)]
> > +pub struct Descriptor {
> > +    /// The C descriptor, exposed for static-branch field
> > addressing.
> > +    pub inner: bindings::_ddebug,
> > +}
> 
> Should this be #[repr(C, align(8))] to match how C adds __aligned(8)
> in DEFINE_DYNAMIC_DEBUG_METADATA_CLS?
> 

IMHO, I guess this should be already aligned(8)， looks like the
repr(transparent) also inherit the alignment from the bindings? [1] I
agree making it explicit looking better for readability.

[1]
https://doc.rust-lang.org/reference/type-layout.html#r-layout.repr.transparent.layout-abi

> > +impl Descriptor {
> > +    /// Creates an initially disabled descriptor.
> > +    pub const fn new(
> > +        module: &'static CStr,
> > +        file: &'static CStr,
> > +        format: &'static CStr,
> > +        line: u32,
> > +    ) -> Self {

...snip...

> > +        Self { inner: descriptor }
> > +    }
> 
> Two comments here:
> 
> 1. Instead of hardcoding `c"<unknown>"` for `descriptor.function`, how
>    about passing `$crate::c_str!(module_path!())` from
> `dynamic_debug!` (an idea from Thomas Weißschuh's RFC series [1])?
> Even though Rust doesn't have a `function_name!()` macro, recording
> `module_path!()` makes the `+f` prefix flag useful and allows `func`
> queries in `/proc/dynamic_debug/control` to filter by Rust module
> path within a multi-file crate.
> 
>    [1]
> https://lore.kernel.org/rust-for-linux/20261011-rust-dyndbg-v1-8-832e4039b71f@weissschuh.net/
> 
> 2. The inline bitfield manipulation and `unsafe { core::mem::zeroed()
> }` is pretty ugly. Please use the same strategy as `FamilyFlags` in
>    `rust/kernel/net/netlink.rs` to handle bitfields in `const`
> context:
> 
>    - Move the bitfield construction into a dedicated helper struct
> with a `const fn into_bitfield(self) ->
> bindings::__BindgenBitfieldUnit<[u8; 4]>` method.
>    - Construct `bindings::_ddebug` directly using struct literal
> syntax with `..pin_init::zeroed()`, avoiding `unsafe {
> core::mem::zeroed() }` and field-by-field mutation.
>    - Add a kunit test (like `test_family_flags_bitfield`) that
> verifies `into_bitfield()` against bindgen's generated `set_lineno`,
>      `set_class_id`, and `set_flags` (or
> `bindings::_ddebug::new_bitfield_1`).
> 

Thanks so much for the detailed suggestions! I will try to see how to
cope these with the combined patches with Thomas. 

> > +    /// Prints arguments with the crate's log prefix and the
> > dynamic debug prefixes.

...snip...

> > +macro_rules! dynamic_debug {
> > +    ($descriptor:ident, ($($arg:tt)*), $body:block) => {{
> > +        // Keep item names out of the user's scope, including
> > implicit format captures.
> > +        let (descriptor, enabled) = {
> > +            // `used(compiler)` avoids `SHF_GNU_RETAIN`, which the
> > module loader interprets as
> > +            // `SHF_RO_AFTER_INIT`.
> > +            #[used(compiler)]
> > +            #[unsafe(link_section = "__dyndbg")]
> > +            static mut DESCRIPTOR:
> > $crate::print::dynamic_debug::Descriptor =
> > +                $crate::print::dynamic_debug::Descriptor::new(
> > +                    $crate::c_str!(env!("RUST_MODNAME")),
> > +                    $crate::c_str!(file!()),
> > +                    $crate::dynamic_debug_format!($($arg)*),
> > +                    line!(),
> > +                );
> 
> Is `#[used(compiler)]` needed here at all?
> 
> When the `pr_debug!` / `dev_dbg!` statement is reachable, `DESCRIPTOR`
> is already referenced by `static_branch_unlikely!(DESCRIPTOR, ...)`
> and by `&raw mut DESCRIPTOR` passed to `Descriptor::print`. And if the
> callsite is optimized out (for example, inside an unused function or
> dead branch), we *want* the static `DESCRIPTOR` to be deleted as well
> rather than leaving an orphan entry in `__dyndbg`. This would match
> C's `DEFINE_DYNAMIC_DEBUG_METADATA_CLS`, which does not use `__used`.
> 
> For instance, with `#[used(compiler)]` present, the uncalled `fn
> example` in the `dev_dbg!` doctest still emits a dead `DESCRIPTOR`
> into `vmlinux`'s `__dyndbg` section.
> 

I see. Thanks for the detailed explanation. I am thinking if we can
remove #[used] here? This is originally from Andrew's patch and causes
kernel write fault when I try to enable the dynamic debug prints. I
also see Thomas' patch is using used(compiler) [2], guess he might
encounter the same problem.

My problem is my rustc/LLVM (rustc 1.94.0/LLVM 21.1.8) generates this
symbol as SHF_GNU_RETAIN in the .ko when with #[used] and Linux kernel
module loader explains this to SHF_RO_AFTER_INIT, then write fault is
triggered when enabling the print...

[2]
https://lore.kernel.org/rust-for-linux/20261011-rust-dyndbg-v1-8-832e4039b71f@weissschuh.net/

> > +/// Stub for host doctests, matching [`crate::print_macro!`].
> > +#[cfg(testlib)]
> > +#[macro_export]
> > +macro_rules! dynamic_debug {
> > +    ($descriptor:ident, ($($arg:tt)*), $body:block) => {
> > +        ()
> > +    };
> > +}
> 
> If `dynamic_debug!` drops `$body` under `#[cfg(testlib)]`, any
> variables used only inside `pr_debug!` or `dev_dbg!` will trigger
> `unused_variables` warnings under `testlib`, and `$body` won't be
> type-checked. How about:
> 
>     if false {
>         let $descriptor: *mut
> $crate::print::dynamic_debug::Descriptor = ::core::ptr::null_mut();
>         $body
>     }
> 
> Alice