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

[Yury Norov <yury.norov@gmail.com>]

On Wed, Feb 05, 2025 at 12:06:51PM -0500, Yazen Ghannam wrote:
> 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?

Yeah.. I actually tried GENMASK(). This is my code. (I have to pull
more macros because I run it against Ubuntu 6.8.0-52-generic kernel)

  $ cat tst.c
  #include <linux/const.h>
  #include <stdio.h>
  
  #define false 0
  #define __is_constexpr(x) \
          (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
  #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
  #define const_true(x) __builtin_choose_expr(__is_constexpr(x), x, false)
  
  #define __BITS_PER_LONG (64)
  #define __GENMASK(h, l) \
          (((~_UL(0)) - (_UL(1) << (l)) + 1) & \
           (~_UL(0) >> (__BITS_PER_LONG - 1 - (h))))
  
  #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
  #define GENMASK(h, l) (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
  
  int main()
  {
  	printf("%lx\n", GENMASK(10,0));
  	return 0;
  }
  $ gcc -Wall -Wextra -Wtype-limits tst.c ; ./a.out
  7ff
  $ gcc --version
  gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0
  Copyright (C) 2023 Free Software Foundation, Inc.
  This is free software; see the source for copying conditions.  There is NO
  warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

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

This looks like a specific compiler issue. Nothing to fix on kernel
side, but you may want to file a bug in GCC.

Thanks,
Yury