linux-kernel - Re: [PATCH] linux/find: ignore -Wtype-limits to reduce W=2 warnings by 34% tree-wide

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMZ6RqJuUPuEJQvyHZr0Gxzh9ZZ2iACTHe3XE70jZ38hmePfuA@mail.gmail.com>
Date:   Wed, 27 Apr 2022 11:58:58 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Yury Norov <yury.norov@...il.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Tom Rix <trix@...hat.com>, linux-kernel@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>, gcc@....gnu.org,
        Rikard Falkeborn <rikard.falkeborn@...il.com>,
        Alexander Lobakin <alexandr.lobakin@...el.com>
Subject: Re: [PATCH] linux/find: ignore -Wtype-limits to reduce W=2 warnings
 by 34% tree-wide

+ Alexander Lobakin <alexandr.lobakin@...el.com>

On Wed. 27 Apr 2022 at 05:42, Yury Norov <yury.norov@...il.com> wrote:
> + gcc@....gnu.org
> + Rikard Falkeborn <rikard.falkeborn@...il.com>
>
> On Wed, Apr 27, 2022 at 01:16:58AM +0900, Vincent Mailhol wrote:
> > find_first_bit(), find_first_and_bit(), find_first_and_bit() and
> > find_first_and_bit() all invokes GENMASK(size - 1, 0).
> >
> > This triggers below warning when compiled with W=2.
> >
> > | ./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);
> > |       |                                             ^~~~~~~
> >
> > linux/find.h being a widely used header file, above warning show up in
> > thousand of files which include this header (either directly on
> > indirectly).
> >
> > Because it is a false positive, we just silence -Wtype-limits flag
> > locally to remove the spam. clang does not warn about it, so we just
> > apply the diag_ignore() directive to gcc.
> >
> > By doing so, the warnings for a W=2 build are reduced by
> > 34%. Benchmark was done with gcc 11.2.1 on Linux v5.17 x86_64
> > allyesconfig (except CONFIG_WERROR). Beforethe patch: 515496 warnings
> > and after: 340097.
> >
> > For reference, past proposal to modify GENMASK_INPUT_CHECK() was
> > rejected in:
> > https://lore.kernel.org/all/20220304124416.1181029-1-mailhol.vincent@wanadoo.fr/
>
> So, here is nothing wrong with the kernel code and we have an alternative
> compiler (clang) that doesn't throw Wtype-limits. It sounds to me like an
> internal GCC problem, and I don't understand how hiding broken Wtype-limits
> on kernel side would help people to improve GCC.
>
> On the thread you mentioned above:
>
> > > > > 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.
>
> If you are concerned about a particular driver - why don't you silence
> the warning in there? Or alternatively build it with clang?

Sorry if my previous comments looked selfish. I used the first
person to illustrate my point but because this W=2 appears in
thousands of files my real intent is to fix it for everybody, not
only for myself.

> With all that, I think that the right way to go is to fix the root
> cause of this churn - broken Wtype-limits in GCC, and after that move
> Wtype-limits to W=1. Anything else looks hacky to me.

Why is this use of __diag_ignore() hacky compared when compared
to the other use of __diag_ignore() or the use of -Wno-something
in local Makefiles?

I did my due diligence and researched GCC’s buzilla before
sending this patch. There is an opened ticket here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86647

In a perfect word, yes, all false positives should be fixed in
the compiler, but the reality is that this bug was reported in
July 2018, nearly four years ago. GCC developers have their own
priorities and fixing this bug does not appear to be part of
those. And I do not have the knowledge of GCC's internals to fix
this myself.  So what do we do next, blame GCC and do nothing or
silence it on our side in order to have a mininfull W=2 output?


Yours sincerely,
Vincent Mailhol