linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] printf: fix Woverride-init warning for EDEADLK errno
@ 2020-10-26 21:49 Arnd Bergmann
  2020-10-27  6:55 ` Uwe Kleine-König
  2020-10-27  7:23 ` Rasmus Villemoes
  0 siblings, 2 replies; 9+ messages in thread
From: Arnd Bergmann @ 2020-10-26 21:49 UTC (permalink / raw)
  To: Andrew Morton, Petr Mladek, Uwe Kleine-König,
	Rasmus Villemoes, Andy Shevchenko
  Cc: Arnd Bergmann, linux-kernel

From: Arnd Bergmann <arnd@arndb.de>

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 <arnd@arndb.de>
---
 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] printf: fix Woverride-init warning for EDEADLK errno
  2020-10-26 21:49 [PATCH] printf: fix Woverride-init warning for EDEADLK errno Arnd Bergmann
@ 2020-10-27  6:55 ` Uwe Kleine-König
  2020-10-27  8:41   ` Arnd Bergmann
  2020-10-27  7:23 ` Rasmus Villemoes
  1 sibling, 1 reply; 9+ messages in thread
From: Uwe Kleine-König @ 2020-10-27  6:55 UTC (permalink / raw)
  To: Arnd Bergmann, Andrew Morton, Petr Mladek, Rasmus Villemoes,
	Andy Shevchenko
  Cc: Arnd Bergmann, linux-kernel


[-- Attachment #1.1: Type: text/plain, Size: 1871 bytes --]

Good morning Arnd,

On 10/26/20 10:49 PM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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 <arnd@arndb.de>
> ---
>  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
>  



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] printf: fix Woverride-init warning for EDEADLK errno
  2020-10-26 21:49 [PATCH] printf: fix Woverride-init warning for EDEADLK errno Arnd Bergmann
  2020-10-27  6:55 ` Uwe Kleine-König
@ 2020-10-27  7:23 ` Rasmus Villemoes
  2020-10-27  7:25   ` Rasmus Villemoes
  2020-10-27  8:46   ` Arnd Bergmann
  1 sibling, 2 replies; 9+ messages in thread
From: Rasmus Villemoes @ 2020-10-27  7:23 UTC (permalink / raw)
  To: Arnd Bergmann, Andrew Morton, Petr Mladek, Uwe Kleine-König,
	Rasmus Villemoes, Andy Shevchenko
  Cc: Arnd Bergmann, linux-kernel

On 26/10/2020 22.49, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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 <linux@rasmusvillemoes.dk>

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 <arnd@kernel.org>
Fixes: 57f5677e535b ("printf: add support for printing symbolic error
names")
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] printf: fix Woverride-init warning for EDEADLK errno
  2020-10-27  7:23 ` Rasmus Villemoes
@ 2020-10-27  7:25   ` Rasmus Villemoes
  2020-10-27  8:46   ` Arnd Bergmann
  1 sibling, 0 replies; 9+ messages in thread
From: Rasmus Villemoes @ 2020-10-27  7:25 UTC (permalink / raw)
  To: Rasmus Villemoes, Arnd Bergmann, Andrew Morton, Petr Mladek,
	Uwe Kleine-König, Andy Shevchenko
  Cc: Arnd Bergmann, linux-kernel

On 27/10/2020 08.23, Rasmus Villemoes wrote:

> How about we do this instead?
> 
> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>

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 <arnd@kernel.org>
> Fixes: 57f5677e535b ("printf: add support for printing symbolic error
> names")
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  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
rasmus.villemoes@prevas.dk
www.prevas.dk

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] printf: fix Woverride-init warning for EDEADLK errno
  2020-10-27  6:55 ` Uwe Kleine-König
@ 2020-10-27  8:41   ` Arnd Bergmann
  0 siblings, 0 replies; 9+ messages in thread
From: Arnd Bergmann @ 2020-10-27  8:41 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Andrew Morton, Petr Mladek, Rasmus Villemoes, Andy Shevchenko,
	linux-kernel

On Tue, Oct 27, 2020 at 7:55 AM Uwe Kleine-König <uwe@kleine-koenig.org> 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 <arnd@arndb.de>
> > ---
> >  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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] printf: fix Woverride-init warning for EDEADLK errno
  2020-10-27  7:23 ` Rasmus Villemoes
  2020-10-27  7:25   ` Rasmus Villemoes
@ 2020-10-27  8:46   ` Arnd Bergmann
  2020-10-27  9:12     ` Petr Mladek
  1 sibling, 1 reply; 9+ messages in thread
From: Arnd Bergmann @ 2020-10-27  8:46 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Andrew Morton, Petr Mladek, Uwe Kleine-König,
	Andy Shevchenko, linux-kernel

On Tue, Oct 27, 2020 at 8:23 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On 26/10/2020 22.49, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
>
> 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 <linux@rasmusvillemoes.dk>
>
> 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 <arnd@kernel.org>
> Fixes: 57f5677e535b ("printf: add support for printing symbolic error
> names")
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] printf: fix Woverride-init warning for EDEADLK errno
  2020-10-27  8:46   ` Arnd Bergmann
@ 2020-10-27  9:12     ` Petr Mladek
  2020-10-27 10:55       ` Rasmus Villemoes
  0 siblings, 1 reply; 9+ messages in thread
From: Petr Mladek @ 2020-10-27  9:12 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Rasmus Villemoes, Andrew Morton, Uwe Kleine-König,
	Andy Shevchenko, linux-kernel

On Tue 2020-10-27 09:46:28, Arnd Bergmann wrote:
> On Tue, Oct 27, 2020 at 8:23 AM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> > On 26/10/2020 22.49, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> >
> > 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] printf: fix Woverride-init warning for EDEADLK errno
  2020-10-27  9:12     ` Petr Mladek
@ 2020-10-27 10:55       ` Rasmus Villemoes
  2020-10-27 12:03         ` Petr Mladek
  0 siblings, 1 reply; 9+ messages in thread
From: Rasmus Villemoes @ 2020-10-27 10:55 UTC (permalink / raw)
  To: Petr Mladek, Arnd Bergmann
  Cc: Andrew Morton, Uwe Kleine-König, Andy Shevchenko, linux-kernel

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
>> <linux@rasmusvillemoes.dk> wrote:
>>> On 26/10/2020 22.49, Arnd Bergmann wrote:
>>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] printf: fix Woverride-init warning for EDEADLK errno
  2020-10-27 10:55       ` Rasmus Villemoes
@ 2020-10-27 12:03         ` Petr Mladek
  0 siblings, 0 replies; 9+ messages in thread
From: Petr Mladek @ 2020-10-27 12:03 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Arnd Bergmann, Andrew Morton, Uwe Kleine-König,
	Andy Shevchenko, linux-kernel

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
> >> <linux@rasmusvillemoes.dk> wrote:
> >>> On 26/10/2020 22.49, Arnd Bergmann wrote:
> >>>> From: Arnd Bergmann <arnd@arndb.de>
> >>>
> >>> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-10-27 12:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-26 21:49 [PATCH] printf: fix Woverride-init warning for EDEADLK errno Arnd Bergmann
2020-10-27  6:55 ` Uwe Kleine-König
2020-10-27  8:41   ` Arnd Bergmann
2020-10-27  7:23 ` Rasmus Villemoes
2020-10-27  7:25   ` Rasmus Villemoes
2020-10-27  8:46   ` Arnd Bergmann
2020-10-27  9:12     ` Petr Mladek
2020-10-27 10:55       ` Rasmus Villemoes
2020-10-27 12:03         ` Petr Mladek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).