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

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

[Qian Cai]

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@lca.pw>
---
 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)

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

[Nathan Chancellor]

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@lca.pw>

It's a shame we can't just use one cc-disable-warning statement but
-Woverride-init wasn't added for GCC compatibility until clang 8.0.0
and we don't have an established minimum clang version.

With that said, I applied your patch and I don't see with warning with
W=1 anymore and I see both options get added to the clang command line
with V=1.

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
Tested-by: Nathan Chancellor <natechancellor@gmail.com>

Cheers!

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

[Mark Rutland]

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@lca.pw>

This is _not_ a fix, and should not require backporting to stable trees.

What about all the other instances that we have in mainline?

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

Please just turn this off by default for clang.

If we want to enable this, we need a mechanism to permit overridable
assignments as we use range initializers for.

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

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

[Nathan Chancellor]

On Thu, Aug 08, 2019 at 11:38:08AM +0100, Mark Rutland 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@lca.pw>
> 
> This is _not_ a fix, and should not require backporting to stable trees.
> 
> What about all the other instances that we have in mainline?
> 
> 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...
> 
> Please just turn this off by default for clang.
> 
> If we want to enable this, we need a mechanism to permit overridable
> assignments as we use range initializers for.
> 
> Thanks,
> Mark.
> 

For what it's worth, this is disabled by default for clang in the
kernel:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Makefile.extrawarn?h=v5.3-rc3#n69

It only becomes visible with clang at W=1 because that section doesn't
get applied. It becomes visible with GCC at W=1 because of -Wextra.

Cheers,
Nathan

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

[Qian Cai]

> On Aug 8, 2019, at 6:38 AM, Mark Rutland <mark.rutland@arm.com> 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@lca.pw>
> 
> 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)

Explicitly marking initializer overrides (was "Re: [PATCH] arm64/cache: silence -Woverride-init warnings")

[Mark Rutland]

On Thu, Aug 08, 2019 at 10:09:16AM -0700, Nathan Chancellor wrote:
> On Thu, Aug 08, 2019 at 11:38:08AM +0100, Mark Rutland 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@lca.pw>
> > 
> > This is _not_ a fix, and should not require backporting to stable trees.
> > 
> > What about all the other instances that we have in mainline?
> > 
> > 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...
> > 
> > Please just turn this off by default for clang.
> > 
> > If we want to enable this, we need a mechanism to permit overridable
> > assignments as we use range initializers for.
> > 
> > Thanks,
> > Mark.
> > 
> 
> For what it's worth, this is disabled by default for clang in the
> kernel:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Makefile.extrawarn?h=v5.3-rc3#n69
> 
> It only becomes visible with clang at W=1 because that section doesn't
> get applied. It becomes visible with GCC at W=1 because of -Wextra.

Thanks for clarifying that!

Do you know if there's any existing mechanism that we can use to silence
the warning on a per-assignment basis? Either to say that an assignment
can be overridden, or that the assignment is expected to override an
existing assignment?

If not, who would be able to look at adding a mechanism to clang for
this?

If we could have some attribute or intrinsic that we could wrap like:

struct foo f = {
	.bar __defaultval = <default>,
	.bar = <newval>,		// no warning
	.bar = <anotherval>,		// warning
};

... or:

struct foo f = {
	.bar = <default>,
	.bar __override = <newval>,	// no warning
	.bar = <anotherval>,		// warning
};

... or:
	
	.bar = OVERRIDE(<newval>),	// no warning

... or:
	OVERRIDE(.bar) = <newval>,	// no warning

... then I think it would be possible to make use of the warning
effectively, as we could distinguish intentional overrides from
unintentional ones, and annotating assignments in this way doesn't seem
onerous to me.

Thanks,
Mark.

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@arm.com> 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@lca.pw>
> > 
> > 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.

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

[Will Deacon]

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@arm.com> wrote:
> > 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,

Every single patch you've sent to me resulting from building with "W=1" has
been a false positive. Every. Single. One. That's not what I would call
"very valuable". It's probably what I'd call a "colossal waste of
everybody's time", especially as your tendancy to shoot from the hip when
writing these so-called fixes has resulted in some patches that ACTUALLY
INTRODUCED REAL BUGS. Do you see the insanity here?

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

So that's where you and I have a disagreement. I think maintainability
of the code is the single most important thing to consider after
correctness.

If code isn't maintainable, then it's liable to churn and be a constant
source of bugs as people keep tripping over whatever subtleties lie within.
In some cases, we have little choice and the combination of things like
performance requirements and compatibility force us down a path which we
wouldn't otherwise have considered. However, appeasing a compiler warning
that *doesn't even appear with the default build options* does not quality
for this sort of treatment in my opinion, so I will not be applying your
patch.

Please stop sending it. Real fixes, sure, but not this rubbish.

Will

Re: Explicitly marking initializer overrides (was "Re: [PATCH] arm64/cache: silence -Woverride-init warnings")

[Robin Murphy]

On 09/08/2019 09:32, Mark Rutland wrote:
> On Thu, Aug 08, 2019 at 10:09:16AM -0700, Nathan Chancellor wrote:
>> On Thu, Aug 08, 2019 at 11:38:08AM +0100, Mark Rutland 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@lca.pw>
>>>
>>> This is _not_ a fix, and should not require backporting to stable trees.
>>>
>>> What about all the other instances that we have in mainline?
>>>
>>> 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...
>>>
>>> Please just turn this off by default for clang.
>>>
>>> If we want to enable this, we need a mechanism to permit overridable
>>> assignments as we use range initializers for.
>>>
>>> Thanks,
>>> Mark.
>>>
>>
>> For what it's worth, this is disabled by default for clang in the
>> kernel:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Makefile.extrawarn?h=v5.3-rc3#n69
>>
>> It only becomes visible with clang at W=1 because that section doesn't
>> get applied. It becomes visible with GCC at W=1 because of -Wextra.
> 
> Thanks for clarifying that!
> 
> Do you know if there's any existing mechanism that we can use to silence
> the warning on a per-assignment basis? Either to say that an assignment
> can be overridden, or that the assignment is expected to override an
> existing assignment?
> 
> If not, who would be able to look at adding a mechanism to clang for
> this?
> 
> If we could have some attribute or intrinsic that we could wrap like:
> 
> struct foo f = {
> 	.bar __defaultval = <default>,
> 	.bar = <newval>,		// no warning
> 	.bar = <anotherval>,		// warning
> };
> 
> ... or:
> 
> struct foo f = {
> 	.bar = <default>,
> 	.bar __override = <newval>,	// no warning
> 	.bar = <anotherval>,		// warning
> };
> 
> ... or:
> 	
> 	.bar = OVERRIDE(<newval>),	// no warning
> 
> ... or:
> 	OVERRIDE(.bar) = <newval>,	// no warning
> 
> ... then I think it would be possible to make use of the warning
> effectively, as we could distinguish intentional overrides from
> unintentional ones, and annotating assignments in this way doesn't seem
> onerous to me.

Tangentially, there might also be value in some kind of "must be 
explicitly initialised" attribute that would warn if any element was not 
covered by (at least one) initialiser. For cases like our 
icache_policy_str one, where using the "default + overrides" pattern for 
the sake of one reserved entry is more about robustness against the 
array growing in future than simpler code today, that could arguably be 
a more appropriate option.

Robin.

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

[Nick Desaulniers]

On Fri, Aug 9, 2019 at 1:53 AM Mark Rutland <mark.rutland@arm.com> wrote:
> * Find a mechanism to suppress the warning on a per-assignment (not
>   per-file) basis, without altering the structure of the existing code.

#pragma push/pop can be used to suppress warnings in a localized
section of a translation unit.

-- 
Thanks,
~Nick Desaulniers

Re: Explicitly marking initializer overrides (was "Re: [PATCH] arm64/cache: silence -Woverride-init warnings")

[Nathan Chancellor]

On Fri, Aug 09, 2019 at 09:32:51AM +0100, Mark Rutland wrote:
> On Thu, Aug 08, 2019 at 10:09:16AM -0700, Nathan Chancellor wrote:
> > On Thu, Aug 08, 2019 at 11:38:08AM +0100, Mark Rutland 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@lca.pw>
> > > 
> > > This is _not_ a fix, and should not require backporting to stable trees.
> > > 
> > > What about all the other instances that we have in mainline?
> > > 
> > > 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...
> > > 
> > > Please just turn this off by default for clang.
> > > 
> > > If we want to enable this, we need a mechanism to permit overridable
> > > assignments as we use range initializers for.
> > > 
> > > Thanks,
> > > Mark.
> > > 
> > 
> > For what it's worth, this is disabled by default for clang in the
> > kernel:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Makefile.extrawarn?h=v5.3-rc3#n69
> > 
> > It only becomes visible with clang at W=1 because that section doesn't
> > get applied. It becomes visible with GCC at W=1 because of -Wextra.
> 
> Thanks for clarifying that!
> 
> Do you know if there's any existing mechanism that we can use to silence
> the warning on a per-assignment basis? Either to say that an assignment
> can be overridden, or that the assignment is expected to override an
> existing assignment?
> 

I don't think there is, from the brief amount of research I did.

> If not, who would be able to look at adding a mechanism to clang for
> this?
> 

I've filed https://github.com/ClangBuiltLinux/linux/issues/639 on our
issue tracker so that I can try to remember to distill all of this
down and file an LLVM bug.

> If we could have some attribute or intrinsic that we could wrap like:
> 
> struct foo f = {
> 	.bar __defaultval = <default>,
> 	.bar = <newval>,		// no warning
> 	.bar = <anotherval>,		// warning
> };
> 
> ... or:
> 
> struct foo f = {
> 	.bar = <default>,
> 	.bar __override = <newval>,	// no warning
> 	.bar = <anotherval>,		// warning
> };
> 
> ... or:
> 	
> 	.bar = OVERRIDE(<newval>),	// no warning
> 
> ... or:
> 	OVERRIDE(.bar) = <newval>,	// no warning
> 
> ... then I think it would be possible to make use of the warning
> effectively, as we could distinguish intentional overrides from
> unintentional ones, and annotating assignments in this way doesn't seem
> onerous to me.
> 
> Thanks,
> Mark.

I definitely think it is an interesting idea, hopefully some of our
resident clang experts can weigh in and see how feasible it would be to
implement.

Cheers,
Nathan