On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein wrote: > Introduce a `fmt!` macro which wraps all arguments in > `kernel::fmt::Adapter` and a `kernel::fmt::Display` trait. This enables > formatting of foreign types (like `core::ffi::CStr`) that do not > implement `core::fmt::Display` due to concerns around lossy conversions which > do not apply in the kernel. > > Replace all direct calls to `format_args!` with `fmt!`. > > Replace all implementations of `core::fmt::Display` with implementations > of `kernel::fmt::Display`. > > Suggested-by: Alice Ryhl <aliceryhl@xxxxxxxxxx> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467 > Acked-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > Reviewed-by: Alice Ryhl <aliceryhl@xxxxxxxxxx> > Signed-off-by: Tamir Duberstein <tamird@xxxxxxxxx> > --- > drivers/block/rnull.rs | 2 +- > drivers/gpu/nova-core/gpu.rs | 4 +- > rust/kernel/block/mq.rs | 2 +- > rust/kernel/device.rs | 2 +- > rust/kernel/fmt.rs | 89 +++++++++++++++++++++++++++++++++++++++ > 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 | 22 ++++------ > rust/macros/fmt.rs | 99 ++++++++++++++++++++++++++++++++++++++++++++ > rust/macros/lib.rs | 19 +++++++++ > rust/macros/quote.rs | 7 ++++ > scripts/rustdoc_test_gen.rs | 2 +- > 15 files changed, 236 insertions(+), 28 deletions(-) This would be a lot easier to review if he proc-macro and the call replacement were different patches. Also the `kernel/fmt.rs` file should be a different commit. > diff --git a/rust/kernel/fmt.rs b/rust/kernel/fmt.rs > new file mode 100644 > index 000000000000..348d16987de6 > --- /dev/null > +++ b/rust/kernel/fmt.rs > @@ -0,0 +1,89 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Formatting utilities. > + > +use core::fmt; I think we should pub export all types that we are still using from `core::fmt`. For example `Result`, `Formatter`, `Debug` etc. That way I can still use the same pattern of importing `fmt` and then writing impl fmt::Display for MyType { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {} } > + > +/// 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); > + > +/// A copy of [`fmt::Display`] that allows us to implement it for foreign types. > +/// > +/// Types should implement this trait rather than [`fmt::Display`]. Together with the [`Adapter`] > +/// type and [`fmt!`] macro, it allows for formatting foreign types (e.g. types from core) which do > +/// not implement [`fmt::Display`] directly. > +/// > +/// [`fmt!`]: crate::prelude::fmt! > +pub trait Display { > + /// Same as [`fmt::Display::fmt`]. > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result; > +} > + > +impl<T: ?Sized + Display> Display for &T { > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > + Display::fmt(*self, f) > + } > +} > + > +impl<T: ?Sized + Display> fmt::Display for Adapter<&T> { > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > + let Self(t) = self; > + Display::fmt(t, f) Why not `Display::fmt(&self.0, f)`? > + } > +} > + > +macro_rules! impl_display_forward { > + ($( > + $( { $($generics:tt)* } )? $ty:ty $( { where $($where:tt)* } )? > + ),* $(,)?) => { > + $( > + impl$($($generics)*)? Display for $ty $(where $($where)*)? { > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > + fmt::Display::fmt(self, f) > + } > + } > + )* > + }; > +} > + > +impl_display_forward!( > + bool, > + char, > + core::panic::PanicInfo<'_>, > + 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}, > +); > diff --git a/rust/macros/fmt.rs b/rust/macros/fmt.rs > new file mode 100644 > index 000000000000..edc37c220a89 > --- /dev/null > +++ b/rust/macros/fmt.rs > @@ -0,0 +1,99 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +use proc_macro::{Ident, 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)) You're only using first_lit to get the span later, so why not just get the span directly here? > + }) > + } > + _ => None, > + }) else { > + return first_opt.into_iter().chain(input).collect(); > + }; > + while let Some((_, rest)) = first_str.split_once('{') { Let's put a comment above this loop mentioning [1] and saying that it parses the identifiers from the format arguments. [1]: https://doc.rust-lang.org/std/fmt/index.html#syntax > + first_str = rest; > + if let Some(rest) = first_str.strip_prefix('{') { > + first_str = rest; > + continue; > + } > + if let Some((name, rest)) = first_str.split_once('}') { > + first_str = rest; > + 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); > + } > + } > + } > + first_lit > + }; > + > + let first_span = first_lit.span(); > + let adapter = quote_spanned! { > + first_span => ::kernel::fmt::Adapter > + }; I think we should follow the formatting convention from the quote crate: let adapter = quote_spanned!(first_span=> ::kernel::fmt::Adapter); > + > + let mut args = TokenStream::from_iter(first_opt); > + { > + let mut flush = |args: &mut TokenStream, current: &mut TokenStream| { You don't need to pass `args` as a closure argument, since you always call it with `&mut args`. > + let current = std::mem::take(current); > + if !current.is_empty() { > + let (lhs, rhs) = (|| { > + let mut current = current.into_iter(); > + let mut acc = TokenStream::new(); > + while let Some(tt) = current.next() { > + // Split on `=` only once to handle cases like `a = b = c`. > + if matches!(&tt, TokenTree::Punct(p) if p.as_char() == '=') { > + names.remove(acc.to_string().as_str()); > + // Include the `=` itself to keep the handling below uniform. > + acc.extend([tt]); > + return (Some(acc), current.collect::<TokenStream>()); > + } > + acc.extend([tt]); > + } > + (None, acc) > + })(); > + args.extend(quote_spanned! { > + first_span => #lhs #adapter(&#rhs) > + }); > + } > + }; > + > + let mut current = TokenStream::new(); Define this before the closure, then you don't need to pass it as an argument. --- Cheers, Benno > + for tt in input { > + match &tt { > + TokenTree::Punct(p) if p.as_char() == ',' => { > + flush(&mut args, &mut current); > + &mut args > + } > + _ => &mut current, > + } > + .extend([tt]); > + } > + flush(&mut args, &mut current); > + } > + > + for name in names { > + let name = Ident::new(name, first_span); > + args.extend(quote_spanned! { > + first_span => , #name = #adapter(&#name) > + }); > + } > + > + quote_spanned! { > + first_span => ::core::format_args!(#args) > + } > +}
