[PATCH] mm/vmstat: Fix -Wenum-enum-conversion warning in vmstat.h

[PATCH] mm/vmstat: Fix -Wenum-enum-conversion warning in vmstat.h

[Shivamurthy Shastri]

A W=1 build with -Wenum-enum-conversion enabled, results in the
following build warning due to an arithmetic operation between different
enumeration types 'enum node_stat_item' and 'enum lru_list':

  include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
    514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
        |                               ~~~~~~~~~~~ ^ ~~~

Address this by casting lru to the proper type.

Signed-off-by: Shivamurthy Shastri <shivamurthy.shastri@linutronix.de>
Reviewed-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
---
 include/linux/vmstat.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
index 735eae6e272c..72ecd46fd0c4 100644
--- a/include/linux/vmstat.h
+++ b/include/linux/vmstat.h
@@ -511,7 +511,7 @@ static inline const char *node_stat_name(enum node_stat_item item)
 
 static inline const char *lru_list_name(enum lru_list lru)
 {
-	return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
+	return node_stat_name(NR_LRU_BASE + (enum node_stat_item)lru) + 3; // skip "nr_"
 }
 
 static inline const char *writeback_stat_name(enum writeback_stat_item item)
-- 
2.34.1

Re: [PATCH] mm/vmstat: Fix -Wenum-enum-conversion warning in vmstat.h

[Matthew Wilcox]

On Fri, Jun 21, 2024 at 01:16:04PM +0200, Shivamurthy Shastri wrote:
> A W=1 build with -Wenum-enum-conversion enabled, results in the
> following build warning due to an arithmetic operation between different
> enumeration types 'enum node_stat_item' and 'enum lru_list':

OK, but why do we want -Wenum-enum-conversion enabled?  The code looks
perfectly fine before, and now it looks ugly.  What bugs does this
warning catch?

>  static inline const char *lru_list_name(enum lru_list lru)
>  {
> -	return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
> +	return node_stat_name(NR_LRU_BASE + (enum node_stat_item)lru) + 3; // skip "nr_"
>  }

and honestly, I'd convert it to an int instead of enum node_stat_item.
Because it is not a node_stat_item, and it wouldn't make sense to
add two node_stat_items together.  Just like it doesn't make sense to
add two pointers together (but it does make sense to add an integer to a
pointer).

Re: [PATCH] mm/vmstat: Fix -Wenum-enum-conversion warning in vmstat.h

[Andrew Morton]

On Fri, 21 Jun 2024 19:13:57 +0100 Matthew Wilcox <willy@infradead.org> wrote:

> On Fri, Jun 21, 2024 at 01:16:04PM +0200, Shivamurthy Shastri wrote:
> > A W=1 build with -Wenum-enum-conversion enabled, results in the
> > following build warning due to an arithmetic operation between different
> > enumeration types 'enum node_stat_item' and 'enum lru_list':
> 
> OK, but why do we want -Wenum-enum-conversion enabled?  The code looks
> perfectly fine before, and now it looks ugly.  What bugs does this
> warning catch?
> 
> >  static inline const char *lru_list_name(enum lru_list lru)
> >  {
> > -	return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
> > +	return node_stat_name(NR_LRU_BASE + (enum node_stat_item)lru) + 3; // skip "nr_"
> >  }
> 
> and honestly, I'd convert it to an int instead of enum node_stat_item.
> Because it is not a node_stat_item, and it wouldn't make sense to
> add two node_stat_items together.  Just like it doesn't make sense to
> add two pointers together (but it does make sense to add an integer to a
> pointer).

Yeah, I suppose so.  The calling code iterates across enums with an
int, imaginatively called `i'.

Then again, it seems right that a function called lru_list_name() takes
an enum lru_list.

Re: [PATCH] mm/vmstat: Fix -Wenum-enum-conversion warning in vmstat.h

[Aleksei Vetrov]

Hi Shivamurthy,

On Fri, Jun 21, 2024 at 01:16:04PM +0200, Shivamurthy Shastri wrote:
> A W=1 build with -Wenum-enum-conversion enabled, results in the
> following build warning due to an arithmetic operation between different
> enumeration types 'enum node_stat_item' and 'enum lru_list':
> 
>   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
>     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
>         |                               ~~~~~~~~~~~ ^ ~~~
> 
> Address this by casting lru to the proper type.
> 
> Signed-off-by: Shivamurthy Shastri <shivamurthy.shastri@linutronix.de>
> Reviewed-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
> ---
>  include/linux/vmstat.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
> index 735eae6e272c..72ecd46fd0c4 100644
> --- a/include/linux/vmstat.h
> +++ b/include/linux/vmstat.h
> @@ -511,7 +511,7 @@ static inline const char *node_stat_name(enum node_stat_item item)
>  
>  static inline const char *lru_list_name(enum lru_list lru)
>  {
> -	return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
> +	return node_stat_name(NR_LRU_BASE + (enum node_stat_item)lru) + 3; // skip "nr_"
>  }
>  
>  static inline const char *writeback_stat_name(enum writeback_stat_item item)
> -- 
> 2.34.1
> 

We have encountered the same problem after trying to update Clang to the
latest version and this is a blocker because we use W=1 to compile the
kernel.

Do you plan to address review comments about casting to int instead of
enum node_stat_item? Or I can submit another patch myself that addresses
it.

Re: [PATCH] mm/vmstat: Fix -Wenum-enum-conversion warning in vmstat.h

[Nathan Chancellor]

Hi Aleksei,

On Mon, Oct 07, 2024 at 08:13:41PM +0000, Aleksei Vetrov wrote:
> Hi Shivamurthy,
> 
> On Fri, Jun 21, 2024 at 01:16:04PM +0200, Shivamurthy Shastri wrote:
> > A W=1 build with -Wenum-enum-conversion enabled, results in the
> > following build warning due to an arithmetic operation between different
> > enumeration types 'enum node_stat_item' and 'enum lru_list':
> > 
> >   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
> >     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
> >         |                               ~~~~~~~~~~~ ^ ~~~
> > 
> > Address this by casting lru to the proper type.
> > 
> > Signed-off-by: Shivamurthy Shastri <shivamurthy.shastri@linutronix.de>
> > Reviewed-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
> > ---
> >  include/linux/vmstat.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
> > index 735eae6e272c..72ecd46fd0c4 100644
> > --- a/include/linux/vmstat.h
> > +++ b/include/linux/vmstat.h
> > @@ -511,7 +511,7 @@ static inline const char *node_stat_name(enum node_stat_item item)
> >  
> >  static inline const char *lru_list_name(enum lru_list lru)
> >  {
> > -	return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
> > +	return node_stat_name(NR_LRU_BASE + (enum node_stat_item)lru) + 3; // skip "nr_"
> >  }
> >  
> >  static inline const char *writeback_stat_name(enum writeback_stat_item item)
> > -- 
> > 2.34.1
> > 
> 
> We have encountered the same problem after trying to update Clang to the
> latest version and this is a blocker because we use W=1 to compile the
> kernel.
> 
> Do you plan to address review comments about casting to int instead of
> enum node_stat_item? Or I can submit another patch myself that addresses
> it.

For what it's worth, I never really saw Matthew's comment around what
value does this warning provide addressed. I was the one who originally
moved it into W=1 at the request of Arnd because he felt that instances
of this warning could be bugs and they should be audited.  However, I
have not seen many instances of this warning pop up in new code through
0day build reports and the ones that I have seen seem to be intentional,
as they are using enums like integral values, such as here. If that is
just going to result in a bunch of patches like this adding unnecessary
casts, I think it would just be better to consider disabling this
warning altogether or at the very least, moving it to W=2 (which is for
warnings that are noisy but might contain bugs), since more people are
using W=1 as their normal build configuration nowadays.

Cheers,
Nathan

Re: [PATCH] mm/vmstat: Fix -Wenum-enum-conversion warning in vmstat.h

[Aleksei Vetrov]

Hi Nathan,

On Mon, Oct 07, 2024 at 05:51:36PM -0700, Nathan Chancellor wrote:
> For what it's worth, I never really saw Matthew's comment around what
> value does this warning provide addressed. I was the one who originally
> moved it into W=1 at the request of Arnd because he felt that instances
> of this warning could be bugs and they should be audited.  However, I
> have not seen many instances of this warning pop up in new code through
> 0day build reports and the ones that I have seen seem to be intentional,
> as they are using enums like integral values, such as here. If that is
> just going to result in a bunch of patches like this adding unnecessary
> casts, I think it would just be better to consider disabling this
> warning altogether or at the very least, moving it to W=2 (which is for
> warnings that are noisy but might contain bugs), since more people are
> using W=1 as their normal build configuration nowadays.

If time has proven that this warning has never found an unintended enum
conversion, then it is worth to disable it for everyone. As you said in
the original thread ([PATCH] kbuild: Disable two Clang specific
enumeration warnings), W=2 is not run by any CI, so I would prefer to
disable it completely.

Alternatives considered:

* Enable -Wenum-enum-conversion only for 0day build reports through
  KCFLAGS. It will eliminate noise for regular users while keeping
  developers informed about new instances of this warning.
* -Wno-error=enum-enum-conversion to keep warning but don't block
  compilation for CONFIG_WERROR users.

Arnd Bergmann, what do you think? Have you found it useful after all?

Re: [PATCH] mm/vmstat: Fix -Wenum-enum-conversion warning in vmstat.h

[Arnd Bergmann]

On Thu, Oct 10, 2024, at 10:40, Aleksei Vetrov wrote:
> On Mon, Oct 07, 2024 at 05:51:36PM -0700, Nathan Chancellor wrote:
>> For what it's worth, I never really saw Matthew's comment around what
>> value does this warning provide addressed. I was the one who originally
>> moved it into W=1 at the request of Arnd because he felt that instances
>> of this warning could be bugs and they should be audited.  However, I
>> have not seen many instances of this warning pop up in new code through
>> 0day build reports and the ones that I have seen seem to be intentional,
>> as they are using enums like integral values, such as here. If that is
>> just going to result in a bunch of patches like this adding unnecessary
>> casts, I think it would just be better to consider disabling this
>> warning altogether or at the very least, moving it to W=2 (which is for
>> warnings that are noisy but might contain bugs), since more people are
>> using W=1 as their normal build configuration nowadays.
>
> If time has proven that this warning has never found an unintended enum
> conversion, then it is worth to disable it for everyone. As you said in
> the original thread ([PATCH] kbuild: Disable two Clang specific
> enumeration warnings), W=2 is not run by any CI, so I would prefer to
> disable it completely.
>
> Alternatives considered:
>
> * Enable -Wenum-enum-conversion only for 0day build reports through
>   KCFLAGS. It will eliminate noise for regular users while keeping
>   developers informed about new instances of this warning.
> * -Wno-error=enum-enum-conversion to keep warning but don't block
>   compilation for CONFIG_WERROR users.
>
> Arnd Bergmann, what do you think? Have you found it useful after all?

I'm fairly sure I saw users mix up 'dma_data_direction' with
'dma_transfer_direction' and unrelated enum-enum mixups in
amdgpu. There were probably more.

I think what happened is that in clang-18 and earlier, the
warning option caught mistakes of passing the wrong enum 
to a function and a few others, but it did not catch arithmetic
operations between enums, so clang-19 now produces a lot more
output than older versions, and I don't think we can
control those independently.

       Arnd

Re: [PATCH] mm/vmstat: Fix -Wenum-enum-conversion warning in vmstat.h

[Nathan Chancellor]

On Fri, Oct 11, 2024 at 02:05:00PM +0000, Arnd Bergmann wrote:
> I'm fairly sure I saw users mix up 'dma_data_direction' with
> 'dma_transfer_direction' and unrelated enum-enum mixups in
> amdgpu. There were probably more.

Yes, those have definitely happened but those are -Wenum-conversion, not
-Wenum-enum-conversion.

> I think what happened is that in clang-18 and earlier, the
> warning option caught mistakes of passing the wrong enum 
> to a function and a few others, but it did not catch arithmetic
> operations between enums, so clang-19 now produces a lot more
> output than older versions, and I don't think we can
> control those independently.

A contrived example: https://godbolt.org/z/Ydx6rxsvb

We should be able to disable -Wenum-enum-conversion without impacting
the ability to catch the cases that you mentioned above. It also helps
that GCC supports -Wenum-conversion, but it does not seem like they have
an equivalent for -Wenum-enum-conversion.

Cheers,
Nathan

Re: [PATCH] mm/vmstat: Fix -Wenum-enum-conversion warning in vmstat.h

[Aleksei Vetrov]

On Tue, Oct 15, 2024 at 01:47:14AM -0700, Nathan Chancellor wrote:
> We should be able to disable -Wenum-enum-conversion without impacting
> the ability to catch the cases that you mentioned above. It also helps
> that GCC supports -Wenum-conversion, but it does not seem like they have
> an equivalent for -Wenum-enum-conversion.

Could you please create and send a patch to disable
-Wenum-enum-conversion?

Re: [PATCH] mm/vmstat: Fix -Wenum-enum-conversion warning in vmstat.h

[Nathan Chancellor]

On Tue, Oct 15, 2024 at 04:55:23PM +0000, Aleksei Vetrov wrote:
> On Tue, Oct 15, 2024 at 01:47:14AM -0700, Nathan Chancellor wrote:
> > We should be able to disable -Wenum-enum-conversion without impacting
> > the ability to catch the cases that you mentioned above. It also helps
> > that GCC supports -Wenum-conversion, but it does not seem like they have
> > an equivalent for -Wenum-enum-conversion.
> 
> Could you please create and send a patch to disable
> -Wenum-enum-conversion?

Sure, I have sent
https://lore.kernel.org/20241016-disable-two-clang-enum-warnings-v1-1-ae886d7a0269@kernel.org/
now, we can continue discussion over there.

Cheers,
Nathan