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