On Mon, 2026-05-04 at 10:39 +0200, Nam Cao wrote:
> Gabriele Monaco <[email protected]> writes:
> > +obj-$(CONFIG_RV_MONITORS_KUNIT_TEST) += rv_monitors_test.o
> > +# Stubbing rv_react triggers the error
> > +CFLAGS_rv_monitors_test.o += -Wno-suggest-attribute=format
>
> I try removing this flag, but my compiler does not produce any
> warning. Which warning did you see?
That's quite odd, I don't remember the exact GCC version that was showing the
warning, my current one does not enable it by default, you can however enable it
with:
CFLAGS_rv_monitors_test.o += -Wsuggest-attribute=format
Then you get:
In file included from kernel/trace/rv/rv_monitors_test.c:11:
kernel/trace/rv/rv_monitors_test.c: In function ‘rv_trigger_test_init’:
kernel/trace/rv/rv_monitors_test.c:68:42: error: argument 2 of
‘__kunit_activate_static_stub’ might be a candidate for a format attribute
[-Werror=suggest-attribute=format]
68 | kunit_activate_static_stub(test, rv_react, stub_rv_react);
| ^~~~~~~~
./include/kunit/static_stub.h:97:44: note: in definition of macro
‘kunit_activate_static_stub’
97 | __kunit_activate_static_stub(test, real_fn_addr,
replacement_addr); \
| ^~~~~~~~~~~~
kernel/trace/rv/rv_monitors_test.c:68:52: error: argument 3 of
‘__kunit_activate_static_stub’ might be a candidate for a format attribute
[-Werror=suggest-attribute=format]
68 | kunit_activate_static_stub(test, rv_react, stub_rv_react);
| ^~~~~~~~~~~~~
./include/kunit/static_stub.h:97:58: note: in definition of macro
‘kunit_activate_static_stub’
97 | __kunit_activate_static_stub(test, real_fn_addr,
replacement_addr); \
|
^~~~~~~~~~~~~~~~
> If it is not a hassle, I would prefer to address the warning in C code.
> Grep tells me the rest of the kernel does not use this, how do other
> subsystems not suffer from this warning?
When the compiler was caring about it, I tried all I could think of to avoid the
warning, but I didn't manage, some other places do disable this warning (just
not in the makefile but with pragmas):
$ git grep "suggest-attribute=format"
drivers/infiniband/hw/hfi1/trace_dbg.h:#pragma GCC diagnostic ignored
"-Wsuggest-attribute=format"
drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.h:#pragma GCC
diagnostic ignored "-Wsuggest-attribute=format"
drivers/net/wireless/broadcom/brcm80211/brcmsmac/brcms_trace_brcmsmac_msg.h:#pragma
GCC diagnostic ignored "-Wsuggest-attribute=format"
drivers/net/wireless/intel/iwlwifi/iwl-devtrace.c:#pragma GCC diagnostic
ignored "-Wsuggest-attribute=format"
include/trace/events/qla.h:#pragma GCC diagnostic ignored
"-Wsuggest-attribute=format"
kernel/panic.c:#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
lib/vsprintf.c:__diag_ignore(GCC, all, "-Wsuggest-attribute=format",
Here, the error comes from macro expansions in KUnit and I'm not sure there's
any practical way to solve it, that's why I resorted to disabling the warning
altogether.
I'm not sure whether a pragma would be cleaner though.
> > +void rv_test_opid(struct kunit *test)
> > +{
> > + struct rv_kunit_ctx *ctx = test->priv;
> > +
> > + da_prepare_test(test, &rv_this);
> > +
> > + /*
> > + * The handlers are called with an additional level of preemption,
> > + * ensure we start from 0 but apply it here to avoid warnings.
> > + */
> > + KUNIT_ASSERT_TRUE(test, preemptible());
> > + guard(preempt)();
> > +
> > + /* Wakeup with preemption and interrupts enabled */
> > + handle_sched_waking(NULL, NULL);
> > + RV_KUNIT_EXPECT_REACTION(test, ctx);
> > +
> > + /* Need resched with interrupts enabled */
> > + scoped_guard(preempt)
> > + handle_sched_need_resched(NULL, NULL, 0, TIF_NEED_RESCHED);
>
> From my understanding, this last one is testing that need_resched with
> interrupt enabled does not invoke the reactor? And if the monitor is
> broken and the reactor is invoked, we would have no test failure here,
> but instead we have test failure the next time
> RV_KUNIT_EXPECT_REACTION() is called. And if this is the last test, we
> would not see a failure?
Not really, I just forgot an RV_KUNIT_EXPECT_REACTION().
sched_need_resched just requires interrupts disabled, so I disable preemption
and show that seeing it with only interrupts enabled should react (touching
preemption is actually not required, but just to get a different test case).
> If so, should we perhaps have something like RV_KUNIT_EXPECT_NO_REACTION()
> here? So that if the monitor is broken and the reactor is called, then
> the kunit test will fail exactly where the failure is.
>
> Reading the patch further down below, we actually do have that
> macro! Shouldn't we use it here?
So I'm actually thinking of defining yet another macro that fundamentally does
RV_KUNIT_EXPECT_NO_REACTION()
handle_event()
RV_KUNIT_EXPECT_REACTION()
which would make sure the reaction happens exactly there, plus I'd add an
RV_KUNIT_EXPECT_NO_REACTION() in the cleanup sequence to ensure no unexpected
reaction occurred (or nobody forgot to expect a reaction like I did above).
> > +#ifdef CONFIG_RV_MON_SCO
> > +extern void rv_test_sco(struct kunit *test);
> > +#else
> > +static inline void rv_test_sco(struct kunit *test)
> > +{
> > + kunit_skip(test, "Monitor not enabled\n");
> > +}
> > +#endif
>
> Instead of these, I would prefer we have something like
>
> #define KUNIT_CASE_IF(test_name, enabled) \
> { .run_case = enabled ? test_name : some_stub, ... }
>
> and
>
> static struct kunit_case rv_trigger_test_cases[] = {
> KUNIT_CASE_IF(rv_test_sco, CONFIG_RV_MON_SCO),
> ...
> {}
> };
>
> or something like that. But no big deal.
Yeah that should be neater, but weren't you the one not liking macros? ;)
Thanks,
Gabriele
