Date:   Mon, 7 Mar 2022 22:50:56 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Rikard Falkeborn <rikard.falkeborn@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

Hi Arnd and Alexander,

Thanks for the support!

On Mon. 7 Mar 2022 at 21:15, Arnd Bergmann <arnd@...db.de> wrote:
> On Mon, Mar 7, 2022 at 11:58 AM Alexander Lobakin
> <alexandr.lobakin@...el.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@...il.com>
> > > 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?
> > When I do `make W=2 path/to/my/code`, I want to see the actual code
> > problems, not something that comes from the include files.
> > When I do `make W=2 path/to/new/code/from/lkml`, I want to see the
> > actual new warnings, not something coming from the includes.
> > It's much easier to overlook or miss some real warnings when the
> > stderr is being flooded by the warnings from the include files.
> > I'm aware there are some scripts to compare before/after, but I
> > don't want to use them just because "this has to value".
> > I don't want to do `make W=2 KCFLAGS='-Wno-shadow -Wno-type-limits'`
> > because then I'm not able to spot the actual shadow or type limit
> > problems in my/new code.
> > I fixed several `-Wshadow` warnings previously in the include files
> > related to networking, and *nobody* said "this has no value" or
> > NAKed it. And `-Wshadow` has always been in W=2.
>
> I agree: if we decide that W=2 warnings are completely useless, we should
> either remove the option to build a W=2 kernel or remove some of the warning
> flags that go into it. My feeling is that both W=2 in general, and the
> Wtype-limits have some value, and that reducing the number of W=2 by
> 30% as this patch does is a useful goal by itself.
>
> A different question is whether this particular patch is the best
> workaround for the warnings, or if a nicer alternative can be found,
> such as moving -Wtype-limits to W=3,

I disagree with moving it to W=3 for two reasons:

  1/ This would just move the issue elsewhere. If I had to
  compile with W=3 (which I admittedly *almost* never do), the
  -Wtype-limits spam would still be there.

  2/ After this patch, the number of remaining -Wtype-limits
  drops to only 431 for an allyesconfig (and I guess that there
  are a fair amount of true positives here). This warning is not
  *as broken* as people think. W=2 is a good place I think.

That said, moving it to W=3 would still solve the core issue: W=2
being spammed. Definitely not my favorite solution, but still an
acceptable consensus for me.

> or using an open-coded variant
> of __is_constexpr() that includes the comparison in a way that avoids the
> warning.

This is easier said than done. This is the __is_constexpr()
macro:

| #define __is_constexpr(x) \
|     (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))

Good luck doing an open-coded variant of it!

What I mean here is that there definitely might be a smarter
way than my solution to tackle the issue, but I could not see
it. If you have any concrete ideas, please do not hesitate to
share :)


Yours sincerely,
Vincent Mailhol

Powered by blists - more mailing lists