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

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

[David Gow]

Hi all,

After much delay, v6 of the KUnit/Rust integration patchset is here.
This change incorporates most of Miguels suggestions from v5 (save for
some of the copyright headers I wasn't comfortable unilaterally
changing). This means the documentation is much improved, and it should
work more cleanly on Rust 1.83 and 1.84, no longer requiring
static_mut_refs or const_mut_refs. (I'm not 100% sure I understand all
of the details of this, but I'm comfortable enough with how it's ended
up.)

This has been rebased against 6.14-rc1/rust-next, and should be able to
comfortably go in via either the KUnit or Rust trees. My suspicion is
that there's more likely to be conflicts with the Rust work (due to the
changes in rust/macros/lib.rs) than with KUnit, where there are no
current patches which would break the API, so maybe it makes the most
sense for it to go in via Rust for 6.15.

This series was originally written by José Expósito, and has been
modified and updated by Matt Gilbride, Miguel Ojeda, 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 v5:
https://lore.kernel.org/all/20241213081035.2069066-1-davidgow@google.com/
- Rebased against 6.14-rc1
- Fixed a bunch of warnings / clippy lints introduced in Rust 1.83 and
  1.84.
- No longer needs static_mut_refs / const_mut_refs, and is much cleaned
  up as a result. (Thanks, Miguel)
- Major documentation and example fixes. (Thanks, Miguel)

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 | 199 +++++++++++++++++++++++++++++++++++++++++++
 rust/macros/kunit.rs | 161 ++++++++++++++++++++++++++++++++++
 rust/macros/lib.rs   |  29 +++++++
 4 files changed, 390 insertions(+)
 create mode 100644 rust/macros/kunit.rs

-- 
2.48.1.601.g30ceb7b040-goog

[PATCH v6 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: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Co-developed-by: David Gow <davidgow@google.com>
Signed-off-by: David Gow <davidgow@google.com>
---

Changes since v5:
https://lore.kernel.org/all/20241213081035.2069066-2-davidgow@google.com/
- Rebased against 6.14-rc1
- Several documentation touch-ups, including noting that the example
  test function need not be unsafe. (Thanks, Miguel)
- Remove the need for static_mut_refs, by using core::addr_of_mut!()
  combined with a cast. (Thanks, Miguel)
  - While this should also avoid the need for const_mut_refs, it seems
    to have been enabled for other users anyway.
- Use ::kernel::ffi::c_char for C strings, rather than i8 directly.
  (Thanks, Miguel)

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 | 121 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index 824da0e9738a..d34a92075174 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -161,3 +161,124 @@ macro_rules! kunit_assert_eq {
         $crate::kunit_assert!($name, $file, $diff, $left == $right);
     }};
 }
+
+/// Represents an individual test case.
+///
+/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of valid test cases.
+/// Use `kunit_case_null` to generate such a delimiter.
+#[doc(hidden)]
+pub 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 returns such a delimiter.
+#[doc(hidden)]
+pub 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 valid test cases.
+///
+/// # Examples
+///
+/// ```ignore
+/// 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);
+/// ```
+#[doc(hidden)]
+#[macro_export]
+macro_rules! kunit_unsafe_test_suite {
+    ($name:ident, $test_cases:ident) => {
+        const _: () = {
+            const KUNIT_TEST_SUITE_NAME: [::kernel::ffi::c_char; 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 ::kernel::ffi::c_char;
+                    i += 1;
+                }
+
+                ret
+            };
+
+            #[allow(unused_unsafe)]
+            static mut KUNIT_TEST_SUITE: ::kernel::bindings::kunit_suite =
+                ::kernel::bindings::kunit_suite {
+                    name: KUNIT_TEST_SUITE_NAME,
+                    // SAFETY: User is expected to pass a correct `test_cases`, given the safety
+                    // precondition; hence this macro named `unsafe`.
+                    test_cases: unsafe {
+                        ::core::ptr::addr_of_mut!($test_cases)
+                            .cast::<::kernel::bindings::kunit_case>()
+                    },
+                    suite_init: None,
+                    suite_exit: None,
+                    init: None,
+                    exit: None,
+                    attr: ::kernel::bindings::kunit_attributes {
+                        speed: ::kernel::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 ::kernel::bindings::kunit_suite =
+                // SAFETY: `KUNIT_TEST_SUITE` is static.
+                unsafe { ::core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) };
+        };
+    };
+}
-- 
2.48.1.601.g30ceb7b040-goog

[PATCH v6 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>
Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: David Gow <davidgow@google.com>
---
Changes since v5:
https://lore.kernel.org/all/20241213081035.2069066-3-davidgow@google.com/
- Fix some rustfmt-related formatting shenanigans. (Thanks, Miguel)
- Fix some documentation comment formatting as well. (Thanks, Miguel)
- Tidy up the generated code to avoid unneeded &mut[] everywhere.
  (Thanks, Miguel)
- Fix a new clippy warning for using .as_bytes().len() instead of .len()
  directly.

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 |  12 ++++
 rust/macros/kunit.rs | 161 +++++++++++++++++++++++++++++++++++++++++++
 rust/macros/lib.rs   |  29 ++++++++
 4 files changed, 203 insertions(+)
 create mode 100644 rust/macros/kunit.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index c8d9e8187eb0..4e7a6d2f2c49 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12677,6 +12677,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 d34a92075174..9e27b74a605b 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.
@@ -275,6 +277,7 @@ macro_rules! kunit_unsafe_test_suite {
                 };
 
             #[used]
+            #[allow(unused_unsafe)]
             #[link_section = ".kunit_test_suites"]
             static mut KUNIT_TEST_SUITE_ENTRY: *const ::kernel::bindings::kunit_suite =
                 // SAFETY: `KUNIT_TEST_SUITE` is static.
@@ -282,3 +285,12 @@ macro_rules! kunit_unsafe_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..4f553ecf40c0
--- /dev/null
+++ b/rust/macros/kunit.rs
@@ -0,0 +1,161 @@
+// 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 {
+    let attr = attr.to_string();
+
+    if attr.is_empty() {
+        panic!("Missing test name in `#[kunit_tests(test_name)]` macro")
+    }
+
+    if attr.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(); }
+    // 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);
+    // ```
+    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_wrapper = format!(
+            "unsafe extern \"C\" fn {}(_test: *mut kernel::bindings::kunit) {{ {}(); }}",
+            kunit_wrapper_fn_name, test
+        );
+        writeln!(kunit_macros, "{kunit_wrapper}").unwrap();
+        writeln!(
+            test_cases,
+            "    kernel::kunit::kunit_case(kernel::c_str!(\"{}\"), {}),",
+            test, kunit_wrapper_fn_name
+        )
+        .unwrap();
+    }
+
+    writeln!(kunit_macros).unwrap();
+    writeln!(
+        kunit_macros,
+        "static mut TEST_CASES: [kernel::bindings::kunit_case; {}] = [\n{test_cases}    kernel::kunit::kunit_case_null(),\n];",
+        tests.len() + 1
+    )
+    .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 d61bc6a56425..50b58259c577 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.48.1.601.g30ceb7b040-goog

[PATCH v6 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>
Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---

Changes since v5:
https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/
- Greatly improved documentation, which is both clearer and better
  matches the rustdoc norm. (Thanks, Miguel)
- The examples and safety comments are also both more idiomatic an
  cleaner. (Thanks, Miguel)
- More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)

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 | 66 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index 9e27b74a605b..3aad7a281b6d 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -286,11 +286,77 @@ macro_rules! kunit_unsafe_test_suite {
     };
 }
 
+/// 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.
+///
+/// # 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() {
+///         return 100;
+///     }
+///
+///     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;
+/// #[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);
+/// ```
+pub fn in_kunit_test() -> bool {
+    // SAFETY: `kunit_get_current_test()` is always safe to call (it has fallbacks for
+    // when KUnit is not enabled).
+    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() {
+        assert!(in_kunit_test());
+    }
 }
-- 
2.48.1.601.g30ceb7b040-goog

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

[Tamir Duberstein]

Very excited to see this progress.

On Fri, Feb 14, 2025 at 2:41 AM David Gow <davidgow@google.com> wrote:
>
> 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: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> Co-developed-by: David Gow <davidgow@google.com>
> Signed-off-by: David Gow <davidgow@google.com>
> ---
>
> Changes since v5:
> https://lore.kernel.org/all/20241213081035.2069066-2-davidgow@google.com/
> - Rebased against 6.14-rc1
> - Several documentation touch-ups, including noting that the example
>   test function need not be unsafe. (Thanks, Miguel)
> - Remove the need for static_mut_refs, by using core::addr_of_mut!()
>   combined with a cast. (Thanks, Miguel)
>   - While this should also avoid the need for const_mut_refs, it seems
>     to have been enabled for other users anyway.
> - Use ::kernel::ffi::c_char for C strings, rather than i8 directly.
>   (Thanks, Miguel)
>
> 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 | 121 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 121 insertions(+)
>
> diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> index 824da0e9738a..d34a92075174 100644
> --- a/rust/kernel/kunit.rs
> +++ b/rust/kernel/kunit.rs
> @@ -161,3 +161,124 @@ macro_rules! kunit_assert_eq {
>          $crate::kunit_assert!($name, $file, $diff, $left == $right);
>      }};
>  }
> +
> +/// Represents an individual test case.
> +///
> +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of valid test cases.
> +/// Use `kunit_case_null` to generate such a delimiter.

Can both of these be linkified? [`kunit_unsafe_test_suite!`] and
[`kunit_case_null`]. There are more instances below.

> +#[doc(hidden)]
> +pub 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(),

These members, after `name`, can be spelled `..kunit_case_null()` to
avoid some repetition.

> +    }
> +}
> +
> +/// Represents the NULL test case delimiter.
> +///
> +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. This
> +/// function returns such a delimiter.
> +#[doc(hidden)]
> +pub 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 valid test cases.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +/// 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);
> +/// ```
> +#[doc(hidden)]
> +#[macro_export]
> +macro_rules! kunit_unsafe_test_suite {
> +    ($name:ident, $test_cases:ident) => {
> +        const _: () = {
> +            const KUNIT_TEST_SUITE_NAME: [::kernel::ffi::c_char; 256] = {
> +                let name_u8 = ::core::stringify!($name).as_bytes();

This can be a little simpler:

let name = $crate::c_str!(::core::stringify!($name)).as_bytes_with_nul();

> +                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 ::kernel::ffi::c_char;
> +                    i += 1;
> +                }

I'd suggest `ret[..name.len()].copy_from_slice(name)` but
`copy_from_slice` isn't `const`. This can stay the same with the
now-unnecessary cast removed, or it can be the body of
`copy_from_slice`:

                // SAFETY: `name` is valid for `name.len()` elements
by definition, and `ret` was
                // checked to be at least as large as `name`. The
buffers are statically know to not
                // overlap.
                unsafe {
                    ::core::ptr::copy_nonoverlapping(name.as_ptr(),
ret.as_mut_ptr(), name.len());

                }

> +
> +                ret
> +            };
> +
> +            #[allow(unused_unsafe)]
> +            static mut KUNIT_TEST_SUITE: ::kernel::bindings::kunit_suite =
> +                ::kernel::bindings::kunit_suite {
> +                    name: KUNIT_TEST_SUITE_NAME,
> +                    // SAFETY: User is expected to pass a correct `test_cases`, given the safety
> +                    // precondition; hence this macro named `unsafe`.
> +                    test_cases: unsafe {
> +                        ::core::ptr::addr_of_mut!($test_cases)
> +                            .cast::<::kernel::bindings::kunit_case>()
> +                    },

This safety comment seems to be referring to the safety of
`addr_of_mut!` but this was just a compiler limitation until Rust
1.82, right? Same thing below on `KUNIT_TEST_SUITE_ENTRY`.

Can we also narrow the `#[allow]`? This seems to work:

                    #[allow(unused_unsafe)]
                    // SAFETY: ...
                    test_cases: unsafe {
                        ::core::ptr::addr_of_mut!($test_cases)
                            .cast::<::kernel::bindings::kunit_case>()
                    },

> +                    suite_init: None,
> +                    suite_exit: None,
> +                    init: None,
> +                    exit: None,
> +                    attr: ::kernel::bindings::kunit_attributes {
> +                        speed: ::kernel::bindings::kunit_speed_KUNIT_SPEED_NORMAL,
> +                    },
> +                    status_comment: [0; 256usize],

I don't think you need the usize hint here, there's always a usize in
the length position.

> +                    debugfs: ::core::ptr::null_mut(),
> +                    log: ::core::ptr::null_mut(),
> +                    suite_init_err: 0,
> +                    is_init: false,
> +                };
> +
> +            #[used]
> +            #[link_section = ".kunit_test_suites"]

This attribute causes the same problem I describe in
https://lore.kernel.org/all/20250210-macros-section-v2-1-3bb9ff44b969@gmail.com/.
That's because the KUnit tests are generated both on target and on
host (even though they won't run on host). I don't think you need to
deal with that here, just pointing it out. I think we'll need a cfg to
indicate we're building for host to avoid emitting these attributes
that only make sense in the kernel.

> +            static mut KUNIT_TEST_SUITE_ENTRY: *const ::kernel::bindings::kunit_suite =
> +                // SAFETY: `KUNIT_TEST_SUITE` is static.
> +                unsafe { ::core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) };

This is missing the `#[allow(unused_unsafe)]` (it appears in the next
patch). This means this commit will not compile on bisection.

> +        };
> +    };
> +}
> --
> 2.48.1.601.g30ceb7b040-goog
>
>

Global note: it seems more customary to use crate:: and $crate::
instead of kernel:: and ::kernel in functions and macros,
respectively.

Cheers.




Tamir

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

[Tamir Duberstein]

On Fri, Feb 14, 2025 at 2:41 AM David Gow <davidgow@google.com> wrote:
>
> 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>
> Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: David Gow <davidgow@google.com>
> ---
> Changes since v5:
> https://lore.kernel.org/all/20241213081035.2069066-3-davidgow@google.com/
> - Fix some rustfmt-related formatting shenanigans. (Thanks, Miguel)
> - Fix some documentation comment formatting as well. (Thanks, Miguel)
> - Tidy up the generated code to avoid unneeded &mut[] everywhere.
>   (Thanks, Miguel)
> - Fix a new clippy warning for using .as_bytes().len() instead of .len()
>   directly.
>
> 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 |  12 ++++
>  rust/macros/kunit.rs | 161 +++++++++++++++++++++++++++++++++++++++++++
>  rust/macros/lib.rs   |  29 ++++++++
>  4 files changed, 203 insertions(+)
>  create mode 100644 rust/macros/kunit.rs
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c8d9e8187eb0..4e7a6d2f2c49 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12677,6 +12677,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 d34a92075174..9e27b74a605b 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.
> @@ -275,6 +277,7 @@ macro_rules! kunit_unsafe_test_suite {
>                  };
>
>              #[used]
> +            #[allow(unused_unsafe)]
>              #[link_section = ".kunit_test_suites"]
>              static mut KUNIT_TEST_SUITE_ENTRY: *const ::kernel::bindings::kunit_suite =
>                  // SAFETY: `KUNIT_TEST_SUITE` is static.
> @@ -282,3 +285,12 @@ macro_rules! kunit_unsafe_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..4f553ecf40c0
> --- /dev/null
> +++ b/rust/macros/kunit.rs
> @@ -0,0 +1,161 @@
> +// 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 {
> +    let attr = attr.to_string();
> +
> +    if attr.is_empty() {
> +        panic!("Missing test name in `#[kunit_tests(test_name)]` macro")
> +    }
> +
> +    if attr.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(); }
> +    // 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);
> +    // ```

This is a really helpful comment. It got me wondering: can we have
host-side unit tests for our proc macros? Code is better than
comments, of course.

> +    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_wrapper = format!(
> +            "unsafe extern \"C\" fn {}(_test: *mut kernel::bindings::kunit) {{ {}(); }}",
> +            kunit_wrapper_fn_name, test
> +        );
> +        writeln!(kunit_macros, "{kunit_wrapper}").unwrap();
> +        writeln!(
> +            test_cases,
> +            "    kernel::kunit::kunit_case(kernel::c_str!(\"{}\"), {}),",
> +            test, kunit_wrapper_fn_name
> +        )
> +        .unwrap();
> +    }
> +
> +    writeln!(kunit_macros).unwrap();
> +    writeln!(
> +        kunit_macros,
> +        "static mut TEST_CASES: [kernel::bindings::kunit_case; {}] = [\n{test_cases}    kernel::kunit::kunit_case_null(),\n];",
> +        tests.len() + 1
> +    )
> +    .unwrap();
> +
> +    writeln!(
> +        kunit_macros,
> +        "kernel::kunit_unsafe_test_suite!({attr}, TEST_CASES);"
> +    )
> +    .unwrap();
> +
> +    // Remove the `#[test]` macros.

This makes sense. I wonder if we should think about being able to
declare a test that runs both on host and in KUnit.



> +    // 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 d61bc6a56425..50b58259c577 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.48.1.601.g30ceb7b040-goog
>
>

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

[Tamir Duberstein]

On Fri, Feb 14, 2025 at 2:42 AM David Gow <davidgow@google.com> wrote:
>
> 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>
> Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
>
> Changes since v5:
> https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/
> - Greatly improved documentation, which is both clearer and better
>   matches the rustdoc norm. (Thanks, Miguel)
> - The examples and safety comments are also both more idiomatic an
>   cleaner. (Thanks, Miguel)
> - More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)
>
> 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>
> diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> index 9e27b74a605b..3aad7a281b6d 100644
> --- a/rust/kernel/kunit.rs
> +++ b/rust/kernel/kunit.rs
> @@ -286,11 +286,77 @@ macro_rules! kunit_unsafe_test_suite {
>      };
>  }
>
> +/// 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.
> +///
> +/// # 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() {
> +///         return 100;
> +///     }
> +///
> +///     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:

[`bindings`] here, please. There are two more instances below but
those aren't doc comments, so I don't think bracketing them will do
anything.

> +///
> +/// ```
> +/// // 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);
> +/// ```

Isn't this swapping out the bindings module at compile time, and for
the whole build? In other words cfg(CONFIG_KUNIT) will apply to all
code, both test and non-test.

> +pub fn in_kunit_test() -> bool {
> +    // SAFETY: `kunit_get_current_test()` is always safe to call (it has fallbacks for
> +    // when KUnit is not enabled).
> +    unsafe { !bindings::kunit_get_current_test().is_null() }

Nit if you care about reducing unsafe blocks:

!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() {
> +        assert!(in_kunit_test());
> +    }
>  }
> --
> 2.48.1.601.g30ceb7b040-goog
>
>

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

[Miguel Ojeda]

On Fri, Feb 14, 2025 at 3:41 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> This is a really helpful comment. It got me wondering: can we have
> host-side unit tests for our proc macros? Code is better than
> comments, of course.

That makes sense (in fact, e.g. Benno wanted them for pinned-init),
but I will defer that until we have the new build system to avoid
adding more things to our plate.

> This makes sense. I wonder if we should think about being able to
> declare a test that runs both on host and in KUnit.

Yeah, when we originally discussed `#[test]`s (years ago), we wanted
to have "attributes" or "tags" like `#[test(host, kernel)]`.

But, again, something for later -- I would rather we finally land `#[test]`s.

Cheers,
Miguel

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

[Tamir Duberstein]

On Fri, Feb 14, 2025 at 1:38 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Fri, Feb 14, 2025 at 3:41 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > This is a really helpful comment. It got me wondering: can we have
> > host-side unit tests for our proc macros? Code is better than
> > comments, of course.
>
> That makes sense (in fact, e.g. Benno wanted them for pinned-init),
> but I will defer that until we have the new build system to avoid
> adding more things to our plate.
>
> > This makes sense. I wonder if we should think about being able to
> > declare a test that runs both on host and in KUnit.
>
> Yeah, when we originally discussed `#[test]`s (years ago), we wanted
> to have "attributes" or "tags" like `#[test(host, kernel)]`.
>
> But, again, something for later -- I would rather we finally land `#[test]`s.
>
> Cheers,
> Miguel

Works for me. Could you link to issues tracking these, if they exist?

Reviewed-by: Tamir Duberstein <tamird@gmail.com>

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

[Miguel Ojeda]

On Fri, Feb 14, 2025 at 8:41 AM David Gow <davidgow@google.com> wrote:
>
> After much delay, v6 of the KUnit/Rust integration patchset is here.
> This change incorporates most of Miguels suggestions from v5 (save for
> some of the copyright headers I wasn't comfortable unilaterally
> changing). This means the documentation is much improved, and it should
> work more cleanly on Rust 1.83 and 1.84, no longer requiring
> static_mut_refs or const_mut_refs. (I'm not 100% sure I understand all
> of the details of this, but I'm comfortable enough with how it's ended
> up.)

Thanks a lot, David, really.

I am happy to take it through Rust or KUnit -- I am not sure yet how
many conflicts we will have on our side.

I will give it a go when you think it is ready (since I saw comments
from Tamir, though not sure how many you may want to address --
perhaps the sooner we land this, the easier it may be...).

Cheers,
Miguel

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

[David Gow]

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

On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein <tamird@gmail.com> wrote:
>
> Very excited to see this progress.
>
> On Fri, Feb 14, 2025 at 2:41 AM David Gow <davidgow@google.com> wrote:
> >
> > 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: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > Co-developed-by: David Gow <davidgow@google.com>
> > Signed-off-by: David Gow <davidgow@google.com>
> > ---
> >
> > Changes since v5:
> > https://lore.kernel.org/all/20241213081035.2069066-2-davidgow@google.com/
> > - Rebased against 6.14-rc1
> > - Several documentation touch-ups, including noting that the example
> >   test function need not be unsafe. (Thanks, Miguel)
> > - Remove the need for static_mut_refs, by using core::addr_of_mut!()
> >   combined with a cast. (Thanks, Miguel)
> >   - While this should also avoid the need for const_mut_refs, it seems
> >     to have been enabled for other users anyway.
> > - Use ::kernel::ffi::c_char for C strings, rather than i8 directly.
> >   (Thanks, Miguel)
> >
> > 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 | 121 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 121 insertions(+)
> >
> > diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> > index 824da0e9738a..d34a92075174 100644
> > --- a/rust/kernel/kunit.rs
> > +++ b/rust/kernel/kunit.rs
> > @@ -161,3 +161,124 @@ macro_rules! kunit_assert_eq {
> >          $crate::kunit_assert!($name, $file, $diff, $left == $right);
> >      }};
> >  }
> > +
> > +/// Represents an individual test case.
> > +///
> > +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of valid test cases.
> > +/// Use `kunit_case_null` to generate such a delimiter.
>
> Can both of these be linkified? [`kunit_unsafe_test_suite!`] and
> [`kunit_case_null`]. There are more instances below.
>

I've done this here. I've not linkified absolutely everything, but the
most obvious/prominent ones should be done.

> > +#[doc(hidden)]
> > +pub 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(),
>
> These members, after `name`, can be spelled `..kunit_case_null()` to
> avoid some repetition.

I'm going to leave this as-is, as logically, the NULL terminator case
and the 'default' case are two different things (even if, in practice,
they have the same values). And personally, I find it clearer to have
them more explicitly set here for now.

It is a nice thought, though, so I reserve the right to change my mind
in the future. :-)

> > +    }
> > +}
> > +
> > +/// Represents the NULL test case delimiter.
> > +///
> > +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. This
> > +/// function returns such a delimiter.
> > +#[doc(hidden)]
> > +pub 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 valid test cases.
> > +///
> > +/// # Examples
> > +///
> > +/// ```ignore
> > +/// 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);
> > +/// ```
> > +#[doc(hidden)]
> > +#[macro_export]
> > +macro_rules! kunit_unsafe_test_suite {
> > +    ($name:ident, $test_cases:ident) => {
> > +        const _: () = {
> > +            const KUNIT_TEST_SUITE_NAME: [::kernel::ffi::c_char; 256] = {
> > +                let name_u8 = ::core::stringify!($name).as_bytes();
>
> This can be a little simpler:
>
> let name = $crate::c_str!(::core::stringify!($name)).as_bytes_with_nul();
>

I'm not sure this ends up being much simpler: it makes it (possible?,
obvious?) to get rid of the cast below, but we don't actually need to
copy the null byte at the end, so it seems wasteful to bother.

So after playing around both ways, I think this is probably best.

> > +                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 ::kernel::ffi::c_char;
> > +                    i += 1;
> > +                }
>
> I'd suggest `ret[..name.len()].copy_from_slice(name)` but
> `copy_from_slice` isn't `const`. This can stay the same with the
> now-unnecessary cast removed, or it can be the body of
> `copy_from_slice`:
>
>                 // SAFETY: `name` is valid for `name.len()` elements
> by definition, and `ret` was
>                 // checked to be at least as large as `name`. The
> buffers are statically know to not
>                 // overlap.
>                 unsafe {
>                     ::core::ptr::copy_nonoverlapping(name.as_ptr(),
> ret.as_mut_ptr(), name.len());
>
>                 }
>

I think I'll keep this as the loop for now, as that's more obvious to
me, and avoids the extra unsafe block.

If copy_from_slice ends up working in a const context, we can consider
that later (though, personally, I still find the loop easier to
understand).

> > +
> > +                ret
> > +            };
> > +
> > +            #[allow(unused_unsafe)]
> > +            static mut KUNIT_TEST_SUITE: ::kernel::bindings::kunit_suite =
> > +                ::kernel::bindings::kunit_suite {
> > +                    name: KUNIT_TEST_SUITE_NAME,
> > +                    // SAFETY: User is expected to pass a correct `test_cases`, given the safety
> > +                    // precondition; hence this macro named `unsafe`.
> > +                    test_cases: unsafe {
> > +                        ::core::ptr::addr_of_mut!($test_cases)
> > +                            .cast::<::kernel::bindings::kunit_case>()
> > +                    },
>
> This safety comment seems to be referring to the safety of
> `addr_of_mut!` but this was just a compiler limitation until Rust
> 1.82, right? Same thing below on `KUNIT_TEST_SUITE_ENTRY`.
>

Yeah, this comment was originally written prior to 1.82 existing.

But I think we probably should keep the safety comment here anyway, as
-- if I understand it -- 1.82 only makes this safe if $test_cases is
static, so it's still worth documenting the preconditions here.

If we eventually stop supporting rust < 1.82, though, I'd be happy to
remove the unsafe and thereby enforce the use of this macro only on
statics at compile time.

> Can we also narrow the `#[allow]`? This seems to work:
>
>                     #[allow(unused_unsafe)]
>                     // SAFETY: ...
>                     test_cases: unsafe {
>                         ::core::ptr::addr_of_mut!($test_cases)
>                             .cast::<::kernel::bindings::kunit_case>()
>                     },
>

We hit problems before with #[allow] not working on expressions, so
assumed we couldn't narrow it further. Seems to be working here,
though, so I've changed it.

> > +                    suite_init: None,
> > +                    suite_exit: None,
> > +                    init: None,
> > +                    exit: None,
> > +                    attr: ::kernel::bindings::kunit_attributes {
> > +                        speed: ::kernel::bindings::kunit_speed_KUNIT_SPEED_NORMAL,
> > +                    },
> > +                    status_comment: [0; 256usize],
>
> I don't think you need the usize hint here, there's always a usize in
> the length position.
>
> > +                    debugfs: ::core::ptr::null_mut(),
> > +                    log: ::core::ptr::null_mut(),
> > +                    suite_init_err: 0,
> > +                    is_init: false,
> > +                };
> > +
> > +            #[used]
> > +            #[link_section = ".kunit_test_suites"]
>
> This attribute causes the same problem I describe in
> https://lore.kernel.org/all/20250210-macros-section-v2-1-3bb9ff44b969@gmail.com/.
> That's because the KUnit tests are generated both on target and on
> host (even though they won't run on host). I don't think you need to
> deal with that here, just pointing it out. I think we'll need a cfg to
> indicate we're building for host to avoid emitting these attributes
> that only make sense in the kernel.
>

I've added the same exclusion for macos here, but don't have a macos
machine to test it on. If it's broken, we can fix it in a follow-up.

> > +            static mut KUNIT_TEST_SUITE_ENTRY: *const ::kernel::bindings::kunit_suite =
> > +                // SAFETY: `KUNIT_TEST_SUITE` is static.
> > +                unsafe { ::core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) };
>
> This is missing the `#[allow(unused_unsafe)]` (it appears in the next
> patch). This means this commit will not compile on bisection.

Whoops. This must have snuck in during one of the many squashes and
rebases. I'll fix that now.

Annoyingly, I'm no longer able to actually reproduce the warning,
which is strange.
>
> > +        };
> > +    };
> > +}
> > --
> > 2.48.1.601.g30ceb7b040-goog
> >
> >
>
> Global note: it seems more customary to use crate:: and $crate::
> instead of kernel:: and ::kernel in functions and macros,
> respectively.

Hmm... I think I had that at one point and was told to change it to
kernel. Personally, I think kernel:: makes more sense. So if it's not
breaking anything, and I'll only get yelled at a little bit, let's
keep it as is.

In any case, I'm sending out v7 with the changes above. My hope is
that this is then probably "good enough" to let us get started, and
future changes can be done as independent patches, so we're both not
holding this up for too long, and can better parallelise any fixes,
rather than having to re-spin the whole series each time.

Thanks a lot,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5294 bytes --]

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

[David Gow]

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

On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Fri, Feb 14, 2025 at 2:42 AM David Gow <davidgow@google.com> wrote:
> >
> > 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>
> > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > ---
> >
> > Changes since v5:
> > https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/
> > - Greatly improved documentation, which is both clearer and better
> >   matches the rustdoc norm. (Thanks, Miguel)
> > - The examples and safety comments are also both more idiomatic an
> >   cleaner. (Thanks, Miguel)
> > - More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)
> >
> > 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >
> > diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> > index 9e27b74a605b..3aad7a281b6d 100644
> > --- a/rust/kernel/kunit.rs
> > +++ b/rust/kernel/kunit.rs
> > @@ -286,11 +286,77 @@ macro_rules! kunit_unsafe_test_suite {
> >      };
> >  }
> >
> > +/// 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.
> > +///
> > +/// # 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() {
> > +///         return 100;
> > +///     }
> > +///
> > +///     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:
>
> [`bindings`] here, please. There are two more instances below but
> those aren't doc comments, so I don't think bracketing them will do
> anything.
>

Done in v7. Alas, I'll have to keep getting used to the differences
between kerneldoc and rustdoc...

> > +///
> > +/// ```
> > +/// // 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);
> > +/// ```
>
> Isn't this swapping out the bindings module at compile time, and for
> the whole build? In other words cfg(CONFIG_KUNIT) will apply to all
> code, both test and non-test.
>

I believe so, so this is probably something best done only in test files.

Ideally, we'd have support for something like the KUnit function
mocking features here, but that's horribly C-specific at the moment.

> > +pub fn in_kunit_test() -> bool {
> > +    // SAFETY: `kunit_get_current_test()` is always safe to call (it has fallbacks for
> > +    // when KUnit is not enabled).
> > +    unsafe { !bindings::kunit_get_current_test().is_null() }
>
> Nit if you care about reducing unsafe blocks:
>
> !unsafe { bindings::kunit_get_current_test() }.is_null()
>
>

Huh, I thought this wouldn't work, but it's working fine for me here,
so I've made the change.

Thanks!

> > +}
> > +
> >  #[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() {
> > +        assert!(in_kunit_test());
> > +    }
> >  }
> > --
> > 2.48.1.601.g30ceb7b040-goog
> >
> >

Thanks a lot, these should be fixed in v7.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5294 bytes --]

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

[David Gow]

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

On Sat, 15 Feb 2025 at 04:49, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Fri, Feb 14, 2025 at 8:41 AM David Gow <davidgow@google.com> wrote:
> >
> > After much delay, v6 of the KUnit/Rust integration patchset is here.
> > This change incorporates most of Miguels suggestions from v5 (save for
> > some of the copyright headers I wasn't comfortable unilaterally
> > changing). This means the documentation is much improved, and it should
> > work more cleanly on Rust 1.83 and 1.84, no longer requiring
> > static_mut_refs or const_mut_refs. (I'm not 100% sure I understand all
> > of the details of this, but I'm comfortable enough with how it's ended
> > up.)
>
> Thanks a lot, David, really.
>
> I am happy to take it through Rust or KUnit -- I am not sure yet how
> many conflicts we will have on our side.
>
> I will give it a go when you think it is ready (since I saw comments
> from Tamir, though not sure how many you may want to address --
> perhaps the sooner we land this, the easier it may be...).

Thanks!

I've sent out v7:
https://lore.kernel.org/rust-for-linux/20250215090622.2381038-1-davidgow@google.com/T/

This should address (most of) Tamir's comments. If that looks good to
you, feel free to apply it: it'd certainly be easier on my end to have
any nonessential fixes done separately, and if we can have people
start to use it in the rust tree concurrently, that'd be a bonus.

Thanks again for your patience -- I think we're finally nearly there!

-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5294 bytes --]

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

[Tamir Duberstein]

On Sat, Feb 15, 2025 at 4:03 AM David Gow <davidgow@google.com> wrote:
>
> On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > Very excited to see this progress.
> >
> > On Fri, Feb 14, 2025 at 2:41 AM David Gow <davidgow@google.com> wrote:
> > >
> > > 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: Miguel Ojeda <ojeda@kernel.org>
> > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > Co-developed-by: David Gow <davidgow@google.com>
> > > Signed-off-by: David Gow <davidgow@google.com>
> > > ---
> > >
> > > Changes since v5:
> > > https://lore.kernel.org/all/20241213081035.2069066-2-davidgow@google.com/
> > > - Rebased against 6.14-rc1
> > > - Several documentation touch-ups, including noting that the example
> > >   test function need not be unsafe. (Thanks, Miguel)
> > > - Remove the need for static_mut_refs, by using core::addr_of_mut!()
> > >   combined with a cast. (Thanks, Miguel)
> > >   - While this should also avoid the need for const_mut_refs, it seems
> > >     to have been enabled for other users anyway.
> > > - Use ::kernel::ffi::c_char for C strings, rather than i8 directly.
> > >   (Thanks, Miguel)
> > >
> > > 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 | 121 +++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 121 insertions(+)
> > >
> > > diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> > > index 824da0e9738a..d34a92075174 100644
> > > --- a/rust/kernel/kunit.rs
> > > +++ b/rust/kernel/kunit.rs
> > > @@ -161,3 +161,124 @@ macro_rules! kunit_assert_eq {
> > >          $crate::kunit_assert!($name, $file, $diff, $left == $right);
> > >      }};
> > >  }
> > > +
> > > +/// Represents an individual test case.
> > > +///
> > > +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of valid test cases.
> > > +/// Use `kunit_case_null` to generate such a delimiter.
> >
> > Can both of these be linkified? [`kunit_unsafe_test_suite!`] and
> > [`kunit_case_null`]. There are more instances below.
> >
>
> I've done this here. I've not linkified absolutely everything, but the
> most obvious/prominent ones should be done.
>
> > > +#[doc(hidden)]
> > > +pub 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(),
> >
> > These members, after `name`, can be spelled `..kunit_case_null()` to
> > avoid some repetition.
>
> I'm going to leave this as-is, as logically, the NULL terminator case
> and the 'default' case are two different things (even if, in practice,
> they have the same values). And personally, I find it clearer to have
> them more explicitly set here for now.
>
> It is a nice thought, though, so I reserve the right to change my mind
> in the future. :-)
>
> > > +    }
> > > +}
> > > +
> > > +/// Represents the NULL test case delimiter.
> > > +///
> > > +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. This
> > > +/// function returns such a delimiter.
> > > +#[doc(hidden)]
> > > +pub 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 valid test cases.
> > > +///
> > > +/// # Examples
> > > +///
> > > +/// ```ignore
> > > +/// 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);
> > > +/// ```
> > > +#[doc(hidden)]
> > > +#[macro_export]
> > > +macro_rules! kunit_unsafe_test_suite {
> > > +    ($name:ident, $test_cases:ident) => {
> > > +        const _: () = {
> > > +            const KUNIT_TEST_SUITE_NAME: [::kernel::ffi::c_char; 256] = {
> > > +                let name_u8 = ::core::stringify!($name).as_bytes();
> >
> > This can be a little simpler:
> >
> > let name = $crate::c_str!(::core::stringify!($name)).as_bytes_with_nul();
> >
>
> I'm not sure this ends up being much simpler: it makes it (possible?,
> obvious?) to get rid of the cast below, but we don't actually need to
> copy the null byte at the end, so it seems wasteful to bother.
>
> So after playing around both ways, I think this is probably best.

If you don't want to copy the null byte, you can s/as_bytes_with_nul/as_bytes.

The main thing is that `as` casts in Rust are a rare instance of
unchecked coercion in the language, so I encourage folks to avoid
them. The rest I don't feel strongly about.

> > > +                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 ::kernel::ffi::c_char;
> > > +                    i += 1;
> > > +                }
> >
> > I'd suggest `ret[..name.len()].copy_from_slice(name)` but
> > `copy_from_slice` isn't `const`. This can stay the same with the
> > now-unnecessary cast removed, or it can be the body of
> > `copy_from_slice`:
> >
> >                 // SAFETY: `name` is valid for `name.len()` elements
> > by definition, and `ret` was
> >                 // checked to be at least as large as `name`. The
> > buffers are statically know to not
> >                 // overlap.
> >                 unsafe {
> >                     ::core::ptr::copy_nonoverlapping(name.as_ptr(),
> > ret.as_mut_ptr(), name.len());
> >
> >                 }
> >
>
> I think I'll keep this as the loop for now, as that's more obvious to
> me, and avoids the extra unsafe block.
>
> If copy_from_slice ends up working in a const context, we can consider
> that later (though, personally, I still find the loop easier to
> understand).
>
> > > +
> > > +                ret
> > > +            };
> > > +
> > > +            #[allow(unused_unsafe)]
> > > +            static mut KUNIT_TEST_SUITE: ::kernel::bindings::kunit_suite =
> > > +                ::kernel::bindings::kunit_suite {
> > > +                    name: KUNIT_TEST_SUITE_NAME,
> > > +                    // SAFETY: User is expected to pass a correct `test_cases`, given the safety
> > > +                    // precondition; hence this macro named `unsafe`.
> > > +                    test_cases: unsafe {
> > > +                        ::core::ptr::addr_of_mut!($test_cases)
> > > +                            .cast::<::kernel::bindings::kunit_case>()
> > > +                    },
> >
> > This safety comment seems to be referring to the safety of
> > `addr_of_mut!` but this was just a compiler limitation until Rust
> > 1.82, right? Same thing below on `KUNIT_TEST_SUITE_ENTRY`.
> >
>
> Yeah, this comment was originally written prior to 1.82 existing.
>
> But I think we probably should keep the safety comment here anyway, as
> -- if I understand it -- 1.82 only makes this safe if $test_cases is
> static, so it's still worth documenting the preconditions here.

I don't think the safety guarantees changed. In the Rust 1.82 release notes:

> In an expression context, STATIC_MUT and EXTERN_STATIC are place expressions. Previously, the compiler's safety checks were not aware that the raw ref operator did not actually affect the operand's place, treating it as a possible read or write to a pointer. No unsafety is actually present, however, as it just creates a pointer.

https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-unsafe-statics

That's why I flagged this; the SAFETY comment is not actually correct.

> If we eventually stop supporting rust < 1.82, though, I'd be happy to
> remove the unsafe and thereby enforce the use of this macro only on
> statics at compile time.
>
> > Can we also narrow the `#[allow]`? This seems to work:
> >
> >                     #[allow(unused_unsafe)]
> >                     // SAFETY: ...
> >                     test_cases: unsafe {
> >                         ::core::ptr::addr_of_mut!($test_cases)
> >                             .cast::<::kernel::bindings::kunit_case>()
> >                     },
> >
>
> We hit problems before with #[allow] not working on expressions, so
> assumed we couldn't narrow it further. Seems to be working here,
> though, so I've changed it.
>
> > > +                    suite_init: None,
> > > +                    suite_exit: None,
> > > +                    init: None,
> > > +                    exit: None,
> > > +                    attr: ::kernel::bindings::kunit_attributes {
> > > +                        speed: ::kernel::bindings::kunit_speed_KUNIT_SPEED_NORMAL,
> > > +                    },
> > > +                    status_comment: [0; 256usize],
> >
> > I don't think you need the usize hint here, there's always a usize in
> > the length position.
> >
> > > +                    debugfs: ::core::ptr::null_mut(),
> > > +                    log: ::core::ptr::null_mut(),
> > > +                    suite_init_err: 0,
> > > +                    is_init: false,
> > > +                };
> > > +
> > > +            #[used]
> > > +            #[link_section = ".kunit_test_suites"]
> >
> > This attribute causes the same problem I describe in
> > https://lore.kernel.org/all/20250210-macros-section-v2-1-3bb9ff44b969@gmail.com/.
> > That's because the KUnit tests are generated both on target and on
> > host (even though they won't run on host). I don't think you need to
> > deal with that here, just pointing it out. I think we'll need a cfg to
> > indicate we're building for host to avoid emitting these attributes
> > that only make sense in the kernel.
> >
>
> I've added the same exclusion for macos here, but don't have a macos
> machine to test it on. If it's broken, we can fix it in a follow-up.
>
> > > +            static mut KUNIT_TEST_SUITE_ENTRY: *const ::kernel::bindings::kunit_suite =
> > > +                // SAFETY: `KUNIT_TEST_SUITE` is static.
> > > +                unsafe { ::core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) };
> >
> > This is missing the `#[allow(unused_unsafe)]` (it appears in the next
> > patch). This means this commit will not compile on bisection.
>
> Whoops. This must have snuck in during one of the many squashes and
> rebases. I'll fix that now.
>
> Annoyingly, I'm no longer able to actually reproduce the warning,
> which is strange.
> >
> > > +        };
> > > +    };
> > > +}
> > > --
> > > 2.48.1.601.g30ceb7b040-goog
> > >
> > >
> >
> > Global note: it seems more customary to use crate:: and $crate::
> > instead of kernel:: and ::kernel in functions and macros,
> > respectively.
>
> Hmm... I think I had that at one point and was told to change it to
> kernel. Personally, I think kernel:: makes more sense. So if it's not
> breaking anything, and I'll only get yelled at a little bit, let's
> keep it as is.
>
> In any case, I'm sending out v7 with the changes above. My hope is
> that this is then probably "good enough" to let us get started, and
> future changes can be done as independent patches, so we're both not
> holding this up for too long, and can better parallelise any fixes,
> rather than having to re-spin the whole series each time.
>
> Thanks a lot,
> -- David

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

[Tamir Duberstein]

On Sat, Feb 15, 2025 at 4:03 AM David Gow <davidgow@google.com> wrote:
>
> On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > On Fri, Feb 14, 2025 at 2:42 AM David Gow <davidgow@google.com> wrote:
> > >
> > > 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>
> > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > ---
> > >
> > > Changes since v5:
> > > https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/
> > > - Greatly improved documentation, which is both clearer and better
> > >   matches the rustdoc norm. (Thanks, Miguel)
> > > - The examples and safety comments are also both more idiomatic an
> > >   cleaner. (Thanks, Miguel)
> > > - More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)
> > >
> > > 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 66 insertions(+)
> > >
> > > diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> > > index 9e27b74a605b..3aad7a281b6d 100644
> > > --- a/rust/kernel/kunit.rs
> > > +++ b/rust/kernel/kunit.rs
> > > @@ -286,11 +286,77 @@ macro_rules! kunit_unsafe_test_suite {
> > >      };
> > >  }
> > >
> > > +/// 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.
> > > +///
> > > +/// # 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() {
> > > +///         return 100;
> > > +///     }
> > > +///
> > > +///     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:
> >
> > [`bindings`] here, please. There are two more instances below but
> > those aren't doc comments, so I don't think bracketing them will do
> > anything.
> >
>
> Done in v7. Alas, I'll have to keep getting used to the differences
> between kerneldoc and rustdoc...
>
> > > +///
> > > +/// ```
> > > +/// // 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);
> > > +/// ```
> >
> > Isn't this swapping out the bindings module at compile time, and for
> > the whole build? In other words cfg(CONFIG_KUNIT) will apply to all
> > code, both test and non-test.
> >
>
> I believe so, so this is probably something best done only in test files.

Why would you need conditional compilation of this kind in test files?

What I was getting at with this comment is that this example might
mislead a user to think that this is how they should imbue their code
with test-specific behavior, which is not what this will do. Instead
this would break kernels built with CONFIG_KUNIT, which I think is not
where we want to be going. Right?

>
> Ideally, we'd have support for something like the KUnit function
> mocking features here, but that's horribly C-specific at the moment.
>
> > > +pub fn in_kunit_test() -> bool {
> > > +    // SAFETY: `kunit_get_current_test()` is always safe to call (it has fallbacks for
> > > +    // when KUnit is not enabled).
> > > +    unsafe { !bindings::kunit_get_current_test().is_null() }
> >
> > Nit if you care about reducing unsafe blocks:
> >
> > !unsafe { bindings::kunit_get_current_test() }.is_null()
> >
> >
>
> Huh, I thought this wouldn't work, but it's working fine for me here,
> so I've made the change.
>
> Thanks!
>
> > > +}
> > > +
> > >  #[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() {
> > > +        assert!(in_kunit_test());
> > > +    }
> > >  }
> > > --
> > > 2.48.1.601.g30ceb7b040-goog
> > >
> > >
>
> Thanks a lot, these should be fixed in v7.
>
> Cheers,
> -- David

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

[David Gow]

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

On Sat, 15 Feb 2025 at 21:04, Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Sat, Feb 15, 2025 at 4:03 AM David Gow <davidgow@google.com> wrote:
> >
> > On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein <tamird@gmail.com> wrote:
> > >
> > > On Fri, Feb 14, 2025 at 2:42 AM David Gow <davidgow@google.com> wrote:
> > > >
> > > > 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>
> > > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > > ---
> > > >
> > > > Changes since v5:
> > > > https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/
> > > > - Greatly improved documentation, which is both clearer and better
> > > >   matches the rustdoc norm. (Thanks, Miguel)
> > > > - The examples and safety comments are also both more idiomatic an
> > > >   cleaner. (Thanks, Miguel)
> > > > - More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)
> > > >
> > > > 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 66 insertions(+)
> > > >
> > > > diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> > > > index 9e27b74a605b..3aad7a281b6d 100644
> > > > --- a/rust/kernel/kunit.rs
> > > > +++ b/rust/kernel/kunit.rs
> > > > @@ -286,11 +286,77 @@ macro_rules! kunit_unsafe_test_suite {
> > > >      };
> > > >  }
> > > >
> > > > +/// 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.
> > > > +///
> > > > +/// # 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() {
> > > > +///         return 100;
> > > > +///     }
> > > > +///
> > > > +///     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:
> > >
> > > [`bindings`] here, please. There are two more instances below but
> > > those aren't doc comments, so I don't think bracketing them will do
> > > anything.
> > >
> >
> > Done in v7. Alas, I'll have to keep getting used to the differences
> > between kerneldoc and rustdoc...
> >
> > > > +///
> > > > +/// ```
> > > > +/// // 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);
> > > > +/// ```
> > >
> > > Isn't this swapping out the bindings module at compile time, and for
> > > the whole build? In other words cfg(CONFIG_KUNIT) will apply to all
> > > code, both test and non-test.
> > >
> >
> > I believe so, so this is probably something best done only in test files.
>
> Why would you need conditional compilation of this kind in test files?
>
> What I was getting at with this comment is that this example might
> mislead a user to think that this is how they should imbue their code
> with test-specific behavior, which is not what this will do. Instead
> this would break kernels built with CONFIG_KUNIT, which I think is not
> where we want to be going. Right?
>

Hmm... I think I get this now. (I confess, I'm still wrapping my head
around the crate/module/file boundaries in Rust, so assumed there must
be something clever going on.)

I'd definitely rather CONFIG_KUNIT not break a kernel, particularly
since some distros do enable it (at least as a module) by default. So,
at the very least, this should be behind a more specific kconfig. But
it'd be better still to not break anything at all.

José: am I missing something here, or does this need either changing
or removing?

Maybe we should just have the 'mock' version include something like:
if in_kunit_test() {
    1234
} else {
    bindings::ktime_get_boot_fast_ns()
}

And in the long term, we can try to hook into the static stubbing
framework to allow this to be turned on and off for individual
functions. (I imagine we could get close with macros, which is what we
do in C, though the thought of trying to deal with function pointers
and Rust's lack of stable ABI here scares me.)

Either way, we're definitely going to need a follow-up documentation
patch for this whole feature, so if we want to get rid of or update
this example, we can either do it then, or now.

I'm afraid I'm unlikely to have time to work on Rust stuff probably
for the rest of the month, so I'll let the discussion play out (and if
people desperately want the patches before then, I'd be happy to fix
them in follow-ups).

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5294 bytes --]

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

[David Gow]

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

On Sat, 15 Feb 2025 at 21:01, Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Sat, Feb 15, 2025 at 4:03 AM David Gow <davidgow@google.com> wrote:
> >
> > On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein <tamird@gmail.com> wrote:
> > >
> > > Very excited to see this progress.
> > >
> > > On Fri, Feb 14, 2025 at 2:41 AM David Gow <davidgow@google.com> wrote:
> > > >
> > > > 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: Miguel Ojeda <ojeda@kernel.org>
> > > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > > Co-developed-by: David Gow <davidgow@google.com>
> > > > Signed-off-by: David Gow <davidgow@google.com>
> > > > ---
> > > >
> > > > Changes since v5:
> > > > https://lore.kernel.org/all/20241213081035.2069066-2-davidgow@google.com/
> > > > - Rebased against 6.14-rc1
> > > > - Several documentation touch-ups, including noting that the example
> > > >   test function need not be unsafe. (Thanks, Miguel)
> > > > - Remove the need for static_mut_refs, by using core::addr_of_mut!()
> > > >   combined with a cast. (Thanks, Miguel)
> > > >   - While this should also avoid the need for const_mut_refs, it seems
> > > >     to have been enabled for other users anyway.
> > > > - Use ::kernel::ffi::c_char for C strings, rather than i8 directly.
> > > >   (Thanks, Miguel)
> > > >
> > > > 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 | 121 +++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 121 insertions(+)
> > > >
> > > > diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> > > > index 824da0e9738a..d34a92075174 100644
> > > > --- a/rust/kernel/kunit.rs
> > > > +++ b/rust/kernel/kunit.rs
> > > > @@ -161,3 +161,124 @@ macro_rules! kunit_assert_eq {
> > > >          $crate::kunit_assert!($name, $file, $diff, $left == $right);
> > > >      }};
> > > >  }
> > > > +
> > > > +/// Represents an individual test case.
> > > > +///
> > > > +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of valid test cases.
> > > > +/// Use `kunit_case_null` to generate such a delimiter.
> > >
> > > Can both of these be linkified? [`kunit_unsafe_test_suite!`] and
> > > [`kunit_case_null`]. There are more instances below.
> > >
> >
> > I've done this here. I've not linkified absolutely everything, but the
> > most obvious/prominent ones should be done.
> >
> > > > +#[doc(hidden)]
> > > > +pub 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(),
> > >
> > > These members, after `name`, can be spelled `..kunit_case_null()` to
> > > avoid some repetition.
> >
> > I'm going to leave this as-is, as logically, the NULL terminator case
> > and the 'default' case are two different things (even if, in practice,
> > they have the same values). And personally, I find it clearer to have
> > them more explicitly set here for now.
> >
> > It is a nice thought, though, so I reserve the right to change my mind
> > in the future. :-)
> >
> > > > +    }
> > > > +}
> > > > +
> > > > +/// Represents the NULL test case delimiter.
> > > > +///
> > > > +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. This
> > > > +/// function returns such a delimiter.
> > > > +#[doc(hidden)]
> > > > +pub 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 valid test cases.
> > > > +///
> > > > +/// # Examples
> > > > +///
> > > > +/// ```ignore
> > > > +/// 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);
> > > > +/// ```
> > > > +#[doc(hidden)]
> > > > +#[macro_export]
> > > > +macro_rules! kunit_unsafe_test_suite {
> > > > +    ($name:ident, $test_cases:ident) => {
> > > > +        const _: () = {
> > > > +            const KUNIT_TEST_SUITE_NAME: [::kernel::ffi::c_char; 256] = {
> > > > +                let name_u8 = ::core::stringify!($name).as_bytes();
> > >
> > > This can be a little simpler:
> > >
> > > let name = $crate::c_str!(::core::stringify!($name)).as_bytes_with_nul();
> > >
> >
> > I'm not sure this ends up being much simpler: it makes it (possible?,
> > obvious?) to get rid of the cast below, but we don't actually need to
> > copy the null byte at the end, so it seems wasteful to bother.
> >
> > So after playing around both ways, I think this is probably best.
>
> If you don't want to copy the null byte, you can s/as_bytes_with_nul/as_bytes.
>
> The main thing is that `as` casts in Rust are a rare instance of
> unchecked coercion in the language, so I encourage folks to avoid
> them. The rest I don't feel strongly about.
>

As far as I can tell, as_bytes() returns a u8 slice anyway, so
shouldn't we need the cast anyway? Or is there something I'm missing?

(Also, is there a difference between this and the Rust stdlib's
to_bytes()? Or is the name difference just a glitch?)

Either way, I don't personally feel too strongly about it: the 'as'
cast doesn't worry me particularly (particularly out-on-the open like
this, rather than hidden behind lots of macros/indirection), but I'm
happy to bow to stronger opinions for now.


> > > > +                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 ::kernel::ffi::c_char;
> > > > +                    i += 1;
> > > > +                }
> > >
> > > I'd suggest `ret[..name.len()].copy_from_slice(name)` but
> > > `copy_from_slice` isn't `const`. This can stay the same with the
> > > now-unnecessary cast removed, or it can be the body of
> > > `copy_from_slice`:
> > >
> > >                 // SAFETY: `name` is valid for `name.len()` elements
> > > by definition, and `ret` was
> > >                 // checked to be at least as large as `name`. The
> > > buffers are statically know to not
> > >                 // overlap.
> > >                 unsafe {
> > >                     ::core::ptr::copy_nonoverlapping(name.as_ptr(),
> > > ret.as_mut_ptr(), name.len());
> > >
> > >                 }
> > >
> >
> > I think I'll keep this as the loop for now, as that's more obvious to
> > me, and avoids the extra unsafe block.
> >
> > If copy_from_slice ends up working in a const context, we can consider
> > that later (though, personally, I still find the loop easier to
> > understand).
> >
> > > > +
> > > > +                ret
> > > > +            };
> > > > +
> > > > +            #[allow(unused_unsafe)]
> > > > +            static mut KUNIT_TEST_SUITE: ::kernel::bindings::kunit_suite =
> > > > +                ::kernel::bindings::kunit_suite {
> > > > +                    name: KUNIT_TEST_SUITE_NAME,
> > > > +                    // SAFETY: User is expected to pass a correct `test_cases`, given the safety
> > > > +                    // precondition; hence this macro named `unsafe`.
> > > > +                    test_cases: unsafe {
> > > > +                        ::core::ptr::addr_of_mut!($test_cases)
> > > > +                            .cast::<::kernel::bindings::kunit_case>()
> > > > +                    },
> > >
> > > This safety comment seems to be referring to the safety of
> > > `addr_of_mut!` but this was just a compiler limitation until Rust
> > > 1.82, right? Same thing below on `KUNIT_TEST_SUITE_ENTRY`.
> > >
> >
> > Yeah, this comment was originally written prior to 1.82 existing.
> >
> > But I think we probably should keep the safety comment here anyway, as
> > -- if I understand it -- 1.82 only makes this safe if $test_cases is
> > static, so it's still worth documenting the preconditions here.
>
> I don't think the safety guarantees changed. In the Rust 1.82 release notes:
>
> > In an expression context, STATIC_MUT and EXTERN_STATIC are place expressions. Previously, the compiler's safety checks were not aware that the raw ref operator did not actually affect the operand's place, treating it as a possible read or write to a pointer. No unsafety is actually present, however, as it just creates a pointer.
>
> https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-unsafe-statics
>
> That's why I flagged this; the SAFETY comment is not actually correct.
>

I won't pretend to be an expert on the exact semantics of Rust
references / place expressions / etc here -- I still don't totally
understand why this needs the cast for a start -- but I do still think
there's more to the story than "this is just a compiler limitation".

The reason is that, whilst -- as you suggest -- this is always safe
when $test_cases is static, that's not actually guaranteed anywhere.
And with the unsafe block, it's up to the _user_ of
kunit_unsafe_test_suite() to guarantee that $test_cases is safe here.

Now, I don't think the current documentation is particularly clear
about this, so I'm definitely open to it changing, though I think we'd
need to change the overall documentation for the macro, not just the
safety comment. And maybe this will be something which, presumably, we
can have enforced by the compiler in the future, should we be able to
depend on rustc>1.82 and remove the 'unsafe' entirely. (But support
for older compilers is important to me, so I don't want to push that
point too much.)

(Also, does anyone else find the whole 'lets change the unsafe rules
in a minor compiler version, and require a weird attribute to avoid a
warning' thing incredibly frustrating? I've read that it's not what
the formal purpose of Rust editions is, but it _feels_ like this sort
of change should be in an edition intuitively to me.)

Anyway, I've got a few higher-priority non-Rust things to do by the
end of the month, so I'm unlikely to have time to spin up a v8 myself
for a few weeks. So if folks want to either send out an updated
version or the series, or accept it as-is and send out a follow-up
fix, I'm happy to review it, but otherwise it'll have to wait a little
bit.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5294 bytes --]

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

[Tamir Duberstein]

On Tue, Feb 18, 2025 at 3:51 AM David Gow <davidgow@google.com> wrote:
>
> On Sat, 15 Feb 2025 at 21:01, Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > On Sat, Feb 15, 2025 at 4:03 AM David Gow <davidgow@google.com> wrote:
> > >
> > > On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein <tamird@gmail.com> wrote:
> > > >
> > > > Very excited to see this progress.
> > > >
> > > > On Fri, Feb 14, 2025 at 2:41 AM David Gow <davidgow@google.com> wrote:
> > > > >
> > > > > 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: Miguel Ojeda <ojeda@kernel.org>
> > > > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > > > Co-developed-by: David Gow <davidgow@google.com>
> > > > > Signed-off-by: David Gow <davidgow@google.com>
> > > > > ---
> > > > >
> > > > > Changes since v5:
> > > > > https://lore.kernel.org/all/20241213081035.2069066-2-davidgow@google.com/
> > > > > - Rebased against 6.14-rc1
> > > > > - Several documentation touch-ups, including noting that the example
> > > > >   test function need not be unsafe. (Thanks, Miguel)
> > > > > - Remove the need for static_mut_refs, by using core::addr_of_mut!()
> > > > >   combined with a cast. (Thanks, Miguel)
> > > > >   - While this should also avoid the need for const_mut_refs, it seems
> > > > >     to have been enabled for other users anyway.
> > > > > - Use ::kernel::ffi::c_char for C strings, rather than i8 directly.
> > > > >   (Thanks, Miguel)
> > > > >
> > > > > 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 | 121 +++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 121 insertions(+)
> > > > >
> > > > > diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> > > > > index 824da0e9738a..d34a92075174 100644
> > > > > --- a/rust/kernel/kunit.rs
> > > > > +++ b/rust/kernel/kunit.rs
> > > > > @@ -161,3 +161,124 @@ macro_rules! kunit_assert_eq {
> > > > >          $crate::kunit_assert!($name, $file, $diff, $left == $right);
> > > > >      }};
> > > > >  }
> > > > > +
> > > > > +/// Represents an individual test case.
> > > > > +///
> > > > > +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of valid test cases.
> > > > > +/// Use `kunit_case_null` to generate such a delimiter.
> > > >
> > > > Can both of these be linkified? [`kunit_unsafe_test_suite!`] and
> > > > [`kunit_case_null`]. There are more instances below.
> > > >
> > >
> > > I've done this here. I've not linkified absolutely everything, but the
> > > most obvious/prominent ones should be done.
> > >
> > > > > +#[doc(hidden)]
> > > > > +pub 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(),
> > > >
> > > > These members, after `name`, can be spelled `..kunit_case_null()` to
> > > > avoid some repetition.
> > >
> > > I'm going to leave this as-is, as logically, the NULL terminator case
> > > and the 'default' case are two different things (even if, in practice,
> > > they have the same values). And personally, I find it clearer to have
> > > them more explicitly set here for now.
> > >
> > > It is a nice thought, though, so I reserve the right to change my mind
> > > in the future. :-)
> > >
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +/// Represents the NULL test case delimiter.
> > > > > +///
> > > > > +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. This
> > > > > +/// function returns such a delimiter.
> > > > > +#[doc(hidden)]
> > > > > +pub 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 valid test cases.
> > > > > +///
> > > > > +/// # Examples
> > > > > +///
> > > > > +/// ```ignore
> > > > > +/// 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);
> > > > > +/// ```
> > > > > +#[doc(hidden)]
> > > > > +#[macro_export]
> > > > > +macro_rules! kunit_unsafe_test_suite {
> > > > > +    ($name:ident, $test_cases:ident) => {
> > > > > +        const _: () = {
> > > > > +            const KUNIT_TEST_SUITE_NAME: [::kernel::ffi::c_char; 256] = {
> > > > > +                let name_u8 = ::core::stringify!($name).as_bytes();
> > > >
> > > > This can be a little simpler:
> > > >
> > > > let name = $crate::c_str!(::core::stringify!($name)).as_bytes_with_nul();
> > > >
> > >
> > > I'm not sure this ends up being much simpler: it makes it (possible?,
> > > obvious?) to get rid of the cast below, but we don't actually need to
> > > copy the null byte at the end, so it seems wasteful to bother.
> > >
> > > So after playing around both ways, I think this is probably best.
> >
> > If you don't want to copy the null byte, you can s/as_bytes_with_nul/as_bytes.
> >
> > The main thing is that `as` casts in Rust are a rare instance of
> > unchecked coercion in the language, so I encourage folks to avoid
> > them. The rest I don't feel strongly about.
> >
>
> As far as I can tell, as_bytes() returns a u8 slice anyway, so
> shouldn't we need the cast anyway? Or is there something I'm missing?

Ah, we don't need the cast at all, I think. `kernel::ffi::c_char` is u8.

> (Also, is there a difference between this and the Rust stdlib's
> to_bytes()? Or is the name difference just a glitch?)

It's just an inconsistency. I believe our CStr predates some necessary
features in the standard library. There is
https://lore.kernel.org/all/20250203-cstr-core-v8-0-cb3f26e78686@gmail.com/t/#u
to replace our custom implementation with upstream's.

> Either way, I don't personally feel too strongly about it: the 'as'
> cast doesn't worry me particularly (particularly out-on-the open like
> this, rather than hidden behind lots of macros/indirection), but I'm
> happy to bow to stronger opinions for now.
>
>
> > > > > +                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 ::kernel::ffi::c_char;
> > > > > +                    i += 1;
> > > > > +                }
> > > >
> > > > I'd suggest `ret[..name.len()].copy_from_slice(name)` but
> > > > `copy_from_slice` isn't `const`. This can stay the same with the
> > > > now-unnecessary cast removed, or it can be the body of
> > > > `copy_from_slice`:
> > > >
> > > >                 // SAFETY: `name` is valid for `name.len()` elements
> > > > by definition, and `ret` was
> > > >                 // checked to be at least as large as `name`. The
> > > > buffers are statically know to not
> > > >                 // overlap.
> > > >                 unsafe {
> > > >                     ::core::ptr::copy_nonoverlapping(name.as_ptr(),
> > > > ret.as_mut_ptr(), name.len());
> > > >
> > > >                 }
> > > >
> > >
> > > I think I'll keep this as the loop for now, as that's more obvious to
> > > me, and avoids the extra unsafe block.
> > >
> > > If copy_from_slice ends up working in a const context, we can consider
> > > that later (though, personally, I still find the loop easier to
> > > understand).
> > >
> > > > > +
> > > > > +                ret
> > > > > +            };
> > > > > +
> > > > > +            #[allow(unused_unsafe)]
> > > > > +            static mut KUNIT_TEST_SUITE: ::kernel::bindings::kunit_suite =
> > > > > +                ::kernel::bindings::kunit_suite {
> > > > > +                    name: KUNIT_TEST_SUITE_NAME,
> > > > > +                    // SAFETY: User is expected to pass a correct `test_cases`, given the safety
> > > > > +                    // precondition; hence this macro named `unsafe`.
> > > > > +                    test_cases: unsafe {
> > > > > +                        ::core::ptr::addr_of_mut!($test_cases)
> > > > > +                            .cast::<::kernel::bindings::kunit_case>()
> > > > > +                    },
> > > >
> > > > This safety comment seems to be referring to the safety of
> > > > `addr_of_mut!` but this was just a compiler limitation until Rust
> > > > 1.82, right? Same thing below on `KUNIT_TEST_SUITE_ENTRY`.
> > > >
> > >
> > > Yeah, this comment was originally written prior to 1.82 existing.
> > >
> > > But I think we probably should keep the safety comment here anyway, as
> > > -- if I understand it -- 1.82 only makes this safe if $test_cases is
> > > static, so it's still worth documenting the preconditions here.
> >
> > I don't think the safety guarantees changed. In the Rust 1.82 release notes:
> >
> > > In an expression context, STATIC_MUT and EXTERN_STATIC are place expressions. Previously, the compiler's safety checks were not aware that the raw ref operator did not actually affect the operand's place, treating it as a possible read or write to a pointer. No unsafety is actually present, however, as it just creates a pointer.
> >
> > https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-unsafe-statics
> >
> > That's why I flagged this; the SAFETY comment is not actually correct.
> >
>
> I won't pretend to be an expert on the exact semantics of Rust
> references / place expressions / etc here -- I still don't totally
> understand why this needs the cast for a start -- but I do still think
> there's more to the story than "this is just a compiler limitation".
>
> The reason is that, whilst -- as you suggest -- this is always safe
> when $test_cases is static, that's not actually guaranteed anywhere.
> And with the unsafe block, it's up to the _user_ of
> kunit_unsafe_test_suite() to guarantee that $test_cases is safe here.

It's the other way around. In Rust, it is unsafe to take mutable
references to statics. What the compiler didn't understand until 1.82
is that taking a raw pointer to the static did not require forming a
reference, and thus wasn't unsafe (later dereferencing that pointer to
create a reference would be unsafe, static or not).

> Now, I don't think the current documentation is particularly clear
> about this, so I'm definitely open to it changing, though I think we'd
> need to change the overall documentation for the macro, not just the
> safety comment. And maybe this will be something which, presumably, we
> can have enforced by the compiler in the future, should we be able to
> depend on rustc>1.82 and remove the 'unsafe' entirely. (But support
> for older compilers is important to me, so I don't want to push that
> point too much.)

I think there's a bit of confusion going on here because the same word
is being used to describe the Rust notion of memory safety as well as
the preconditions expected by KUnit. That's why this comment is so
misleading; the compiler says "please tell me why it's safe to form a
mutable reference to this static here" and the answer comes "it's safe
because the user pinky promised to end the array with a null test
case". It doesn't make sense.

>
> (Also, does anyone else find the whole 'lets change the unsafe rules
> in a minor compiler version, and require a weird attribute to avoid a
> warning' thing incredibly frustrating? I've read that it's not what
> the formal purpose of Rust editions is, but it _feels_ like this sort
> of change should be in an edition intuitively to me.)
>
> Anyway, I've got a few higher-priority non-Rust things to do by the
> end of the month, so I'm unlikely to have time to spin up a v8 myself
> for a few weeks. So if folks want to either send out an updated
> version or the series, or accept it as-is and send out a follow-up
> fix, I'm happy to review it, but otherwise it'll have to wait a little
> bit.
>
> Cheers,
> -- David