Message-ID: <20250206020227.GA322224@yaz-khff2.amd.com>
Date: Wed, 5 Feb 2025 21:02:27 -0500
From: Yazen Ghannam <yazen.ghannam@....com>
To: Yury Norov <yury.norov@...il.com>
Cc: Rasmus Villemoes <linux@...musvillemoes.dk>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] linux/bits.h: Squash unsigned comparison warning for
 GENMASK

On Wed, Feb 05, 2025 at 03:59:49PM -0500, Yury Norov wrote:
> 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@....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.
> 

I'm able to reproduce the issue with your test code by using a variable
for the 'h' parameter. I assume many of the warnings around the kernel
may fall into this case, i.e. 'h' is 'unsigned' variable and 'l' is '0'.

>From the GCC docs:
Warn if a comparison is always true or always false due to the limited
range of the data type, but do not warn for constant expressions.

So it seems the warning only applies to non-constant expression, hence
the need for the 'h' parameter to be a unsigned variable to trip the
warning.

My changes to your tst.c:

	int main(void)
	{
	        unsigned int TEST;
	
	        printf("%lx\n", GENMASK(TEST,0));
	        return 0;
	}

When compiling and keeping the intermediate files, I see the following
for the __is_constexpr() macro:

(sizeof(int) == sizeof(*(8 ? ((void *)((long)((0) > (TEST)) * 0l)) : (int *)8))), (0) > (TEST), 0)))

The "(0) > (TEST)" check seems to be the issue in that "TEST" is
'unsigned int'. So it seems that the GCC warning is correct, AFAICT.

Clang generates the same code, but doesn't complain. So I'm inclined to
think something is different with its checking. It does catch a simple
case like "if (0 > TEST)", so maybe there's something that gets applied
differently through the macros.

Maybe Clang immediately fails the __builtin_choose_expr() check when it
sees that any part of the 'const_exp' is a variable. And it short
circuits its warning checks. If so, then this could be a limitation in
GCC. Of course, this is just conjecture from me.

We could ignore the warning or do more complex type checking. Though a
simple fix is to reverse the conditional. Please see the patch below.

In any case, this was an interesting topic. :)

Thanks,
Yazen

>From 00e95116ef5a971e53e49b732839edd5d0887545 Mon Sep 17 00:00:00 2001
From: Yazen Ghannam <yazen.ghannam@....com>
Date: Mon, 3 Feb 2025 18:44:33 +0000
Subject: [PATCH] linux/bits.h: Squash unsigned comparison warning for GENMASK
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Invert conditionals to avoid the following 'type-limits' warning:
  warning: comparison of unsigned expression in ‘< 0’ is always false

The warning is enabled with W=2 and is present on GCC.

Signed-off-by: Yazen Ghannam <yazen.ghannam@....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 61a75d3f294b..45f70fb56ac3 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(!((l) < (h))))
 #else
 /*
  * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
-- 
2.43.0

Powered by blists - more mailing lists