* [PATCH] log: Allow LOG_DEBUG to always enable log output
@ 2020-07-27 2:27 Simon Glass
2020-07-27 6:15 ` Heinrich Schuchardt
2020-08-05 12:18 ` Tom Rini
0 siblings, 2 replies; 13+ messages in thread
From: Simon Glass @ 2020-07-27 2:27 UTC (permalink / raw)
To: u-boot
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(-)
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);
--
2.28.0.rc0.142.g3c755180ce-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] log: Allow LOG_DEBUG to always enable log output
2020-07-27 2:27 [PATCH] log: Allow LOG_DEBUG to always enable log output Simon Glass
@ 2020-07-27 6:15 ` Heinrich Schuchardt
2020-07-27 23:17 ` Simon Glass
2020-08-05 12:18 ` Tom Rini
1 sibling, 1 reply; 13+ messages in thread
From: Heinrich Schuchardt @ 2020-07-27 6:15 UTC (permalink / raw)
To: u-boot
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 at 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);
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] log: Allow LOG_DEBUG to always enable log output
2020-07-27 6:15 ` Heinrich Schuchardt
@ 2020-07-27 23:17 ` Simon Glass
0 siblings, 0 replies; 13+ messages in thread
From: Simon Glass @ 2020-07-27 23:17 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] log: Allow LOG_DEBUG to always enable log output
2020-07-27 2:27 [PATCH] log: Allow LOG_DEBUG to always enable log output Simon Glass
2020-07-27 6:15 ` Heinrich Schuchardt
@ 2020-08-05 12:18 ` Tom Rini
2020-08-05 12:54 ` Heinrich Schuchardt
1 sibling, 1 reply; 13+ messages in thread
From: Tom Rini @ 2020-08-05 12:18 UTC (permalink / raw)
To: u-boot
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
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200805/6eaf247d/attachment.sig>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] log: Allow LOG_DEBUG to always enable log output
2020-08-05 12:18 ` Tom Rini
@ 2020-08-05 12:54 ` Heinrich Schuchardt
2020-08-05 12:56 ` Tom Rini
0 siblings, 1 reply; 13+ messages in thread
From: Heinrich Schuchardt @ 2020-08-05 12:54 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] log: Allow LOG_DEBUG to always enable log output
2020-08-05 12:54 ` Heinrich Schuchardt
@ 2020-08-05 12:56 ` Tom Rini
2020-08-05 18:31 ` Heinrich Schuchardt
0 siblings, 1 reply; 13+ messages in thread
From: Tom Rini @ 2020-08-05 12:56 UTC (permalink / raw)
To: u-boot
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.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200805/dcdd3217/attachment.sig>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] log: Allow LOG_DEBUG to always enable log output
2020-08-05 12:56 ` Tom Rini
@ 2020-08-05 18:31 ` Heinrich Schuchardt
2020-08-05 18:37 ` Tom Rini
0 siblings, 1 reply; 13+ messages in thread
From: Heinrich Schuchardt @ 2020-08-05 18:31 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] log: Allow LOG_DEBUG to always enable log output
2020-08-05 18:31 ` Heinrich Schuchardt
@ 2020-08-05 18:37 ` Tom Rini
2020-08-05 18:56 ` Simon Glass
0 siblings, 1 reply; 13+ messages in thread
From: Tom Rini @ 2020-08-05 18:37 UTC (permalink / raw)
To: u-boot
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 at 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.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200805/0edf6d54/attachment.sig>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] log: Allow LOG_DEBUG to always enable log output
2020-08-05 18:37 ` Tom Rini
@ 2020-08-05 18:56 ` Simon Glass
2020-08-05 19:03 ` Tom Rini
0 siblings, 1 reply; 13+ messages in thread
From: Simon Glass @ 2020-08-05 18:56 UTC (permalink / raw)
To: u-boot
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 at 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] log: Allow LOG_DEBUG to always enable log output
2020-08-05 18:56 ` Simon Glass
@ 2020-08-05 19:03 ` Tom Rini
2020-08-05 19:08 ` Heinrich Schuchardt
0 siblings, 1 reply; 13+ messages in thread
From: Tom Rini @ 2020-08-05 19:03 UTC (permalink / raw)
To: u-boot
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 at 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.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200805/6ae0559f/attachment.sig>
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] log: Allow LOG_DEBUG to always enable log output
2020-08-05 19:03 ` Tom Rini
@ 2020-08-05 19:08 ` Heinrich Schuchardt
2020-08-05 19:32 ` Tom Rini
0 siblings, 1 reply; 13+ messages in thread
From: Heinrich Schuchardt @ 2020-08-05 19:08 UTC (permalink / raw)
To: u-boot
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 at 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] log: Allow LOG_DEBUG to always enable log output
2020-08-05 19:08 ` Heinrich Schuchardt
@ 2020-08-05 19:32 ` Tom Rini
2020-09-28 4:24 ` Simon Glass
0 siblings, 1 reply; 13+ messages in thread
From: Tom Rini @ 2020-08-05 19:32 UTC (permalink / raw)
To: u-boot
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 at 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.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200805/87489458/attachment.sig>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] log: Allow LOG_DEBUG to always enable log output
2020-08-05 19:32 ` Tom Rini
@ 2020-09-28 4:24 ` Simon Glass
0 siblings, 0 replies; 13+ messages in thread
From: Simon Glass @ 2020-09-28 4:24 UTC (permalink / raw)
To: u-boot
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 at 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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-09-28 4:24 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-27 2:27 [PATCH] log: Allow LOG_DEBUG to always enable log output Simon Glass
2020-07-27 6:15 ` Heinrich Schuchardt
2020-07-27 23:17 ` Simon Glass
2020-08-05 12:18 ` Tom Rini
2020-08-05 12:54 ` Heinrich Schuchardt
2020-08-05 12:56 ` Tom Rini
2020-08-05 18:31 ` Heinrich Schuchardt
2020-08-05 18:37 ` Tom Rini
2020-08-05 18:56 ` Simon Glass
2020-08-05 19:03 ` Tom Rini
2020-08-05 19:08 ` Heinrich Schuchardt
2020-08-05 19:32 ` Tom Rini
2020-09-28 4:24 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.