[Qemu-devel] [PATCH] Suppress warning: zero-length gnu_printf format string

[Qemu-devel] [PATCH] Suppress warning: zero-length gnu_printf format string

[Markus Armbruster]

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
 
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH] Suppress warning: zero-length gnu_printf format string

[Paolo Bonzini]

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

[Qemu-devel] Re: [PATCH] Suppress warning: zero-length gnu_printf format string

[Blue Swirl]

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.

[Qemu-devel] Re: [PATCH] Suppress warning: zero-length gnu_printf format string

[Markus Armbruster]

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.

[Qemu-devel] Re: [PATCH] Suppress warning: zero-length gnu_printf format string

[Blue Swirl]

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.

[Qemu-devel] [PATCH] configure: Support disabling warnings in $gcc_flags

[Markus Armbruster]

-Wall enables a bunch of warnings at once.  configure puts it after
$gcc_flags.  This makes it impossible to disable warnings enabled by
-Wall there.  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
@@ -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
 
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH] configure: Support disabling warnings in $gcc_flags

[Blue Swirl]

Thanks, applied.

On Thu, Oct 14, 2010 at 9:19 AM, Markus Armbruster <armbru@redhat.com> wrote:
> -Wall enables a bunch of warnings at once.  configure puts it after
> $gcc_flags.  This makes it impossible to disable warnings enabled by
> -Wall there.  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
> @@ -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
>
> --
> 1.7.2.3
>

[Qemu-devel] Re: [PATCH] Suppress warning: zero-length gnu_printf format string

[Markus Armbruster]

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.

[Qemu-devel] Re: [PATCH] Suppress warning: zero-length gnu_printf format string

[Blue Swirl]

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.

[Qemu-devel] Re: [PATCH] Suppress warning: zero-length gnu_printf format string

[Paolo Bonzini]

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

[Qemu-devel] Re: [PATCH] Suppress warning: zero-length gnu_printf format string

[Blue Swirl]

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?

[Qemu-devel] Re: [PATCH] Suppress warning: zero-length gnu_printf format string

[Paolo Bonzini]

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

Re: [Qemu-devel] Re: [PATCH] Suppress warning: zero-length gnu_printf format string

[Blue Swirl]

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.

[Qemu-devel] Re: [PATCH] Suppress warning: zero-length gnu_printf format string

[Paolo Bonzini]

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

Re: [Qemu-devel] Re: [PATCH] Suppress warning: zero-length gnu_printf format string

[Blue Swirl]

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.

[Qemu-devel] [PATCH] Silence compiler warning in json test case

[Jan Kiszka]

Am 16.10.2010 18:28, Blue Swirl wrote:
> 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.
> 

From: Jan Kiszka <jan.kiszka@siemens.com>

This avoids

    error: zero-length gnu_printf format string

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 check-qjson.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/check-qjson.c b/check-qjson.c
index 0b60e45..64fcdcb 100644
--- a/check-qjson.c
+++ b/check-qjson.c
@@ -639,7 +639,9 @@ END_TEST
 
 START_TEST(empty_input)
 {
-    QObject *obj = qobject_from_json("");
+    const char *empty = "";
+
+    QObject *obj = qobject_from_json(empty);
     fail_unless(obj == NULL);
 }
 END_TEST

Re: [Qemu-devel] [PATCH] Silence compiler warning in json test case

[Luiz Capitulino]

On Sat, 16 Oct 2010 19:42:43 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> Am 16.10.2010 18:28, Blue Swirl wrote:
> > 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.
> > 
> 
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> This avoids
> 
>     error: zero-length gnu_printf format string
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

I've applied this one to the QMP queue, but of course that Blue can
push it if he wants to.

> ---
>  check-qjson.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/check-qjson.c b/check-qjson.c
> index 0b60e45..64fcdcb 100644
> --- a/check-qjson.c
> +++ b/check-qjson.c
> @@ -639,7 +639,9 @@ END_TEST
>  
>  START_TEST(empty_input)
>  {
> -    QObject *obj = qobject_from_json("");
> +    const char *empty = "";
> +
> +    QObject *obj = qobject_from_json(empty);
>      fail_unless(obj == NULL);
>  }
>  END_TEST
>