Re: [PATCH 6/6] qemu-io-cmds: Silent GCC9 format-overflow warning

[Richard Henderson]

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~