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

[Vincent Mailhol <mailhol.vincent@wanadoo.fr>]

Because this is more a discussion on build flags than on GENMASK(), I am
adding the maintainers of scripts/Makefile.extrawarn in CC:

+CC: Masahiro Yamada <masahiroy@kernel.org>
+CC: Nathan Chancellor <nathan@kernel.org>
+CC: Nicolas Schier <nicolas@fjasle.eu>
+CC: linux-kbuild@vger.kernel.org

On 07/02/2025 at 02:53, Yury Norov wrote:
> 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@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. 

I complained, but I was NAKed by Linus :)

  Link:
https://lore.kernel.org/all/CAHk-=whvGWbpsTa538CvQ9e=VF+m8WPQmES2y6-=0=-64uGkgg@mail.gmail.com/

Because this is an old discussion, let me summarize it here. My past
complain is that this -Wtype-limits warning is on an header file, as
such it pollutes the error log of other files.

When I compile my file, I want to see my warnings, not other people's one.

And we are not speaking of a few lines of spam. This
GENMASK_INPUT_CHECK() is responsible of the vast majority of all W=12
warning. For example, on a defconfig (minus CONFIG_WERROR):

  $ make W=12 -j8 2>&1 >/dev/null | \
      sed -n "s/\(^.*:\).*\(\[-W.*\]\)/\1 \2/p" | \
      sort | uniq -c | sort -rn | head -n5
    50276 ./include/linux/bits.h:24:68: warning: [-Wtype-limits]
    12194 ./include/linux/minmax.h:68:57: warning: [-Wtype-limits]
      564 ./include/linux/container_of.h:19:15: warning: [-Wshadow]
      327 ./include/linux/overflow.h:215:13: warning: [-Wtype-limits]
      327 ./include/linux/overflow.h:214:21: warning: [-Wtype-limits]

Incontestable victory of GENMASK_INPUT_CHECK() as the biggest W=2 spam
(if I do the full stats, this single line in bits.h represent 75% of all
warnings on the defconfig build).

My honest opinion is that the top two warnings of this list should be fixed.

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

The warning is silenced on integer constant expression. Here, unsigned
int h is not an integer constant expression (yes, this is confusing, but
in C the const qualifier does not declare constant expressions, you need
the C23 constexpr qualifier for that). So you are getting the warning.

It is happy with int h = 10, l = 0 because here, it is not a tautology.

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

The easiest fix is, I think:

  #define GENMASK_INPUT_CHECK(h, l) \
  	BUILD_BUG_ON_ZERO(const_true((l) > (int)(h)))

The warning only occurs when l is zero, only h need to be casted to a
signed integer. And the lost of precision is not an issue either, out of
bound numbers are already caught by -Wshift-count-overflow.

> Maybe true compiler gurus will help...

I am not the compiler guru, but I know a bit of this subject to shed
some lights.

So, that warning is implemented similarly in clang and gcc: it warn for
tautologies in comparisons only if the operand being compared is not an
integer constant expression. So:

  0 > 42U		/* OK: tautology but constant expression */
  0 > signed_var	/* OK: not a tautology */
  0 > unsigned_var	/* Warning: tautology and
			 * non constant expression */

Where gcc and clang differs is on the implementation of
__builtin_choose_expr(). clang will only emit a warning if it is on the
chosen expression, gcc will warn even if the expression is discarded. So:

  __builtin_choose_expr(__builtin_constant_p(a), 0 > unsigned_var, 0);

will only yield a warning on gcc.

This is a godbolt link with above examples:

  https://godbolt.org/z/nzxaP1oqe

Note that this is consistent with gcc documentation, I quote:

  Furthermore, the unused expression (exp1 or exp2 depending on the
  value of const_exp) may still generate syntax errors.


https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005fchoose_005fexpr

In summary, in , the warning only occurs when the the results is discarded.

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

-- 
Yours sincerely,
Vincent Mailhol