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

[Qian Cai]

> 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.  

> 
> 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.

> 
> 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.

> 
> 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. 

> 
> 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.
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.

> 
> Thanks,
> Mark.
> 
>> ---
>> arch/arm64/kernel/Makefile | 3 +++
>> 1 file changed, 3 insertions(+)
>> 
>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>> index 478491f07b4f..397ed5f7be1e 100644
>> --- a/arch/arm64/kernel/Makefile
>> +++ b/arch/arm64/kernel/Makefile
>> @@ -11,6 +11,9 @@ CFLAGS_REMOVE_ftrace.o = $(CC_FLAGS_FTRACE)
>> CFLAGS_REMOVE_insn.o = $(CC_FLAGS_FTRACE)
>> CFLAGS_REMOVE_return_address.o = $(CC_FLAGS_FTRACE)
>> 
>> +CFLAGS_cpuinfo.o += $(call cc-disable-warning, override-init)
>> +CFLAGS_cpuinfo.o += $(call cc-disable-warning, initializer-overrides)
>> +
>> # Object file lists.
>> obj-y			:= debug-monitors.o entry.o irq.o fpsimd.o		\
>> 			   entry-fpsimd.o process.o ptrace.o setup.o signal.o	\
>> -- 
>> 2.20.1 (Apple Git-117)