Date:   Mon,  7 Mar 2022 16:07:50 +0100
From:   Alexander Lobakin <alexandr.lobakin@...el.com>
To:     Vincent MAILHOL <mailhol.vincent@...adoo.fr>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        Arnd Bergmann <arnd@...db.de>,
        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()

From: Vincent MAILHOL <mailhol.vincent@...adoo.fr>
Date: Mon, 7 Mar 2022 22:50:56 +0900

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

Agree, W=2 is the best place for -Wtype-limits to me.
I've never seen a single useful warning on W=3, but there were
lots of them on W=2 which revealed some real bugs. That said, it
makes no sense at all to even try to check all new code from LKML
for warnings with W=3, but it's actually feasible to make it build
cleanly with W=2 most of times.

Re -Wtype-limits in particular, it's not useless at all.
For example, people tend to make the following mistake:

	unsigned int i;

	for (i = 0; i ...) {
		ret = setup_something(array[i]);
		if (ret)
			goto unroll;
	}

unroll:
	while (--i)
		unroll_something(array[i]);

The loop will never end as `i` was declared as unsigned.
-Wtype-limits catches this.

Not speaking of checking unsigned variables on < 0:

	unsigned int num;

	/* calculate_something() returns the number of something
	 * or -ERRNO in case of an error
	 */
	num = calculate_something();
	if (num < 0)
		...

Catches as well.

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

Thanks,
Al

Powered by blists - more mailing lists