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 <tamird@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(-)
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)* } )?
+ ),* $(,)?) => {
+ $(
+ 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},
+);
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index 1604fb6a5b1b..c29e34192553 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -72,14 +72,14 @@ macro_rules! kunit_assert {
// mistake (it is hidden to prevent that).
//
// This mimics KUnit's failed assertion format.
- $crate::kunit::err(format_args!(
+ $crate::kunit::err($crate::prelude::fmt!(
" # {}: ASSERTION FAILED at {FILE}:{LINE}\n",
$name
));
- $crate::kunit::err(format_args!(
+ $crate::kunit::err($crate::prelude::fmt!(
" Expected {CONDITION} to be true, but is false\n"
));
- $crate::kunit::err(format_args!(
+ $crate::kunit::err($crate::prelude::fmt!(
" Failure not reported to KUnit since this is a non-KUnit task\n"
));
break 'out;
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6e9287136cac..ec48c818d512 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -66,6 +66,7 @@
pub mod faux;
#[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
pub mod firmware;
+pub mod fmt;
pub mod fs;
pub mod init;
pub mod io;
diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index baa774a351ce..ef1efcb9d945 100644
--- a/rust/kernel/prelude.rs
+++ b/rust/kernel/prelude.rs
@@ -17,7 +17,7 @@
pub use crate::alloc::{flags::*, Box, KBox, KVBox, KVVec, KVec, VBox, VVec, Vec};
#[doc(no_inline)]
-pub use macros::{export, module, vtable};
+pub use macros::{export, fmt, module, vtable};
pub use pin_init::{init, pin_data, pin_init, pinned_drop, InPlaceWrite, Init, PinInit, Zeroable};
@@ -26,7 +26,6 @@
// `super::std_vendor` is hidden, which makes the macro inline for some reason.
#[doc(no_inline)]
pub use super::dbg;
-pub use super::fmt;
pub use super::{dev_alert, dev_crit, dev_dbg, dev_emerg, dev_err, dev_info, dev_notice, dev_warn};
pub use super::{pr_alert, pr_crit, pr_debug, pr_emerg, pr_err, pr_info, pr_notice, pr_warn};
diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
index 9783d960a97a..0f5e15128005 100644
--- a/rust/kernel/print.rs
+++ b/rust/kernel/print.rs
@@ -149,7 +149,7 @@ macro_rules! print_macro (
// takes borrows on the arguments, but does not extend the scope of temporaries.
// Therefore, a `match` expression is used to keep them around, since
// the scrutinee is kept until the end of the `match`.
- match format_args!($($arg)+) {
+ match $crate::prelude::fmt!($($arg)+) {
// SAFETY: This hidden macro should only be called by the documented
// printing macros which ensure the format string is one of the fixed
// ones. All `__LOG_PREFIX`s are null-terminated as they are generated
@@ -168,7 +168,7 @@ macro_rules! print_macro (
// The `CONT` case.
($format_string:path, true, $($arg:tt)+) => (
$crate::print::call_printk_cont(
- format_args!($($arg)+),
+ $crate::prelude::fmt!($($arg)+),
);
);
);
diff --git a/rust/kernel/seq_file.rs b/rust/kernel/seq_file.rs
index 7a9403eb6e5b..627bc2f7b3d2 100644
--- a/rust/kernel/seq_file.rs
+++ b/rust/kernel/seq_file.rs
@@ -47,7 +47,7 @@ pub fn call_printf(&self, args: core::fmt::Arguments<'_>) {
#[macro_export]
macro_rules! seq_print {
($m:expr, $($arg:tt)+) => (
- $m.call_printf(format_args!($($arg)+))
+ $m.call_printf($crate::prelude::fmt!($($arg)+))
);
}
pub use seq_print;
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 98d5c74ec4f7..302423ca5eb0 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -54,7 +54,7 @@ impl fmt::Display for BStr {
/// Formats printable ASCII characters, escaping the rest.
///
/// ```
- /// # use kernel::{fmt, b_str, str::{BStr, CString}};
+ /// # use kernel::{prelude::fmt, b_str, str::{BStr, CString}};
/// let ascii = b_str!("Hello, BStr!");
/// let s = CString::try_from_fmt(fmt!("{}", ascii))?;
/// assert_eq!(s.as_bytes(), "Hello, BStr!".as_bytes());
@@ -85,7 +85,7 @@ impl fmt::Debug for BStr {
/// escaping the rest.
///
/// ```
- /// # use kernel::{fmt, b_str, str::{BStr, CString}};
+ /// # use kernel::{prelude::fmt, b_str, str::{BStr, CString}};
/// // Embedded double quotes are escaped.
/// let ascii = b_str!("Hello, \"BStr\"!");
/// let s = CString::try_from_fmt(fmt!("{:?}", ascii))?;
@@ -424,12 +424,12 @@ pub fn to_ascii_uppercase(&self) -> Result<CString, AllocError> {
}
}
-impl fmt::Display for CStr {
+impl fmt::Display for crate::fmt::Adapter<&CStr> {
/// Formats printable ASCII characters, escaping the rest.
///
/// ```
/// # use kernel::c_str;
- /// # use kernel::fmt;
+ /// # use kernel::prelude::fmt;
/// # use kernel::str::CStr;
/// # use kernel::str::CString;
/// let penguin = c_str!("🐧");
@@ -442,7 +442,8 @@ impl fmt::Display for CStr {
/// # Ok::<(), kernel::error::Error>(())
/// ```
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
- for &c in self.as_bytes() {
+ let Self(cstr) = self;
+ for &c in cstr.as_bytes() {
if (0x20..0x7f).contains(&c) {
// Printable character.
f.write_char(c as char)?;
@@ -459,7 +460,7 @@ impl fmt::Debug for CStr {
///
/// ```
/// # use kernel::c_str;
- /// # use kernel::fmt;
+ /// # use kernel::prelude::fmt;
/// # use kernel::str::CStr;
/// # use kernel::str::CString;
/// let penguin = c_str!("🐧");
@@ -595,7 +596,7 @@ fn deref(&self) -> &str {
macro_rules! format {
($($f:tt)*) => ({
- &*String::from_fmt(kernel::fmt!($($f)*))
+ &*String::from_fmt(crate::prelude::fmt!($($f)*))
})
}
@@ -850,7 +851,7 @@ fn write_str(&mut self, s: &str) -> fmt::Result {
/// # Examples
///
/// ```
-/// use kernel::{str::CString, fmt};
+/// use kernel::{str::CString, prelude::fmt};
///
/// let s = CString::try_from_fmt(fmt!("{}{}{}", "abc", 10, 20))?;
/// assert_eq!(s.as_bytes_with_nul(), "abc1020\0".as_bytes());
@@ -940,9 +941,3 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt::Debug::fmt(&**self, f)
}
}
-
-/// A convenience alias for [`core::format_args`].
-#[macro_export]
-macro_rules! fmt {
- ($($f:tt)*) => ( core::format_args!($($f)*) )
-}
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();
+ };
+ 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('}') {
+ 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
+ };
+
+ 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))])
+ };
+
+ 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]);
+ }
+ 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())),
+ );
+ }
+
+ TokenStream::from_iter(make_ident(first_span, ["core", "format_args"]).chain([
+ TokenTree::Punct(Punct::new('!', Spacing::Alone)),
+ TokenTree::Group(Group::new(Delimiter::Parenthesis, 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 {
+ 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)]
--
2.49.0
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 <tamird@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)]
On Mon, May 26, 2025 at 10:48 AM Benno Lossin <lossin@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@google.com>
> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
> > Signed-off-by: Tamir Duberstein <tamird@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)]
>
On Tue, May 27, 2025 at 12:18 AM Tamir Duberstein <tamird@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
On Tue, May 27, 2025 at 8:44 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Tue, May 27, 2025 at 12:18 AM Tamir Duberstein <tamird@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?
compiler says no, e.g.:
error[E0659]: `format_args` is ambiguous
--> rust/doctests_kernel_generated.rs:8783:25
|
8783 | kernel::kunit::info(format_args!(" #
rust_doctest_kernel_workqueue_rs_3.location:
rust/kernel/workqueue.rs:77\n"));
| ^^^^^^^^^^^ ambiguous name
|
= note: ambiguous because of a conflict between a name from a
glob import and an outer scope during import or macro resolution
= note: `format_args` could refer to a macro from prelude
note: `format_args` could also refer to the macro imported here
--> rust/doctests_kernel_generated.rs:8772:9
|
8772 | use kernel::prelude::*;
| ^^^^^^^^^^^^^^^^^^
= help: consider adding an explicit import of `format_args` to disambiguate
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@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@google.com>
>> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
>> > Signed-off-by: Tamir Duberstein <tamird@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=5f529d93da7cf46b3c99ba7772623e33
Yes 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
On Mon, May 26, 2025 at 7:01 PM Benno Lossin <lossin@kernel.org> wrote:
>
> 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@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@google.com>
> >> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
> >> > Signed-off-by: Tamir Duberstein <tamird@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.
OK, but why? This seems entirely subjective.
> >> > +/// 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(),
> };
What happens if the invocation is utterly malformed, e.g.
`fmt!("hello)`? You're unwrapping here, which I intentionally avoid.
>
> >> > + 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=5f529d93da7cf46b3c99ba7772623e33
Makes sense to me.
>
> Yes 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 not sure - I would guess syntax errors "mask" typeck errors.
>
> 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) = ...;
It requires you to unwrap, like you did above, which is what I'm
trying to avoid.
>
> 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.
You need to give idents a span, so each of `kernel`, `fmt`, and
`adapter` need a span. I *could* use `quote!` and get whatever span it
uses (mixed_site) but I'd rather retain control.
>
> >> > + };
> >> > +
> >> > + 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
On Tue May 27, 2025 at 5:02 PM CEST, Tamir Duberstein wrote:
> On Mon, May 26, 2025 at 7:01 PM Benno Lossin <lossin@kernel.org> wrote:
>> 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@kernel.org> wrote:
>> >> On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
>> >> > +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.
>
> OK, but why? This seems entirely subjective.
If more types are added to the list, it will grow over one screen size.
With my formatting, leaving related types on a single line, that will
only happen much later.
>> >> > +/// 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(),
>> };
>
> What happens if the invocation is utterly malformed, e.g.
> `fmt!("hello)`? You're unwrapping here, which I intentionally avoid.
That example won't even survive lexing (macros always will get valid
rust tokens as input). If a literal begins with a `"`, it also will end
with one AFAIK.
>> Yes 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 not sure - I would guess syntax errors "mask" typeck errors.
I checked and it seems to be so, that's good.
>> >> > + 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) = ...;
>
> It requires you to unwrap, like you did above, which is what I'm
> trying to avoid.
How so? What do you need to unwrap?
>> >> > + };
>> >> > +
>> >> > + 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.
>
> You need to give idents a span, so each of `kernel`, `fmt`, and
> `adapter` need a span. I *could* use `quote!` and get whatever span it
> uses (mixed_site) but I'd rather retain control.
Please use `quote!` if it works. No need to make this more complex than
it already is. If it doesn't work then that's another story.
---
Cheers,
Benno
On Tue, May 27, 2025 at 4:49 PM Benno Lossin <lossin@kernel.org> wrote:
>
> On Tue May 27, 2025 at 5:02 PM CEST, Tamir Duberstein wrote:
> > On Mon, May 26, 2025 at 7:01 PM Benno Lossin <lossin@kernel.org> wrote:
> >> 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@kernel.org> wrote:
> >> >> On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
> >> >> > +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.
> >
> > OK, but why? This seems entirely subjective.
>
> If more types are added to the list, it will grow over one screen size.
> With my formatting, leaving related types on a single line, that will
> only happen much later.
>
> >> >> > +/// 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(),
> >> };
> >
> > What happens if the invocation is utterly malformed, e.g.
> > `fmt!("hello)`? You're unwrapping here, which I intentionally avoid.
>
> That example won't even survive lexing (macros always will get valid
> rust tokens as input). If a literal begins with a `"`, it also will end
> with one AFAIK.
>
> >> Yes 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 not sure - I would guess syntax errors "mask" typeck errors.
>
> I checked and it seems to be so, that's good.
👍
>
> >> >> > + 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) = ...;
> >
> > It requires you to unwrap, like you did above, which is what I'm
> > trying to avoid.
>
> How so? What do you need to unwrap?
I was referring to your unwraps above.
> >> >> > + };
> >> >> > +
> >> >> > + 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.
> >
> > You need to give idents a span, so each of `kernel`, `fmt`, and
> > `adapter` need a span. I *could* use `quote!` and get whatever span it
> > uses (mixed_site) but I'd rather retain control.
>
> Please use `quote!` if it works. No need to make this more complex than
> it already is. If it doesn't work then that's another story.
Let's adjudicate that on v11, where you can see the code.
© 2016 - 2025 Red Hat, Inc.