Messages in this thread | | | Date | Tue, 27 Oct 2020 13:03:30 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH] printf: fix Woverride-init warning for EDEADLK errno |
| |
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
| |