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

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

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