On Wed, 3 Apr 2024 at 21:19, Guenter Roeck wrote:
>
> Some unit tests intentionally trigger warning backtraces by passing
> bad parameters to API functions. Such unit tests typically check the
> return value from those calls, not the existence of the warning backtrace.
>
> Such intentionally generated warning backtraces are neither desirable
> nor useful for a number of reasons.
> - They can result in overlooked real problems.
> - A warning that suddenly starts to show up in unit tests needs to be
> investigated and has to be marked to be ignored, for example by
> adjusting filter scripts. Such filters are ad-hoc because there is
> no real standard format for warnings. On top of that, such filter
> scripts would require constant maintenance.
>
> One option to address problem would be to add messages such as "expected
> warning backtraces start / end here" to the kernel log. However, that
> would again require filter scripts, it might result in missing real
> problematic warning backtraces triggered while the test is running, and
> the irrelevant backtrace(s) would still clog the kernel log.
>
> Solve the problem by providing a means to identify and suppress specific
> warning backtraces while executing test code. Since the new functionality
> results in an image size increase of about 1% if CONFIG_KUNIT is enabled,
> provide configuration option KUNIT_SUPPRESS_BACKTRACE to be able to disable
> the new functionality. This option is by default enabled since almost all
> systems with CONFIG_KUNIT enabled will want to benefit from it.
>
> Cc: Dan Carpenter
> Cc: Daniel Diaz
> Cc: Naresh Kamboju
> Cc: Kees Cook
> Tested-by: Linux Kernel Functional Testing
> Acked-by: Dan Carpenter
> Reviewed-by: Kees Cook
> Signed-off-by: Guenter Roeck
> ---
Sorry it took so long to get to this.
I love the idea, we've needed this for a while.
There are some downsides to this being entirely based on the name of
the function which contains WARN(). Partly because there could be
several WARN()s within a function, and there'd be overlap, and partly
because the function name is never actually printed during a warning
(it may come from the stack trace, but that can be misleading with
inlined functions). I don't think either of these are showstoppers,
though, but it'd be nice to extend this in the future with (a) other
ways of identifying warnings, such as the format string, and (b) print
the function name in the report, if it's present. The function name is
probably a good middle ground, complexity-wise, though, so I'm happy
to have it thus far.
I also think we're missing some opportunities to integrate this
better with existing KUnit infrastructure, like the
action/resource/cleanup system. In particular, it'd be nice to have a
way of ensuring that suppressions won't get leaked if the test aborts
between START_SUPPRESSED_WARNING() and END_SUPPRESSED_WARNING(). It's
not difficult to use this as-is, but it'd be nice to have some
helpers, rather than having to, for instance:
KUNIT_DEFINE_ACTION_WRAPPER(kunit_stop_suppressing_warning,
__end_suppress_warning, struct __suppressed_warning *);
DEFINE_SUPPRESSED_WARNING(vfree);
START_SUPPRESSED_WARNING(vfree);
kunit_add_action(test, kunit_stop_suppressing_warning, (void
*)&__kunit_suppress_vfree);
(With the note that the DEFINE_SUPPRESSED_WARNING() will have to be
global, or put on the heap, lest it become a dangling pointer by the
time the suppression has stopped.)
Equally, do we want to make the
__{start,end,is}_suppress[ed]_warning() functions KUnit 'hooks'? This
would allow them to be used in modules which don't depend directly on
KUnit. I suspect it's not important in this case: but worth keeping in
mind in case we find a situation where we'd need to suppress a warning
elsewhere.
These are all things which could be added/changed in follow-up
patches, though, so I don't think they're blockers. Otherwise, this
looks good: perhaps the naming could be a bit more consistent with
other KUnit things, but that depends on how much we want this to be 'a
part of KUnit' versus an independent bit of functionality.
> v2:
> - Rebased to v6.9-rc1
> - Added Tested-by:, Acked-by:, and Reviewed-by: tags
> - Added CONFIG_KUNIT_SUPPRESS_BACKTRACE configuration option,
> enabled by default
> v3:
> - Rebased to v6.9-rc2
>
> include/asm-generic/bug.h | 16 +---
> include/kunit/bug.h | 51 +++
> include/kunit/test.h | 1 +
> include/linux/bug.h | 13 ++
> lib/bug.c | 51 ---
> lib/kunit/Kconfig | 9 +++
> lib/kunit/Makefile| 6 +++--
> lib/kunit/bug.c | 40 ++
> 8 files changed, 178 insertions(+), 9 deletions(-)
> create mode 100644 include/kunit/bug.h
> create mode 100644 lib/kunit/bug.c
>
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 6e794420bd39..c170b6477689 100644
