[PATCH] kbuild: Disable two Clang specific enumeration warnings

[PATCH] kbuild: Disable two Clang specific enumeration warnings

[Nathan Chancellor]

Clang enables -Wenum-enum-conversion and -Wenum-compare-conditional
under -Wenum-conversion. A recent change in Clang strengthened these
warnings and they appear frequently in common builds, primarily due to
several instances in common headers but there are quite a few drivers
that have individual instances as well.

  include/linux/vmstat.h:508:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
    508 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
        |                            ~~~~~~~~~~~~~~~~~~~~~ ^
    509 |                            item];
        |                            ~~~~

  drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c:955:24: warning: conditional expression between different enumeration types ('enum iwl_mac_beacon_flags' and 'enum iwl_mac_beacon_flags_v1') [-Wenum-compare-conditional]
    955 |                 flags |= is_new_rate ? IWL_MAC_BEACON_CCK
        |                                      ^ ~~~~~~~~~~~~~~~~~~
    956 |                           : IWL_MAC_BEACON_CCK_V1;
        |                             ~~~~~~~~~~~~~~~~~~~~~
  drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c:1120:21: warning: conditional expression between different enumeration types ('enum iwl_mac_beacon_flags' and 'enum iwl_mac_beacon_flags_v1') [-Wenum-compare-conditional]
   1120 |                                                0) > 10 ?
        |                                                        ^
   1121 |                         IWL_MAC_BEACON_FILS :
        |                         ~~~~~~~~~~~~~~~~~~~
   1122 |                         IWL_MAC_BEACON_FILS_V1;
        |                         ~~~~~~~~~~~~~~~~~~~~~~

While doing arithmetic with different types of enums may be potentially
problematic, inspecting several instances of the warning does not reveal
any obvious problems. To silence the warnings at the source level, an
integral cast must be added to each mismatched enum (which is incredibly
ugly when done frequently) or the value must moved out of the enum to a
macro, which can remove the type safety offered by enums in other
places, such as assignments that would trigger -Wenum-conversion.

As the warnings do not appear to have a high signal to noise ratio and
the source level silencing options are not sustainable, disable the
warnings unconditionally, as they will be enabled with -Wenum-conversion
and are supported in all versions of clang that can build the kernel.

Cc: stable@vger.kernel.org
Closes: https://github.com/ClangBuiltLinux/linux/issues/2002
Link: https://github.com/llvm/llvm-project/commit/8c2ae42b3e1c6aa7c18f873edcebff7c0b45a37e
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 scripts/Makefile.extrawarn | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index a9e552a1e910..6053aa22b8f5 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -81,6 +81,14 @@ KBUILD_CFLAGS += $(call cc-option,-Werror=designated-init)
 
 # Warn if there is an enum types mismatch
 KBUILD_CFLAGS += $(call cc-option,-Wenum-conversion)
+ifdef CONFIG_CC_IS_CLANG
+# Clang enables these extra warnings under -Wenum-conversion but the kernel
+# performs arithmetic using or has conditionals returning enums of different
+# types in several different places, which is rarely a bug in the kernel's
+# case, so disable the warnings.
+KBUILD_CFLAGS += -Wno-enum-compare-conditional
+KBUILD_CFLAGS += -Wno-enum-enum-conversion
+endif
 
 #
 # W=1 - warnings which may be relevant and do not occur too often

---
base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
change-id: 20240304-disable-extra-clang-enum-warnings-bf574c7c99fd

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>

Re: [PATCH] kbuild: Disable two Clang specific enumeration warnings

[Yonghong Song]

On 3/5/24 9:42 AM, Nathan Chancellor wrote:
> Clang enables -Wenum-enum-conversion and -Wenum-compare-conditional
> under -Wenum-conversion. A recent change in Clang strengthened these
> warnings and they appear frequently in common builds, primarily due to
> several instances in common headers but there are quite a few drivers
> that have individual instances as well.
>
>    include/linux/vmstat.h:508:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
>      508 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
>          |                            ~~~~~~~~~~~~~~~~~~~~~ ^
>      509 |                            item];
>          |                            ~~~~
>
>    drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c:955:24: warning: conditional expression between different enumeration types ('enum iwl_mac_beacon_flags' and 'enum iwl_mac_beacon_flags_v1') [-Wenum-compare-conditional]
>      955 |                 flags |= is_new_rate ? IWL_MAC_BEACON_CCK
>          |                                      ^ ~~~~~~~~~~~~~~~~~~
>      956 |                           : IWL_MAC_BEACON_CCK_V1;
>          |                             ~~~~~~~~~~~~~~~~~~~~~
>    drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c:1120:21: warning: conditional expression between different enumeration types ('enum iwl_mac_beacon_flags' and 'enum iwl_mac_beacon_flags_v1') [-Wenum-compare-conditional]
>     1120 |                                                0) > 10 ?
>          |                                                        ^
>     1121 |                         IWL_MAC_BEACON_FILS :
>          |                         ~~~~~~~~~~~~~~~~~~~
>     1122 |                         IWL_MAC_BEACON_FILS_V1;
>          |                         ~~~~~~~~~~~~~~~~~~~~~~
>
> While doing arithmetic with different types of enums may be potentially
> problematic, inspecting several instances of the warning does not reveal
> any obvious problems. To silence the warnings at the source level, an
> integral cast must be added to each mismatched enum (which is incredibly
> ugly when done frequently) or the value must moved out of the enum to a
> macro, which can remove the type safety offered by enums in other
> places, such as assignments that would trigger -Wenum-conversion.
>
> As the warnings do not appear to have a high signal to noise ratio and
> the source level silencing options are not sustainable, disable the
> warnings unconditionally, as they will be enabled with -Wenum-conversion
> and are supported in all versions of clang that can build the kernel.
>
> Cc: stable@vger.kernel.org
> Closes: https://github.com/ClangBuiltLinux/linux/issues/2002
> Link: https://github.com/llvm/llvm-project/commit/8c2ae42b3e1c6aa7c18f873edcebff7c0b45a37e
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

Thanks for the fix. LGTM.

Acked-by: Yonghong Song <yonghong.song@linux.dev>

Re: [PATCH] kbuild: Disable two Clang specific enumeration warnings

[Arnd Bergmann]

On Tue, Mar 5, 2024, at 18:42, Nathan Chancellor wrote:
>
> As the warnings do not appear to have a high signal to noise ratio and
> the source level silencing options are not sustainable, disable the
> warnings unconditionally, as they will be enabled with -Wenum-conversion
> and are supported in all versions of clang that can build the kernel.

I took a look at a sample of warnings in an allmodconfig build
and found a number that need attention. I would much prefer to 
leave these turned on at the W=1 level and only disable them
at the default warning level.

      Arnd

Re: [PATCH] kbuild: Disable two Clang specific enumeration warnings

[Nick Desaulniers]

On Tue, Mar 5, 2024 at 10:50 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Mar 5, 2024, at 18:42, Nathan Chancellor wrote:
> >
> > As the warnings do not appear to have a high signal to noise ratio and
> > the source level silencing options are not sustainable, disable the
> > warnings unconditionally, as they will be enabled with -Wenum-conversion
> > and are supported in all versions of clang that can build the kernel.
>
> I took a look at a sample of warnings in an allmodconfig build
> and found a number that need attention. I would much prefer to
> leave these turned on at the W=1 level and only disable them
> at the default warning level.

Sounds like these new diagnostics are very noisy. 0day bot sends
people reports at W=1. Perhaps W=2?

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] kbuild: Disable two Clang specific enumeration warnings

[Nathan Chancellor]

On Tue, Mar 05, 2024 at 10:52:16AM -0800, Nick Desaulniers wrote:
> On Tue, Mar 5, 2024 at 10:50 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tue, Mar 5, 2024, at 18:42, Nathan Chancellor wrote:
> > >
> > > As the warnings do not appear to have a high signal to noise ratio and
> > > the source level silencing options are not sustainable, disable the
> > > warnings unconditionally, as they will be enabled with -Wenum-conversion
> > > and are supported in all versions of clang that can build the kernel.
> >
> > I took a look at a sample of warnings in an allmodconfig build
> > and found a number that need attention. I would much prefer to
> > leave these turned on at the W=1 level and only disable them
> > at the default warning level.
> 
> Sounds like these new diagnostics are very noisy. 0day bot sends
> people reports at W=1. Perhaps W=2?

A number of subsystems test with W=1 as well and while opting into W=1
means that you are potentially asking for new warnings across newer
compiler releases, a warning with this number of instances is going to
cause a lot of issues (I think of netdev for example).

I think if we are going to leave it enabled at W=2, we might as well
just take this change as is then have people who are developing the
fixes use 'KCFLAGS=-Wenum-conversion' when building to override it,
which is more targeted than using W=2. W=2 is not run by any CI as far
as I am aware, so there is not really any difference between disabled
altogether vs.  enabled at W=2 in terms of widespread testing. Once all
the fixes (or patches to hide instances) are picked up and merged into
Linus's tree, this change can just be reverted.

Fundamentally, I do not really care which avenue we take (either this
change or off by default, on at W=1), I am happy to do whatever.
Unfortunately, CONFIG_WERROR makes these decisions much more urgent
because it is either disable it and have other warnings creep in amongst
the sprawl of these warnings or leave it on and miss other errors for
the same reason.

Cheers,
Nathan

Re: [PATCH] kbuild: Disable two Clang specific enumeration warnings

[Arnd Bergmann]

On Tue, Mar 5, 2024, at 20:30, Nathan Chancellor wrote:
> On Tue, Mar 05, 2024 at 10:52:16AM -0800, Nick Desaulniers wrote:
>> On Tue, Mar 5, 2024 at 10:50 AM Arnd Bergmann <arnd@arndb.de> wrote:
>> >
>> > On Tue, Mar 5, 2024, at 18:42, Nathan Chancellor wrote:
>> > >
>> > > As the warnings do not appear to have a high signal to noise ratio and
>> > > the source level silencing options are not sustainable, disable the
>> > > warnings unconditionally, as they will be enabled with -Wenum-conversion
>> > > and are supported in all versions of clang that can build the kernel.
>> >
>> > I took a look at a sample of warnings in an allmodconfig build
>> > and found a number that need attention. I would much prefer to
>> > leave these turned on at the W=1 level and only disable them
>> > at the default warning level.
>> 
>> Sounds like these new diagnostics are very noisy. 0day bot sends
>> people reports at W=1. Perhaps W=2?

It feels like this is not a great reason for moving it to W=2
instead of W=1, but W=2 is still better than always disabling
it I think.

Specifically, the 0day bot warns for newly added W=1 warnings
but not for preexisting ones, and I think there are other warnings
at the W=1 level that are similarly noisy to this one.

> A number of subsystems test with W=1 as well and while opting into W=1
> means that you are potentially asking for new warnings across newer
> compiler releases, a warning with this number of instances is going to
> cause a lot of issues (I think of netdev for example).

I only see a handful of warnings in net (devlink, bpf) and
drivers/net (ethernet/{3com,amd8111e,funeth,hns,idpf,jme,mlx4} and
wireless/{iwlwifi,mt76,rtw88,rtw89}). 

These are also some of the ones that I think need a closer look.

> Fundamentally, I do not really care which avenue we take (either this
> change or off by default, on at W=1), I am happy to do whatever.
> Unfortunately, CONFIG_WERROR makes these decisions much more urgent
> because it is either disable it and have other warnings creep in amongst
> the sprawl of these warnings or leave it on and miss other errors for
> the same reason.

Agreed.

       Arnd

Re: [PATCH] kbuild: Disable two Clang specific enumeration warnings

[Nathan Chancellor]

On Tue, Mar 05, 2024 at 10:52:54PM +0100, Arnd Bergmann wrote:
> On Tue, Mar 5, 2024, at 20:30, Nathan Chancellor wrote:
> > On Tue, Mar 05, 2024 at 10:52:16AM -0800, Nick Desaulniers wrote:
> >> On Tue, Mar 5, 2024 at 10:50 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >> >
> >> > On Tue, Mar 5, 2024, at 18:42, Nathan Chancellor wrote:
> >> > >
> >> > > As the warnings do not appear to have a high signal to noise ratio and
> >> > > the source level silencing options are not sustainable, disable the
> >> > > warnings unconditionally, as they will be enabled with -Wenum-conversion
> >> > > and are supported in all versions of clang that can build the kernel.
> >> >
> >> > I took a look at a sample of warnings in an allmodconfig build
> >> > and found a number that need attention. I would much prefer to
> >> > leave these turned on at the W=1 level and only disable them
> >> > at the default warning level.
> >> 
> >> Sounds like these new diagnostics are very noisy. 0day bot sends
> >> people reports at W=1. Perhaps W=2?
> 
> It feels like this is not a great reason for moving it to W=2
> instead of W=1, but W=2 is still better than always disabling
> it I think.
> 
> Specifically, the 0day bot warns for newly added W=1 warnings
> but not for preexisting ones, and I think there are other warnings
> at the W=1 level that are similarly noisy to this one.
> 
> > A number of subsystems test with W=1 as well and while opting into W=1
> > means that you are potentially asking for new warnings across newer
> > compiler releases, a warning with this number of instances is going to
> > cause a lot of issues (I think of netdev for example).
> 
> I only see a handful of warnings in net (devlink, bpf) and
> drivers/net (ethernet/{3com,amd8111e,funeth,hns,idpf,jme,mlx4} and
> wireless/{iwlwifi,mt76,rtw88,rtw89}). 
> 
> These are also some of the ones that I think need a closer look.

Fair enough, I have sent v2 that just moves these warnings to W=1.

Cheers,
Nathan