Mailing List Archive: [PATCH 4/4] arm64: traps: fix -Woverride-init warnings

[PATCH 4/4] arm64: traps: fix -Woverride-init warnings

Oct 26, 2020, 10:15 AM

Post #1 of 7 (16 views)

From: Arnd Bergmann <arnd@arndb.de>

There are many warnings in this file when we re-enable the
Woverride-init flag:

arch/arm64/kernel/traps.c:704:26: warning: initialized field overwritten [-Woverride-init]
704 | [ESR_ELx_EC_UNKNOWN] = "Unknown/Uncategorized",
| ^~~~~~~~~~~~~~~~~~~~~~~
arch/arm64/kernel/traps.c:704:26: note: (near initialization for 'esr_class_str[0]')
arch/arm64/kernel/traps.c:705:22: warning: initialized field overwritten [-Woverride-init]
705 | [ESR_ELx_EC_WFx] = "WFI/WFE",
| ^~~~~~~~~

This is harmless since they are only informational strings,
but it's easy to change the code to ignore missing initialization
and instead warn about possible duplicate initializers.

Fixes: 60a1f02c9e91 ("arm64: decode ESR_ELx.EC when reporting exceptions")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
arch/arm64/kernel/traps.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 8af4e0e85736..d21cb25f9e1f 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -700,7 +700,6 @@ void do_sysinstr(unsigned int esr, struct pt_regs *regs)
NOKPROBE_SYMBOL(do_sysinstr);

static const char *esr_class_str[] = {
- [0 ... ESR_ELx_EC_MAX] = "UNRECOGNIZED EC",
[ESR_ELx_EC_UNKNOWN] = "Unknown/Uncategorized",
[ESR_ELx_EC_WFx] = "WFI/WFE",
[ESR_ELx_EC_CP15_32] = "CP15 MCR/MRC",
@@ -746,7 +745,7 @@ static const char *esr_class_str[] = {

const char *esr_get_class_string(u32 esr)
{
- return esr_class_str[ESR_ELx_EC(esr)];
+ return esr_class_str[ESR_ELx_EC(esr)] ?: "UNRECOGNIZED EC";
}

/*
--
2.27.0

Re: [PATCH 4/4] arm64: traps: fix -Woverride-init warnings [ In reply to ]

mark.rutland at arm

Oct 26, 2020, 10:15 AM

Post #2 of 7 (16 views)

Permalink

Hi Arnd,

On Mon, Oct 26, 2020 at 05:03:31PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> There are many warnings in this file when we re-enable the
> Woverride-init flag:
>
> arch/arm64/kernel/traps.c:704:26: warning: initialized field overwritten [-Woverride-init]
> 704 | [ESR_ELx_EC_UNKNOWN] = "Unknown/Uncategorized",
> | ^~~~~~~~~~~~~~~~~~~~~~~
> arch/arm64/kernel/traps.c:704:26: note: (near initialization for 'esr_class_str[0]')
> arch/arm64/kernel/traps.c:705:22: warning: initialized field overwritten [-Woverride-init]
> 705 | [ESR_ELx_EC_WFx] = "WFI/WFE",
> | ^~~~~~~~~
>
> This is harmless since they are only informational strings,
> but it's easy to change the code to ignore missing initialization
> and instead warn about possible duplicate initializers.

This has come up before, and IMO the warning is more hindrance than
helpful, given the prevalance of spurious warnings, and the (again IMO)
the rework needed to avoid those making the code harder to reason about.

We use this pattern all througout the kernel (e.g. in the syscall
wrappers), so unless the plan is to avoid this everywhere, I don't think
that we should alter individual cases. I also don't think that the Fixes
tag is appropriate given the code is correct.

Could we instead convince the compiler folk to give us better tools to
deal with this? For example, if we could annotate assignmments as
overridable or being an override, it'd be possible to distinguish the
benign cases from bad ones, without forcing us to have dynamic checks.

Thanks,
Mark.

>
> Fixes: 60a1f02c9e91 ("arm64: decode ESR_ELx.EC when reporting exceptions")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> arch/arm64/kernel/traps.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 8af4e0e85736..d21cb25f9e1f 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -700,7 +700,6 @@ void do_sysinstr(unsigned int esr, struct pt_regs *regs)
> NOKPROBE_SYMBOL(do_sysinstr);
>
> static const char *esr_class_str[] = {
> - [0 ... ESR_ELx_EC_MAX] = "UNRECOGNIZED EC",
> [ESR_ELx_EC_UNKNOWN] = "Unknown/Uncategorized",
> [ESR_ELx_EC_WFx] = "WFI/WFE",
> [ESR_ELx_EC_CP15_32] = "CP15 MCR/MRC",
> @@ -746,7 +745,7 @@ static const char *esr_class_str[] = {
>
> const char *esr_get_class_string(u32 esr)
> {
> - return esr_class_str[ESR_ELx_EC(esr)];
> + return esr_class_str[ESR_ELx_EC(esr)] ?: "UNRECOGNIZED EC";
> }
>
> /*
> --
> 2.27.0
>

Re: [PATCH 4/4] arm64: traps: fix -Woverride-init warnings [ In reply to ]

arnd at kernel

Oct 26, 2020, 10:15 AM

Post #3 of 7 (16 views)

Permalink

On Mon, Oct 26, 2020 at 5:23 PM Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Oct 26, 2020 at 05:03:31PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > There are many warnings in this file when we re-enable the
> > Woverride-init flag:
> >
> > arch/arm64/kernel/traps.c:704:26: warning: initialized field overwritten [-Woverride-init]
> > 704 | [ESR_ELx_EC_UNKNOWN] = "Unknown/Uncategorized",
> > | ^~~~~~~~~~~~~~~~~~~~~~~
> > arch/arm64/kernel/traps.c:704:26: note: (near initialization for 'esr_class_str[0]')
> > arch/arm64/kernel/traps.c:705:22: warning: initialized field overwritten [-Woverride-init]
> > 705 | [ESR_ELx_EC_WFx] = "WFI/WFE",
> > | ^~~~~~~~~
> >
> > This is harmless since they are only informational strings,
> > but it's easy to change the code to ignore missing initialization
> > and instead warn about possible duplicate initializers.
>
> This has come up before, and IMO the warning is more hindrance than
> helpful, given the prevalance of spurious warnings, and the (again IMO)
> the rework needed to avoid those making the code harder to reason about.
>
> We use this pattern all througout the kernel (e.g. in the syscall
> wrappers), so unless the plan is to avoid this everywhere, I don't think
> that we should alter individual cases.

I have patches for all instances, yes.

> I also don't think that the Fixes tag is appropriate given the code is correct.

I tend to add fixes tags even for false-positive warnings, as they
are helpful whenever someone tries to backport the warning
suppression patch to older kernels. That could easily be dropped
here of course.

> Could we instead convince the compiler folk to give us better tools to
> deal with this? For example, if we could annotate assignmments as
> overridable or being an override, it'd be possible to distinguish the
> benign cases from bad ones, without forcing us to have dynamic checks.

There are only a handful of instances that need this, and half of these
are in arch/arm64/. I have another patch that disables the warning
locally in arch/arm64/kernel/{perf_event,sys,sys32}.c and
arch/arm64/kvm/handle_exit.c, but this needs some extra
infrastructure to make it possible to disable it for both gcc
and clang (which use different warning flags for it), so I did not
include the patch in this series.

I had also considered disabling the two warning flags for the
entire arch/arm64/kernel/ directory, but that seemed wrong
after I noticed the cpu_errata.c warnings that are indeed suspicious
and could lead to bugs when additional changes are made to
that file.

Arnd

Re: [PATCH 4/4] arm64: traps: fix -Woverride-init warnings [ In reply to ]

peter.maydell at linaro

Oct 26, 2020, 10:15 AM

Post #4 of 7 (16 views)

Permalink

On Mon, 26 Oct 2020 at 16:23, Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Arnd,
>
> On Mon, Oct 26, 2020 at 05:03:31PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > There are many warnings in this file when we re-enable the
> > Woverride-init flag:
> >
> > arch/arm64/kernel/traps.c:704:26: warning: initialized field overwritten [-Woverride-init]
> > 704 | [ESR_ELx_EC_UNKNOWN] = "Unknown/Uncategorized",
> > | ^~~~~~~~~~~~~~~~~~~~~~~
> > arch/arm64/kernel/traps.c:704:26: note: (near initialization for 'esr_class_str[0]')
> > arch/arm64/kernel/traps.c:705:22: warning: initialized field overwritten [-Woverride-init]
> > 705 | [ESR_ELx_EC_WFx] = "WFI/WFE",
> > | ^~~~~~~~~
> >
> > This is harmless since they are only informational strings,
> > but it's easy to change the code to ignore missing initialization
> > and instead warn about possible duplicate initializers.
>
> This has come up before, and IMO the warning is more hindrance than
> helpful, given the prevalance of spurious warnings, and the (again IMO)
> the rework needed to avoid those making the code harder to reason about

FWIW in QEMU we turn the clang version of this off with
-Wno-initializer-overrides because we agree that the code is
fine and the compiler is being unhelpful in this case. (There's
a reason gcc doesn't put it in -Wall.)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91688 is a request
for something that would catch bugs without breaking ranged-array
initializer syntax usage, but the gcc devs don't seem to have
responded.

thanks
-- PMM

Re: [PATCH 4/4] arm64: traps: fix -Woverride-init warnings [ In reply to ]

will at kernel

Oct 26, 2020, 12:15 PM

Post #5 of 7 (16 views)

Permalink

On Mon, Oct 26, 2020 at 05:13:30PM +0000, Peter Maydell wrote:
> On Mon, 26 Oct 2020 at 16:23, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Oct 26, 2020 at 05:03:31PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > There are many warnings in this file when we re-enable the
> > > Woverride-init flag:
> > >
> > > arch/arm64/kernel/traps.c:704:26: warning: initialized field overwritten [-Woverride-init]
> > > 704 | [ESR_ELx_EC_UNKNOWN] = "Unknown/Uncategorized",
> > > | ^~~~~~~~~~~~~~~~~~~~~~~
> > > arch/arm64/kernel/traps.c:704:26: note: (near initialization for 'esr_class_str[0]')
> > > arch/arm64/kernel/traps.c:705:22: warning: initialized field overwritten [-Woverride-init]
> > > 705 | [ESR_ELx_EC_WFx] = "WFI/WFE",
> > > | ^~~~~~~~~
> > >
> > > This is harmless since they are only informational strings,
> > > but it's easy to change the code to ignore missing initialization
> > > and instead warn about possible duplicate initializers.
> >
> > This has come up before, and IMO the warning is more hindrance than
> > helpful, given the prevalance of spurious warnings, and the (again IMO)
> > the rework needed to avoid those making the code harder to reason about
>
> FWIW in QEMU we turn the clang version of this off with
> -Wno-initializer-overrides because we agree that the code is
> fine and the compiler is being unhelpful in this case. (There's
> a reason gcc doesn't put it in -Wall.)
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91688 is a request
> for something that would catch bugs without breaking ranged-array
> initializer syntax usage, but the gcc devs don't seem to have
> responded.

Yes, I'm inclined to agree. The code is fine, and "fixing" it just leads to
churn and the possible introduction of bugs.

Will

Re: [PATCH 4/4] arm64: traps: fix -Woverride-init warnings [ In reply to ]

arnd at kernel

Oct 26, 2020, 2:15 PM

Post #6 of 7 (16 views)

Permalink

On Mon, Oct 26, 2020 at 6:27 PM Will Deacon <will@kernel.org> wrote:
> On Mon, Oct 26, 2020 at 05:13:30PM +0000, Peter Maydell wrote:
> > On Mon, 26 Oct 2020 at 16:23, Mark Rutland <mark.rutland@arm.com> wrote:
> > > On Mon, Oct 26, 2020 at 05:03:31PM +0100, Arnd Bergmann wrote:
> > > > From: Arnd Bergmann <arnd@arndb.de>
> > > >
> > > > There are many warnings in this file when we re-enable the
> > > > Woverride-init flag:
> > > >
> > > > arch/arm64/kernel/traps.c:704:26: warning: initialized field overwritten [-Woverride-init]
> > > > 704 | [ESR_ELx_EC_UNKNOWN] = "Unknown/Uncategorized",
> > > > | ^~~~~~~~~~~~~~~~~~~~~~~
> > > > arch/arm64/kernel/traps.c:704:26: note: (near initialization for 'esr_class_str[0]')
> > > > arch/arm64/kernel/traps.c:705:22: warning: initialized field overwritten [-Woverride-init]
> > > > 705 | [ESR_ELx_EC_WFx] = "WFI/WFE",
> > > > | ^~~~~~~~~
> > > >
> > > > This is harmless since they are only informational strings,
> > > > but it's easy to change the code to ignore missing initialization
> > > > and instead warn about possible duplicate initializers.
> > >
> > > This has come up before, and IMO the warning is more hindrance than
> > > helpful, given the prevalance of spurious warnings, and the (again IMO)
> > > the rework needed to avoid those making the code harder to reason about
> >
> > FWIW in QEMU we turn the clang version of this off with
> > -Wno-initializer-overrides because we agree that the code is
> > fine and the compiler is being unhelpful in this case. (There's
> > a reason gcc doesn't put it in -Wall.)
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91688 is a request
> > for something that would catch bugs without breaking ranged-array
> > initializer syntax usage, but the gcc devs don't seem to have
> > responded.
>
> Yes, I'm inclined to agree. The code is fine, and "fixing" it just leads to
> churn and the possible introduction of bugs.

Ok, shall we just disable it for all of arch/arm64/kernel then?
The other parts of the kernel that follow the same line of thinking
are drivers/gpu/drm/amd/ and drivers/ata, for which I already
just turn them off. The rest of the kernel is mostly clean for
the warning, or there are actual bugs that it finds.

Arnd

Re: [PATCH 4/4] arm64: traps: fix -Woverride-init warnings [ In reply to ]

will at kernel

Oct 26, 2020, 2:15 PM

Post #7 of 7 (16 views)

Permalink

On Mon, Oct 26, 2020 at 08:28:45PM +0100, Arnd Bergmann wrote:
> On Mon, Oct 26, 2020 at 6:27 PM Will Deacon <will@kernel.org> wrote:
> > On Mon, Oct 26, 2020 at 05:13:30PM +0000, Peter Maydell wrote:
> > > On Mon, 26 Oct 2020 at 16:23, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > On Mon, Oct 26, 2020 at 05:03:31PM +0100, Arnd Bergmann wrote:
> > > > > From: Arnd Bergmann <arnd@arndb.de>
> > > > >
> > > > > There are many warnings in this file when we re-enable the
> > > > > Woverride-init flag:
> > > > >
> > > > > arch/arm64/kernel/traps.c:704:26: warning: initialized field overwritten [-Woverride-init]
> > > > > 704 | [ESR_ELx_EC_UNKNOWN] = "Unknown/Uncategorized",
> > > > > | ^~~~~~~~~~~~~~~~~~~~~~~
> > > > > arch/arm64/kernel/traps.c:704:26: note: (near initialization for 'esr_class_str[0]')
> > > > > arch/arm64/kernel/traps.c:705:22: warning: initialized field overwritten [-Woverride-init]
> > > > > 705 | [ESR_ELx_EC_WFx] = "WFI/WFE",
> > > > > | ^~~~~~~~~
> > > > >
> > > > > This is harmless since they are only informational strings,
> > > > > but it's easy to change the code to ignore missing initialization
> > > > > and instead warn about possible duplicate initializers.
> > > >
> > > > This has come up before, and IMO the warning is more hindrance than
> > > > helpful, given the prevalance of spurious warnings, and the (again IMO)
> > > > the rework needed to avoid those making the code harder to reason about
> > >
> > > FWIW in QEMU we turn the clang version of this off with
> > > -Wno-initializer-overrides because we agree that the code is
> > > fine and the compiler is being unhelpful in this case. (There's
> > > a reason gcc doesn't put it in -Wall.)
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91688 is a request
> > > for something that would catch bugs without breaking ranged-array
> > > initializer syntax usage, but the gcc devs don't seem to have
> > > responded.
> >
> > Yes, I'm inclined to agree. The code is fine, and "fixing" it just leads to
> > churn and the possible introduction of bugs.
>
> Ok, shall we just disable it for all of arch/arm64/kernel then?
> The other parts of the kernel that follow the same line of thinking
> are drivers/gpu/drm/amd/ and drivers/ata, for which I already
> just turn them off. The rest of the kernel is mostly clean for
> the warning, or there are actual bugs that it finds.

I'm happy to take a patch disabling it, either for arch/arm64/kernel or
arch/arm64 as a whole. Thanks!

Will