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

[Rikard Falkeborn]

On Fri, May 22, 2020 at 07:50:19PM +0100, Emil Velikov wrote:
> 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 at gmail.com>
> > > Cc: Linus Torvalds <torvalds at linux-foundation.org>
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > Cc: dri-devel at lists.freedesktop.org
> > > Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
> > > Reported-by: kbuild test robot <lkp at intel.com>
> > > Reported-by: kbuild test robot <lkp at 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.
> 

That works for me, but I'm not the one you need to convince. :)

Rikard

> 
> Thanks
> Emil
> 
> [1]
> https://cgit.freedesktop.org/drm/drm-misc/commit/?id=2803aa743fd38f66acca555ae6e5fc677bb71251