On Mon, May 26, 2025 at 10:48 AM Benno Lossin <los...@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 <alicer...@google.com>
> > Link:
> > https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
> > Signed-off-by: Tamir Duberstein <tam...@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&gist=5f529d93da7cf46b3c99ba7772623e33
>
> > + 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)]
>
