Message-ID: <Z6T3Dv09chNN3lh5@thinkpad>
Date: Thu, 6 Feb 2025 12:53:18 -0500
From: Yury Norov <yury.norov@...il.com>
To: Yazen Ghannam <yazen.ghannam@....com>
Cc: Rasmus Villemoes <linux@...musvillemoes.dk>,
	Kees Cook <kees@...nel.org>,
	Maxim Kuvyrkov <maxim.kuvyrkov@...aro.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] linux/bits.h: Squash unsigned comparison warning for
 GENMASK

Add a couple of compiler experts.

On Wed, Feb 05, 2025 at 09:02:27PM -0500, Yazen Ghannam wrote:
> 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;

Uninitialized?

> 	
> 	        printf("%lx\n", GENMASK(TEST,0));
> 	        return 0;
> 	}

I also tried some combinations, and it makes no sense to me.
'const unsigned int' should definitely help to evaluate this
expression at compile time:
 
        const unsigned int h = 10;
        printf("%lx\n", GENMASK(h, 0));

But gcc -Wall -Wextra -Wtype-limits tst.c doesn't think so.

I can understand why Wtype-limits is happy with 'int h = 10, l=0'.
I can't understand why it's happy with the 'unsigned int h = 10, l=0',
and at the same time is not happy with the above.

> 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.
 
You can try something more complex, just keep in mind that from kernel
perspective, we'd like to be able to call GENMASK() with non-constant
high and/or low limits, as soon as the 'l > h' combination eventually
evaluates to a const expression. For example, in linux/find.h
we call GENMASK() like this:

        if (small_const_nbits(size)) {
                unsigned long val;

                if (unlikely(offset >= size))
                        return size;

                val = *addr1 & *addr2 & GENMASK(size - 1, offset);
                return val ? __ffs(val) : size;
        }

In lib/test_bitmap.h we've got this:

        for (nbits = 0; nbits < EXP1_IN_BITS; ++nbits) {
                ...
                GENMASK_ULL((nbits - 1) % 64, 0));
        }

I would like to keep both examples working.

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

(l > h) negation would be: !(l <= h). And if you do this properly, the
warning will stay there.

Maybe true compiler gurus will help...

Thanks,
Yury

>  #else
>  /*
>   * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> -- 
> 2.43.0

Powered by blists - more mailing lists