| Message ID | m3zkuksxgv.fsf@blackfin.pond.sub.org |
|---|---|
| State | New |
| Headers | show |
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
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.
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.
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.
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.
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.
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
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?
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
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.
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
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.
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
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(-)