| Message ID | 1564759944-2197-1-git-send-email-cai@lca.pw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/cache: fix -Woverride-init compiler warnings | expand |
On Fri, Aug 02, 2019 at 11:32:24AM -0400, Qian Cai wrote: > The commit 155433cb365e ("arm64: cache: Remove support for ASID-tagged > VIVT I-caches") introduced some compiation warnings from GCC, > > 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. > > Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches") > Signed-off-by: Qian Cai <cai@lca.pw> > --- > arch/arm64/kernel/cpuinfo.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c > index 876055e37352..193b38da8d96 100644 > --- a/arch/arm64/kernel/cpuinfo.c > +++ b/arch/arm64/kernel/cpuinfo.c > @@ -34,10 +34,10 @@ > static struct cpuinfo_arm64 boot_cpu_data; > > static char *icache_policy_str[] = { > - [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN", > + [ICACHE_POLICY_VPIPT] = "VPIPT", > + [ICACHE_POLICY_VPIPT + 1] = "RESERVED/UNKNOWN", > [ICACHE_POLICY_VIPT] = "VIPT", > [ICACHE_POLICY_PIPT] = "PIPT", > - [ICACHE_POLICY_VPIPT] = "VPIPT", I really don't like this patch. Using "[0 ... MAXIDX] = <default>" is a useful idiom and I think the code is more error-prone the way you have restructured it. Why are you passing -Woverride-init to the compiler anyway? There's only one Makefile that references that option, and it's specific to a pinctrl driver. Will
> On Aug 5, 2019, at 5:52 AM, Will Deacon <will@kernel.org> wrote: > > On Fri, Aug 02, 2019 at 11:32:24AM -0400, Qian Cai wrote: >> The commit 155433cb365e ("arm64: cache: Remove support for ASID-tagged >> VIVT I-caches") introduced some compiation warnings from GCC, >> >> 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. >> >> Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches") >> Signed-off-by: Qian Cai <cai@lca.pw> >> --- >> arch/arm64/kernel/cpuinfo.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c >> index 876055e37352..193b38da8d96 100644 >> --- a/arch/arm64/kernel/cpuinfo.c >> +++ b/arch/arm64/kernel/cpuinfo.c >> @@ -34,10 +34,10 @@ >> static struct cpuinfo_arm64 boot_cpu_data; >> >> static char *icache_policy_str[] = { >> - [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN", >> + [ICACHE_POLICY_VPIPT] = "VPIPT", >> + [ICACHE_POLICY_VPIPT + 1] = "RESERVED/UNKNOWN", >> [ICACHE_POLICY_VIPT] = "VIPT", >> [ICACHE_POLICY_PIPT] = "PIPT", >> - [ICACHE_POLICY_VPIPT] = "VPIPT", > > I really don't like this patch. Using "[0 ... MAXIDX] = <default>" is a > useful idiom and I think the code is more error-prone the way you have > restructured it. > > Why are you passing -Woverride-init to the compiler anyway? There's only > one Makefile that references that option, and it's specific to a pinctrl > driver. Those extra warnings can be enabled by “make W=1”. “-Woverride-init “ seems to be useful to catch potential developer mistakes with unintented double-initializations. It is normal to start to fix the most of false-positives first before globally enabling the flag by default just like “-Wimplicit-fallthrough” mentioned in, https://lwn.net/Articles/794944/
On Mon, Aug 05, 2019 at 07:47:37AM -0400, Qian Cai wrote: > > > > On Aug 5, 2019, at 5:52 AM, Will Deacon <will@kernel.org> wrote: > > > > On Fri, Aug 02, 2019 at 11:32:24AM -0400, Qian Cai wrote: > >> The commit 155433cb365e ("arm64: cache: Remove support for ASID-tagged > >> VIVT I-caches") introduced some compiation warnings from GCC, > >> > >> 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. > >> > >> Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches") > >> Signed-off-by: Qian Cai <cai@lca.pw> > >> --- > >> arch/arm64/kernel/cpuinfo.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c > >> index 876055e37352..193b38da8d96 100644 > >> --- a/arch/arm64/kernel/cpuinfo.c > >> +++ b/arch/arm64/kernel/cpuinfo.c > >> @@ -34,10 +34,10 @@ > >> static struct cpuinfo_arm64 boot_cpu_data; > >> > >> static char *icache_policy_str[] = { > >> - [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN", > >> + [ICACHE_POLICY_VPIPT] = "VPIPT", > >> + [ICACHE_POLICY_VPIPT + 1] = "RESERVED/UNKNOWN", > >> [ICACHE_POLICY_VIPT] = "VIPT", > >> [ICACHE_POLICY_PIPT] = "PIPT", > >> - [ICACHE_POLICY_VPIPT] = "VPIPT", > > > > I really don't like this patch. Using "[0 ... MAXIDX] = <default>" is a > > useful idiom and I think the code is more error-prone the way you have > > restructured it. > > > > Why are you passing -Woverride-init to the compiler anyway? There's only > > one Makefile that references that option, and it's specific to a pinctrl > > driver. > > Those extra warnings can be enabled by “make W=1”. “-Woverride-init “ seems to be useful > to catch potential developer mistakes with unintented double-initializations. It is normal to > start to fix the most of false-positives first before globally enabling the flag by default just like > “-Wimplicit-fallthrough” mentioned in, > > https://lwn.net/Articles/794944/ I think this case is completely different to the implicit fallthrough stuff. The solution there was simply to add a comment without restructuring the surrounding code. What your patch does here is actively make the code harder to understand. Initialising a static array with a non-zero pattern is a useful idiom and I don't think we should throw that away just to appease a silly compiler warning that appears only with non-default build options. Have a look at the way we use PERF_MAP_ALL_UNSUPPORTED in the Arm PMU code, for example. Will
> On Aug 5, 2019, at 10:01 AM, Will Deacon <will@kernel.org> wrote: > > On Mon, Aug 05, 2019 at 07:47:37AM -0400, Qian Cai wrote: >> >> >>> On Aug 5, 2019, at 5:52 AM, Will Deacon <will@kernel.org> wrote: >>> >>> On Fri, Aug 02, 2019 at 11:32:24AM -0400, Qian Cai wrote: >>>> The commit 155433cb365e ("arm64: cache: Remove support for ASID-tagged >>>> VIVT I-caches") introduced some compiation warnings from GCC, >>>> >>>> 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. >>>> >>>> Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches") >>>> Signed-off-by: Qian Cai <cai@lca.pw> >>>> --- >>>> arch/arm64/kernel/cpuinfo.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c >>>> index 876055e37352..193b38da8d96 100644 >>>> --- a/arch/arm64/kernel/cpuinfo.c >>>> +++ b/arch/arm64/kernel/cpuinfo.c >>>> @@ -34,10 +34,10 @@ >>>> static struct cpuinfo_arm64 boot_cpu_data; >>>> >>>> static char *icache_policy_str[] = { >>>> - [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN", >>>> + [ICACHE_POLICY_VPIPT] = "VPIPT", >>>> + [ICACHE_POLICY_VPIPT + 1] = "RESERVED/UNKNOWN", >>>> [ICACHE_POLICY_VIPT] = "VIPT", >>>> [ICACHE_POLICY_PIPT] = "PIPT", >>>> - [ICACHE_POLICY_VPIPT] = "VPIPT", >>> >>> I really don't like this patch. Using "[0 ... MAXIDX] = <default>" is a >>> useful idiom and I think the code is more error-prone the way you have >>> restructured it. >>> >>> Why are you passing -Woverride-init to the compiler anyway? There's only >>> one Makefile that references that option, and it's specific to a pinctrl >>> driver. >> >> Those extra warnings can be enabled by “make W=1”. “-Woverride-init “ seems to be useful >> to catch potential developer mistakes with unintented double-initializations. It is normal to >> start to fix the most of false-positives first before globally enabling the flag by default just like >> “-Wimplicit-fallthrough” mentioned in, >> >> https://lwn.net/Articles/794944/ > > I think this case is completely different to the implicit fallthrough stuff. > The solution there was simply to add a comment without restructuring the > surrounding code. What your patch does here is actively make the code harder > to understand. > > Initialising a static array with a non-zero pattern is a useful idiom and I > don't think we should throw that away just to appease a silly compiler > warning that appears only with non-default build options. Have a look at > the way we use PERF_MAP_ALL_UNSUPPORTED in the Arm PMU code, for example. Well, both GCC and Clang would generate warnings for those. Clang even enable this by default, https://releases.llvm.org/8.0.0/tools/clang/docs/DiagnosticsReference.html#winitializer-overrides Assume compiler people are sane, I probably not call those are “silly”.
On Mon, Aug 05, 2019 at 11:50:03PM -0400, Qian Cai wrote: > > > > On Aug 5, 2019, at 10:01 AM, Will Deacon <will@kernel.org> wrote: > > > > On Mon, Aug 05, 2019 at 07:47:37AM -0400, Qian Cai wrote: > >> > >> > >>> On Aug 5, 2019, at 5:52 AM, Will Deacon <will@kernel.org> wrote: > >>> > >>> On Fri, Aug 02, 2019 at 11:32:24AM -0400, Qian Cai wrote: > >>>> The commit 155433cb365e ("arm64: cache: Remove support for ASID-tagged > >>>> VIVT I-caches") introduced some compiation warnings from GCC, > >>>> > >>>> 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. > >>>> > >>>> Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches") > >>>> Signed-off-by: Qian Cai <cai@lca.pw> > >>>> --- > >>>> arch/arm64/kernel/cpuinfo.c | 4 ++-- > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c > >>>> index 876055e37352..193b38da8d96 100644 > >>>> --- a/arch/arm64/kernel/cpuinfo.c > >>>> +++ b/arch/arm64/kernel/cpuinfo.c > >>>> @@ -34,10 +34,10 @@ > >>>> static struct cpuinfo_arm64 boot_cpu_data; > >>>> > >>>> static char *icache_policy_str[] = { > >>>> - [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN", > >>>> + [ICACHE_POLICY_VPIPT] = "VPIPT", > >>>> + [ICACHE_POLICY_VPIPT + 1] = "RESERVED/UNKNOWN", > >>>> [ICACHE_POLICY_VIPT] = "VIPT", > >>>> [ICACHE_POLICY_PIPT] = "PIPT", > >>>> - [ICACHE_POLICY_VPIPT] = "VPIPT", > >>> > >>> I really don't like this patch. Using "[0 ... MAXIDX] = <default>" is a > >>> useful idiom and I think the code is more error-prone the way you have > >>> restructured it. > >>> > >>> Why are you passing -Woverride-init to the compiler anyway? There's only > >>> one Makefile that references that option, and it's specific to a pinctrl > >>> driver. > >> > >> Those extra warnings can be enabled by “make W=1”. “-Woverride-init “ seems to be useful > >> to catch potential developer mistakes with unintented double-initializations. It is normal to > >> start to fix the most of false-positives first before globally enabling the flag by default just like > >> “-Wimplicit-fallthrough” mentioned in, > >> > >> https://lwn.net/Articles/794944/ > > > > I think this case is completely different to the implicit fallthrough stuff. > > The solution there was simply to add a comment without restructuring the > > surrounding code. What your patch does here is actively make the code harder > > to understand. > > > > Initialising a static array with a non-zero pattern is a useful idiom and I > > don't think we should throw that away just to appease a silly compiler > > warning that appears only with non-default build options. Have a look at > > the way we use PERF_MAP_ALL_UNSUPPORTED in the Arm PMU code, for example. > > Well, both GCC and Clang would generate warnings for those. Clang even enable this by > default, > > https://releases.llvm.org/8.0.0/tools/clang/docs/DiagnosticsReference.html#winitializer-overrides > > Assume compiler people are sane, I probably not call those are “silly”. We're not disputing the sanity of compiler folk; Will did not say anything about that. The warning is unhelpful in the case of the [0 ... MAXIDX] = <default> idiom, and the modification you suggest: * Makes the code harder to read. * Increases the necessary context. e.g. I must know the specific values of the enum to know that ICACHE_POLICY_VPIPT + 1 is the unallocated slot. * Less robust. If the enum gets re-ordered, we must update the array. If the enum is expanded, new elements must be added to the array to initialize entries to the default value, which also makes the code more verbose and painful to read. IIUC if we don't explicitly initialize an element, we won't get a warning, which would be harmful. If there's some way to mark the default initialization as overridable, I think that would be fine, e.g. struct foo *array[] = { [0 ... MAXIDX] __default = <default>, [SOMEIDX] = <someval>, [OTHERIDX] = <otherval>, } We have a number of cases where the [0 ... MAXIDX] = <default> idiom are used, and I don't think that any of them should be changed in the manner suggested by this patch. Thanks, Mark.
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c index 876055e37352..193b38da8d96 100644 --- a/arch/arm64/kernel/cpuinfo.c +++ b/arch/arm64/kernel/cpuinfo.c @@ -34,10 +34,10 @@ static struct cpuinfo_arm64 boot_cpu_data; static char *icache_policy_str[] = { - [0 ... ICACHE_POLICY_PIPT] = "RESERVED/UNKNOWN", + [ICACHE_POLICY_VPIPT] = "VPIPT", + [ICACHE_POLICY_VPIPT + 1] = "RESERVED/UNKNOWN", [ICACHE_POLICY_VIPT] = "VIPT", [ICACHE_POLICY_PIPT] = "PIPT", - [ICACHE_POLICY_VPIPT] = "VPIPT", }; unsigned long __icache_flags;
The commit 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches") introduced some compiation warnings from GCC, 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. Fixes: 155433cb365e ("arm64: cache: Remove support for ASID-tagged VIVT I-caches") Signed-off-by: Qian Cai <cai@lca.pw> --- arch/arm64/kernel/cpuinfo.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)