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

Commit Message

Emil Velikov May 19, 2020, 9:14 p.m. UTC

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 <[e-mail]>
Cc: Linus Torvalds <[e-mail]>
Cc: Chris Wilson <[e-mail]>
Cc: [e-mail]
Signed-off-by: Emil Velikov <[e-mail]>
Reported-by: kbuild test robot <[e-mail]>
Reported-by: kbuild test robot <[e-mail]>
---
 include/linux/bits.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Rikard Falkeborn May 19, 2020, 9:28 p.m. UTC | #1

+ 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 <[e-mail]>
> Cc: Linus Torvalds <[e-mail]>
> Cc: Chris Wilson <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Emil Velikov <[e-mail]>
> Reported-by: kbuild test robot <[e-mail]>
> Reported-by: kbuild test robot <[e-mail]>
> ---
>  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.

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.

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

Rikard

Emil Velikov May 22, 2020, 6:50 p.m. UTC | #2

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 <[e-mail]>
> > Cc: Linus Torvalds <[e-mail]>
> > Cc: Chris Wilson <[e-mail]>
> > Cc: [e-mail]
> > Signed-off-by: Emil Velikov <[e-mail]>
> > Reported-by: kbuild test robot <[e-mail]>
> > Reported-by: kbuild test robot <[e-mail]>
> > ---
> >  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

Rikard Falkeborn May 23, 2020, 7:35 p.m. UTC | #3

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 <[e-mail]>
> > > Cc: Linus Torvalds <[e-mail]>
> > > Cc: Chris Wilson <[e-mail]>
> > > Cc: [e-mail]
> > > Signed-off-by: Emil Velikov <[e-mail]>
> > > Reported-by: kbuild test robot <[e-mail]>
> > > Reported-by: kbuild test robot <[e-mail]>
> > > ---
> > >  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

Patch

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)))
 #else
 /*
  * BUILD_BUG_ON_ZERO is not available in h files included from asm files,