Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such 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. Support suppressing multiple backtraces while at the same time limiting changes to generic code to the absolute minimum. Architecture specific changes are kept at minimum by retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and CONFIG_KUNIT are enabled.
The first patch of the series introduces the necessary infrastructure. The second patch introduces support for counting suppressed backtraces. This capability is used in patch three to implement unit tests. Patch four documents the new API. The next two patches add support for suppressing backtraces in drm_rect and dev_addr_lists unit tests. These patches are intended to serve as examples for the use of the functionality introduced with this series. The remaining patches implement the necessary changes for all architectures with GENERIC_BUG support.
With CONFIG_KUNIT enabled, image size increase with this series applied is approximately 1%. The image size increase (and with it the functionality introduced by this series) can be avoided by disabling CONFIG_KUNIT_SUPPRESS_BACKTRACE.
This series is based on the RFC patch and subsequent discussion at https://patchwork.kernel.org/project/linux-kselftest/patch/02546e59-1afe-4b0... and offers a more comprehensive solution of the problem discussed there.
Design note: Function pointers are only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image size increases if CONFIG_KUNIT is disabled. There would be some benefits to adding those pointers all the time (reduced complexity, ability to display function names in BUG/WARNING messages). That change, if desired, can be made later.
Checkpatch note: Remaining checkpatch errors and warnings were deliberately ignored. Some are triggered by matching coding style or by comments interpreted as code, others by assembler macros which are disliked by checkpatch. Suggestions for improvements are welcome.
Changes since RFC: - Introduced CONFIG_KUNIT_SUPPRESS_BACKTRACE - Minor cleanups and bug fixes - Added support for all affected architectures - Added support for counting suppressed warnings - Added unit tests using those counters - Added patch to suppress warning backtraces in dev_addr_lists tests
Changes since v1: - Rebased to v6.9-rc1 - Added Tested-by:, Acked-by:, and Reviewed-by: tags [I retained those tags since there have been no functional changes] - Introduced KUNIT_SUPPRESS_BACKTRACE configuration option, enabled by default.
Changes since v2: - Rebased to v6.9-rc2 - Added comments to drm warning suppression explaining why it is needed. - Added patch to move conditional code in arch/sh/include/asm/bug.h to avoid kerneldoc warning - Added architecture maintainers to Cc: for architecture specific patches - No functional changes
Changes since v3: - Rebased to v6.14-rc6 - Dropped net: "kunit: Suppress lock warning noise at end of dev_addr_lists tests" since 3db3b62955cd6d73afde05a17d7e8e106695c3b9 - Added __kunit_ and KUNIT_ prefixes. - Tested on interessed architectures.
---- Guenter Roeck (14): bug/kunit: Core support for suppressing warning backtraces kunit: bug: Count suppressed warning backtraces kunit: Add test cases for backtrace warning suppression kunit: Add documentation for warning backtrace suppression API drm: Suppress intentional warning backtraces in scaling unit tests x86: Add support for suppressing warning backtraces arm64: Add support for suppressing warning backtraces loongarch: Add support for suppressing warning backtraces parisc: Add support for suppressing warning backtraces s390: Add support for suppressing warning backtraces sh: Add support for suppressing warning backtraces sh: Move defines needed for suppressing warning backtraces riscv: Add support for suppressing warning backtraces powerpc: Add support for suppressing warning backtraces
Documentation/dev-tools/kunit/usage.rst | 30 ++++++- arch/arm64/include/asm/asm-bug.h | 27 ++++-- arch/arm64/include/asm/bug.h | 8 +- arch/loongarch/include/asm/bug.h | 42 +++++++--- arch/parisc/include/asm/bug.h | 29 +++++-- arch/powerpc/include/asm/bug.h | 37 +++++++-- arch/riscv/include/asm/bug.h | 38 ++++++--- arch/s390/include/asm/bug.h | 17 +++- arch/sh/include/asm/bug.h | 28 ++++++- arch/x86/include/asm/bug.h | 21 +++-- drivers/gpu/drm/tests/drm_rect_test.c | 16 ++++ include/asm-generic/bug.h | 16 +++- include/kunit/bug.h | 56 +++++++++++++ include/kunit/test.h | 1 + include/linux/bug.h | 13 +++ lib/bug.c | 51 +++++++++++- lib/kunit/Kconfig | 9 ++ lib/kunit/Makefile | 7 +- lib/kunit/backtrace-suppression-test.c | 104 ++++++++++++++++++++++++ lib/kunit/bug.c | 42 ++++++++++ 20 files changed, 519 insertions(+), 73 deletions(-) create mode 100644 include/kunit/bug.h create mode 100644 lib/kunit/backtrace-suppression-test.c create mode 100644 lib/kunit/bug.c
From: Guenter Roeck linux@roeck-us.net
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 dan.carpenter@linaro.org Cc: Daniel Diaz daniel.diaz@linaro.org Cc: Naresh Kamboju naresh.kamboju@linaro.org Cc: Kees Cook keescook@chromium.org Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Reviewed-by: Kees Cook keescook@chromium.org Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com --- 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 387720933973..9194cf743ec3 100644 --- a/include/asm-generic/bug.h +++ b/include/asm-generic/bug.h @@ -18,6 +18,7 @@ #endif
#ifndef __ASSEMBLY__ +#include <kunit/bug.h> #include <linux/panic.h> #include <linux/printk.h>
@@ -39,8 +40,14 @@ struct bug_entry { #ifdef CONFIG_DEBUG_BUGVERBOSE #ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS const char *file; +#ifdef HAVE_BUG_FUNCTION + const char *function; +#endif #else signed int file_disp; +#ifdef HAVE_BUG_FUNCTION + signed int function_disp; +#endif #endif unsigned short line; #endif @@ -96,15 +103,18 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #define __WARN() __WARN_printf(TAINT_WARN, NULL) #define __WARN_printf(taint, arg...) do { \ instrumentation_begin(); \ - warn_slowpath_fmt(__FILE__, __LINE__, taint, arg); \ + if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ + warn_slowpath_fmt(__FILE__, __LINE__, taint, arg);\ instrumentation_end(); \ } while (0) #else #define __WARN() __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN)) #define __WARN_printf(taint, arg...) do { \ instrumentation_begin(); \ - __warn_printk(arg); \ - __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\ + if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \ + __warn_printk(arg); \ + __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\ + } \ instrumentation_end(); \ } while (0) #define WARN_ON_ONCE(condition) ({ \ diff --git a/include/kunit/bug.h b/include/kunit/bug.h new file mode 100644 index 000000000000..0a8e62c1fcaf --- /dev/null +++ b/include/kunit/bug.h @@ -0,0 +1,51 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * KUnit helpers for backtrace suppression + * + * Copyright (c) 2024 Guenter Roeck linux@roeck-us.net + */ + +#ifndef _KUNIT_BUG_H +#define _KUNIT_BUG_H + +#ifndef __ASSEMBLY__ + +#include <linux/kconfig.h> + +#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE + +#include <linux/stringify.h> +#include <linux/types.h> + +struct __suppressed_warning { + struct list_head node; + const char *function; +}; + +void __kunit_start_suppress_warning(struct __suppressed_warning *warning); +void __kunit_end_suppress_warning(struct __suppressed_warning *warning); +bool __kunit_is_suppressed_warning(const char *function); + +#define DEFINE_SUPPRESSED_WARNING(func) \ + struct __suppressed_warning __kunit_suppress_##func = \ + { .function = __stringify(func) } + +#define KUNIT_START_SUPPRESSED_WARNING(func) \ + __kunit_start_suppress_warning(&__kunit_suppress_##func) + +#define KUNIT_END_SUPPRESSED_WARNING(func) \ + __kunit_end_suppress_warning(&__kunit_suppress_##func) + +#define KUNIT_IS_SUPPRESSED_WARNING(func) \ + __kunit_is_suppressed_warning(func) + +#else /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */ + +#define DEFINE_SUPPRESSED_WARNING(func) +#define KUNIT_START_SUPPRESSED_WARNING(func) +#define KUNIT_END_SUPPRESSED_WARNING(func) +#define KUNIT_IS_SUPPRESSED_WARNING(func) (false) + +#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */ +#endif /* __ASSEMBLY__ */ +#endif /* _KUNIT_BUG_H */ diff --git a/include/kunit/test.h b/include/kunit/test.h index 58dbab60f853..cad32d174872 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -10,6 +10,7 @@ #define _KUNIT_TEST_H
#include <kunit/assert.h> +#include <kunit/bug.h> #include <kunit/try-catch.h>
#include <linux/args.h> diff --git a/include/linux/bug.h b/include/linux/bug.h index a9948a9f1093..e42f20cf8830 100644 --- a/include/linux/bug.h +++ b/include/linux/bug.h @@ -36,6 +36,9 @@ static inline int is_warning_bug(const struct bug_entry *bug) return bug->flags & BUGFLAG_WARNING; }
+void bug_get_file_function_line(struct bug_entry *bug, const char **file, + const char **function, unsigned int *line); + void bug_get_file_line(struct bug_entry *bug, const char **file, unsigned int *line);
@@ -62,6 +65,16 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr, }
struct bug_entry; +static inline void bug_get_file_function_line(struct bug_entry *bug, + const char **file, + const char **function, + unsigned int *line) +{ + *file = NULL; + *function = NULL; + *line = 0; +} + static inline void bug_get_file_line(struct bug_entry *bug, const char **file, unsigned int *line) { diff --git a/lib/bug.c b/lib/bug.c index e0ff21989990..5eb2ee66916f 100644 --- a/lib/bug.c +++ b/lib/bug.c @@ -26,6 +26,14 @@ when CONFIG_DEBUG_BUGVERBOSE is not enabled, so you must generate the values accordingly.
+ 2a.Optionally implement support for the "function" entry in struct + bug_entry. This entry must point to the name of the function triggering + the warning or bug trap (normally __func__). This is only needed if + both CONFIG_DEBUG_BUGVERBOSE and CONFIG_KUNIT_SUPPRESS_BACKTRACE are + enabled and if the architecture wants to implement support for suppressing + warning backtraces. The architecture must define HAVE_BUG_FUNCTION if it + adds pointers to function names to struct bug_entry. + 3. Implement the trap - In the illegal instruction trap handler (typically), verify that the fault was in kernel mode, and call report_bug() @@ -127,14 +135,21 @@ static inline struct bug_entry *module_find_bug(unsigned long bugaddr) } #endif
-void bug_get_file_line(struct bug_entry *bug, const char **file, - unsigned int *line) +void bug_get_file_function_line(struct bug_entry *bug, const char **file, + const char **function, unsigned int *line) { + *function = NULL; #ifdef CONFIG_DEBUG_BUGVERBOSE #ifdef CONFIG_GENERIC_BUG_RELATIVE_POINTERS *file = (const char *)&bug->file_disp + bug->file_disp; +#ifdef HAVE_BUG_FUNCTION + *function = (const char *)&bug->function_disp + bug->function_disp; +#endif #else *file = bug->file; +#ifdef HAVE_BUG_FUNCTION + *function = bug->function; +#endif #endif *line = bug->line; #else @@ -143,6 +158,13 @@ void bug_get_file_line(struct bug_entry *bug, const char **file, #endif }
+void bug_get_file_line(struct bug_entry *bug, const char **file, unsigned int *line) +{ + const char *function; + + bug_get_file_function_line(bug, file, &function, line); +} + struct bug_entry *find_bug(unsigned long bugaddr) { struct bug_entry *bug; @@ -157,8 +179,9 @@ struct bug_entry *find_bug(unsigned long bugaddr) static enum bug_trap_type __report_bug(unsigned long bugaddr, struct pt_regs *regs) { struct bug_entry *bug; - const char *file; + const char *file, *function; unsigned line, warning, once, done; + char __maybe_unused sym[KSYM_SYMBOL_LEN];
if (!is_valid_bugaddr(bugaddr)) return BUG_TRAP_TYPE_NONE; @@ -169,12 +192,32 @@ static enum bug_trap_type __report_bug(unsigned long bugaddr, struct pt_regs *re
disable_trace_on_warning();
- bug_get_file_line(bug, &file, &line); + bug_get_file_function_line(bug, &file, &function, &line); +#if defined(CONFIG_KUNIT_SUPPRESS_BACKTRACE) && defined(CONFIG_KALLSYMS) + if (!function) { + /* + * This will be seen if report_bug is called on an architecture + * with no architecture-specific support for suppressing warning + * backtraces, if CONFIG_DEBUG_BUGVERBOSE is not enabled, or if + * the calling code is from assembler which does not record a + * function name. Extracting the function name from the bug + * address is less than perfect since compiler optimization may + * result in 'bugaddr' pointing to a function which does not + * actually trigger the warning, but it is better than no + * suppression at all. + */ + sprint_symbol_no_offset(sym, bugaddr); + function = sym; + } +#endif /* defined(CONFIG_KUNIT_SUPPRESS_BACKTRACE) && defined(CONFIG_KALLSYMS) */
warning = (bug->flags & BUGFLAG_WARNING) != 0; once = (bug->flags & BUGFLAG_ONCE) != 0; done = (bug->flags & BUGFLAG_DONE) != 0;
+ if (warning && KUNIT_IS_SUPPRESSED_WARNING(function)) + return BUG_TRAP_TYPE_WARN; + if (warning && once) { if (done) return BUG_TRAP_TYPE_WARN; diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig index a97897edd964..201402f0ab49 100644 --- a/lib/kunit/Kconfig +++ b/lib/kunit/Kconfig @@ -15,6 +15,15 @@ menuconfig KUNIT
if KUNIT
+config KUNIT_SUPPRESS_BACKTRACE + bool "KUnit - Enable backtrace suppression" + default y + help + Enable backtrace suppression for KUnit. If enabled, backtraces + generated intentionally by KUnit tests are suppressed. Disable + to reduce kernel image size if image size is more important than + suppression of backtraces generated by KUnit tests. + config KUNIT_DEBUGFS bool "KUnit - Enable /sys/kernel/debug/kunit debugfs representation" if !KUNIT_ALL_TESTS default KUNIT_ALL_TESTS diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile index 5aa51978e456..3195e861d63c 100644 --- a/lib/kunit/Makefile +++ b/lib/kunit/Makefile @@ -16,8 +16,10 @@ ifeq ($(CONFIG_KUNIT_DEBUGFS),y) kunit-objs += debugfs.o endif
-# KUnit 'hooks' are built-in even when KUnit is built as a module. -obj-y += hooks.o +# KUnit 'hooks' and bug handling are built-in even when KUnit is built +# as a module. +obj-y += hooks.o \ + bug.o
obj-$(CONFIG_KUNIT_TEST) += kunit-test.o obj-$(CONFIG_KUNIT_TEST) += platform-test.o diff --git a/lib/kunit/bug.c b/lib/kunit/bug.c new file mode 100644 index 000000000000..351f9a595a71 --- /dev/null +++ b/lib/kunit/bug.c @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit helpers for backtrace suppression + * + * Copyright (c) 2024 Guenter Roeck linux@roeck-us.net + */ + +#include <kunit/bug.h> +#include <linux/export.h> +#include <linux/list.h> +#include <linux/string.h> + +static LIST_HEAD(suppressed_warnings); + +void __kunit_start_suppress_warning(struct __suppressed_warning *warning) +{ + list_add(&warning->node, &suppressed_warnings); +} +EXPORT_SYMBOL_GPL(__kunit_start_suppress_warning); + +void __kunit_end_suppress_warning(struct __suppressed_warning *warning) +{ + list_del(&warning->node); +} +EXPORT_SYMBOL_GPL(__kunit_end_suppress_warning); + +bool __kunit_is_suppressed_warning(const char *function) +{ + struct __suppressed_warning *warning; + + if (!function) + return false; + + list_for_each_entry(warning, &suppressed_warnings, node) { + if (!strcmp(function, warning->function)) + return true; + } + return false; +} +EXPORT_SYMBOL_GPL(__kunit_is_suppressed_warning);
On Thu, 13 Mar 2025 at 19:44, Alessandro Carminati acarmina@redhat.com wrote:
From: Guenter Roeck linux@roeck-us.net
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 dan.carpenter@linaro.org Cc: Daniel Diaz daniel.diaz@linaro.org Cc: Naresh Kamboju naresh.kamboju@linaro.org Cc: Kees Cook keescook@chromium.org Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Reviewed-by: Kees Cook keescook@chromium.org Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com
Thanks Guenter & Alessandro: I'm very happy with this.
Reviewed-by: David Gow davidgow@google.com
-- David
From: Guenter Roeck linux@roeck-us.net
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 dan.carpenter@linaro.org Reviewed-by: Kees Cook keescook@chromium.org Tested-by: Linux Kernel Functional Testing lkft@linaro.org Signed-off-by: Guenter Roeck linux@roeck-us.net Reviewed-by: David Gow davidgow@google.com Signed-off-by: Alessandro Carminati acarmina@redhat.com --- 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 0a8e62c1fcaf..44efa7d5c902 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 __kunit_start_suppress_warning(struct __suppressed_warning *warning); @@ -28,7 +29,7 @@ bool __kunit_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 KUNIT_START_SUPPRESSED_WARNING(func) \ __kunit_start_suppress_warning(&__kunit_suppress_##func) @@ -39,12 +40,16 @@ bool __kunit_is_suppressed_warning(const char *function); #define KUNIT_IS_SUPPRESSED_WARNING(func) \ __kunit_is_suppressed_warning(func)
+#define SUPPRESSED_WARNING_COUNT(func) \ + (__kunit_suppress_##func.counter) + #else /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */
#define DEFINE_SUPPRESSED_WARNING(func) #define KUNIT_START_SUPPRESSED_WARNING(func) #define KUNIT_END_SUPPRESSED_WARNING(func) #define KUNIT_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 351f9a595a71..84c05b1a9e8b 100644 --- a/lib/kunit/bug.c +++ b/lib/kunit/bug.c @@ -32,8 +32,10 @@ bool __kunit_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; }
On Thu, 13 Mar 2025 at 19:44, Alessandro Carminati acarmina@redhat.com wrote:
From: Guenter Roeck linux@roeck-us.net
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 dan.carpenter@linaro.org Reviewed-by: Kees Cook keescook@chromium.org Tested-by: Linux Kernel Functional Testing lkft@linaro.org Signed-off-by: Guenter Roeck linux@roeck-us.net Reviewed-by: David Gow davidgow@google.com Signed-off-by: Alessandro Carminati acarmina@redhat.com
Looks good. I'd definitely prefer the atomics to work one day, but I doubt we're likely to have backtraces from multiple threads happening in a KUnit test, so it's definitely not urgent.
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
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 0a8e62c1fcaf..44efa7d5c902 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 __kunit_start_suppress_warning(struct __suppressed_warning *warning); @@ -28,7 +29,7 @@ bool __kunit_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 KUNIT_START_SUPPRESSED_WARNING(func) \ __kunit_start_suppress_warning(&__kunit_suppress_##func) @@ -39,12 +40,16 @@ bool __kunit_is_suppressed_warning(const char *function); #define KUNIT_IS_SUPPRESSED_WARNING(func) \ __kunit_is_suppressed_warning(func)
+#define SUPPRESSED_WARNING_COUNT(func) \
(__kunit_suppress_##func.counter)
#else /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */
#define DEFINE_SUPPRESSED_WARNING(func) #define KUNIT_START_SUPPRESSED_WARNING(func) #define KUNIT_END_SUPPRESSED_WARNING(func) #define KUNIT_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 351f9a595a71..84c05b1a9e8b 100644 --- a/lib/kunit/bug.c +++ b/lib/kunit/bug.c @@ -32,8 +32,10 @@ bool __kunit_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.34.1
From: Guenter Roeck linux@roeck-us.net
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 lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Reviewed-by: Kees Cook keescook@chromium.org Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com --- 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 3195e861d63c..539a044a9f12 100644 --- a/lib/kunit/Makefile +++ b/lib/kunit/Makefile @@ -18,11 +18,14 @@ 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 obj-$(CONFIG_KUNIT_TEST) += platform-test.o +ifeq ($(CONFIG_KUNIT_SUPPRESS_BACKTRACE),y) +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 000000000000..8b4125af2481 --- /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 linux@roeck-us.net + */ + +#include <kunit/test.h> +#include <linux/bug.h> + +static void backtrace_suppression_test_warn_direct(struct kunit *test) +{ + DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct); + + KUNIT_START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct); + WARN(1, "This backtrace should be suppressed"); + KUNIT_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); + + KUNIT_START_SUPPRESSED_WARNING(trigger_backtrace_warn); + trigger_backtrace_warn(); + KUNIT_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); + + KUNIT_START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi); + KUNIT_START_SUPPRESSED_WARNING(trigger_backtrace_warn); + WARN(1, "This backtrace should be suppressed"); + trigger_backtrace_warn(); + KUNIT_END_SUPPRESSED_WARNING(trigger_backtrace_warn); + KUNIT_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"); + + KUNIT_START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct); + WARN_ON(1); + KUNIT_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, "requires CONFIG_DEBUG_BUGVERBOSE"); + + KUNIT_START_SUPPRESSED_WARNING(trigger_backtrace_warn_on); + trigger_backtrace_warn_on(); + KUNIT_END_SUPPRESSED_WARNING(trigger_backtrace_warn_on); + + KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn_on), 1); +} + +static struct kunit_case backtrace_suppression_test_cases[] = { + KUNIT_CASE(backtrace_suppression_test_warn_direct), + KUNIT_CASE(backtrace_suppression_test_warn_indirect), + KUNIT_CASE(backtrace_suppression_test_warn_multi), + KUNIT_CASE(backtrace_suppression_test_warn_on_direct), + KUNIT_CASE(backtrace_suppression_test_warn_on_indirect), + {} +}; + +static struct kunit_suite backtrace_suppression_test_suite = { + .name = "backtrace-suppression-test", + .test_cases = backtrace_suppression_test_cases, +}; +kunit_test_suites(&backtrace_suppression_test_suite); + +MODULE_LICENSE("GPL");
On Thu, 13 Mar 2025 at 19:44, Alessandro Carminati acarmina@redhat.com wrote:
From: Guenter Roeck linux@roeck-us.net
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 lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Reviewed-by: Kees Cook keescook@chromium.org Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com
Always nice to have tests. :-)
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
From: Guenter Roeck linux@roeck-us.net
Document API functions for suppressing warning backtraces.
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Reviewed-by: Kees Cook keescook@chromium.org Signed-off-by: Guenter Roeck linux@roeck-us.net Reviewed-by: David Gow davidgow@google.com Signed-off-by: Alessandro Carminati acarmina@redhat.com --- 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..b2f1e56d53b4 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); + + KUNIT_START_SUPPRESSED_WARNING(some_function); + trigger_backtrace(); + KUNIT_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 + }
From: Guenter Roeck linux@roeck-us.net
The drm_test_rect_calc_hscale and drm_test_rect_calc_vscale unit tests intentionally trigger warning backtraces by providing bad parameters to the tested functions. What is tested is the return value, not the existence of a warning backtrace. Suppress the backtraces to avoid clogging the kernel log and distraction from real problems.
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Acked-by: Maíra Canal mcanal@igalia.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: David Airlie airlied@gmail.com Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com --- drivers/gpu/drm/tests/drm_rect_test.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/tests/drm_rect_test.c b/drivers/gpu/drm/tests/drm_rect_test.c index 17e1f34b7610..e8d707b4a101 100644 --- a/drivers/gpu/drm/tests/drm_rect_test.c +++ b/drivers/gpu/drm/tests/drm_rect_test.c @@ -406,22 +406,38 @@ KUNIT_ARRAY_PARAM(drm_rect_scale, drm_rect_scale_cases, drm_rect_scale_case_desc
static void drm_test_rect_calc_hscale(struct kunit *test) { + DEFINE_SUPPRESSED_WARNING(drm_calc_scale); const struct drm_rect_scale_case *params = test->param_value; int scaling_factor;
+ /* + * drm_rect_calc_hscale() generates a warning backtrace whenever bad + * parameters are passed to it. This affects all unit tests with an + * error code in expected_scaling_factor. + */ + KUNIT_START_SUPPRESSED_WARNING(drm_calc_scale); scaling_factor = drm_rect_calc_hscale(¶ms->src, ¶ms->dst, params->min_range, params->max_range); + KUNIT_END_SUPPRESSED_WARNING(drm_calc_scale);
KUNIT_EXPECT_EQ(test, scaling_factor, params->expected_scaling_factor); }
static void drm_test_rect_calc_vscale(struct kunit *test) { + DEFINE_SUPPRESSED_WARNING(drm_calc_scale); const struct drm_rect_scale_case *params = test->param_value; int scaling_factor;
+ /* + * drm_rect_calc_vscale() generates a warning backtrace whenever bad + * parameters are passed to it. This affects all unit tests with an + * error code in expected_scaling_factor. + */ + KUNIT_START_SUPPRESSED_WARNING(drm_calc_scale); scaling_factor = drm_rect_calc_vscale(¶ms->src, ¶ms->dst, params->min_range, params->max_range); + KUNIT_END_SUPPRESSED_WARNING(drm_calc_scale);
KUNIT_EXPECT_EQ(test, scaling_factor, params->expected_scaling_factor); }
From: Guenter Roeck linux@roeck-us.net
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Cc: Thomas Gleixner tglx@linutronix.de Cc: Ingo Molnar mingo@redhat.com Cc: Borislav Petkov bp@alien8.de Cc: Dave Hansen dave.hansen@linux.intel.com Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com --- arch/x86/include/asm/bug.h | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h index e85ac0c7c039..f6e13fc675ab 100644 --- a/arch/x86/include/asm/bug.h +++ b/arch/x86/include/asm/bug.h @@ -35,18 +35,28 @@
#ifdef CONFIG_DEBUG_BUGVERBOSE
+#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR __BUG_REL(%c1) +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC_PTR +# define __BUG_FUNC NULL +#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */ + #define _BUG_FLAGS(ins, flags, extra) \ do { \ asm_inline volatile("1:\t" ins "\n" \ ".pushsection __bug_table,"aw"\n" \ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \ "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \ - "\t.word %c1" "\t# bug_entry::line\n" \ - "\t.word %c2" "\t# bug_entry::flags\n" \ - "\t.org 2b+%c3\n" \ + "\t" __BUG_FUNC_PTR "\t# bug_entry::function\n" \ + "\t.word %c2" "\t# bug_entry::line\n" \ + "\t.word %c3" "\t# bug_entry::flags\n" \ + "\t.org 2b+%c4\n" \ ".popsection\n" \ extra \ - : : "i" (__FILE__), "i" (__LINE__), \ + : : "i" (__FILE__), "i" (__BUG_FUNC), "i" (__LINE__),\ "i" (flags), \ "i" (sizeof(struct bug_entry))); \ } while (0) @@ -92,7 +102,8 @@ do { \ do { \ __auto_type __flags = BUGFLAG_WARNING|(flags); \ instrumentation_begin(); \ - _BUG_FLAGS(ASM_UD2, __flags, ANNOTATE_REACHABLE(1b)); \ + if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ + _BUG_FLAGS(ASM_UD2, __flags, ANNOTATE_REACHABLE(1b)); \ instrumentation_end(); \ } while (0)
On Thu, 13 Mar 2025 at 19:44, Alessandro Carminati acarmina@redhat.com wrote:
From: Guenter Roeck linux@roeck-us.net
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Cc: Thomas Gleixner tglx@linutronix.de Cc: Ingo Molnar mingo@redhat.com Cc: Borislav Petkov bp@alien8.de Cc: Dave Hansen dave.hansen@linux.intel.com Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
arch/x86/include/asm/bug.h | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h index e85ac0c7c039..f6e13fc675ab 100644 --- a/arch/x86/include/asm/bug.h +++ b/arch/x86/include/asm/bug.h @@ -35,18 +35,28 @@
#ifdef CONFIG_DEBUG_BUGVERBOSE
+#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR __BUG_REL(%c1) +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC_PTR +# define __BUG_FUNC NULL +#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */
#define _BUG_FLAGS(ins, flags, extra) \ do { \ asm_inline volatile("1:\t" ins "\n" \ ".pushsection __bug_table,"aw"\n" \ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \ "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \
"\t.word %c1" "\t# bug_entry::line\n" \
"\t.word %c2" "\t# bug_entry::flags\n" \
"\t.org 2b+%c3\n" \
"\t" __BUG_FUNC_PTR "\t# bug_entry::function\n" \
"\t.word %c2" "\t# bug_entry::line\n" \
"\t.word %c3" "\t# bug_entry::flags\n" \
"\t.org 2b+%c4\n" \ ".popsection\n" \ extra \
: : "i" (__FILE__), "i" (__LINE__), \
: : "i" (__FILE__), "i" (__BUG_FUNC), "i" (__LINE__),\ "i" (flags), \ "i" (sizeof(struct bug_entry))); \
} while (0) @@ -92,7 +102,8 @@ do { \ do { \ __auto_type __flags = BUGFLAG_WARNING|(flags); \ instrumentation_begin(); \
_BUG_FLAGS(ASM_UD2, __flags, ANNOTATE_REACHABLE(1b)); \
if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) \
_BUG_FLAGS(ASM_UD2, __flags, ANNOTATE_REACHABLE(1b)); \ instrumentation_end(); \
} while (0)
-- 2.34.1
On Thu, Mar 13, 2025 at 11:43:21AM +0000, Alessandro Carminati wrote:
From: Guenter Roeck linux@roeck-us.net
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Cc: Thomas Gleixner tglx@linutronix.de Cc: Ingo Molnar mingo@redhat.com Cc: Borislav Petkov bp@alien8.de Cc: Dave Hansen dave.hansen@linux.intel.com Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com
arch/x86/include/asm/bug.h | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h index e85ac0c7c039..f6e13fc675ab 100644 --- a/arch/x86/include/asm/bug.h +++ b/arch/x86/include/asm/bug.h @@ -35,18 +35,28 @@ #ifdef CONFIG_DEBUG_BUGVERBOSE +#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR __BUG_REL(%c1) +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC_PTR +# define __BUG_FUNC NULL +#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */
#define _BUG_FLAGS(ins, flags, extra) \ do { \ asm_inline volatile("1:\t" ins "\n" \ ".pushsection __bug_table,"aw"\n" \ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \ "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \
"\t.word %c1" "\t# bug_entry::line\n" \
"\t.word %c2" "\t# bug_entry::flags\n" \
"\t.org 2b+%c3\n" \
"\t" __BUG_FUNC_PTR "\t# bug_entry::function\n" \
"\t.word %c2" "\t# bug_entry::line\n" \
"\t.word %c3" "\t# bug_entry::flags\n" \
"\t.org 2b+%c4\n" \ ".popsection\n" \ extra \
: : "i" (__FILE__), "i" (__LINE__), \
: : "i" (__FILE__), "i" (__BUG_FUNC), "i" (__LINE__),\ "i" (flags), \ "i" (sizeof(struct bug_entry))); \
} while (0) @@ -92,7 +102,8 @@ do { \ do { \ __auto_type __flags = BUGFLAG_WARNING|(flags); \ instrumentation_begin(); \
- _BUG_FLAGS(ASM_UD2, __flags, ANNOTATE_REACHABLE(1b)); \
- if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) \
instrumentation_end(); \_BUG_FLAGS(ASM_UD2, __flags, ANNOTATE_REACHABLE(1b)); \
} while (0)
NAK, this grows the BUG site for now appreciable reason.
On 4/1/25 10:08, Peter Zijlstra wrote:
On Thu, Mar 13, 2025 at 11:43:21AM +0000, Alessandro Carminati wrote:
From: Guenter Roeck linux@roeck-us.net
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Cc: Thomas Gleixner tglx@linutronix.de Cc: Ingo Molnar mingo@redhat.com Cc: Borislav Petkov bp@alien8.de Cc: Dave Hansen dave.hansen@linux.intel.com Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com
arch/x86/include/asm/bug.h | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h index e85ac0c7c039..f6e13fc675ab 100644 --- a/arch/x86/include/asm/bug.h +++ b/arch/x86/include/asm/bug.h @@ -35,18 +35,28 @@ #ifdef CONFIG_DEBUG_BUGVERBOSE +#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR __BUG_REL(%c1) +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC_PTR +# define __BUG_FUNC NULL +#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */
- #define _BUG_FLAGS(ins, flags, extra) \ do { \ asm_inline volatile("1:\t" ins "\n" \ ".pushsection __bug_table,"aw"\n" \ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \ "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \
"\t.word %c1" "\t# bug_entry::line\n" \
"\t.word %c2" "\t# bug_entry::flags\n" \
"\t.org 2b+%c3\n" \
"\t" __BUG_FUNC_PTR "\t# bug_entry::function\n" \
"\t.word %c2" "\t# bug_entry::line\n" \
"\t.word %c3" "\t# bug_entry::flags\n" \
"\t.org 2b+%c4\n" \ ".popsection\n" \ extra \
: : "i" (__FILE__), "i" (__LINE__), \
} while (0): : "i" (__FILE__), "i" (__BUG_FUNC), "i" (__LINE__),\ "i" (flags), \ "i" (sizeof(struct bug_entry))); \
@@ -92,7 +102,8 @@ do { \ do { \ __auto_type __flags = BUGFLAG_WARNING|(flags); \ instrumentation_begin(); \
- _BUG_FLAGS(ASM_UD2, __flags, ANNOTATE_REACHABLE(1b)); \
- if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) \
instrumentation_end(); \ } while (0)_BUG_FLAGS(ASM_UD2, __flags, ANNOTATE_REACHABLE(1b)); \
NAK, this grows the BUG site for now appreciable reason.
Only if CONFIG_KUNIT_SUPPRESS_BACKTRACE is enabled. Why does that warrant a NACK ?
Guenter
On Tue, Apr 01, 2025 at 10:53:46AM -0700, Guenter Roeck wrote:
On 4/1/25 10:08, Peter Zijlstra wrote:
- if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) \
instrumentation_end(); \ } while (0)_BUG_FLAGS(ASM_UD2, __flags, ANNOTATE_REACHABLE(1b)); \
NAK, this grows the BUG site for now appreciable reason.
Only if CONFIG_KUNIT_SUPPRESS_BACKTRACE is enabled. Why does that warrant a NACK ?
I agree with Peter, this bloats the code around thousands of UD2 sites.
It would be much better to do the checking after the exception. In fact it looks like you're already doing that in report_bug()?
if (warning && KUNIT_IS_SUPPRESSED_WARNING(function)) return BUG_TRAP_TYPE_WARN;
Why check it twice?
On Tue, Apr 01, 2025 at 10:53:46AM -0700, Guenter Roeck wrote:
@@ -92,7 +102,8 @@ do { \ do { \ __auto_type __flags = BUGFLAG_WARNING|(flags); \ instrumentation_begin(); \
- _BUG_FLAGS(ASM_UD2, __flags, ANNOTATE_REACHABLE(1b)); \
- if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) \
instrumentation_end(); \ } while (0)_BUG_FLAGS(ASM_UD2, __flags, ANNOTATE_REACHABLE(1b)); \
NAK, this grows the BUG site for now appreciable reason.
Only if CONFIG_KUNIT_SUPPRESS_BACKTRACE is enabled. Why does that warrant a NACK ?
And isn't that something distros will want enabled? All I'm seeing is bloating every single UD2 site, and no real justification. As Josh said, this should be done on the other side of the trap if at all.
On Tue, Apr 01, 2025 at 10:53:46AM -0700, Guenter Roeck wrote:
#define _BUG_FLAGS(ins, flags, extra) \ do { \ asm_inline volatile("1:\t" ins "\n" \ ".pushsection __bug_table,"aw"\n" \ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \ "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \
"\t.word %c1" "\t# bug_entry::line\n" \
"\t.word %c2" "\t# bug_entry::flags\n" \
"\t.org 2b+%c3\n" \
"\t" __BUG_FUNC_PTR "\t# bug_entry::function\n" \
"\t.word %c2" "\t# bug_entry::line\n" \
"\t.word %c3" "\t# bug_entry::flags\n" \
"\t.org 2b+%c4\n" \ ".popsection\n" \ extra \
: : "i" (__FILE__), "i" (__LINE__), \
} while (0): : "i" (__FILE__), "i" (__BUG_FUNC), "i" (__LINE__),\ "i" (flags), \ "i" (sizeof(struct bug_entry))); \
Also this, why do you need this extra function in the bug entry? Isn't that trivial from the trap site itself? symbol information should be able to get you the function from the trap ip.
None of this makes any sense.
From: Guenter Roeck linux@roeck-us.net
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Cc: Catalin Marinas catalin.marinas@arm.com Cc: Will Deacon will@kernel.org Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com --- arch/arm64/include/asm/asm-bug.h | 27 ++++++++++++++++++--------- arch/arm64/include/asm/bug.h | 8 +++++++- 2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/include/asm/asm-bug.h b/arch/arm64/include/asm/asm-bug.h index 6e73809f6492..bf0a5ba81611 100644 --- a/arch/arm64/include/asm/asm-bug.h +++ b/arch/arm64/include/asm/asm-bug.h @@ -8,37 +8,46 @@ #include <asm/brk-imm.h>
#ifdef CONFIG_DEBUG_BUGVERBOSE -#define _BUGVERBOSE_LOCATION(file, line) __BUGVERBOSE_LOCATION(file, line) -#define __BUGVERBOSE_LOCATION(file, line) \ + +#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR(func) .long func - .; +#else +# define __BUG_FUNC_PTR(func) +#endif + +#define _BUGVERBOSE_LOCATION(file, func, line) __BUGVERBOSE_LOCATION(file, func, line) +#define __BUGVERBOSE_LOCATION(file, func, line) \ .pushsection .rodata.str,"aMS",@progbits,1; \ 14472: .string file; \ .popsection; \ \ .long 14472b - .; \ + __BUG_FUNC_PTR(func) \ .short line; #else -#define _BUGVERBOSE_LOCATION(file, line) +#define _BUGVERBOSE_LOCATION(file, func, line) #endif
#ifdef CONFIG_GENERIC_BUG
-#define __BUG_ENTRY(flags) \ +#define __BUG_ENTRY(flags, func) \ .pushsection __bug_table,"aw"; \ .align 2; \ 14470: .long 14471f - .; \ -_BUGVERBOSE_LOCATION(__FILE__, __LINE__) \ +_BUGVERBOSE_LOCATION(__FILE__, func, __LINE__) \ .short flags; \ .align 2; \ .popsection; \ 14471: #else -#define __BUG_ENTRY(flags) +#define __BUG_ENTRY(flags, func) #endif
-#define ASM_BUG_FLAGS(flags) \ - __BUG_ENTRY(flags) \ +#define ASM_BUG_FLAGS(flags, func) \ + __BUG_ENTRY(flags, func) \ brk BUG_BRK_IMM
-#define ASM_BUG() ASM_BUG_FLAGS(0) +#define ASM_BUG() ASM_BUG_FLAGS(0, .)
#endif /* __ASM_ASM_BUG_H */ diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h index 28be048db3f6..044c5e24a17d 100644 --- a/arch/arm64/include/asm/bug.h +++ b/arch/arm64/include/asm/bug.h @@ -11,8 +11,14 @@
#include <asm/asm-bug.h>
+#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC NULL +#endif + #define __BUG_FLAGS(flags) \ - asm volatile (__stringify(ASM_BUG_FLAGS(flags))); + asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i" (__BUG_FUNC));
#define BUG() do { \ __BUG_FLAGS(0); \
On Thu, Mar 13, 2025 at 11:43:22AM +0000, Alessandro Carminati wrote:
diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h index 28be048db3f6..044c5e24a17d 100644 --- a/arch/arm64/include/asm/bug.h +++ b/arch/arm64/include/asm/bug.h @@ -11,8 +11,14 @@ #include <asm/asm-bug.h> +#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC NULL +#endif
#define __BUG_FLAGS(flags) \
- asm volatile (__stringify(ASM_BUG_FLAGS(flags)));
- asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i" (__BUG_FUNC));
Why is 'i' the right asm constraint to use here? It seems a bit odd to use that for a pointer.
Will
Hello Will,
On Thu, Mar 13, 2025 at 1:25 PM Will Deacon will@kernel.org wrote:
On Thu, Mar 13, 2025 at 11:43:22AM +0000, Alessandro Carminati wrote:
diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h index 28be048db3f6..044c5e24a17d 100644 --- a/arch/arm64/include/asm/bug.h +++ b/arch/arm64/include/asm/bug.h @@ -11,8 +11,14 @@
#include <asm/asm-bug.h>
+#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC NULL +#endif
#define __BUG_FLAGS(flags) \
asm volatile (__stringify(ASM_BUG_FLAGS(flags)));
asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i" (__BUG_FUNC));
Why is 'i' the right asm constraint to use here? It seems a bit odd to use that for a pointer.
I received this code as legacy from a previous version. In my review, I considered the case when HAVE_BUG_FUNCTION is defined: Here, __BUG_FUNC is defined as __func__, which is the name of the current function as a string literal. Using the constraint "i" seems appropriate to me in this case.
However, when HAVE_BUG_FUNCTION is not defined: __BUG_FUNC is defined as NULL. Initially, I considered it literal 0, but after investigating your concern, I found:
``` $ echo -E "#include <stdio.h>\n#include <stddef.h>\nint main() {\nreturn 0;\n}" | aarch64-linux-gnu-gcc -E -dM - | grep NULL #define NULL ((void *)0) ```
I realized that NULL is actually a pointer that is not a link time symbol, and using the "i" constraint with NULL may result in undefined behavior.
Would the following alternative definition for __BUG_FUNC be more convincing?
``` #ifdef HAVE_BUG_FUNCTION #define __BUG_FUNC __func__ #else #define __BUG_FUNC (uintptr_t)0 #endif ``` Let me know your thoughts.
Will
-- --- 172
On Thu, Mar 13, 2025 at 05:40:59PM +0100, Alessandro Carminati wrote:
On Thu, Mar 13, 2025 at 1:25 PM Will Deacon will@kernel.org wrote:
On Thu, Mar 13, 2025 at 11:43:22AM +0000, Alessandro Carminati wrote:
diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h index 28be048db3f6..044c5e24a17d 100644 --- a/arch/arm64/include/asm/bug.h +++ b/arch/arm64/include/asm/bug.h @@ -11,8 +11,14 @@
#include <asm/asm-bug.h>
+#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC NULL +#endif
#define __BUG_FLAGS(flags) \
asm volatile (__stringify(ASM_BUG_FLAGS(flags)));
asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i" (__BUG_FUNC));
Why is 'i' the right asm constraint to use here? It seems a bit odd to use that for a pointer.
I received this code as legacy from a previous version. In my review, I considered the case when HAVE_BUG_FUNCTION is defined: Here, __BUG_FUNC is defined as __func__, which is the name of the current function as a string literal. Using the constraint "i" seems appropriate to me in this case.
However, when HAVE_BUG_FUNCTION is not defined: __BUG_FUNC is defined as NULL. Initially, I considered it literal 0, but after investigating your concern, I found:
$ echo -E "#include <stdio.h>\n#include <stddef.h>\nint main() {\nreturn 0;\n}" | aarch64-linux-gnu-gcc -E -dM - | grep NULL #define NULL ((void *)0)
I realized that NULL is actually a pointer that is not a link time symbol, and using the "i" constraint with NULL may result in undefined behavior.
Would the following alternative definition for __BUG_FUNC be more convincing?
#ifdef HAVE_BUG_FUNCTION #define __BUG_FUNC __func__ #else #define __BUG_FUNC (uintptr_t)0 #endif
Let me know your thoughts.
Thanks for the analysis; I hadn't noticed this specific issue, it just smelled a bit fishy. Anyway, the diff above looks better, thanks.
Will
On 3/18/25 08:59, Will Deacon wrote:
On Thu, Mar 13, 2025 at 05:40:59PM +0100, Alessandro Carminati wrote:
On Thu, Mar 13, 2025 at 1:25 PM Will Deacon will@kernel.org wrote:
On Thu, Mar 13, 2025 at 11:43:22AM +0000, Alessandro Carminati wrote:
diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h index 28be048db3f6..044c5e24a17d 100644 --- a/arch/arm64/include/asm/bug.h +++ b/arch/arm64/include/asm/bug.h @@ -11,8 +11,14 @@
#include <asm/asm-bug.h>
+#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC NULL +#endif
- #define __BUG_FLAGS(flags) \
asm volatile (__stringify(ASM_BUG_FLAGS(flags)));
asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i" (__BUG_FUNC));
Why is 'i' the right asm constraint to use here? It seems a bit odd to use that for a pointer.
I received this code as legacy from a previous version. In my review, I considered the case when HAVE_BUG_FUNCTION is defined: Here, __BUG_FUNC is defined as __func__, which is the name of the current function as a string literal. Using the constraint "i" seems appropriate to me in this case.
However, when HAVE_BUG_FUNCTION is not defined: __BUG_FUNC is defined as NULL. Initially, I considered it literal 0, but after investigating your concern, I found:
$ echo -E "#include <stdio.h>\n#include <stddef.h>\nint main() {\nreturn 0;\n}" | aarch64-linux-gnu-gcc -E -dM - | grep NULL #define NULL ((void *)0)
I realized that NULL is actually a pointer that is not a link time symbol, and using the "i" constraint with NULL may result in undefined behavior.
Would the following alternative definition for __BUG_FUNC be more convincing?
#ifdef HAVE_BUG_FUNCTION #define __BUG_FUNC __func__ #else #define __BUG_FUNC (uintptr_t)0 #endif
Let me know your thoughts.
Thanks for the analysis; I hadn't noticed this specific issue, it just smelled a bit fishy. Anyway, the diff above looks better, thanks.
It has been a long time, but I seem to recall that I ran into trouble when trying to use a different constraint.
Guenter
Le 18/03/2025 à 16:59, Will Deacon a écrit :
On Thu, Mar 13, 2025 at 05:40:59PM +0100, Alessandro Carminati wrote:
On Thu, Mar 13, 2025 at 1:25 PM Will Deacon will@kernel.org wrote:
On Thu, Mar 13, 2025 at 11:43:22AM +0000, Alessandro Carminati wrote:
diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h index 28be048db3f6..044c5e24a17d 100644 --- a/arch/arm64/include/asm/bug.h +++ b/arch/arm64/include/asm/bug.h @@ -11,8 +11,14 @@
#include <asm/asm-bug.h>
+#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC NULL +#endif
- #define __BUG_FLAGS(flags) \
asm volatile (__stringify(ASM_BUG_FLAGS(flags)));
asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i" (__BUG_FUNC));
Why is 'i' the right asm constraint to use here? It seems a bit odd to use that for a pointer.
I received this code as legacy from a previous version. In my review, I considered the case when HAVE_BUG_FUNCTION is defined: Here, __BUG_FUNC is defined as __func__, which is the name of the current function as a string literal. Using the constraint "i" seems appropriate to me in this case.
However, when HAVE_BUG_FUNCTION is not defined: __BUG_FUNC is defined as NULL. Initially, I considered it literal 0, but after investigating your concern, I found:
$ echo -E "#include <stdio.h>\n#include <stddef.h>\nint main() {\nreturn 0;\n}" | aarch64-linux-gnu-gcc -E -dM - | grep NULL #define NULL ((void *)0)
I realized that NULL is actually a pointer that is not a link time symbol, and using the "i" constraint with NULL may result in undefined behavior.
Would the following alternative definition for __BUG_FUNC be more convincing?
#ifdef HAVE_BUG_FUNCTION #define __BUG_FUNC __func__ #else #define __BUG_FUNC (uintptr_t)0 #endif
Let me know your thoughts.
Thanks for the analysis; I hadn't noticed this specific issue, it just smelled a bit fishy. Anyway, the diff above looks better, thanks.
That propably deserves a comment.
Doesn't sparse and/or checkpatch complain about 0 being used in lieu of NULL ?
By the way I had similar problem in the past with GCC not seeing NULL as a __builtin_constant_p(), refer commit 1d8f739b07bd ("powerpc/kuap: Fix set direction in allow/prevent_user_access()")
Christophe
On Wed, Mar 19, 2025 at 09:05:27AM +0100, Christophe Leroy wrote:
Doesn't sparse and/or checkpatch complain about 0 being used in lieu of NULL ?
Sparse does have a "Using plain integer as NULL pointer" warning, yes.
I can't apply this patchset and I haven't been following the conversation closely (plus I'm pretty stupid as well) so I'm not sure if it will trigger here...
regards, dan carpenter
On 3/19/25 01:05, Christophe Leroy wrote:
Le 18/03/2025 à 16:59, Will Deacon a écrit :
On Thu, Mar 13, 2025 at 05:40:59PM +0100, Alessandro Carminati wrote:
On Thu, Mar 13, 2025 at 1:25 PM Will Deacon will@kernel.org wrote:
On Thu, Mar 13, 2025 at 11:43:22AM +0000, Alessandro Carminati wrote:
diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h index 28be048db3f6..044c5e24a17d 100644 --- a/arch/arm64/include/asm/bug.h +++ b/arch/arm64/include/asm/bug.h @@ -11,8 +11,14 @@
#include <asm/asm-bug.h>
+#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC NULL +#endif
#define __BUG_FLAGS(flags) \ - asm volatile (__stringify(ASM_BUG_FLAGS(flags))); + asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i" (__BUG_FUNC));
Why is 'i' the right asm constraint to use here? It seems a bit odd to use that for a pointer.
I received this code as legacy from a previous version. In my review, I considered the case when HAVE_BUG_FUNCTION is defined: Here, __BUG_FUNC is defined as __func__, which is the name of the current function as a string literal. Using the constraint "i" seems appropriate to me in this case.
However, when HAVE_BUG_FUNCTION is not defined: __BUG_FUNC is defined as NULL. Initially, I considered it literal 0, but after investigating your concern, I found:
$ echo -E "#include <stdio.h>\n#include <stddef.h>\nint main() {\nreturn 0;\n}" | aarch64-linux-gnu-gcc -E -dM - | grep NULL #define NULL ((void *)0)
I realized that NULL is actually a pointer that is not a link time symbol, and using the "i" constraint with NULL may result in undefined behavior.
Would the following alternative definition for __BUG_FUNC be more convincing?
#ifdef HAVE_BUG_FUNCTION #define __BUG_FUNC __func__ #else #define __BUG_FUNC (uintptr_t)0 #endif
Let me know your thoughts.
Thanks for the analysis; I hadn't noticed this specific issue, it just smelled a bit fishy. Anyway, the diff above looks better, thanks.
That propably deserves a comment.
Doesn't sparse and/or checkpatch complain about 0 being used in lieu of NULL ?
__BUG_FUNC is only used as parameter to asm code, not as pointer.
From the diff:
- : : "i" (__FILE__), "i" (__LINE__), \ + : : "i" (__FILE__), "i" (__BUG_FUNC), "i" (__LINE__),\
The use is quite similar to __FILE__ and __LINE__. It might even be possible and appropriate to just define __BUG_FUNC as 0 if HAVE_BUG_FUNCTION is not defined.
Guenter
From: Guenter Roeck linux@roeck-us.net
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Cc: Huacai Chen chenhuacai@kernel.org Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com --- arch/loongarch/include/asm/bug.h | 42 ++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 13 deletions(-)
diff --git a/arch/loongarch/include/asm/bug.h b/arch/loongarch/include/asm/bug.h index f6f254f2c5db..b79ff6696ce6 100644 --- a/arch/loongarch/include/asm/bug.h +++ b/arch/loongarch/include/asm/bug.h @@ -3,49 +3,65 @@ #define __ASM_BUG_H
#include <asm/break.h> +#include <kunit/bug.h> #include <linux/stringify.h> #include <linux/objtool.h>
#ifndef CONFIG_DEBUG_BUGVERBOSE -#define _BUGVERBOSE_LOCATION(file, line) +#define _BUGVERBOSE_LOCATION(file, func, line) #else -#define __BUGVERBOSE_LOCATION(file, line) \ +#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR(func) .long func - .; +#else +# define __BUG_FUNC_PTR(func) +#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */ + +#define __BUGVERBOSE_LOCATION(file, func, line) \ .pushsection .rodata.str, "aMS", @progbits, 1; \ 10002: .string file; \ .popsection; \ \ .long 10002b - .; \ + __BUG_FUNC_PTR(func) \ .short line; -#define _BUGVERBOSE_LOCATION(file, line) __BUGVERBOSE_LOCATION(file, line) +#define _BUGVERBOSE_LOCATION(file, func, line) __BUGVERBOSE_LOCATION(file, func, line) #endif
#ifndef CONFIG_GENERIC_BUG -#define __BUG_ENTRY(flags) +#define __BUG_ENTRY(flags, func) #else -#define __BUG_ENTRY(flags) \ +#define __BUG_ENTRY(flags, func) \ .pushsection __bug_table, "aw"; \ .align 2; \ 10000: .long 10001f - .; \ - _BUGVERBOSE_LOCATION(__FILE__, __LINE__) \ + _BUGVERBOSE_LOCATION(__FILE__, func, __LINE__) \ .short flags; \ .popsection; \ 10001: #endif
-#define ASM_BUG_FLAGS(flags) \ - __BUG_ENTRY(flags) \ +#define ASM_BUG_FLAGS(flags, func) \ + __BUG_ENTRY(flags, func) \ break BRK_BUG;
-#define ASM_BUG() ASM_BUG_FLAGS(0) +#define ASM_BUG() ASM_BUG_FLAGS(0, .) + +#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC NULL +#endif
-#define __BUG_FLAGS(flags, extra) \ - asm_inline volatile (__stringify(ASM_BUG_FLAGS(flags)) \ - extra); +#define __BUG_FLAGS(flags, extra) \ + asm_inline volatile (__stringify(ASM_BUG_FLAGS(flags, %0)) \ + extra : : "i" (__BUG_FUNC) );
#define __WARN_FLAGS(flags) \ do { \ instrumentation_begin(); \ - __BUG_FLAGS(BUGFLAG_WARNING|(flags), ANNOTATE_REACHABLE(10001b));\ + if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ + __BUG_FLAGS(BUGFLAG_WARNING|(flags), ANNOTATE_REACHABLE(10001b));\ instrumentation_end(); \ } while (0)
From: Guenter Roeck linux@roeck-us.net
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
While at it, declare assembler parameters as constants where possible. Refine .blockz instructions to calculate the necessary padding instead of using fixed values.
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Acked-by: Helge Deller deller@gmx.de Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com --- arch/parisc/include/asm/bug.h | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/arch/parisc/include/asm/bug.h b/arch/parisc/include/asm/bug.h index 833555f74ffa..b59c3f7380bf 100644 --- a/arch/parisc/include/asm/bug.h +++ b/arch/parisc/include/asm/bug.h @@ -23,8 +23,17 @@ # define __BUG_REL(val) ".word " __stringify(val) #endif
- #ifdef CONFIG_DEBUG_BUGVERBOSE + +#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR __BUG_REL(%c1) +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC_PTR +# define __BUG_FUNC NULL +#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */ + #define BUG() \ do { \ asm volatile("\n" \ @@ -33,10 +42,12 @@ "\t.align 4\n" \ "2:\t" __BUG_REL(1b) "\n" \ "\t" __BUG_REL(%c0) "\n" \ - "\t.short %1, %2\n" \ - "\t.blockz %3-2*4-2*2\n" \ + "\t" __BUG_FUNC_PTR "\n" \ + "\t.short %c2, %c3\n" \ + "\t.blockz %c4-(.-2b)\n" \ "\t.popsection" \ - : : "i" (__FILE__), "i" (__LINE__), \ + : : "i" (__FILE__), "i" (__BUG_FUNC), \ + "i" (__LINE__), \ "i" (0), "i" (sizeof(struct bug_entry)) ); \ unreachable(); \ } while(0) @@ -58,10 +69,12 @@ "\t.align 4\n" \ "2:\t" __BUG_REL(1b) "\n" \ "\t" __BUG_REL(%c0) "\n" \ - "\t.short %1, %2\n" \ - "\t.blockz %3-2*4-2*2\n" \ + "\t" __BUG_FUNC_PTR "\n" \ + "\t.short %c2, %3\n" \ + "\t.blockz %c4-(.-2b)\n" \ "\t.popsection" \ - : : "i" (__FILE__), "i" (__LINE__), \ + : : "i" (__FILE__), "i" (__BUG_FUNC), \ + "i" (__LINE__), \ "i" (BUGFLAG_WARNING|(flags)), \ "i" (sizeof(struct bug_entry)) ); \ } while(0) @@ -74,7 +87,7 @@ "\t.align 4\n" \ "2:\t" __BUG_REL(1b) "\n" \ "\t.short %0\n" \ - "\t.blockz %1-4-2\n" \ + "\t.blockz %c1-(.-2b)\n" \ "\t.popsection" \ : : "i" (BUGFLAG_WARNING|(flags)), \ "i" (sizeof(struct bug_entry)) ); \
From: Guenter Roeck linux@roeck-us.net
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Cc: Heiko Carstens hca@linux.ibm.com Cc: Vasily Gorbik gor@linux.ibm.com Cc: Alexander Gordeev agordeev@linux.ibm.com Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com --- arch/s390/include/asm/bug.h | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/arch/s390/include/asm/bug.h b/arch/s390/include/asm/bug.h index c500d45fb465..44d4e9f24ae0 100644 --- a/arch/s390/include/asm/bug.h +++ b/arch/s390/include/asm/bug.h @@ -8,6 +8,15 @@
#ifdef CONFIG_DEBUG_BUGVERBOSE
+#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR " .long %0-.\n" +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC_PTR +# define __BUG_FUNC NULL +#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */ + #define __EMIT_BUG(x) do { \ asm_inline volatile( \ "0: mc 0,0\n" \ @@ -17,10 +26,12 @@ ".section __bug_table,"aw"\n" \ "2: .long 0b-.\n" \ " .long 1b-.\n" \ - " .short %0,%1\n" \ - " .org 2b+%2\n" \ + __BUG_FUNC_PTR \ + " .short %1,%2\n" \ + " .org 2b+%3\n" \ ".previous\n" \ - : : "i" (__LINE__), \ + : : "i" (__BUG_FUNC), \ + "i" (__LINE__), \ "i" (x), \ "i" (sizeof(struct bug_entry))); \ } while (0)
On 3/13/25 04:43, Alessandro Carminati wrote:
From: Guenter Roeck linux@roeck-us.net
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Cc: Heiko Carstens hca@linux.ibm.com Cc: Vasily Gorbik gor@linux.ibm.com Cc: Alexander Gordeev agordeev@linux.ibm.com Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com
arch/s390/include/asm/bug.h | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/arch/s390/include/asm/bug.h b/arch/s390/include/asm/bug.h index c500d45fb465..44d4e9f24ae0 100644 --- a/arch/s390/include/asm/bug.h +++ b/arch/s390/include/asm/bug.h @@ -8,6 +8,15 @@ #ifdef CONFIG_DEBUG_BUGVERBOSE +#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR " .long %0-.\n" +# define __BUG_FUNC __func__
gcc 7.5.0 on s390 barfs; it doesn't like the use of "__func__" with "%0-."
drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.c: In function 'anx_dp_aux_transfer': ././include/linux/compiler_types.h:492:20: warning: asm operand 0 probably doesn't match constraints
I was unable to find an alternate constraint that the compiler would accept.
I don't know if the same problem is seen with older compilers on other architectures, or if the problem is relevant in the first place.
gcc 10.3.0 and later do not have this problem. I also tried s390 builds with gcc 9.4 and 9.5 but they both crash for unrelated reasons.
If this is a concern, the best idea I have is to make KUNIT_SUPPRESS_BACKTRACE depend on, say, depends on CC_IS_CLANG || (CC_IS_GCC && GCC_VERSION >= 100300)
A more complex solution might be to define an architecture flag such as HAVE_SUPPRESS_BACKTRACE, make that conditional on the gcc version for s390 only, and make KUNIT_SUPPRESS_BACKTRACE depend on it.
Guenter
On Fri, Mar 21, 2025, at 18:05, Guenter Roeck wrote:
On 3/13/25 04:43, Alessandro Carminati wrote:
gcc 10.3.0 and later do not have this problem. I also tried s390 builds with gcc 9.4 and 9.5 but they both crash for unrelated reasons.
If this is a concern, the best idea I have is to make KUNIT_SUPPRESS_BACKTRACE depend on, say, depends on CC_IS_CLANG || (CC_IS_GCC && GCC_VERSION >= 100300)
A more complex solution might be to define an architecture flag such as HAVE_SUPPRESS_BACKTRACE, make that conditional on the gcc version for s390 only, and make KUNIT_SUPPRESS_BACKTRACE depend on it.
That is probably fine, there are very few users on s390 that build their own kernels, and they likely all have modern compilers anyway.
I should still send a patch to raise the minimum compiler version to gcc-8.1, but unfortunately that is not enough here.
Arnd
Hello Guenter, Sorry for being late to the party.
On Fri, Mar 21, 2025 at 6:06 PM Guenter Roeck linux@roeck-us.net wrote:
On 3/13/25 04:43, Alessandro Carminati wrote:
From: Guenter Roeck linux@roeck-us.net
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Cc: Heiko Carstens hca@linux.ibm.com Cc: Vasily Gorbik gor@linux.ibm.com Cc: Alexander Gordeev agordeev@linux.ibm.com Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com
arch/s390/include/asm/bug.h | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/arch/s390/include/asm/bug.h b/arch/s390/include/asm/bug.h index c500d45fb465..44d4e9f24ae0 100644 --- a/arch/s390/include/asm/bug.h +++ b/arch/s390/include/asm/bug.h @@ -8,6 +8,15 @@
#ifdef CONFIG_DEBUG_BUGVERBOSE
+#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR " .long %0-.\n" +# define __BUG_FUNC __func__
gcc 7.5.0 on s390 barfs; it doesn't like the use of "__func__" with "%0-."
drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.c: In function 'anx_dp_aux_transfer': ././include/linux/compiler_types.h:492:20: warning: asm operand 0 probably doesn't match constraints
I was unable to find an alternate constraint that the compiler would accept.
I don't know if the same problem is seen with older compilers on other architectures, or if the problem is relevant in the first place.
gcc 10.3.0 and later do not have this problem. I also tried s390 builds with gcc 9.4 and 9.5 but they both crash for unrelated reasons.
If this is a concern, the best idea I have is to make KUNIT_SUPPRESS_BACKTRACE depend on, say, depends on CC_IS_CLANG || (CC_IS_GCC && GCC_VERSION >= 100300)
A more complex solution might be to define an architecture flag such as HAVE_SUPPRESS_BACKTRACE, make that conditional on the gcc version for s390 only, and make KUNIT_SUPPRESS_BACKTRACE depend on it.
I've spent some time trying to better define the problem. Although it may seem trivial, the old compiler simply doesn't work—I believe the issue is a bit more complex.
So, let me share some code and then comment on it. $ cat bug-s390.c #include "bug_entry.h" #define asm_inline asm __inline # define __BUG_FUNC_PTR " .long %0-.\n" # define __BUG_FUNC __func__ #define __EMIT_BUG(x) do { \ asm_inline volatile( \ "0: mc 0,0\n" \ ".section .rodata.str,"aMS",@progbits,1\n" \ "1: .asciz ""__FILE__""\n" \ ".previous\n" \ ".section __bug_table,"aw"\n" \ "2: .long 0b-.\n" \ " .long 1b-.\n" \ __BUG_FUNC_PTR \ " .short %1,%2\n" \ " .org 2b+%3\n" \ ".previous\n" \ : : "i" (__BUG_FUNC), \ "i" (__LINE__), \ "i" (x), \ "i" (sizeof(struct bug_entry))); \ } while (0)
#define BUG() do { \ __EMIT_BUG(0); \ } while (0)
void f1(){ BUG(); } void f2(){ BUG(); } int main() { BUG(); f1(); f2(); return 0; } $ # This is a stripped version of the s390x code for bug $ ~/x-tools/s390x-ibm-linux-gnu_14/bin/s390x-ibm-linux-gnu-gcc -v Using built-in specs. COLLECT_GCC=/home/alessandro/x-tools/s390x-ibm-linux-gnu_14/bin/s390x-ibm-linux-gnu-gcc COLLECT_LTO_WRAPPER=/home/alessandro/x-tools/s390x-ibm-linux-gnu_14/bin/../libexec/gcc/s390x-ibm-linux-gnu/14.2.0/lto-wrapper Target: s390x-ibm-linux-gnu Configured with: /home/alessandro/src/s390x-toolchain/.build/s390x-ibm-linux-gnu/src/gcc/configure --build=x86_64-build_pc-linux-gnu --host=x86_64-build_pc-linux-gnu --target=s390x-ibm-linux-gnu --prefix=/home/alessandro/x-tools/s390x-ibm-linux-gnu --exec_prefix=/home/alessandro/x-tools/s390x-ibm-linux-gnu --with-sysroot=/home/alessandro/x-tools/s390x-ibm-linux-gnu/s390x-ibm-linux-gnu/sysroot --enable-languages=c,c++ --with-pkgversion='crosstool-NG 1.27.0.18_7458341' --enable-__cxa_atexit --disable-libmudflap --disable-libgomp --disable-libssp --disable-libquadmath --disable-libquadmath-support --disable-libsanitizer --disable-libmpx --with-gmp=/home/alessandro/src/s390x-toolchain/.build/s390x-ibm-linux-gnu/buildtools --with-mpfr=/home/alessandro/src/s390x-toolchain/.build/s390x-ibm-linux-gnu/buildtools --with-mpc=/home/alessandro/src/s390x-toolchain/.build/s390x-ibm-linux-gnu/buildtools --with-isl=/home/alessandro/src/s390x-toolchain/.build/s390x-ibm-linux-gnu/buildtools --enable-lto --enable-threads=posix --enable-target-optspace --disable-plugin --disable-nls --disable-multilib --with-local-prefix=/home/alessandro/x-tools/s390x-ibm-linux-gnu/s390x-ibm-linux-gnu/sysroot --enable-long-long Thread model: posix Supported LTO compression algorithms: zlib zstd gcc version 14.2.0 (crosstool-NG 1.27.0.18_7458341) $ ~/x-tools/s390x-ibm-linux-gnu_14/bin/s390x-ibm-linux-gnu-gcc -S -m64 bug-s390.c $ ~/x-tools/s390x-ibm-linux-gnu_14/bin/s390x-ibm-linux-gnu-gcc -S -m64 -fPIC bug-s390.c $ ~/x-tools/s390x-ibm-linux-gnu/bin/s390x-ibm-linux-gnu-gcc -v Using built-in specs. COLLECT_GCC=/home/alessandro/x-tools/s390x-ibm-linux-gnu/bin/s390x-ibm-linux-gnu-gcc COLLECT_LTO_WRAPPER=/home/alessandro/x-tools/s390x-ibm-linux-gnu/libexec/gcc/s390x-ibm-linux-gnu/7.5.0/lto-wrapper Target: s390x-ibm-linux-gnu Configured with: /home/alessandro/src/cross-s390/.build/s390x-ibm-linux-gnu/src/gcc/configure --build=x86_64-build_pc-linux-gnu --host=x86_64-build_pc-linux-gnu --target=s390x-ibm-linux-gnu --prefix=/home/alessandro/x-tools/s390x-ibm-linux-gnu --exec_prefix=/home/alessandro/x-tools/s390x-ibm-linux-gnu --with-sysroot=/home/alessandro/x-tools/s390x-ibm-linux-gnu/s390x-ibm-linux-gnu/sysroot --enable-languages=c --with-pkgversion='crosstool-NG 1.27.0.18_7458341' --enable-__cxa_atexit --disable-tm-clone-registry --disable-libmudflap --disable-libgomp --disable-libssp --disable-libquadmath --disable-libquadmath-support --disable-libsanitizer --disable-libmpx --disable-libstdcxx-verbose --with-gmp=/home/alessandro/src/cross-s390/.build/s390x-ibm-linux-gnu/buildtools --with-mpfr=/home/alessandro/src/cross-s390/.build/s390x-ibm-linux-gnu/buildtools --with-mpc=/home/alessandro/src/cross-s390/.build/s390x-ibm-linux-gnu/buildtools --with-isl=/home/alessandro/src/cross-s390/.build/s390x-ibm-linux-gnu/buildtools --enable-lto --without-zstd --enable-threads=posix --enable-target-optspace --disable-plugin --disable-nls --disable-multilib --with-local-prefix=/home/alessandro/x-tools/s390x-ibm-linux-gnu/s390x-ibm-linux-gnu/sysroot --enable-long-long Thread model: posix gcc version 7.5.0 (crosstool-NG 1.27.0.18_7458341) $ ~/x-tools/s390x-ibm-linux-gnu/bin/s390x-ibm-linux-gnu-gcc -S -m64 bug-s390.c $ ~/x-tools/s390x-ibm-linux-gnu/bin/s390x-ibm-linux-gnu-gcc -S -m64 -fPIC bug-s390.c bug-s390.c: In function 'f1': bug-s390.c:2:20: warning: asm operand 0 probably doesn't match constraints #define asm_inline asm __inline ^ bug-s390.c:6:2: note: in expansion of macro 'asm_inline' asm_inline volatile( \ ^~~~~~~~~~ bug-s390.c:25:2: note: in expansion of macro '__EMIT_BUG' __EMIT_BUG(0); \ ^~~~~~~~~~ bug-s390.c:29:2: note: in expansion of macro 'BUG' BUG(); ^~~ bug-s390.c:2:20: error: impossible constraint in 'asm' #define asm_inline asm __inline ^ bug-s390.c:6:2: note: in expansion of macro 'asm_inline' asm_inline volatile( \ ^~~~~~~~~~ bug-s390.c:25:2: note: in expansion of macro '__EMIT_BUG' __EMIT_BUG(0); \ ^~~~~~~~~~ bug-s390.c:29:2: note: in expansion of macro 'BUG' BUG(); ^~~ bug-s390.c: In function 'f2': bug-s390.c:2:20: warning: asm operand 0 probably doesn't match constraints #define asm_inline asm __inline ^ bug-s390.c:6:2: note: in expansion of macro 'asm_inline' asm_inline volatile( \ ^~~~~~~~~~ bug-s390.c:25:2: note: in expansion of macro '__EMIT_BUG' __EMIT_BUG(0); \ ^~~~~~~~~~ bug-s390.c:32:2: note: in expansion of macro 'BUG' BUG(); ^~~ bug-s390.c: In function 'main': bug-s390.c:2:20: warning: asm operand 0 probably doesn't match constraints #define asm_inline asm __inline ^ bug-s390.c:6:2: note: in expansion of macro 'asm_inline' asm_inline volatile( \ ^~~~~~~~~~ bug-s390.c:25:2: note: in expansion of macro '__EMIT_BUG' __EMIT_BUG(0); \ ^~~~~~~~~~ bug-s390.c:35:2: note: in expansion of macro 'BUG' BUG(); ^~~ $ cat linux-6.14-rc7/arch/s390/Makefile| grep "fPIC" KBUILD_AFLAGS_MODULE += -fPIC KBUILD_CFLAGS_MODULE += -fPIC KBUILD_CFLAGS += -fPIC
As you can see, the problem is not that the compiler itself doesn't work, but rather that -fPIC introduces some complications. __func__ is a compile-time constant, but this holds true only for traditionally linked code. When compiling position-independent code, this assumption no longer applies.
GCC makes significant efforts to handle this, and for several architectures, it manages to solve the problem. However, this is not universally the case. Additionally, -fPIC is not widely used in kernel code... I have only seen it used for VDSO, the x86 boot piggyback decompressor, PowerPC boot, and the s390x architecture.
That said, GCC has been mitigating this issue, allowing us to treat a non-compile-time constant as if it were one. A proof of this is that, at least since GCC 11, the s390x version of GCC is able to build this code. Before that... certainly in GCC 7.5 it couldn't.
A simple fix would be to restrict usage to GCC versions greater than 11 for s390.
The real concern is that we have a latent issue that could be triggered by changes in build settings, as "feature" support varies across GCC versions and architectures. For example, while x86_64 at version 14 seems to work both with and without -fPIC, this is not the case for AArch64, where enabling -fPIC causes failures even in version 14.
I'm currently working on a long-term fix for this.
Guenter
On Fri, Mar 21, 2025 at 10:05:42PM +0100, Alessandro Carminati wrote:
+#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR " .long %0-.\n" +# define __BUG_FUNC __func__
gcc 7.5.0 on s390 barfs; it doesn't like the use of "__func__" with "%0-."
...
GCC makes significant efforts to handle this, and for several architectures, it manages to solve the problem. However, this is not universally the case. Additionally, -fPIC is not widely used in kernel code... I have only seen it used for VDSO, the x86 boot piggyback decompressor, PowerPC boot, and the s390x architecture.
That said, GCC has been mitigating this issue, allowing us to treat a non-compile-time constant as if it were one. A proof of this is that, at least since GCC 11, the s390x version of GCC is able to build this code. Before that... certainly in GCC 7.5 it couldn't.
A simple fix would be to restrict usage to GCC versions greater than 11 for s390.
But please add that dependency only for this new feature for the time being. Right now I would not like to see that s390 is the only architecture (besides parisc) which requires a much higher minimum gcc level than every other architecture. Unless there are specific reasons.
From: Guenter Roeck linux@roeck-us.net
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Cc: Yoshinori Sato ysato@users.sourceforge.jp Cc: Rich Felker dalias@libc.org Cc: John Paul Adrian Glaubitz glaubitz@physik.fu-berlin.de Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com --- arch/sh/include/asm/bug.h | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/arch/sh/include/asm/bug.h b/arch/sh/include/asm/bug.h index 05a485c4fabc..470ce6567d20 100644 --- a/arch/sh/include/asm/bug.h +++ b/arch/sh/include/asm/bug.h @@ -24,21 +24,36 @@ * The offending file and line are encoded in the __bug_table section. */ #ifdef CONFIG_DEBUG_BUGVERBOSE + +#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR "\t.long %O2\n" +#else +# define __BUG_FUNC_PTR +#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */ + #define _EMIT_BUG_ENTRY \ "\t.pushsection __bug_table,"aw"\n" \ "2:\t.long 1b, %O1\n" \ - "\t.short %O2, %O3\n" \ - "\t.org 2b+%O4\n" \ + __BUG_FUNC_PTR \ + "\t.short %O3, %O4\n" \ + "\t.org 2b+%O5\n" \ "\t.popsection\n" #else #define _EMIT_BUG_ENTRY \ "\t.pushsection __bug_table,"aw"\n" \ "2:\t.long 1b\n" \ - "\t.short %O3\n" \ - "\t.org 2b+%O4\n" \ + "\t.short %O4\n" \ + "\t.org 2b+%O5\n" \ "\t.popsection\n" #endif
+#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC NULL +#endif + #define BUG() \ do { \ __asm__ __volatile__ ( \ @@ -47,6 +62,7 @@ do { \ : \ : "n" (TRAPA_BUG_OPCODE), \ "i" (__FILE__), \ + "i" (__BUG_FUNC), \ "i" (__LINE__), "i" (0), \ "i" (sizeof(struct bug_entry))); \ unreachable(); \ @@ -60,6 +76,7 @@ do { \ : \ : "n" (TRAPA_BUG_OPCODE), \ "i" (__FILE__), \ + "i" (__BUG_FUNC), \ "i" (__LINE__), \ "i" (BUGFLAG_WARNING|(flags)), \ "i" (sizeof(struct bug_entry))); \ @@ -85,6 +102,7 @@ do { \ : \ : "n" (TRAPA_BUG_OPCODE), \ "i" (__FILE__), \ + "i" (__BUG_FUNC), \ "i" (__LINE__), \ "i" (BUGFLAG_UNWINDER), \ "i" (sizeof(struct bug_entry))); \
From: Guenter Roeck linux@roeck-us.net
Declaring the defines needed for suppressing warning inside '#ifdef CONFIG_DEBUG_BUGVERBOSE' results in a kerneldoc warning.
.../bug.h:29: warning: expecting prototype for _EMIT_BUG_ENTRY(). Prototype was for HAVE_BUG_FUNCTION() instead
Move the defines above the kerneldoc entry for _EMIT_BUG_ENTRY to make kerneldoc happy.
Reported-by: Simon Horman horms@kernel.org Cc: Simon Horman horms@kernel.org Cc: Yoshinori Sato ysato@users.sourceforge.jp Cc: Rich Felker dalias@libc.org Cc: John Paul Adrian Glaubitz glaubitz@physik.fu-berlin.de Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com --- arch/sh/include/asm/bug.h | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/arch/sh/include/asm/bug.h b/arch/sh/include/asm/bug.h index 470ce6567d20..bf4947d51d69 100644 --- a/arch/sh/include/asm/bug.h +++ b/arch/sh/include/asm/bug.h @@ -11,6 +11,15 @@ #define HAVE_ARCH_BUG #define HAVE_ARCH_WARN_ON
+#ifdef CONFIG_DEBUG_BUGVERBOSE +#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR "\t.long %O2\n" +#else +# define __BUG_FUNC_PTR +#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */ +#endif /* CONFIG_DEBUG_BUGVERBOSE */ + /** * _EMIT_BUG_ENTRY * %1 - __FILE__ @@ -25,13 +34,6 @@ */ #ifdef CONFIG_DEBUG_BUGVERBOSE
-#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE -# define HAVE_BUG_FUNCTION -# define __BUG_FUNC_PTR "\t.long %O2\n" -#else -# define __BUG_FUNC_PTR -#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */ - #define _EMIT_BUG_ENTRY \ "\t.pushsection __bug_table,"aw"\n" \ "2:\t.long 1b, %O1\n" \
From: Guenter Roeck linux@roeck-us.net
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
To simplify the implementation, unify the __BUG_ENTRY_ADDR and __BUG_ENTRY_FILE macros into a single macro named __BUG_REL() which takes the address, file, or function reference as parameter.
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Cc: Paul Walmsley paul.walmsley@sifive.com Cc: Palmer Dabbelt palmer@dabbelt.com Cc: Albert Ou aou@eecs.berkeley.edu Signed-off-by: Guenter Roeck linux@roeck-us.net Reviewed-by: Charlie Jenkins charlie@rivosinc.com Signed-off-by: Alessandro Carminati acarmina@redhat.com --- arch/riscv/include/asm/bug.h | 38 ++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-)
diff --git a/arch/riscv/include/asm/bug.h b/arch/riscv/include/asm/bug.h index 1aaea81fb141..79f360af4ad8 100644 --- a/arch/riscv/include/asm/bug.h +++ b/arch/riscv/include/asm/bug.h @@ -30,26 +30,39 @@ typedef u32 bug_insn_t;
#ifdef CONFIG_GENERIC_BUG_RELATIVE_POINTERS -#define __BUG_ENTRY_ADDR RISCV_INT " 1b - ." -#define __BUG_ENTRY_FILE RISCV_INT " %0 - ." +#define __BUG_REL(val) RISCV_INT " " __stringify(val) " - ." #else -#define __BUG_ENTRY_ADDR RISCV_PTR " 1b" -#define __BUG_ENTRY_FILE RISCV_PTR " %0" +#define __BUG_REL(val) RISCV_PTR " " __stringify(val) #endif
#ifdef CONFIG_DEBUG_BUGVERBOSE + +#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR __BUG_REL(%1) +#else +# define __BUG_FUNC_PTR +#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */ + #define __BUG_ENTRY \ - __BUG_ENTRY_ADDR "\n\t" \ - __BUG_ENTRY_FILE "\n\t" \ - RISCV_SHORT " %1\n\t" \ - RISCV_SHORT " %2" + __BUG_REL(1b) "\n\t" \ + __BUG_REL(%0) "\n\t" \ + __BUG_FUNC_PTR "\n\t" \ + RISCV_SHORT " %2\n\t" \ + RISCV_SHORT " %3" #else #define __BUG_ENTRY \ - __BUG_ENTRY_ADDR "\n\t" \ - RISCV_SHORT " %2" + __BUG_REL(1b) "\n\t" \ + RISCV_SHORT " %3" #endif
#ifdef CONFIG_GENERIC_BUG +#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC NULL +#endif + #define __BUG_FLAGS(flags) \ do { \ __asm__ __volatile__ ( \ @@ -58,10 +71,11 @@ do { \ ".pushsection __bug_table,"aw"\n\t" \ "2:\n\t" \ __BUG_ENTRY "\n\t" \ - ".org 2b + %3\n\t" \ + ".org 2b + %4\n\t" \ ".popsection" \ : \ - : "i" (__FILE__), "i" (__LINE__), \ + : "i" (__FILE__), "i" (__BUG_FUNC), \ + "i" (__LINE__), \ "i" (flags), \ "i" (sizeof(struct bug_entry))); \ } while (0)
From: Guenter Roeck linux@roeck-us.net
Add name of functions triggering warning backtraces to the __bug_table object section to enable support for suppressing WARNING backtraces.
To limit image size impact, the pointer to the function name is only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled. Otherwise, the __func__ assembly parameter is replaced with a (dummy) NULL parameter to avoid an image size increase due to unused __func__ entries (this is necessary because __func__ is not a define but a virtual variable).
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Cc: Michael Ellerman mpe@ellerman.id.au Signed-off-by: Guenter Roeck linux@roeck-us.net Acked-by: Michael Ellerman mpe@ellerman.id.au Signed-off-by: Alessandro Carminati acarmina@redhat.com --- arch/powerpc/include/asm/bug.h | 37 +++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h index 1db485aacbd9..5b06745d20aa 100644 --- a/arch/powerpc/include/asm/bug.h +++ b/arch/powerpc/include/asm/bug.h @@ -14,6 +14,9 @@ .section __bug_table,"aw" 5001: .4byte \addr - . .4byte 5002f - . +#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE + .4byte 0 +#endif .short \line, \flags .org 5001b+BUG_ENTRY_SIZE .previous @@ -32,30 +35,46 @@ #endif /* verbose */
#else /* !__ASSEMBLY__ */ -/* _EMIT_BUG_ENTRY expects args %0,%1,%2,%3 to be FILE, LINE, flags and - sizeof(struct bug_entry), respectively */ +/* _EMIT_BUG_ENTRY expects args %0,%1,%2,%3,%4 to be FILE, __func__, LINE, flags + and sizeof(struct bug_entry), respectively */ #ifdef CONFIG_DEBUG_BUGVERBOSE + +#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE +# define HAVE_BUG_FUNCTION +# define __BUG_FUNC_PTR " .4byte %1 - .\n" +#else +# define __BUG_FUNC_PTR +#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */ + #define _EMIT_BUG_ENTRY \ ".section __bug_table,"aw"\n" \ "2: .4byte 1b - .\n" \ " .4byte %0 - .\n" \ - " .short %1, %2\n" \ - ".org 2b+%3\n" \ + __BUG_FUNC_PTR \ + " .short %2, %3\n" \ + ".org 2b+%4\n" \ ".previous\n" #else #define _EMIT_BUG_ENTRY \ ".section __bug_table,"aw"\n" \ "2: .4byte 1b - .\n" \ - " .short %2\n" \ - ".org 2b+%3\n" \ + " .short %3\n" \ + ".org 2b+%4\n" \ ".previous\n" #endif
+#ifdef HAVE_BUG_FUNCTION +# define __BUG_FUNC __func__ +#else +# define __BUG_FUNC NULL +#endif + #define BUG_ENTRY(insn, flags, ...) \ __asm__ __volatile__( \ "1: " insn "\n" \ _EMIT_BUG_ENTRY \ - : : "i" (__FILE__), "i" (__LINE__), \ + : : "i" (__FILE__), "i" (__BUG_FUNC), \ + "i" (__LINE__), \ "i" (flags), \ "i" (sizeof(struct bug_entry)), \ ##__VA_ARGS__) @@ -80,7 +99,7 @@ if (x) \ BUG(); \ } else { \ - BUG_ENTRY(PPC_TLNEI " %4, 0", 0, "r" ((__force long)(x))); \ + BUG_ENTRY(PPC_TLNEI " %5, 0", 0, "r" ((__force long)(x))); \ } \ } while (0)
@@ -90,7 +109,7 @@ if (__ret_warn_on) \ __WARN(); \ } else { \ - BUG_ENTRY(PPC_TLNEI " %4, 0", \ + BUG_ENTRY(PPC_TLNEI " %5, 0", \ BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), \ "r" (__ret_warn_on)); \ } \
On Thu, Mar 13, 2025 at 11:43:15AM +0000, Alessandro Carminati wrote:
Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such calls, not the existence of the warning backtrace.
Thanks for picking this series back up! I honestly thought this had already landed. :)
With CONFIG_KUNIT enabled, image size increase with this series applied is approximately 1%. The image size increase (and with it the functionality introduced by this series) can be avoided by disabling CONFIG_KUNIT_SUPPRESS_BACKTRACE.
Yeah, as with my prior review, I'm a fan of this. It makes a bunch of my very noisy tests much easier to deal with.
-Kees
Hi,
On Thu, Mar 13, 2025 at 10:17:49AM -0700, Kees Cook wrote:
On Thu, Mar 13, 2025 at 11:43:15AM +0000, Alessandro Carminati wrote:
Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such calls, not the existence of the warning backtrace.
Thanks for picking this series back up! I honestly thought this had already landed. :)
With CONFIG_KUNIT enabled, image size increase with this series applied is approximately 1%. The image size increase (and with it the functionality introduced by this series) can be avoided by disabling CONFIG_KUNIT_SUPPRESS_BACKTRACE.
Yeah, as with my prior review, I'm a fan of this. It makes a bunch of my very noisy tests much easier to deal with.
And for the record, we're also affected by this in DRM and would very much like to get it merged in one shape or another.
Maxime
On Thu, Mar 13, 2025 at 06:24:25PM +0100, Maxime Ripard wrote:
Yeah, as with my prior review, I'm a fan of this. It makes a bunch of my very noisy tests much easier to deal with.
And for the record, we're also affected by this in DRM and would very much like to get it merged in one shape or another.
I was unable to get maintainers of major architectures interested enough to provide feedback, and did not see a path forward. Maybe Alessandro has more success than me.
Guenter
On Thu, 13 Mar 2025 11:31:12 -0700 Guenter Roeck linux@roeck-us.net wrote:
On Thu, Mar 13, 2025 at 06:24:25PM +0100, Maxime Ripard wrote:
Yeah, as with my prior review, I'm a fan of this. It makes a bunch of my very noisy tests much easier to deal with.
And for the record, we're also affected by this in DRM and would very much like to get it merged in one shape or another.
I was unable to get maintainers of major architectures interested enough to provide feedback, and did not see a path forward. Maybe Alessandro has more success than me.
I'll put them into mm.git, to advance things a bit.
If someone wants to merge via a different tree, please speak up.
Hopefully the various arch maintainers will review at least their parts of the series.
On 3/13/25 16:05, Andrew Morton wrote:
On Thu, 13 Mar 2025 11:31:12 -0700 Guenter Roeck linux@roeck-us.net wrote:
On Thu, Mar 13, 2025 at 06:24:25PM +0100, Maxime Ripard wrote:
Yeah, as with my prior review, I'm a fan of this. It makes a bunch of my very noisy tests much easier to deal with.
And for the record, we're also affected by this in DRM and would very much like to get it merged in one shape or another.
I was unable to get maintainers of major architectures interested enough to provide feedback, and did not see a path forward. Maybe Alessandro has more success than me.
I'll put them into mm.git, to advance things a bit.
I haven't heard from kunit maintainers yet. This thread got lost in inbox due to travel.
David/Brendan/Rae, Okay to take this series?
Andrew, Okay to take this through your tree - this needs merging.
thanks, -- Shuah
On Fri, 28 Mar 2025 16:14:55 -0600 Shuah Khan skhan@linuxfoundation.org wrote:
On 3/13/25 16:05, Andrew Morton wrote:
On Thu, 13 Mar 2025 11:31:12 -0700 Guenter Roeck linux@roeck-us.net wrote:
On Thu, Mar 13, 2025 at 06:24:25PM +0100, Maxime Ripard wrote:
Yeah, as with my prior review, I'm a fan of this. It makes a bunch of my very noisy tests much easier to deal with.
And for the record, we're also affected by this in DRM and would very much like to get it merged in one shape or another.
I was unable to get maintainers of major architectures interested enough to provide feedback, and did not see a path forward. Maybe Alessandro has more success than me.
I'll put them into mm.git, to advance things a bit.
I haven't heard from kunit maintainers yet. This thread got lost in inbox due to travel.
David/Brendan/Rae, Okay to take this series?
Andrew, Okay to take this through your tree - this needs merging.
The review for 07/14 made me expect an update - perhaps tweak the asm constraints and add a comment. This can be addressed later, as long as we don't forget.
However https://lkml.kernel.org/r/20250324104702.12139E73-hca@linux.ibm.com needs to be addressed before a merge. The series in mm.git breaks the s390 build.
On Thu, Mar 13, 2025 at 06:24:25PM +0100, Maxime Ripard wrote:
Hi,
On Thu, Mar 13, 2025 at 10:17:49AM -0700, Kees Cook wrote:
On Thu, Mar 13, 2025 at 11:43:15AM +0000, Alessandro Carminati wrote:
Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such calls, not the existence of the warning backtrace.
Thanks for picking this series back up! I honestly thought this had already landed. :)
With CONFIG_KUNIT enabled, image size increase with this series applied is approximately 1%. The image size increase (and with it the functionality introduced by this series) can be avoided by disabling CONFIG_KUNIT_SUPPRESS_BACKTRACE.
Yeah, as with my prior review, I'm a fan of this. It makes a bunch of my very noisy tests much easier to deal with.
And for the record, we're also affected by this in DRM and would very much like to get it merged in one shape or another.
Here is another case: https://lore.kernel.org/all/20250328.Ahc0thi6CaiJ@digikod.net/
It would be very useful to have this feature merged. Without it, we may need to remove useful tests.
On Fri, Mar 28, 2025 at 11:38:23AM +0100, Mickaël Salaün wrote:
On Thu, Mar 13, 2025 at 06:24:25PM +0100, Maxime Ripard wrote:
Hi,
On Thu, Mar 13, 2025 at 10:17:49AM -0700, Kees Cook wrote:
On Thu, Mar 13, 2025 at 11:43:15AM +0000, Alessandro Carminati wrote:
Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such calls, not the existence of the warning backtrace.
Thanks for picking this series back up! I honestly thought this had already landed. :)
With CONFIG_KUNIT enabled, image size increase with this series applied is approximately 1%. The image size increase (and with it the functionality introduced by this series) can be avoided by disabling CONFIG_KUNIT_SUPPRESS_BACKTRACE.
Yeah, as with my prior review, I'm a fan of this. It makes a bunch of my very noisy tests much easier to deal with.
And for the record, we're also affected by this in DRM and would very much like to get it merged in one shape or another.
Here is another case: https://lore.kernel.org/all/20250328.Ahc0thi6CaiJ@digikod.net/
It would be very useful to have this feature merged. Without it, we may need to remove useful tests.
AFAIK, it's been merged in next a couple of weeks ago, so it should be in 6.15.
Maxime
On Thu, 13 Mar 2025 at 19:44, Alessandro Carminati acarmina@redhat.com wrote:
Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such 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. Support suppressing multiple backtraces while at the same time limiting changes to generic code to the absolute minimum. Architecture specific changes are kept at minimum by retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and CONFIG_KUNIT are enabled.
The first patch of the series introduces the necessary infrastructure. The second patch introduces support for counting suppressed backtraces. This capability is used in patch three to implement unit tests. Patch four documents the new API. The next two patches add support for suppressing backtraces in drm_rect and dev_addr_lists unit tests. These patches are intended to serve as examples for the use of the functionality introduced with this series. The remaining patches implement the necessary changes for all architectures with GENERIC_BUG support.
With CONFIG_KUNIT enabled, image size increase with this series applied is approximately 1%. The image size increase (and with it the functionality introduced by this series) can be avoided by disabling CONFIG_KUNIT_SUPPRESS_BACKTRACE.
This series is based on the RFC patch and subsequent discussion at https://patchwork.kernel.org/project/linux-kselftest/patch/02546e59-1afe-4b0... and offers a more comprehensive solution of the problem discussed there.
Design note: Function pointers are only added to the __bug_table section if both CONFIG_KUNIT_SUPPRESS_BACKTRACE and CONFIG_DEBUG_BUGVERBOSE are enabled to avoid image size increases if CONFIG_KUNIT is disabled. There would be some benefits to adding those pointers all the time (reduced complexity, ability to display function names in BUG/WARNING messages). That change, if desired, can be made later.
Checkpatch note: Remaining checkpatch errors and warnings were deliberately ignored. Some are triggered by matching coding style or by comments interpreted as code, others by assembler macros which are disliked by checkpatch. Suggestions for improvements are welcome.
Changes since RFC:
- Introduced CONFIG_KUNIT_SUPPRESS_BACKTRACE
- Minor cleanups and bug fixes
- Added support for all affected architectures
- Added support for counting suppressed warnings
- Added unit tests using those counters
- Added patch to suppress warning backtraces in dev_addr_lists tests
Changes since v1:
- Rebased to v6.9-rc1
- Added Tested-by:, Acked-by:, and Reviewed-by: tags [I retained those tags since there have been no functional changes]
- Introduced KUNIT_SUPPRESS_BACKTRACE configuration option, enabled by default.
Changes since v2:
- Rebased to v6.9-rc2
- Added comments to drm warning suppression explaining why it is needed.
- Added patch to move conditional code in arch/sh/include/asm/bug.h to avoid kerneldoc warning
- Added architecture maintainers to Cc: for architecture specific patches
- No functional changes
Changes since v3:
- Rebased to v6.14-rc6
- Dropped net: "kunit: Suppress lock warning noise at end of dev_addr_lists tests" since 3db3b62955cd6d73afde05a17d7e8e106695c3b9
- Added __kunit_ and KUNIT_ prefixes.
- Tested on interessed architectures.
Sorry: I also thought this had already landed.
I'm definitely in favour of us taking this, though agree that we definitely can't afford to break the s390x build.
I've (re-)reviewed the early patches as well, and am generally acking the series (though some of the architecture-specific patches are definitely beyond my expertise to review fully).
Acked-by: David Gow davidgow@google.com
Cheers, -- David
Guenter Roeck (14): bug/kunit: Core support for suppressing warning backtraces kunit: bug: Count suppressed warning backtraces kunit: Add test cases for backtrace warning suppression kunit: Add documentation for warning backtrace suppression API drm: Suppress intentional warning backtraces in scaling unit tests x86: Add support for suppressing warning backtraces arm64: Add support for suppressing warning backtraces loongarch: Add support for suppressing warning backtraces parisc: Add support for suppressing warning backtraces s390: Add support for suppressing warning backtraces sh: Add support for suppressing warning backtraces sh: Move defines needed for suppressing warning backtraces riscv: Add support for suppressing warning backtraces powerpc: Add support for suppressing warning backtraces
Documentation/dev-tools/kunit/usage.rst | 30 ++++++- arch/arm64/include/asm/asm-bug.h | 27 ++++-- arch/arm64/include/asm/bug.h | 8 +- arch/loongarch/include/asm/bug.h | 42 +++++++--- arch/parisc/include/asm/bug.h | 29 +++++-- arch/powerpc/include/asm/bug.h | 37 +++++++-- arch/riscv/include/asm/bug.h | 38 ++++++--- arch/s390/include/asm/bug.h | 17 +++- arch/sh/include/asm/bug.h | 28 ++++++- arch/x86/include/asm/bug.h | 21 +++-- drivers/gpu/drm/tests/drm_rect_test.c | 16 ++++ include/asm-generic/bug.h | 16 +++- include/kunit/bug.h | 56 +++++++++++++ include/kunit/test.h | 1 + include/linux/bug.h | 13 +++ lib/bug.c | 51 +++++++++++- lib/kunit/Kconfig | 9 ++ lib/kunit/Makefile | 7 +- lib/kunit/backtrace-suppression-test.c | 104 ++++++++++++++++++++++++ lib/kunit/bug.c | 42 ++++++++++ 20 files changed, 519 insertions(+), 73 deletions(-) create mode 100644 include/kunit/bug.h create mode 100644 lib/kunit/backtrace-suppression-test.c create mode 100644 lib/kunit/bug.c
-- 2.34.1
linux-kselftest-mirror@lists.linaro.org