[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAMZ6RqJ5Pyup4RgjMA5fG5Lt4tpA_tvv61snpKbF4DJAkPzdAg@mail.gmail.com>
Date: Wed, 9 Mar 2022 11:23:15 +0900
From: Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Rikard Falkeborn <rikard.falkeborn@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Arnd Bergmann <arnd@...db.de>,
Andy Shevchenko <andy.shevchenko@...il.com>,
Kees Cook <keescook@...omium.org>,
Alexander Lobakin <alexandr.lobakin@...el.com>,
Herbert Xu <herbert@...dor.apana.org.au>,
Emil Velikov <emil.l.velikov@...il.com>,
Geert Uytterhoeven <geert@...ux-m68k.org>,
Linus Walleij <linus.walleij@...aro.org>,
linux-arch <linux-arch@...r.kernel.org>,
kernel test robot <lkp@...el.com>,
Syed Nayyar Waris <syednwaris@...il.com>,
William Breathitt Gray <vilhelm.gray@...il.com>,
Masahiro Yamada <yamada.masahiro@...ionext.com>
Subject: Re: [PATCH v2] linux/bits.h: GENMASK_INPUT_CHECK: reduce W=2 noise by
31% treewide
Hi Linus,
On Wed. 9 Mar 2022 at 03:13, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Tue, Mar 8, 2022 at 6:12 AM Vincent Mailhol
> <mailhol.vincent@...adoo.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
Powered by blists - more mailing lists