On Sat, Feb 15, 2025 at 4:03 AM David Gow <david...@google.com> wrote:
>
> On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein <tam...@gmail.com> wrote:
> >
> > Very excited to see this progress.
> >
> > On Fri, Feb 14, 2025 at 2:41 AM David Gow <david...@google.com> wrote:
> > >
> > > From: José Expósito <jose.exposit...@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.exposit...@gmail.com>
> > > Co-developed-by: Matt Gilbride <mattgilbr...@google.com>
> > > Signed-off-by: Matt Gilbride <mattgilbr...@google.com>
> > > Co-developed-by: Miguel Ojeda <oj...@kernel.org>
> > > Signed-off-by: Miguel Ojeda <oj...@kernel.org>
> > > Co-developed-by: David Gow <david...@google.com>
> > > Signed-off-by: David Gow <david...@google.com>
> > > ---
> > >
> > > Changes since v5:
> > > https://lore.kernel.org/all/20241213081035.2069066-2-david...@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-david...@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-david...@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-david...@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-c80db349e...@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-3bb9ff44b...@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
