| Message ID | 20191217173425.5082-7-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix more GCC9 -O3 warnings | expand |
On 12/17/19 7:34 AM, Philippe Mathieu-Daudé wrote: > GCC9 is confused when building with CFLAG -O3: > > In function ‘help_oneline’, > inlined from ‘help_all’ at qemu-io-cmds.c:2414:9, > inlined from ‘help_f’ at qemu-io-cmds.c:2424:9: > qemu-io-cmds.c:2389:9: error: ‘%s’ directive argument is null [-Werror=format-overflow=] > 2389 | printf("%s ", ct->name); > | ^~~~~~~~~~~~~~~~~~~~~~~ > > Audit shows this can't happen. Give a hint to GCC adding an > assert() call. This deserves more investigation. From my glance it appears you are right -- and moreover impossible for gcc to have come to this conclusion. Which begs the question of how that is. Did you file a gcc bug report? r~ > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > Cc: Kevin Wolf <kwolf@redhat.com> > Cc: Max Reitz <mreitz@redhat.com> > Cc: qemu-block@nongnu.org > --- > qemu-io-cmds.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > index 1b7e700020..9e956a5dd4 100644 > --- a/qemu-io-cmds.c > +++ b/qemu-io-cmds.c > @@ -2411,6 +2411,7 @@ static void help_all(void) > const cmdinfo_t *ct; > > for (ct = cmdtab; ct < &cmdtab[ncmds]; ct++) { > + assert(ct->name); > help_oneline(ct->name, ct); > } > printf("\nUse 'help commandname' for extended help.\n"); >
On 12/18/19 5:15 AM, Richard Henderson wrote: > On 12/17/19 7:34 AM, Philippe Mathieu-Daudé wrote: >> GCC9 is confused when building with CFLAG -O3: >> >> In function ‘help_oneline’, >> inlined from ‘help_all’ at qemu-io-cmds.c:2414:9, >> inlined from ‘help_f’ at qemu-io-cmds.c:2424:9: >> qemu-io-cmds.c:2389:9: error: ‘%s’ directive argument is null [-Werror=format-overflow=] >> 2389 | printf("%s ", ct->name); >> | ^~~~~~~~~~~~~~~~~~~~~~~ >> >> Audit shows this can't happen. Give a hint to GCC adding an >> assert() call. > > This deserves more investigation. From my glance it appears you are right -- > and moreover impossible for gcc to have come to this conclusion. Which begs > the question of how that is. New entries are added to cmdtab[] in qemuio_add_command() which is public (also called in qemu-io.c). So you can insert a cmdinfo_t with a name being NULL. This is why I thought GCC was correct first, and I tried: -- >8 -- --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -42,6 +42,7 @@ void qemuio_add_command(const cmdinfo_t *ci) * Catch it now rather than letting it manifest as a crash if a * particular set of command line options are used. */ + assert(ci->name); assert(ci->perm == 0 || (ci->flags & (CMD_FLAG_GLOBAL | CMD_NOFILE_OK)) == 0); cmdtab = g_renew(cmdinfo_t, cmdtab, ++ncmds); --- But this didn't fix the warning... So I moved the assert() in the other location. > > Did you file a gcc bug report? No because I'm not sure this is a bug, but if you confirm I'll file one :) >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> Cc: Kevin Wolf <kwolf@redhat.com> >> Cc: Max Reitz <mreitz@redhat.com> >> Cc: qemu-block@nongnu.org >> --- >> qemu-io-cmds.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c >> index 1b7e700020..9e956a5dd4 100644 >> --- a/qemu-io-cmds.c >> +++ b/qemu-io-cmds.c >> @@ -2411,6 +2411,7 @@ static void help_all(void) >> const cmdinfo_t *ct; >> >> for (ct = cmdtab; ct < &cmdtab[ncmds]; ct++) { >> + assert(ct->name); >> help_oneline(ct->name, ct); >> } >> printf("\nUse 'help commandname' for extended help.\n"); >> >
On 12/18/19 7:45 AM, Philippe Mathieu-Daudé wrote: > On 12/18/19 5:15 AM, Richard Henderson wrote: >> On 12/17/19 7:34 AM, Philippe Mathieu-Daudé wrote: >>> GCC9 is confused when building with CFLAG -O3: >>> >>> In function ‘help_oneline’, >>> inlined from ‘help_all’ at qemu-io-cmds.c:2414:9, >>> inlined from ‘help_f’ at qemu-io-cmds.c:2424:9: >>> qemu-io-cmds.c:2389:9: error: ‘%s’ directive argument is null >>> [-Werror=format-overflow=] >>> 2389 | printf("%s ", ct->name); >>> | ^~~~~~~~~~~~~~~~~~~~~~~ >>> >>> Audit shows this can't happen. Give a hint to GCC adding an >>> assert() call. >> >> This deserves more investigation. From my glance it appears you are right -- >> and moreover impossible for gcc to have come to this conclusion. Which begs >> the question of how that is. > > New entries are added to cmdtab[] in qemuio_add_command() which is public (also > called in qemu-io.c). So you can insert a cmdinfo_t with a name being NULL. > This is why I thought GCC was correct first, and I tried: > > -- >8 -- > --- a/qemu-io-cmds.c > +++ b/qemu-io-cmds.c > @@ -42,6 +42,7 @@ void qemuio_add_command(const cmdinfo_t *ci) > * Catch it now rather than letting it manifest as a crash if a > * particular set of command line options are used. > */ > + assert(ci->name); > assert(ci->perm == 0 || > (ci->flags & (CMD_FLAG_GLOBAL | CMD_NOFILE_OK)) == 0); > cmdtab = g_renew(cmdinfo_t, cmdtab, ++ncmds); > --- > > But this didn't fix the warning... So I moved the assert() in the other location. > >> >> Did you file a gcc bug report? > > No because I'm not sure this is a bug, but if you confirm I'll file one :) The error message is not saying the value *might* be null, but that it *is* null. That is, the compiler thinks that it has proven the value can be no other value than null. Can I assume that you've tested this particular code path, rather than merely removed the Werror? If the compiler really is "proving" that the value must be null, then the assert should be transformed to assert(false), and qemu will abort at runtime. In this case, the reason why the Werror went away is that the printf is removed as unreachable beforehand. But of course the value assigned in qemuio_add_command is truly variable, and since -O3 does not imply -flto the compiler cannot have proven to see all callers. So it follows that the compiler should not have proven that the value is null. So there *must* be a compiler bug. The only question is whether it is isolated to the printf warning or if it goes further into value propagation and wrong code generation. r~
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index 1b7e700020..9e956a5dd4 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -2411,6 +2411,7 @@ static void help_all(void) const cmdinfo_t *ct; for (ct = cmdtab; ct < &cmdtab[ncmds]; ct++) { + assert(ct->name); help_oneline(ct->name, ct); } printf("\nUse 'help commandname' for extended help.\n");
GCC9 is confused when building with CFLAG -O3: In function ‘help_oneline’, inlined from ‘help_all’ at qemu-io-cmds.c:2414:9, inlined from ‘help_f’ at qemu-io-cmds.c:2424:9: qemu-io-cmds.c:2389:9: error: ‘%s’ directive argument is null [-Werror=format-overflow=] 2389 | printf("%s ", ct->name); | ^~~~~~~~~~~~~~~~~~~~~~~ Audit shows this can't happen. Give a hint to GCC adding an assert() call. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- Cc: Kevin Wolf <kwolf@redhat.com> Cc: Max Reitz <mreitz@redhat.com> Cc: qemu-block@nongnu.org --- qemu-io-cmds.c | 1 + 1 file changed, 1 insertion(+)