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@go...
- 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@go...
- 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@go...
- 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()` elementsby 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-uns...
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