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