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

[Nathan Chancellor <natechancellor@gmail.com>]

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