| Message ID | 20190808032916.879-1-cai@lca.pw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/cache: silence -Woverride-init warnings | expand |
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!
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) >
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
> 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)
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.
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.
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
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.
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.
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
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 \
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(+)