Benno Lossin
2025-May-26 14:48 UTC
[PATCH v10 2/5] rust: support formatting of foreign types
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 at google.com> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467 > Signed-off-by: Tamir Duberstein <tamird at 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)]
Tamir Duberstein
2025-May-26 22:18 UTC
[PATCH v10 2/5] rust: support formatting of foreign types
On Mon, May 26, 2025 at 10:48?AM Benno Lossin <lossin at 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 at google.com> > > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467 > > Signed-off-by: Tamir Duberstein <tamird at 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)] >
Benno Lossin
2025-May-26 23:01 UTC
[PATCH v10 2/5] rust: support formatting of foreign types
On Tue May 27, 2025 at 12:17 AM CEST, Tamir Duberstein wrote:> On Mon, May 26, 2025 at 10:48?AM Benno Lossin <lossin at 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 at google.com> >> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467 >> > Signed-off-by: Tamir Duberstein <tamird at 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?It makes reviewing significantly easier.>> > +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, > | > ^ > ```Ah right that's a shame, forgot about the `tt`s at the end...>> > +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.Yeah rustfmt doesn't touch macro parameters enclosed in `{}`. I think it's better.>> > +/// 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?let (mut first_str, first_lit) match first_opt.as_ref() { Some(TokenTree::Literal(lit)) if lit.to_string().starts_with('"') => { let contents = lit.to_string(); let contents = contents.strip_prefix('"').unwrap().strip_suffix('"').unwrap(); ((contents, lit)) } _ => return first_opt.into_iter().chain(input).collect(), };>> > + 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.My suggestion in this case would be to just remove this if-let. The search for `{` above would skip the `}` if it's correct.> https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=5f529d93da7cf46b3c99ba7772623e33Yes it will error like that, but if we do the replacement only when the syntax is correct, there also will be compile errors because of a missing `Display` impl, or is that not the case? I'm a bit concerned about the ergonomics that this change will introduce, but I guess there really isn't anything that we can do about except not do it.>> > + 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.I don't see how this is hard, just do: let (first_str, first_lit) = ...; while ...>> > + }; >> > + >> > + 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.I don't think that you need `quote_spanned` here at all. If you do, then let me know, something weird with spans is going on then.>> > + }; >> > + >> > + 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.Sounds good, if there is no `=`, then ignore it.>> > +/// 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.Ahh that's unfortunate. --- Cheers, Benno
Alice Ryhl
2025-May-27 12:44 UTC
[PATCH v10 2/5] rust: support formatting of foreign types
On Tue, May 27, 2025 at 12:18?AM Tamir Duberstein <tamird at gmail.com> wrote:> > > +} > > > + > > > +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.I'm pretty sure that glob imports are higher priority than the core prelude? Or is this because there are macros that now incorrectly use kernel::prelude::format_args when they should use the one from core? Alice