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

[Mark Rutland]

On Mon, Aug 05, 2019 at 11:50:03PM -0400, Qian Cai wrote:
> 
> 
> > On Aug 5, 2019, at 10:01 AM, Will Deacon <will@xxxxxxxxxx> wrote:
> > 
> > On Mon, Aug 05, 2019 at 07:47:37AM -0400, Qian Cai wrote:
> >> 
> >> 
> >>> On Aug 5, 2019, at 5:52 AM, Will Deacon <will@xxxxxxxxxx> wrote:
> >>> 
> >>> On Fri, Aug 02, 2019 at 11:32:24AM -0400, Qian Cai wrote:
> >>>> The commit 155433cb365e ("arm64: cache: Remove support for ASID-tagged
> >>>> VIVT I-caches") introduced some compiation warnings from GCC,
> >>>> 
> >>>> 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.
> >>>> 
> >>>> Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches")
> >>>> Signed-off-by: Qian Cai <cai@xxxxxx>
> >>>> ---
> >>>> arch/arm64/kernel/cpuinfo.c | 4 ++--
> >>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>> 
> >>>> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> >>>> index 876055e37352..193b38da8d96 100644
> >>>> --- a/arch/arm64/kernel/cpuinfo.c
> >>>> +++ b/arch/arm64/kernel/cpuinfo.c
> >>>> @@ -34,10 +34,10 @@
> >>>> static struct cpuinfo_arm64 boot_cpu_data;
> >>>> 
> >>>> static char *icache_policy_str[] = {
> >>>> -	[0 ... ICACHE_POLICY_PIPT]	= "RESERVED/UNKNOWN",
> >>>> +	[ICACHE_POLICY_VPIPT]		= "VPIPT",
> >>>> +	[ICACHE_POLICY_VPIPT + 1]	= "RESERVED/UNKNOWN",
> >>>> 	[ICACHE_POLICY_VIPT]		= "VIPT",
> >>>> 	[ICACHE_POLICY_PIPT]		= "PIPT",
> >>>> -	[ICACHE_POLICY_VPIPT]		= "VPIPT",
> >>> 
> >>> I really don't like this patch. Using "[0 ... MAXIDX] = <default>" is a
> >>> useful idiom and I think the code is more error-prone the way you have
> >>> restructured it.
> >>> 
> >>> Why are you passing -Woverride-init to the compiler anyway? There's only
> >>> one Makefile that references that option, and it's specific to a pinctrl
> >>> driver.
> >> 
> >> Those extra warnings can be enabled by âmake W=1â. â-Woverride-init â seems to be useful
> >> to catch potential developer mistakes with unintented double-initializations. It is normal to
> >> start to fix the most of false-positives first before globally enabling the flag by default just like
> >> â-Wimplicit-fallthroughâ mentioned in,
> >> 
> >> https://lwn.net/Articles/794944/
> > 
> > I think this case is completely different to the implicit fallthrough stuff.
> > The solution there was simply to add a comment without restructuring the
> > surrounding code. What your patch does here is actively make the code harder
> > to understand.
> > 
> > Initialising a static array with a non-zero pattern is a useful idiom and I
> > don't think we should throw that away just to appease a silly compiler
> > warning that appears only with non-default build options. Have a look at
> > the way we use PERF_MAP_ALL_UNSUPPORTED in the Arm PMU code, for example.
> 
> Well, both GCC and Clang would generate warnings for those. Clang even enable this by
> default,
> 
> https://releases.llvm.org/8.0.0/tools/clang/docs/DiagnosticsReference.html#winitializer-overrides
> 
> Assume compiler people are sane, I probably not call those are âsillyâ.

We're not disputing the sanity of compiler folk; Will did not say
anything about that.

The warning is unhelpful in the case of the [0 ... MAXIDX] = <default>
idiom, and the modification you suggest:

* Makes the code harder to read.

* Increases the necessary context. e.g. I must know the specific values
  of the enum to know that ICACHE_POLICY_VPIPT + 1 is the unallocated
  slot.

* Less robust. If the enum gets re-ordered, we must update the array.
  If the enum is expanded, new elements must be added to the array to
  initialize entries to the default value, which also makes the code
  more verbose and painful to read. IIUC if we don't explicitly
  initialize an element, we won't get a warning, which would be harmful.

If there's some way to mark the default initialization as overridable,
I think that would be fine, e.g.

struct foo *array[] = {
	[0 ... MAXIDX] __default = <default>,
	[SOMEIDX] = <someval>,
	[OTHERIDX] = <otherval>,
}

We have a number of cases where the [0 ... MAXIDX] = <default> idiom are
used, and I don't think that any of them should be changed in the manner
suggested by this patch.


Thanks,
Mark.