[0/6] KUnit integration for Rust doctests

  This is the initial KUnit integration for running Rust documentation
tests within the kernel.

Thank you to the KUnit team for all the input and feedback on this
over the months, as well as the Intel LKP 0-Day team!

This may be merged through either the KUnit or the Rust trees. If
the KUnit team wants to merge it, then that would be great.

Please see the message in the main commit for the details.


Miguel Ojeda (6):
  rust: init: make doctests compilable/testable
  rust: str: make doctests compilable/testable
  rust: sync: make doctests compilable/testable
  rust: types: make doctests compilable/testable
  rust: support running Rust documentation tests as KUnit ones
  MAINTAINERS: add Rust KUnit files to the KUnit entry

 MAINTAINERS                       |   2 +
 lib/Kconfig.debug                 |  13 +++
 rust/.gitignore                   |   2 +
 rust/Makefile                     |  29 ++++++
 rust/bindings/bindings_helper.h   |   1 +
 rust/helpers.c                    |   7 ++
 rust/kernel/init.rs               |  25 +++--
 rust/kernel/kunit.rs              | 156 ++++++++++++++++++++++++++++
 rust/kernel/lib.rs                |   2 +
 rust/kernel/str.rs                |   4 +-
 rust/kernel/sync/arc.rs           |   9 +-
 rust/kernel/sync/lock/mutex.rs    |   1 +
 rust/kernel/sync/lock/spinlock.rs |   1 +
 rust/kernel/types.rs              |   6 +-
 scripts/.gitignore                |   2 +
 scripts/Makefile                  |   4 +
 scripts/rustdoc_test_builder.rs   |  73 ++++++++++++++
 scripts/rustdoc_test_gen.rs       | 162 ++++++++++++++++++++++++++++++
 18 files changed, 484 insertions(+), 15 deletions(-)
 create mode 100644 rust/kernel/kunit.rs
 create mode 100644 scripts/rustdoc_test_builder.rs
 create mode 100644 scripts/rustdoc_test_gen.rs


base-commit: d2e3115d717197cb2bc020dd1f06b06538474ac3
  

On Wed, Jun 14, 2023 at 08:08:24PM +0200, Miguel Ojeda wrote:
> This is the initial KUnit integration for running Rust documentation
> tests within the kernel.
> 
> Thank you to the KUnit team for all the input and feedback on this
> over the months, as well as the Intel LKP 0-Day team!
> 
> This may be merged through either the KUnit or the Rust trees. If
> the KUnit team wants to merge it, then that would be great.
> 
> Please see the message in the main commit for the details.
> 

Great work! I've played this for a while, and it's really useful ;-)

One thing though, maybe we can provide more clues for users to locate
the corresponding Doctests? For example, I did the following to trigger
an assertion:

	diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs
	index 91eb2c9e9123..9ead152e2c7e 100644
	--- a/rust/kernel/sync/lock/spinlock.rs
	+++ b/rust/kernel/sync/lock/spinlock.rs
	@@ -58,7 +58,7 @@ macro_rules! new_spinlock {
	 ///
	 /// // Allocate a boxed `Example`.
	 /// let e = Box::pin_init(Example::new())?;
	-/// assert_eq!(e.c, 10);
	+/// assert_eq!(e.c, 11);
	 /// assert_eq!(e.d.lock().a, 20);
	 /// assert_eq!(e.d.lock().b, 30);
	 /// # Ok::<(), Error>(())

Originally I got:

	[..] # Doctest from line 35
	[..] # rust_doctest_kernel_sync_lock_spinlock_rs_0: ASSERTION FAILED at rust/doctests_kernel_generated.rs:2437
	[..] Expected e.c == 11 to be true, but is false
	[..] [FAILED] rust_doctest_kernel_sync_lock_spinlock_rs_0

The assertion warning only says line 35 but which file? Yes, the
".._sync_lock_spinlock_rs" name does provide the lead, however since we
generate the test code, so we actually know the line # for each real
test body, so I come up a way to give us the following:

	[..] # rust_doctest_kernel_sync_lock_spinlock_rs_0: ASSERTION FAILED at rust/kernel/sync/lock/spinlock.rs:61
	[..] Expected e.c == 11 to be true, but is false
	[..] [FAILED] rust_doctest_kernel_sync_lock_spinlock_rs_0

Thoughts?

Regards,
Boqun

----------------->8
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index 3c94efcd7f76..807fe3633567 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -49,15 +49,15 @@ pub fn info(args: fmt::Arguments<'_>) {
 #[doc(hidden)]
 #[macro_export]
 macro_rules! kunit_assert {
-    ($name:literal, $condition:expr $(,)?) => {
+    ($name:literal, $diff:expr, $file:expr, $condition:expr $(,)?) => {
         'out: {
             // Do nothing if the condition is `true`.
             if $condition {
                 break 'out;
             }
 
-            static LINE: i32 = core::line!() as i32;
-            static FILE: &'static $crate::str::CStr = $crate::c_str!(core::file!());
+            static LINE: i32 = core::line!() as i32 - $diff;
+            static FILE: &'static $crate::str::CStr = $crate::c_str!($file);
             static CONDITION: &'static $crate::str::CStr = $crate::c_str!(stringify!($condition));
 
             // SAFETY: FFI call without safety requirements.
@@ -148,9 +148,9 @@ unsafe impl Sync for UnaryAssert {}
 #[doc(hidden)]
 #[macro_export]
 macro_rules! kunit_assert_eq {
-    ($name:literal, $left:expr, $right:expr $(,)?) => {{
+    ($name:literal, $diff:expr, $file:expr, $left:expr, $right:expr $(,)?) => {{
         // For the moment, we just forward to the expression assert because, for binary asserts,
         // KUnit supports only a few types (e.g. integers).
-        $crate::kunit_assert!($name, $left == $right);
+        $crate::kunit_assert!($name, $diff, $file, $left == $right);
     }};
 }
diff --git a/scripts/rustdoc_test_gen.rs b/scripts/rustdoc_test_gen.rs
index 793885c32c0d..4786a2ef0dc6 100644
--- a/scripts/rustdoc_test_gen.rs
+++ b/scripts/rustdoc_test_gen.rs
@@ -75,6 +75,11 @@ fn main() {
 
         let line = line.parse::<core::ffi::c_int>().unwrap();
 
+        let src_file = format!("rust/kernel/{}", file.replace("_rs", ".rs").replace("_", "/"));
+
+        // Calculate how many lines before `main` function (including the `main` function line).
+        let body_offset = body.lines().take_while(|l| !l.contains("fn main() {")).count() + 1;
+
         use std::fmt::Write;
         write!(
             rust_tests,
@@ -85,7 +90,7 @@ pub extern "C" fn {kunit_name}(__kunit_test: *mut kernel::bindings::kunit) {{
     #[allow(unused)]
     macro_rules! assert {{
         ($cond:expr $(,)?) => {{{{
-            kernel::kunit_assert!("{kunit_name}", $cond);
+            kernel::kunit_assert!("{kunit_name}", anchor - {line}, "{src_file}", $cond);
         }}}}
     }}
 
@@ -93,7 +98,7 @@ macro_rules! assert {{
     #[allow(unused)]
     macro_rules! assert_eq {{
         ($left:expr, $right:expr $(,)?) => {{{{
-            kernel::kunit_assert_eq!("{kunit_name}", $left, $right);
+            kernel::kunit_assert_eq!("{kunit_name}", anchor - {line}, "{src_file}", $left, $right);
         }}}}
     }}
 
@@ -101,9 +106,8 @@ macro_rules! assert_eq {{
     #[allow(unused)]
     use kernel::prelude::*;
 
-    // Display line number so that developers can map the test easily to the source code.
-    kernel::kunit::info(format_args!("    # Doctest from line {line}\n"));
-
+    // The anchor where the test code body starts.
+    static anchor: i32 = core::line!() as i32 + {body_offset} + 1;
     {{
         {body}
         main();
  

On Thu, Jun 15, 2023 at 3:44 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Great work! I've played this for a while, and it's really useful ;-)

Thanks!

> The assertion warning only says line 35 but which file? Yes, the
> ".._sync_lock_spinlock_rs" name does provide the lead, however since we
> generate the test code, so we actually know the line # for each real
> test body, so I come up a way to give us the following:
>
>         [..] # rust_doctest_kernel_sync_lock_spinlock_rs_0: ASSERTION FAILED at rust/kernel/sync/lock/spinlock.rs:61
>         [..] Expected e.c == 11 to be true, but is false
>         [..] [FAILED] rust_doctest_kernel_sync_lock_spinlock_rs_0
>
> Thoughts?

Sounds good to me. However, David/Philip, is this OK or do you really
need/use the actual/compiled source file there? If you don't need it,
does it need to exist / be a real file at least? If the latter answer
is a "yes", which I guess it may be likely, then:

> +        let src_file = format!("rust/kernel/{}", file.replace("_rs", ".rs").replace("_", "/"));

This would not work for files with a `_` in their name, like
`locked_by.rs`. I guess we could still find the real filename based on
that information walking the dir, which is another hack I recall
considering at some point.

Otherwise, if "fake" filenames in the line above are OK for
David/Philip (I suspect they may want to open them for reporting?),
then I guess the `file` one may be good enough and eventually we
should get `rustdoc` to give us the proper metadata anyway.

Cheers,
Miguel
  

On Thu, 15 Jun 2023 at 16:20, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Thu, Jun 15, 2023 at 3:44 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > Great work! I've played this for a while, and it's really useful ;-)
>
> Thanks!
>
> > The assertion warning only says line 35 but which file? Yes, the
> > ".._sync_lock_spinlock_rs" name does provide the lead, however since we
> > generate the test code, so we actually know the line # for each real
> > test body, so I come up a way to give us the following:
> >
> >         [..] # rust_doctest_kernel_sync_lock_spinlock_rs_0: ASSERTION FAILED at rust/kernel/sync/lock/spinlock.rs:61
> >         [..] Expected e.c == 11 to be true, but is false
> >         [..] [FAILED] rust_doctest_kernel_sync_lock_spinlock_rs_0
> >
> > Thoughts?

I like it.

A part of me would like to keep the
kernel::kunit::info(format_args!("    # Doctest from line {line}\n"));

If only so we can preserve the line information when the tests
actually pass. This is something that'd probably ultimately fit as a
"test attribute":
https://lore.kernel.org/linux-kselftest/20230610005149.1145665-1-rmoar@google.com/

For C tests we're not bothering outputting line numbers now (though
again, are considering if we can do it with an attribute), but those
tests have much more searchable names, so I think it still makes sense
for the Rust ones.

How about printing something like:
    # source_line: {}

kunit.py will hide this when the test passes unless the user
explicitly passes --raw_output.

>
> Sounds good to me. However, David/Philip, is this OK or do you really
> need/use the actual/compiled source file there? If you don't need it,
> does it need to exist / be a real file at least? If the latter answer
> is a "yes", which I guess it may be likely, then:

I don't think there's anything automated using the file:line in
assertion messages, it's just for human consumption.
This may change in the future (and there are probably some text
editors around which will turn a path like that into, e.g., a
clickable link now), but we're not currently doing anything which
would actually open the file.

>
> > +        let src_file = format!("rust/kernel/{}", file.replace("_rs", ".rs").replace("_", "/"));
>
> This would not work for files with a `_` in their name, like
> `locked_by.rs`. I guess we could still find the real filename based on
> that information walking the dir, which is another hack I recall
> considering at some point.
>
> Otherwise, if "fake" filenames in the line above are OK for
> David/Philip (I suspect they may want to open them for reporting?),
> then I guess the `file` one may be good enough and eventually we
> should get `rustdoc` to give us the proper metadata anyway.

Personally, I'd think a "fake filename" is okay (especially if it's
temporary until we can get the right one), though I'd prefer there to
be some indication that it's "fake": maybe leaving the _ separator in,
or wrapping it in brackets, or something? Unless the whole
walk-the-filesystem technique ends up being worth doing, so we don't
have a significant chance of the filename being dud.

-- David
  

On Thu, 15 Jun 2023 at 02:09, Miguel Ojeda <ojeda@kernel.org> wrote:
>
> This is the initial KUnit integration for running Rust documentation
> tests within the kernel.
>
> Thank you to the KUnit team for all the input and feedback on this
> over the months, as well as the Intel LKP 0-Day team!
>
> This may be merged through either the KUnit or the Rust trees. If
> the KUnit team wants to merge it, then that would be great.
>
> Please see the message in the main commit for the details.
>
>

Thanks very much for putting this together! I've been looking forward
to it, and it works well here.

I've been running it on linux-next to get both the pending KUnit and
Rust changes, and it works well apart from needing to fix a couple of
conflicts from
https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=260755184cbdb267a046e7ffd397c1d2ba09bb5e

In particular, the tests run with:
./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_RUST=y
--make_options LLVM=1 'rust_doctests_kernel'

And also under QEMU / x86_64 with:
./tools/testing/kunit/kunit.py run --arch x86_64 --kconfig_add
CONFIG_RUST=y --make_options LLVM=1 'rust_doctests_kernel'

(And I'm looking forward to trying out the other architecture support
patches with it, too)

The doctests also run nicely as part of the default test suite when
CONFIG_RUST=y. At some point, we might want to add a Rust-specific
.kunitconfig to make it easier to just run Rust-related test suites,
but it's not a big deal for just these.

I assume we'll take this in via the kselftest/kunit tree for 6.6, but
if you'd rather take them via the Rust tree, that's fine too.

Cheers,
-- David

> Miguel Ojeda (6):
>   rust: init: make doctests compilable/testable
>   rust: str: make doctests compilable/testable
>   rust: sync: make doctests compilable/testable
>   rust: types: make doctests compilable/testable
>   rust: support running Rust documentation tests as KUnit ones
>   MAINTAINERS: add Rust KUnit files to the KUnit entry
>
>  MAINTAINERS                       |   2 +
>  lib/Kconfig.debug                 |  13 +++
>  rust/.gitignore                   |   2 +
>  rust/Makefile                     |  29 ++++++
>  rust/bindings/bindings_helper.h   |   1 +
>  rust/helpers.c                    |   7 ++
>  rust/kernel/init.rs               |  25 +++--
>  rust/kernel/kunit.rs              | 156 ++++++++++++++++++++++++++++
>  rust/kernel/lib.rs                |   2 +
>  rust/kernel/str.rs                |   4 +-
>  rust/kernel/sync/arc.rs           |   9 +-
>  rust/kernel/sync/lock/mutex.rs    |   1 +
>  rust/kernel/sync/lock/spinlock.rs |   1 +
>  rust/kernel/types.rs              |   6 +-
>  scripts/.gitignore                |   2 +
>  scripts/Makefile                  |   4 +
>  scripts/rustdoc_test_builder.rs   |  73 ++++++++++++++
>  scripts/rustdoc_test_gen.rs       | 162 ++++++++++++++++++++++++++++++
>  18 files changed, 484 insertions(+), 15 deletions(-)
>  create mode 100644 rust/kernel/kunit.rs
>  create mode 100644 scripts/rustdoc_test_builder.rs
>  create mode 100644 scripts/rustdoc_test_gen.rs
>
>
> base-commit: d2e3115d717197cb2bc020dd1f06b06538474ac3
> --
> 2.41.0
>