Gabriele Monaco <[email protected]> writes:
> 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.
Thanks for sharing the details. I can see now that disabling the warning
is the way to go.
>> > +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().
I added that missing RV_KUNIT_EXPECT_REACTION(), but I still see a test
failure:
[ 1.070721] # module: rv_monitors_test
[ 1.073512] 1..7
[ 1.077641] scsi 1:0:0:0: CD-ROM QEMU QEMU DVD-ROM 2.5+
PQ: 0 ANSI: 5
[ 1.078494] ok 1 rv_test_sco
[ 1.083256] ok 2 rv_test_sssw
[ 1.085783] ok 3 rv_test_sts # SKIP Monitor not enabled
[ 1.092365] ok 4 rv_test_opid
[ 1.093462] # rv_test_nomiss: EXPECTATION FAILED at
kernel/trace/rv/monitors/nomiss/nomiss.c:306
[ 1.093462] Expected ctx->reactions == ++ctx->expected, but
[ 1.093462] ctx->reactions == 2 (0x2)
[ 1.093462] ++ctx->expected == 1 (0x1)
[ 1.095699] not ok 5 rv_test_nomiss
[ 1.109418] ok 6 rv_test_pagefault # SKIP Monitor not enabled
[ 1.115146] ok 7 rv_test_sleep # SKIP Monitor not enabled
[ 1.118050] # rv_trigger: pass:3 fail:1 skip:3 total:7
[ 1.118053] # Totals: pass:3 fail:1 skip:3 total:7
[ 1.120622] not ok 1 rv_trigger
Any idea why?
> 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).
Sounds nice, go for it.
> Yeah that should be neater, but weren't you the one not liking macros? ;)
It's not black and white, I like whatever makes the code clean and easy
to read. Sometimes macros are nice, other times not so much. I have
spent hours reading the tracepoints' macros and they are still black
magic to me (but to be fair, macros are probably the best we can do for
that case). I hope we can rewrite those in Rust's generic one day.
Nam
