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@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, Mar 8, 2022 at 6:12 AM Vincent Mailhol
> <mailhol.vincent@xxxxxxxxxx> 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