Messages in this thread | | | Subject | Re: [PATCH] printf: fix Woverride-init warning for EDEADLK errno | From | Rasmus Villemoes <> | Date | Tue, 27 Oct 2020 11:55:56 +0100 |
| |
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
| |