| Message ID | 20250214074051.1619256-2-davidgow@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rust: kunit: Support KUnit tests with a user-space like syntax | expand |
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
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
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) }; + }; + }; +}