Date:   Wed, 27 Apr 2022 07:04:18 -0700
From:   Yury Norov <yury.norov@...il.com>
To:     Vincent MAILHOL <mailhol.vincent@...adoo.fr>
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

On Wed, Apr 27, 2022 at 11:58:58AM +0900, Vincent MAILHOL wrote:
> + 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.

Globally, we have not yet fixed W=1, that's why W=2 isn't that important.
(If the above statement is wrong - can someone explain me the logic of
splitting warnings by levels?)

Locally you cleared W=1, which is great, and I understand that you'd 
like to have clean W=2 too.
 
> > 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?

__diag_ignore() is not hacky when it's well-justified. Globally
there's a single user of __diag_ignore() - SYSCALL_DEFINE, and
it looks well-justified to me:
    The new warning seems reasonable in principle, but it doesn't
    help us here, since we rely on the type mismatch to sanitize the
    system call arguments. After I reported this as GCC PR82435, a new
    -Wno-attribute-alias option was added that could be used to turn the
    warning off globally on the command line, but I'd prefer to do it a
    little more fine-grained.

Locally, there are just 3 users of the macro in the codebase. All
they appeal to local issues, i.e. don't shut up warnings because
they are broken.

In case of Wtype-limits, the proposed solution is hacky because it
silences broken warning instead of fixing compiler.

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

1. Yes, do blame GCC and disable Wtype-limits locally where W=1
   is clean.
2. Use clang.
3. Move Wtype-limits to W=3.
4. Test Wtype-limits in Makefile, and enable it if not broken.

My main objection is that this patch puts dirt in *my* selfish area
of responsibility. The code that causes issues is GENMASK_INPUT_CHECK,
but the suggested patch modifies find.h - an innocent random user.

The argument that we need to silence find_bit because it clears 34%
of warnings doesn't work for me. Instead, we'd push GCC community
to provide proper fix and clear 34% of warnings.

Thanks,
Yury

Powered by blists - more mailing lists