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 <redacted>
---
 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?

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},
    }

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)*`)

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