Re: [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support

["Nicolás Antinori" <nico.antinori.7@gmail.com>]

Thank you for the feedback!

On Tue Sep 22, 2026 at 4:56 AM -03, David Gow wrote:
> Le 16/09/2026 à 03:33, Nicolás Antinori a écrit :
>> This patch series intends to implement two features for KUnit tests
>> written in Rust. The work is based on a TODO comment made in the
>> `bitmap.rs` module [1].
>> 
>
> Thanks very much for this series! It works fine here, but I think there 
> are a few other options for how this could be implemented, and it's 
> probably worth our at least considering them.
>
> In particular, we've already got code for suppressing warnings, and I'm 
> not sure whether it makes sense to unify all of the different attempts 
> to intercept panics / bugs / warnings of various kinds.
>
> That being said, Rust has unwinding and panic handlers as a core part of 
> the language, and the C side of the kernel doesn't. Combine that with 
> the fact that #[should_panic] is already standardised in Rust, and the 
> argument for a separate implementation is not totally silly either.
>
> Do you think that #[should_panic] should only trigger on a rust 
> panic!(), or on any kernel panic? I'm leaning towards the former, but if 
> the latter then we'd need to implement it in C and provide a C interface 
> to it.

When I sent the series I leaned towards the former too. But thinking
about it I believe there are situations where a kernel panic can be
originated from C code called by Rust, for example, this test case:

#[test]
#[should_panic]
fn rust_test_kunit_panic_in_kunit_test_bug() {
    unsafe { bindings::BUG() };
}

This kernel panic is not caught by the Rust's panic handler. In the
current version of my code, I catch that in
lib/kunit/test.c::kunit_run_case_catch_errors function. With that
modification there's no need of a Rust side panic handler (as it catches
Rust's panics too, since the panic hanlder executes a bindings::BUG()).

>
>> 1. Supporting `#[should_panic]` [2]:
>> 
>> KUnit tests in Rust follow the user-space syntax, but at the moment
>> `#[should_panic]` is not supported. The first patch of this series adds
>> support for the attribute (only in its basic form, `#[should_panic =
>> "message"]` is not supported, and I don't know if it makes sense to
>> support it)
>> 
>> The way it is supported is by having a separate `#[panic_handler]` when
>> `CONFIG_KUNIT` is enabled. When a test is marked with `#[should_panic]`,
>> a static value (KUNIT_SHOULD_PANIC = 0xDEAD7357) is assigned to the
>> kunit's `priv` field, since it is meant for saving arbitrary user data
>> [3]. At the moment, I did not find any place where Rust tests use that
>> field, so it should be safe to write it.
>
> I don't think the `priv` field is the optimal place to put this. I don't 
> think it's strictly a _problem_, particularly since Rust tests aren't 
> using it, but nominally `priv` is for test use, and I'd rather not use 
> it here (there may be future tests which want to use priv for something 
> else, particularly as a quick way of passing test state between C and Rust).
>
> For most of these sorts of things, I'd recommend using a KUnit 'named 
> resource', but alas, there aren't any Rust binding for these. That being 
> said, we've used named resources in C because they're setup at runtime 
> (which is how we've handled this in the past). That's useful if we want 
> to note that a particular line in the test wants to panic, but if we're 
> only concerned with whether a test as a whole panics, then this could be 
> static.

I did not know that you could test particular lines for panics! That
said, I believe the #[should_panic] attribute is meant to check if the
test panics as a whole.

If I had to test a particular line for panic that I'd write a new test,
but that's just how I'd do it :P.

>
> In that case, how about adding a new `rust_should_panic` field to 
> `struct kunit_attributes`. If you only care about whether a panic 
> occurs, this could just be a boolean, but it also could be a place to 
> store, for example, a string to support #[should_panic = "message"] if 
> you wish.

I could not find a way to retrieve the kunit_case struct from Rust. I
believe this is needed for implementing the check because the actual
panic message can only be retrieved from the PanicInfo [1] struct. 

I am sure this can be implemented (the first things that comes to mind
is having a C api that retrieves the current kunit_case struct, but I am
not sure if the kunit_case meant to be leaked outside the runner) but
I'd do it in another iteration if we find that it is useful.

>
> As an attribute, you could also then add it to lib/kunit/attributes.c 
> (probably with PRINT_NEVER, as I don't think we need it included in KTAP 
> output), which would, for example, allow us to filter tests by whether 
> or not they expect to panic.

Excellent! I'll do that!

>> ...
>> 2. Allow same test name with different #[cfg(...)]:
>> 
>> When testing `#[should_panic]` in `bitmap.rs` (check the last patch of
>> the series) I found that the test that was supposed to panic had the
>> same name as another one, but they were run on different configurations.
>> This caused the following compilation error:
>> 
>> ERROR:root:error[E0428]: the name `kunit_rust_wrapper_owned_bitmap_out_of_bounds` is defined multiple times
>>     --> ../rust/kernel/bitmap.rs:503:1
>>      |
>> 503 | #[macros::kunit_tests(rust_kernel_bitmap)]
>>      | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `kunit_rust_wrapper_owned_bitmap_out_of_bounds` redefined here
>>      |
>>      = note: `kunit_rust_wrapper_owned_bitmap_out_of_bounds` must be defined only once in the value namespace of this module
>>      = note: this error originates in the attribute macro `macros::kunit_tests` (in Nightly builds, run with -Z macro-backtrace for more info)
>> 
>> 
>> To fix this problem, I appended to the `kunit_rust_wrapper_*`
>> identifiers an 'stringified' version of the test's #[cfg(...)]
>> arguments. The purpose of this is that, if we have a test with the same
>> name and configuration, it would fail.
>> 
>> The configuration string was also appended to the tests names. This was
>> done to have a better test run report:
>> 
>> ...
>> [SKIPPED] owned_bitmap_out_of_bounds_cfg_not_config_rust_bitmap_hardened
>> [PASSED] owned_bitmap_out_of_bounds_cfg_config_rust_bitmap_hardened
>> ...
>> 
>> Otherwise we would have something like the following:
>> ...
>> [SKIPPED] owned_bitmap_out_of_bounds
>> [PASSED] owned_bitmap_out_of_bounds
>> ...
>> 
>> Regarding this:
>> - Does it makes sense to allow same test names with different cfgs?
>
> Yes-ish. I think it definitely makes sense for the same test to be 
> redefined with different cfgs, but I'd rather only one of those tests 
> then actually be compiled in (see below).
>
>> - Is it ok to 'stringify' the configuration so it can be distinguished
>>    in the report? Would you prefer something like `_case_1` `_case_2` ..
>>    instead?
>
> I don't _like_ this: my preference would be for us to keep the same 
> name, and just not emit a test_case for anything which should be 
> compiled out with cfg. Unfortunately, implementing that is a bit harder 
> than would be ideal: we need a way of evaluating the cfg() arguments in 
> a proc macro, I think. (Ultimately, because otherwise there's no way of 
> statically determining the length of the TEST_CASES array?)
>
> Unless you've got a good idea how to fix this, though, I'm happy to put 
> up with adding the configs to the name for now. Though if there's a nice 
> way to make the names shorter 
> (rust_test_kunit_parse_cfg_in_kunit_test_cfg_config_rust_kunit_selftest_equals_n 
> is definitely too long a test name, for instance), that'd be best.

I did not like it either but I could not find a way of evaluating the
correspondig cfgs and not including the ones that were not active in the
TEST_CASES array. I implemented the Gary's solution [2] (very neat
trick!) and it worked really well!

Again, thank you both for the feedback. I'll be sending a patch soon.

Best regards,
Nicolás

[1] https://github.com/Rust-for-Linux/linux/blob/93f51579e7df248780214094418f205253383cc5/rust/kernel/lib.rs#L179
[2] https://lore.kernel.org/rust-for-linux/DLLYGLVEA0R3.3D1733XFFTFPV@garyguo.net/