Message-ID: <CABVgOSmQVXAdRpZBCDo2G4at3-MNaX2xV565m_jVcyw0y7sDfg@mail.gmail.com>
Date: Tue, 18 Feb 2025 16:51:41 +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 Sat, 15 Feb 2025 at 21:01, Tamir Duberstein <tamird@...il.com> wrote:
>
> On Sat, Feb 15, 2025 at 4:03 AM David Gow <davidgow@...gle.com> wrote:
> >
> > 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.
>
> 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

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

Powered by blists - more mailing lists