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

[Rikard Falkeborn <rikard.falkeborn@gmail.com>]

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

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel