rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] rust: introduce sfile macro for succinct code tracing
@ 2025-06-16 19:34 Timur Tabi
  2025-06-16 21:03 ` Miguel Ojeda
  2025-06-18 11:20 ` kernel test robot
  0 siblings, 2 replies; 11+ messages in thread
From: Timur Tabi @ 2025-06-16 19:34 UTC (permalink / raw)
  To: Miguel Ojeda, Benno Lossin, Alice Ryhl, Danilo Krummrich,
	rust-for-linux

Introduce the sfile (short file) macro that returns the stem of
the current source file filename.

Rust provides a file!() macro that is similar to C's __FILE__ predefined
macro.  Unlike __FILE__, however, file!() returns a full path, which is
klunky when used for debug traces such as

	pr_info!("{}:{}\n", file!(), line!());

sfile!() can be used in situations instead, to provide a more compact
print.  For example, if file!() returns "rust/kernel/print.rs", sfile!()
returns just "print".

The macro avoids str::rfind() because currently, that function is not
const.  The compiler emits a call to memrchr, even when called on string
literals.  Instead, the macro implements its own versions of rfind(),
allowing the compiler to generate the slice at compile time.

The macro also uses some unsafe functions in order to avoid indexing
into a str, which is necessary to fully support const contexts.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 rust/kernel/print.rs | 57 ++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/str.rs   | 25 +++++++++++++++++++
 2 files changed, 82 insertions(+)

diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
index 9783d960a97a..992f8c7f6a7b 100644
--- a/rust/kernel/print.rs
+++ b/rust/kernel/print.rs
@@ -423,3 +423,60 @@ macro_rules! pr_cont (
         $crate::print_macro!($crate::print::format_strings::CONT, true, $($arg)*)
     )
 );
+
+/// Returns just the base filename of the current file.
+///
+/// This differs from the built-in [`file!()`] macro, which returns the full
+/// path of the current file.
+///
+/// Useful for succinct logging purposes.
+///
+/// # Examples
+///
+/// ```
+/// use kernel::sfile;
+/// pr_err!("{}:{}\n", sfile!(), line!());
+/// const SFILE: &'static str = sfile!();
+/// ```
+#[macro_export]
+macro_rules! sfile {
+    () => {{
+        const {
+            const FILE: &str = core::file!();
+
+            // [`.as_bytes()`] does not support non-ASCII filenames
+            assert!(::core::primitive::str::is_ascii(FILE));
+
+            let start = match ::kernel::str::rfind_const(FILE, '/') {
+                Some(slash) => slash + 1,
+                None => 0,
+            };
+            let end = match ::kernel::str::rfind_const(FILE, '.') {
+                Some(dot) => dot,
+                None => FILE.len(),
+            };
+
+            // Make sure that there actually is something to return.  This also
+            // covers a lot of corner cases, such as eliminating filenames that
+            // end in a slash (not possible) or don't have an extension,
+            // and making sure that `start` < `FILE.len()` and `end` - `start` > 0.
+            assert!(start < end);
+
+            // The following code the equivalent of &FILE[start..start+len],
+            // except that it allows for const contexts.  For example,
+            //   const SFILE: &'static str = sfile!();
+
+            let base_ptr: *const u8 = FILE.as_ptr();
+
+            // SAFETY: The above assertion ensures that `start` points to inside
+            // the string.
+            let ptr: *const u8 = unsafe { base_ptr.add(start) };
+            // SAFETY: Based on all constraints on `start` and `end`, this slice
+            // will never extend beyond the string.
+            let slice = unsafe { core::slice::from_raw_parts(ptr, end - start) };
+            // SAFETY: We know that the slice is valid UTF-8, because we checked
+            // that FILE is ASCII (via is_ascii() above).
+            unsafe { core::str::from_utf8_unchecked(slice) }
+        }
+    }};
+}
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index a927db8e079c..3e4eb7a62871 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -936,3 +936,28 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
 macro_rules! fmt {
     ($($f:tt)*) => ( ::core::format_args!($($f)*) )
 }
+
+/// Returns the index of the last occurrence of `needle` in `haystack`, or None.
+///
+/// Similar to [`str::rfind()`], except this one is const.  It also only supports
+/// ASCII strings.
+///
+/// # Examples
+///
+/// ```
+/// use kernel::print::rfind_const;
+/// let l = rfind_const("when will then be now?", 'l');
+/// assert!(l == Some(8));
+/// ```
+#[inline]
+pub const fn rfind_const(haystack: &str, needle: char) -> Option<usize> {
+    let haystack = haystack.as_bytes();
+    let mut i = haystack.len();
+    while i > 0 {
+        i -= 1;
+        if haystack[i] == needle as u8 {
+            return Some(i);
+        }
+    }
+    None
+}

base-commit: 27605c8c0f69e319df156b471974e4e223035378
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v4] rust: introduce sfile macro for succinct code tracing
  2025-06-16 19:34 [PATCH v4] rust: introduce sfile macro for succinct code tracing Timur Tabi
@ 2025-06-16 21:03 ` Miguel Ojeda
  2025-06-17  7:29   ` Benno Lossin
  2025-06-18 20:07   ` Timur Tabi
  2025-06-18 11:20 ` kernel test robot
  1 sibling, 2 replies; 11+ messages in thread
From: Miguel Ojeda @ 2025-06-16 21:03 UTC (permalink / raw)
  To: Timur Tabi, Benno Lossin
  Cc: Miguel Ojeda, Alice Ryhl, Danilo Krummrich, rust-for-linux

On Mon, Jun 16, 2025 at 9:35 PM Timur Tabi <ttabi@nvidia.com> wrote:
>
> +/// use kernel::sfile;

I would hide this line (`/// # use ...`) -- when it is just a single
`use` importing the thing that we are giving an example for, then it
does not add much.

> +/// pr_err!("{}:{}\n", sfile!(), line!());

Maybe add a comment in a new, previous line showing what output this
line could give, e.g.

    /// Output: ...

> +/// const SFILE: &'static str = sfile!();

Another comment (and an empty line to separate between the previous
case) to mention what `SFILE` could contain would be nice too.

Or, perhaps even better, since they are really separate examples with
a comment each, you could put the explanation outside the code.

Also perhaps consider if showing the `SFILE` first would look better.
If not, then "Useful for succinct logging purposes." could perhaps be
moved as part of the `pr_err` one since it would be the first anyway.

Anyway, it is not a big deal either way.

> +#[macro_export]
> +macro_rules! sfile {
> +    () => {{
> +        const {

Could we avoid the double block?

> +            const FILE: &str = core::file!();

`::core`, same below.

> +            // [`.as_bytes()`] does not support non-ASCII filenames

I appreciate that you tried to use an intra-doc link, and one
long-term wish of mine is to have those work even in comments for
source-views and IDEs, but currently we don't use them in normal
comments (we do use the code span).

Anyway, is the comment right? There is no `as_bytes()` call here --
what does support ASCII only is our `rfind`, right?

> +            let start = match ::kernel::str::rfind_const(FILE, '/') {
> +                Some(slash) => slash + 1,
> +                None => 0,
> +            };
> +            let end = match ::kernel::str::rfind_const(FILE, '.') {
> +                Some(dot) => dot,
> +                None => FILE.len(),
> +            };

I wonder if we should just panic if they are not found. Maybe I am not
creative enough, but do you have in mind a use case for either?

We already remove some cases anyway with the `assert!` below, so it is
not usable in every case anyway.

> +            // The following code the equivalent of &FILE[start..start+len],
> +            // except that it allows for const contexts.  For example,

This seems strangely worded: missing "is" and it probably should be
"except that it is allowed in const contexts". I would remove the
example, too.

> +            let ptr: *const u8 = unsafe { base_ptr.add(start) };

Benno: do you want here (and above) qualified calls too? (I would move
everything to a `const fn` though -- please see below)

I would just call both pointers `p`, by the way. No need to keep the
base one around.

> +            // that FILE is ASCII (via is_ascii() above).

`FILE`, `is_ascii()`

> +/// Returns the index of the last occurrence of `needle` in `haystack`, or None.

[`None`]

> +/// use kernel::print::rfind_const;

Consider hiding this one too.

> +/// let l = rfind_const("when will then be now?", 'l');
> +/// assert!(l == Some(8));

Please add an example for the `None` case too.

Finally, should we put most of this logic in a `const fn` too for the
usual benefits (docs and tests)? Then the macro would just be
essentially a const `basename(file!())` call, possibly fallible, and
you can even avoid having all those qualified calls :)

Thanks!

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4] rust: introduce sfile macro for succinct code tracing
  2025-06-16 21:03 ` Miguel Ojeda
@ 2025-06-17  7:29   ` Benno Lossin
  2025-06-17  7:54     ` Miguel Ojeda
  2025-06-18 20:07   ` Timur Tabi
  1 sibling, 1 reply; 11+ messages in thread
From: Benno Lossin @ 2025-06-17  7:29 UTC (permalink / raw)
  To: Miguel Ojeda, Timur Tabi
  Cc: Miguel Ojeda, Alice Ryhl, Danilo Krummrich, rust-for-linux

On Mon Jun 16, 2025 at 11:03 PM CEST, Miguel Ojeda wrote:
> On Mon, Jun 16, 2025 at 9:35 PM Timur Tabi <ttabi@nvidia.com> wrote:
>> +#[macro_export]
>> +macro_rules! sfile {
>> +    () => {{
>> +        const {
>
> Could we avoid the double block?

We should be able to.

>> +            let start = match ::kernel::str::rfind_const(FILE, '/') {
>> +                Some(slash) => slash + 1,
>> +                None => 0,
>> +            };
>> +            let end = match ::kernel::str::rfind_const(FILE, '.') {
>> +                Some(dot) => dot,
>> +                None => FILE.len(),
>> +            };
>
> I wonder if we should just panic if they are not found. Maybe I am not
> creative enough, but do you have in mind a use case for either?

Sounds like a good idea.

> We already remove some cases anyway with the `assert!` below, so it is
> not usable in every case anyway.

>> +            let ptr: *const u8 = unsafe { base_ptr.add(start) };
>
> Benno: do you want here (and above) qualified calls too? (I would move
> everything to a `const fn` though -- please see below)

Yeah we should do that. Some of my macros don't do it (eg use `Ok` and
`Err` without qualifying them), but we should make all macros always
exclusively use absolute paths for everything. (`Ok` and `Err` are
probably never going to be different ones in scope, but you never know)

@Timur for this case, you can qualify the call like this:

    let ptr = unsafe { <*const ::core::primitives::u8>::add(base_ptr, start) };

Maybe the stdlib should have a re-export of `*const` and `*mut` in
primitives...

> I would just call both pointers `p`, by the way. No need to keep the
> base one around.
>
>> +            // that FILE is ASCII (via is_ascii() above).
>
> `FILE`, `is_ascii()`
>
>> +/// Returns the index of the last occurrence of `needle` in `haystack`, or None.
>
> [`None`]
>
>> +/// use kernel::print::rfind_const;
>
> Consider hiding this one too.
>
>> +/// let l = rfind_const("when will then be now?", 'l');
>> +/// assert!(l == Some(8));
>
> Please add an example for the `None` case too.
>
> Finally, should we put most of this logic in a `const fn` too for the
> usual benefits (docs and tests)? Then the macro would just be
> essentially a const `basename(file!())` call, possibly fallible, and
> you can even avoid having all those qualified calls :)

A function sounds like a good idea.

But I don't see the value in the macro being fallible. Then the macro is
less convenient to use. The function can be fallible, but then the macro
should use `Result::expect()`.

---
Cheers,
Benno

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4] rust: introduce sfile macro for succinct code tracing
  2025-06-17  7:29   ` Benno Lossin
@ 2025-06-17  7:54     ` Miguel Ojeda
  2025-06-18 20:05       ` Timur Tabi
  0 siblings, 1 reply; 11+ messages in thread
From: Miguel Ojeda @ 2025-06-17  7:54 UTC (permalink / raw)
  To: Benno Lossin
  Cc: Timur Tabi, Miguel Ojeda, Alice Ryhl, Danilo Krummrich,
	rust-for-linux

On Tue, Jun 17, 2025 at 9:29 AM Benno Lossin <lossin@kernel.org> wrote:
>
> But I don't see the value in the macro being fallible. Then the macro is
> less convenient to use. The function can be fallible, but then the macro
> should use `Result::expect()`.

Yeah, I meant the `const fn`, not the macro, i.e. returning `Option`
(not `Result`) so that we keep the const block in the macro with an
`unwrap()` in the macro (so no runtime panics).

Thanks!

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4] rust: introduce sfile macro for succinct code tracing
  2025-06-16 19:34 [PATCH v4] rust: introduce sfile macro for succinct code tracing Timur Tabi
  2025-06-16 21:03 ` Miguel Ojeda
@ 2025-06-18 11:20 ` kernel test robot
  1 sibling, 0 replies; 11+ messages in thread
From: kernel test robot @ 2025-06-18 11:20 UTC (permalink / raw)
  To: Timur Tabi, Miguel Ojeda, Benno Lossin, Alice Ryhl,
	Danilo Krummrich, rust-for-linux
  Cc: llvm, oe-kbuild-all

Hi Timur,

kernel test robot noticed the following build errors:

[auto build test ERROR on 27605c8c0f69e319df156b471974e4e223035378]

url:    https://github.com/intel-lab-lkp/linux/commits/Timur-Tabi/rust-introduce-sfile-macro-for-succinct-code-tracing/20250617-034332
base:   27605c8c0f69e319df156b471974e4e223035378
patch link:    https://lore.kernel.org/r/20250616193433.2643407-1-ttabi%40nvidia.com
patch subject: [PATCH v4] rust: introduce sfile macro for succinct code tracing
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250618/202506181849.TxpYI9gO-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
rustc: rustc 1.78.0 (9b00956e5 2024-04-29)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250618/202506181849.TxpYI9gO-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506181849.TxpYI9gO-lkp@intel.com/

All errors (new ones prefixed by >>):

>> error[E0432]: unresolved import `kernel::print::rfind_const`
   --> rust/doctests_kernel_generated.rs:7755:5
   |
   7755 | use kernel::print::rfind_const;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ no `rfind_const` in `print`
--
>> error[E0308]: mismatched types
   --> rust/doctests_kernel_generated.rs:5834:29
   |
   5834 | const SFILE: &'static str = sfile!();
   |                             ^^^^^^^^
   |                             |
   |                             expected `&str`, found `&CStr`
   |                             arguments to this function are incorrect
   |
   = note: expected reference `&str`
   found reference `&'static rust_doctest_kernel_alloc_kbox_rs_7::kernel::prelude::CStr`
   note: method defined here
   --> /opt/cross/rustc-1.78.0-bindgen-0.65.1/rustup/toolchains/1.78.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/str/mod.rs:2446:18
   |
   2446 |     pub const fn is_ascii(&self) -> bool {
   |                  ^^^^^^^^
   = note: this error originates in the macro `sfile` (in Nightly builds, run with -Z macro-backtrace for more info)
--
>> error[E0308]: mismatched types
   --> rust/doctests_kernel_generated.rs:5833:20
   |
   5833 | pr_err!("{}:{}n", sfile!(), line!());
   |                    ^^^^^^^^
   |                    |
   |                    expected `&str`, found `&CStr`
   |                    arguments to this function are incorrect
   |
   = note: expected reference `&str`
   found reference `&'static rust_doctest_kernel_alloc_kbox_rs_7::kernel::prelude::CStr`
   note: method defined here
   --> /opt/cross/rustc-1.78.0-bindgen-0.65.1/rustup/toolchains/1.78.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/str/mod.rs:2446:18
   |
   2446 |     pub const fn is_ascii(&self) -> bool {
   |                  ^^^^^^^^
   = note: this error originates in the macro `sfile` (in Nightly builds, run with -Z macro-backtrace for more info)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4] rust: introduce sfile macro for succinct code tracing
  2025-06-17  7:54     ` Miguel Ojeda
@ 2025-06-18 20:05       ` Timur Tabi
  2025-06-21 16:59         ` Miguel Ojeda
  0 siblings, 1 reply; 11+ messages in thread
From: Timur Tabi @ 2025-06-18 20:05 UTC (permalink / raw)
  To: lossin@kernel.org, miguel.ojeda.sandonis@gmail.com
  Cc: ojeda@kernel.org, aliceryhl@google.com, dakr@kernel.org,
	rust-for-linux@vger.kernel.org

On Tue, 2025-06-17 at 09:54 +0200, Miguel Ojeda wrote:
> Yeah, I meant the `const fn`, not the macro, i.e. returning `Option`
> (not `Result`) so that we keep the const block in the macro with an
> `unwrap()` in the macro (so no runtime panics).

I'm having trouble coding this up.  I have this so far, but it doesn't compile:

#[macro_export]
macro_rules! sfile {
    () => {{
        const fn shortname() -> Option<&'static str> {
            const FILE: &str = core::file!();
...

            unsafe { core::str::from_utf8_unchecked(p) }
        };

        shortname().unwrap()
    }};
}

I get a bunch of errors, starting with this:

error[E0308]: mismatched types
   --> /home/ttabi/linux/rust/kernel/print.rs:492:22
    |
456 | macro_rules! sfile {
    | ------------------ in this expansion of `sfile!`
457 |     () => {{
458 |         const fn shortname() -> Option<&'static str> {
    |                                 -------------------- expected `Option<&'static str>` because
of return type
...
492 |             unsafe { core::str::from_utf8_unchecked(p) }
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Option<&str>`, found `&str`
    |
   ::: samples/rust/rust_minimal.rs:25:37
    |
25  |         const SFILE: &'static str = sfile!();
    |                                     -------- in this macro invocation
    |
    = note:   expected enum `Option<&'static str>`
            found reference `&str`


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4] rust: introduce sfile macro for succinct code tracing
  2025-06-16 21:03 ` Miguel Ojeda
  2025-06-17  7:29   ` Benno Lossin
@ 2025-06-18 20:07   ` Timur Tabi
  2025-06-21 16:47     ` Miguel Ojeda
  1 sibling, 1 reply; 11+ messages in thread
From: Timur Tabi @ 2025-06-18 20:07 UTC (permalink / raw)
  To: lossin@kernel.org, miguel.ojeda.sandonis@gmail.com
  Cc: ojeda@kernel.org, aliceryhl@google.com, dakr@kernel.org,
	rust-for-linux@vger.kernel.org

On Mon, 2025-06-16 at 23:03 +0200, Miguel Ojeda wrote:
> On Mon, Jun 16, 2025 at 9:35 PM Timur Tabi <ttabi@nvidia.com> wrote:
> > 
> > +/// use kernel::sfile;
> 
> I would hide this line (`/// # use ...`) -- when it is just a single
> `use` importing the thing that we are giving an example for, then it
> does not add much.

Sure.

> 
> > +/// pr_err!("{}:{}\n", sfile!(), line!());
> 
> Maybe add a comment in a new, previous line showing what output this
> line could give, e.g.
> 
>     /// Output: ...
> 
> > +/// const SFILE: &'static str = sfile!();

Sure, but I have not found any other examples of a comment example showing the output of that
example.

> Another comment (and an empty line to separate between the previous
> case) to mention what `SFILE` could contain would be nice too.
> 
> Or, perhaps even better, since they are really separate examples with
> a comment each, you could put the explanation outside the code.
> 
> Also perhaps consider if showing the `SFILE` first would look better.
> If not, then "Useful for succinct logging purposes." could perhaps be
> moved as part of the `pr_err` one since it would be the first anyway.
> 
> Anyway, it is not a big deal either way.

I will make some changes, but aren't we heading into bikeshedding territory now?

Anyway, I think this looks good:

/// # Examples
///
/// ### Example 1
/// ```
/// # use kernel::sfile;
/// pr_info!("{}:{}\n", sfile!(), line!());
/// ```
/// Output:
/// ```
/// mydriver: filename:23
/// ```
/// ### Example 2
/// ```
/// # use kernel::sfile;
/// const SFILE: &'static str = sfile!();
/// pr_info!("SFILE is {}\n", SFILE);
/// ```
/// Output:
/// ```
/// mydriver: SFILE is filename
/// ```

> > +#[macro_export]
> > +macro_rules! sfile {
> > +    () => {{
> > +        const {
> 
> Could we avoid the double block?

Yes, fixed.

> > +            const FILE: &str = core::file!();
> 
> `::core`, same below.
> 
> > +            // [`.as_bytes()`] does not support non-ASCII filenames
> 
> I appreciate that you tried to use an intra-doc link, and one
> long-term wish of mine is to have those work even in comments for
> source-views and IDEs, but currently we don't use them in normal
> comments (we do use the code span).

I've removed the brackets.

> Anyway, is the comment right? There is no `as_bytes()` call here --
> what does support ASCII only is our `rfind`, right?
> 
> > +            let start = match ::kernel::str::rfind_const(FILE, '/') {
> > +                Some(slash) => slash + 1,
> > +                None => 0,
> > +            };
> > +            let end = match ::kernel::str::rfind_const(FILE, '.') {
> > +                Some(dot) => dot,
> > +                None => FILE.len(),
> > +            };
> 
> I wonder if we should just panic if they are not found. Maybe I am not
> creative enough, but do you have in mind a use case for either?

For the first one, there might not be a slash if the file is in the root directory.

For the second, this is for files that don't have a .rs (or any) filename extension.  Technically,
that shouldn't happen, but why panic if it does?

> We already remove some cases anyway with the `assert!` below, so it is
> not usable in every case anyway.
> 
> > +            // The following code the equivalent of &FILE[start..start+len],
> > +            // except that it allows for const contexts.  For example,
> 
> This seems strangely worded: missing "is" and it probably should be
> "except that it is allowed in const contexts". I would remove the
> example, too.

Ok.

> 
> > +            let ptr: *const u8 = unsafe { base_ptr.add(start) };
> 
> Benno: do you want here (and above) qualified calls too? (I would move
> everything to a `const fn` though -- please see below)
> 
> I would just call both pointers `p`, by the way. No need to keep the
> base one around.
> 
> > +            // that FILE is ASCII (via is_ascii() above).
> 
> `FILE`, `is_ascii()`
> 
> > +/// Returns the index of the last occurrence of `needle` in `haystack`, or None.
> 
> [`None`]
> 
> > +/// use kernel::print::rfind_const;
> 
> Consider hiding this one too.
> 
> > +/// let l = rfind_const("when will then be now?", 'l');
> > +/// assert!(l == Some(8));
> 
> Please add an example for the `None` case too.

Will do.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4] rust: introduce sfile macro for succinct code tracing
  2025-06-18 20:07   ` Timur Tabi
@ 2025-06-21 16:47     ` Miguel Ojeda
  0 siblings, 0 replies; 11+ messages in thread
From: Miguel Ojeda @ 2025-06-21 16:47 UTC (permalink / raw)
  To: Timur Tabi
  Cc: lossin@kernel.org, ojeda@kernel.org, aliceryhl@google.com,
	dakr@kernel.org, rust-for-linux@vger.kernel.org

On Wed, Jun 18, 2025 at 10:07 PM Timur Tabi <ttabi@nvidia.com> wrote:
>
> > > +/// const SFILE: &'static str = sfile!();
>
> Sure, but I have not found any other examples of a comment example showing the output of that
> example.

I meant something simple, like:

    /// // Contains: `...`.
    /// const ...

> I will make some changes, but aren't we heading into bikeshedding territory now?

As I said, it is not a big deal, it can be improved later, if needed/ever.

Sometimes, for "core" things that everybody will read and learn from,
it is good to try to do it as best as possible, so that it serves as
an example for others later, if that makes sense.

> Anyway, I think this looks good:

So the idea of splitting was to add some actual explanation
in-between. For instance, I was thinking of something like:

    /// # Examples
    ///
    /// The macro may be used for succinct logging purposes, e.g.:
    ///
    /// ```
    /// # use kernel::sfile;
    /// // Output: `example:42`.
    /// pr_err!("{}:{}\n", sfile!(), line!());
    /// ```
    ///
    /// The value is a constant expression, so it can be used in const
    /// contexts, e.g.:
    ///
    /// ```
    /// # use kernel::sfile;
    /// // Contains: `example`.
    /// const SFILE: &'static str = sfile!();
    /// ```

> For the first one, there might not be a slash if the file is in the root directory.
>
> For the second, this is for files that don't have a .rs (or any) filename extension.  Technically,
> that shouldn't happen, but why panic if it does?

I wanted to know if you had a particular use case in mind, i.e. root
files and non-`.rs` files are unlikely to ever happen.

As for panicking -- having dead paths isn't great. This is a
compile-time thing, so if it ever panics, we will know about it, and
we can then relax it if it was intended.

So that is why I am asking if we already know about a concrete use
case, because if not, then we can just simplify and wait until we
actually need it.

Thanks!

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4] rust: introduce sfile macro for succinct code tracing
  2025-06-18 20:05       ` Timur Tabi
@ 2025-06-21 16:59         ` Miguel Ojeda
  2025-06-26 16:30           ` Timur Tabi
  0 siblings, 1 reply; 11+ messages in thread
From: Miguel Ojeda @ 2025-06-21 16:59 UTC (permalink / raw)
  To: Timur Tabi
  Cc: lossin@kernel.org, ojeda@kernel.org, aliceryhl@google.com,
	dakr@kernel.org, rust-for-linux@vger.kernel.org

On Wed, Jun 18, 2025 at 10:05 PM Timur Tabi <ttabi@nvidia.com> wrote:
>
> I'm having trouble coding this up.  I have this so far, but it doesn't compile:

(snip)

> 492 |             unsafe { core::str::from_utf8_unchecked(p) }
>     |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Option<&str>`, found `&str`

If you are asking about that last line, you need to return a `Some(...)`.

If you are asking in general, then since now you can return `None`,
you can just call the safe `from_utf8` and convert to an `Option`,
which also allows you to avoid the `is_ascii` call, and also allows
you to return `None` in other cases above too (instead of panicking).

That also allows you to keep the macro straightforward and small.

I hope that helps.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4] rust: introduce sfile macro for succinct code tracing
  2025-06-21 16:59         ` Miguel Ojeda
@ 2025-06-26 16:30           ` Timur Tabi
  2025-06-26 20:57             ` Miguel Ojeda
  0 siblings, 1 reply; 11+ messages in thread
From: Timur Tabi @ 2025-06-26 16:30 UTC (permalink / raw)
  To: miguel.ojeda.sandonis@gmail.com
  Cc: ojeda@kernel.org, aliceryhl@google.com, lossin@kernel.org,
	dakr@kernel.org, rust-for-linux@vger.kernel.org

On Sat, 2025-06-21 at 18:59 +0200, Miguel Ojeda wrote:
> On Wed, Jun 18, 2025 at 10:05 PM Timur Tabi <ttabi@nvidia.com> wrote:
> > 
> > I'm having trouble coding this up.  I have this so far, but it doesn't compile:
> 
> (snip)
> 
> > 492 |             unsafe { core::str::from_utf8_unchecked(p) }
> >     |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Option<&str>`, found
> > `&str`
> 
> If you are asking about that last line, you need to return a `Some(...)`.

Thanks, I should have figure that one out myself.

> If you are asking in general, then since now you can return `None`,
> you can just call the safe `from_utf8` and convert to an `Option`,
> which also allows you to avoid the `is_ascii` call, and also allows
> you to return `None` in other cases above too (instead of panicking).

So you're saying that instead of calling assert!(), shortname() should just return None if there
are any failure conditions, and the macro should then panic if it gets None? 

I'm probably missing something, but it's not working as smoothly as you make it sound.

I don't think I can call from_utf8() because it returns Result, and converting that to an Option
requires me to call .ok(), which (for some reason) cannot be called in a const function.

I also tried doing this:

            let start = ::kernel::str::rfind_const(FILE, '/')? + 1;

But you can't use ? in a const function either.  I can use .unwrap() though, but if I do that,
then I no longer return None for any failures, I just panic.

I don't see how this lets me avoid the is_ascii() call.  However, I just discovered that in UTF-
8, you cannot have a slash (0x2F) or dot (0x2E) as one of the UTF-8 bytes, since all UTF-8 bytes
have bit 7 set.  So you won't accidentally find a / inside a UTF character.  This means that
this macro should be able to handle Unicode file names.  So I'll just remove the is_ascii()
check altogether.

However, I don't see how this code is improving.  I will post a v5, but I think v4 is better.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4] rust: introduce sfile macro for succinct code tracing
  2025-06-26 16:30           ` Timur Tabi
@ 2025-06-26 20:57             ` Miguel Ojeda
  0 siblings, 0 replies; 11+ messages in thread
From: Miguel Ojeda @ 2025-06-26 20:57 UTC (permalink / raw)
  To: Timur Tabi
  Cc: ojeda@kernel.org, aliceryhl@google.com, lossin@kernel.org,
	dakr@kernel.org, rust-for-linux@vger.kernel.org

[-- Attachment #1: Type: text/plain, Size: 1875 bytes --]

On Thu, Jun 26, 2025 at 6:30 PM Timur Tabi <ttabi@nvidia.com> wrote:
>
> So you're saying that instead of calling assert!(), shortname() should just return None if there
> are any failure conditions, and the macro should then panic if it gets None?

Yes, but note that the panic in the macro will not be a runtime panic,
but a compile time error, which is why it is OK.

> I don't think I can call from_utf8() because it returns Result, and converting that to an Option
> requires me to call .ok(), which (for some reason) cannot be called in a const function.

You can -- `ok()` is not `const fn` because it is generic and thus
needs to work in more cases, but you can do it "manually" for this
particular `Result`.

> I don't see how this lets me avoid the is_ascii() call.  However, I just discovered that in UTF-
> 8, you cannot have a slash (0x2F) or dot (0x2E) as one of the UTF-8 bytes, since all UTF-8 bytes
> have bit 7 set.  So you won't accidentally find a / inside a UTF character.  This means that
> this macro should be able to handle Unicode file names.  So I'll just remove the is_ascii()
> check altogether.

We can avoid it because `from_utf8` already checks it is valid UTF-8,
so if something bad is passed as input, such as broken UTF-8, it will
already return an error. We are not expecting Unicode filenames
anyway.

We can in fact do it all in safe code, because we can take advantage
of `split_at_checked` -- sadly only on `slice`, but in some months
(minimum Rust >= 1.84) we should be able to simplify further using the
`str` one.

And if we are OK with panicking, e.g. if we just want to use it from
the macro, we can simplify further.

See the attached diff to see how `shortname()` could look like.

`inline_const` and `const_option` are just to make the macro nicer,
they are optional.

Cheers,
Miguel

[-- Attachment #2: 0001-Show-how-shortname-could-be-written.patch --]
[-- Type: text/x-patch, Size: 7961 bytes --]

From f91fa96256775a30f8b757603c56caa7736ad81e Mon Sep 17 00:00:00 2001
From: Miguel Ojeda <ojeda@kernel.org>
Date: Thu, 26 Jun 2025 20:48:01 +0200
Subject: [PATCH] Show how `shortname()` could be written

Not-Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 rust/kernel/lib.rs     |  3 ++
 rust/kernel/print.rs   | 46 +++-----------------
 rust/kernel/str.rs     | 96 +++++++++++++++++++++++++++++++++++++++---
 scripts/Makefile.build |  4 +-
 4 files changed, 102 insertions(+), 47 deletions(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6b4774b2b1c3..2383b4565ef1 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -19,6 +19,9 @@
 // Stable since Rust 1.79.0.
 #![feature(inline_const)]
 //
+// Stable since Rust 1.80.0.
+#![feature(split_at_checked)]
+//
 // Stable since Rust 1.81.0.
 #![feature(lint_reasons)]
 //
diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
index e598311f198c..c37b27873c6f 100644
--- a/rust/kernel/print.rs
+++ b/rust/kernel/print.rs
@@ -434,49 +434,13 @@ macro_rules! pr_cont (
 /// # Examples
 ///
 /// ```
-/// use kernel::sfile;
+/// # use kernel::sfile;
 /// pr_err!("{}:{}\n", sfile!(), line!());
-/// const SFILE: &'static str = sfile!();
+/// const SFILE: &str = sfile!();
 /// ```
 #[macro_export]
 macro_rules! sfile {
-    () => {{
-        const {
-            const FILE: &str = core::file!();
-
-            // [`.as_bytes()`] does not support non-ASCII filenames
-            assert!(::core::primitive::str::is_ascii(FILE));
-
-            let start = match ::kernel::str::rfind_const(FILE, '/') {
-                Some(slash) => slash + 1,
-                None => 0,
-            };
-            let end = match ::kernel::str::rfind_const(FILE, '.') {
-                Some(dot) => dot,
-                None => FILE.len(),
-            };
-
-            // Make sure that there actually is something to return.  This also
-            // covers a lot of corner cases, such as eliminating filenames that
-            // end in a slash (not possible) or don't have an extension,
-            // and making sure that `start` < `FILE.len()` and `end` - `start` > 0.
-            assert!(start < end);
-
-            // The following code the equivalent of &FILE[start..start+len],
-            // except that it allows for const contexts.  For example,
-            //   const SFILE: &'static str = sfile!();
-
-            let base_ptr: *const u8 = FILE.as_ptr();
-
-            // SAFETY: The above assertion ensures that `start` points to inside
-            // the string.
-            let ptr: *const u8 = unsafe { base_ptr.add(start) };
-            // SAFETY: Based on all constraints on `start` and `end`, this slice
-            // will never extend beyond the string.
-            let slice = unsafe { core::slice::from_raw_parts(ptr, end - start) };
-            // SAFETY: We know that the slice is valid UTF-8, because we checked
-            // that FILE is ASCII (via is_ascii() above).
-            unsafe { core::str::from_utf8_unchecked(slice) }
-        }
-    }};
+    () => {
+        const { ::kernel::str::shortname(file!()).unwrap() }
+    };
 }
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 680c631c2eb3..84d72d74a9d4 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -945,19 +945,105 @@ macro_rules! fmt {
 /// # Examples
 ///
 /// ```
-/// use kernel::print::rfind_const;
-/// let l = rfind_const("when will then be now?", 'l');
+/// # use kernel::str::slice_u8_rfind_const;
+/// let l = slice_u8_rfind_const("when will then be now?".as_bytes(), b'l');
 /// assert!(l == Some(8));
 /// ```
 #[inline]
-pub const fn rfind_const(haystack: &str, needle: char) -> Option<usize> {
-    let haystack = haystack.as_bytes();
+pub const fn slice_u8_rfind_const(haystack: &[u8], needle: u8) -> Option<usize> {
     let mut i = haystack.len();
     while i > 0 {
         i -= 1;
-        if haystack[i] == needle as u8 {
+        if haystack[i] == needle {
             return Some(i);
         }
     }
     None
 }
+
+/// ...
+///
+/// # Examples
+///
+/// ```
+/// # use kernel::str::shortname;
+/// assert_eq!(shortname(""), None);
+/// assert_eq!(shortname("example"), None);
+/// assert_eq!(shortname("example.rs"), None);
+/// assert_eq!(shortname("path/example"), None);
+/// assert_eq!(shortname("path/example.rs"), Some("example"));
+/// assert_eq!(shortname("longer/path/example.foo.rs"), Some("example.foo"));
+///
+/// assert_eq!(shortname("/e."), Some("e"));
+/// assert_eq!(shortname(".e/"), None);
+/// assert_eq!(shortname("/e.e"), Some("e"));
+/// assert_eq!(shortname(".e/e"), None);
+/// assert_eq!(shortname("e/e."), Some("e"));
+/// assert_eq!(shortname("e.e/"), None);
+/// assert_eq!(shortname("e/e.e"), Some("e"));
+/// assert_eq!(shortname("e.e/e"), None);
+///
+/// assert_eq!(shortname(".e/"), None);
+/// assert_eq!(shortname("e.e/e"), None);
+///
+/// assert_eq!(shortname(".."), None);
+/// assert_eq!(shortname("..e"), None);
+/// assert_eq!(shortname("e.."), None);
+/// assert_eq!(shortname("e..e"), None);
+///
+/// assert_eq!(shortname("//"), None);
+/// assert_eq!(shortname("//e"), None);
+/// assert_eq!(shortname("e//"), None);
+/// assert_eq!(shortname("e//e"), None);
+///
+/// assert_eq!(shortname("/."), None);
+/// assert_eq!(shortname("./"), None);
+/// assert_eq!(shortname("/.e"), None);
+/// assert_eq!(shortname("./e"), None);
+/// assert_eq!(shortname("e/."), None);
+/// assert_eq!(shortname("e./"), None);
+/// assert_eq!(shortname("e/.e"), None);
+/// assert_eq!(shortname("e./e"), None);
+///
+/// assert_eq!(shortname("."), None);
+/// assert_eq!(shortname("/"), None);
+/// assert_eq!(shortname(".e"), None);
+/// assert_eq!(shortname("/e"), None);
+/// assert_eq!(shortname("e."), None);
+/// assert_eq!(shortname("e/"), None);
+/// assert_eq!(shortname("e.e"), None);
+/// assert_eq!(shortname("e/e"), None);
+///
+/// assert_eq!(shortname(""), None);
+/// ```
+pub const fn shortname(path: &str) -> Option<&str> {
+    let path = path.as_bytes();
+
+    let Some(last_slash) = slice_u8_rfind_const(path, b'/') else {
+        return None;
+    };
+    let Some(path) = path.split_at_checked(last_slash + 1) else {
+        return None;
+    };
+    let path = path.1;
+
+    let Some(last_period) = slice_u8_rfind_const(path, b'.') else {
+        return None;
+    };
+    let Some(path) = path.split_at_checked(last_period) else {
+        return None;
+    };
+    let path = path.0;
+
+    // TODO: after Rust 1.84.0, we could use `split_at_checked` on `str`
+    // directly to avoid going through `&[u8]`.
+    let Ok(path) = core::str::from_utf8(path) else {
+        return None;
+    };
+
+    if path.is_empty() {
+        return None;
+    }
+
+    Some(path)
+}
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index a6461ea411f7..207f2ff504e9 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -308,14 +308,16 @@ $(obj)/%.lst: $(obj)/%.c FORCE
 
 # The features in this list are the ones allowed for non-`rust/` code.
 #
+#   - Stable since Rust 1.79.0: `feature(inline_const)`.
 #   - Stable since Rust 1.81.0: `feature(lint_reasons)`.
 #   - Stable since Rust 1.82.0: `feature(asm_const)`, `feature(raw_ref_op)`.
+#   - Stable since Rust 1.83.0: `feature(const_option)`.
 #   - Stable since Rust 1.87.0: `feature(asm_goto)`.
 #   - Expected to become stable: `feature(arbitrary_self_types)`.
 #
 # Please see https://github.com/Rust-for-Linux/linux/issues/2 for details on
 # the unstable features in use.
-rust_allowed_features := asm_const,asm_goto,arbitrary_self_types,lint_reasons,raw_ref_op
+rust_allowed_features := asm_const,asm_goto,arbitrary_self_types,const_option,inline_const,lint_reasons,raw_ref_op
 
 # `--out-dir` is required to avoid temporaries being created by `rustc` in the
 # current working directory, which may be not accessible in the out-of-tree

base-commit: 773f2f949f02d4633a51b41c1204ca7df1b7d23b
-- 
2.50.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-06-26 20:57 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-16 19:34 [PATCH v4] rust: introduce sfile macro for succinct code tracing Timur Tabi
2025-06-16 21:03 ` Miguel Ojeda
2025-06-17  7:29   ` Benno Lossin
2025-06-17  7:54     ` Miguel Ojeda
2025-06-18 20:05       ` Timur Tabi
2025-06-21 16:59         ` Miguel Ojeda
2025-06-26 16:30           ` Timur Tabi
2025-06-26 20:57             ` Miguel Ojeda
2025-06-18 20:07   ` Timur Tabi
2025-06-21 16:47     ` Miguel Ojeda
2025-06-18 11:20 ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).