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@google.com Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Cu... Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Reviewed-by: Alice Ryhl aliceryhl@google.com Signed-off-by: Tamir Duberstein tamird@gmail.com
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)- }
+}