| Message ID | 20200727022735.1435481-1-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | log: Allow LOG_DEBUG to always enable log output | expand |
On 7/27/20 4:27 AM, Simon Glass wrote: > At present if CONFIG_LOG enabled, putting LOG_DEBUG at the top of a file > (before log.h inclusion) causes _log() to be executed for every log() > call, regardless of the build- or run-time logging level. > > However there is no guarantee that the log record will actually be > displayed. If the current log level is lower than LOGL_DEBUG then it will > not be. > > Add a way to signal that the log record should always be displayed and > update log_passes_filters() to handle this. > > Signed-off-by: Simon Glass <sjg@chromium.org> Hello Simon, we have different debug levels: LOGL_DEBUG LOGL_DEBUG_CONTENT LOGL_DEBUG_IO If I understand you right, with your patch putting #define LOG_DEBUG at the top of the file enables all of these. This may be more than I want. Wouldn't it be more flexible if we could put something like: #define LOG_DEBUG LOGL_DEBUG at the top of the file if we want logging up to level 7 in this file. Best regards Heinrich Best regards > --- > > common/log.c | 11 ++++++++--- > doc/README.log | 10 ++++------ > include/log.h | 16 ++++++++++++---- > test/log/syslog_test.c | 2 +- > 4 files changed, 25 insertions(+), 14 deletions(-) > > diff --git a/common/log.c b/common/log.c > index 734d26de4a..5ff8245922 100644 > --- a/common/log.c > +++ b/common/log.c > @@ -156,16 +156,20 @@ static bool log_has_file(const char *file_list, const char *file) > static bool log_passes_filters(struct log_device *ldev, struct log_rec *rec) > { > struct log_filter *filt; > + int level = rec->level & LOGL_LEVEL_MASK; > + > + if (rec->force_debug && level <= LOGL_DEBUG) > + return true; > > /* If there are no filters, filter on the default log level */ > if (list_empty(&ldev->filter_head)) { > - if (rec->level > gd->default_log_level) > + if (level > gd->default_log_level) > return false; > return true; > } > > list_for_each_entry(filt, &ldev->filter_head, sibling_node) { > - if (rec->level > filt->max_level) > + if (level > filt->max_level) > continue; > if ((filt->flags & LOGFF_HAS_CAT) && > !log_has_cat(filt->cat_list, rec->cat)) > @@ -208,7 +212,8 @@ int _log(enum log_category_t cat, enum log_level_t level, const char *file, > va_list args; > > rec.cat = cat; > - rec.level = level; > + rec.level = level & LOGL_LEVEL_MASK; > + rec.force_debug = level & LOGL_FORCE_DEBUG; > rec.file = file; > rec.line = line; > rec.func = func; > diff --git a/doc/README.log b/doc/README.log > index ba838824a9..554e99ca4c 100644 > --- a/doc/README.log > +++ b/doc/README.log > @@ -77,12 +77,10 @@ Sometimes it is useful to turn on logging just in one file. You can use this: > > #define LOG_DEBUG > > -to enable building in of all logging statements in a single file. Put it at > -the top of the file, before any #includes. > - > -To actually get U-Boot to output this you need to also set the default logging > -level - e.g. set CONFIG_LOG_DEFAULT_LEVEL to 7 (LOGL_DEBUG) or more. Otherwise > -debug output is suppressed and will not be generated. > +to enable building in of all debug logging statements in a single file. Put it > +at the top of the file, before any #includes. This overrides any log-level > +setting in U-Boot, including CONFIG_LOG_DEFAULT_LEVEL, but just for that file. > +All logging statements, up to and including LOGL_DEBUG, will be displayed. > > > Convenience functions > diff --git a/include/log.h b/include/log.h > index 2859ce1f2e..63052f74eb 100644 > --- a/include/log.h > +++ b/include/log.h > @@ -33,6 +33,9 @@ enum log_level_t { > LOGL_COUNT, > LOGL_NONE, > > + LOGL_LEVEL_MASK = 0xf, /* Mask for valid log levels */ > + LOGL_FORCE_DEBUG = 0x10, /* Mask to force output due to LOG_DEBUG */ > + > LOGL_FIRST = LOGL_EMERG, > LOGL_MAX = LOGL_DEBUG_IO, > }; > @@ -133,7 +136,7 @@ static inline int _log_nop(enum log_category_t cat, enum log_level_t level, > > #if CONFIG_IS_ENABLED(LOG) > #ifdef LOG_DEBUG > -#define _LOG_DEBUG 1 > +#define _LOG_DEBUG LOGL_FORCE_DEBUG > #else > #define _LOG_DEBUG 0 > #endif > @@ -141,9 +144,9 @@ static inline int _log_nop(enum log_category_t cat, enum log_level_t level, > /* Emit a log record if the level is less that the maximum */ > #define log(_cat, _level, _fmt, _args...) ({ \ > int _l = _level; \ > - if (CONFIG_IS_ENABLED(LOG) && (_l <= _LOG_MAX_LEVEL || _LOG_DEBUG)) \ > - _log((enum log_category_t)(_cat), _l, __FILE__, __LINE__, \ > - __func__, \ > + if (CONFIG_IS_ENABLED(LOG) && (_LOG_DEBUG || _l <= _LOG_MAX_LEVEL)) \ > + _log((enum log_category_t)(_cat), _l | _LOG_DEBUG, __FILE__, \ > + __LINE__, __func__, \ > pr_fmt(_fmt), ##_args); \ > }) > #else > @@ -279,8 +282,12 @@ void __assert_fail(const char *assertion, const char *file, unsigned int line, > * Memebers marked as 'allocated' are allocated (e.g. via strdup()) by the log > * system. > * > + * TODO(sjg@chromium.org): Compress this struct down a bit to reduce space, e.g. > + * a single u32 for cat, level, line and force_debug > + * > * @cat: Category, representing a uclass or part of U-Boot > * @level: Severity level, less severe is higher > + * @force_debug: Force output of debug > * @file: Name of file where the log record was generated (not allocated) > * @line: Line number where the log record was generated > * @func: Function where the log record was generated (not allocated) > @@ -289,6 +296,7 @@ void __assert_fail(const char *assertion, const char *file, unsigned int line, > struct log_rec { > enum log_category_t cat; > enum log_level_t level; > + bool force_debug; > const char *file; > int line; > const char *func; > diff --git a/test/log/syslog_test.c b/test/log/syslog_test.c > index 120a8b2537..df239d1e8d 100644 > --- a/test/log/syslog_test.c > +++ b/test/log/syslog_test.c > @@ -276,7 +276,7 @@ static int log_test_syslog_nodebug(struct unit_test_state *uts) > sandbox_eth_set_tx_handler(0, sb_log_tx_handler); > /* Used by ut_assert macros in the tx_handler */ > sandbox_eth_set_priv(0, &env); > - log_debug("testing %s\n", "log_debug"); > + log_content("testing %s\n", "log_debug"); > sandbox_eth_set_tx_handler(0, NULL); > /* Check that the callback function was not called */ > ut_assertnonnull(env.expected); >
Hi Heinrich, On Mon, 27 Jul 2020 at 00:15, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 7/27/20 4:27 AM, Simon Glass wrote: > > At present if CONFIG_LOG enabled, putting LOG_DEBUG at the top of a file > > (before log.h inclusion) causes _log() to be executed for every log() > > call, regardless of the build- or run-time logging level. > > > > However there is no guarantee that the log record will actually be > > displayed. If the current log level is lower than LOGL_DEBUG then it will > > not be. > > > > Add a way to signal that the log record should always be displayed and > > update log_passes_filters() to handle this. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > Hello Simon, > > we have different debug levels: > > LOGL_DEBUG > LOGL_DEBUG_CONTENT > LOGL_DEBUG_IO > > If I understand you right, with your patch putting #define LOG_DEBUG at > the top of the file enables all of these. This may be more than I want. > > Wouldn't it be more flexible if we could put something like: > > #define LOG_DEBUG LOGL_DEBUG > > at the top of the file if we want logging up to level 7 in this file. Actually the way I implemented it, it only enables update LOGL_DEBUG. Certainly your idea is more flexible. I wonder if we could make the level optional, so: #define LOG_DEBUG means #define LOG_DEBUG LOGL_DEBUG Regards, Simon
On Sun, Jul 26, 2020 at 08:27:35PM -0600, Simon Glass wrote: > At present if CONFIG_LOG enabled, putting LOG_DEBUG at the top of a file > (before log.h inclusion) causes _log() to be executed for every log() > call, regardless of the build- or run-time logging level. > > However there is no guarantee that the log record will actually be > displayed. If the current log level is lower than LOGL_DEBUG then it will > not be. > > Add a way to signal that the log record should always be displayed and > update log_passes_filters() to handle this. > > Signed-off-by: Simon Glass <sjg@chromium.org> This exposes an underlying problem with LOG and clang I believe: https://gitlab.denx.de/u-boot/u-boot/-/jobs/135789
On 05.08.20 14:18, Tom Rini wrote: > On Sun, Jul 26, 2020 at 08:27:35PM -0600, Simon Glass wrote: > >> At present if CONFIG_LOG enabled, putting LOG_DEBUG at the top of a file >> (before log.h inclusion) causes _log() to be executed for every log() >> call, regardless of the build- or run-time logging level. >> >> However there is no guarantee that the log record will actually be >> displayed. If the current log level is lower than LOGL_DEBUG then it will >> not be. >> >> Add a way to signal that the log record should always be displayed and >> update log_passes_filters() to handle this. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> > > This exposes an underlying problem with LOG and clang I believe: > https://gitlab.denx.de/u-boot/u-boot/-/jobs/135789 > include/log.h:147:44: note: expanded from macro 'log' if (CONFIG_IS_ENABLED(LOG) && (_LOG_DEBUG || _l <= _LOG_MAX_LEVEL)) \ ^ drivers/misc/p2sb_emul.c:197:10: error: converting the enum constant to a boolean [-Werror,-Wint-in-bool-context] This seems to be a Clang bug. _LOG_DEBUG is not an enum: #if CONFIG_IS_ENABLED(LOG) #ifdef LOG_DEBUG #define _LOG_DEBUG 1 #else #define _LOG_DEBUG 0 #endif So there seems to be a bug in the Clang you used. Compiling with clang on Debian Bullseye does not show the problem: make HOSTCC=clang sandbox_defconfig make HOSTCC=clang CC=clang -j8 clang version 9.0.1-13 LLVM version 9.0.1 Which Clang version did you use? This is the change that added the test: https://reviews.llvm.org/D63082 -Wint-in-bool-context seems to be new in Clang 10. All over the U-Boot code we assume that a non-zero integer is true. Do we really want to enable -Wint-in-bool-context? Best regards Heinrich
On Wed, Aug 05, 2020 at 02:54:05PM +0200, Heinrich Schuchardt wrote: > On 05.08.20 14:18, Tom Rini wrote: > > On Sun, Jul 26, 2020 at 08:27:35PM -0600, Simon Glass wrote: > > > >> At present if CONFIG_LOG enabled, putting LOG_DEBUG at the top of a file > >> (before log.h inclusion) causes _log() to be executed for every log() > >> call, regardless of the build- or run-time logging level. > >> > >> However there is no guarantee that the log record will actually be > >> displayed. If the current log level is lower than LOGL_DEBUG then it will > >> not be. > >> > >> Add a way to signal that the log record should always be displayed and > >> update log_passes_filters() to handle this. > >> > >> Signed-off-by: Simon Glass <sjg@chromium.org> > > > > This exposes an underlying problem with LOG and clang I believe: > > https://gitlab.denx.de/u-boot/u-boot/-/jobs/135789 > > > > include/log.h:147:44: note: expanded from macro 'log' > if (CONFIG_IS_ENABLED(LOG) && (_LOG_DEBUG || _l <= > _LOG_MAX_LEVEL)) \ > ^ > drivers/misc/p2sb_emul.c:197:10: error: converting the enum constant to > a boolean [-Werror,-Wint-in-bool-context] > > This seems to be a Clang bug. _LOG_DEBUG is not an enum: > > #if CONFIG_IS_ENABLED(LOG) > #ifdef LOG_DEBUG > #define _LOG_DEBUG 1 > #else > #define _LOG_DEBUG 0 > #endif > > So there seems to be a bug in the Clang you used. > > Compiling with clang on Debian Bullseye does not show the problem: > > make HOSTCC=clang sandbox_defconfig > make HOSTCC=clang CC=clang -j8 > > clang version 9.0.1-13 > LLVM version 9.0.1 > > Which Clang version did you use? > > This is the change that added the test: > https://reviews.llvm.org/D63082 > > -Wint-in-bool-context seems to be new in Clang 10. > > All over the U-Boot code we assume that a non-zero integer is true. Do > we really want to enable -Wint-in-bool-context? I'm using the official Clang 10 stable builds.
On 05.08.20 14:56, Tom Rini wrote: > On Wed, Aug 05, 2020 at 02:54:05PM +0200, Heinrich Schuchardt wrote: >> On 05.08.20 14:18, Tom Rini wrote: >>> On Sun, Jul 26, 2020 at 08:27:35PM -0600, Simon Glass wrote: >>> >>>> At present if CONFIG_LOG enabled, putting LOG_DEBUG at the top of a file >>>> (before log.h inclusion) causes _log() to be executed for every log() >>>> call, regardless of the build- or run-time logging level. >>>> >>>> However there is no guarantee that the log record will actually be >>>> displayed. If the current log level is lower than LOGL_DEBUG then it will >>>> not be. >>>> >>>> Add a way to signal that the log record should always be displayed and >>>> update log_passes_filters() to handle this. >>>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> >>> This exposes an underlying problem with LOG and clang I believe: >>> https://gitlab.denx.de/u-boot/u-boot/-/jobs/135789 >>> >> >> include/log.h:147:44: note: expanded from macro 'log' >> if (CONFIG_IS_ENABLED(LOG) && (_LOG_DEBUG || _l <= >> _LOG_MAX_LEVEL)) \ >> ^ >> drivers/misc/p2sb_emul.c:197:10: error: converting the enum constant to >> a boolean [-Werror,-Wint-in-bool-context] >> >> This seems to be a Clang bug. _LOG_DEBUG is not an enum: >> >> #if CONFIG_IS_ENABLED(LOG) >> #ifdef LOG_DEBUG >> #define _LOG_DEBUG 1 >> #else >> #define _LOG_DEBUG 0 >> #endif >> >> So there seems to be a bug in the Clang you used. >> >> Compiling with clang on Debian Bullseye does not show the problem: >> >> make HOSTCC=clang sandbox_defconfig >> make HOSTCC=clang CC=clang -j8 >> >> clang version 9.0.1-13 >> LLVM version 9.0.1 >> >> Which Clang version did you use? >> >> This is the change that added the test: >> https://reviews.llvm.org/D63082 >> >> -Wint-in-bool-context seems to be new in Clang 10. >> >> All over the U-Boot code we assume that a non-zero integer is true. Do >> we really want to enable -Wint-in-bool-context? > > I'm using the official Clang 10 stable builds. > Do you really want to forbid using integers as booleans (-Wint-in-bool-context)? Best regards Heinrich
On Wed, Aug 05, 2020 at 08:31:55PM +0200, Heinrich Schuchardt wrote: > On 05.08.20 14:56, Tom Rini wrote: > > On Wed, Aug 05, 2020 at 02:54:05PM +0200, Heinrich Schuchardt wrote: > >> On 05.08.20 14:18, Tom Rini wrote: > >>> On Sun, Jul 26, 2020 at 08:27:35PM -0600, Simon Glass wrote: > >>> > >>>> At present if CONFIG_LOG enabled, putting LOG_DEBUG at the top of a file > >>>> (before log.h inclusion) causes _log() to be executed for every log() > >>>> call, regardless of the build- or run-time logging level. > >>>> > >>>> However there is no guarantee that the log record will actually be > >>>> displayed. If the current log level is lower than LOGL_DEBUG then it will > >>>> not be. > >>>> > >>>> Add a way to signal that the log record should always be displayed and > >>>> update log_passes_filters() to handle this. > >>>> > >>>> Signed-off-by: Simon Glass <sjg@chromium.org> > >>> > >>> This exposes an underlying problem with LOG and clang I believe: > >>> https://gitlab.denx.de/u-boot/u-boot/-/jobs/135789 > >>> > >> > >> include/log.h:147:44: note: expanded from macro 'log' > >> if (CONFIG_IS_ENABLED(LOG) && (_LOG_DEBUG || _l <= > >> _LOG_MAX_LEVEL)) \ > >> ^ > >> drivers/misc/p2sb_emul.c:197:10: error: converting the enum constant to > >> a boolean [-Werror,-Wint-in-bool-context] > >> > >> This seems to be a Clang bug. _LOG_DEBUG is not an enum: > >> > >> #if CONFIG_IS_ENABLED(LOG) > >> #ifdef LOG_DEBUG > >> #define _LOG_DEBUG 1 > >> #else > >> #define _LOG_DEBUG 0 > >> #endif > >> > >> So there seems to be a bug in the Clang you used. > >> > >> Compiling with clang on Debian Bullseye does not show the problem: > >> > >> make HOSTCC=clang sandbox_defconfig > >> make HOSTCC=clang CC=clang -j8 > >> > >> clang version 9.0.1-13 > >> LLVM version 9.0.1 > >> > >> Which Clang version did you use? > >> > >> This is the change that added the test: > >> https://reviews.llvm.org/D63082 > >> > >> -Wint-in-bool-context seems to be new in Clang 10. > >> > >> All over the U-Boot code we assume that a non-zero integer is true. Do > >> we really want to enable -Wint-in-bool-context? > > > > I'm using the official Clang 10 stable builds. > > > > Do you really want to forbid using integers as booleans > (-Wint-in-bool-context)? So, interesting. The Linux kernel isn't disabling this warning. It's mentioned in two commits, one of which is "clang found a bug here", of which this is not the case. The other is more like ours: commit 968e5170939662341242812b9c82ef51cf140a33 Author: Nathan Chancellor <natechancellor@gmail.com> Date: Thu Sep 26 09:22:59 2019 -0700 tracing: Fix clang -Wint-in-bool-context warnings in IF_ASSIGN macro After r372664 in clang, the IF_ASSIGN macro causes a couple hundred warnings along the lines of: kernel/trace/trace_output.c:1331:2: warning: converting the enum constant to a boolean [-Wint-in-bool-context] kernel/trace/trace.h:409:3: note: expanded from macro 'trace_assign_type' IF_ASSIGN(var, ent, struct ftrace_graph_ret_entry, ^ kernel/trace/trace.h:371:14: note: expanded from macro 'IF_ASSIGN' WARN_ON(id && (entry)->type != id); \ ^ 264 warnings generated. This warning can catch issues with constructs like: if (state == A || B) where the developer really meant: if (state == A || state == B) This is currently the only occurrence of the warning in the kernel tree across defconfig, allyesconfig, allmodconfig for arm32, arm64, and x86_64. Add the implicit '!= 0' to the WARN_ON statement to fix the warnings and find potential issues in the future. Link: https://github.com/llvm/llvm-project/commit/28b38c277a2941e9e891b2db30652cfd962f070b Link: https://github.com/ClangBuiltLinux/linux/issues/686 Link: http://lkml.kernel.org/r/20190926162258.466321-1-natechancellor@gmail.com Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> Which is like our case, and reworking the test to be explicit. I lean towards that.
Hi Tom, On Wed, 5 Aug 2020 at 12:37, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Aug 05, 2020 at 08:31:55PM +0200, Heinrich Schuchardt wrote: > > On 05.08.20 14:56, Tom Rini wrote: > > > On Wed, Aug 05, 2020 at 02:54:05PM +0200, Heinrich Schuchardt wrote: > > >> On 05.08.20 14:18, Tom Rini wrote: > > >>> On Sun, Jul 26, 2020 at 08:27:35PM -0600, Simon Glass wrote: > > >>> > > >>>> At present if CONFIG_LOG enabled, putting LOG_DEBUG at the top of a file > > >>>> (before log.h inclusion) causes _log() to be executed for every log() > > >>>> call, regardless of the build- or run-time logging level. > > >>>> > > >>>> However there is no guarantee that the log record will actually be > > >>>> displayed. If the current log level is lower than LOGL_DEBUG then it will > > >>>> not be. > > >>>> > > >>>> Add a way to signal that the log record should always be displayed and > > >>>> update log_passes_filters() to handle this. > > >>>> > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org> > > >>> > > >>> This exposes an underlying problem with LOG and clang I believe: > > >>> https://gitlab.denx.de/u-boot/u-boot/-/jobs/135789 > > >>> > > >> > > >> include/log.h:147:44: note: expanded from macro 'log' > > >> if (CONFIG_IS_ENABLED(LOG) && (_LOG_DEBUG || _l <= > > >> _LOG_MAX_LEVEL)) \ > > >> ^ > > >> drivers/misc/p2sb_emul.c:197:10: error: converting the enum constant to > > >> a boolean [-Werror,-Wint-in-bool-context] > > >> > > >> This seems to be a Clang bug. _LOG_DEBUG is not an enum: > > >> > > >> #if CONFIG_IS_ENABLED(LOG) > > >> #ifdef LOG_DEBUG > > >> #define _LOG_DEBUG 1 > > >> #else > > >> #define _LOG_DEBUG 0 > > >> #endif > > >> > > >> So there seems to be a bug in the Clang you used. > > >> > > >> Compiling with clang on Debian Bullseye does not show the problem: > > >> > > >> make HOSTCC=clang sandbox_defconfig > > >> make HOSTCC=clang CC=clang -j8 > > >> > > >> clang version 9.0.1-13 > > >> LLVM version 9.0.1 > > >> > > >> Which Clang version did you use? > > >> > > >> This is the change that added the test: > > >> https://reviews.llvm.org/D63082 > > >> > > >> -Wint-in-bool-context seems to be new in Clang 10. > > >> > > >> All over the U-Boot code we assume that a non-zero integer is true. Do > > >> we really want to enable -Wint-in-bool-context? > > > > > > I'm using the official Clang 10 stable builds. > > > > > > > Do you really want to forbid using integers as booleans > > (-Wint-in-bool-context)? > > So, interesting. The Linux kernel isn't disabling this warning. It's > mentioned in two commits, one of which is "clang found a bug here", of > which this is not the case. The other is more like ours: > commit 968e5170939662341242812b9c82ef51cf140a33 > Author: Nathan Chancellor <natechancellor@gmail.com> > Date: Thu Sep 26 09:22:59 2019 -0700 > > tracing: Fix clang -Wint-in-bool-context warnings in IF_ASSIGN macro > > After r372664 in clang, the IF_ASSIGN macro causes a couple hundred > warnings along the lines of: > > kernel/trace/trace_output.c:1331:2: warning: converting the enum > constant to a boolean [-Wint-in-bool-context] > kernel/trace/trace.h:409:3: note: expanded from macro > 'trace_assign_type' > IF_ASSIGN(var, ent, struct ftrace_graph_ret_entry, > ^ > kernel/trace/trace.h:371:14: note: expanded from macro 'IF_ASSIGN' > WARN_ON(id && (entry)->type != id); \ > ^ > 264 warnings generated. > > This warning can catch issues with constructs like: > > if (state == A || B) > > where the developer really meant: > > if (state == A || state == B) > > This is currently the only occurrence of the warning in the kernel > tree across defconfig, allyesconfig, allmodconfig for arm32, arm64, > and x86_64. Add the implicit '!= 0' to the WARN_ON statement to fix > the warnings and find potential issues in the future. > > Link: https://github.com/llvm/llvm-project/commit/28b38c277a2941e9e891b2db30652cfd962f070b > Link: https://github.com/ClangBuiltLinux/linux/issues/686 > Link: http://lkml.kernel.org/r/20190926162258.466321-1-natechancellor@gmail.com > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > > Which is like our case, and reworking the test to be explicit. I lean > towards that. Oh dear I really want to vote against that. if (x) should allow C to consider x to be a boolean. I am hitting this in Zephyr and feels like it is undoing a long-standing C feature, with major disruption, for no benefit I can detect. Hopefully I am misunderstanding what -Wint-in-bool-context means, and it just applies to enums? Regards, Simon
On Wed, Aug 05, 2020 at 12:56:17PM -0600, Simon Glass wrote: > Hi Tom, > > On Wed, 5 Aug 2020 at 12:37, Tom Rini <trini@konsulko.com> wrote: > > > > On Wed, Aug 05, 2020 at 08:31:55PM +0200, Heinrich Schuchardt wrote: > > > On 05.08.20 14:56, Tom Rini wrote: > > > > On Wed, Aug 05, 2020 at 02:54:05PM +0200, Heinrich Schuchardt wrote: > > > >> On 05.08.20 14:18, Tom Rini wrote: > > > >>> On Sun, Jul 26, 2020 at 08:27:35PM -0600, Simon Glass wrote: > > > >>> > > > >>>> At present if CONFIG_LOG enabled, putting LOG_DEBUG at the top of a file > > > >>>> (before log.h inclusion) causes _log() to be executed for every log() > > > >>>> call, regardless of the build- or run-time logging level. > > > >>>> > > > >>>> However there is no guarantee that the log record will actually be > > > >>>> displayed. If the current log level is lower than LOGL_DEBUG then it will > > > >>>> not be. > > > >>>> > > > >>>> Add a way to signal that the log record should always be displayed and > > > >>>> update log_passes_filters() to handle this. > > > >>>> > > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org> > > > >>> > > > >>> This exposes an underlying problem with LOG and clang I believe: > > > >>> https://gitlab.denx.de/u-boot/u-boot/-/jobs/135789 > > > >>> > > > >> > > > >> include/log.h:147:44: note: expanded from macro 'log' > > > >> if (CONFIG_IS_ENABLED(LOG) && (_LOG_DEBUG || _l <= > > > >> _LOG_MAX_LEVEL)) \ > > > >> ^ > > > >> drivers/misc/p2sb_emul.c:197:10: error: converting the enum constant to > > > >> a boolean [-Werror,-Wint-in-bool-context] > > > >> > > > >> This seems to be a Clang bug. _LOG_DEBUG is not an enum: > > > >> > > > >> #if CONFIG_IS_ENABLED(LOG) > > > >> #ifdef LOG_DEBUG > > > >> #define _LOG_DEBUG 1 > > > >> #else > > > >> #define _LOG_DEBUG 0 > > > >> #endif > > > >> > > > >> So there seems to be a bug in the Clang you used. > > > >> > > > >> Compiling with clang on Debian Bullseye does not show the problem: > > > >> > > > >> make HOSTCC=clang sandbox_defconfig > > > >> make HOSTCC=clang CC=clang -j8 > > > >> > > > >> clang version 9.0.1-13 > > > >> LLVM version 9.0.1 > > > >> > > > >> Which Clang version did you use? > > > >> > > > >> This is the change that added the test: > > > >> https://reviews.llvm.org/D63082 > > > >> > > > >> -Wint-in-bool-context seems to be new in Clang 10. > > > >> > > > >> All over the U-Boot code we assume that a non-zero integer is true. Do > > > >> we really want to enable -Wint-in-bool-context? > > > > > > > > I'm using the official Clang 10 stable builds. > > > > > > > > > > Do you really want to forbid using integers as booleans > > > (-Wint-in-bool-context)? > > > > So, interesting. The Linux kernel isn't disabling this warning. It's > > mentioned in two commits, one of which is "clang found a bug here", of > > which this is not the case. The other is more like ours: > > commit 968e5170939662341242812b9c82ef51cf140a33 > > Author: Nathan Chancellor <natechancellor@gmail.com> > > Date: Thu Sep 26 09:22:59 2019 -0700 > > > > tracing: Fix clang -Wint-in-bool-context warnings in IF_ASSIGN macro > > > > After r372664 in clang, the IF_ASSIGN macro causes a couple hundred > > warnings along the lines of: > > > > kernel/trace/trace_output.c:1331:2: warning: converting the enum > > constant to a boolean [-Wint-in-bool-context] > > kernel/trace/trace.h:409:3: note: expanded from macro > > 'trace_assign_type' > > IF_ASSIGN(var, ent, struct ftrace_graph_ret_entry, > > ^ > > kernel/trace/trace.h:371:14: note: expanded from macro 'IF_ASSIGN' > > WARN_ON(id && (entry)->type != id); \ > > ^ > > 264 warnings generated. > > > > This warning can catch issues with constructs like: > > > > if (state == A || B) > > > > where the developer really meant: > > > > if (state == A || state == B) > > > > This is currently the only occurrence of the warning in the kernel > > tree across defconfig, allyesconfig, allmodconfig for arm32, arm64, > > and x86_64. Add the implicit '!= 0' to the WARN_ON statement to fix > > the warnings and find potential issues in the future. > > > > Link: https://github.com/llvm/llvm-project/commit/28b38c277a2941e9e891b2db30652cfd962f070b > > Link: https://github.com/ClangBuiltLinux/linux/issues/686 > > Link: http://lkml.kernel.org/r/20190926162258.466321-1-natechancellor@gmail.com > > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > > > > Which is like our case, and reworking the test to be explicit. I lean > > towards that. > > Oh dear I really want to vote against that. > > if (x) > > should allow C to consider x to be a boolean. I am hitting this in > Zephyr and feels like it is undoing a long-standing C feature, with > major disruption, for no benefit I can detect. > > Hopefully I am misunderstanding what -Wint-in-bool-context means, and > it just applies to enums? Yes, it's not the simple case, it's the complex case as noted in the kernel commit message. Our change I believe would be: diff --git a/include/log.h b/include/log.h index 2859ce1f2e72..91ca2e0523f7 100644 --- a/include/log.h +++ b/include/log.h @@ -141,7 +141,7 @@ static inline int _log_nop(enum log_category_t cat, enum log_level_t level, /* Emit a log record if the level is less that the maximum */ #define log(_cat, _level, _fmt, _args...) ({ \ int _l = _level; \ - if (CONFIG_IS_ENABLED(LOG) && (_l <= _LOG_MAX_LEVEL || _LOG_DEBUG)) \ + if (CONFIG_IS_ENABLED(LOG) && (_l <= _LOG_MAX_LEVEL || _LOG_DEBUG == 1)) \ _log((enum log_category_t)(_cat), _l, __FILE__, __LINE__, \ __func__, \ pr_fmt(_fmt), ##_args); \ It's not a new warning from clang (We've been on LLVM-10 since February), it's only that the logging code is now being run through it via CI.
Am 5. August 2020 21:03:28 MESZ schrieb Tom Rini <trini@konsulko.com>: >On Wed, Aug 05, 2020 at 12:56:17PM -0600, Simon Glass wrote: >> Hi Tom, >> >> On Wed, 5 Aug 2020 at 12:37, Tom Rini <trini@konsulko.com> wrote: >> > >> > On Wed, Aug 05, 2020 at 08:31:55PM +0200, Heinrich Schuchardt >wrote: >> > > On 05.08.20 14:56, Tom Rini wrote: >> > > > On Wed, Aug 05, 2020 at 02:54:05PM +0200, Heinrich Schuchardt >wrote: >> > > >> On 05.08.20 14:18, Tom Rini wrote: >> > > >>> On Sun, Jul 26, 2020 at 08:27:35PM -0600, Simon Glass wrote: >> > > >>> >> > > >>>> At present if CONFIG_LOG enabled, putting LOG_DEBUG at the >top of a file >> > > >>>> (before log.h inclusion) causes _log() to be executed for >every log() >> > > >>>> call, regardless of the build- or run-time logging level. >> > > >>>> >> > > >>>> However there is no guarantee that the log record will >actually be >> > > >>>> displayed. If the current log level is lower than LOGL_DEBUG >then it will >> > > >>>> not be. >> > > >>>> >> > > >>>> Add a way to signal that the log record should always be >displayed and >> > > >>>> update log_passes_filters() to handle this. >> > > >>>> >> > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >> > > >>> >> > > >>> This exposes an underlying problem with LOG and clang I >believe: >> > > >>> https://gitlab.denx.de/u-boot/u-boot/-/jobs/135789 >> > > >>> >> > > >> >> > > >> include/log.h:147:44: note: expanded from macro 'log' >> > > >> if (CONFIG_IS_ENABLED(LOG) && (_LOG_DEBUG || _l <= >> > > >> _LOG_MAX_LEVEL)) \ >> > > >> ^ >> > > >> drivers/misc/p2sb_emul.c:197:10: error: converting the enum >constant to >> > > >> a boolean [-Werror,-Wint-in-bool-context] >> > > >> >> > > >> This seems to be a Clang bug. _LOG_DEBUG is not an enum: >> > > >> >> > > >> #if CONFIG_IS_ENABLED(LOG) >> > > >> #ifdef LOG_DEBUG >> > > >> #define _LOG_DEBUG 1 >> > > >> #else >> > > >> #define _LOG_DEBUG 0 >> > > >> #endif >> > > >> >> > > >> So there seems to be a bug in the Clang you used. >> > > >> >> > > >> Compiling with clang on Debian Bullseye does not show the >problem: >> > > >> >> > > >> make HOSTCC=clang sandbox_defconfig >> > > >> make HOSTCC=clang CC=clang -j8 >> > > >> >> > > >> clang version 9.0.1-13 >> > > >> LLVM version 9.0.1 >> > > >> >> > > >> Which Clang version did you use? >> > > >> >> > > >> This is the change that added the test: >> > > >> https://reviews.llvm.org/D63082 >> > > >> >> > > >> -Wint-in-bool-context seems to be new in Clang 10. >> > > >> >> > > >> All over the U-Boot code we assume that a non-zero integer is >true. Do >> > > >> we really want to enable -Wint-in-bool-context? >> > > > >> > > > I'm using the official Clang 10 stable builds. >> > > > >> > > >> > > Do you really want to forbid using integers as booleans >> > > (-Wint-in-bool-context)? >> > >> > So, interesting. The Linux kernel isn't disabling this warning. >It's >> > mentioned in two commits, one of which is "clang found a bug here", >of >> > which this is not the case. The other is more like ours: >> > commit 968e5170939662341242812b9c82ef51cf140a33 >> > Author: Nathan Chancellor <natechancellor@gmail.com> >> > Date: Thu Sep 26 09:22:59 2019 -0700 >> > >> > tracing: Fix clang -Wint-in-bool-context warnings in IF_ASSIGN >macro >> > >> > After r372664 in clang, the IF_ASSIGN macro causes a couple >hundred >> > warnings along the lines of: >> > >> > kernel/trace/trace_output.c:1331:2: warning: converting the >enum >> > constant to a boolean [-Wint-in-bool-context] >> > kernel/trace/trace.h:409:3: note: expanded from macro >> > 'trace_assign_type' >> > IF_ASSIGN(var, ent, struct >ftrace_graph_ret_entry, >> > ^ >> > kernel/trace/trace.h:371:14: note: expanded from macro >'IF_ASSIGN' >> > WARN_ON(id && (entry)->type != id); \ >> > ^ >> > 264 warnings generated. >> > >> > This warning can catch issues with constructs like: >> > >> > if (state == A || B) >> > >> > where the developer really meant: >> > >> > if (state == A || state == B) >> > >> > This is currently the only occurrence of the warning in the >kernel >> > tree across defconfig, allyesconfig, allmodconfig for arm32, >arm64, >> > and x86_64. Add the implicit '!= 0' to the WARN_ON statement to >fix >> > the warnings and find potential issues in the future. >> > >> > Link: >https://github.com/llvm/llvm-project/commit/28b38c277a2941e9e891b2db30652cfd962f070b >> > Link: https://github.com/ClangBuiltLinux/linux/issues/686 >> > Link: >http://lkml.kernel.org/r/20190926162258.466321-1-natechancellor@gmail.com >> > >> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> >> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> >> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> >> > >> > Which is like our case, and reworking the test to be explicit. I >lean >> > towards that. >> >> Oh dear I really want to vote against that. >> >> if (x) >> >> should allow C to consider x to be a boolean. I am hitting this in >> Zephyr and feels like it is undoing a long-standing C feature, with >> major disruption, for no benefit I can detect. >> >> Hopefully I am misunderstanding what -Wint-in-bool-context means, and >> it just applies to enums? > >Yes, it's not the simple case, it's the complex case as noted in the >kernel commit message. Our change I believe would be: > >diff --git a/include/log.h b/include/log.h >index 2859ce1f2e72..91ca2e0523f7 100644 >--- a/include/log.h >+++ b/include/log.h >@@ -141,7 +141,7 @@ static inline int _log_nop(enum log_category_t cat, >enum log_level_t level, > /* Emit a log record if the level is less that the maximum */ > #define log(_cat, _level, _fmt, _args...) ({ \ > int _l = _level; \ >- if (CONFIG_IS_ENABLED(LOG) && (_l <= _LOG_MAX_LEVEL || _LOG_DEBUG)) \ >+ if (CONFIG_IS_ENABLED(LOG) && (_l <= _LOG_MAX_LEVEL || _LOG_DEBUG == >1)) \ Why wouldn't we define _LOG_DEBUG as true or false? Shouldn't clang know that true is boolean? > _log((enum log_category_t)(_cat), _l, __FILE__, __LINE__, \ > __func__, \ > pr_fmt(_fmt), ##_args); \ > >It's not a new warning from clang (We've been on LLVM-10 since >February), it's only that the logging code is now being run through it >via CI.
On Wed, Aug 05, 2020 at 09:08:40PM +0200, Heinrich Schuchardt wrote: > Am 5. August 2020 21:03:28 MESZ schrieb Tom Rini <trini@konsulko.com>: > >On Wed, Aug 05, 2020 at 12:56:17PM -0600, Simon Glass wrote: > >> Hi Tom, > >> > >> On Wed, 5 Aug 2020 at 12:37, Tom Rini <trini@konsulko.com> wrote: > >> > > >> > On Wed, Aug 05, 2020 at 08:31:55PM +0200, Heinrich Schuchardt > >wrote: > >> > > On 05.08.20 14:56, Tom Rini wrote: > >> > > > On Wed, Aug 05, 2020 at 02:54:05PM +0200, Heinrich Schuchardt > >wrote: > >> > > >> On 05.08.20 14:18, Tom Rini wrote: > >> > > >>> On Sun, Jul 26, 2020 at 08:27:35PM -0600, Simon Glass wrote: > >> > > >>> > >> > > >>>> At present if CONFIG_LOG enabled, putting LOG_DEBUG at the > >top of a file > >> > > >>>> (before log.h inclusion) causes _log() to be executed for > >every log() > >> > > >>>> call, regardless of the build- or run-time logging level. > >> > > >>>> > >> > > >>>> However there is no guarantee that the log record will > >actually be > >> > > >>>> displayed. If the current log level is lower than LOGL_DEBUG > >then it will > >> > > >>>> not be. > >> > > >>>> > >> > > >>>> Add a way to signal that the log record should always be > >displayed and > >> > > >>>> update log_passes_filters() to handle this. > >> > > >>>> > >> > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org> > >> > > >>> > >> > > >>> This exposes an underlying problem with LOG and clang I > >believe: > >> > > >>> https://gitlab.denx.de/u-boot/u-boot/-/jobs/135789 > >> > > >>> > >> > > >> > >> > > >> include/log.h:147:44: note: expanded from macro 'log' > >> > > >> if (CONFIG_IS_ENABLED(LOG) && (_LOG_DEBUG || _l <= > >> > > >> _LOG_MAX_LEVEL)) \ > >> > > >> ^ > >> > > >> drivers/misc/p2sb_emul.c:197:10: error: converting the enum > >constant to > >> > > >> a boolean [-Werror,-Wint-in-bool-context] > >> > > >> > >> > > >> This seems to be a Clang bug. _LOG_DEBUG is not an enum: > >> > > >> > >> > > >> #if CONFIG_IS_ENABLED(LOG) > >> > > >> #ifdef LOG_DEBUG > >> > > >> #define _LOG_DEBUG 1 > >> > > >> #else > >> > > >> #define _LOG_DEBUG 0 > >> > > >> #endif > >> > > >> > >> > > >> So there seems to be a bug in the Clang you used. > >> > > >> > >> > > >> Compiling with clang on Debian Bullseye does not show the > >problem: > >> > > >> > >> > > >> make HOSTCC=clang sandbox_defconfig > >> > > >> make HOSTCC=clang CC=clang -j8 > >> > > >> > >> > > >> clang version 9.0.1-13 > >> > > >> LLVM version 9.0.1 > >> > > >> > >> > > >> Which Clang version did you use? > >> > > >> > >> > > >> This is the change that added the test: > >> > > >> https://reviews.llvm.org/D63082 > >> > > >> > >> > > >> -Wint-in-bool-context seems to be new in Clang 10. > >> > > >> > >> > > >> All over the U-Boot code we assume that a non-zero integer is > >true. Do > >> > > >> we really want to enable -Wint-in-bool-context? > >> > > > > >> > > > I'm using the official Clang 10 stable builds. > >> > > > > >> > > > >> > > Do you really want to forbid using integers as booleans > >> > > (-Wint-in-bool-context)? > >> > > >> > So, interesting. The Linux kernel isn't disabling this warning. > >It's > >> > mentioned in two commits, one of which is "clang found a bug here", > >of > >> > which this is not the case. The other is more like ours: > >> > commit 968e5170939662341242812b9c82ef51cf140a33 > >> > Author: Nathan Chancellor <natechancellor@gmail.com> > >> > Date: Thu Sep 26 09:22:59 2019 -0700 > >> > > >> > tracing: Fix clang -Wint-in-bool-context warnings in IF_ASSIGN > >macro > >> > > >> > After r372664 in clang, the IF_ASSIGN macro causes a couple > >hundred > >> > warnings along the lines of: > >> > > >> > kernel/trace/trace_output.c:1331:2: warning: converting the > >enum > >> > constant to a boolean [-Wint-in-bool-context] > >> > kernel/trace/trace.h:409:3: note: expanded from macro > >> > 'trace_assign_type' > >> > IF_ASSIGN(var, ent, struct > >ftrace_graph_ret_entry, > >> > ^ > >> > kernel/trace/trace.h:371:14: note: expanded from macro > >'IF_ASSIGN' > >> > WARN_ON(id && (entry)->type != id); \ > >> > ^ > >> > 264 warnings generated. > >> > > >> > This warning can catch issues with constructs like: > >> > > >> > if (state == A || B) > >> > > >> > where the developer really meant: > >> > > >> > if (state == A || state == B) > >> > > >> > This is currently the only occurrence of the warning in the > >kernel > >> > tree across defconfig, allyesconfig, allmodconfig for arm32, > >arm64, > >> > and x86_64. Add the implicit '!= 0' to the WARN_ON statement to > >fix > >> > the warnings and find potential issues in the future. > >> > > >> > Link: > >https://github.com/llvm/llvm-project/commit/28b38c277a2941e9e891b2db30652cfd962f070b > >> > Link: https://github.com/ClangBuiltLinux/linux/issues/686 > >> > Link: > >http://lkml.kernel.org/r/20190926162258.466321-1-natechancellor@gmail.com > >> > > >> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > >> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > >> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > >> > > >> > Which is like our case, and reworking the test to be explicit. I > >lean > >> > towards that. > >> > >> Oh dear I really want to vote against that. > >> > >> if (x) > >> > >> should allow C to consider x to be a boolean. I am hitting this in > >> Zephyr and feels like it is undoing a long-standing C feature, with > >> major disruption, for no benefit I can detect. > >> > >> Hopefully I am misunderstanding what -Wint-in-bool-context means, and > >> it just applies to enums? > > > >Yes, it's not the simple case, it's the complex case as noted in the > >kernel commit message. Our change I believe would be: > > > >diff --git a/include/log.h b/include/log.h > >index 2859ce1f2e72..91ca2e0523f7 100644 > >--- a/include/log.h > >+++ b/include/log.h > >@@ -141,7 +141,7 @@ static inline int _log_nop(enum log_category_t cat, > >enum log_level_t level, > > /* Emit a log record if the level is less that the maximum */ > > #define log(_cat, _level, _fmt, _args...) ({ \ > > int _l = _level; \ > >- if (CONFIG_IS_ENABLED(LOG) && (_l <= _LOG_MAX_LEVEL || _LOG_DEBUG)) \ > >+ if (CONFIG_IS_ENABLED(LOG) && (_l <= _LOG_MAX_LEVEL || _LOG_DEBUG == > >1)) \ > > Why wouldn't we define _LOG_DEBUG as true or false? Shouldn't clang know that true is boolean? Because I didn't think of that instead, mainly.
Hi, On Wed, 5 Aug 2020 at 13:32, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Aug 05, 2020 at 09:08:40PM +0200, Heinrich Schuchardt wrote: > > Am 5. August 2020 21:03:28 MESZ schrieb Tom Rini <trini@konsulko.com>: > > >On Wed, Aug 05, 2020 at 12:56:17PM -0600, Simon Glass wrote: > > >> Hi Tom, > > >> > > >> On Wed, 5 Aug 2020 at 12:37, Tom Rini <trini@konsulko.com> wrote: > > >> > > > >> > On Wed, Aug 05, 2020 at 08:31:55PM +0200, Heinrich Schuchardt > > >wrote: > > >> > > On 05.08.20 14:56, Tom Rini wrote: > > >> > > > On Wed, Aug 05, 2020 at 02:54:05PM +0200, Heinrich Schuchardt > > >wrote: > > >> > > >> On 05.08.20 14:18, Tom Rini wrote: > > >> > > >>> On Sun, Jul 26, 2020 at 08:27:35PM -0600, Simon Glass wrote: > > >> > > >>> > > >> > > >>>> At present if CONFIG_LOG enabled, putting LOG_DEBUG at the > > >top of a file > > >> > > >>>> (before log.h inclusion) causes _log() to be executed for > > >every log() > > >> > > >>>> call, regardless of the build- or run-time logging level. > > >> > > >>>> > > >> > > >>>> However there is no guarantee that the log record will > > >actually be > > >> > > >>>> displayed. If the current log level is lower than LOGL_DEBUG > > >then it will > > >> > > >>>> not be. > > >> > > >>>> > > >> > > >>>> Add a way to signal that the log record should always be > > >displayed and > > >> > > >>>> update log_passes_filters() to handle this. > > >> > > >>>> > > >> > > >>>> Signed-off-by: Simon Glass <sjg@chromium.org> > > >> > > >>> > > >> > > >>> This exposes an underlying problem with LOG and clang I > > >believe: > > >> > > >>> https://gitlab.denx.de/u-boot/u-boot/-/jobs/135789 > > >> > > >>> > > >> > > >> > > >> > > >> include/log.h:147:44: note: expanded from macro 'log' > > >> > > >> if (CONFIG_IS_ENABLED(LOG) && (_LOG_DEBUG || _l <= > > >> > > >> _LOG_MAX_LEVEL)) \ > > >> > > >> ^ > > >> > > >> drivers/misc/p2sb_emul.c:197:10: error: converting the enum > > >constant to > > >> > > >> a boolean [-Werror,-Wint-in-bool-context] > > >> > > >> > > >> > > >> This seems to be a Clang bug. _LOG_DEBUG is not an enum: > > >> > > >> > > >> > > >> #if CONFIG_IS_ENABLED(LOG) > > >> > > >> #ifdef LOG_DEBUG > > >> > > >> #define _LOG_DEBUG 1 > > >> > > >> #else > > >> > > >> #define _LOG_DEBUG 0 > > >> > > >> #endif > > >> > > >> > > >> > > >> So there seems to be a bug in the Clang you used. > > >> > > >> > > >> > > >> Compiling with clang on Debian Bullseye does not show the > > >problem: > > >> > > >> > > >> > > >> make HOSTCC=clang sandbox_defconfig > > >> > > >> make HOSTCC=clang CC=clang -j8 > > >> > > >> > > >> > > >> clang version 9.0.1-13 > > >> > > >> LLVM version 9.0.1 > > >> > > >> > > >> > > >> Which Clang version did you use? > > >> > > >> > > >> > > >> This is the change that added the test: > > >> > > >> https://reviews.llvm.org/D63082 > > >> > > >> > > >> > > >> -Wint-in-bool-context seems to be new in Clang 10. > > >> > > >> > > >> > > >> All over the U-Boot code we assume that a non-zero integer is > > >true. Do > > >> > > >> we really want to enable -Wint-in-bool-context? > > >> > > > > > >> > > > I'm using the official Clang 10 stable builds. > > >> > > > > > >> > > > > >> > > Do you really want to forbid using integers as booleans > > >> > > (-Wint-in-bool-context)? > > >> > > > >> > So, interesting. The Linux kernel isn't disabling this warning. > > >It's > > >> > mentioned in two commits, one of which is "clang found a bug here", > > >of > > >> > which this is not the case. The other is more like ours: > > >> > commit 968e5170939662341242812b9c82ef51cf140a33 > > >> > Author: Nathan Chancellor <natechancellor@gmail.com> > > >> > Date: Thu Sep 26 09:22:59 2019 -0700 > > >> > > > >> > tracing: Fix clang -Wint-in-bool-context warnings in IF_ASSIGN > > >macro > > >> > > > >> > After r372664 in clang, the IF_ASSIGN macro causes a couple > > >hundred > > >> > warnings along the lines of: > > >> > > > >> > kernel/trace/trace_output.c:1331:2: warning: converting the > > >enum > > >> > constant to a boolean [-Wint-in-bool-context] > > >> > kernel/trace/trace.h:409:3: note: expanded from macro > > >> > 'trace_assign_type' > > >> > IF_ASSIGN(var, ent, struct > > >ftrace_graph_ret_entry, > > >> > ^ > > >> > kernel/trace/trace.h:371:14: note: expanded from macro > > >'IF_ASSIGN' > > >> > WARN_ON(id && (entry)->type != id); \ > > >> > ^ > > >> > 264 warnings generated. > > >> > > > >> > This warning can catch issues with constructs like: > > >> > > > >> > if (state == A || B) > > >> > > > >> > where the developer really meant: > > >> > > > >> > if (state == A || state == B) > > >> > > > >> > This is currently the only occurrence of the warning in the > > >kernel > > >> > tree across defconfig, allyesconfig, allmodconfig for arm32, > > >arm64, > > >> > and x86_64. Add the implicit '!= 0' to the WARN_ON statement to > > >fix > > >> > the warnings and find potential issues in the future. > > >> > > > >> > Link: > > >https://github.com/llvm/llvm-project/commit/28b38c277a2941e9e891b2db30652cfd962f070b > > >> > Link: https://github.com/ClangBuiltLinux/linux/issues/686 > > >> > Link: > > >http://lkml.kernel.org/r/20190926162258.466321-1-natechancellor@gmail.com > > >> > > > >> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > >> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > >> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > > >> > > > >> > Which is like our case, and reworking the test to be explicit. I > > >lean > > >> > towards that. > > >> > > >> Oh dear I really want to vote against that. > > >> > > >> if (x) > > >> > > >> should allow C to consider x to be a boolean. I am hitting this in > > >> Zephyr and feels like it is undoing a long-standing C feature, with > > >> major disruption, for no benefit I can detect. > > >> > > >> Hopefully I am misunderstanding what -Wint-in-bool-context means, and > > >> it just applies to enums? > > > > > >Yes, it's not the simple case, it's the complex case as noted in the > > >kernel commit message. Our change I believe would be: > > > > > >diff --git a/include/log.h b/include/log.h > > >index 2859ce1f2e72..91ca2e0523f7 100644 > > >--- a/include/log.h > > >+++ b/include/log.h > > >@@ -141,7 +141,7 @@ static inline int _log_nop(enum log_category_t cat, > > >enum log_level_t level, > > > /* Emit a log record if the level is less that the maximum */ > > > #define log(_cat, _level, _fmt, _args...) ({ \ > > > int _l = _level; \ > > >- if (CONFIG_IS_ENABLED(LOG) && (_l <= _LOG_MAX_LEVEL || _LOG_DEBUG)) \ > > >+ if (CONFIG_IS_ENABLED(LOG) && (_l <= _LOG_MAX_LEVEL || _LOG_DEBUG == > > >1)) \ > > > > Why wouldn't we define _LOG_DEBUG as true or false? Shouldn't clang know that true is boolean? > > Because I didn't think of that instead, mainly. > I've resent this patch with just this change to fix clang-10. Regards, Simon
diff --git a/common/log.c b/common/log.c index 734d26de4a..5ff8245922 100644 --- a/common/log.c +++ b/common/log.c @@ -156,16 +156,20 @@ static bool log_has_file(const char *file_list, const char *file) static bool log_passes_filters(struct log_device *ldev, struct log_rec *rec) { struct log_filter *filt; + int level = rec->level & LOGL_LEVEL_MASK; + + if (rec->force_debug && level <= LOGL_DEBUG) + return true; /* If there are no filters, filter on the default log level */ if (list_empty(&ldev->filter_head)) { - if (rec->level > gd->default_log_level) + if (level > gd->default_log_level) return false; return true; } list_for_each_entry(filt, &ldev->filter_head, sibling_node) { - if (rec->level > filt->max_level) + if (level > filt->max_level) continue; if ((filt->flags & LOGFF_HAS_CAT) && !log_has_cat(filt->cat_list, rec->cat)) @@ -208,7 +212,8 @@ int _log(enum log_category_t cat, enum log_level_t level, const char *file, va_list args; rec.cat = cat; - rec.level = level; + rec.level = level & LOGL_LEVEL_MASK; + rec.force_debug = level & LOGL_FORCE_DEBUG; rec.file = file; rec.line = line; rec.func = func; diff --git a/doc/README.log b/doc/README.log index ba838824a9..554e99ca4c 100644 --- a/doc/README.log +++ b/doc/README.log @@ -77,12 +77,10 @@ Sometimes it is useful to turn on logging just in one file. You can use this: #define LOG_DEBUG -to enable building in of all logging statements in a single file. Put it at -the top of the file, before any #includes. - -To actually get U-Boot to output this you need to also set the default logging -level - e.g. set CONFIG_LOG_DEFAULT_LEVEL to 7 (LOGL_DEBUG) or more. Otherwise -debug output is suppressed and will not be generated. +to enable building in of all debug logging statements in a single file. Put it +at the top of the file, before any #includes. This overrides any log-level +setting in U-Boot, including CONFIG_LOG_DEFAULT_LEVEL, but just for that file. +All logging statements, up to and including LOGL_DEBUG, will be displayed. Convenience functions diff --git a/include/log.h b/include/log.h index 2859ce1f2e..63052f74eb 100644 --- a/include/log.h +++ b/include/log.h @@ -33,6 +33,9 @@ enum log_level_t { LOGL_COUNT, LOGL_NONE, + LOGL_LEVEL_MASK = 0xf, /* Mask for valid log levels */ + LOGL_FORCE_DEBUG = 0x10, /* Mask to force output due to LOG_DEBUG */ + LOGL_FIRST = LOGL_EMERG, LOGL_MAX = LOGL_DEBUG_IO, }; @@ -133,7 +136,7 @@ static inline int _log_nop(enum log_category_t cat, enum log_level_t level, #if CONFIG_IS_ENABLED(LOG) #ifdef LOG_DEBUG -#define _LOG_DEBUG 1 +#define _LOG_DEBUG LOGL_FORCE_DEBUG #else #define _LOG_DEBUG 0 #endif @@ -141,9 +144,9 @@ static inline int _log_nop(enum log_category_t cat, enum log_level_t level, /* Emit a log record if the level is less that the maximum */ #define log(_cat, _level, _fmt, _args...) ({ \ int _l = _level; \ - if (CONFIG_IS_ENABLED(LOG) && (_l <= _LOG_MAX_LEVEL || _LOG_DEBUG)) \ - _log((enum log_category_t)(_cat), _l, __FILE__, __LINE__, \ - __func__, \ + if (CONFIG_IS_ENABLED(LOG) && (_LOG_DEBUG || _l <= _LOG_MAX_LEVEL)) \ + _log((enum log_category_t)(_cat), _l | _LOG_DEBUG, __FILE__, \ + __LINE__, __func__, \ pr_fmt(_fmt), ##_args); \ }) #else @@ -279,8 +282,12 @@ void __assert_fail(const char *assertion, const char *file, unsigned int line, * Memebers marked as 'allocated' are allocated (e.g. via strdup()) by the log * system. * + * TODO(sjg@chromium.org): Compress this struct down a bit to reduce space, e.g. + * a single u32 for cat, level, line and force_debug + * * @cat: Category, representing a uclass or part of U-Boot * @level: Severity level, less severe is higher + * @force_debug: Force output of debug * @file: Name of file where the log record was generated (not allocated) * @line: Line number where the log record was generated * @func: Function where the log record was generated (not allocated) @@ -289,6 +296,7 @@ void __assert_fail(const char *assertion, const char *file, unsigned int line, struct log_rec { enum log_category_t cat; enum log_level_t level; + bool force_debug; const char *file; int line; const char *func; diff --git a/test/log/syslog_test.c b/test/log/syslog_test.c index 120a8b2537..df239d1e8d 100644 --- a/test/log/syslog_test.c +++ b/test/log/syslog_test.c @@ -276,7 +276,7 @@ static int log_test_syslog_nodebug(struct unit_test_state *uts) sandbox_eth_set_tx_handler(0, sb_log_tx_handler); /* Used by ut_assert macros in the tx_handler */ sandbox_eth_set_priv(0, &env); - log_debug("testing %s\n", "log_debug"); + log_content("testing %s\n", "log_debug"); sandbox_eth_set_tx_handler(0, NULL); /* Check that the callback function was not called */ ut_assertnonnull(env.expected);
At present if CONFIG_LOG enabled, putting LOG_DEBUG at the top of a file (before log.h inclusion) causes _log() to be executed for every log() call, regardless of the build- or run-time logging level. However there is no guarantee that the log record will actually be displayed. If the current log level is lower than LOGL_DEBUG then it will not be. Add a way to signal that the log record should always be displayed and update log_passes_filters() to handle this. Signed-off-by: Simon Glass <sjg@chromium.org> --- common/log.c | 11 ++++++++--- doc/README.log | 10 ++++------ include/log.h | 16 ++++++++++++---- test/log/syslog_test.c | 2 +- 4 files changed, 25 insertions(+), 14 deletions(-)