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@xxxxxxxxx>
> > > Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > Signed-off-by: Emil Velikov <emil.l.velikov@xxxxxxxxx>
> > > Reported-by: kbuild test robot <lkp@xxxxxxxxx>
> > > Reported-by: kbuild test robot <lkp@xxxxxxxxx>
> > > ---
> > > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
