From: Arnd Bergmann <[email protected]>
On most architectures, gcc -Wextra warns about the list of error
numbers containing both EDEADLK and EDEADLOCK:
lib/errname.c:15:67: warning: initialized field overwritten [-Woverride-init]
15 | #define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err
| ^~~
lib/errname.c:172:2: note: in expansion of macro 'E'
172 | E(EDEADLK), /* EDEADLOCK */
| ^
lib/errname.c:15:67: note: (near initialization for 'names_0[35]')
15 | #define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err
| ^~~
lib/errname.c:172:2: note: in expansion of macro 'E'
172 | E(EDEADLK), /* EDEADLOCK */
| ^
Make that line conditional on the two values being distinct.
Fixes: 57f5677e535b ("printf: add support for printing symbolic error names")
Signed-off-by: Arnd Bergmann <[email protected]>
---
lib/errname.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/lib/errname.c b/lib/errname.c
index 0c4d3e66170e..6adff0bf2445 100644
--- a/lib/errname.c
+++ b/lib/errname.c
@@ -169,7 +169,9 @@ static const char *names_0[] = {
E(ECANCELED), /* ECANCELLED */
E(EAGAIN), /* EWOULDBLOCK */
E(ECONNREFUSED), /* EREFUSED */
+#if EDEADLK != EDEADLOCK
E(EDEADLK), /* EDEADLOCK */
+#endif
};
#undef E
--
2.27.0
On 26/10/2020 22.49, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> On most architectures, gcc -Wextra warns about the list of error
> numbers containing both EDEADLK and EDEADLOCK:
>
> lib/errname.c:15:67: warning: initialized field overwritten [-Woverride-init]
> 15 | #define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err
> | ^~~
> lib/errname.c:172:2: note: in expansion of macro 'E'
> 172 | E(EDEADLK), /* EDEADLOCK */
> | ^
> lib/errname.c:15:67: note: (near initialization for 'names_0[35]')
> 15 | #define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err
> | ^~~
> lib/errname.c:172:2: note: in expansion of macro 'E'
> 172 | E(EDEADLK), /* EDEADLOCK */
> | ^
>
> Make that line conditional on the two values being distinct.
>
NAK. That would end up using the "EDEADLOCK" string for the value 35 on
those architectures where they are the same, despite EDEADLK being the
by far the most used symbol. See the comments and original commit log,
the placement of these is deliberate.
How about we do this instead?
From: Rasmus Villemoes <[email protected]>
The table of errno value->name contains a few duplicate entries since
e.g. EDEADLK == EDEADLOCK on most architectures. For the known cases,
the most used symbolic constant is listed last so that takes
precedence - the idea being that if someone sees "can't do that:
-EDEADLK" in dmesg, grepping for EDEADLK is more likely to find the
place where that error was generated (grepping for "can't do that"
will find the printk() that emitted it, but the source would often be
a few calls down).
However, that means one gets
warning: initialized field overwritten [-Woverride-init]
when building with W=1. As the use of multiple initializers for the
same entry here is quite deliberate, explicitly disable that warning
for errname.o.
Reported-by: Arnd Bergmann <[email protected]>
Fixes: 57f5677e535b ("printf: add support for printing symbolic error
names")
Signed-off-by: Rasmus Villemoes <[email protected]>
---
lib/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/Makefile b/lib/Makefile
index ce45af50983a2a5e3582..a98119519e100103818d 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -224,6 +224,7 @@ obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
obj-$(CONFIG_DYNAMIC_DEBUG_CORE) += dynamic_debug.o
obj-$(CONFIG_SYMBOLIC_ERRNAME) += errname.o
+CFLAGS_errname.o += $(call cc-disable-warning, override-init)
obj-$(CONFIG_NLATTR) += nlattr.o
--
2.23.0
On 27/10/2020 08.23, Rasmus Villemoes wrote:
> How about we do this instead?
>
> From: Rasmus Villemoes <[email protected]>
Missed:
Subject: lib/errname.o: disable -Woverride-init
>
> The table of errno value->name contains a few duplicate entries since
> e.g. EDEADLK == EDEADLOCK on most architectures. For the known cases,
> the most used symbolic constant is listed last so that takes
> precedence - the idea being that if someone sees "can't do that:
> -EDEADLK" in dmesg, grepping for EDEADLK is more likely to find the
> place where that error was generated (grepping for "can't do that"
> will find the printk() that emitted it, but the source would often be
> a few calls down).
>
> However, that means one gets
>
> warning: initialized field overwritten [-Woverride-init]
>
> when building with W=1. As the use of multiple initializers for the
> same entry here is quite deliberate, explicitly disable that warning
> for errname.o.
>
> Reported-by: Arnd Bergmann <[email protected]>
> Fixes: 57f5677e535b ("printf: add support for printing symbolic error
> names")
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> lib/Makefile | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/lib/Makefile b/lib/Makefile
> index ce45af50983a2a5e3582..a98119519e100103818d 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -224,6 +224,7 @@ obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
>
> obj-$(CONFIG_DYNAMIC_DEBUG_CORE) += dynamic_debug.o
> obj-$(CONFIG_SYMBOLIC_ERRNAME) += errname.o
> +CFLAGS_errname.o += $(call cc-disable-warning, override-init)
>
> obj-$(CONFIG_NLATTR) += nlattr.o
>
--
Rasmus Villemoes
Software Developer
Prevas A/S
Hedeager 3
DK-8200 Aarhus N
+45 51210274
[email protected]
http://www.prevas.dk
Good morning Arnd,
On 10/26/20 10:49 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> On most architectures, gcc -Wextra warns about the list of error
> numbers containing both EDEADLK and EDEADLOCK:
>
> lib/errname.c:15:67: warning: initialized field overwritten [-Woverride-init]
> 15 | #define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err
> | ^~~
> lib/errname.c:172:2: note: in expansion of macro 'E'
> 172 | E(EDEADLK), /* EDEADLOCK */
> | ^
> lib/errname.c:15:67: note: (near initialization for 'names_0[35]')
> 15 | #define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err
> | ^~~
> lib/errname.c:172:2: note: in expansion of macro 'E'
> 172 | E(EDEADLK), /* EDEADLOCK */
> | ^
bad performance of gcc to warn twice about the same line and not
mentioning the line that has E(EDEADLOCK).
> Make that line conditional on the two values being distinct.
>
> Fixes: 57f5677e535b ("printf: add support for printing symbolic error names")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> lib/errname.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/lib/errname.c b/lib/errname.c
> index 0c4d3e66170e..6adff0bf2445 100644
> --- a/lib/errname.c
> +++ b/lib/errname.c
> @@ -169,7 +169,9 @@ static const char *names_0[] = {
> E(ECANCELED), /* ECANCELLED */
> E(EAGAIN), /* EWOULDBLOCK */
> E(ECONNREFUSED), /* EREFUSED */
> +#if EDEADLK != EDEADLOCK
> E(EDEADLK), /* EDEADLOCK */
> +#endif
The comments suggest that duplicates are expected. Would it make sense
to add similar conditions to the other three entries?
Best regards
Uwe
> };
> #undef E
>
On Tue, Oct 27, 2020 at 7:55 AM Uwe Kleine-König <[email protected]> wrote:
> On 10/26/20 10:49 PM, Arnd Bergmann wrote:
> > Make that line conditional on the two values being distinct.
> >
> > Fixes: 57f5677e535b ("printf: add support for printing symbolic error names")
> > Signed-off-by: Arnd Bergmann <[email protected]>
> > ---
> > lib/errname.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/lib/errname.c b/lib/errname.c
> > index 0c4d3e66170e..6adff0bf2445 100644
> > --- a/lib/errname.c
> > +++ b/lib/errname.c
> > @@ -169,7 +169,9 @@ static const char *names_0[] = {
> > E(ECANCELED), /* ECANCELLED */
> > E(EAGAIN), /* EWOULDBLOCK */
> > E(ECONNREFUSED), /* EREFUSED */
> > +#if EDEADLK != EDEADLOCK
> > E(EDEADLK), /* EDEADLOCK */
> > +#endif
>
> The comments suggest that duplicates are expected. Would it make sense
> to add similar conditions to the other three entries?
The other ones are always aliases, so there is no point in having
an #ifdef. The reason we need to handle EDEADLK separately
is that it's an alias on most architectures but not on on others,
specifically mips, powerpc and sparc.
Arnd
On Tue, Oct 27, 2020 at 8:23 AM Rasmus Villemoes
<[email protected]> wrote:
> On 26/10/2020 22.49, Arnd Bergmann wrote:
> > From: Arnd Bergmann <[email protected]>
>
> NAK. That would end up using the "EDEADLOCK" string for the value 35 on
> those architectures where they are the same, despite EDEADLK being the
> by far the most used symbol. See the comments and original commit log,
> the placement of these is deliberate.
Ok, I see.
> How about we do this instead?
>
> From: Rasmus Villemoes <[email protected]>
>
> The table of errno value->name contains a few duplicate entries since
> e.g. EDEADLK == EDEADLOCK on most architectures. For the known cases,
> the most used symbolic constant is listed last so that takes
> precedence - the idea being that if someone sees "can't do that:
> -EDEADLK" in dmesg, grepping for EDEADLK is more likely to find the
> place where that error was generated (grepping for "can't do that"
> will find the printk() that emitted it, but the source would often be
> a few calls down).
>
> However, that means one gets
>
> warning: initialized field overwritten [-Woverride-init]
>
> when building with W=1. As the use of multiple initializers for the
> same entry here is quite deliberate, explicitly disable that warning
> for errname.o.
>
> Reported-by: Arnd Bergmann <[email protected]>
> Fixes: 57f5677e535b ("printf: add support for printing symbolic error
> names")
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> lib/Makefile | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/lib/Makefile b/lib/Makefile
> index ce45af50983a2a5e3582..a98119519e100103818d 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -224,6 +224,7 @@ obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
>
> obj-$(CONFIG_DYNAMIC_DEBUG_CORE) += dynamic_debug.o
> obj-$(CONFIG_SYMBOLIC_ERRNAME) += errname.o
> +CFLAGS_errname.o += $(call cc-disable-warning, override-init)
>
This works, but it conflicts with a different patch series I have, and
it disables a potentially useful warning in case we get another conflict
in this file, so I'd prefer to find a way to avoid the warning rather
than force-disabling it.
How about adding the #ifdef around the EDEADLOCK line
instead of the EDEADLK one? Something like
diff --git a/lib/errname.c b/lib/errname.c
index 0c4d3e66170e..93043fb960cc 100644
--- a/lib/errname.c
+++ b/lib/errname.c
@@ -38,7 +38,10 @@ static const char *names_0[] = {
E(ECOMM),
E(ECONNABORTED),
E(ECONNRESET),
+ E(EDEADLK), /* EDEADLOCK */
+#if EDEADLK != EDEADLOCK /* mips, sparc, powerpc */
E(EDEADLOCK),
+#endif
E(EDESTADDRREQ),
E(EDOM),
E(EDOTDOT),
@@ -169,7 +172,6 @@ static const char *names_0[] = {
E(ECANCELED), /* ECANCELLED */
E(EAGAIN), /* EWOULDBLOCK */
E(ECONNREFUSED), /* EREFUSED */
- E(EDEADLK), /* EDEADLOCK */
};
#undef E
Arnd
On Tue 2020-10-27 09:46:28, Arnd Bergmann wrote:
> On Tue, Oct 27, 2020 at 8:23 AM Rasmus Villemoes
> <[email protected]> wrote:
> > On 26/10/2020 22.49, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <[email protected]>
> >
> > NAK. That would end up using the "EDEADLOCK" string for the value 35 on
> > those architectures where they are the same, despite EDEADLK being the
> > by far the most used symbol. See the comments and original commit log,
> > the placement of these is deliberate.
Good point.
> Ok, I see.
>
> > How about we do this instead?
> >
> > when building with W=1. As the use of multiple initializers for the
> > same entry here is quite deliberate, explicitly disable that warning
> > for errname.o.
> >
> > diff --git a/lib/Makefile b/lib/Makefile
> > index ce45af50983a2a5e3582..a98119519e100103818d 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -224,6 +224,7 @@ obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
> >
> > obj-$(CONFIG_DYNAMIC_DEBUG_CORE) += dynamic_debug.o
> > obj-$(CONFIG_SYMBOLIC_ERRNAME) += errname.o
> > +CFLAGS_errname.o += $(call cc-disable-warning, override-init)
> >
>
> This works, but it disables a potentially useful warning in case we
> get another conflict in this file, so I'd prefer to find a way to
> avoid the warning rather than force-disabling it.
Yeah, I think that it is better to explicitely disable the less used
variant in the code than hiding the double initialization. It will
be clear what is going on.
> How about adding the #ifdef around the EDEADLOCK line
> instead of the EDEADLK one? Something like
>
> diff --git a/lib/errname.c b/lib/errname.c
> index 0c4d3e66170e..93043fb960cc 100644
> --- a/lib/errname.c
> +++ b/lib/errname.c
> @@ -38,7 +38,10 @@ static const char *names_0[] = {
> E(ECOMM),
> E(ECONNABORTED),
> E(ECONNRESET),
> + E(EDEADLK), /* EDEADLOCK */
> +#if EDEADLK != EDEADLOCK /* mips, sparc, powerpc */
> E(EDEADLOCK),
> +#endif
> E(EDESTADDRREQ),
> E(EDOM),
> E(EDOTDOT),
> @@ -169,7 +172,6 @@ static const char *names_0[] = {
> E(ECANCELED), /* ECANCELLED */
> E(EAGAIN), /* EWOULDBLOCK */
> E(ECONNREFUSED), /* EREFUSED */
> - E(EDEADLK), /* EDEADLOCK */
This should stay :-)
And we should remove the ECANCELLED definition. It is always the same
as ECANCELED and replaced. We do not define EWOULDBLOCK and
EREFUSED either.
Best Regards,
Petr
On 27/10/2020 10.12, Petr Mladek wrote:
> On Tue 2020-10-27 09:46:28, Arnd Bergmann wrote:
>> On Tue, Oct 27, 2020 at 8:23 AM Rasmus Villemoes
>> <[email protected]> wrote:
>>> On 26/10/2020 22.49, Arnd Bergmann wrote:
>>>> From: Arnd Bergmann <[email protected]>
>>>
>>> NAK. That would end up using the "EDEADLOCK" string for the value 35 on
>>> those architectures where they are the same, despite EDEADLK being the
>>> by far the most used symbol. See the comments and original commit log,
>>> the placement of these is deliberate.
>
> Good point.
>
>> Ok, I see.
>>
>>> How about we do this instead?
>>>
>>> when building with W=1. As the use of multiple initializers for the
>>> same entry here is quite deliberate, explicitly disable that warning
>>> for errname.o.
>>>
>>> diff --git a/lib/Makefile b/lib/Makefile
>>> index ce45af50983a2a5e3582..a98119519e100103818d 100644
>>> --- a/lib/Makefile
>>> +++ b/lib/Makefile
>>> @@ -224,6 +224,7 @@ obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
>>>
>>> obj-$(CONFIG_DYNAMIC_DEBUG_CORE) += dynamic_debug.o
>>> obj-$(CONFIG_SYMBOLIC_ERRNAME) += errname.o
>>> +CFLAGS_errname.o += $(call cc-disable-warning, override-init)
>>>
>>
>> This works, but it disables a potentially useful warning in case we
>> get another conflict in this file, so I'd prefer to find a way to
>> avoid the warning rather than force-disabling it.
>
> Yeah, I think that it is better to explicitely disable the less used
> variant in the code than hiding the double initialization. It will
> be clear what is going on.
>
>
>> How about adding the #ifdef around the EDEADLOCK line
>> instead of the EDEADLK one? Something like
>>
>> diff --git a/lib/errname.c b/lib/errname.c
>> index 0c4d3e66170e..93043fb960cc 100644
>> --- a/lib/errname.c
>> +++ b/lib/errname.c
>> @@ -38,7 +38,10 @@ static const char *names_0[] = {
>> E(ECOMM),
>> E(ECONNABORTED),
>> E(ECONNRESET),
>> + E(EDEADLK), /* EDEADLOCK */
>> +#if EDEADLK != EDEADLOCK /* mips, sparc, powerpc */
>> E(EDEADLOCK),
>> +#endif
>> E(EDESTADDRREQ),
>> E(EDOM),
>> E(EDOTDOT),
>> @@ -169,7 +172,6 @@ static const char *names_0[] = {
>> E(ECANCELED), /* ECANCELLED */
>> E(EAGAIN), /* EWOULDBLOCK */
>> E(ECONNREFUSED), /* EREFUSED */
>> - E(EDEADLK), /* EDEADLOCK */
>
> This should stay :-)
>
No, Arnd moved it next to EDEADLOCK, which is fine (it can lose the
comment /* EDEADLOCK */, though; the comment on the ifdef is
sufficient). Especially when:
> And we should remove the ECANCELLED definition. It is always the same
> as ECANCELED and replaced. We do not define EWOULDBLOCK and
> EREFUSED either.
Yes, I'm not sure why I elided EWOULDBLOCK and EREFUSED but not
ECANCELLED. So let's move EAGAIN, ECONNREFUSED and ECANCELED to their
proper alphabetic place. But I also want to add a check that the things
we've elided match some value that we do handle. So add something like
#ifdef EREFUSED /* parisc */
static_assert(EREFUSED == ECONNREFUSED);
#endif
#ifdef ECANCELLED /* parisc */
static_assert(ECANCELLED == ECANCELED);
#endif
static_assert(EAGAIN == EWOULDBLOCK); /* everywhere */
so that if we ever import some arch that defines EREFUSED to something
other than ECONNREFUSED, it would be caught. Essentially, errname.c
should mention every #define E* that appears in any errno*.h.
Rasmus
On Tue 2020-10-27 11:55:56, Rasmus Villemoes wrote:
> On 27/10/2020 10.12, Petr Mladek wrote:
> > On Tue 2020-10-27 09:46:28, Arnd Bergmann wrote:
> >> On Tue, Oct 27, 2020 at 8:23 AM Rasmus Villemoes
> >> <[email protected]> wrote:
> >>> On 26/10/2020 22.49, Arnd Bergmann wrote:
> >>>> From: Arnd Bergmann <[email protected]>
> >>>
> >>> NAK. That would end up using the "EDEADLOCK" string for the value 35 on
> >>> those architectures where they are the same, despite EDEADLK being the
> >>> by far the most used symbol. See the comments and original commit log,
> >>> the placement of these is deliberate.
> >
> > Good point.
> >
> >> Ok, I see.
> >>
> >>> How about we do this instead?
> >>>
> >>> when building with W=1. As the use of multiple initializers for the
> >>> same entry here is quite deliberate, explicitly disable that warning
> >>> for errname.o.
> >>>
> >>> diff --git a/lib/Makefile b/lib/Makefile
> >>> index ce45af50983a2a5e3582..a98119519e100103818d 100644
> >>> --- a/lib/Makefile
> >>> +++ b/lib/Makefile
> >>> @@ -224,6 +224,7 @@ obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
> >>>
> >>> obj-$(CONFIG_DYNAMIC_DEBUG_CORE) += dynamic_debug.o
> >>> obj-$(CONFIG_SYMBOLIC_ERRNAME) += errname.o
> >>> +CFLAGS_errname.o += $(call cc-disable-warning, override-init)
> >>>
> >>
> >> This works, but it disables a potentially useful warning in case we
> >> get another conflict in this file, so I'd prefer to find a way to
> >> avoid the warning rather than force-disabling it.
> >
> > Yeah, I think that it is better to explicitely disable the less used
> > variant in the code than hiding the double initialization. It will
> > be clear what is going on.
> >
> >
> >> How about adding the #ifdef around the EDEADLOCK line
> >> instead of the EDEADLK one? Something like
> >>
> >> diff --git a/lib/errname.c b/lib/errname.c
> >> index 0c4d3e66170e..93043fb960cc 100644
> >> --- a/lib/errname.c
> >> +++ b/lib/errname.c
> >> @@ -38,7 +38,10 @@ static const char *names_0[] = {
> >> E(ECOMM),
> >> E(ECONNABORTED),
> >> E(ECONNRESET),
> >> + E(EDEADLK), /* EDEADLOCK */
> >> +#if EDEADLK != EDEADLOCK /* mips, sparc, powerpc */
> >> E(EDEADLOCK),
> >> +#endif
> >> E(EDESTADDRREQ),
> >> E(EDOM),
> >> E(EDOTDOT),
> >> @@ -169,7 +172,6 @@ static const char *names_0[] = {
> >> E(ECANCELED), /* ECANCELLED */
> >> E(EAGAIN), /* EWOULDBLOCK */
> >> E(ECONNREFUSED), /* EREFUSED */
> >> - E(EDEADLK), /* EDEADLOCK */
> >
> > This should stay :-)
> >
>
> No, Arnd moved it next to EDEADLOCK, which is fine (it can lose the
> comment /* EDEADLOCK */, though; the comment on the ifdef is
> sufficient). Especially when:
>
> > And we should remove the ECANCELLED definition. It is always the same
> > as ECANCELED and replaced. We do not define EWOULDBLOCK and
> > EREFUSED either.
>
> Yes, I'm not sure why I elided EWOULDBLOCK and EREFUSED but not
> ECANCELLED. So let's move EAGAIN, ECONNREFUSED and ECANCELED to their
> proper alphabetic place. But I also want to add a check that the things
> we've elided match some value that we do handle. So add something like
>
> #ifdef EREFUSED /* parisc */
> static_assert(EREFUSED == ECONNREFUSED);
> #endif
>
> #ifdef ECANCELLED /* parisc */
> static_assert(ECANCELLED == ECANCELED);
> #endif
>
> static_assert(EAGAIN == EWOULDBLOCK); /* everywhere */
>
> so that if we ever import some arch that defines EREFUSED to something
> other than ECONNREFUSED, it would be caught. Essentially, errname.c
> should mention every #define E* that appears in any errno*.h.
Sounds like a good plan.
Arnd, are you going to take care of this or should we clean up it ourself?
Best Regards,
Petr