Re: [PATCH v3 04/15] kunit: Add documentation for warning backtrace suppression API
On Wed, 3 Apr 2024 at 21:19, Guenter Roeck wrote:
>
> Document API functions for suppressing warning backtraces.
>
> Tested-by: Linux Kernel Functional Testing
> Acked-by: Dan Carpenter
> Reviewed-by: Kees Cook
> Signed-off-by: Guenter Roeck
> ---
This looks good to me: thanks for adding the documentation!
If we add integration between this and the KUnit resource system,
we'll need to add that to this documentation.
I wonder if it would make sense to have an example where the
DEFINE_SUPPRESSED_WARNING() is global, e.g., in the test init/exit
functions. That might overcomplicate it a bit.
It also might be nice to document the individual macros with kerneldoc
comments. (Though, that could equally fit in patch #1).
Still, this is the most important bit, so I'm happy to have it as-is.
Reviewed-by: David Gow
Cheers,
-- David
> v2:
> - Rebased to v6.9-rc1
> - Added Tested-by:, Acked-by:, and Reviewed-by: tags
> v3:
> - Rebased to v6.9-rc2
>
> Documentation/dev-tools/kunit/usage.rst | 30 -
> 1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/dev-tools/kunit/usage.rst
> b/Documentation/dev-tools/kunit/usage.rst
> index 22955d56b379..8d3d36d4103d 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -157,6 +157,34 @@ Alternatively, one can take full control over the error
> message by using
> if (some_setup_function())
> KUNIT_FAIL(test, "Failed to setup thing for testing");
>
> +Suppressing warning backtraces
> +--
> +
> +Some unit tests trigger warning backtraces either intentionally or as side
> +effect. Such backtraces are normally undesirable since they distract from
> +the actual test and may result in the impression that there is a problem.
> +
> +Such backtraces can be suppressed. To suppress a backtrace in
> some_function(),
> +use the following code.
> +
> +.. code-block:: c
> +
> + static void some_test(struct kunit *test)
> + {
> + DEFINE_SUPPRESSED_WARNING(some_function);
> +
> + START_SUPPRESSED_WARNING(some_function);
> + trigger_backtrace();
> + END_SUPPRESSED_WARNING(some_function);
> + }
> +
> +SUPPRESSED_WARNING_COUNT() returns the number of suppressed backtraces. If
> the
> +suppressed backtrace was triggered on purpose, this can be used to check if
> +the backtrace was actually triggered.
> +
> +.. code-block:: c
> +
> + KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(some_function), 1);
>
> Test Suites
> ~~~
> @@ -857,4 +885,4 @@ For example:
> dev_managed_string = devm_kstrdup(fake_device, "Hello,
> World!");
>
> // Everything is cleaned up automatically when the test ends.
> - }
> \ No newline at end of file
> + }
> --
> 2.39.2
>
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH v3 03/15] kunit: Add test cases for backtrace warning suppression
On Wed, 3 Apr 2024 at 21:19, Guenter Roeck wrote:
>
> Add unit tests to verify that warning backtrace suppression works.
>
> If backtrace suppression does _not_ work, the unit tests will likely
> trigger unsuppressed backtraces, which should actually help to get
> the affected architectures / platforms fixed.
>
> Tested-by: Linux Kernel Functional Testing
> Acked-by: Dan Carpenter
> Reviewed-by: Kees Cook
> Signed-off-by: Guenter Roeck
> ---
There's a typo in the Makefile, which stops this from being built at
all. Otherwise, it seems good to me.
-- David
> v2:
> - Rebased to v6.9-rc1
> - Added Tested-by:, Acked-by:, and Reviewed-by: tags
> - Introduced KUNIT_SUPPRESS_BACKTRACE configuration option
> v3:
> - Rebased to v6.9-rc2
>
> lib/kunit/Makefile | 7 +-
> lib/kunit/backtrace-suppression-test.c | 104 +
> 2 files changed, 109 insertions(+), 2 deletions(-)
> create mode 100644 lib/kunit/backtrace-suppression-test.c
>
> diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
> index 545b57c3be48..3eee1bd0ce5e 100644
> --- a/lib/kunit/Makefile
> +++ b/lib/kunit/Makefile
> @@ -16,10 +16,13 @@ endif
>
> # KUnit 'hooks' and bug handling are built-in even when KUnit is built
> # as a module.
> -obj-y += hooks.o \
> - bug.o
> +obj-y += hooks.o
> +obj-$(CONFIG_KUNIT_SUPPRESS_BACKTRACE) += bug.o
>
> obj-$(CONFIG_KUNIT_TEST) +=kunit-test.o
> +ifeq ($(CCONFIG_KUNIT_SUPPRESS_BACKTRACE),y)
s/CCONFIG_/CONFIG_/ ?
> +obj-$(CONFIG_KUNIT_TEST) +=backtrace-suppression-test.o
> +endif
>
> # string-stream-test compiles built-in only.
> ifeq ($(CONFIG_KUNIT_TEST),y)
> diff --git a/lib/kunit/backtrace-suppression-test.c
> b/lib/kunit/backtrace-suppression-test.c
> new file mode 100644
> index ..47c619283802
> --- /dev/null
> +++ b/lib/kunit/backtrace-suppression-test.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit test for suppressing warning tracebacks
> + *
> + * Copyright (C) 2024, Guenter Roeck
> + * Author: Guenter Roeck
> + */
> +
> +#include
> +#include
> +
> +static void backtrace_suppression_test_warn_direct(struct kunit *test)
> +{
> + DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct);
> +
> + START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct);
> + WARN(1, "This backtrace should be suppressed");
> + END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct);
> +
> + KUNIT_EXPECT_EQ(test,
> SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_direct), 1);
> +}
> +
> +static void trigger_backtrace_warn(void)
> +{
> + WARN(1, "This backtrace should be suppressed");
> +}
> +
> +static void backtrace_suppression_test_warn_indirect(struct kunit *test)
> +{
> + DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn);
> +
> + START_SUPPRESSED_WARNING(trigger_backtrace_warn);
> + trigger_backtrace_warn();
> + END_SUPPRESSED_WARNING(trigger_backtrace_warn);
> +
> + KUNIT_EXPECT_EQ(test,
> SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn), 1);
> +}
> +
> +static void backtrace_suppression_test_warn_multi(struct kunit *test)
> +{
> + DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn);
> + DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi);
> +
> + START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi);
> + START_SUPPRESSED_WARNING(trigger_backtrace_warn);
> + WARN(1, "This backtrace should be suppressed");
> + trigger_backtrace_warn();
> + END_SUPPRESSED_WARNING(trigger_backtrace_warn);
> + END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi);
> +
> + KUNIT_EXPECT_EQ(test,
> SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_multi), 1);
> + KUNIT_EXPECT_EQ(test,
> SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn), 1);
> +}
> +
> +static void backtrace_suppression_test_warn_on_direct(struct kunit *test)
> +{
> + DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct);
> +
> + if (!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE) &&
> !IS_ENABLED(CONFIG_KALLSYMS))
> + kunit_skip(test, "requires CONFIG_DEBUG_BUGVERBOSE or
> CONFIG_KALLSYMS");
> +
> + START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct);
> + WARN_ON(1);
> + END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct);
> +
> + KUNIT_EXPECT_EQ(test,
> +
> SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_on_direct), 1);
> +}
> +
> +static void trigger_backtrace_warn_on(void)
> +{
> + WARN_ON(1);
> +}
> +
> +static void backtrace_suppression_test_warn_on_indirect(struct kunit *test)
> +{
> + DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn_on);
> +
> + if (!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE))
> + kunit_skip(test, "r
Re: [PATCH v3 02/15] kunit: bug: Count suppressed warning backtraces
On Wed, 3 Apr 2024 at 21:19, Guenter Roeck wrote:
>
> Count suppressed warning backtraces to enable code which suppresses
> warning backtraces to check if the expected backtrace(s) have been
> observed.
>
> Using atomics for the backtrace count resulted in build errors on some
> architectures due to include file recursion, so use a plain integer
> for now.
>
> Acked-by: Dan Carpenter
> Reviewed-by: Kees Cook
> Tested-by: Linux Kernel Functional Testing
> Signed-off-by: Guenter Roeck
> ---
Looks good to me, thanks.
Reviewed-by: David Gow
Cheers,
-- David
> v2:
> - Rebased to v6.9-rc1
> - Added Tested-by:, Acked-by:, and Reviewed-by: tags
> - Introduced KUNIT_SUPPRESS_BACKTRACE configuration option
> v3:
> - Rebased to v6.9-rc2
>
> include/kunit/bug.h | 7 ++-
> lib/kunit/bug.c | 4 +++-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/kunit/bug.h b/include/kunit/bug.h
> index bd0fe047572b..72e9fb23bbd5 100644
> --- a/include/kunit/bug.h
> +++ b/include/kunit/bug.h
> @@ -20,6 +20,7 @@
> struct __suppressed_warning {
> struct list_head node;
> const char *function;
> + int counter;
> };
>
> void __start_suppress_warning(struct __suppressed_warning *warning);
> @@ -28,7 +29,7 @@ bool __is_suppressed_warning(const char *function);
>
> #define DEFINE_SUPPRESSED_WARNING(func)\
> struct __suppressed_warning __kunit_suppress_##func = \
> - { .function = __stringify(func) }
> + { .function = __stringify(func), .counter = 0 }
>
> #define START_SUPPRESSED_WARNING(func) \
> __start_suppress_warning(&__kunit_suppress_##func)
> @@ -39,12 +40,16 @@ bool __is_suppressed_warning(const char *function);
> #define IS_SUPPRESSED_WARNING(func) \
> __is_suppressed_warning(func)
>
> +#define SUPPRESSED_WARNING_COUNT(func) \
> + (__kunit_suppress_##func.counter)
> +
> #else /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */
>
> #define DEFINE_SUPPRESSED_WARNING(func)
> #define START_SUPPRESSED_WARNING(func)
> #define END_SUPPRESSED_WARNING(func)
> #define IS_SUPPRESSED_WARNING(func) (false)
> +#define SUPPRESSED_WARNING_COUNT(func) (0)
>
> #endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */
> #endif /* __ASSEMBLY__ */
> diff --git a/lib/kunit/bug.c b/lib/kunit/bug.c
> index f93544d7a9d1..13b3d896c114 100644
> --- a/lib/kunit/bug.c
> +++ b/lib/kunit/bug.c
> @@ -32,8 +32,10 @@ bool __is_suppressed_warning(const char *function)
> return false;
>
> list_for_each_entry(warning, &suppressed_warnings, node) {
> - if (!strcmp(function, warning->function))
> + if (!strcmp(function, warning->function)) {
> + warning->counter++;
> return true;
> + }
> }
> return false;
> }
> --
> 2.39.2
>
> --
> You received this message because you are subscribed to the Google Groups
> "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kunit-dev+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kunit-dev/20240403131936.787234-3-linux%40roeck-us.net.
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH v3 01/15] bug/kunit: Core support for suppressing warning backtraces
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
>
Re: [PATCH 09/14] kunit: include debugfs header file
On Wed, 17 May 2023 at 21:12, Arnd Bergmann wrote: > > From: Arnd Bergmann > > An extra #include statement is needed to ensure the prototypes > for debugfs interfaces are visible, avoiding this warning: > > lib/kunit/debugfs.c:28:6: error: no previous prototype for > 'kunit_debugfs_cleanup' [-Werror=missing-prototypes] > lib/kunit/debugfs.c:33:6: error: no previous prototype for > 'kunit_debugfs_init' [-Werror=missing-prototypes] > lib/kunit/debugfs.c:102:6: error: no previous prototype for > 'kunit_debugfs_create_suite' [-Werror=missing-prototypes] > lib/kunit/debugfs.c:118:6: error: no previous prototype for > 'kunit_debugfs_destroy_suite' [-Werror=missing-prototypes] > > Signed-off-by: Arnd Bergmann > --- Nice catch, thanks. I'm fine with this going in via -mm, but if you'd prefer it to go via kselftest/kunit, let me know. Reviewed-by: David Gow Cheers, -- David > lib/kunit/debugfs.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c > index b08bb1fba106..22c5c496a68f 100644 > --- a/lib/kunit/debugfs.c > +++ b/lib/kunit/debugfs.c > @@ -10,6 +10,7 @@ > #include > > #include "string-stream.h" > +#include "debugfs.h" > > #define KUNIT_DEBUGFS_ROOT "kunit" > #define KUNIT_DEBUGFS_RESULTS "results" > -- > 2.39.2 > > -- > You received this message because you are subscribed to the Google Groups > "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to kunit-dev+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/kunit-dev/20230517131102.934196-10-arnd%40kernel.org. smime.p7s Description: S/MIME Cryptographic Signature
