[PATCH] rust: introduce sfile macro for easier code tracing

[PATCH] rust: introduce sfile macro for easier code tracing

[Timur Tabi]

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 goes 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.

Unfortunately, Rust does not consider the .. operator to be const either,
so sfind!() cannot be assigned to consts.  For example, the following will
not compile:

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

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 rust/kernel/print.rs | 48 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
index cf4714242e14..3c7b0c74bfb9 100644
--- a/rust/kernel/print.rs
+++ b/rust/kernel/print.rs
@@ -414,3 +414,51 @@ macro_rules! pr_cont (
         $crate::print_macro!($crate::print::format_strings::CONT, true, $($arg)*)
     )
 );
+
+/// Returns just the base filename of the current file.
+/// file!() returns the full path of the current file, which is often too long.
+/// Use this macro to trace your code:
+/// pr_err!("{}:{}\n", sfile!(), line!());
+/// Note: Avoiding rfind() allows this macro to be evaluated at compile time
+/// in most situations, such as the above pr_err!() example.  However,
+/// because .. is apparently a non-const operator, the following will not work:
+///     const SFILE: &'static str = sfile!();
+#[macro_export]
+macro_rules! sfile {
+    () => {{
+        const FILE: &str = file!();
+
+        /// Return the index of the last occurrence of @needle in @haystack,
+        /// or zero if not found.  We can't use rfind() because it's not const (yet).
+        const fn find_last_or_zero(haystack: &str, needle: char) -> usize {
+            let bytes = haystack.as_bytes();
+            let mut i = haystack.len();
+            while i > 0 {
+                i -= 1;
+                if bytes[i] == needle as u8 {
+                    return i;
+                }
+            }
+            0
+        }
+
+        /// Return the index of the last occurrence of @needle in @haystack,
+        /// or the length of the string if not found.
+        const fn find_last_or_len(haystack: &str, needle: char) -> usize {
+            let len = haystack.len();
+            let bytes = haystack.as_bytes();
+            let mut i = len;
+            while i > 0 {
+                i -= 1;
+                if bytes[i] == needle as u8 {
+                    return i;
+                }
+            }
+            len
+        }
+
+        let start = find_last_or_zero(FILE, '/') + 1;
+        let len = find_last_or_len(&FILE[start..], '.');
+        &FILE[start..start+len]
+    }};
+}
-- 
2.43.0

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Benno Lossin]

On Thu May 29, 2025 at 8:45 PM CEST, Timur Tabi wrote:
> 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 goes 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.
>
> Unfortunately, Rust does not consider the .. operator to be const either,
> so sfind!() cannot be assigned to consts.  For example, the following will
> not compile:

This is not the `..` operator, but rather the index operation of a str
(`&FILE[start..]`). It can be made const by using `from_utf8` [1] and
manually creating the correct byte slice using `unsafe` [2] instead.

[1]: https://doc.rust-lang.org/std/str/fn.from_utf8.html
[2]: https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html

> 	const SFILE: &'static str = sfile!();
>
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>  rust/kernel/print.rs | 48 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
> index cf4714242e14..3c7b0c74bfb9 100644
> --- a/rust/kernel/print.rs
> +++ b/rust/kernel/print.rs
> @@ -414,3 +414,51 @@ macro_rules! pr_cont (
>          $crate::print_macro!($crate::print::format_strings::CONT, true, $($arg)*)
>      )
>  );
> +
> +/// Returns just the base filename of the current file.
> +/// file!() returns the full path of the current file, which is often too long.
> +/// Use this macro to trace your code:
> +/// pr_err!("{}:{}\n", sfile!(), line!());
> +/// Note: Avoiding rfind() allows this macro to be evaluated at compile time
> +/// in most situations, such as the above pr_err!() example.  However,
> +/// because .. is apparently a non-const operator, the following will not work:
> +///     const SFILE: &'static str = sfile!();
> +#[macro_export]
> +macro_rules! sfile {
> +    () => {{
> +        const FILE: &str = file!();
> +
> +        /// Return the index of the last occurrence of @needle in @haystack,
> +        /// or zero if not found.  We can't use rfind() because it's not const (yet).
> +        const fn find_last_or_zero(haystack: &str, needle: char) -> usize {
> +            let bytes = haystack.as_bytes();

Using bytes doesn't consider non-ascii filenames (which I think we don't
have), but we could enforce that by checking `is_ascii` and panicking
otherwise.

---
Cheers,
Benno

> +            let mut i = haystack.len();
> +            while i > 0 {
> +                i -= 1;
> +                if bytes[i] == needle as u8 {
> +                    return i;
> +                }
> +            }
> +            0
> +        }
> +
> +        /// Return the index of the last occurrence of @needle in @haystack,
> +        /// or the length of the string if not found.
> +        const fn find_last_or_len(haystack: &str, needle: char) -> usize {
> +            let len = haystack.len();
> +            let bytes = haystack.as_bytes();
> +            let mut i = len;
> +            while i > 0 {
> +                i -= 1;
> +                if bytes[i] == needle as u8 {
> +                    return i;
> +                }
> +            }
> +            len
> +        }
> +
> +        let start = find_last_or_zero(FILE, '/') + 1;
> +        let len = find_last_or_len(&FILE[start..], '.');
> +        &FILE[start..start+len]
> +    }};
> +}

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Miguel Ojeda]

On Thu, May 29, 2025 at 8:46 PM Timur Tabi <ttabi@nvidia.com> wrote:
>
> Introduce the sfile (short file) macro that returns the stem of
> the current source file filename.

What is the use case, temporary debug statements or something more
permanent? If the former, what about `dbg!()`?

i.e. can we give an example of how it will be used? (Ideally please
put it in the docs, so that it gets built and tested)

> The macro goes avoids str::rfind() because currently, that function is not

"goes avoids"

> +/// Returns just the base filename of the current file.
> +/// file!() returns the full path of the current file, which is often too long.
> +/// Use this macro to trace your code:
> +/// pr_err!("{}:{}\n", sfile!(), line!());

Please format docs as we usually do (please check the rendered form --
this will probably be a single paragraph without code spans etc.).

> +/// Note: Avoiding rfind() allows this macro to be evaluated at compile time
> +/// in most situations, such as the above pr_err!() example.  However,
> +/// because .. is apparently a non-const operator, the following will not work:

This seems like an implementation detail, so probably it should not be
part of the docs (`///`).

Anyway, please see what Benno said, of course.

> +        /// Return the index of the last occurrence of @needle in @haystack,

We don't use `@` in Rust docs (we do have a single case of that -- I
will create a "good first issue" for it).

> +        const fn find_last_or_zero(haystack: &str, needle: char) -> usize {

If helpers within macros are needed, and especially if the use case is
not just temporary debugging statements, then it may be best to put
them as helpers outside, to avoid generating all this code on every
macro invocation.

Thanks!

Cheers,
Miguel

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[kernel test robot]

Hi Timur,

kernel test robot noticed the following build errors:

[auto build test ERROR on rust/rust-next]
[also build test ERROR on linus/master v6.15 next-20250529]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Timur-Tabi/rust-introduce-sfile-macro-for-easier-code-tracing/20250530-024820
base:   https://github.com/Rust-for-Linux/linux rust-next
patch link:    https://lore.kernel.org/r/20250529184547.1447995-1-ttabi%40nvidia.com
patch subject: [PATCH] rust: introduce sfile macro for easier code tracing
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250530/202505301109.ySBz6GOs-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/20250530/202505301109.ySBz6GOs-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/202505301109.ySBz6GOs-lkp@intel.com/

All errors (new ones prefixed by >>):

   PATH=/opt/cross/clang-18/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
   INFO PATH=/opt/cross/rustc-1.78.0-bindgen-0.65.1/cargo/bin:/opt/cross/clang-18/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
   /usr/bin/timeout -k 100 12h /usr/bin/make KCFLAGS= -Wno-error=return-type -Wreturn-type -funsigned-char -Wundef W=1 --keep-going LLVM=1 -j32 -C source O=/kbuild/obj/consumer/x86_64-rhel-9.4-rust ARCH=x86_64 SHELL=/bin/bash rustfmtcheck
   make: Entering directory '/kbuild/src/consumer'
   make[1]: Entering directory '/kbuild/obj/consumer/x86_64-rhel-9.4-rust'
>> Diff in rust/kernel/print.rs at line 468:
    
            let start = find_last_or_zero(FILE, '/') + 1;
            let len = find_last_or_len(&FILE[start..], '.');
   -        &FILE[start..start+len]
   +        &FILE[start..start + len]
        }};
    }
    
>> Diff in rust/kernel/print.rs at line 468:
    
            let start = find_last_or_zero(FILE, '/') + 1;
            let len = find_last_or_len(&FILE[start..], '.');
   -        &FILE[start..start+len]
   +        &FILE[start..start + len]
        }};
    }
    
   make[2]: *** [Makefile:1826: rustfmt] Error 123
   make[2]: Target 'rustfmtcheck' not remade because of errors.
   make[1]: Leaving directory '/kbuild/obj/consumer/x86_64-rhel-9.4-rust'
   make[1]: *** [Makefile:248: __sub-make] Error 2
   make[1]: Target 'rustfmtcheck' not remade because of errors.
   make: *** [Makefile:248: __sub-make] Error 2
   make: Target 'rustfmtcheck' not remade because of errors.
   make: Leaving directory '/kbuild/src/consumer'

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

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Timur Tabi]

On Thu, 2025-05-29 at 22:14 +0200, Benno Lossin wrote:
> > 
> > The macro goes 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.
> > 
> > Unfortunately, Rust does not consider the .. operator to be const either,
> > so sfind!() cannot be assigned to consts.  For example, the following will
> > not compile:
> 
> This is not the `..` operator, but rather the index operation of a str
> (`&FILE[start..]`). It can be made const by using `from_utf8` [1] and
> manually creating the correct byte slice using `unsafe` [2] instead.
> 
> [1]: https://doc.rust-lang.org/std/str/fn.from_utf8.html
> [2]: https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html


Ok, it took a lot of trial-and-error on my part, but I got this to work.  It's not pretty, though. 
Is this what you intended?

        let start = find_last_or_zero(FILE, '/') + 1;
        let end = find_last_or_len(FILE, '.');
        
        let base_ptr: *const u8 = FILE.as_ptr();
        let ptr: *const u8 = unsafe { base_ptr.add(start) };
        let slice = unsafe { core::slice::from_raw_parts(ptr, end - start) };
        unsafe { core::str::from_utf8_unchecked(slice) }

The good news is that this allows

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

to work, so thank you.


> > +        const fn find_last_or_zero(haystack: &str, needle: char) -> usize {
> > +            let bytes = haystack.as_bytes();
> 
> Using bytes doesn't consider non-ascii filenames (which I think we don't
> have), but we could enforce that by checking `is_ascii` and panicking
> otherwise.

Done.

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Timur Tabi]

On Thu, 2025-05-29 at 22:21 +0200, Miguel Ojeda wrote:
> On Thu, May 29, 2025 at 8:46 PM Timur Tabi <ttabi@nvidia.com> wrote:
> > 
> > Introduce the sfile (short file) macro that returns the stem of
> > the current source file filename.
> 
> What is the use case, temporary debug statements or something more
> permanent? If the former, what about `dbg!()`?

Temporary, definitely.

So dbg!() uses file!(), which means that it prints the full path, e.g.:

	[src/main.rs:3:8] n <= 1 = false

If the full path is not too long for you, that's okay.  It is too long for me, which is why I wrote
this macro.  So if dbg!() were to use sfile!(), it would look like this:

	[main:3:8] n <= 1 = false

> i.e. can we give an example of how it will be used? (Ideally please
> put it in the docs, so that it gets built and tested)
> 
> > The macro goes avoids str::rfind() because currently, that function is not
> 
> "goes avoids"

Fixed, thanks.

> 
> > +/// Returns just the base filename of the current file.
> > +/// file!() returns the full path of the current file, which is often too long.
> > +/// Use this macro to trace your code:
> > +/// pr_err!("{}:{}\n", sfile!(), line!());
> 
> Please format docs as we usually do (please check the rendered form --
> this will probably be a single paragraph without code spans etc.).

Ah, I see what you mean.  Thanks, I'll fix it.

> 
> > +/// Note: Avoiding rfind() allows this macro to be evaluated at compile time
> > +/// in most situations, such as the above pr_err!() example.  However,
> > +/// because .. is apparently a non-const operator, the following will not work:
> 
> This seems like an implementation detail, so probably it should not be
> part of the docs (`///`).

Ok.

> 
> Anyway, please see what Benno said, of course.
> 
> > +        /// Return the index of the last occurrence of @needle in @haystack,
> 
> We don't use `@` in Rust docs (we do have a single case of that -- I
> will create a "good first issue" for it).

I see other comments use back-ticks.  

> > +        const fn find_last_or_zero(haystack: &str, needle: char) -> usize {
> 
> If helpers within macros are needed, and especially if the use case is
> not just temporary debugging statements, then it may be best to put
> them as helpers outside, to avoid generating all this code on every
> macro invocation.

That's the beauty of this macro -- it doesn't generate any code.  Not really, at least.  The
compiler determines at compile time the slice inside file!() that this macro generates, so all that 
happens is the compiler emits hard-coded offsets into the string literal.

You can see for yourself here:  https://rust.godbolt.org/z/e947zsxfP

This is the whole reason I go through such lengths to avoid rfind() and the .. operator.

> 
> Thanks!
> 
> Cheers,
> Miguel

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Benno Lossin]

On Tue Jun 3, 2025 at 7:15 PM CEST, Timur Tabi wrote:
> On Thu, 2025-05-29 at 22:14 +0200, Benno Lossin wrote:
>> > 
>> > The macro goes 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.
>> > 
>> > Unfortunately, Rust does not consider the .. operator to be const either,
>> > so sfind!() cannot be assigned to consts.  For example, the following will
>> > not compile:
>> 
>> This is not the `..` operator, but rather the index operation of a str
>> (`&FILE[start..]`). It can be made const by using `from_utf8` [1] and
>> manually creating the correct byte slice using `unsafe` [2] instead.
>> 
>> [1]: https://doc.rust-lang.org/std/str/fn.from_utf8.html
>> [2]: https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html
>
>
> Ok, it took a lot of trial-and-error on my part, but I got this to work.

Great!

> It's not pretty, though.  Is this what you intended?
>
>         let start = find_last_or_zero(FILE, '/') + 1;
>         let end = find_last_or_len(FILE, '.');
>         
>         let base_ptr: *const u8 = FILE.as_ptr();
>         let ptr: *const u8 = unsafe { base_ptr.add(start) };
>         let slice = unsafe { core::slice::from_raw_parts(ptr, end - start) };
>         unsafe { core::str::from_utf8_unchecked(slice) }

Yeah that's roughly what I had in mind.

Maybe we should check that `end >= start` in case the filename is empty
(which shouldn't happen)? I'm not sure what happens with overflows /
other `unsafe` precondition violations...

> The good news is that this allows
>
> 	const SFILE: &'static str = sfile!();
>
> to work, so thank you.

That'll be useful :)

---
Cheers,
Benno

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Benno Lossin]

On Tue Jun 3, 2025 at 8:15 PM CEST, Timur Tabi wrote:
> On Thu, 2025-05-29 at 22:21 +0200, Miguel Ojeda wrote:
>> If helpers within macros are needed, and especially if the use case is
>> not just temporary debugging statements, then it may be best to put
>> them as helpers outside, to avoid generating all this code on every
>> macro invocation.
>
> That's the beauty of this macro -- it doesn't generate any code.  Not really, at least.  The
> compiler determines at compile time the slice inside file!() that this macro generates, so all that 
> happens is the compiler emits hard-coded offsets into the string literal.
>
> You can see for yourself here:  https://rust.godbolt.org/z/e947zsxfP
>
> This is the whole reason I go through such lengths to avoid rfind() and the .. operator.

It still generates source code that the compiler has to check and
remove. So it'll probably be faster in the long term if we move the
functions outside (and thus only define them once).

---
Cheers,
Benno

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Timur Tabi]

On Tue, 2025-06-03 at 23:58 +0200, Benno Lossin wrote:
> It still generates source code that the compiler has to check and
> remove. So it'll probably be faster in the long term if we move the
> functions outside (and thus only define them once).

But won't that cause the functions to always be compiled and exist in the kernel, even if sfile!()
is never used?

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Miguel Ojeda]

On Tue, Jun 3, 2025 at 8:15 PM Timur Tabi <ttabi@nvidia.com> wrote:
>
> If the full path is not too long for you, that's okay.  It is too long for me, which is why I wrote
> this macro.  So if dbg!() were to use sfile!(), it would look like this:

Why is too long for you? Please explain the use case. For instance, if
you consider `dbg!` output is too big in some cases (say, when nested
in `drivers/.../.../...`), then perhaps we want to modify that one.

And if the use case is something temporary similar to `dbg!`, then we
should probably provide a facility to help with that, rather than
asking everyone to write all the time `pr_info!("{}:{}\n", sfile!(),
line!());`.

> That's the beauty of this macro -- it doesn't generate any code.  Not really, at least.  The
> compiler determines at compile time the slice inside file!() that this macro generates, so all that
> happens is the compiler emits hard-coded offsets into the string literal.

It does generate code -- I am not talking about the codegen/assembly,
but about what the macro generates, which is code.

Cheers,
Miguel

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Miguel Ojeda]

On Wed, Jun 4, 2025 at 12:05 AM Timur Tabi <ttabi@nvidia.com> wrote:
>
> But won't that cause the functions to always be compiled and exist in the kernel, even if sfile!()
> is never used?

Not necessarily, why?

Cheers,
Miguel

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Timur Tabi]

On Wed, 2025-06-04 at 00:15 +0200, Miguel Ojeda wrote:
> On Tue, Jun 3, 2025 at 8:15 PM Timur Tabi <ttabi@nvidia.com> wrote:
> > 
> > If the full path is not too long for you, that's okay.  It is too long for me, which is why I
> > wrote
> > this macro.  So if dbg!() were to use sfile!(), it would look like this:
> 
> Why is too long for you? Please explain the use case. 

I don't know how else to explain it.  I personally think that 

	drivers/gpu/nova-core/gpu.rs:65 chipset_id=3 blabla=16

is too long, and I would prefer

	gpu:65 chipset_id=3 blabla=16

I didn't want to change the existing behavior of any other macro or function.  This is my first
significant rust-for-linux contribution, and I didn't want to be presumptuous.

> For instance, if
> you consider `dbg!` output is too big in some cases (say, when nested
> in `drivers/.../.../...`), then perhaps we want to modify that one.

We could do that, but it's not my decision to make.  Some people might have multiple files with the
same base name, and would get annoyed if suddenly their debug prints became ambiguous.

> And if the use case is something temporary similar to `dbg!`, then we
> should probably provide a facility to help with that, rather than
> asking everyone to write all the time `pr_info!("{}:{}\n", sfile!(),
> line!());`.

I could add a sdbg!() if you think it would be useful.

> > That's the beauty of this macro -- it doesn't generate any code.  Not really, at least.  The
> > compiler determines at compile time the slice inside file!() that this macro generates, so all
> > that
> > happens is the compiler emits hard-coded offsets into the string literal.
> 
> It does generate code -- I am not talking about the codegen/assembly,
> but about what the macro generates, which is code.

Oh, I didn't think that would be a problem.  I will move the functions outside the macro.

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Miguel Ojeda]

On Wed, Jun 4, 2025 at 1:29 AM Timur Tabi <ttabi@nvidia.com> wrote:
>
> I didn't want to change the existing behavior of any other macro or function.  This is my first
> significant rust-for-linux contribution, and I didn't want to be presumptuous.

No worries -- things can be changed, and in particular the behavior of
`dbg!` wouldn't be too bad, in the sense that calls are not meant to
be committed anyway. A CI checking for the message from the sample
could break I guess, but that is not the end of the world.

> I could add a sdbg!() if you think it would be useful.

Yeah, that was what I was thinking, though it is sadly one more letter.

We could also shorten a bit more by not printing the column like
`dbg!` does, which I suspect many may not care about.

I wonder if we could make the `dbg!` the one with the longer name
instead, i.e. to keep the functionality in case someone really needs
it. Sadly, that means making it not match the stdlib name, but it may
be worth it if we expect everyone to end up typing `sdbg!` all the
time, and only very rarely `dbg!`.

> Oh, I didn't think that would be a problem.  I will move the functions outside the macro.

It generally isn't a big deal, especially here since the intention was
temporary use anyway as you clarified, but if a function may be useful
in its own, then we probably want to take it out anyway, which also
"forces" us to provide proper docs and examples/tests for it, etc.

Thanks!

Cheers,
Miguel

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Benno Lossin]

On Wed Jun 4, 2025 at 12:28 PM CEST, Miguel Ojeda wrote:
> On Wed, Jun 4, 2025 at 1:29 AM Timur Tabi <ttabi@nvidia.com> wrote:
>>
>> I didn't want to change the existing behavior of any other macro or function.  This is my first
>> significant rust-for-linux contribution, and I didn't want to be presumptuous.
>
> No worries -- things can be changed, and in particular the behavior of
> `dbg!` wouldn't be too bad, in the sense that calls are not meant to
> be committed anyway. A CI checking for the message from the sample
> could break I guess, but that is not the end of the world.
>
>> I could add a sdbg!() if you think it would be useful.
>
> Yeah, that was what I was thinking, though it is sadly one more letter.
>
> We could also shorten a bit more by not printing the column like
> `dbg!` does, which I suspect many may not care about.
>
> I wonder if we could make the `dbg!` the one with the longer name
> instead, i.e. to keep the functionality in case someone really needs
> it. Sadly, that means making it not match the stdlib name, but it may
> be worth it if we expect everyone to end up typing `sdbg!` all the
> time, and only very rarely `dbg!`.

Why not control the amount of path segments with a kconfig? That way we
can have both worlds.

---
Cheers,
Benno

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Miguel Ojeda]

On Wed, Jun 4, 2025 at 5:16 PM Benno Lossin <lossin@kernel.org> wrote:
>
> Why not control the amount of path segments with a kconfig? That way we
> can have both worlds.

I thought about that, and it would work in the sense that in most
cases one does not care having both (or a number as you say) available
at the same time, but it may be annoying having to change it if you
move to another part of the tree or between builds.

I am also not sure what the default would then be (it could be
annoying both/N ways, depending on what you are doing), and avoiding
extra Kconfigs is usually nice (and avoids rebuilds). The examples
(and maybe CIs out there) could need some extra care too (e.g. if they
match the text).

So, I think it would be fine, but not sure if it is worth it vs. just
having 2 ones easily usable.

Cheers,
Miguel

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Timur Tabi]

On Wed, 2025-06-04 at 12:28 +0200, Miguel Ojeda wrote:
> 
> > I could add a sdbg!() if you think it would be useful.
> 
> Yeah, that was what I was thinking, though it is sadly one more letter.

Maybe dbs!(), or just d!() ?

> We could also shorten a bit more by not printing the column like
> `dbg!` does, which I suspect many may not care about.

Frankly, I think the column number is useless at best.

> I wonder if we could make the `dbg!` the one with the longer name
> instead, i.e. to keep the functionality in case someone really needs
> it. Sadly, that means making it not match the stdlib name, but it may
> be worth it if we expect everyone to end up typing `sdbg!` all the
> time, and only very rarely `dbg!`.

How about calling it d!() and dropping the column number? That would make it extra short.

> > Oh, I didn't think that would be a problem.  I will move the functions outside the macro.
> 
> It generally isn't a big deal, especially here since the intention was
> temporary use anyway as you clarified, but if a function may be useful
> in its own, then we probably want to take it out anyway, which also
> "forces" us to provide proper docs and examples/tests for it, etc.

I ran some tests on godbolt, and moving the functions out doesn't appear to cause any problems.

I have doubts that they would be useful anywhere else.  Remember, they exist only because rfind() is
not const.  If we fix that in the compiler, we wouldn't need these functions.

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Timur Tabi]

On Wed, 2025-06-04 at 00:15 +0200, Miguel Ojeda wrote:
> On Wed, Jun 4, 2025 at 12:05 AM Timur Tabi <ttabi@nvidia.com> wrote:
> > 
> > But won't that cause the functions to always be compiled and exist in the kernel, even if
> > sfile!()
> > is never used?
> 
> Not necessarily, why?

If I move the function outside of the macro, I need to declare the function as "pub".  If I don't do
this, then other modules that use kernel::sfile won't compile.

But in doing so, the function is compiled as a stand-alone function, even though the macro doesn't
actually use it.  The compiler apparently re-inlines the function in the macro call.  

Compare the two:

Without pub: https://rust.godbolt.org/z/T9YKs3c1r
With pub: https://rust.godbolt.org/z/qEb3x7vbj

I've never actually seen a compiler do this, to be honest, but it makes a lot of sense.  If you
compare the example::main function on both links, they are identical.  But the version with "pub"
also has a full implementation of rfind_const() that is just not used.

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Miguel Ojeda]

On Thu, Jun 5, 2025 at 1:12 AM Timur Tabi <ttabi@nvidia.com> wrote:
>
> I've never actually seen a compiler do this, to be honest, but it makes a lot of sense.  If you

I am not sure what you meant here, but this sort of thing is common
and expected -- e.g. you can see it here for C on both GCC and Clang:

    https://godbolt.org/z/3dqYznsbz

If you mark it `pub` (e.g. non-`static` in C), then the compiler needs
to "export" it. It may still be inlined, or not.

However, in Rust, even if you mark something `pub`, that does not
necessarily mean it will actually create a symbol. For instance, in
your Rust example, if you mark your public `rfind_const` as
`#[inline]`, then you will see it does not get generated anymore and
your CE outputs match again.

Moreover, if the function is small enough (and your compiler is recent
enough), Rust will do it for you.

I hope that helps.

Cheers,
Miguel

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Greg KH]

On Wed, Jun 04, 2025 at 05:16:54PM +0200, Benno Lossin wrote:
> On Wed Jun 4, 2025 at 12:28 PM CEST, Miguel Ojeda wrote:
> > On Wed, Jun 4, 2025 at 1:29 AM Timur Tabi <ttabi@nvidia.com> wrote:
> >>
> >> I didn't want to change the existing behavior of any other macro or function.  This is my first
> >> significant rust-for-linux contribution, and I didn't want to be presumptuous.
> >
> > No worries -- things can be changed, and in particular the behavior of
> > `dbg!` wouldn't be too bad, in the sense that calls are not meant to
> > be committed anyway. A CI checking for the message from the sample
> > could break I guess, but that is not the end of the world.
> >
> >> I could add a sdbg!() if you think it would be useful.
> >
> > Yeah, that was what I was thinking, though it is sadly one more letter.
> >
> > We could also shorten a bit more by not printing the column like
> > `dbg!` does, which I suspect many may not care about.
> >
> > I wonder if we could make the `dbg!` the one with the longer name
> > instead, i.e. to keep the functionality in case someone really needs
> > it. Sadly, that means making it not match the stdlib name, but it may
> > be worth it if we expect everyone to end up typing `sdbg!` all the
> > time, and only very rarely `dbg!`.
> 
> Why not control the amount of path segments with a kconfig? That way we
> can have both worlds.

Ugh, no, please no.

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Greg KH]

On Wed, Jun 04, 2025 at 08:38:12PM +0000, Timur Tabi wrote:
> On Wed, 2025-06-04 at 12:28 +0200, Miguel Ojeda wrote:
> > 
> > > I could add a sdbg!() if you think it would be useful.
> > 
> > Yeah, that was what I was thinking, though it is sadly one more letter.
> 
> Maybe dbs!(), or just d!() ?

Wait, why are we trying to reimplement the existing pr_dbg() code in a
non-standard-way?

Please just do exactly what that provides, we don't want different
logging functions in rust code from the c code, that way lies madness
and very grumpy maintainers who have to read code on both sides.

Let's just do EXACTLY what the C code does, both in functionality AND in
output to the kernel log please.

thanks,

greg k-h

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Timur Tabi]

On Thu, 2025-06-05 at 08:07 +0200, Greg KH wrote:
> > Maybe dbs!(), or just d!() ?
> 
> Wait, why are we trying to reimplement the existing pr_dbg() code in a
> non-standard-way?

pr_dbg() is not changing.  We're talking about the standard Rust macro dbg!()

	https://doc.rust-lang.org/std/macro.dbg.html

the "problem" with dbg! is that it uses file!(), which outputs the full relative path of the
source file, which can be quite lengthy.  IOW, I think dbg!() is often too noisy.

And we're not talking about changing dbg!() either.  In cases where a given Rust driver has
multiple source files of the same name but in different paths, you probably want dbg!() to use
file!().

But in situations where you don't have that file layout, maybe you would prefer the output of my
sfile!() macro.

I'll be honest, this thing has blown up more than I expected.  I got a lot of positive feedback
from my patch that has not only improved the code, but also taught me a lot about Rust itself. 
So even if this whole thing gets nack'd, I won't be too upset.  I'll just keep using sfile! for
my own purposes.

But I am excited about the *possibility* that we can make everyone happy with my code.

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[gregkh]

On Thu, Jun 05, 2025 at 03:02:03PM +0000, Timur Tabi wrote:
> On Thu, 2025-06-05 at 08:07 +0200, Greg KH wrote:
> > > Maybe dbs!(), or just d!() ?
> > 
> > Wait, why are we trying to reimplement the existing pr_dbg() code in a
> > non-standard-way?
> 
> pr_dbg() is not changing.  We're talking about the standard Rust macro dbg!()
> 
> 	https://doc.rust-lang.org/std/macro.dbg.html
> 
> the "problem" with dbg! is that it uses file!(), which outputs the full relative path of the
> source file, which can be quite lengthy.  IOW, I think dbg!() is often too noisy.

And I'm saying "don't use dbg!()" :)

Or, better yet, have it do exactly what pr_dbg() does, as we don't seem
to _have_ to have dbg!() follow anything other than whatever we want it
to follow.

> And we're not talking about changing dbg!() either.  In cases where a given Rust driver has
> multiple source files of the same name but in different paths, you probably want dbg!() to use
> file!().

Again, just do what pr_dbg() does, nothing more and nothing less.

> But in situations where you don't have that file layout, maybe you would prefer the output of my
> sfile!() macro.

Nope, use what pr_dbg() does please.  It will provide the needed file
info if the user asks for it through the standard user/kernel api that
we all know and "love".

thanks,

greg k-h

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Miguel Ojeda]

On Thu, Jun 5, 2025 at 5:21 PM gregkh@linuxfoundation.org
<gregkh@linuxfoundation.org> wrote:
>
> And I'm saying "don't use dbg!()" :)

You likely know all this, but in case it helps to clarify the
discussion: `dbg!` is only meant for temporary code, i.e. so it
shouldn't be used (in the sense of committing into Git).

That is, it is a development tool, unlike `pr_dbg!`.

It is also fairly different in interface: it is meant to be trivial to
use, taking and returning a single expression (essentially). That
means you can even use it within other expressions, without having to
deal with formatting etc. For instance, if you have some code like:

    let x = f() + g();

Then you can temporarily "inject" it there:

    let x = dbg!(f()) + dbg!(g());

To get an output like:

    [example.rs:10:13] f() = 42
    [example.rs:10:25] g() = 43

That is also why we picked `pr_info!` instead of higher levels, since
if you use `dbg!`, it is because you really want to see the output in
the log (and we perhaps could use a lower level, for that matter).

Now, we could look into changing its output to be something that looks
more like a usual print, of course. On the other hand, making it very
visible/different than the rest of the kernel log is also the point.

I hope that explains why I think we shouldn't just do exactly what
`pr_dbg!()` does.

Cheers,
Miguel

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[gregkh]

On Thu, Jun 05, 2025 at 05:38:56PM +0200, Miguel Ojeda wrote:
> On Thu, Jun 5, 2025 at 5:21 PM gregkh@linuxfoundation.org
> <gregkh@linuxfoundation.org> wrote:
> >
> > And I'm saying "don't use dbg!()" :)
> 
> You likely know all this, but in case it helps to clarify the
> discussion: `dbg!` is only meant for temporary code, i.e. so it
> shouldn't be used (in the sense of committing into Git).
> 
> That is, it is a development tool, unlike `pr_dbg!`.

Ah, I didn't realize this, so thanks.  The dbg!() implementation in the
kernel tree matches the userspace stuff, so the documentation refers to
"release builds" and the like.  It would be good if we could say
something like "Use ONLY for local debugging code, NEVER submit a patch
with this used in it."

Or we can add it to checkpatch to catch when people inevitably do submit
patches with it?  :)

thanks,

greg k-h

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Miguel Ojeda]

On Thu, Jun 5, 2025 at 6:42 PM gregkh@linuxfoundation.org
<gregkh@linuxfoundation.org> wrote:
>
> Ah, I didn't realize this, so thanks.  The dbg!() implementation in the
> kernel tree matches the userspace stuff, so the documentation refers to
> "release builds" and the like.  It would be good if we could say
> something like "Use ONLY for local debugging code, NEVER submit a patch
> with this used in it."

You're welcome!

So we added this at the bottom of those docs:

    Note that the macro is intended as a temporary debugging tool to
be used during development. Therefore, avoid committing `dbg!` macro
invocations into the kernel tree.

But, you are right, we may want to remove the sentence above that one
about "release builds" (or, rather, replace it with some of what I
wrote in the previous reply).

> Or we can add it to checkpatch to catch when people inevitably do submit
> patches with it?  :)

We (are supposed to) have something better -- we disallow the macro in
Clippy, i.e. one should get a warning about it:

    disallowed-macros = [
        # The `clippy::dbg_macro` lint only works with `std::dbg!`,
thus we simulate
        # it here, see: https://github.com/rust-lang/rust-clippy/issues/11303.
        { path = "kernel::dbg", reason = "the `dbg!` macro is intended
as a debugging tool", allow-invalid = true },
    ]

Sadly, I reported false negatives with it a while ago:

    https://github.com/rust-lang/rust-clippy/issues/11431

And it appears that Clippy is not recognizing `kernel::dbg` at the
moment -- commit c016722fd575 ("rust: clean Rust 1.88.0's warning
about `clippy::disallowed_macros` configuration") has some more
details.

But eventually it should work. Worst case, indeed, we can move it to
`checkpatch.pl`.

Cheers,
Miguel

Re: [PATCH] rust: introduce sfile macro for easier code tracing

[Miguel Ojeda]

On Thu, Jun 5, 2025 at 5:02 PM Timur Tabi <ttabi@nvidia.com> wrote:
>
> But I am excited about the *possibility* that we can make everyone happy with my code.

Another possibility to consider is outputting the last N characters,
possibly without the `.rs` nor the column number, which gives us also
the chance to fix the size for the line number too, e.g.

    [  rust/kernel/lib:1   ]
    [l/sync/lock/mutex:12  ]
    [ova-core/firmware:123 ]
    [gpu/nova-core/gpu:1234]

But this is a compromise solution, and sometimes it is just better to
provide 2 different solutions.

Cheers,
Miguel