Re: [PATCH v10 2/5] rust: support formatting of foreign types

On Mon, May 26, 2025 at 10:48 AM Benno Lossin lossin@kernel.org wrote:

On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:

...

Introduce a `fmt!` macro which wraps all arguments in `kernel::fmt::Adapter` This enables formatting of foreign types (like `core::ffi::CStr`) that do not implement `fmt::Display` due to concerns around lossy conversions which do not apply in the kernel.

Replace all direct calls to `format_args!` with `fmt!`.

In preparation for replacing our `CStr` with `core::ffi::CStr`, move its `fmt::Display` implementation to `kernel::fmt::Adapter<&CStr>`.

Suggested-by: Alice Ryhl aliceryhl@google.com Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Cu... Signed-off-by: Tamir Duberstein tamird@gmail.com

---

drivers/block/rnull.rs | 2 +- rust/kernel/block/mq.rs | 2 +- rust/kernel/device.rs | 2 +- rust/kernel/fmt.rs | 77 +++++++++++++++++++++++++++++ rust/kernel/kunit.rs | 6 +-- rust/kernel/lib.rs | 1 + rust/kernel/prelude.rs | 3 +- rust/kernel/print.rs | 4 +- rust/kernel/seq_file.rs | 2 +- rust/kernel/str.rs | 23 ++++----- rust/macros/fmt.rs | 118 ++++++++++++++++++++++++++++++++++++++++++++ rust/macros/lib.rs | 19 +++++++ scripts/rustdoc_test_gen.rs | 2 +- 13 files changed, 235 insertions(+), 26 deletions(-)

Can you split this into creating the proc-macro, forwarding the display impls and replacing all the uses with the proc macro?

Can you help me understand why that's better?

...

diff --git a/drivers/block/rnull.rs b/drivers/block/rnull.rs index d07e76ae2c13..6366da12c5a5 100644 --- a/drivers/block/rnull.rs +++ b/drivers/block/rnull.rs @@ -51,7 +51,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> { .logical_block_size(4096)? .physical_block_size(4096)? .rotational(false)

•            .build(format_args!("rnullb{}", 0), tagset)
  

•            .build(fmt!("rnullb{}", 0), tagset)
     })();
  
     try_pin_init!(Self {
  

diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs index fb0f393c1cea..842be88aa1cf 100644 --- a/rust/kernel/block/mq.rs +++ b/rust/kernel/block/mq.rs @@ -82,7 +82,7 @@ //! Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?; //! let mut disk = gen_disk::GenDiskBuilder::new() //! .capacity_sectors(4096) -//! .build(format_args!("myblk"), tagset)?; +//! .build(fmt!("myblk"), tagset)?; //! //! # Ok::<(), kernel::error::Error>(()) //! ``` diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs index 5c372cf27ed0..99d99a76934c 100644 --- a/rust/kernel/device.rs +++ b/rust/kernel/device.rs @@ -240,7 +240,7 @@ impl DeviceContext for Normal {} macro_rules! dev_printk { ($method:ident, $dev:expr, $($f:tt)*) => { {

•        ($dev).$method(core::format_args!($($f)*));
  

•        ($dev).$method($crate::prelude::fmt!($($f)*));
     }
  

  }

} diff --git a/rust/kernel/fmt.rs b/rust/kernel/fmt.rs new file mode 100644 index 000000000000..12b08debc3b3 --- /dev/null +++ b/rust/kernel/fmt.rs @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: GPL-2.0

+//! Formatting utilities.

+use core::fmt;

+/// Internal adapter used to route allow implementations of formatting traits for foreign types. +/// +/// It is inserted automatically by the [`fmt!`] macro and is not meant to be used directly. +/// +/// [`fmt!`]: crate::prelude::fmt! +#[doc(hidden)] +pub struct Adapter<T>(pub T);

+macro_rules! impl_fmt_adapter_forward {

• ($($trait:ident),* $(,)?) => {

•    $(
  

•        impl<T: fmt::$trait> fmt::$trait for Adapter<T> {
  

•            fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
  

•                let Self(t) = self;
  

•                fmt::$trait::fmt(t, f)
  

•            }
  

•        }
  

•    )*
  

• };

+}

+impl_fmt_adapter_forward!(Debug, LowerHex, UpperHex, Octal, Binary, Pointer, LowerExp, UpperExp);

+macro_rules! impl_display_forward {

• ($(

•    $( { $($generics:tt)* } )? $ty:ty $( { where $($where:tt)* } )?
  

You don't need `{}` around the `where` clause, as a `where` keyword can follow a `ty` fragment.

This doesn't work: ``` error: local ambiguity when calling macro `impl_display_forward`: multiple parsing options: built-in NTs tt ('r#where') or 2 other options. --> rust/kernel/fmt.rs:75:78 | 75 | {<T: ?Sized>} crate::sync::Arc<T> where crate::sync::Arc<T>: fmt::Display, | ^ ```

...

• ),* $(,)?) => {

•    $(
  

•        impl$($($generics)*)? fmt::Display for Adapter<&$ty>
  

•        $(where $($where)*)? {
  

•            fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
  

•                let Self(t) = self;
  

•                fmt::Display::fmt(t, f)
  

•            }
  

•        }
  

•    )*
  

• };

+}

+impl<T: ?Sized> fmt::Display for Adapter<&&T> +where

• for<'a> Adapter<&'a T>: fmt::Display,

+{

• fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {

•    let Self(t) = self;
  

•    Adapter::<&T>(**t).fmt(f)
  

• }

+}

+impl_display_forward!(

• bool,
• char,
• core::panic::PanicInfo<'_>,
• crate::str::BStr,
• fmt::Arguments<'_>,
• i128,
• i16,
• i32,
• i64,
• i8,
• isize,
• str,
• u128,
• u16,
• u32,
• u64,
• u8,
• usize,
• {<T: ?Sized>} crate::sync::Arc<T> {where crate::sync::Arc<T>: fmt::Display},
• {<T: ?Sized>} crate::sync::UniqueArc<T> {where crate::sync::UniqueArc<T>: fmt::Display},

+);

If we use `{}` instead of `()`, then we can format the contents differently:

impl_display_forward! {
    i8, i16, i32, i64, i128, isize,
    u8, u16, u32, u64, u128, usize,
    bool, char, str,
    crate::str::BStr,
    fmt::Arguments<'_>,
    core::panic::PanicInfo<'_>,
    {<T: ?Sized>} crate::sync::Arc<T> {where Self: fmt::Display},
    {<T: ?Sized>} crate::sync::UniqueArc<T> {where Self: fmt::Display},
}

Is that formatting better? rustfmt refuses to touch it either way.

...

diff --git a/rust/macros/fmt.rs b/rust/macros/fmt.rs new file mode 100644 index 000000000000..6b6bd9295d18 --- /dev/null +++ b/rust/macros/fmt.rs @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: GPL-2.0

+use proc_macro::{Delimiter, Group, Ident, Punct, Spacing, Span, TokenStream, TokenTree}; +use std::collections::BTreeSet;

+/// Please see [`crate::fmt`] for documentation. +pub(crate) fn fmt(input: TokenStream) -> TokenStream {

• let mut input = input.into_iter();
• let first_opt = input.next();
• let first_owned_str;
• let mut names = BTreeSet::new();
• let first_lit = {

•    let Some((mut first_str, first_lit)) = (match first_opt.as_ref() {
  

•        Some(TokenTree::Literal(first_lit)) => {
  

•            first_owned_str = first_lit.to_string();
  

•            Some(first_owned_str.as_str()).and_then(|first| {
  

•                let first = first.strip_prefix('"')?;
  

•                let first = first.strip_suffix('"')?;
  

•                Some((first, first_lit))
  

•            })
  

•        }
  

•        _ => None,
  

•    }) else {
  

•        return first_opt.into_iter().chain(input).collect();
  

•    };
  

This usage of let-else + match is pretty confusing and could just be a single match statement.

I don't think so. Can you try rewriting it into the form you like?

...

•    while let Some((_, rest)) = first_str.split_once('{') {
  

•        first_str = rest;
  

•        if let Some(rest) = first_str.strip_prefix('{') {
  

•            first_str = rest;
  

•            continue;
  

•        }
  

•        while let Some((name, rest)) = first_str.split_once('}') {
  

•            first_str = rest;
  

•            if let Some(rest) = first_str.strip_prefix('}') {
  

This doesn't make sense, we've matched a `{`, some text and a `}`. You can't escape a `}` that is associated to a `{`.

Sure, but such input would be malformed, so I don't think it's necessary to handle it "perfectly". We'll get a nice error from format_args anyhow.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&a...

...

•                first_str = rest;
  

•                continue;
  

•            }
  

•            let name = name.split_once(':').map_or(name, |(name, _)| name);
  

•            if !name.is_empty() && !name.chars().all(|c| c.is_ascii_digit()) {
  

•                names.insert(name);
  

•            }
  

•            break;
  

•        }
  

•    }
  

•    first_lit
  

`first_lit` is not modified, so could we just the code above it into a block instead of keeping it in the expr for `first_lit`?

As above, can you suggest the alternate form you like better? The gymnastics here are all in service of being able to let malformed input fall through to core::format_args which will do the hard work of producing good diagnostics.

...

• };
• let first_span = first_lit.span();
• let adapt = |expr| {

•    let mut borrow =
  

•        TokenStream::from_iter([TokenTree::Punct(Punct::new('&', Spacing::Alone))]);
  

•    borrow.extend(expr);
  

•    make_ident(first_span, ["kernel", "fmt", "Adapter"])
  

•        .chain([TokenTree::Group(Group::new(Delimiter::Parenthesis, borrow))])
  

This should be fine with using `quote!`:

quote!(::kernel::fmt::Adapter(&#expr))

Yeah, I have a local commit that uses quote_spanned to remove all the manual constructions.

...

• };
• let flush = |args: &mut TokenStream, current: &mut TokenStream| {

•    let current = std::mem::take(current);
  

•    if !current.is_empty() {
  

•        args.extend(adapt(current));
  

•    }
  

• };
• let mut args = TokenStream::from_iter(first_opt);
• {

•    let mut current = TokenStream::new();
  

•    for tt in input {
  

•        match &tt {
  

•            TokenTree::Punct(p) => match p.as_char() {
  

•                ',' => {
  

•                    flush(&mut args, &mut current);
  

•                    &mut args
  

•                }
  

•                '=' => {
  

•                    names.remove(current.to_string().as_str());
  

•                    args.extend(std::mem::take(&mut current));
  

•                    &mut args
  

•                }
  

•                _ => &mut current,
  

•            },
  

•            _ => &mut current,
  

•        }
  

•        .extend([tt]);
  

•    }
  

This doesn't handle the following code correctly ):

let mut a = 0;
pr_info!("{a:?}", a = a = a);

Looks like we'll have to remember what "kind" of an equals we parsed...

Hmm, good point. Maybe we can just avoid dealing with `=` at all until we hit the `,` and just split on the leftmost `=`. WDYT? I'll have that in v11.

...

•    flush(&mut args, &mut current);
  

• }
• for name in names {

•    args.extend(
  

•        [
  

•            TokenTree::Punct(Punct::new(',', Spacing::Alone)),
  

•            TokenTree::Ident(Ident::new(name, first_span)),
  

•            TokenTree::Punct(Punct::new('=', Spacing::Alone)),
  

•        ]
  

•        .into_iter()
  

•        .chain(adapt(TokenTree::Ident(Ident::new(name, first_span)).into())),
  

•    );
  

This can probably be:

let name = Ident::new(name, first_span);
let value = adapt(name.clone());
args.extend(quote!(, #name = #value));

Indeed, see above - manual construction will be gone in v11.

...

• }
• TokenStream::from_iter(make_ident(first_span, ["core", "format_args"]).chain([

•    TokenTree::Punct(Punct::new('!', Spacing::Alone)),
  

•    TokenTree::Group(Group::new(Delimiter::Parenthesis, args)),
  

• ]))

This can be:

quote!(::core::format_args!(#args))

(not sure if you need `#(#args)*`)

...

+}

+fn make_ident<'a, T: IntoIterator<Item = &'a str>>(

• span: Span,
• names: T,

+) -> impl Iterator<Item = TokenTree> + use<'a, T> {

• names.into_iter().flat_map(move |name| {

•    [
  

•        TokenTree::Punct(Punct::new(':', Spacing::Joint)),
  

•        TokenTree::Punct(Punct::new(':', Spacing::Alone)),
  

•        TokenTree::Ident(Ident::new(name, span)),
  

•    ]
  

• })

+} diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index d31e50c446b0..fa956eaa3ba7 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -10,6 +10,7 @@ mod quote; mod concat_idents; mod export; +mod fmt; mod helpers; mod kunit; mod module; @@ -196,6 +197,24 @@ pub fn export(attr: TokenStream, ts: TokenStream) -> TokenStream { export::export(attr, ts) }

+/// Like [`core::format_args!`], but automatically wraps arguments in [`kernel::fmt::Adapter`]. +/// +/// This macro allows generating `core::fmt::Arguments` while ensuring that each argument is wrapped +/// with `::kernel::fmt::Adapter`, which customizes formatting behavior for kernel logging. +/// +/// Named arguments used in the format string (e.g. `{foo}`) are detected and resolved from local +/// bindings. All positional and named arguments are automatically wrapped. +/// +/// This macro is an implementation detail of other kernel logging macros like [`pr_info!`] and +/// should not typically be used directly. +/// +/// [`kernel::fmt::Adapter`]: ../kernel/fmt/struct.Adapter.html +/// [`pr_info!`]: ../kernel/macro.pr_info.html +#[proc_macro] +pub fn fmt(input: TokenStream) -> TokenStream {

I'm wondering if we should name this `format_args` instead in order to better communicate that it's a replacement for `core::format_args!`.

Unfortunately that introduces ambiguity in cases where kernel::prelude::* is imported because core::format_args is in core's prelude.

---

Cheers, Benno

...

• fmt::fmt(input)

+}

/// Concatenate two identifiers. /// /// This is useful in macros that need to declare or reference items with names diff --git a/scripts/rustdoc_test_gen.rs b/scripts/rustdoc_test_gen.rs index ec8d70ac888b..22ed9ee14053 100644 --- a/scripts/rustdoc_test_gen.rs +++ b/scripts/rustdoc_test_gen.rs @@ -197,7 +197,7 @@ macro_rules! assert_eq {{ // This follows the syntax for declaring test metadata in the proposed KTAP v2 spec, which may // be used for the proposed KUnit test attributes API. Thus hopefully this will make migration // easier later on.

• kernel::kunit::info(format_args!(" # {kunit_name}.location: {real_path}:{line}\n"));

• kernel::kunit::info(fmt!(" # {kunit_name}.location: {real_path}:{line}\n"));

  /// The anchor where the test code body starts. #[allow(unused)]