When compiling with -Wtype-limits (activated for example with make
W=2), GENMASK_INPUT_CHECK() will generate some warnings if invoked
with an unsigned integer and zero.
For example, this:
| #include <linux/bits.h>
| u32 foo(u32 bar)
| { return GENMASK(bar, 0); }
would yield:
| In file included from ./include/linux/bits.h:22,
| from ./foo.c:1:
| foo.c: In function 'foo':
| ./include/linux/bits.h:25:36: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
| 25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| | ^
| ./include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
| 16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| | ^
| ./include/linux/bits.h:25:17: note: in expansion of macro '__is_constexpr'
| 25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| | ^~~~~~~~~~~~~~
| ./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
| 38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| | ^~~~~~~~~~~~~~~~~~~
| foo.c:16:10: note: in expansion of macro 'GENMASK'
| 16 | { return GENMASK(bar, 0); }
| | ^~~~~~~
| ./include/linux/bits.h:25:48: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
| 25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| | ^
| ./include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
| 16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| | ^
| ./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
| 38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| | ^~~~~~~~~~~~~~~~~~~
| foo.c:16:10: note: in expansion of macro 'GENMASK'
| 16 | { return GENMASK(bar, 0); }
| | ^~~~~~~
This pattern is harmless but because it occurs in header files
(example find_first_bit() from linux/find.h [1]) and because of the
include hell, the macro GENMASK_INPUT_CHECK() is accountable for 31%
(164714/532484) of all warnings when compiling all modules at W=2
level.
Reference (using gcc 11.2 and linux v5.17-rc6):
| $ make allyesconfig
| $ sed -i '/CONFIG_WERROR/d' .config
| $ make W=2 -j8 2> kernel_w2.log > /dev/null
| $ grep "\./include/linux/bits\.h:.*: warning" kernel_w2\.log | wc -l
| 164714
| $ grep ": warning: " kernel_w2.log | wc -l
| 532484
This heavily pollutes the output of make W=2 and make it painful to
find other relevant issues.
In this patch, we silent this warning by:
* replacing the comparison > by and logical and && in the first
argument of __builtin_choose_expr().
* casting the high bit of the mask to a signed integer in the second
argument of __builtin_choose_expr().
By doing so, 31% of W=2 warnings get removed.
[1] https://elixir.bootlin.com/linux/v5.17-rc6/source/include/linux/find.h#L119
Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of
GENMASK inputs")
Signed-off-by: Vincent Mailhol <[email protected]>
---
include/linux/bits.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 87d112650dfb..542e9a8985b1 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -22,7 +22,7 @@
#include <linux/build_bug.h>
#define GENMASK_INPUT_CHECK(h, l) \
(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
- __is_constexpr((l) > (h)), (l) > (h), 0)))
+ __is_constexpr((h)) && __is_constexpr((l)), (l) > (int)(h), 0)))
#else
/*
* BUILD_BUG_ON_ZERO is not available in h files included from asm files,
--
2.34.1
On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
<[email protected]> wrote:
>
> This pattern is harmless but because it occurs in header files
> (example find_first_bit() from linux/find.h [1]) and because of the
> include hell, the macro GENMASK_INPUT_CHECK() is accountable for 31%
> (164714/532484) of all warnings when compiling all modules at W=2
> level.
Have you fixed W=1 warnings?
Without fixing W=1 (which makes much more sense, when used with
WERROR=y && COMPILE_TEST=y) this has no value.
NAK.
--
With Best Regards,
Andy Shevchenko
On Tue. 5 Mar 2022 at 03:46, Andy Shevchenko <[email protected]> wrote:
> On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> <[email protected]> wrote:
> >
>
> > This pattern is harmless but because it occurs in header files
> > (example find_first_bit() from linux/find.h [1]) and because of the
> > include hell, the macro GENMASK_INPUT_CHECK() is accountable for 31%
> > (164714/532484) of all warnings when compiling all modules at W=2
> > level.
>
> Have you fixed W=1 warnings?
I am not sure to which W=1 warnings you are referring to.
linux/bits.h does yield any W=1 warnings if this is your concern.
Concerning other files, it depends. linux/bits.h is
included (either directly or indirectly) in thousands of files,
some of which would yield some W=1, some of which would not.
> Without fixing W=1 (which makes much more sense, when used with
> WERROR=y && COMPILE_TEST=y) this has no value.
Let me try to explain why I think this has some value. I am the
maintainer of one driver:
drivers/net/can/usb/etas_es58x/
When I compile it, with W=1, no warnings. When I compile it with W=2,
this is the output:
$ make W=2 drivers/net/can/usb/etas_es58x/etas_es58x.o
CALL scripts/checksyscalls.sh
<stdin>:21: warning: macro "__IGNORE_stat64" is not used [-Wunused-macros]
<stdin>:22: warning: macro "__IGNORE_lstat64" is not used [-Wunused-macros]
<stdin>:75: warning: macro "__IGNORE_llseek" is not used [-Wunused-macros]
<stdin>:159: warning: macro "__IGNORE_madvise1" is not used [-Wunused-macros]
CALL scripts/atomic/check-atomics.sh
DESCEND objtool
CC [M] drivers/net/can/usb/etas_es58x/es58x_core.o
In file included from ./include/linux/bitops.h:33,
from ./include/linux/kernel.h:22,
from drivers/net/can/usb/etas_es58x/es58x_core.c:13:
./arch/x86/include/asm/bitops.h:283:28: warning: declaration of 'ffs'
shadows a built-in function [-Wshadow]
283 | static __always_inline int ffs(int x)
| ^~~
In file included from ./include/linux/container_of.h:5,
from ./include/linux/kernel.h:21,
from drivers/net/can/usb/etas_es58x/es58x_core.c:13:
./include/linux/find.h: In function 'find_first_bit':
./include/linux/bits.h:25:36: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:25:17: note: in expansion of macro '__is_constexpr'
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^~~~~~~~~~~~~~
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:119:45: note: in expansion of macro 'GENMASK'
119 | unsigned long val = *addr & GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/bits.h:25:48: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:119:45: note: in expansion of macro 'GENMASK'
119 | unsigned long val = *addr & GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/find.h: In function 'find_first_and_bit':
./include/linux/bits.h:25:36: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:25:17: note: in expansion of macro '__is_constexpr'
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^~~~~~~~~~~~~~
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:144:55: note: in expansion of macro 'GENMASK'
144 | unsigned long val = *addr1 & *addr2 &
GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/bits.h:25:48: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:144:55: note: in expansion of macro 'GENMASK'
144 | unsigned long val = *addr1 & *addr2 &
GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/find.h: In function 'find_first_zero_bit':
./include/linux/bits.h:25:36: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:25:17: note: in expansion of macro '__is_constexpr'
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^~~~~~~~~~~~~~
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:166:46: note: in expansion of macro 'GENMASK'
166 | unsigned long val = *addr | ~GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/bits.h:25:48: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:166:46: note: in expansion of macro 'GENMASK'
166 | unsigned long val = *addr | ~GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/find.h: In function 'find_last_bit':
./include/linux/bits.h:25:36: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:25:17: note: in expansion of macro '__is_constexpr'
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^~~~~~~~~~~~~~
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:187:45: note: in expansion of macro 'GENMASK'
187 | unsigned long val = *addr & GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/bits.h:25:48: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:187:45: note: in expansion of macro 'GENMASK'
187 | unsigned long val = *addr & GENMASK(size - 1, 0);
| ^~~~~~~
CC [M] drivers/net/can/usb/etas_es58x/es581_4.o
In file included from ./include/linux/bitops.h:33,
from ./include/linux/kernel.h:22,
from drivers/net/can/usb/etas_es58x/es581_4.c:12:
./arch/x86/include/asm/bitops.h:283:28: warning: declaration of 'ffs'
shadows a built-in function [-Wshadow]
283 | static __always_inline int ffs(int x)
| ^~~
In file included from ./include/linux/container_of.h:5,
from ./include/linux/kernel.h:21,
from drivers/net/can/usb/etas_es58x/es581_4.c:12:
./include/linux/find.h: In function 'find_first_bit':
./include/linux/bits.h:25:36: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:25:17: note: in expansion of macro '__is_constexpr'
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^~~~~~~~~~~~~~
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:119:45: note: in expansion of macro 'GENMASK'
119 | unsigned long val = *addr & GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/bits.h:25:48: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:119:45: note: in expansion of macro 'GENMASK'
119 | unsigned long val = *addr & GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/find.h: In function 'find_first_and_bit':
./include/linux/bits.h:25:36: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:25:17: note: in expansion of macro '__is_constexpr'
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^~~~~~~~~~~~~~
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:144:55: note: in expansion of macro 'GENMASK'
144 | unsigned long val = *addr1 & *addr2 &
GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/bits.h:25:48: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:144:55: note: in expansion of macro 'GENMASK'
144 | unsigned long val = *addr1 & *addr2 &
GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/find.h: In function 'find_first_zero_bit':
./include/linux/bits.h:25:36: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:25:17: note: in expansion of macro '__is_constexpr'
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^~~~~~~~~~~~~~
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:166:46: note: in expansion of macro 'GENMASK'
166 | unsigned long val = *addr | ~GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/bits.h:25:48: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:166:46: note: in expansion of macro 'GENMASK'
166 | unsigned long val = *addr | ~GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/find.h: In function 'find_last_bit':
./include/linux/bits.h:25:36: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:25:17: note: in expansion of macro '__is_constexpr'
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^~~~~~~~~~~~~~
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:187:45: note: in expansion of macro 'GENMASK'
187 | unsigned long val = *addr & GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/bits.h:25:48: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:187:45: note: in expansion of macro 'GENMASK'
187 | unsigned long val = *addr & GENMASK(size - 1, 0);
| ^~~~~~~
CC [M] drivers/net/can/usb/etas_es58x/es58x_fd.o
In file included from ./include/linux/bitops.h:33,
from ./include/linux/kernel.h:22,
from drivers/net/can/usb/etas_es58x/es58x_fd.c:14:
./arch/x86/include/asm/bitops.h:283:28: warning: declaration of 'ffs'
shadows a built-in function [-Wshadow]
283 | static __always_inline int ffs(int x)
| ^~~
In file included from ./include/linux/container_of.h:5,
from ./include/linux/kernel.h:21,
from drivers/net/can/usb/etas_es58x/es58x_fd.c:14:
./include/linux/find.h: In function 'find_first_bit':
./include/linux/bits.h:25:36: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:25:17: note: in expansion of macro '__is_constexpr'
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^~~~~~~~~~~~~~
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:119:45: note: in expansion of macro 'GENMASK'
119 | unsigned long val = *addr & GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/bits.h:25:48: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:119:45: note: in expansion of macro 'GENMASK'
119 | unsigned long val = *addr & GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/find.h: In function 'find_first_and_bit':
./include/linux/bits.h:25:36: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:25:17: note: in expansion of macro '__is_constexpr'
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^~~~~~~~~~~~~~
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:144:55: note: in expansion of macro 'GENMASK'
144 | unsigned long val = *addr1 & *addr2 &
GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/bits.h:25:48: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:144:55: note: in expansion of macro 'GENMASK'
144 | unsigned long val = *addr1 & *addr2 &
GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/find.h: In function 'find_first_zero_bit':
./include/linux/bits.h:25:36: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:25:17: note: in expansion of macro '__is_constexpr'
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^~~~~~~~~~~~~~
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:166:46: note: in expansion of macro 'GENMASK'
166 | unsigned long val = *addr | ~GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/bits.h:25:48: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:166:46: note: in expansion of macro 'GENMASK'
166 | unsigned long val = *addr | ~GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/find.h: In function 'find_last_bit':
./include/linux/bits.h:25:36: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:25:17: note: in expansion of macro '__is_constexpr'
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^~~~~~~~~~~~~~
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:187:45: note: in expansion of macro 'GENMASK'
187 | unsigned long val = *addr & GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/bits.h:25:48: warning: comparison of unsigned
expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^
./include/linux/build_bug.h:16:62: note: in definition of macro
'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
./include/linux/bits.h:38:10: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:187:45: note: in expansion of macro 'GENMASK'
187 | unsigned long val = *addr & GENMASK(size - 1, 0);
| ^~~~~~~
LD [M] drivers/net/can/usb/etas_es58x/etas_es58x.o
None of these are from the driver, only from the includes.
After this patch, the output becomes:
$ make W=2 drivers/net/can/usb/etas_es58x/etas_es58x.o
CALL scripts/checksyscalls.sh
<stdin>:21: warning: macro "__IGNORE_stat64" is not used [-Wunused-macros]
<stdin>:22: warning: macro "__IGNORE_lstat64" is not used [-Wunused-macros]
<stdin>:75: warning: macro "__IGNORE_llseek" is not used [-Wunused-macros]
<stdin>:159: warning: macro "__IGNORE_madvise1" is not used [-Wunused-macros]
CALL scripts/atomic/check-atomics.sh
DESCEND objtool
CC [M] drivers/net/can/usb/etas_es58x/es58x_core.o
In file included from ./include/linux/bitops.h:33,
from ./include/linux/kernel.h:22,
from drivers/net/can/usb/etas_es58x/es58x_core.c:13:
./arch/x86/include/asm/bitops.h:283:28: warning: declaration of 'ffs'
shadows a built-in function [-Wshadow]
283 | static __always_inline int ffs(int x)
| ^~~
CC [M] drivers/net/can/usb/etas_es58x/es581_4.o
In file included from ./include/linux/bitops.h:33,
from ./include/linux/kernel.h:22,
from drivers/net/can/usb/etas_es58x/es581_4.c:12:
./arch/x86/include/asm/bitops.h:283:28: warning: declaration of 'ffs'
shadows a built-in function [-Wshadow]
283 | static __always_inline int ffs(int x)
| ^~~
CC [M] drivers/net/can/usb/etas_es58x/es58x_fd.o
In file included from ./include/linux/bitops.h:33,
from ./include/linux/kernel.h:22,
from drivers/net/can/usb/etas_es58x/es58x_fd.c:14:
./arch/x86/include/asm/bitops.h:283:28: warning: declaration of 'ffs'
shadows a built-in function [-Wshadow]
283 | static __always_inline int ffs(int x)
| ^~~
LD [M] drivers/net/can/usb/etas_es58x/etas_es58x.o
The output gets reduced from 375 lines to only 30 lines.
This is a tremendous life improvement for me (considering the ffs
shadowing, I would also like to fix it, but this is not the topic
here). Generally speaking, when I develop a new file, I would
like to periodically compile it at W=2 but the warnings for
linux/bits.h makes it painful (to stay polite).
As mentioned in the patch, linux/bits.h is responsible for 164714
W=2 warnings tree wide. So I guess that many other developers who
have a W=1 clean project would be reluctant to address the W=2
due to the polluted output.
> NAK.
Are you willing to change your decision following my comments?
Yours sincerely,
Vincent Mailhol
On Sat, Mar 5, 2022 at 2:43 PM Vincent MAILHOL
<[email protected]> wrote:
> On Tue. 5 Mar 2022 at 03:46, Andy Shevchenko <[email protected]> wrote:
> > On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> > <[email protected]> wrote:
...
> > NAK.
>
> Are you willing to change your decision following my comments?
Have you read this discussion (read the thread in full)
https://lore.kernel.org/lkml/[email protected]/
--
With Best Regards,
Andy Shevchenko
On Sun. 6 Mar 2022 at 06:33, Andy Shevchenko <[email protected]> wrote:
> On Sat, Mar 5, 2022 at 2:43 PM Vincent MAILHOL
> <[email protected]> wrote:
> > On Tue. 5 Mar 2022 at 03:46, Andy Shevchenko <[email protected]> wrote:
> > > On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> > > <[email protected]> wrote:
>
> ...
>
> > > NAK.
> >
> > Are you willing to change your decision following my comments?
>
> Have you read this discussion (read the thread in full)
> https://lore.kernel.org/lkml/[email protected]/
Thank you, this was an instructive read.
For what I understand, there was an effort to fix this when
-Wtype-limits was still a W=1 warning but the effort was stopped
after -Wtype-limits was moved to W=2 despite a v4 patch being very
close to the goal.
Back to my patch, it successfully passes the lib/test_bits.c
build test (including the TEST_GENMASK_FAILURES) and it also
fixes the last open warning from the thread you pointed me to (on
drivers/crypto/inside-secure/safexcel.o):
https://lore.kernel.org/lkml/[email protected]/
So, I am still not sure to understand what issue you see with my
patch. Is it that we should just not care about fixing W=2? Or
do you still see some issues which are not being addressed (if
so, sorry for not understanding)?
I do agree that fixing a W=2 has small value for all the files
which are still emitting some W=1. However, I think it is
beneficial to remove this W=2 spam for all the developers who
produced W=1 clean files and would like to tackle the W=2
warnings.
Yours sincerely,
Vincent Mailhol
On Sun, Mar 6, 2022 at 7:35 AM Vincent MAILHOL
<[email protected]> wrote:
> On Sun. 6 Mar 2022 at 06:33, Andy Shevchenko <[email protected]> wrote:
> > On Sat, Mar 5, 2022 at 2:43 PM Vincent MAILHOL
> > <[email protected]> wrote:
> > > On Tue. 5 Mar 2022 at 03:46, Andy Shevchenko <[email protected]> wrote:
> > > > On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> > > > <[email protected]> wrote:
> >
> > ...
> >
> > > > NAK.
> > >
> > > Are you willing to change your decision following my comments?
> >
> > Have you read this discussion (read the thread in full)
> > https://lore.kernel.org/lkml/[email protected]/
>
> Thank you, this was an instructive read.
>
> For what I understand, there was an effort to fix this when
> -Wtype-limits was still a W=1 warning but the effort was stopped
> after -Wtype-limits was moved to W=2 despite a v4 patch being very
> close to the goal.
My understanding of that discussion is that Wtype-limits is broken,
and Linus pointed out many times that compiler warning on
if ((unsigned int)foo < 0)
is bogus. I.o.w. there is no issue with the code and hence nothing to fix.
> Back to my patch, it successfully passes the lib/test_bits.c
> build test (including the TEST_GENMASK_FAILURES) and it also
> fixes the last open warning from the thread you pointed me to (on
> drivers/crypto/inside-secure/safexcel.o):
> https://lore.kernel.org/lkml/[email protected]/
>
> So, I am still not sure to understand what issue you see with my
> patch. Is it that we should just not care about fixing W=2? Or
> do you still see some issues which are not being addressed (if
> so, sorry for not understanding)?
See above. You may Cc Linus himself to reignite the discussion.
> I do agree that fixing a W=2 has small value for all the files
> which are still emitting some W=1. However, I think it is
> beneficial to remove this W=2 spam for all the developers who
> produced W=1 clean files and would like to tackle the W=2
> warnings.
--
With Best Regards,
Andy Shevchenko
From: Vincent MAILHOL <[email protected]>
Date: Mon, 7 Mar 2022 22:50:56 +0900
> Hi Arnd and Alexander,
>
> Thanks for the support!
>
> On Mon. 7 Mar 2022 at 21:15, Arnd Bergmann <[email protected]> wrote:
> > On Mon, Mar 7, 2022 at 11:58 AM Alexander Lobakin
> > <[email protected]> wrote:
> > > From: Andy Shevchenko <[email protected]>
> > > > Have you fixed W=1 warnings?
> > > > Without fixing W=1 (which makes much more sense, when used with
> > > > WERROR=y && COMPILE_TEST=y) this has no value.
> > >
> > > How is this connected?
> > > When I do `make W=2 path/to/my/code`, I want to see the actual code
> > > problems, not something that comes from the include files.
> > > When I do `make W=2 path/to/new/code/from/lkml`, I want to see the
> > > actual new warnings, not something coming from the includes.
> > > It's much easier to overlook or miss some real warnings when the
> > > stderr is being flooded by the warnings from the include files.
> > > I'm aware there are some scripts to compare before/after, but I
> > > don't want to use them just because "this has to value".
> > > I don't want to do `make W=2 KCFLAGS='-Wno-shadow -Wno-type-limits'`
> > > because then I'm not able to spot the actual shadow or type limit
> > > problems in my/new code.
> > > I fixed several `-Wshadow` warnings previously in the include files
> > > related to networking, and *nobody* said "this has no value" or
> > > NAKed it. And `-Wshadow` has always been in W=2.
> >
> > I agree: if we decide that W=2 warnings are completely useless, we should
> > either remove the option to build a W=2 kernel or remove some of the warning
> > flags that go into it. My feeling is that both W=2 in general, and the
> > Wtype-limits have some value, and that reducing the number of W=2 by
> > 30% as this patch does is a useful goal by itself.
> >
> > A different question is whether this particular patch is the best
> > workaround for the warnings, or if a nicer alternative can be found,
> > such as moving -Wtype-limits to W=3,
>
> I disagree with moving it to W=3 for two reasons:
>
> 1/ This would just move the issue elsewhere. If I had to
> compile with W=3 (which I admittedly *almost* never do), the
> -Wtype-limits spam would still be there.
>
> 2/ After this patch, the number of remaining -Wtype-limits
> drops to only 431 for an allyesconfig (and I guess that there
> are a fair amount of true positives here). This warning is not
> *as broken* as people think. W=2 is a good place I think.
Agree, W=2 is the best place for -Wtype-limits to me.
I've never seen a single useful warning on W=3, but there were
lots of them on W=2 which revealed some real bugs. That said, it
makes no sense at all to even try to check all new code from LKML
for warnings with W=3, but it's actually feasible to make it build
cleanly with W=2 most of times.
Re -Wtype-limits in particular, it's not useless at all.
For example, people tend to make the following mistake:
unsigned int i;
for (i = 0; i ...) {
ret = setup_something(array[i]);
if (ret)
goto unroll;
}
unroll:
while (--i)
unroll_something(array[i]);
The loop will never end as `i` was declared as unsigned.
-Wtype-limits catches this.
Not speaking of checking unsigned variables on < 0:
unsigned int num;
/* calculate_something() returns the number of something
* or -ERRNO in case of an error
*/
num = calculate_something();
if (num < 0)
...
Catches as well.
>
> That said, moving it to W=3 would still solve the core issue: W=2
> being spammed. Definitely not my favorite solution, but still an
> acceptable consensus for me.
>
> > or using an open-coded variant
> > of __is_constexpr() that includes the comparison in a way that avoids the
> > warning.
>
> This is easier said than done. This is the __is_constexpr()
> macro:
>
> | #define __is_constexpr(x) \
> | (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
>
> Good luck doing an open-coded variant of it!
>
> What I mean here is that there definitely might be a smarter
> way than my solution to tackle the issue, but I could not see
> it. If you have any concrete ideas, please do not hesitate to
> share :)
>
>
> Yours sincerely,
> Vincent Mailhol
Thanks,
Al
On Mon, Mar 7, 2022 at 5:10 PM Alexander Lobakin
<[email protected]> wrote:
> From: Vincent MAILHOL <[email protected]>
> Date: Mon, 7 Mar 2022 22:50:56 +0900
...
> For example, people tend to make the following mistake:
>
> unsigned int i;
>
> for (i = 0; i ...) {
> ret = setup_something(array[i]);
> if (ret)
> goto unroll;
> }
>
> unroll:
> while (--i)
> unroll_something(array[i]);
>
> The loop will never end as `i` was declared as unsigned.
> -Wtype-limits catches this.
This looks like a wrapping value issue, not sure if the type limits
makes logical sense. What I'm saying is that the waning is
controversial. It may help or it may make noise.
> Not speaking of checking unsigned variables on < 0:
>
> unsigned int num;
>
> /* calculate_something() returns the number of something
> * or -ERRNO in case of an error
> */
> num = calculate_something();
> if (num < 0)
> ...
Depends on the context. Here is a mistake, but there are plenty of
cases when it's okay to do so. And in the above the variable name is
misleading with its semantics, The proper code should be
unsigned int num;
int ret;
ret = ...
if (ret < 0)
...
num = ret;
Again, the warning is controversial in my opinion.
--
With Best Regards,
Andy Shevchenko
From: Andy Shevchenko <[email protected]>
Date: Fri, 4 Mar 2022 20:46:08 +0200
> On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> <[email protected]> wrote:
> >
>
> > This pattern is harmless but because it occurs in header files
> > (example find_first_bit() from linux/find.h [1]) and because of the
> > include hell, the macro GENMASK_INPUT_CHECK() is accountable for 31%
> > (164714/532484) of all warnings when compiling all modules at W=2
> > level.
Nice catch, thanks! I wanted to submit the very same fix, but
postponed it for some reason, and now here we are.
>
> Have you fixed W=1 warnings?
> Without fixing W=1 (which makes much more sense, when used with
> WERROR=y && COMPILE_TEST=y) this has no value.
How is this connected?
When I do `make W=2 path/to/my/code`, I want to see the actual code
problems, not something that comes from the include files.
When I do `make W=2 path/to/new/code/from/lkml`, I want to see the
actual new warnings, not something coming from the includes.
It's much easier to overlook or miss some real warnings when the
stderr is being flooded by the warnings from the include files.
I'm aware there are some scripts to compare before/after, but I
don't want to use them just because "this has to value".
I don't want to do `make W=2 KCFLAGS='-Wno-shadow -Wno-type-limits'`
because then I'm not able to spot the actual shadow or type limit
problems in my/new code.
I fixed several `-Wshadow` warnings previously in the include files
related to networking, and *nobody* said "this has no value" or
NAKed it. And `-Wshadow` has always been in W=2.
>
> NAK.
...
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks,
Al
On Mon. 7 Mar 2022 at 22:40, Andy Shevchenko <[email protected]> wrote:
> On Mon, Mar 7, 2022 at 1:00 PM Alexander Lobakin
> <[email protected]> wrote:
> > From: Andy Shevchenko <[email protected]>
> > Date: Fri, 4 Mar 2022 20:46:08 +0200
> > > On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> > > <[email protected]> wrote:
> > >
> > > > This pattern is harmless but because it occurs in header files
> > > > (example find_first_bit() from linux/find.h [1]) and because of the
> > > > include hell, the macro GENMASK_INPUT_CHECK() is accountable for 31%
> > > > (164714/532484) of all warnings when compiling all modules at W=2
> > > > level.
> >
> > Nice catch, thanks! I wanted to submit the very same fix, but
> > postponed it for some reason, and now here we are.
> >
> > > Have you fixed W=1 warnings?
> > > Without fixing W=1 (which makes much more sense, when used with
> > > WERROR=y && COMPILE_TEST=y) this has no value.
> >
> > How is this connected?
>
> By priorities.
> I don't see much value in fixing W=2 per se if the code doesn't compile for W=1.
*My code* compiles for W=1. For me, fixing this W=2 in the next in line
if speaking of priorities.
I do not understand why I should be forbidden to fix a W=2 in the
file which I am maintaining on the grounds that some code to which
I do not care still has some W=1.
> > When I do `make W=2 path/to/my/code`, I want to see the actual code
> > problems, not something that comes from the include files.
> > When I do `make W=2 path/to/new/code/from/lkml`, I want to see the
> > actual new warnings, not something coming from the includes.
> > It's much easier to overlook or miss some real warnings when the
> > stderr is being flooded by the warnings from the include files.
> > I'm aware there are some scripts to compare before/after, but I
> > don't want to use them just because "this has to value".
>
> I rephrased above.
>
> > I don't want to do `make W=2 KCFLAGS='-Wno-shadow -Wno-type-limits'`
> > because then I'm not able to spot the actual shadow or type limit
> > problems in my/new code.
> > I fixed several `-Wshadow` warnings previously in the include files
> > related to networking, and *nobody* said "this has no value" or
> > NAKed it. And `-Wshadow` has always been in W=2.
>
> Yes, because people rarely enable COMPILE_TEST + WERROR.
> To be clear, my comment is given in that scope.
And my comments are given in a different scope: a developer who
wants to solve the issue for his *own* file without being spammed.
Yours sincerely,
Vincent Mailhol
On Mon, Mar 7, 2022 at 1:00 PM Alexander Lobakin
<[email protected]> wrote:
> From: Andy Shevchenko <[email protected]>
> Date: Fri, 4 Mar 2022 20:46:08 +0200
> > On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> > <[email protected]> wrote:
> >
> > > This pattern is harmless but because it occurs in header files
> > > (example find_first_bit() from linux/find.h [1]) and because of the
> > > include hell, the macro GENMASK_INPUT_CHECK() is accountable for 31%
> > > (164714/532484) of all warnings when compiling all modules at W=2
> > > level.
>
> Nice catch, thanks! I wanted to submit the very same fix, but
> postponed it for some reason, and now here we are.
>
> > Have you fixed W=1 warnings?
> > Without fixing W=1 (which makes much more sense, when used with
> > WERROR=y && COMPILE_TEST=y) this has no value.
>
> How is this connected?
By priorities.
I don't see much value in fixing W=2 per se if the code doesn't compile for W=1.
> When I do `make W=2 path/to/my/code`, I want to see the actual code
> problems, not something that comes from the include files.
> When I do `make W=2 path/to/new/code/from/lkml`, I want to see the
> actual new warnings, not something coming from the includes.
> It's much easier to overlook or miss some real warnings when the
> stderr is being flooded by the warnings from the include files.
> I'm aware there are some scripts to compare before/after, but I
> don't want to use them just because "this has to value".
I rephrased above.
> I don't want to do `make W=2 KCFLAGS='-Wno-shadow -Wno-type-limits'`
> because then I'm not able to spot the actual shadow or type limit
> problems in my/new code.
> I fixed several `-Wshadow` warnings previously in the include files
> related to networking, and *nobody* said "this has no value" or
> NAKed it. And `-Wshadow` has always been in W=2.
Yes, because people rarely enable COMPILE_TEST + WERROR.
To be clear, my comment is given in that scope.
--
With Best Regards,
Andy Shevchenko
On Mon, Mar 7, 2022 at 11:58 AM Alexander Lobakin
<[email protected]> wrote:
> From: Andy Shevchenko <[email protected]>
> > Have you fixed W=1 warnings?
> > Without fixing W=1 (which makes much more sense, when used with
> > WERROR=y && COMPILE_TEST=y) this has no value.
>
> How is this connected?
> When I do `make W=2 path/to/my/code`, I want to see the actual code
> problems, not something that comes from the include files.
> When I do `make W=2 path/to/new/code/from/lkml`, I want to see the
> actual new warnings, not something coming from the includes.
> It's much easier to overlook or miss some real warnings when the
> stderr is being flooded by the warnings from the include files.
> I'm aware there are some scripts to compare before/after, but I
> don't want to use them just because "this has to value".
> I don't want to do `make W=2 KCFLAGS='-Wno-shadow -Wno-type-limits'`
> because then I'm not able to spot the actual shadow or type limit
> problems in my/new code.
> I fixed several `-Wshadow` warnings previously in the include files
> related to networking, and *nobody* said "this has no value" or
> NAKed it. And `-Wshadow` has always been in W=2.
I agree: if we decide that W=2 warnings are completely useless, we should
either remove the option to build a W=2 kernel or remove some of the warning
flags that go into it. My feeling is that both W=2 in general, and the
Wtype-limits have some value, and that reducing the number of W=2 by
30% as this patch does is a useful goal by itself.
A different question is whether this particular patch is the best
workaround for the warnings, or if a nicer alternative can be found,
such as moving -Wtype-limits to W=3, or using an open-coded variant
of __is_constexpr() that includes the comparison in a way that avoids the
warning.
Arnd
Hi Arnd and Alexander,
Thanks for the support!
On Mon. 7 Mar 2022 at 21:15, Arnd Bergmann <[email protected]> wrote:
> On Mon, Mar 7, 2022 at 11:58 AM Alexander Lobakin
> <[email protected]> wrote:
> > From: Andy Shevchenko <[email protected]>
> > > Have you fixed W=1 warnings?
> > > Without fixing W=1 (which makes much more sense, when used with
> > > WERROR=y && COMPILE_TEST=y) this has no value.
> >
> > How is this connected?
> > When I do `make W=2 path/to/my/code`, I want to see the actual code
> > problems, not something that comes from the include files.
> > When I do `make W=2 path/to/new/code/from/lkml`, I want to see the
> > actual new warnings, not something coming from the includes.
> > It's much easier to overlook or miss some real warnings when the
> > stderr is being flooded by the warnings from the include files.
> > I'm aware there are some scripts to compare before/after, but I
> > don't want to use them just because "this has to value".
> > I don't want to do `make W=2 KCFLAGS='-Wno-shadow -Wno-type-limits'`
> > because then I'm not able to spot the actual shadow or type limit
> > problems in my/new code.
> > I fixed several `-Wshadow` warnings previously in the include files
> > related to networking, and *nobody* said "this has no value" or
> > NAKed it. And `-Wshadow` has always been in W=2.
>
> I agree: if we decide that W=2 warnings are completely useless, we should
> either remove the option to build a W=2 kernel or remove some of the warning
> flags that go into it. My feeling is that both W=2 in general, and the
> Wtype-limits have some value, and that reducing the number of W=2 by
> 30% as this patch does is a useful goal by itself.
>
> A different question is whether this particular patch is the best
> workaround for the warnings, or if a nicer alternative can be found,
> such as moving -Wtype-limits to W=3,
I disagree with moving it to W=3 for two reasons:
1/ This would just move the issue elsewhere. If I had to
compile with W=3 (which I admittedly *almost* never do), the
-Wtype-limits spam would still be there.
2/ After this patch, the number of remaining -Wtype-limits
drops to only 431 for an allyesconfig (and I guess that there
are a fair amount of true positives here). This warning is not
*as broken* as people think. W=2 is a good place I think.
That said, moving it to W=3 would still solve the core issue: W=2
being spammed. Definitely not my favorite solution, but still an
acceptable consensus for me.
> or using an open-coded variant
> of __is_constexpr() that includes the comparison in a way that avoids the
> warning.
This is easier said than done. This is the __is_constexpr()
macro:
| #define __is_constexpr(x) \
| (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
Good luck doing an open-coded variant of it!
What I mean here is that there definitely might be a smarter
way than my solution to tackle the issue, but I could not see
it. If you have any concrete ideas, please do not hesitate to
share :)
Yours sincerely,
Vincent Mailhol
On Tue, Mar 8, 2022 at 2:22 PM Vincent MAILHOL
<[email protected]> wrote:
> On Tue. 8 Mar 2022 at 01:33, Andy Shevchenko <[email protected]> wrote:
> > On Mon, Mar 7, 2022 at 4:06 PM Vincent MAILHOL
> > <[email protected]> wrote:
> > > On Mon. 7 Mar 2022 at 22:40, Andy Shevchenko <[email protected]> wrote:
...
> > > I do not understand why I should be forbidden to fix a W=2 in the
> > > file which I am maintaining on the grounds that some code to which
> > > I do not care still has some W=1.
> >
> > It's not forbidden. I said something different.
> >
> > Whatever, thank you for doing it, perhaps we will have less noise in W=2 case.
>
> Great! So does it mean you are withdrawing your NAK?
> Or do you still have concern on the patch itself?
I'm not stopping you from amending a commit message (including
dropping noise from it and leaving only a partial example of the
compiler complaint), Cc'ing a new version to the people from this and
the discussion mentioned previously.
--
With Best Regards,
Andy Shevchenko
On Tue. 8 Mar 2022 at 01:33, Andy Shevchenko <[email protected]> wrote:
> On Mon, Mar 7, 2022 at 4:06 PM Vincent MAILHOL
> <[email protected]> wrote:
> > On Mon. 7 Mar 2022 at 22:40, Andy Shevchenko <[email protected]> wrote:
> > > On Mon, Mar 7, 2022 at 1:00 PM Alexander Lobakin
> > > <[email protected]> wrote:
> > > > From: Andy Shevchenko <[email protected]>
> > > > Date: Fri, 4 Mar 2022 20:46:08 +0200
> > > > > On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> > > > > <[email protected]> wrote:
>
> ...
>
> > > > > Have you fixed W=1 warnings?
> > > > > Without fixing W=1 (which makes much more sense, when used with
> > > > > WERROR=y && COMPILE_TEST=y) this has no value.
> > > >
> > > > How is this connected?
> > >
> > > By priorities.
> > > I don't see much value in fixing W=2 per se if the code doesn't compile for W=1.
> >
> > *My code* compiles for W=1. For me, fixing this W=2 in the next in line
> > if speaking of priorities.
>
> > I do not understand why I should be forbidden to fix a W=2 in the
> > file which I am maintaining on the grounds that some code to which
> > I do not care still has some W=1.
>
> It's not forbidden. I said something different.
>
> Whatever, thank you for doing it, perhaps we will have less noise in W=2 case.
Great! So does it mean you are withdrawing your NAK?
Or do you still have concern on the patch itself?
Yours sincerely,
Vincent Mailhol
On Mon, Mar 7, 2022 at 4:06 PM Vincent MAILHOL
<[email protected]> wrote:
> On Mon. 7 Mar 2022 at 22:40, Andy Shevchenko <[email protected]> wrote:
> > On Mon, Mar 7, 2022 at 1:00 PM Alexander Lobakin
> > <[email protected]> wrote:
> > > From: Andy Shevchenko <[email protected]>
> > > Date: Fri, 4 Mar 2022 20:46:08 +0200
> > > > On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> > > > <[email protected]> wrote:
...
> > > > Have you fixed W=1 warnings?
> > > > Without fixing W=1 (which makes much more sense, when used with
> > > > WERROR=y && COMPILE_TEST=y) this has no value.
> > >
> > > How is this connected?
> >
> > By priorities.
> > I don't see much value in fixing W=2 per se if the code doesn't compile for W=1.
>
> *My code* compiles for W=1. For me, fixing this W=2 in the next in line
> if speaking of priorities.
> I do not understand why I should be forbidden to fix a W=2 in the
> file which I am maintaining on the grounds that some code to which
> I do not care still has some W=1.
It's not forbidden. I said something different.
Whatever, thank you for doing it, perhaps we will have less noise in W=2 case.
--
With Best Regards,
Andy Shevchenko
On Tue. 8 Mar 2022 à 01:30, Andy Shevchenko <[email protected]> wrote:
> On Mon, Mar 7, 2022 at 5:10 PM Alexander Lobakin
> <[email protected]> wrote:
> > From: Vincent MAILHOL <[email protected]>
> > Date: Mon, 7 Mar 2022 22:50:56 +0900
>
> ...
>
> > For example, people tend to make the following mistake:
> >
> > unsigned int i;
> >
> > for (i = 0; i ...) {
> > ret = setup_something(array[i]);
> > if (ret)
> > goto unroll;
> > }
> >
> > unroll:
> > while (--i)
> > unroll_something(array[i]);
> >
> > The loop will never end as `i` was declared as unsigned.
> > -Wtype-limits catches this.
>
> This looks like a wrapping value issue, not sure if the type limits
> makes logical sense. What I'm saying is that the waning is
> controversial. It may help or it may make noise.
>
> > Not speaking of checking unsigned variables on < 0:
> >
> > unsigned int num;
> >
> > /* calculate_something() returns the number of something
> > * or -ERRNO in case of an error
> > */
> > num = calculate_something();
> > if (num < 0)
> > ...
>
> Depends on the context. Here is a mistake, but there are plenty of
> cases when it's okay to do so.
I am curious to see which case you are thinking of.
Personally, I see two cases, both with macro:
1/ Cases similar to GENMASK_INPUT_CHECK() in which the macro is
made for generic purpose and in which there was no way to know
in advance that one parameter would be unsigned and the other
zero.
2/ Comparaison again a macro which might or might not be
zero. e.g.:
#ifdef FOO
#define BAR 42
#else
#define BAR 0
#endif
void baz(void)
{
unsigned int i;
if (i > BAR) /* yields -Wtype-limits if FOO is not defined. */
}
And because of those two false positives, moving it to W=2 was a
wise choice.
But I am not aware of any use cases outside of macro where doing
an:
unsigned int num;
/* ... */
if (num < 0)
would be okay. At best it is dead code, at worse, it is a bug as
pointed out by Alexander.
I am not sure what I am missing here.
> And in the above the variable name is
> misleading with its semantics, The proper code should be
>
> unsigned int num;
> int ret;
>
> ret = ...
> if (ret < 0)
> ...
> num = ret;
>
> Again, the warning is controversial in my opinion.
>
> --
> With Best Regards,
> Andy Shevchenko