[PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Vincent Mailhol]

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 <mailhol.vincent@wanadoo.fr>
---
 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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Andy Shevchenko]

On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
<mailhol.vincent@wanadoo.fr> 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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Vincent MAILHOL]

On Tue. 5 Mar 2022 at 03:46, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> <mailhol.vincent@wanadoo.fr> 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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Andy Shevchenko]

On Sat, Mar 5, 2022 at 2:43 PM Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> On Tue. 5 Mar 2022 at 03:46, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> > <mailhol.vincent@wanadoo.fr> 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/cover.1590017578.git.syednwaris@gmail.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Vincent MAILHOL]

On Sun. 6 Mar 2022 at 06:33, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Sat, Mar 5, 2022 at 2:43 PM Vincent MAILHOL
> <mailhol.vincent@wanadoo.fr> wrote:
> > On Tue. 5 Mar 2022 at 03:46, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> > > <mailhol.vincent@wanadoo.fr> 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/cover.1590017578.git.syednwaris@gmail.com/

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/20200709123011.GA18734@gondor.apana.org.au/

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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Andy Shevchenko]

On Sun, Mar 6, 2022 at 7:35 AM Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> On Sun. 6 Mar 2022 at 06:33, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Sat, Mar 5, 2022 at 2:43 PM Vincent MAILHOL
> > <mailhol.vincent@wanadoo.fr> wrote:
> > > On Tue. 5 Mar 2022 at 03:46, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> > > > <mailhol.vincent@wanadoo.fr> 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/cover.1590017578.git.syednwaris@gmail.com/
>
> 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/20200709123011.GA18734@gondor.apana.org.au/
>
> 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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Alexander Lobakin]

From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Fri, 4 Mar 2022 20:46:08 +0200

> On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> <mailhol.vincent@wanadoo.fr> 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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Arnd Bergmann]

On Mon, Mar 7, 2022 at 11:58 AM Alexander Lobakin
<alexandr.lobakin@intel.com> wrote:
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > 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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Vincent MAILHOL]

Hi Arnd and Alexander,

Thanks for the support!

On Mon. 7 Mar 2022 at 21:15, Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Mar 7, 2022 at 11:58 AM Alexander Lobakin
> <alexandr.lobakin@intel.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > 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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Alexander Lobakin]

From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
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 <arnd@arndb.de> wrote:
> > On Mon, Mar 7, 2022 at 11:58 AM Alexander Lobakin
> > <alexandr.lobakin@intel.com> wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > 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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Andy Shevchenko]

On Mon, Mar 7, 2022 at 5:10 PM Alexander Lobakin
<alexandr.lobakin@intel.com> wrote:
> From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
> 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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Vincent MAILHOL]

On Tue. 8 Mar 2022 à 01:30, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Mar 7, 2022 at 5:10 PM Alexander Lobakin
> <alexandr.lobakin@intel.com> wrote:
> > From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
> > 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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Andy Shevchenko]

On Mon, Mar 7, 2022 at 1:00 PM Alexander Lobakin
<alexandr.lobakin@intel.com> wrote:
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Date: Fri, 4 Mar 2022 20:46:08 +0200
> > On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> > <mailhol.vincent@wanadoo.fr> 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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Vincent MAILHOL]

On Mon. 7 Mar 2022 at 22:40, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Mar 7, 2022 at 1:00 PM Alexander Lobakin
> <alexandr.lobakin@intel.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Date: Fri, 4 Mar 2022 20:46:08 +0200
> > > On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> > > <mailhol.vincent@wanadoo.fr> 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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Andy Shevchenko]

On Mon, Mar 7, 2022 at 4:06 PM Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> On Mon. 7 Mar 2022 at 22:40, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Mon, Mar 7, 2022 at 1:00 PM Alexander Lobakin
> > <alexandr.lobakin@intel.com> wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Date: Fri, 4 Mar 2022 20:46:08 +0200
> > > > On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> > > > <mailhol.vincent@wanadoo.fr> 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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Vincent MAILHOL]

On Tue. 8 Mar 2022 at 01:33, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Mar 7, 2022 at 4:06 PM Vincent MAILHOL
> <mailhol.vincent@wanadoo.fr> wrote:
> > On Mon. 7 Mar 2022 at 22:40, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Mon, Mar 7, 2022 at 1:00 PM Alexander Lobakin
> > > <alexandr.lobakin@intel.com> wrote:
> > > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > Date: Fri, 4 Mar 2022 20:46:08 +0200
> > > > > On Fri, Mar 4, 2022 at 7:36 PM Vincent Mailhol
> > > > > <mailhol.vincent@wanadoo.fr> 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

Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Andy Shevchenko]

On Tue, Mar 8, 2022 at 2:22 PM Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> On Tue. 8 Mar 2022 at 01:33, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Mon, Mar 7, 2022 at 4:06 PM Vincent MAILHOL
> > <mailhol.vincent@wanadoo.fr> wrote:
> > > On Mon. 7 Mar 2022 at 22:40, Andy Shevchenko <andy.shevchenko@gmail.com> 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

[PATCH v2] linux/bits.h: GENMASK_INPUT_CHECK: reduce W=2 noise by 31% treewide

[Vincent Mailhol]

This patch silences a -Wtypes-limits warning in GENMASK_INPUT_CHECK()
which is accountable for 31% of all warnings when compiling with W=2.

Indeed, GENMASK_INPUT_CHECK() will generate some warnings at W=2 level
if invoked with an unsigned integer and zero. For example, this:

| #include <linux/bits.h>
| unsigned int foo(unsigned int bar)
| { return GENMASK(bar, 0); }

would yield 30 lines of warning. Extract:

| 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)))
|       |                                    ^

This pattern is harmless (false positive) and for that reason,
-Wtypes-limits was moved to W=2. c.f. [1].

However because it occurs in header files (example: find_first_bit()
from linux/find.h [2]), GENMASK_INPUT_CHECK() is accountable for
roughly 31% (164714/532484) of all W=2 warnings for an
allyesconfig. This is an issue because that noise makes it harder to
triage and find relevant W=2 warnings.

Reference (using gcc 11.2, linux v5.17-rc6 on x86_64):

| $ 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

In this patch, we silence 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().

[1] https://lore.kernel.org/lkml/20200708190756.16810-1-rikard.falkeborn@gmail.com/
[2] https://elixir.bootlin.com/linux/v5.17-rc6/source/include/linux/find.h#L119

Link: https://lore.kernel.org/lkml/cover.1590017578.git.syednwaris@gmail.com/
Link: https://lore.kernel.org/lkml/20220304124416.1181029-1-mailhol.vincent@wanadoo.fr/
Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of GENMASK inputs")
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
* Changelog *

v1 -> v2:

  * Rewrote the commit message to make it less verbose
  * Add in CC all people from:
  https://lore.kernel.org/lkml/cover.1590017578.git.syednwaris@gmail.com/
---
 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

Re: [PATCH v2] linux/bits.h: GENMASK_INPUT_CHECK: reduce W=2 noise by 31% treewide

[Linus Torvalds]

On Tue, Mar 8, 2022 at 6:12 AM Vincent Mailhol
<mailhol.vincent@wanadoo.fr> wrote:
>
> This patch silences a -Wtypes-limits warning in GENMASK_INPUT_CHECK()
> which is accountable for 31% of all warnings when compiling with W=2.

Please, just make the patch be "remote -Wtypes-limits".

Instead of making an already complicated check more complicated, and
making it more fragile.

I don't see why that int cast on h would be valid, for example. Why
just h? And should you not then check that the cast doesn't actually
change the value?

But the basic issue is that the compiler warns about bad things, and
the problem isn't the code, but the compiler.

                      Linus

Re: [PATCH v2] linux/bits.h: GENMASK_INPUT_CHECK: reduce W=2 noise by 31% treewide

[Vincent MAILHOL]

Hi Linus,

On Wed. 9 Mar 2022 at 03:13, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Mar 8, 2022 at 6:12 AM Vincent Mailhol
> <mailhol.vincent@wanadoo.fr> wrote:
> >
> > This patch silences a -Wtypes-limits warning in GENMASK_INPUT_CHECK()
> > which is accountable for 31% of all warnings when compiling with W=2.
>
> Please, just make the patch be "remote -Wtypes-limits".

After this patch, the number of remaining -Wtype-limits drops by
99.7% from 164714 to only 431 for an allyesconfig (some of which
could be true positives). So I am inclined to keep
-Wtype-limits at W=2 because it still catches some relevant
issues. Aside from the issue pointed out here, it is not a hindrance.

> Instead of making an already complicated check more complicated, and
> making it more fragile.

ACK, this patch makes it more complicated. About making it more
fragile, lib/test_bits.c is here to catch issues and this patch
passes those tests including the TEST_GENMASK_FAILURES.

> I don't see why that int cast on h would be valid, for example. Why
> just h?

The compiler only complains on ((unsigned int)foo > 0) patterns,
i.e. when h is unsigned and l is zero. The signness of l is not relevant
here.

> And should you not then check that the cast doesn't actually
> change the value?

The loss of precision only occurs on big values
e.g. GENMASK(UINT_MAX + 1, 0).

GENMASK (and GENMASK_ULL) already requires h and l to be between
0 and 31 (or 63). Out of band positive values are caught by
-Wshift-count-overflow (and negative values by
-Wshift-count-negative).

So the use cases in which the int cast would change h value are
already caught elsewhere.

> But the basic issue is that the compiler warns about bad things, and
> the problem isn't the code, but the compiler.

ACK, the code is not broken, the compiler is guilty. I tend to
agree to the rule "if not broken, don’t fix", but I consider this
patch to be *the exception* because of the outstanding level of
noise generated here.

If my message did not convince you, then I am fine to move
-Wtypes-limits from W=2 to W=3 as a compromise. But this is not
my preferred solution because some -Wtypes-limits warnings are
useful.


Yours sincerely,
Vincent Mailhol