Message-ID: <CABVgOSkcDPwWEz=Uo0q+HXSeQT4a5yPg8vb4BMkpZ0yyDu4wQA@mail.gmail.com>
Date: Sat, 15 Feb 2025 17:03:35 +0800
From: David Gow <davidgow@...gle.com>
To: Tamir Duberstein <tamird@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, José Expósito <jose.exposito89@...il.com>, 
	Rae Moar <rmoar@...gle.com>, Boqun Feng <boqun.feng@...il.com>, 
	Alex Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>, 
	Benno Lossin <benno.lossin@...ton.me>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Alice Ryhl <aliceryhl@...gle.com>, Matt Gilbride <mattgilbride@...gle.com>, 
	Brendan Higgins <brendan.higgins@...ux.dev>, kunit-dev@...glegroups.com, 
	linux-kselftest@...r.kernel.org, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 1/3] rust: kunit: add KUnit case and suite macros

On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein <tamird@...il.com> wrote:
>
> Very excited to see this progress.
>
> On Fri, Feb 14, 2025 at 2:41 AM David Gow <davidgow@...gle.com> wrote:
> >
> > From: José Expósito <jose.exposito89@...il.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@...il.com>
> > Co-developed-by: Matt Gilbride <mattgilbride@...gle.com>
> > Signed-off-by: Matt Gilbride <mattgilbride@...gle.com>
> > Co-developed-by: Miguel Ojeda <ojeda@...nel.org>
> > Signed-off-by: Miguel Ojeda <ojeda@...nel.org>
> > Co-developed-by: David Gow <davidgow@...gle.com>
> > Signed-off-by: David Gow <davidgow@...gle.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

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5294 bytes)

Powered by blists - more mailing lists