* [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-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-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 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).