[PATCH v5 0/3] rust: kunit: Support KUnit tests with a user-space like syntax

[PATCH v5 0/3] rust: kunit: Support KUnit tests with a user-space like syntax

[David Gow]

Hi all,

v5 here is a small set of fixes and a rebase of the previous versions.
If there are no major issues, I'd like to land this soon so it can be
used and tested ready for 6.14.

This series was originally written by José Expósito, and has been
modified and updated by Matt Gilbride and myself. The original version
can be found here:
https://github.com/Rust-for-Linux/linux/pull/950

Add support for writing KUnit tests in Rust. While Rust doctests are
already converted to KUnit tests and run, they're really better suited
for examples, rather than as first-class unit tests.

This series implements a series of direct Rust bindings for KUnit tests,
as well as a new macro which allows KUnit tests to be written using a
close variant of normal Rust unit test syntax. The only change required
is replacing '#[cfg(test)]' with '#[kunit_tests(kunit_test_suite_name)]'

An example test would look like:
	#[kunit_tests(rust_kernel_hid_driver)]
	mod tests {
	    use super::*;
	    use crate::{c_str, driver, hid, prelude::*};
	    use core::ptr;

	    struct SimpleTestDriver;
	    impl Driver for SimpleTestDriver {
	        type Data = ();
	    }

	    #[test]
	    fn rust_test_hid_driver_adapter() {
	        let mut hid = bindings::hid_driver::default();
	        let name = c_str!("SimpleTestDriver");
	        static MODULE: ThisModule = unsafe { ThisModule::from_ptr(ptr::null_mut()) };

        	let res = unsafe {
	            <hid::Adapter<SimpleTestDriver> as driver::DriverOps>::register(&mut hid, name, &MODULE)
	        };
	        assert_eq!(res, Err(ENODEV)); // The mock returns -19
	    }
	}


Please give this a go, and make sure I haven't broken it! There's almost
certainly a lot of improvements which can be made -- and there's a fair
case to be made for replacing some of this with generated C code which
can use the C macros -- but this is hopefully an adequate implementation
for now, and the interface can (with luck) remain the same even if the
implementation changes.

A few small notable missing features:
- Attributes (like the speed of a test) are hardcoded to the default
  value.
- Similarly, the module name attribute is hardcoded to NULL. In C, we
  use the KBUILD_MODNAME macro, but I couldn't find a way to use this
  from Rust which wasn't more ugly than just disabling it.
- Assertions are not automatically rewritten to use KUnit assertions.

---

Changes since v4:
https://lore.kernel.org/linux-kselftest/20241101064505.3820737-1-davidgow@google.com/
- Rebased against 6.13-rc1
- Allowed an unused_unsafe warning after the behaviour of addr_of_mut!()
  changed in Rust 1.82. (Thanks Boqun, Miguel)
- "Expect" that the sample assert_eq!(1+1, 2) produces a clippy warning
  due to a redundant assertion. (Thanks Boqun, Miguel)
- Fix some missing safety comments, and remove some unneeded 'unsafe'
  blocks. (Thanks Boqun)
- Fix a couple of minor rustfmt issues which were triggering checkpatch
  warnings.

Changes since v3:
https://lore.kernel.org/linux-kselftest/20241030045719.3085147-2-davidgow@google.com/T/
- The kunit_unsafe_test_suite!() macro now panic!s if the suite name is
  too long, triggering a compile error. (Thanks, Alice!)
- The #[kunit_tests()] macro now preserves span information, so
  errors can be better reported. (Thanks, Boqun!)
- The example tests have been updated to no longer use assert_eq!() with
  a constant bool argument (which triggered a clippy warning now we
  have the span info).

Changes since v2:
https://lore.kernel.org/linux-kselftest/20241029092422.2884505-1-davidgow@google.com/T/
- Include missing rust/macros/kunit.rs file from v2. (Thanks Boqun!)
- The kunit_unsafe_test_suite!() macro will truncate the name of the
  suite if it is too long. (Thanks Alice!)
- The proc macro now emits an error if the suite name is too long.
- We no longer needlessly use UnsafeCell<> in
  kunit_unsafe_test_suite!(). (Thanks Alice!)

Changes since v1:
https://lore.kernel.org/lkml/20230720-rustbind-v1-0-c80db349e3b5@google.com/T/
- Rebase on top of the latest rust-next (commit 718c4069896c)
- Make kunit_case a const fn, rather than a macro (Thanks Boqun)
- As a result, the null terminator is now created with
  kernel::kunit::kunit_case_null()
- Use the C kunit_get_current_test() function to implement
  in_kunit_test(), rather than re-implementing it (less efficiently)
  ourselves.

Changes since the GitHub PR:
- Rebased on top of kselftest/kunit
- Add const_mut_refs feature
  This may conflict with https://lore.kernel.org/lkml/20230503090708.2524310-6-nmi@metaspace.dk/
- Add rust/macros/kunit.rs to the KUnit MAINTAINERS entry

---

José Expósito (3):
  rust: kunit: add KUnit case and suite macros
  rust: macros: add macro to easily run KUnit tests
  rust: kunit: allow to know if we are in a test

 MAINTAINERS          |   1 +
 rust/kernel/kunit.rs | 207 +++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs   |   1 +
 rust/macros/kunit.rs | 168 +++++++++++++++++++++++++++++++++++
 rust/macros/lib.rs   |  29 ++++++
 5 files changed, 406 insertions(+)
 create mode 100644 rust/macros/kunit.rs

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v5 1/3] rust: kunit: add KUnit case and suite macros

[David Gow]

From: José Expósito <jose.exposito89@gmail.com>

Add a couple of Rust const functions and macros to allow to develop
KUnit tests without relying on generated C code:

 - The `kunit_unsafe_test_suite!` Rust macro is similar to the
   `kunit_test_suite` C macro. It requires a NULL-terminated array of
   test cases (see below).
 - The `kunit_case` Rust function is similar to the `KUNIT_CASE` C macro.
   It generates as case from the name and function.
 - The `kunit_case_null` Rust function generates a NULL test case, which
   is to be used as delimiter in `kunit_test_suite!`.

While these functions and macros can be used on their own, a future
patch will introduce another macro to create KUnit tests using a
user-space like syntax.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
Co-developed-by: Matt Gilbride <mattgilbride@google.com>
Signed-off-by: Matt Gilbride <mattgilbride@google.com>
Co-developed-by: David Gow <davidgow@google.com>
Signed-off-by: David Gow <davidgow@google.com>
---

Changes since v4:
https://lore.kernel.org/linux-kselftest/20241101064505.3820737-2-davidgow@google.com/
- Rebased against 6.13-rc1
- Allowed an unused_unsafe warning after the behaviour of addr_of_mut!()
  changed in Rust 1.82. (Thanks Boqun, Miguel)
- Fix a couple of minor rustfmt issues which were triggering checkpatch
  warnings.

Changes since v3:
https://lore.kernel.org/linux-kselftest/20241030045719.3085147-4-davidgow@google.com/
- The kunit_unsafe_test_suite!() macro now panic!s if the suite name is
  too long, triggering a compile error. (Thanks, Alice!)

Changes since v2:
https://lore.kernel.org/linux-kselftest/20241029092422.2884505-2-davidgow@google.com/
- The kunit_unsafe_test_suite!() macro will truncate the name of the
  suite if it is too long. (Thanks Alice!)
- We no longer needlessly use UnsafeCell<> in
  kunit_unsafe_test_suite!(). (Thanks Alice!)

Changes since v1:
https://lore.kernel.org/lkml/20230720-rustbind-v1-1-c80db349e3b5@google.com/
- Rebase on top of rust-next
- As a result, KUnit attributes are new set. These are hardcoded to the
  defaults of "normal" speed and no module name.
- Split the kunit_case!() macro into two const functions, kunit_case()
  and kunit_case_null() (for the NULL terminator).

---
 rust/kernel/kunit.rs | 118 +++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs   |   1 +
 2 files changed, 119 insertions(+)

diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index 824da0e9738a..5003282cb4c4 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -161,3 +161,121 @@ macro_rules! kunit_assert_eq {
         $crate::kunit_assert!($name, $file, $diff, $left == $right);
     }};
 }
+
+/// Represents an individual test case.
+///
+/// The test case should have the signature
+/// `unsafe extern "C" fn test_case(test: *mut crate::bindings::kunit)`.
+///
+/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases.
+/// Use `kunit_case_null` to generate such a delimeter.
+const fn kunit_case(
+    name: &'static kernel::str::CStr,
+    run_case: unsafe extern "C" fn(*mut kernel::bindings::kunit),
+) -> kernel::bindings::kunit_case {
+    kernel::bindings::kunit_case {
+        run_case: Some(run_case),
+        name: name.as_char_ptr(),
+        attr: kernel::bindings::kunit_attributes {
+            speed: kernel::bindings::kunit_speed_KUNIT_SPEED_NORMAL,
+        },
+        generate_params: None,
+        status: kernel::bindings::kunit_status_KUNIT_SUCCESS,
+        module_name: core::ptr::null_mut(),
+        log: core::ptr::null_mut(),
+    }
+}
+
+/// Represents the NULL test case delimiter.
+///
+/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. This
+/// function retuns such a delimiter.
+const fn kunit_case_null() -> kernel::bindings::kunit_case {
+    kernel::bindings::kunit_case {
+        run_case: None,
+        name: core::ptr::null_mut(),
+        generate_params: None,
+        attr: kernel::bindings::kunit_attributes {
+            speed: kernel::bindings::kunit_speed_KUNIT_SPEED_NORMAL,
+        },
+        status: kernel::bindings::kunit_status_KUNIT_SUCCESS,
+        module_name: core::ptr::null_mut(),
+        log: core::ptr::null_mut(),
+    }
+}
+
+/// Registers a KUnit test suite.
+///
+/// # Safety
+///
+/// `test_cases` must be a NULL terminated array of test cases.
+///
+/// # Examples
+///
+/// ```ignore
+/// unsafe extern "C" fn test_fn(_test: *mut crate::bindings::kunit) {
+///     let actual = 1 + 1;
+///     let expected = 2;
+///     assert_eq!(actual, expected);
+/// }
+///
+/// static mut KUNIT_TEST_CASE: crate::bindings::kunit_case = crate::kunit_case(name, test_fn);
+/// static mut KUNIT_NULL_CASE: crate::bindings::kunit_case = crate::kunit_case_null();
+/// static mut KUNIT_TEST_CASES: &mut[crate::bindings::kunit_case] = unsafe {
+///     &mut[KUNIT_TEST_CASE, KUNIT_NULL_CASE]
+/// };
+/// crate::kunit_unsafe_test_suite!(suite_name, KUNIT_TEST_CASES);
+/// ```
+#[macro_export]
+macro_rules! kunit_unsafe_test_suite {
+    ($name:ident, $test_cases:ident) => {
+        const _: () = {
+            static KUNIT_TEST_SUITE_NAME: [i8; 256] = {
+                let name_u8 = core::stringify!($name).as_bytes();
+                let mut ret = [0; 256];
+
+                if name_u8.len() > 255 {
+                    panic!(concat!(
+                        "The test suite name `",
+                        core::stringify!($name),
+                        "` exceeds the maximum length of 255 bytes."
+                    ));
+                }
+
+                let mut i = 0;
+                while i < name_u8.len() {
+                    ret[i] = name_u8[i] as i8;
+                    i += 1;
+                }
+
+                ret
+            };
+
+            static mut KUNIT_TEST_SUITE: $crate::bindings::kunit_suite =
+                $crate::bindings::kunit_suite {
+                    name: KUNIT_TEST_SUITE_NAME,
+                    // SAFETY: User is expected to pass a correct `test_cases`, hence this macro
+                    // named 'unsafe'.
+                    test_cases: unsafe { $test_cases.as_mut_ptr() },
+                    suite_init: None,
+                    suite_exit: None,
+                    init: None,
+                    exit: None,
+                    attr: $crate::bindings::kunit_attributes {
+                        speed: $crate::bindings::kunit_speed_KUNIT_SPEED_NORMAL,
+                    },
+                    status_comment: [0; 256usize],
+                    debugfs: core::ptr::null_mut(),
+                    log: core::ptr::null_mut(),
+                    suite_init_err: 0,
+                    is_init: false,
+                };
+
+            #[used]
+            #[link_section = ".kunit_test_suites"]
+            static mut KUNIT_TEST_SUITE_ENTRY: *const $crate::bindings::kunit_suite =
+                // SAFETY: `KUNIT_TEST_SUITE` is static.
+                unsafe { core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) };
+        };
+    };
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e1065a7551a3..02a70c5ad8ee 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -18,6 +18,7 @@
 #![feature(inline_const)]
 #![feature(lint_reasons)]
 #![feature(unsize)]
+#![feature(const_mut_refs)]
 
 // Ensure conditional compilation based on the kernel configuration works;
 // otherwise we may silently break things like initcall handling.
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v5 2/3] rust: macros: add macro to easily run KUnit tests

[David Gow]

From: José Expósito <jose.exposito89@gmail.com>

Add a new procedural macro (`#[kunit_tests(kunit_test_suit_name)]`) to
run KUnit tests using a user-space like syntax.

The macro, that should be used on modules, transforms every `#[test]`
in a `kunit_case!` and adds a `kunit_unsafe_test_suite!` registering
all of them.

The only difference with user-space tests is that instead of using
`#[cfg(test)]`, `#[kunit_tests(kunit_test_suit_name)]` is used.

Note that `#[cfg(CONFIG_KUNIT)]` is added so the test module is not
compiled when `CONFIG_KUNIT` is set to `n`.

Reviewed-by: David Gow <davidgow@google.com>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: David Gow <davidgow@google.com>
---
Changes since v4:
https://lore.kernel.org/linux-kselftest/20241101064505.3820737-3-davidgow@google.com/
- Rebased against 6.13-rc1
- "Expect" that the sample assert_eq!(1+1, 2) produces a clippy warning
  due to a redundant assertion. (Thanks Boqun, Miguel)

Changes since v3:
https://lore.kernel.org/linux-kselftest/20241030045719.3085147-6-davidgow@google.com/
- The #[kunit_tests()] macro now preserves span information, so
  errors can be better reported. (Thanks, Boqun!)
- The example test has been replaced to no longer use assert_eq!() with
  a constant bool argument (which triggered a clippy warning now we
  have the span info). It now checks 1 + 1 == 2, to match the C example.
  - (The in_kunit_test() test in the next patch uses assert!() to check
    a bool, so having something different here seemed to make a better
    example.)

Changes since v2:
https://lore.kernel.org/linux-kselftest/20241029092422.2884505-3-davidgow@google.com/
- Include missing rust/macros/kunit.rs file from v2. (Thanks Boqun!)
- The proc macro now emits an error if the suite name is too long.

Changes since v1:
https://lore.kernel.org/lkml/20230720-rustbind-v1-2-c80db349e3b5@google.com/
- Rebased on top of rust-next
- Make use of the new const functions, rather than the kunit_case!()
  macro.

---
 MAINTAINERS          |   1 +
 rust/kernel/kunit.rs |  16 ++++-
 rust/macros/kunit.rs | 168 +++++++++++++++++++++++++++++++++++++++++++
 rust/macros/lib.rs   |  29 ++++++++
 4 files changed, 213 insertions(+), 1 deletion(-)
 create mode 100644 rust/macros/kunit.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index e6e71b05710b..cbe5a99eefea 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12562,6 +12562,7 @@ F:	Documentation/dev-tools/kunit/
 F:	include/kunit/
 F:	lib/kunit/
 F:	rust/kernel/kunit.rs
+F:	rust/macros/kunit.rs
 F:	scripts/rustdoc_test_*
 F:	tools/testing/kunit/
 
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index 5003282cb4c4..a92f12da77d5 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -40,6 +40,8 @@ pub fn info(args: fmt::Arguments<'_>) {
     }
 }
 
+use macros::kunit_tests;
+
 /// Asserts that a boolean expression is `true` at runtime.
 ///
 /// Public but hidden since it should only be used from generated tests.
@@ -272,10 +274,22 @@ macro_rules! kunit_unsafe_test_suite {
                 };
 
             #[used]
+            #[allow(unused_unsafe)]
             #[link_section = ".kunit_test_suites"]
             static mut KUNIT_TEST_SUITE_ENTRY: *const $crate::bindings::kunit_suite =
                 // SAFETY: `KUNIT_TEST_SUITE` is static.
-                unsafe { core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) };
+                unsafe {
+                    core::ptr::addr_of_mut!(KUNIT_TEST_SUITE)
+                };
         };
     };
 }
+
+#[kunit_tests(rust_kernel_kunit)]
+mod tests {
+    #[test]
+    fn rust_test_kunit_example_test() {
+        #![expect(clippy::eq_op)]
+        assert_eq!(1 + 1, 2);
+    }
+}
diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs
new file mode 100644
index 000000000000..c421ff65f7f9
--- /dev/null
+++ b/rust/macros/kunit.rs
@@ -0,0 +1,168 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Procedural macro to run KUnit tests using a user-space like syntax.
+//!
+//! Copyright (c) 2023 José Expósito <jose.exposito89@gmail.com>
+
+use proc_macro::{Delimiter, Group, TokenStream, TokenTree};
+use std::fmt::Write;
+
+pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream {
+    if attr.to_string().is_empty() {
+        panic!("Missing test name in #[kunit_tests(test_name)] macro")
+    }
+
+    if attr.to_string().as_bytes().len() > 255 {
+        panic!("The test suite name `{}` exceeds the maximum length of 255 bytes.", attr)
+    }
+
+    let mut tokens: Vec<_> = ts.into_iter().collect();
+
+    // Scan for the "mod" keyword.
+    tokens
+        .iter()
+        .find_map(|token| match token {
+            TokenTree::Ident(ident) => match ident.to_string().as_str() {
+                "mod" => Some(true),
+                _ => None,
+            },
+            _ => None,
+        })
+        .expect("#[kunit_tests(test_name)] attribute should only be applied to modules");
+
+    // Retrieve the main body. The main body should be the last token tree.
+    let body = match tokens.pop() {
+        Some(TokenTree::Group(group)) if group.delimiter() == Delimiter::Brace => group,
+        _ => panic!("cannot locate main body of module"),
+    };
+
+    // Get the functions set as tests. Search for `[test]` -> `fn`.
+    let mut body_it = body.stream().into_iter();
+    let mut tests = Vec::new();
+    while let Some(token) = body_it.next() {
+        match token {
+            TokenTree::Group(ident) if ident.to_string() == "[test]" => match body_it.next() {
+                Some(TokenTree::Ident(ident)) if ident.to_string() == "fn" => {
+                    let test_name = match body_it.next() {
+                        Some(TokenTree::Ident(ident)) => ident.to_string(),
+                        _ => continue,
+                    };
+                    tests.push(test_name);
+                }
+                _ => continue,
+            },
+            _ => (),
+        }
+    }
+
+    // Add `#[cfg(CONFIG_KUNIT)]` before the module declaration.
+    let config_kunit = "#[cfg(CONFIG_KUNIT)]".to_owned().parse().unwrap();
+    tokens.insert(
+        0,
+        TokenTree::Group(Group::new(Delimiter::None, config_kunit)),
+    );
+
+    // Generate the test KUnit test suite and a test case for each `#[test]`.
+    // The code generated for the following test module:
+    //
+    // ```
+    // #[kunit_tests(kunit_test_suit_name)]
+    // mod tests {
+    //     #[test]
+    //     fn foo() {
+    //         assert_eq!(1, 1);
+    //     }
+    //
+    //     #[test]
+    //     fn bar() {
+    //         assert_eq!(2, 2);
+    //     }
+    // ```
+    //
+    // Looks like:
+    //
+    // ```
+    // unsafe extern "C" fn kunit_rust_wrapper_foo(_test: *mut kernel::bindings::kunit) {
+    //     foo();
+    // }
+    // static mut KUNIT_CASE_FOO: kernel::bindings::kunit_case =
+    //     kernel::kunit::kunit_case(foo, kunit_rust_wrapper_foo);
+    //
+    // unsafe extern "C" fn kunit_rust_wrapper_bar(_test: * mut kernel::bindings::kunit) {
+    //     bar();
+    // }
+    // static mut KUNIT_CASE_BAR: kernel::bindings::kunit_case =
+    //     kernel::kunit::kunit_case(bar, kunit_rust_wrapper_bar);
+    //
+    // static mut KUNIT_CASE_NULL: kernel::bindings::kunit_case = kernel::kunit::kunit_case_null();
+    //
+    // static mut TEST_CASES : &mut[kernel::bindings::kunit_case] = unsafe {
+    //     &mut [KUNIT_CASE_FOO, KUNIT_CASE_BAR, KUNIT_CASE_NULL]
+    // };
+    //
+    // kernel::kunit_unsafe_test_suite!(kunit_test_suit_name, TEST_CASES);
+    // ```
+    let mut kunit_macros = "".to_owned();
+    let mut test_cases = "".to_owned();
+    for test in tests {
+        let kunit_wrapper_fn_name = format!("kunit_rust_wrapper_{}", test);
+        let kunit_case_name = format!("KUNIT_CASE_{}", test.to_uppercase());
+        let kunit_wrapper = format!(
+            "unsafe extern \"C\" fn {}(_test: *mut kernel::bindings::kunit) {{ {}(); }}",
+            kunit_wrapper_fn_name, test
+        );
+        let kunit_case = format!(
+            "static mut {}: kernel::bindings::kunit_case = kernel::kunit::kunit_case(kernel::c_str!(\"{}\"), {});",
+            kunit_case_name, test, kunit_wrapper_fn_name
+        );
+        writeln!(kunit_macros, "{kunit_wrapper}").unwrap();
+        writeln!(kunit_macros, "{kunit_case}").unwrap();
+        writeln!(test_cases, "{kunit_case_name},").unwrap();
+    }
+
+    writeln!(
+        kunit_macros,
+        "static mut KUNIT_CASE_NULL: kernel::bindings::kunit_case = kernel::kunit::kunit_case_null();"
+    )
+    .unwrap();
+
+    writeln!(
+        kunit_macros,
+        "static mut TEST_CASES : &mut[kernel::bindings::kunit_case] = unsafe {{ &mut[{test_cases} KUNIT_CASE_NULL] }};"
+    )
+    .unwrap();
+
+    writeln!(
+        kunit_macros,
+        "kernel::kunit_unsafe_test_suite!({attr}, TEST_CASES);"
+    )
+    .unwrap();
+
+    // Remove the `#[test]` macros.
+    // We do this at a token level, in order to preserve span information.
+    let mut new_body = vec![];
+    let mut body_it = body.stream().into_iter();
+
+    while let Some(token) = body_it.next() {
+        match token {
+            TokenTree::Punct(ref c) if c.as_char() == '#' => {
+                match body_it.next() {
+                    Some(TokenTree::Group(group)) if group.to_string() == "[test]" => (),
+                    Some(next) => { new_body.extend([token, next]); },
+                    _ => { new_body.push(token); },
+                }
+            },
+            _ => { new_body.push(token); },
+        }
+    }
+
+    let mut new_body = TokenStream::from_iter(new_body);
+    new_body.extend::<TokenStream>(kunit_macros.parse().unwrap());
+
+    tokens.push(TokenTree::Group(Group::new(
+        Delimiter::Brace,
+        new_body
+    )));
+
+    tokens.into_iter().collect()
+}
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index 4ab94e44adfe..beff19cc6d16 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -10,6 +10,7 @@
 mod quote;
 mod concat_idents;
 mod helpers;
+mod kunit;
 mod module;
 mod paste;
 mod pin_data;
@@ -492,3 +493,31 @@ pub fn paste(input: TokenStream) -> TokenStream {
 pub fn derive_zeroable(input: TokenStream) -> TokenStream {
     zeroable::derive(input)
 }
+
+/// Registers a KUnit test suite and its test cases using a user-space like syntax.
+///
+/// This macro should be used on modules. If `CONFIG_KUNIT` (in `.config`) is `n`, the target module
+/// is ignored.
+///
+/// # Examples
+///
+/// ```ignore
+/// # use macros::kunit_tests;
+///
+/// #[kunit_tests(kunit_test_suit_name)]
+/// mod tests {
+///     #[test]
+///     fn foo() {
+///         assert_eq!(1, 1);
+///     }
+///
+///     #[test]
+///     fn bar() {
+///         assert_eq!(2, 2);
+///     }
+/// }
+/// ```
+#[proc_macro_attribute]
+pub fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream {
+    kunit::kunit_tests(attr, ts)
+}
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v5 3/3] rust: kunit: allow to know if we are in a test

[David Gow]

From: José Expósito <jose.exposito89@gmail.com>

In some cases, we need to call test-only code from outside the test
case, for example, to mock a function or a module.

In order to check whether we are in a test or not, we need to test if
`CONFIG_KUNIT` is set.
Unfortunately, we cannot rely only on this condition because:
- a test could be running in another thread,
- some distros compile KUnit in production kernels, so checking at runtime
  that `current->kunit_test != NULL` is required.

Forturately, KUnit provides an optimised check in
`kunit_get_current_test()`, which checks CONFIG_KUNIT, a global static
key, and then the current thread's running KUnit test.

Add a safe wrapper function around this to know whether or not we are in
a KUnit test and examples showing how to mock a function and a module.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
Co-developed-by: David Gow <davidgow@google.com>
Signed-off-by: David Gow <davidgow@google.com>
---
Changes since v4:
https://lore.kernel.org/linux-kselftest/20241101064505.3820737-4-davidgow@google.com/
- Rebased against 6.13-rc1
- Fix some missing safety comments, and remove some unneeded 'unsafe'
  blocks. (Thanks Boqun)

Changes since v3:
https://lore.kernel.org/linux-kselftest/20241030045719.3085147-8-davidgow@google.com/
- The example test has been updated to no longer use assert_eq!() with
  a constant bool argument (fixes a clippy warning).

No changes since v2:
https://lore.kernel.org/linux-kselftest/20241029092422.2884505-4-davidgow@google.com/

Changes since v1:
https://lore.kernel.org/lkml/20230720-rustbind-v1-3-c80db349e3b5@google.com/
- Rebased on top of rust-next.
- Use the `kunit_get_current_test()` C function, which wasn't previously
  available, instead of rolling our own.
- (Thanks also to Boqun for suggesting a nicer way of implementing this,
  which I tried, but the `kunit_get_current_test()` version obsoleted.)

---
 rust/kernel/kunit.rs | 75 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index a92f12da77d5..2196e35e5d75 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -285,11 +285,86 @@ macro_rules! kunit_unsafe_test_suite {
     };
 }
 
+/// In some cases, you need to call test-only code from outside the test case, for example, to
+/// create a function mock. This function can be invoked to know whether we are currently running a
+/// KUnit test or not.
+///
+/// # Examples
+///
+/// This example shows how a function can be mocked to return a well-known value while testing:
+///
+/// ```
+/// # use kernel::kunit::in_kunit_test;
+/// #
+/// fn fn_mock_example(n: i32) -> i32 {
+///     if in_kunit_test() {
+///         100
+///     } else {
+///         n + 1
+///     }
+/// }
+///
+/// let mock_res = fn_mock_example(5);
+/// assert_eq!(mock_res, 100);
+/// ```
+///
+/// Sometimes, you don't control the code that needs to be mocked. This example shows how the
+/// `bindings` module can be mocked:
+///
+/// ```
+/// // Import our mock naming it as the real module.
+/// #[cfg(CONFIG_KUNIT)]
+/// use bindings_mock_example as bindings;
+///
+/// // This module mocks `bindings`.
+/// mod bindings_mock_example {
+///     use kernel::kunit::in_kunit_test;
+///     use kernel::bindings::u64_;
+///
+///     // Make the other binding functions available.
+///     pub(crate) use kernel::bindings::*;
+///
+///     /// Mock `ktime_get_boot_fast_ns` to return a well-known value when running a KUnit test.
+///     pub(crate) fn ktime_get_boot_fast_ns() -> u64_ {
+///         if in_kunit_test() {
+///             1234
+///         } else {
+///             // SAFETY: ktime_get_boot_fast_ns() is safe to call, and just returns a u64.
+///             // Additionally, this is never actually called in this example, as we're in a test
+///             // and it's mocked out.
+///             unsafe { kernel::bindings::ktime_get_boot_fast_ns() }
+///         }
+///     }
+/// }
+///
+/// // This is the function we want to test. Since `bindings` has been mocked, we can use its
+/// // functions seamlessly.
+/// fn get_boot_ns() -> u64 {
+///     bindings::ktime_get_boot_fast_ns()
+/// }
+///
+/// let time = get_boot_ns();
+/// assert_eq!(time, 1234);
+/// ```
+pub fn in_kunit_test() -> bool {
+    // SAFETY: kunit_get_current_test() is always safe to call from C (it has fallbacks for
+    // when KUnit is not enabled), and we're only comparing the result to NULL.
+    unsafe { !bindings::kunit_get_current_test().is_null() }
+}
+
 #[kunit_tests(rust_kernel_kunit)]
 mod tests {
+    use super::*;
+
     #[test]
     fn rust_test_kunit_example_test() {
         #![expect(clippy::eq_op)]
         assert_eq!(1 + 1, 2);
     }
+
+    #[test]
+    fn rust_test_kunit_in_kunit_test() {
+        let in_kunit = in_kunit_test();
+        assert!(in_kunit);
+    }
 }
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v5 1/3] rust: kunit: add KUnit case and suite macros

[Miguel Ojeda]

On Fri, Dec 13, 2024 at 9:10 AM David Gow <davidgow@google.com> wrote:
>
> +/// The test case should have the signature
> +/// `unsafe extern "C" fn test_case(test: *mut crate::bindings::kunit)`.

I guess this is here so that people can copy-paste it (i.e. users)?
However, given it is private (i.e. users are not expected to manually
use this function) and that it is already in the signature
(`run_case`), I wonder if we need to mention it this paragraph.

Moreover, does it need to be `unsafe fn`? Safe ones coerce into unsafe
ones (i.e. not in the parameter, but in the one that the user defines)

> +/// Use `kunit_case_null` to generate such a delimeter.

Typo: delimiter

> +/// function retuns such a delimiter.

Typo: returns

> +/// Registers a KUnit test suite.
> +///
> +/// # Safety
> +///
> +/// `test_cases` must be a NULL terminated array of test cases.

I guess "test cases" here also means "valid ones", i.e. without
invalid pointers and so on inside, right? Perhaps it is good to at
least mention "valid" clearly. Otherwise, one can read it as "any
possible instance of `kunit_case`", which would be unsound, no?

We could go finer-grained, and explain exactly what needs to be valid,
which would be good.

A third alternative would be to mention that these test cases must be
created using the two functions above (`kunit_case*()`), and make the
`kunit_case()` one require a suitable `run_case` function pointer
(i.e. making it `unsafe`). That way the requirements are a bit more
localized, even if it means making that one `unsafe fn`.

> +/// unsafe extern "C" fn test_fn(_test: *mut crate::bindings::kunit) {

Does this function need to be `unsafe`? (please see above for the
comment on the docs of `kunit_case`). If so, then we would need a `#
Safety` section if the example was built (see below).

> +/// static mut KUNIT_TEST_CASE: crate::bindings::kunit_case = crate::kunit_case(name, test_fn);

Shouldn't this `name` be a `CStr` for this example? (The example is
ignored, but it would be ideal to keep it up to date).

> +/// static mut KUNIT_TEST_CASES: &mut[crate::bindings::kunit_case] = unsafe {
> +///     &mut[KUNIT_TEST_CASE, KUNIT_NULL_CASE]
> +/// };

These should have a space after `&mut`. However, why do we need the
mutable reference here anyway? (please see discussion below and in the
next patch)

In addition, doesn't this end up with duplicated statics? i.e. one in
the array, and an independent one. So we can instead put them directly
into the array, which would avoid `unsafe` in the initializer too.
(This applies to the generation in the next patch, too).

Finally, should we make this documentation `#[doc(hidden)]` (but
keeping the docs)? i.e. it is not expected to be used by users
directly anyway (and currently the example wouldn't work, since e.g.
`kunit_case` is private).

Speaking of the example, we could fix it and make it non-`ignore`
(assuming we made `kunit_case*` public which we will probably
eventually need, in which case they should also be `#[doc(hidden)]`),
e.g.:

    /// ```
    /// extern "C" fn test_fn(_test: *mut kernel::bindings::kunit) {
    ///     let actual = 1 + 1;
    ///     let expected = 2;
    ///     assert_eq!(actual, expected);
    /// }
    ///
    /// static mut KUNIT_TEST_CASES: [kernel::bindings::kunit_case; 2] = [
    ///     kernel::kunit::kunit_case(kernel::c_str!("name"), test_fn),
    ///     kernel::kunit::kunit_case_null(),
    /// ];
    /// kernel::kunit_unsafe_test_suite!(suite_name, KUNIT_TEST_CASES);
    /// ```

(If so, we should then use explicit names, since otherwise the KUnit
output in the log is confusing.)

> +            static KUNIT_TEST_SUITE_NAME: [i8; 256] = {

This should probably be `::kernel::ffi::c_char` instead (and then the
cast below needs a change too).

And I think we can make this a `const` instead, since we just use it
to initialize the array in the `static mut`, no?

> +                let name_u8 = core::stringify!($name).as_bytes();

Since it is a macro, I would use absolute paths, e.g. `::core::...`.

> +            static mut KUNIT_TEST_SUITE: $crate::bindings::kunit_suite =
> +                $crate::bindings::kunit_suite {
> +                    name: KUNIT_TEST_SUITE_NAME,
> +                    // SAFETY: User is expected to pass a correct `test_cases`, hence this macro

Nit: usually we say "by the safety preconditions" or similar, instead
of "User is expected to pass...".

> +                    // named 'unsafe'.

`unsafe`. (Also, not an English speaker, but should this be "is named" instead?)

> +                    test_cases: unsafe { $test_cases.as_mut_ptr() },

This warns due to `static_mut_refs` starting Rust 1.83:

    error: creating a mutable reference to mutable static is discouraged
    --> rust/kernel/kunit.rs:261:42
        |
    261 |                     test_cases: unsafe { $test_cases.as_mut_ptr() },
        |                                          ^^^^^^^^^^^ mutable
reference to mutable static

    https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-references.html

It can still be `allow`ed; however, doesn't the call to `as_mut_ptr()`
create an intermediate mutable reference that we should avoid anyway?

Instead, can we have an array static directly, rather than a mutable
reference static to an array, and use `addr_of_mut!` here? Then we
also avoid adding the `const_mut_refs` feature (even if it is stable
now).

That is, we would have (with all the feedback in place) something like:

    static mut TEST_CASES: [kernel::bindings::kunit_case; 3] = [
        kernel::kunit::kunit_case(kernel::c_str!("foo"),
kunit_rust_wrapper_foo),
        kernel::kunit::kunit_case(kernel::c_str!("bar"),
kunit_rust_wrapper_bar),
        kernel::kunit::kunit_case_null(),
    ];

And then:

    test_cases: unsafe {
        ::core::ptr::addr_of_mut!($test_cases).cast::<::kernel::bindings::kunit_case>()
    },

So no mutable references in sight.

>  #![feature(unsize)]
> +#![feature(const_mut_refs)]

The list should be kept sorted.

However, we can avoid enabling the feature I think, if we avoid the
`&mut`s (please see above).

Cheers,
Miguel

Re: [PATCH v5 2/3] rust: macros: add macro to easily run KUnit tests

[Miguel Ojeda]

On Fri, Dec 13, 2024 at 9:10 AM David Gow <davidgow@google.com> wrote:
>
> +            #[allow(unused_unsafe)]

Should this be in the first patch?

> -                unsafe { core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) };
> +                unsafe {
> +                    core::ptr::addr_of_mut!(KUNIT_TEST_SUITE)
> +                };

Spurious change?

I thought this should perhaps have been in the previous patch
directly, but running `rustfmt` shows the previous patch is the
correct formatting.

`rustfmt` also needs to be run for these files -- it reveals a few
more changes needed.

> +//! Copyright (c) 2023 José Expósito <jose.exposito89@gmail.com>

(This is not something we need to change now, since it is orthogonal
and debatable, but I think copyright lines should not be in the
generated documentation unless we really want contact points in docs.
I think it could go in a `//` comment after the `SPDX` line instead.)

> +pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream {
> +    if attr.to_string().is_empty() {
> +        panic!("Missing test name in #[kunit_tests(test_name)] macro")
> +    }
> +
> +    if attr.to_string().as_bytes().len() > 255 {
> +        panic!("The test suite name `{}` exceeds the maximum length of 255 bytes.", attr)
> +    }

We could do the `to_string()` step once creating a single string (and
we can keep it as a `String`, too):

    let attr = attr.to_string();

> +    // Scan for the "mod" keyword.

Formatting: `mod`

> +        .expect("#[kunit_tests(test_name)] attribute should only be applied to modules");

Formatting: `#[kunit_tests(test_name)]`

> +        _ => panic!("cannot locate main body of module"),

Formatting: Cannot

> +    //     #[test]
> +    //     fn bar() {
> +    //         assert_eq!(2, 2);
> +    //     }
> +    // ```

Missing closing `}` of the `mod`.

> +    // static mut KUNIT_CASE_FOO: kernel::bindings::kunit_case =
> +    //     kernel::kunit::kunit_case(foo, kunit_rust_wrapper_foo);

I think this is not in sync with the generation, i.e. missing
kernel::c_str!("foo")

(and ditto for the other one)

> +    // static mut TEST_CASES : &mut[kernel::bindings::kunit_case] = unsafe {

Formatting: extra space and missing space.

> +    //     &mut [KUNIT_CASE_FOO, KUNIT_CASE_BAR, KUNIT_CASE_NULL]
> +    // };
> +    //
> +    // kernel::kunit_unsafe_test_suite!(kunit_test_suit_name, TEST_CASES);
> +    // ```

With the suggestions from the previous patch (i.e. avoiding
unused/duplicated `static`s, intermediate mutable references, unstable
feature and `unsafe`), we can simplify the entire example down to:

    // ```
    // unsafe extern "C" fn kunit_rust_wrapper_foo(_test: *mut
kernel::bindings::kunit) { foo(); }
    // unsafe extern "C" fn kunit_rust_wrapper_bar(_test: * mut
kernel::bindings::kunit) { bar(); }
    //
    // static mut TEST_CASES: [kernel::bindings::kunit_case; 3] = [
    //     kernel::kunit::kunit_case(kernel::c_str!("foo"),
kunit_rust_wrapper_foo),
    //     kernel::kunit::kunit_case(kernel::c_str!("bar"),
kunit_rust_wrapper_bar),
    //     kernel::kunit::kunit_case_null()
    // ];
    //
    // kernel::kunit_unsafe_test_suite!(kunit_test_suit_name, TEST_CASES);
    // ```

...with the corresponding removals in the actual generation code
below, of course.

Cheers,
Miguel

Re: [PATCH v5 3/3] rust: kunit: allow to know if we are in a test

[Miguel Ojeda]

On Fri, Dec 13, 2024 at 9:10 AM David Gow <davidgow@google.com> wrote:
>
> +/// In some cases, you need to call test-only code from outside the test case, for example, to
> +/// create a function mock. This function can be invoked to know whether we are currently running a
> +/// KUnit test or not.

The documentation of items use the first paragraph as a "short
description" in some places, so ideally it should be as short as
possible (e.g. one line), similar to Git commit titles.

So what about:

    /// Returns whether we are currently running a KUnit test.
    ///
    /// In some cases, you need to call test-only code from outside
the test case, for example, to
    /// create a function mock. This function allows to change
behavior depending on whether we are
    /// currently running a KUnit test or not.

I tweaked the second sentence to avoid repetition, and to take the
chance to mention "allows to change behavior" instead, since that is
what we want to achieve.

> +/// #

Nit: currently the style we use doesn't keep empty `#` lines to separate.

> +/// fn fn_mock_example(n: i32) -> i32 {
> +///     if in_kunit_test() {
> +///         100
> +///     } else {
> +///         n + 1
> +///     }
> +/// }

Early return would look better here since we really want to do
something completely different, and would avoid indentation in the
"normal code".

> +/// // This module mocks `bindings`.

This could perhaps be documentation (`///`), but either way it is fine.

> +/// mod bindings_mock_example {

Could this get a `#[cfg(CONFIG_KUNIT)]` too?

> +///         if in_kunit_test() {
> +///             1234
> +///         } else {
> +///             // SAFETY: ktime_get_boot_fast_ns() is safe to call, and just returns a u64.

Formatting: `ktime_get_boot_fast_ns()` and `u64`

Perhaps emphasize with "always safe"?

Also, why does the `u64` need to be part of the safety comment?

> +///             // Additionally, this is never actually called in this example, as we're in a test
> +///             // and it's mocked out.

If the function is safe to call, should we have this as part of the
`SAFETY` comment then? We can move it above, if we need to keep it, or
we could just remove it.

In any case, if the `else` is dead code, why do we have it? i.e.
shouldn't the mock just return the 1234 value? (see below)

> +/// // This is the function we want to test. Since `bindings` has been mocked, we can use its
> +/// // functions seamlessly.
> +/// fn get_boot_ns() -> u64 {
> +///     bindings::ktime_get_boot_fast_ns()

I think this wouldn't work: `ktime_get_boot_fast_ns()` is unsafe when
`CONFIG_KUNIT` is disabled, so it wouldn't build for an actual user.

Unless I am missing something, mocks should keep the `unsafe` status
(i.e. in general, the signature should be kept the same), and the
`SAFETY` comment should be here, i.e. in the "normal code", not above
in the mock (we should probably mention this as a guideline in
`Documentation/rust/testing.rst` too, when the docs are added there
for this).

And by doing that, we can remove all the usage of `bindings` inside
the mocking module, and we keep the "normal code" looking as normal as
possible, i.e. we don't hide `// SAFETY` comments inside mocking
modules.

With all put together, we get something like this:

    /// ```
    /// // Import our mock naming it as the real module.
    /// #[cfg(CONFIG_KUNIT)]
    /// use bindings_mock_example as bindings;
    /// #[cfg(not(CONFIG_KUNIT))]
    /// use kernel::bindings;
    ///
    /// // This module mocks `bindings`.
    /// #[cfg(CONFIG_KUNIT)]
    /// mod bindings_mock_example {
    ///     /// Mock `ktime_get_boot_fast_ns` to return a well-known
value when running a KUnit test.
    ///     pub(crate) fn ktime_get_boot_fast_ns() -> u64 {
    ///         1234
    ///     }
    /// }
    ///
    /// // This is the function we want to test. Since `bindings` has
been mocked, we can use its
    /// // functions seamlessly.
    /// fn get_boot_ns() -> u64 {
    ///     // SAFETY: `ktime_get_boot_fast_ns()` is always safe to call.
    ///     unsafe { bindings::ktime_get_boot_fast_ns() }
    /// }
    ///
    /// let time = get_boot_ns();
    /// assert_eq!(time, 1234);
    /// ```

I added a `#[cfg(CONFIG_KUNIT)]` for the mocking module here, like for
the other example.

> +pub fn in_kunit_test() -> bool {
> +    // SAFETY: kunit_get_current_test() is always safe to call from C (it has fallbacks for

Formatting: `kunit_get_current_test()`

Also, I think we should remove "from C" since it may be confusing --
or what is it trying to say here? i.e. it is always safe to call from
both C and Rust, no? Or is there something I am missing?

> +    // when KUnit is not enabled), and we're only comparing the result to NULL.

Does "and we're only comparing the result to NULL" need to be part of
the safety comment? i.e. comparing a pointer is safe (and `is_null()`
too).

> +        let in_kunit = in_kunit_test();
> +        assert!(in_kunit);

I would simplify and call directly the function.

Cheers,
Miguel

Re: [PATCH v5 0/3] rust: kunit: Support KUnit tests with a user-space like syntax

[Miguel Ojeda]

On Fri, Dec 13, 2024 at 9:10 AM David Gow <davidgow@google.com> wrote:
>
> v5 here is a small set of fixes and a rebase of the previous versions.
> If there are no major issues, I'd like to land this soon so it can be
> used and tested ready for 6.14.

Thanks as usual David for keeping this one alive over time.

I was thinking of applying this or giving you an Acked-by so that you
take it, but I ended up noticing a few things that I think need fixing
(the recommended mocking wouldn't work, warns/errors with 1.83,
duplicated/unused `static`s, intermediate mutable references being
created), so I sent a review. The good news is that we can simplify
the code (especially the generated one) while fixing those at the same
time. I hope the review helps.

I have put the review result here too (in a single commit, since I was
testing and didn't expect to fixup, sorry), in case it helps to see
the final result:

    https://github.com/ojeda/linux/commit/151681df53ac8ad52086f6758b51c6fb4414427b

If you agree with the changes (at least the big ones, i.e. that I
didn't miss something), then I think we can take it through KUnit or
Rust, though it may be a good idea to have the result in the list for
a few days (and/or put it early next cycle) given the magnitude of the
changes.

Finally, it is not a blocker for 6.14 or otherwise, but we should
eventually add/explain the new features in
`Documentation/rust/testing.rst`.

Thanks again!

Cheers,
Miguel