Re: [PATCH] arm64/cache: silence -Woverride-init warnings

[Mark Rutland]

On Thu, Aug 08, 2019 at 06:18:39PM -0400, Qian Cai wrote:
> > On Aug 8, 2019, at 6:38 AM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> > 
> > On Wed, Aug 07, 2019 at 11:29:16PM -0400, Qian Cai wrote:
> >> The commit 155433cb365e ("arm64: cache: Remove support for ASID-tagged
> >> VIVT I-caches") introduced some compiation warnings from GCC (and
> >> Clang) with -Winitializer-overrides),
> >> 
> >> arch/arm64/kernel/cpuinfo.c:38:26: warning: initialized field
> >> overwritten [-Woverride-init]
> >> [ICACHE_POLICY_VIPT]  = "VIPT",
> >>                        ^~~~~~
> >> arch/arm64/kernel/cpuinfo.c:38:26: note: (near initialization for
> >> 'icache_policy_str[2]')
> >> arch/arm64/kernel/cpuinfo.c:39:26: warning: initialized field
> >> overwritten [-Woverride-init]
> >> [ICACHE_POLICY_PIPT]  = "PIPT",
> >>                        ^~~~~~
> >> arch/arm64/kernel/cpuinfo.c:39:26: note: (near initialization for
> >> 'icache_policy_str[3]')
> >> arch/arm64/kernel/cpuinfo.c:40:27: warning: initialized field
> >> overwritten [-Woverride-init]
> >> [ICACHE_POLICY_VPIPT]  = "VPIPT",
> >>                         ^~~~~~~
> >> arch/arm64/kernel/cpuinfo.c:40:27: note: (near initialization for
> >> 'icache_policy_str[0]')
> >> 
> >> because it initializes icache_policy_str[0 ... 3] twice. Since
> >> arm64 developers are keen to keep the style of initializing a static
> >> array with a non-zero pattern first, just disable those warnings for
> >> both GCC and Clang of this file.
> >> 
> >> Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches")
> >> Signed-off-by: Qian Cai <cai@xxxxxx>
> > 
> > This is _not_ a fix, and should not require backporting to stable trees.
> 
> From my experience, the stable AI will pick up whatever they want to backport
> not matter if there Is a âFixesâ tag or not unless it is one of those subsystems like
> Networking that exclusively manually flag for. backporting by the maintainer.  

My point is that this patch does not require backporting, and hence does
not require a fixes tag. The stable AI may choose the patch regardless,
so it's irrelevant.

[...]

> > What about all the other instances that we have in mainline?
> 
> I have not had a chance to review all instances yet. It is not unusually to fix one
> warning at a time, and then go on fixing some more if time permit.

Given that:

* All the suggested code changes so far are harmful to legibility,
  robustness, and maintainability of the code.

* The majority of the warnings (by orders of magnitude) occur for
  intentional overrides, rather than unintentional overrides, as
  assigning default values to arrays and structs is a common idiom.

* We have no known mechanism to selectively disable the warning on a
  per-assignment basis.

... I do not think that is an appropriate strategy here.

For example, I'm fairly certain that if you try to "fix" the instances
in syscall tables, many more people will complain.

A much better approach would be to analyse the warnings, and either:

* find the _real_ bugs where we unintentionally override fields and fix
  those first, or:

* Find the instances that produce the greatest set of false positives
  (e.g. the syscall tables), and figure out how to suppress those
  without harming the maintainability or robustness of the code.

> > I really don't think that we need to go down this road; we're just going
> > to end up adding this to every file that happens to include a header
> > using this schemeâ
> 
> How about disable them this way in a top level like arch/arm64/Makefile or
> arch/arm64/kernel/Makefile? Therefore, there is no need to add this to
> every file, but with a drawback that it could miss a few real issues there
> in the future which probably not many people are checking for them of
> the arm64 subsystem nowadays.

This isn't arm64-specific. We validly use duplicate assignments all over
the kernel, and my position is that we either:

* Find a mechanism to suppress the warning on a per-assignment (not
  per-file) basis, without altering the structure of the existing code.

* Disable the warning tree-wide.

I would vastly prefer the former, as I do agree that this warning _can_
find real bugs, but similarly so can a script that warns "Line $N may
contain a bug" for every line of a C file.

> > Please just turn this off by default for clang.
> 
> As mentioned before, it is very valuable to run âmake W=1â given it found
> many real developer mistakes which will enable â-Woverride-initâ for both
> compilers. Even â-Woverride-initâ itself is useful find real issues as in,
> 
> ae5e033d65a (âmfd: rk808: Fix RK818_IRQ_DISCHG_ILIM initializerâ)
> 32df34d875bb (â[media] rc: img-ir: jvc: Remove unused no-leader timingsâ)
> 
> Especially, to find redundant initializations in large structures. e.g.,
> 
> e6ea0b917875 (â[media] dvb_frontend: Don't declare values twice at a tableâ)
> 
> It is important to keep the noise-level as low as possible by keeping the
> amount of false positives under control to be truly benefit from those
> valuable compiler warnings. 

I agree that we want to minimize the noise, but not at the expense of
the maintainability and robustness of the code, and not by disabling
warnings for arbitrary files.

> > If we want to enable this, we need a mechanism to permit overridable
> > assignments as we use range initializers for.
> 
> I am not sure that it is worth filling a RFE for compilers of that feature.

If that's your position, then I see no point continuing this conversation.

> I feel like the range initializers just another way to initialize an array, and
>  it is just easier to make mistakes with unintended double-initializations.
> The compiler developers probably recommend to enforce more of
> â-Woverride-initâ for  the range initializers rather than permitting it.

>From my analysis in a prior reply, the vast majority of duplicate
assignments in the kernel are intentional. We do that for both arrays
and structures in order to have defaults that can be overridden.

If the compiler developers don't think that's worth supporting, then the
feature is not worth using.

Thanks,
Mark.