Re: [PATCH] mm: Fix clang W=1 compiler warnings

Re: [PATCH] mm: Fix clang W=1 compiler warnings

[Jakub Kicinski]

On Fri, 7 Feb 2025 17:01:00 -0800 Linus Torvalds wrote:
> On Fri, 7 Feb 2025 at 16:49, Jakub Kicinski <kuba@kernel.org> wrote:
> > Could we possibly please still consider taking this in for 6.14? :(
> > Since the warning comes from vmstat.h pretty much every object file
> > generates this warning. clang 19 is getting more widely used now,
> > its making it hard to see new warnings.  
> 
> So:
> 
>  - I build the kernel with clang, but I don't have clang-19, so it's
> kind of pointless sending patches that DO NOT EVEN EXPLAIN WHAT THE
> WARNINGS ARE.
> 
>  - and even if you explain *WHAT* the warnings are, please also
> explain *WHY* they are valid and should be cared about.
> 
> Because honestly, W=1 is literally meant for "warnings that aren't
> necessarily valid". That's why they aren't on by default.
> 
> So no, I'm certainly not applying unexplained random patches that
> don't bother to explain the what or the why. Not for 6.14, not ever.

I thought I'd ask.. :) FWIW this for every single object:

  CC      net/core/request_sock.o
In file included from ../net/core/request_sock.c:14:
In file included from ../include/linux/tcp.h:17:
In file included from ../include/linux/skbuff.h:17:
In file included from ../include/linux/bvec.h:10:
In file included from ../include/linux/highmem.h:8:
In file included from ../include/linux/cacheflush.h:5:
In file included from ../arch/x86/include/asm/cacheflush.h:5:
In file included from ../include/linux/mm.h:2224:
../include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
  504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~ ^
  505 |                            item];
      |                            ~~~~
../include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
  511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~ ^
  512 |                            NR_VM_NUMA_EVENT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~~
../include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
  524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~ ^
  525 |                            NR_VM_NUMA_EVENT_ITEMS +
      |                            ~~~~~~~~~~~~~~~~~~~~~~
3 warnings generated.

> Fix the patch. Explain the problem. And possibly just disable the warning.

That'd be great. Nathan, would that be okay with you?

Re: [PATCH] mm: Fix clang W=1 compiler warnings

[Nathan Chancellor]

On Fri, Feb 07, 2025 at 05:38:13PM -0800, Jakub Kicinski wrote:
> On Fri, 7 Feb 2025 17:01:00 -0800 Linus Torvalds wrote:
> > On Fri, 7 Feb 2025 at 16:49, Jakub Kicinski <kuba@kernel.org> wrote:
> > > Could we possibly please still consider taking this in for 6.14? :(
> > > Since the warning comes from vmstat.h pretty much every object file
> > > generates this warning. clang 19 is getting more widely used now,
> > > its making it hard to see new warnings.  
> > 
> > So:
> > 
> >  - I build the kernel with clang, but I don't have clang-19, so it's
> > kind of pointless sending patches that DO NOT EVEN EXPLAIN WHAT THE
> > WARNINGS ARE.
> > 
> >  - and even if you explain *WHAT* the warnings are, please also
> > explain *WHY* they are valid and should be cared about.
> > 
> > Because honestly, W=1 is literally meant for "warnings that aren't
> > necessarily valid". That's why they aren't on by default.
> > 
> > So no, I'm certainly not applying unexplained random patches that
> > don't bother to explain the what or the why. Not for 6.14, not ever.
> 
> I thought I'd ask.. :) FWIW this for every single object:
> 
>   CC      net/core/request_sock.o
> In file included from ../net/core/request_sock.c:14:
> In file included from ../include/linux/tcp.h:17:
> In file included from ../include/linux/skbuff.h:17:
> In file included from ../include/linux/bvec.h:10:
> In file included from ../include/linux/highmem.h:8:
> In file included from ../include/linux/cacheflush.h:5:
> In file included from ../arch/x86/include/asm/cacheflush.h:5:
> In file included from ../include/linux/mm.h:2224:
> ../include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
>   504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
>       |                            ~~~~~~~~~~~~~~~~~~~~~ ^
>   505 |                            item];
>       |                            ~~~~
> ../include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
>   511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
>       |                            ~~~~~~~~~~~~~~~~~~~~~ ^
>   512 |                            NR_VM_NUMA_EVENT_ITEMS +
>       |                            ~~~~~~~~~~~~~~~~~~~~~~
> ../include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
>   524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
>       |                            ~~~~~~~~~~~~~~~~~~~~~ ^
>   525 |                            NR_VM_NUMA_EVENT_ITEMS +
>       |                            ~~~~~~~~~~~~~~~~~~~~~~
> 3 warnings generated.
> 
> > Fix the patch. Explain the problem. And possibly just disable the warning.
> 
> That'd be great. Nathan, would that be okay with you?

FWIW, I sent a patch to move this warning to W=2 in October and pinged
it in December:

https://lore.kernel.org/20241017-disable-two-clang-enum-warnings-v2-1-163ac04346ae@kernel.org/

'b4 shazam' tells me that it is still applicable on Linus's tree so
maybe he can just apply it directly?

Cheers,
Nathan

Re: [PATCH] mm: Fix clang W=1 compiler warnings

[Linus Torvalds]

On Fri, 7 Feb 2025 at 17:38, Jakub Kicinski <kuba@kernel.org> wrote:
>
> ../include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]

Ok.

So my gut feel is that this warning is simply bogus. We have
situations where we very intentionally use enumerations instead of a
list of #define constants.

That NR_VM_ZONE_STAT_ITEMS case really *does* look like it's a great
example of how code that doesn't care about the actual numbers, and
just wants the compiler to generate a sequence for us is supposed to
look.

Because sometimes we do enums just because it's a nice way to get
automatically incrementing values when we don't care exactly what the
values are.

And sometimes we do it for actual namespace reasons - see for example
commit 1a251f52cfdc ("minmax: make generic MIN() and MAX() macros
available everywhere") where in order to avoid clashes with the
"MIN()" macro, I made a couple of drivers that had a 'MIN' value (much
too generic a name, but it was what it was) use an enum instead.

See

   git show 1a251f52cfdc drivers/hwmon/adt7475.c

for details.

Now, at least in that case I don't think there was any arithmetic with
those values, so clang-19 wouldn't complain about it either, but I
mention that commit as a case of "it's actually very valid to use an
enum for actual real honest-to-goodness integer value enumeration".

Which really makes me feel like the new clang warning is seriously misguided.

In sparse I actually wanted to have integer types that don't silently
cast to other integer types. So sparse knows the concept of "bitwise"
and "nocast" attributes.  The "nocast" thing doesn't work well in
practice, but "bitwise" was a huge success and is how a lot of our
endianness problems were solved.

So the *concept* of an enum - or any other integer type - that "stays
in the type" is good, but I think it's a major mistake to think they
have to do so without any sane escape, and to only limit it to enums.

That said, we may not have *many* of those enum cases in the kernel
(and we do tend to have a history of using long series of '#define'
sequences to do these constants), so maybe the warning is acceptable
if it's a case of "this is literally the *only* one in the kernel".

But not having clang-19, I can't see if this is a case of "this is so
rare that let's just avoid it", or "this is the case that causes the
most noise for every file build, but there are lots of other cases of
this".

                  Linus

Re: [PATCH] mm: Fix clang W=1 compiler warnings

[Nathan Chancellor]

On Fri, Feb 07, 2025 at 07:11:08PM -0800, Linus Torvalds wrote:
> That said, we may not have *many* of those enum cases in the kernel
> (and we do tend to have a history of using long series of '#define'
> sequences to do these constants), so maybe the warning is acceptable
> if it's a case of "this is literally the *only* one in the kernel".
> 
> But not having clang-19, I can't see if this is a case of "this is so
> rare that let's just avoid it", or "this is the case that causes the
> most noise for every file build, but there are lots of other cases of
> this".

When this warning was turned on for C in clang-19 (it was C++ only prior
to that IIRC), it was extremely noisy. Some of that was due to the
warning occurring in headers that are included everywhere such as these
couple of ones but even hiding the big ones, there were still a bunch of
locations that triggered it (I did not do the best at hiding all of the
header ones because I had given up trying to make leaving the warning on
for the default build at that point).

The diff to hide some of the really common ones:

https://github.com/ClangBuiltLinux/linux/issues/2002#issuecomment-1970004069

The build log with that:

https://gist.github.com/nathanchance/971e5abeba504d3017cd6ed4517bbda6

I looked at a number of them and none of them really seemed like bugs to
me.

Cheers,
Nathan

Re: [PATCH] mm: Fix clang W=1 compiler warnings

[Linus Torvalds]

On Fri, 7 Feb 2025 at 19:33, Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Fri, Feb 07, 2025 at 07:11:08PM -0800, Linus Torvalds wrote:
> >
> > But not having clang-19, I can't see if this is a case of "this is so
> > rare that let's just avoid it", or "this is the case that causes the
> > most noise for every file build, but there are lots of other cases of
> > this".
>
> The diff to hide some of the really common ones:
>
> https://github.com/ClangBuiltLinux/linux/issues/2002#issuecomment-1970004069

Oh, ok, that's already more than I would have liked to see, and I do
think that the patch is not only bigger than I'd like, it makes the
code actively worse with adding random casts just to shut the compiler
up.

That is, of course, something that C++ people think is a good thing,
since they are used to their traditional NULL casting idiocy. But in
C, we have actual taste and grace.

So no, that warning is no good for the kernel.

> The build log with that:
>
> https://gist.github.com/nathanchance/971e5abeba504d3017cd6ed4517bbda6
>
> I looked at a number of them and none of them really seemed like bugs to
> me.

Yeah, those seem like more of the same: the kernel doing sane things,
using enum's the way they are meant to be used, and the compiler
warning is just simply plain wrong.

I will take your patch to move it to W=2.

Thanks,
                   Linus

Re: [PATCH] mm: Fix clang W=1 compiler warnings

[Linus Torvalds]

On Fri, 7 Feb 2025 at 19:49, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I will take your patch to move it to W=2.

Ok, applied and pushed out as commit 8f6629c004b1 ("kbuild: Move
-Wenum-enum-conversion to W=2").

Jakub, hopefully this fixes your situation,

                Linus

Re: [PATCH] mm: Fix clang W=1 compiler warnings

[Jakub Kicinski]

On Fri, 7 Feb 2025 20:24:18 -0800 Linus Torvalds wrote:
> > I will take your patch to move it to W=2.  
> 
> Ok, applied and pushed out as commit 8f6629c004b1 ("kbuild: Move
> -Wenum-enum-conversion to W=2").
> 
> Jakub, hopefully this fixes your situation,

Much appreciated, thank you!