Patchwork Suppress warning: zero-length gnu_printf format string

login
register
mail settings
Submitter Markus Armbruster
Date Oct. 11, 2010, 12:52 p.m.
Message ID <m3zkuksxgv.fsf@blackfin.pond.sub.org>
Download mbox | patch
Permalink /patch/67421/
State New
Headers show

Comments

Markus Armbruster - Oct. 11, 2010, 12:52 p.m.
Warns about this line in check-qjson.c:
    QObject *obj = qobject_from_json("");

The obvious fix (add -Wno-format-zero-length to gcc_flags) doesn't
work, because -Wall switches it on again.  Fix by putting configured
flags last.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 configure |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
Paolo Bonzini - Oct. 11, 2010, 1:09 p.m.
On 10/11/2010 02:52 PM, Markus Armbruster wrote:
> Warns about this line in check-qjson.c:
>      QObject *obj = qobject_from_json("");
>
> The obvious fix (add -Wno-format-zero-length to gcc_flags) doesn't
> work, because -Wall switches it on again.  Fix by putting configured
> flags last.
>
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
> ---
>   configure |    5 +++--
>   1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/configure b/configure
> index d303061..3a12f92 100755
> --- a/configure
> +++ b/configure
> @@ -146,7 +146,8 @@ QEMU_CFLAGS="-I. -I\$(SRC_PATH) $QEMU_CFLAGS"
>   LDFLAGS="-g $LDFLAGS"
>
>   gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
> -gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags"
> +gcc_flags="-Wformat-security -Wformat-y2k -Wno-format-zero-length $gcc_flags"
> +gcc_flags="-Winit-self -Wignored-qualifiers $gcc_flags"
>   gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
>   gcc_flags="-fstack-protector-all $gcc_flags"
>   cat>  $TMPC<<  EOF
> @@ -154,7 +155,7 @@ int main(void) { return 0; }
>   EOF
>   for flag in $gcc_flags; do
>       if compile_prog "-Werror $QEMU_CFLAGS" "-Werror $flag" ; then
> -	QEMU_CFLAGS="$flag $QEMU_CFLAGS"
> +	QEMU_CFLAGS="$QEMU_CFLAGS $flag"
>       fi
>   done
>

Acked-By: Paolo Bonzini <pbonzini@redhat.com>

Paolo
Blue Swirl - Oct. 12, 2010, 5:35 p.m.
On Mon, Oct 11, 2010 at 12:52 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Warns about this line in check-qjson.c:
>    QObject *obj = qobject_from_json("");
>
> The obvious fix (add -Wno-format-zero-length to gcc_flags) doesn't
> work, because -Wall switches it on again.  Fix by putting configured
> flags last.

This would disable the flag globally. I'd rather disable the flag only
for check-qjson.o or more generically, remove -Werror for checks. For
example, there could be a check for how we handle invalid formats and
then the sources would contain format strings that annoy GCC, but we
wouldn't want warnings from that to stop the build.
Markus Armbruster - Oct. 13, 2010, 7:19 a.m.
Blue Swirl <blauwirbel@gmail.com> writes:

> On Mon, Oct 11, 2010 at 12:52 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Warns about this line in check-qjson.c:
>>    QObject *obj = qobject_from_json("");
>>
>> The obvious fix (add -Wno-format-zero-length to gcc_flags) doesn't
>> work, because -Wall switches it on again.  Fix by putting configured
>> flags last.
>
> This would disable the flag globally. I'd rather disable the flag only
> for check-qjson.o

Is this warning worth the hassle?  What's the problem with empty format
strings?

>                   or more generically, remove -Werror for checks. For
> example, there could be a check for how we handle invalid formats and
> then the sources would contain format strings that annoy GCC, but we
> wouldn't want warnings from that to stop the build.

If you want to go that extra mile, feel free.  For me, --disable-werror
has been good enough.

Regardless, I think we need the second patch hunk, to prevent -Wall from
trampling over $gcc_flags.
Blue Swirl - Oct. 13, 2010, 6:57 p.m.
On Wed, Oct 13, 2010 at 7:19 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>> On Mon, Oct 11, 2010 at 12:52 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Warns about this line in check-qjson.c:
>>>    QObject *obj = qobject_from_json("");
>>>
>>> The obvious fix (add -Wno-format-zero-length to gcc_flags) doesn't
>>> work, because -Wall switches it on again.  Fix by putting configured
>>> flags last.
>>
>> This would disable the flag globally. I'd rather disable the flag only
>> for check-qjson.o
>
> Is this warning worth the hassle?  What's the problem with empty format
> strings?

Your fix solves this specific case, but it also degrades the gcc
checks of the mainstream code (slightly). I think the test suite need
not follow the level of checking that should be applied to mainstream,
or at least the warnings there should not be fatal.

>>                   or more generically, remove -Werror for checks. For
>> example, there could be a check for how we handle invalid formats and
>> then the sources would contain format strings that annoy GCC, but we
>> wouldn't want warnings from that to stop the build.
>
> If you want to go that extra mile, feel free.  For me, --disable-werror
> has been good enough.

In that case, we could ignore the warning, since the only reporter is
able to use a workaround ;-).

> Regardless, I think we need the second patch hunk, to prevent -Wall from
> trampling over $gcc_flags.

I'm fine with that part.
Markus Armbruster - Oct. 14, 2010, 9:20 a.m.
Blue Swirl <blauwirbel@gmail.com> writes:

> On Wed, Oct 13, 2010 at 7:19 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Blue Swirl <blauwirbel@gmail.com> writes:
>>
>>> On Mon, Oct 11, 2010 at 12:52 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Warns about this line in check-qjson.c:
>>>>    QObject *obj = qobject_from_json("");
>>>>
>>>> The obvious fix (add -Wno-format-zero-length to gcc_flags) doesn't
>>>> work, because -Wall switches it on again.  Fix by putting configured
>>>> flags last.
>>>
>>> This would disable the flag globally. I'd rather disable the flag only
>>> for check-qjson.o
>>
>> Is this warning worth the hassle?  What's the problem with empty format
>> strings?
>
> Your fix solves this specific case, but it also degrades the gcc
> checks of the mainstream code (slightly). I think the test suite need
> not follow the level of checking that should be applied to mainstream,
> or at least the warnings there should not be fatal.

"Degrade" implies we miss something that's "wrong" enough to be worth
avoiding.  What's wrong with empty format strings?

>>>                   or more generically, remove -Werror for checks. For
>>> example, there could be a check for how we handle invalid formats and
>>> then the sources would contain format strings that annoy GCC, but we
>>> wouldn't want warnings from that to stop the build.
>>
>> If you want to go that extra mile, feel free.  For me, --disable-werror
>> has been good enough.
>
> In that case, we could ignore the warning, since the only reporter is
> able to use a workaround ;-).
>
>> Regardless, I think we need the second patch hunk, to prevent -Wall from
>> trampling over $gcc_flags.
>
> I'm fine with that part.

Reposted separately.
Blue Swirl - Oct. 14, 2010, 4:38 p.m.
On Thu, Oct 14, 2010 at 9:20 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>> On Wed, Oct 13, 2010 at 7:19 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Blue Swirl <blauwirbel@gmail.com> writes:
>>>
>>>> On Mon, Oct 11, 2010 at 12:52 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> Warns about this line in check-qjson.c:
>>>>>    QObject *obj = qobject_from_json("");
>>>>>
>>>>> The obvious fix (add -Wno-format-zero-length to gcc_flags) doesn't
>>>>> work, because -Wall switches it on again.  Fix by putting configured
>>>>> flags last.
>>>>
>>>> This would disable the flag globally. I'd rather disable the flag only
>>>> for check-qjson.o
>>>
>>> Is this warning worth the hassle?  What's the problem with empty format
>>> strings?
>>
>> Your fix solves this specific case, but it also degrades the gcc
>> checks of the mainstream code (slightly). I think the test suite need
>> not follow the level of checking that should be applied to mainstream,
>> or at least the warnings there should not be fatal.
>
> "Degrade" implies we miss something that's "wrong" enough to be worth
> avoiding.  What's wrong with empty format strings?

They generate useless calls to the formatting function, wasting
performance (slightly). Since there are no calls currently, this is of
course hypothetical.

We don't want those to appear in QEMU, but for test suite they may be
acceptable (for example, to test empty format string handling) because
test suite is not performance critical.
Paolo Bonzini - Oct. 14, 2010, 5:52 p.m.
On 10/14/2010 06:38 PM, Blue Swirl wrote:
> On Thu, Oct 14, 2010 at 9:20 AM, Markus Armbruster<armbru@redhat.com>  wrote:
>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>
>>> On Wed, Oct 13, 2010 at 7:19 AM, Markus Armbruster<armbru@redhat.com>  wrote:
>>>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>>>
>>>>> On Mon, Oct 11, 2010 at 12:52 PM, Markus Armbruster<armbru@redhat.com>  wrote:
>>>>>> Warns about this line in check-qjson.c:
>>>>>>     QObject *obj = qobject_from_json("");
>>>>>>
>>>>>> The obvious fix (add -Wno-format-zero-length to gcc_flags) doesn't
>>>>>> work, because -Wall switches it on again.  Fix by putting configured
>>>>>> flags last.
>>>>>
>>>>> This would disable the flag globally. I'd rather disable the flag only
>>>>> for check-qjson.o
>>>>
>>>> Is this warning worth the hassle?  What's the problem with empty format
>>>> strings?
>>>
>>> Your fix solves this specific case, but it also degrades the gcc
>>> checks of the mainstream code (slightly). I think the test suite need
>>> not follow the level of checking that should be applied to mainstream,
>>> or at least the warnings there should not be fatal.
>>
>> "Degrade" implies we miss something that's "wrong" enough to be worth
>> avoiding.  What's wrong with empty format strings?
>
> They generate useless calls to the formatting function, wasting
> performance (slightly). Since there are no calls currently, this is of
> course hypothetical.

It is even more hypothetical when empty-format printfs are optimized 
away by GCC:

$ gcc -x c - -O2 -S -o -
#include <stdio.h>
main() { printf (""); }

	.file	""
	.text
	.p2align 4,,15
.globl main
	.type	main, @function
main:
.LFB11:
	.cfi_startproc
	rep
	ret
	.cfi_endproc

and other attribute-printf-marked functions are probably not noop when 
the format argument is empty.

Paolo
Blue Swirl - Oct. 14, 2010, 5:59 p.m.
On Thu, Oct 14, 2010 at 5:52 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 10/14/2010 06:38 PM, Blue Swirl wrote:
>>
>> On Thu, Oct 14, 2010 at 9:20 AM, Markus Armbruster<armbru@redhat.com>
>>  wrote:
>>>
>>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>>
>>>> On Wed, Oct 13, 2010 at 7:19 AM, Markus Armbruster<armbru@redhat.com>
>>>>  wrote:
>>>>>
>>>>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>>>>
>>>>>> On Mon, Oct 11, 2010 at 12:52 PM, Markus Armbruster<armbru@redhat.com>
>>>>>>  wrote:
>>>>>>>
>>>>>>> Warns about this line in check-qjson.c:
>>>>>>>    QObject *obj = qobject_from_json("");
>>>>>>>
>>>>>>> The obvious fix (add -Wno-format-zero-length to gcc_flags) doesn't
>>>>>>> work, because -Wall switches it on again.  Fix by putting configured
>>>>>>> flags last.
>>>>>>
>>>>>> This would disable the flag globally. I'd rather disable the flag only
>>>>>> for check-qjson.o
>>>>>
>>>>> Is this warning worth the hassle?  What's the problem with empty format
>>>>> strings?
>>>>
>>>> Your fix solves this specific case, but it also degrades the gcc
>>>> checks of the mainstream code (slightly). I think the test suite need
>>>> not follow the level of checking that should be applied to mainstream,
>>>> or at least the warnings there should not be fatal.
>>>
>>> "Degrade" implies we miss something that's "wrong" enough to be worth
>>> avoiding.  What's wrong with empty format strings?
>>
>> They generate useless calls to the formatting function, wasting
>> performance (slightly). Since there are no calls currently, this is of
>> course hypothetical.
>
> It is even more hypothetical when empty-format printfs are optimized away by
> GCC:
>
> $ gcc -x c - -O2 -S -o -
> #include <stdio.h>
> main() { printf (""); }
>
>        .file   ""
>        .text
>        .p2align 4,,15
> .globl main
>        .type   main, @function
> main:
> .LFB11:
>        .cfi_startproc
>        rep
>        ret
>        .cfi_endproc
>
> and other attribute-printf-marked functions are probably not noop when the
> format argument is empty.

How is that? Does the warning message from qobject_from_json("") mean
that GCC may optimize that call away?
Paolo Bonzini - Oct. 15, 2010, 1:33 a.m.
On 10/14/2010 07:59 PM, Blue Swirl wrote:
>> It is even more hypothetical when empty-format printfs are optimized away by
>> GCC:
>>
>> $ gcc -x c - -O2 -S -o -
>> #include<stdio.h>
>> main() { printf (""); }
>>
>>         .file   ""
>>         .text
>>         .p2align 4,,15
>> .globl main
>>         .type   main, @function
>> main:
>> .LFB11:
>>         .cfi_startproc
>>         rep
>>         ret
>>         .cfi_endproc
>>
>> and other attribute-printf-marked functions are probably not noop when the
>> format argument is empty.
>
> How is that? Does the warning message from qobject_from_json("") mean
> that GCC may optimize that call away?

I meant, the warning is likely bogus for most functions that do 
"something like printf" but it is not printf (like qobject_from_json, or 
asprintf).  If the only good reason to have the warning is a 
hypothetical performance degradation, let's please turn it off, because 
this performance degradation isn't even there.

Paolo
Blue Swirl - Oct. 15, 2010, 5:41 p.m.
On Fri, Oct 15, 2010 at 1:33 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 10/14/2010 07:59 PM, Blue Swirl wrote:
>>>
>>> It is even more hypothetical when empty-format printfs are optimized away
>>> by
>>> GCC:
>>>
>>> $ gcc -x c - -O2 -S -o -
>>> #include<stdio.h>
>>> main() { printf (""); }
>>>
>>>        .file   ""
>>>        .text
>>>        .p2align 4,,15
>>> .globl main
>>>        .type   main, @function
>>> main:
>>> .LFB11:
>>>        .cfi_startproc
>>>        rep
>>>        ret
>>>        .cfi_endproc
>>>
>>> and other attribute-printf-marked functions are probably not noop when
>>> the
>>> format argument is empty.
>>
>> How is that? Does the warning message from qobject_from_json("") mean
>> that GCC may optimize that call away?
>
> I meant, the warning is likely bogus for most functions that do "something
> like printf" but it is not printf (like qobject_from_json, or asprintf).  If
> the only good reason to have the warning is a hypothetical performance
> degradation, let's please turn it off, because this performance degradation
> isn't even there.

Which functions are optimized away and which aren't? What would happen
to for example monitor_printf("")? There are side effects, so it
wouldn't be correct to remove a call like that, but those calls would
still bring useless performance degradation.
Paolo Bonzini - Oct. 16, 2010, 12:37 a.m.
On 10/15/2010 07:41 PM, Blue Swirl wrote:
> Which functions are optimized away and which aren't?

It's builtins only that are optimized away or otherwise inlined (printf, 
sprintf, etc.).  Other calls stay, together with side effects and clock 
cycles.

Paolo
Blue Swirl - Oct. 16, 2010, 4:28 p.m.
On Sat, Oct 16, 2010 at 12:37 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 10/15/2010 07:41 PM, Blue Swirl wrote:
>>
>> Which functions are optimized away and which aren't?
>
> It's builtins only that are optimized away or otherwise inlined (printf,
> sprintf, etc.).  Other calls stay, together with side effects and clock
> cycles.

Then the warning makes sense (slightly) and should remain on main QEMU side.

Patch

diff --git a/configure b/configure
index d303061..3a12f92 100755
--- a/configure
+++ b/configure
@@ -146,7 +146,8 @@  QEMU_CFLAGS="-I. -I\$(SRC_PATH) $QEMU_CFLAGS"
 LDFLAGS="-g $LDFLAGS"
 
 gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
-gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags"
+gcc_flags="-Wformat-security -Wformat-y2k -Wno-format-zero-length $gcc_flags"
+gcc_flags="-Winit-self -Wignored-qualifiers $gcc_flags"
 gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
 gcc_flags="-fstack-protector-all $gcc_flags"
 cat > $TMPC << EOF
@@ -154,7 +155,7 @@  int main(void) { return 0; }
 EOF
 for flag in $gcc_flags; do
     if compile_prog "-Werror $QEMU_CFLAGS" "-Werror $flag" ; then
-	QEMU_CFLAGS="$flag $QEMU_CFLAGS"
+	QEMU_CFLAGS="$QEMU_CFLAGS $flag"
     fi
 done