| Message ID | 20210122172133.2074177-1-f4bug@amsat.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libc: Add missing fallthrough annotation | expand |
On Fri, Jan 22, 2021 at 6:24 PM Thomas Huth <thuth@redhat.com> wrote: > On 22/01/2021 18.21, Philippe Mathieu-Daudé wrote: > > Silence warning when cross-compiling with GCC on Debian 10: > > > > $ s390x-linux-gnu-gcc --version > > s390x-linux-gnu-gcc (Debian 8.3.0-2) 8.3.0 > > > > lib/libc/stdio/vsnprintf.c: In function 'print_format': > > lib/libc/stdio/vsnprintf.c:165:11: warning: this statement may fall through [-Wimplicit-fallthrough=] > > Is -Wimplicit-fallthrough enabled by default there? Or did you enable it? I am using defaults. I just checked and this also happens with QEMU CI: https://travis-ci.org/github/qemu/qemu/jobs/755671724#L1830 > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > lib/libc/stdio/vsnprintf.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/lib/libc/stdio/vsnprintf.c b/lib/libc/stdio/vsnprintf.c > > index 21dd04d..f292dd8 100644 > > --- a/lib/libc/stdio/vsnprintf.c > > +++ b/lib/libc/stdio/vsnprintf.c > > @@ -163,6 +163,7 @@ print_format(char **buffer, size_t bufsize, const char *format, void *var) > > break; > > case 'X': > > upper = true; > > + /* fall through */ > > case 'x': > > sizec[i] = '\0'; > > value = (unsigned long) var & convert[length_mod]; > > > > Reviewed-by: Thomas Huth <thuth@redhat.com> Thanks! Phil.
On Sun, Jan 24, 2021 at 07:18:33AM +0100, Thomas Huth wrote: > On 24/01/2021 07.00, Alexey Kardashevskiy wrote: > >On 23/01/2021 10:34, Segher Boessenkool wrote: > >>On Fri, Jan 22, 2021 at 06:24:20PM +0100, Thomas Huth wrote: > >>>Is -Wimplicit-fallthrough enabled by default there? Or did you enable it? > >> > >>It is enabled by -W (aka -Wextra). Everyone should always use that > >>option (along with -Wall), but I do not know if SLOF does. > > > >SLOF uses -Wall but not -Wextra which produces lot more warnings than this > >patch is fixing. > > I've tried it in a couple of projects already, but IMHO enabling -Wextra is > a bad idea. It is a lot of work if you enable it on a bigger existing codebase. It is a lot less work if you enable it right from the start. I recommend -Wall -W -Wmissing-declarations -Wformat=2 for all new code. > With new versions of GCC, they often added "experimental" > warnings with -Wextra which were just wrong in many cases. Like? I don't remember any noisy warnings added to -Wextra in the last few years. The most annoying one is -Wsign-compare, but that one pretty much always points to real bugs (*future* bugs, the worst kind!) > It also enables > some rather annoying than helpful flags like -Wunused-parameter, so I'd > suggest that we rather only add the helpful flags like > -Wimplicit-fallthrough manually instead. You can disable just one warning if you think it is annoying (just use -Wno-unused-parameter in this case). But all warnings in -Wextra are easy to avoid, and all of them point to potentially serious problems (that is the requirement for putting a warning in -Wextra; for -Wall it is the same but more so). Segher
On Sun, Jan 24, 2021 at 7:05 AM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > On 23/01/2021 04:21, Philippe Mathieu-Daudé wrote: > > Silence warning when cross-compiling with GCC on Debian 10: > > > > $ s390x-linux-gnu-gcc --version > > s390x-linux-gnu-gcc (Debian 8.3.0-2) 8.3.0 > > > > lib/libc/stdio/vsnprintf.c: In function 'print_format': > > lib/libc/stdio/vsnprintf.c:165:11: warning: this statement may fall through [-Wimplicit-fallthrough=] > > upper = true; > > ~~~~~~^~~~~~ > > lib/libc/stdio/vsnprintf.c:166:4: note: here > > case 'x': > > ^~~~ > > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > How did you trigger this exactly? If it is s390's qemu flags as Thomas > says, then i am going to nack this and request a patch fixing all > -Wextra and not just some reused bits, or wait until i do this myself :) I haven't set any flag, and I don't think QEMU flags are passed because QEMU sets convert warnings to errors. Host is Ubuntu 18.04. Clang doesn't emit warning, GCC does. - gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0 - clang version 6.0.0-1ubuntu2 (tags/RELEASE_600/final) https://travis-ci.org/github/qemu/qemu/jobs/755867532 > And there is a question if i really have to update the slof.bin which i > do not expect to change in noticeable way. hmmm. What would David say? > > > > --- > > lib/libc/stdio/vsnprintf.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/lib/libc/stdio/vsnprintf.c b/lib/libc/stdio/vsnprintf.c > > index 21dd04d..f292dd8 100644 > > --- a/lib/libc/stdio/vsnprintf.c > > +++ b/lib/libc/stdio/vsnprintf.c > > @@ -163,6 +163,7 @@ print_format(char **buffer, size_t bufsize, const char *format, void *var) > > break; > > case 'X': > > upper = true; > > + /* fall through */ > > case 'x': > > sizec[i] = '\0'; > > value = (unsigned long) var & convert[length_mod]; > > > > -- > Alexey
On 24/01/2021 17.27, Philippe Mathieu-Daudé wrote: > On Sun, Jan 24, 2021 at 7:05 AM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >> On 23/01/2021 04:21, Philippe Mathieu-Daudé wrote: >>> Silence warning when cross-compiling with GCC on Debian 10: >>> >>> $ s390x-linux-gnu-gcc --version >>> s390x-linux-gnu-gcc (Debian 8.3.0-2) 8.3.0 >>> >>> lib/libc/stdio/vsnprintf.c: In function 'print_format': >>> lib/libc/stdio/vsnprintf.c:165:11: warning: this statement may fall through [-Wimplicit-fallthrough=] >>> upper = true; >>> ~~~~~~^~~~~~ >>> lib/libc/stdio/vsnprintf.c:166:4: note: here >>> case 'x': >>> ^~~~ >>> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> >> >> How did you trigger this exactly? If it is s390's qemu flags as Thomas >> says, then i am going to nack this and request a patch fixing all >> -Wextra and not just some reused bits, or wait until i do this myself :) > > I haven't set any flag, and I don't think QEMU flags are passed because > QEMU sets convert warnings to errors. See pc-bios/s390-ccw/Makefile in the QEMU sources: QEMU_CFLAGS := -Wall $(filter -W%, $(QEMU_CFLAGS)) Thus it's picking up the warnings from QEMU_CFLAGS. Not sure about -Werror, though, seems like the QEMU build system now handles this flag in a different spot. Thomas
On 24/01/2021 09.57, Segher Boessenkool wrote: > On Sun, Jan 24, 2021 at 07:18:33AM +0100, Thomas Huth wrote: >> On 24/01/2021 07.00, Alexey Kardashevskiy wrote: >>> On 23/01/2021 10:34, Segher Boessenkool wrote: >>>> On Fri, Jan 22, 2021 at 06:24:20PM +0100, Thomas Huth wrote: >>>>> Is -Wimplicit-fallthrough enabled by default there? Or did you enable it? >>>> >>>> It is enabled by -W (aka -Wextra). Everyone should always use that >>>> option (along with -Wall), but I do not know if SLOF does. >>> >>> SLOF uses -Wall but not -Wextra which produces lot more warnings than this >>> patch is fixing. >> >> I've tried it in a couple of projects already, but IMHO enabling -Wextra is >> a bad idea. > > It is a lot of work if you enable it on a bigger existing codebase. It > is a lot less work if you enable it right from the start. I recommend > -Wall -W -Wmissing-declarations -Wformat=2 for all new code. > >> With new versions of GCC, they often added "experimental" >> warnings with -Wextra which were just wrong in many cases. > > Like? I don't remember any noisy warnings added to -Wextra in the last > few years. It's been quite a while since I made that experience, so I can't remember the details (it was likely in the days of GCC 3.x or the early 4.x versions, maybe the first incarnation of -Wempty-body or so) and I've been avoiding -Wextra since that point in time. Well, maybe it got better in recent years and I should give it a try again... Thomas
On Mon, Jan 25, 2021 at 08:01:43AM +0100, Thomas Huth wrote: > On 24/01/2021 09.57, Segher Boessenkool wrote: > >>With new versions of GCC, they often added "experimental" > >>warnings with -Wextra which were just wrong in many cases. > > > >Like? I don't remember any noisy warnings added to -Wextra in the last > >few years. > > It's been quite a while since I made that experience, so I can't remember > the details (it was likely in the days of GCC 3.x or the early 4.x > versions, Ah, 15..20 years ago :-) > maybe the first incarnation of -Wempty-body or so) and I've been > avoiding -Wextra since that point in time. Well, maybe it got better in > recent years and I should give it a try again... I'm interested in hearing the results! My main advice is if you do not like some warnings to disable *those*, instead of only enabling the few you found out you like. Secondly, if some warning is annoying, there probably is a good reason it is in -W nonetheless! Segher
diff --git a/lib/libc/stdio/vsnprintf.c b/lib/libc/stdio/vsnprintf.c index 21dd04d..f292dd8 100644 --- a/lib/libc/stdio/vsnprintf.c +++ b/lib/libc/stdio/vsnprintf.c @@ -163,6 +163,7 @@ print_format(char **buffer, size_t bufsize, const char *format, void *var) break; case 'X': upper = true; + /* fall through */ case 'x': sizec[i] = '\0'; value = (unsigned long) var & convert[length_mod];
Silence warning when cross-compiling with GCC on Debian 10: $ s390x-linux-gnu-gcc --version s390x-linux-gnu-gcc (Debian 8.3.0-2) 8.3.0 lib/libc/stdio/vsnprintf.c: In function 'print_format': lib/libc/stdio/vsnprintf.c:165:11: warning: this statement may fall through [-Wimplicit-fallthrough=] upper = true; ~~~~~~^~~~~~ lib/libc/stdio/vsnprintf.c:166:4: note: here case 'x': ^~~~ Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- lib/libc/stdio/vsnprintf.c | 1 + 1 file changed, 1 insertion(+)