| Message ID | 20240621111604.25330-1-shivamurthy.shastri@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/vmstat: Fix -Wenum-enum-conversion warning in vmstat.h | expand |
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).
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.
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.
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
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?
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
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
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?
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
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)
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(-)