Re: [PATCH] linux/bits.h: Squash unsigned comparison warning for GENMASK

[Yazen Ghannam <yazen.ghannam@amd.com>]

On Tue, Feb 04, 2025 at 04:26:20PM -0500, Yury Norov wrote:
> On Tue, Feb 04, 2025 at 05:13:16PM +0000, Yazen Ghannam wrote:
> > Cast inputs to 'long' to avoid the following 'type-limits' warning:
> >   warning: comparison of unsigned expression in ‘< 0’ is always false
> > 
> > The 'long' type can hold +/- 2G which far exceeds valid inputs for the
> > GENMASK helpers (current max use is 128 bits).
> > 
> > Idea is similar to implementation in __is_nonneg().
> > 
> > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> > ---
> > Note to maintainers:
> > I found some previous discussions on this topic in the mailing list
> > archives. The upstream code has changed a bit since then, and this
> > proposed solution seems fairly simple when based on the latest code.
> > 
> > I figured I'd look at something outside my normal focus areas. I
> > apologize for the noise if this solution is too naive or incomplete.
> > 
> > Thanks!
> 
> Hi Yazen,
> 
> Wtype-limits is enabled in W=2 only, see scripts/Makefile.extrawarn.
> We normally shouldn't see this type of warnings even when compiling
> with W=1, at all. 
> 
> We have quite a lot callers in kernel already that do GENMASK(xxx, 0)
> 
>   yury:linux$ git grep GENMASK | grep 0\) | wc -l
>   13788
> 
> And nobody complained so far. 

Right, this is with W=2.

I focus mostly on x86 MCE, and I was doing some extra checking.

> 
> Still, I tried to compile a small userspace app that calls
> __GENMASK(10,0), and found no warnings with Wall, Wextra and
> Wtype-limits enabled.

The warning comes from the GENMASK_INPUT_CHECK(). Using __GENMASK()
would bypass this, correct?

> 
> Can you share more about your compiler, compilation command and config?

Compilers with warning:	GCC 13 and 14
Compilation command:	make W=2 arch/x86/kernel/cpu/mce/
Config:			make defconfig (on x86_64)

I don't see the same warnings with LLVM/clang. The '-Wextra' flag does
not include '-Wtype-limits' (at least on clang 18 and 19). But I still
don't see the same warning when I add it.

Maybe other options modulate the behavior? I'll do some more digging.

> 
> > ---
> >  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 61a75d3f294b..318346f2a5a6 100644
> > --- a/include/linux/bits.h
> > +++ b/include/linux/bits.h
> > @@ -21,7 +21,7 @@
> >  #if !defined(__ASSEMBLY__)
> >  #include <linux/build_bug.h>
> >  #include <linux/compiler.h>
> > -#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
> > +#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((long)(l) > (long)(h)))
> 
> This implies that GENMASK() is OK with signed input, so negative too.
> It's not true. For me typecasting to a signed type just to shut the
> compiler is a bad idea

The cast only affects the compile-time checker. Negative values would
still cause compilation errors in the __GENMASK() helpers. The same is
true for out-of-bound values like '64' for GENMASK_ULL(), etc.

Thanks,
Yazen