[PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support

[PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support

[Nicolás Antinori]

This patch series intends to implement two features for KUnit tests
written in Rust. The work is based on a TODO comment made in the
`bitmap.rs` module [1].

1. Supporting `#[should_panic]` [2]:

KUnit tests in Rust follow the user-space syntax, but at the moment
`#[should_panic]` is not supported. The first patch of this series adds
support for the attribute (only in its basic form, `#[should_panic =
"message"]` is not supported, and I don't know if it makes sense to
support it)

The way it is supported is by having a separate `#[panic_handler]` when
`CONFIG_KUNIT` is enabled. When a test is marked with `#[should_panic]`,
a static value (KUNIT_SHOULD_PANIC = 0xDEAD7357) is assigned to the
kunit's `priv` field, since it is meant for saving arbitrary user data
[3]. At the moment, I did not find any place where Rust tests use that
field, so it should be safe to write it.

When the test panics, the `#[panic_handler]` function is called, obtains
the kunit current test and checks if the `priv` field is not null and
contains the value `KUNIT_SHOULD_PANIC`.

If those conditions are true, it marks the test as successful (since it
panicked as expected) and calls `__kunit_abort_expecting_error`, a new
function that exits the testing thread but fills `try_catch->try_result`
with a 0 so the test runner does not mistake it as a failed test.

If those conditions are not true:
 - If `priv` is null, the test panics as it would have before having
   this feature, priv = null means that the test was not expected to
   panic.
 - If `priv` is not null but its value is not `KUNIT_SHOULD_PANIC`, the
   test panics with an error message informing that the code found in
   `priv` was invalid.

If the test does not panic, the `#[panic_handler]` is not triggered. The
test is marked as failed (since it was expected to panic).

Regarding this feature:
 - Do this approach make sense?
 - Is it ok to mark the `#[should_panic]` tests with a static constant?
   Is another mechanism better to check in the `#[panic_handler]` that
   the test was supposed to panic?

2. Allow same test name with different #[cfg(...)]:

When testing `#[should_panic]` in `bitmap.rs` (check the last patch of
the series) I found that the test that was supposed to panic had the
same name as another one, but they were run on different configurations.
This caused the following compilation error:

ERROR:root:error[E0428]: the name `kunit_rust_wrapper_owned_bitmap_out_of_bounds` is defined multiple times
   --> ../rust/kernel/bitmap.rs:503:1
    |
503 | #[macros::kunit_tests(rust_kernel_bitmap)]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `kunit_rust_wrapper_owned_bitmap_out_of_bounds` redefined here
    |
    = note: `kunit_rust_wrapper_owned_bitmap_out_of_bounds` must be defined only once in the value namespace of this module
    = note: this error originates in the attribute macro `macros::kunit_tests` (in Nightly builds, run with -Z macro-backtrace for more info)


To fix this problem, I appended to the `kunit_rust_wrapper_*`
identifiers an 'stringified' version of the test's #[cfg(...)]
arguments. The purpose of this is that, if we have a test with the same
name and configuration, it would fail.

The configuration string was also appended to the tests names. This was
done to have a better test run report:

...
[SKIPPED] owned_bitmap_out_of_bounds_cfg_not_config_rust_bitmap_hardened
[PASSED] owned_bitmap_out_of_bounds_cfg_config_rust_bitmap_hardened
...

Otherwise we would have something like the following:
...
[SKIPPED] owned_bitmap_out_of_bounds
[PASSED] owned_bitmap_out_of_bounds
...

Regarding this:
- Does it makes sense to allow same test names with different cfgs?
- Is it ok to 'stringify' the configuration so it can be distinguished
  in the report? Would you prefer something like `_case_1` `_case_2` ..
  instead?

This is the first RFC patch I send to the LKML, if there's something not
right with it please let me know.

Kind Regards,
Nicolás

[1] https://github.com/Rust-for-Linux/linux/blob/fd73f4a6659897191fa0d40695fe370925dd3780/rust/kernel/bitmap.rs#L592-L600
[2] https://doc.rust-lang.org/rust-by-example/testing/unit_testing.html#testing-panics
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/kunit/test.h?id=f6e7b42bf05b2427fb8a7a1d1c387a86638bb413#n314

Nicolás Antinori (3):
  rust: kunit: add #[should_panic] support
  rust: kunit: allow same test name with different #[cfg(...)]
  rust: bitmap: kunit: uncomment owned_bitmap_out_of_bounds panic case

 include/kunit/test.h      |  1 +
 include/kunit/try-catch.h |  1 +
 lib/kunit/test.c          | 14 +++++++
 lib/kunit/try-catch.c     |  7 ++++
 rust/kernel/bitmap.rs     | 40 +++++++++-----------
 rust/kernel/kunit.rs      | 24 ++++++++++++
 rust/kernel/lib.rs        | 46 +++++++++++++++++++++--
 rust/macros/kunit.rs      | 78 ++++++++++++++++++++++++++++++++++++---
 8 files changed, 181 insertions(+), 30 deletions(-)

--
2.47.3

[PATCH RFC 1/3] rust: kunit: add #[should_panic] support

[Nicolás Antinori]

KUnit tests in Rust are written using user-space like syntax. This patch
adds support for the `#[should_panic]` attribute, enabling the user to test
conditions that are expected to cause a panic and report the test as
successful.

Signed-off-by: Nicolás Antinori <nico.antinori.7@gmail.com>
---
 include/kunit/test.h      |  1 +
 include/kunit/try-catch.h |  1 +
 lib/kunit/test.c          | 14 ++++++++++++
 lib/kunit/try-catch.c     |  7 ++++++
 rust/kernel/kunit.rs      |  9 ++++++++
 rust/kernel/lib.rs        | 46 ++++++++++++++++++++++++++++++++++++---
 rust/macros/kunit.rs      | 21 ++++++++++++++++--
 7 files changed, 94 insertions(+), 5 deletions(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index da5312e0dfa5..8b42f431e1c1 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -723,6 +723,7 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
 #define KUNIT_SUCCEED(test) _KUNIT_SAVE_LOC(test)

 void __noreturn __kunit_abort(struct kunit *test);
+void __noreturn __kunit_abort_expecting_error(struct kunit *test);

 void __printf(6, 7) __kunit_do_failed_assertion(struct kunit *test,
 						const struct kunit_loc *loc,
diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
index d4e1a5b98ed6..a47b1cfbcf93 100644
--- a/include/kunit/try-catch.h
+++ b/include/kunit/try-catch.h
@@ -54,6 +54,7 @@ struct kunit_try_catch {
 void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context);

 void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch);
+void __noreturn kunit_try_catch_throw_expecting_error(struct kunit_try_catch *try_catch);

 static inline int kunit_try_catch_get_result(struct kunit_try_catch *try_catch)
 {
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 09e3dabfac0c..32e5419a2d52 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -323,6 +323,20 @@ void __noreturn __kunit_abort(struct kunit *test)
 }
 EXPORT_SYMBOL_GPL(__kunit_abort);

+void __noreturn __kunit_abort_expecting_error(struct kunit *test)
+{
+	kunit_try_catch_throw_expecting_error(&test->try_catch); /* Does not return. */
+
+	/*
+	 * Throw could not abort from test.
+	 *
+	 * XXX: we should never reach this line! As kunit_try_catch_throw_expecting_error
+	 * is marked __noreturn.
+	 */
+	WARN_ONCE(true, "Throw could not abort from test!\n");
+}
+EXPORT_SYMBOL_GPL(__kunit_abort_expecting_error);
+
 void __kunit_do_failed_assertion(struct kunit *test,
 			       const struct kunit_loc *loc,
 			       enum kunit_assert_type type,
diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
index d84a879f0a78..123e1f86a5b3 100644
--- a/lib/kunit/try-catch.c
+++ b/lib/kunit/try-catch.c
@@ -22,6 +22,13 @@ void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch)
 }
 EXPORT_SYMBOL_GPL(kunit_try_catch_throw);

+void __noreturn kunit_try_catch_throw_expecting_error(struct kunit_try_catch *try_catch)
+{
+	try_catch->try_result = 0;
+	kthread_exit(0);
+}
+EXPORT_SYMBOL_GPL(kunit_try_catch_throw_expecting_error);
+
 static int kunit_generic_run_threadfn_adapter(void *data)
 {
 	struct kunit_try_catch *try_catch = data;
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index 91eaff8c186a..65a1040ee2b0 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -9,6 +9,9 @@
 use crate::fmt;
 use crate::prelude::*;

+#[doc(hidden)]
+pub static KUNIT_SHOULD_PANIC: u32 = 0xDEAD7357;
+
 /// Prints a KUnit error-level message.
 ///
 /// Public but hidden since it should only be used from KUnit generated code.
@@ -345,6 +348,12 @@ fn rust_test_kunit_in_kunit_test() {
         assert!(in_kunit_test());
     }

+    #[test]
+    #[should_panic]
+    fn rust_test_kunit_panic_in_kunit_test() {
+        panic!("This test should panic and pass");
+    }
+
     #[test]
     #[cfg(not(all()))]
     fn rust_test_kunit_always_disabled_test() {
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 4d5c96ddc49c..0f6c3c00ddd5 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -174,14 +174,54 @@ impl ModuleMetadata for LocalModule {
     };
 }

-#[cfg(not(testlib))]
-#[panic_handler]
-fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
+#[inline]
+fn bug_on_panic(info: &core::panic::PanicInfo<'_>) -> ! {
     pr_emerg!("{}\n", info);
     // SAFETY: FFI call.
     unsafe { bindings::BUG() };
 }

+#[cfg(all(not(testlib), not(CONFIG_KUNIT)))]
+#[panic_handler]
+fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
+    bug_on_panic(info);
+}
+
+#[cfg(all(not(testlib), CONFIG_KUNIT))]
+#[panic_handler]
+fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
+    // SAFETY: This function is safe to call even if CONFIG_KUNIT=n. If a null pointer is returned,
+    // the panic is handled same as if CONFIG_KUNIT=n.
+    let kunit_test = unsafe { ::bindings::kunit_get_current_test() };
+    if kunit_test.is_null() {
+        bug_on_panic(info);
+    } else {
+        // SAFETY: We are in the else branch of kunit_test.is_null() condition, meaning that
+        // `::bindings::kunit_get_current_test()` returned a kunit struct successfully.
+        let should_panic_ptr: *const u32 = unsafe { (*kunit_test).priv_ as *const u32 };
+        let should_panic_code: u32 = if should_panic_ptr.is_null() {
+            bug_on_panic(info);
+        } else {
+            // SAFETY: Already tested that the should_panic_ptr pointer is not null, casting it to
+            // its value should be safe since kunit_test is not null and KUnit Rust tests are
+            // initialized by assigning either null or a u32 value to the priv_ field.
+            unsafe { *should_panic_ptr }
+        };
+
+        if should_panic_code == crate::kunit::KUNIT_SHOULD_PANIC {
+            // SAFETY: We are in the else branch of kunit_test.is_null() condition, meaning that
+            // `::bindings::kunit_get_current_test()` returned a kunit struct successfully.
+            unsafe {
+                (*kunit_test).status = ::kernel::bindings::kunit_status_KUNIT_SUCCESS;
+                bindings::__kunit_abort_expecting_error(kunit_test);
+            };
+        } else {
+            pr_emerg!("Invalid KUnit priv_ code 0x{:x}\n", should_panic_code);
+            bug_on_panic(info);
+        }
+    }
+}
+
 /// Produces a pointer to an object from a pointer to one of its fields.
 ///
 /// If you encounter a type mismatch due to the [`Opaque`] type, then use [`Opaque::cast_into`] or
diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs
index ae20ed6768f1..2c6405cebc0a 100644
--- a/rust/macros/kunit.rs
+++ b/rust/macros/kunit.rs
@@ -106,6 +106,11 @@ pub(crate) fn kunit_tests(test_suite: Ident, mut module: ItemMod) -> Result<Toke
             .cloned()
             .collect();

+        let should_panic = f
+            .attrs
+            .iter()
+            .any(|attr| attr.path().is_ident("should_panic"));
+
         // Before the test, override usual `assert!` and `assert_eq!` macros with ones that call
         // KUnit instead.
         let test_str = test.to_string();
@@ -135,6 +140,19 @@ macro_rules! assert_eq {
             &CString::new(test_str.as_str()).expect("identifier cannot contain NUL"),
             test.span(),
         );
+        let assertion = if should_panic {
+            quote!(
+                (*_test).priv_ = &raw const crate::kunit::KUNIT_SHOULD_PANIC as *mut ffi::c_void;
+                let _ = #test();
+                (*_test).status = ::kernel::bindings::kunit_status_KUNIT_FAILURE;
+            )
+        } else {
+            quote!(
+                (*_test).priv_ = core::ptr::null_mut();
+                use ::kernel::kunit::is_test_result_ok;
+                assert!(is_test_result_ok(#test()));
+            )
+        };
         processed_items.push(parse_quote! {
             unsafe extern "C" fn #kunit_wrapper_fn_name(_test: *mut ::kernel::bindings::kunit) {
                 (*_test).status = ::kernel::bindings::kunit_status_KUNIT_SKIPPED;
@@ -145,8 +163,7 @@ macro_rules! assert_eq {
                 #(#cfg_attrs)*
                 {
                     (*_test).status = ::kernel::bindings::kunit_status_KUNIT_SUCCESS;
-                    use ::kernel::kunit::is_test_result_ok;
-                    assert!(is_test_result_ok(#test()));
+                    #assertion
                 }
             }
         });
--
2.47.3

[PATCH RFC 2/3] rust: kunit: allow same test name with different #[cfg(...)]

[Nicolás Antinori]

Sometimes it is necessary to test the same code paths under different
configurations. The `#[cfg(...)]` macro can be used to check if specific
configurations are enabled and run tests accordingly.

Currently, defining multiple tests with the same name under different
`#[cfg(...)]` attributes results in a compilation error. This patch removes
that restriction, allowing identical test names across different
configurations. Additionally, it appends the active configuration to the
test name, ensuring the runner clearly indicates which test executed and
which was skipped.

Signed-off-by: Nicolás Antinori <nico.antinori.7@gmail.com>
---
 rust/kernel/kunit.rs | 15 ++++++++++++
 rust/macros/kunit.rs | 57 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index 65a1040ee2b0..613c8d2aea78 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -348,6 +348,21 @@ fn rust_test_kunit_in_kunit_test() {
         assert!(in_kunit_test());
     }

+    // Both tests with cfg have the same name on purpose because we are implicitly testing that
+    // tests with the same name but different configs do not throw a compilation error
+    #[test]
+    #[cfg(CONFIG_RUST_KUNIT_SELFTEST = "y")]
+    fn rust_test_kunit_parse_cfg_in_kunit_test() {
+        assert!(in_kunit_test());
+    }
+
+    #[test]
+    #[cfg(CONFIG_RUST_KUNIT_SELFTEST = "n")]
+    fn rust_test_kunit_parse_cfg_in_kunit_test() {
+        // This test should never run because of the `cfg`.
+        assert!(false)
+    }
+
     #[test]
     #[should_panic]
     fn rust_test_kunit_panic_in_kunit_test() {
diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs
index 2c6405cebc0a..605d925cd8f0 100644
--- a/rust/macros/kunit.rs
+++ b/rust/macros/kunit.rs
@@ -6,14 +6,19 @@

 use std::ffi::CString;

-use proc_macro2::TokenStream;
+use proc_macro2::{
+    TokenStream,
+    TokenTree, //
+};
 use quote::{
     format_ident,
     quote,
     ToTokens, //
 };
 use syn::{
+    parse::ParseStream,
     parse_quote,
+    Attribute,
     Error,
     Ident,
     Item,
@@ -22,6 +27,46 @@
     Result, //
 };

+fn get_cfg_string(attr: &Attribute) -> Result<String> {
+    let mut result = String::from("_cfg");
+    attr.parse_args_with(|input: ParseStream<'_>| {
+        while !input.is_empty() {
+            build_cfg_string(input.parse()?, &mut result)?;
+        }
+        Ok(result)
+    })
+}
+
+fn build_cfg_string(tt: TokenTree, result: &mut String) -> Result<()> {
+    match tt {
+        TokenTree::Ident(ident) => {
+            result.push('_');
+            result.push_str(&ident.to_string().to_lowercase());
+        }
+        TokenTree::Punct(ref punct) => match punct.as_char() {
+            '=' => {
+                result.push_str("_equals");
+            }
+            _ => {
+                return Err(Error::new_spanned(
+                    punct,
+                    "only \"=\" is allowed to check configurations",
+                ))
+            }
+        },
+        TokenTree::Literal(lit) => {
+            result.push('_');
+            result.push_str(&lit.to_string().trim_matches('"').to_string());
+        }
+        TokenTree::Group(group) => {
+            for group_tt in group.stream() {
+                build_cfg_string(group_tt, result)?;
+            }
+        }
+    }
+    Ok(())
+}
+
 pub(crate) fn kunit_tests(test_suite: Ident, mut module: ItemMod) -> Result<TokenStream> {
     if test_suite.to_string().len() > 255 {
         return Err(Error::new_spanned(
@@ -106,6 +151,12 @@ pub(crate) fn kunit_tests(test_suite: Ident, mut module: ItemMod) -> Result<Toke
             .cloned()
             .collect();

+        let cfg_attrs_str = cfg_attrs
+            .iter()
+            .map(get_cfg_string)
+            .collect::<Result<Vec<String>>>()?
+            .join("__");
+
         let should_panic = f
             .attrs
             .iter()
@@ -113,7 +164,7 @@ pub(crate) fn kunit_tests(test_suite: Ident, mut module: ItemMod) -> Result<Toke

         // Before the test, override usual `assert!` and `assert_eq!` macros with ones that call
         // KUnit instead.
-        let test_str = test.to_string();
+        let test_str = format!("{test}{cfg_attrs_str}");
         let path = CString::new(crate::helpers::file()).expect("file path cannot contain NUL");
         processed_items.push(parse_quote! {
             #[allow(unused)]
@@ -135,7 +186,7 @@ macro_rules! assert_eq {
         // Add back the test item.
         processed_items.push(Item::Fn(f));

-        let kunit_wrapper_fn_name = format_ident!("kunit_rust_wrapper_{test}");
+        let kunit_wrapper_fn_name = format_ident!("kunit_rust_wrapper_{test}{cfg_attrs_str}");
         let test_cstr = LitCStr::new(
             &CString::new(test_str.as_str()).expect("identifier cannot contain NUL"),
             test.span(),
--
2.47.3

[PATCH RFC 3/3] rust: bitmap: kunit: uncomment owned_bitmap_out_of_bounds panic case

[Nicolás Antinori]

Rust KUnit tests now support `#[should_panic]` attribute. Uncomment the
test so it can be run when CONFIG_RUST_BITMAP_HARDENED=y.

Signed-off-by: Nicolás Antinori <nico.antinori.7@gmail.com>
---
 rust/kernel/bitmap.rs | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
index b27e0ec80d64..a9935b2be8c5 100644
--- a/rust/kernel/bitmap.rs
+++ b/rust/kernel/bitmap.rs
@@ -571,33 +571,29 @@ fn bitmap_set_clear_find() -> Result<(), AllocError> {
         Ok(())
     }

+    #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
     #[test]
     fn owned_bitmap_out_of_bounds() -> Result<(), AllocError> {
-        // TODO: Kunit #[test]s do not support `cfg` yet,
-        // so we add it here in the body.
-        #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
-        {
-            let mut b = BitmapVec::new(128, GFP_KERNEL)?;
-            b.set_bit(2048);
-            b.set_bit_atomic(2048);
-            b.clear_bit(2048);
-            b.clear_bit_atomic(2048);
-            assert_eq!(None, b.next_bit(2048));
-            assert_eq!(None, b.next_zero_bit(2048));
-            assert_eq!(None, b.last_bit());
-        }
+        let mut b = BitmapVec::new(128, GFP_KERNEL)?;
+        b.set_bit(2048);
+        b.set_bit_atomic(2048);
+        b.clear_bit(2048);
+        b.clear_bit_atomic(2048);
+        assert_eq!(None, b.next_bit(2048));
+        assert_eq!(None, b.next_zero_bit(2048));
+        assert_eq!(None, b.last_bit());
         Ok(())
     }

-    // TODO: uncomment once kunit supports [should_panic] and `cfg`.
-    // #[cfg(CONFIG_RUST_BITMAP_HARDENED)]
-    // #[test]
-    // #[should_panic]
-    // fn owned_bitmap_out_of_bounds() -> Result<(), AllocError> {
-    //     let mut b = BitmapVec::new(128, GFP_KERNEL)?;
-    //
-    //     b.set_bit(2048);
-    // }
+    #[cfg(CONFIG_RUST_BITMAP_HARDENED)]
+    #[test]
+    #[should_panic]
+    fn owned_bitmap_out_of_bounds() -> Result<(), AllocError> {
+        let mut b = BitmapVec::new(128, GFP_KERNEL)?;
+
+        b.set_bit(2048);
+        Ok(())
+    }

     #[test]
     fn bitmap_copy_and_extend() -> Result<(), AllocError> {
--
2.47.3