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

["Benno Lossin" <lossin@kernel.org>]

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/Custom.20formatting/with/516476467
> 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?

> 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.

> +    ),* $(,)?) => {
> +        $(
> +            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},
    }

> 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.

> +        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 `{`.

> +                    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`?

> +    };
> +
> +    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))

> +    };
> +
> +    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...

> +        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));

> +    }
> +
> +    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!`.

---
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)]