Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Tue, 26 Apr 2022 13:42:49 -0700
From:   Yury Norov <yury.norov@gmail.com>
To:     Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Tom Rix <trix@redhat.com>, linux-kernel@vger.kernel.org,
        Arnd Bergmann <arnd@arndb.de>, gcc@gcc.gnu.org,
        Rikard Falkeborn <rikard.falkeborn@gmail.com>
Subject: Re: [PATCH] linux/find: ignore -Wtype-limits to reduce W=2 warnings
 by 34% tree-wide
Message-ID: <YmhZSZWg9YZZLRHA@yury-laptop>
References: <20220426161658.437466-1-mailhol.vincent@wanadoo.fr>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220426161658.437466-1-mailhol.vincent@wanadoo.fr>
Precedence: bulk

+ gcc@gcc.gnu.org
+ Rikard Falkeborn <rikard.falkeborn@gmail.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?

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.

Thanks,
Yury

> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  include/linux/find.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/find.h b/include/linux/find.h
> index 5bb6db213bcb..efd4b3f7dd17 100644
> --- a/include/linux/find.h
> +++ b/include/linux/find.h
> @@ -103,6 +103,10 @@ unsigned long find_next_zero_bit(const unsigned long *addr, unsigned long size,
>  }
>  #endif
>  
> +__diag_push();
> +__diag_ignore(GCC, 8, "-Wtype-limits",
> +	      "GENMASK(size - 1, 0) yields 'comparison of unsigned expression in < 0 is always false' which is OK");
> +
>  #ifndef find_first_bit
>  /**
>   * find_first_bit - find the first set bit in a memory region
> @@ -193,6 +197,8 @@ unsigned long find_last_bit(const unsigned long *addr, unsigned long size)
>  }
>  #endif
>  
> +__diag_pop(); /* ignore -Wtype-limits */
> +
>  /**
>   * find_next_clump8 - find next 8-bit clump with set bits in a memory region
>   * @clump: location to store copy of found clump
> -- 
> 2.35.1