Re: [PATCH v2] linux/bits.h: adjust GENMASK_INPUT_CHECK() check

[Emil Velikov <emil.l.velikov@gmail.com>]

Hi Rikard,


On 2020/05/19, Rikard Falkeborn wrote:
> + Andrew et al who recieved mail from the build robot this morning about
> the same issue.
> 
> On Tue, May 19, 2020 at 10:14:52PM +0100, Emil Velikov wrote:
> > Recently the GENMASK_INPUT_CHECK() was added, aiming to catch cases
> > where there GENMASK arguments are flipped.
> > 
> > Although it seems to be triggering -Wtype-limits in the following cases:
> > 
> >    unsigned foo = (10 + x);
> >    unsigned bar = GENMASK(foo, 0);
> > 
> >    const unsigned foo = (10 + x);
> >    unsigned bar = GENMASK(foo, 0);
> > 
> > Here are the warnings, from my GCC 9.2 box.
> > 
> > warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> >    __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> >                             ^
> > warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> >    __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> >                                         ^
> > 
> > This results in people disabling the warning all together or promoting
> > foo to signed. Either of which being a sub par option IMHO.
> > 
> > Add a trivial "+ 1" to each h and l in the constant expression.
> > 
> > v2: drop accidental !
> > 
> > Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of
> > GENMASK inputs")
> > Cc: Rikard Falkeborn <rikard.falkeborn@gmail.com>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> > Reported-by: kbuild test robot <lkp@intel.com>
> > Reported-by: kbuild test robot <lkp@intel.com>
> > ---
> >  include/linux/bits.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > index 4671fbf28842..02a42866d198 100644
> > --- a/include/linux/bits.h
> > +++ b/include/linux/bits.h
> > @@ -23,7 +23,7 @@
> >  #include <linux/build_bug.h>
> >  #define GENMASK_INPUT_CHECK(h, l) \
> >  	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > -		__builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > +		__builtin_constant_p((l + 1) > (h + 1)), (l + 1) > (h + 1), 0)))
> 
> You need parentheses around l and h here.
> 
Sure will do.

> I think I would have prefered a cast to int here instead but I'm fine
> with either (I don't think pragmas for disabling the warning can be used
> since the check is added to the mask). Either way, I think we need a
> comment on why this is done.

How about:

Add trivial "+ 1" when to the h/l arguments. Without this GCC will
complain when comparing unsigned vs 0. Depending on the GCC version,
that can happen within __builtin_constant_p and/or the BUILD_BUG_ON_ZERO
macro.


> 
> >  #else
> >  /*
> >   * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> > -- 
> > 2.25.1
> > 
> 
> I can't reproduce this with gcc 10 and kernelci.org does not show the
> warning (but those builds seem to be gcc 8 only, so maybe this is a gcc
> 9 thing only). A bit strange this shows up now, it's been in Linus's
> tree for six weeks and in next for even longer, but oh well.
> 
I would imagine that people either use "interesting" workarounds like
this [1], or outright disable -Wtype-limits - grep for Wtype-limits.

I'm glad that GCC 10 is saner, although it's far from being the minimum
required for building the kernel :-\


Let me know if the above comment works for you and I'll send out the
next revision.


Thanks
Emil

[1]
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=2803aa743fd38f66acca555ae6e5fc677bb71251
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel