| Message ID | 20200604034513.75103-1-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | configure: Disable -Wtautological-type-limit-compare | expand |
On 04/06/2020 05.45, Richard Henderson wrote: > Clang 10 enables this by default with -Wtype-limit. > > All of the instances flagged by this Werror so far have been > cases in which we really do want the compiler to optimize away > the test completely. Disabling the warning will avoid having > to add ifdefs to work around this. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > configure | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/configure b/configure > index f087d2bcd1..693f01327f 100755 > --- a/configure > +++ b/configure > @@ -2009,6 +2009,8 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags" > gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags" > gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags" > gcc_flags="-Wno-string-plus-int -Wno-typedef-redefinition $gcc_flags" > +gcc_flags="$gcc_flags -Wno-tautological-type-limit-compare" > + > # Note that we do not add -Werror to gcc_flags here, because that would > # enable it for all configure tests. If a configure test failed due > # to -Werror this would just silently disable some features, Acked-by: Thomas Huth <thuth@redhat.com>
On 6/4/20 5:45 AM, Richard Henderson wrote: > Clang 10 enables this by default with -Wtype-limit. > > All of the instances flagged by this Werror so far have been > cases in which we really do want the compiler to optimize away > the test completely. Disabling the warning will avoid having > to add ifdefs to work around this. > Fixes: https://bugs.launchpad.net/qemu/+bug/1878628 Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> I dare to add: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > configure | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/configure b/configure > index f087d2bcd1..693f01327f 100755 > --- a/configure > +++ b/configure > @@ -2009,6 +2009,8 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags" > gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags" > gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags" > gcc_flags="-Wno-string-plus-int -Wno-typedef-redefinition $gcc_flags" > +gcc_flags="$gcc_flags -Wno-tautological-type-limit-compare" > + > # Note that we do not add -Werror to gcc_flags here, because that would > # enable it for all configure tests. If a configure test failed due > # to -Werror this would just silently disable some features, >
On 6/3/20 10:45 PM, Richard Henderson wrote: > Clang 10 enables this by default with -Wtype-limit. > > All of the instances flagged by this Werror so far have been > cases in which we really do want the compiler to optimize away > the test completely. Disabling the warning will avoid having > to add ifdefs to work around this. While I proposed an alternative fix that was able to silence the most recent error without #if, I do like this approach better - the warning causes far more false positives than flagging actual bugs, especially when we write code to allow 32->32, 64->32, and 64->64 host->emulation paths, where one or more of those need the check but the others really do a tautological compare, by the nature of the types involved. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 6/4/20 8:11 AM, Philippe Mathieu-Daudé wrote: > On 6/4/20 5:45 AM, Richard Henderson wrote: >> Clang 10 enables this by default with -Wtype-limit. >> >> All of the instances flagged by this Werror so far have been >> cases in which we really do want the compiler to optimize away >> the test completely. Disabling the warning will avoid having >> to add ifdefs to work around this. >> > > Fixes: https://bugs.launchpad.net/qemu/+bug/1878628 > > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> Clarifying: I tested with clang-10, but Alex/Cornelia reported on IRC the failure persist with clang-9 until using --disabler-werror. > > I dare to add: > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> configure | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/configure b/configure >> index f087d2bcd1..693f01327f 100755 >> --- a/configure >> +++ b/configure >> @@ -2009,6 +2009,8 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags" >> gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags" >> gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags" >> gcc_flags="-Wno-string-plus-int -Wno-typedef-redefinition $gcc_flags" >> +gcc_flags="$gcc_flags -Wno-tautological-type-limit-compare" >> + >> # Note that we do not add -Werror to gcc_flags here, because that would >> # enable it for all configure tests. If a configure test failed due >> # to -Werror this would just silently disable some features, >> >
On 05/06/2020 14.51, Philippe Mathieu-Daudé wrote: > On 6/4/20 8:11 AM, Philippe Mathieu-Daudé wrote: >> On 6/4/20 5:45 AM, Richard Henderson wrote: >>> Clang 10 enables this by default with -Wtype-limit. >>> >>> All of the instances flagged by this Werror so far have been >>> cases in which we really do want the compiler to optimize away >>> the test completely. Disabling the warning will avoid having >>> to add ifdefs to work around this. >>> >> >> Fixes: https://bugs.launchpad.net/qemu/+bug/1878628 >> >> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Clarifying: I tested with clang-10, but Alex/Cornelia reported on IRC > the failure persist with clang-9 until using --disabler-werror. Does -Wno-tautological-constant-compare help on Clang-9 instead? Thomas >> >> I dare to add: >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> configure | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/configure b/configure >>> index f087d2bcd1..693f01327f 100755 >>> --- a/configure >>> +++ b/configure >>> @@ -2009,6 +2009,8 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags" >>> gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags" >>> gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags" >>> gcc_flags="-Wno-string-plus-int -Wno-typedef-redefinition $gcc_flags" >>> +gcc_flags="$gcc_flags -Wno-tautological-type-limit-compare" >>> + >>> # Note that we do not add -Werror to gcc_flags here, because that would >>> # enable it for all configure tests. If a configure test failed due >>> # to -Werror this would just silently disable some features, >>> >> > >
Richard Henderson <richard.henderson@linaro.org> writes: > Clang 10 enables this by default with -Wtype-limit. > > All of the instances flagged by this Werror so far have been > cases in which we really do want the compiler to optimize away > the test completely. Disabling the warning will avoid having > to add ifdefs to work around this. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > configure | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/configure b/configure > index f087d2bcd1..693f01327f 100755 > --- a/configure > +++ b/configure > @@ -2009,6 +2009,8 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags" > gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags" > gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags" > gcc_flags="-Wno-string-plus-int -Wno-typedef-redefinition $gcc_flags" > +gcc_flags="$gcc_flags -Wno-tautological-type-limit-compare" > + nit: the pattern is reversed compared to the previous lines (foo $gcc_flags) I had exactly the same patch in my local tree but it wasn't enough for clang-9 which I was using for a sanitiser build. I ended up having to apply --disable-werrror to the configuration. Anyway: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
Thomas Huth <thuth@redhat.com> writes: > On 05/06/2020 14.51, Philippe Mathieu-Daudé wrote: >> On 6/4/20 8:11 AM, Philippe Mathieu-Daudé wrote: >>> On 6/4/20 5:45 AM, Richard Henderson wrote: >>>> Clang 10 enables this by default with -Wtype-limit. >>>> >>>> All of the instances flagged by this Werror so far have been >>>> cases in which we really do want the compiler to optimize away >>>> the test completely. Disabling the warning will avoid having >>>> to add ifdefs to work around this. >>>> >>> >>> Fixes: https://bugs.launchpad.net/qemu/+bug/1878628 >>> >>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> >> Clarifying: I tested with clang-10, but Alex/Cornelia reported on IRC >> the failure persist with clang-9 until using --disabler-werror. > > Does -Wno-tautological-constant-compare help on Clang-9 instead? Yeah that variant works for clang-9 Tested-by: Alex Bennée <alex.bennee@linaro.org> > > Thomas > > >>> >>> I dare to add: >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> >>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>>> --- >>>> configure | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/configure b/configure >>>> index f087d2bcd1..693f01327f 100755 >>>> --- a/configure >>>> +++ b/configure >>>> @@ -2009,6 +2009,8 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags" >>>> gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags" >>>> gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags" >>>> gcc_flags="-Wno-string-plus-int -Wno-typedef-redefinition $gcc_flags" >>>> +gcc_flags="$gcc_flags -Wno-tautological-type-limit-compare" >>>> + >>>> # Note that we do not add -Werror to gcc_flags here, because that would >>>> # enable it for all configure tests. If a configure test failed due >>>> # to -Werror this would just silently disable some features, >>>> >>> >> >> -- Alex Bennée
On 6/5/20 8:53 AM, Alex Bennée wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > >> Clang 10 enables this by default with -Wtype-limit. >> >> All of the instances flagged by this Werror so far have been >> cases in which we really do want the compiler to optimize away >> the test completely. Disabling the warning will avoid having >> to add ifdefs to work around this. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> configure | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/configure b/configure >> index f087d2bcd1..693f01327f 100755 >> --- a/configure >> +++ b/configure >> @@ -2009,6 +2009,8 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags" >> gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags" >> gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags" >> gcc_flags="-Wno-string-plus-int -Wno-typedef-redefinition $gcc_flags" >> +gcc_flags="$gcc_flags -Wno-tautological-type-limit-compare" >> + > > nit: the pattern is reversed compared to the previous lines (foo $gcc_flags) Not a nit. The -Wno- must follow -Wtype-limit, or -Wtype-limit will turn it back on. r~ > > I had exactly the same patch in my local tree but it wasn't enough for > clang-9 which I was using for a sanitiser build. I ended up > having to apply --disable-werrror to the configuration. > > Anyway: > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >
On Fri, 5 Jun 2020 at 18:47, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 6/5/20 8:53 AM, Alex Bennée wrote: > >> --- a/configure > >> +++ b/configure > >> @@ -2009,6 +2009,8 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags" > >> gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags" > >> gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags" > >> gcc_flags="-Wno-string-plus-int -Wno-typedef-redefinition $gcc_flags" > >> +gcc_flags="$gcc_flags -Wno-tautological-type-limit-compare" > >> + > > > > nit: the pattern is reversed compared to the previous lines (foo $gcc_flags) > > Not a nit. The -Wno- must follow -Wtype-limit, or -Wtype-limit will turn it > back on. If there's an ordering requirement here we should make it clearer, or somebody is going to do the obvious "tidying up" at some point in the future. Perhaps this whole set of lines should be rearranged, something like: # Enable these warnings if the compiler supports them: warn_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits" warn_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $warn_flags" warn_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $warn_flags" warn_flags="-Wendif-labels -Wexpansion-to-defined $warn_flags" # Now disable sub-types of warning we don't want but which are # enabled by some of the warning flags we do want; these must come # later in the compiler command line than the enabling warning options. nowarn_flags="-Wno-missing-include-dirs -Wno-shift-negative-value" nowarn_flags="-Wno-initializer-overrides $nowarn_flags" nowarn_flags="-Wno-string-plus-int -Wno-typedef-redefinition $nowarn_flags" warn_flags="$warn_flags $nowarn_flags" (is there a nicer shell idiom for creating a variable that's a long string of stuff without having over-long lines?) It's also tempting to pull the handful of warning related options currently set directly in QEMU_CFLAGS (-Wall, etc) into this same set of variables. thanks -- PMM
On 6/5/20 1:09 PM, Peter Maydell wrote: > If there's an ordering requirement here we should make it clearer, > or somebody is going to do the obvious "tidying up" at some point > in the future. > > Perhaps this whole set of lines should be rearranged, something like: > > # Enable these warnings if the compiler supports them: > warn_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits" > warn_flags="-Wformat-security -Wformat-y2k -Winit-self > -Wignored-qualifiers $warn_flags" > warn_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $warn_flags" > warn_flags="-Wendif-labels -Wexpansion-to-defined $warn_flags" > # Now disable sub-types of warning we don't want but which are > # enabled by some of the warning flags we do want; these must come > # later in the compiler command line than the enabling warning options. > nowarn_flags="-Wno-missing-include-dirs -Wno-shift-negative-value" > nowarn_flags="-Wno-initializer-overrides $nowarn_flags" > nowarn_flags="-Wno-string-plus-int -Wno-typedef-redefinition $nowarn_flags" > warn_flags="$warn_flags $nowarn_flags" > > (is there a nicer shell idiom for creating a variable that's > a long string of stuff without having over-long lines?) You could always do: # Append $2 into the variable named $1, with space separation add_to () { eval $1=\${$1:+\"\$$1 \"}\$2 } add_to warn_flags -Wold-style-declaration add_to warn_flags -Wold-style-definition add_to warn_flags -Wtype-limits ... add_to nowarn_flags -Wno-string-plus-int add_to nowarn_flags -Wno-typedef-redefinition warn_flags="$warn_flags $nowarn_flags" > > It's also tempting to pull the handful of warning related > options currently set directly in QEMU_CFLAGS (-Wall, etc) into > this same set of variables. > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
пет, 5. јун 2020. у 20:28 Eric Blake <eblake@redhat.com> је написао/ла: > > On 6/5/20 1:09 PM, Peter Maydell wrote: > > > If there's an ordering requirement here we should make it clearer, > > or somebody is going to do the obvious "tidying up" at some point > > in the future. > > > > Perhaps this whole set of lines should be rearranged, something like: > > > > # Enable these warnings if the compiler supports them: > > warn_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits" > > warn_flags="-Wformat-security -Wformat-y2k -Winit-self > > -Wignored-qualifiers $warn_flags" > > warn_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $warn_flags" > > warn_flags="-Wendif-labels -Wexpansion-to-defined $warn_flags" > > # Now disable sub-types of warning we don't want but which are > > # enabled by some of the warning flags we do want; these must come > > # later in the compiler command line than the enabling warning options. > > nowarn_flags="-Wno-missing-include-dirs -Wno-shift-negative-value" > > nowarn_flags="-Wno-initializer-overrides $nowarn_flags" > > nowarn_flags="-Wno-string-plus-int -Wno-typedef-redefinition $nowarn_flags" > > warn_flags="$warn_flags $nowarn_flags" > > > > (is there a nicer shell idiom for creating a variable that's > > a long string of stuff without having over-long lines?) > > You could always do: > > # Append $2 into the variable named $1, with space separation > add_to () { > eval $1=\${$1:+\"\$$1 \"}\$2 > } > > add_to warn_flags -Wold-style-declaration > add_to warn_flags -Wold-style-definition > add_to warn_flags -Wtype-limits > ... > add_to nowarn_flags -Wno-string-plus-int > add_to nowarn_flags -Wno-typedef-redefinition > warn_flags="$warn_flags $nowarn_flags" > I support the ideas outlined above by Peter and Eric. Especially I like "one line per flag" approach, implicitly introduced by Eric. This is a very good opportunity to bring some order and beauty into this, frankly, ugly piece of code. Thanks Aleksandar > > > > It's also tempting to pull the handful of warning related > > options currently set directly in QEMU_CFLAGS (-Wall, etc) into > > this same set of variables. > > > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3226 > Virtualization: qemu.org | libvirt.org > >
diff --git a/configure b/configure index f087d2bcd1..693f01327f 100755 --- a/configure +++ b/configure @@ -2009,6 +2009,8 @@ gcc_flags="-Wno-missing-include-dirs -Wempty-body -Wnested-externs $gcc_flags" gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags" gcc_flags="-Wno-initializer-overrides -Wexpansion-to-defined $gcc_flags" gcc_flags="-Wno-string-plus-int -Wno-typedef-redefinition $gcc_flags" +gcc_flags="$gcc_flags -Wno-tautological-type-limit-compare" + # Note that we do not add -Werror to gcc_flags here, because that would # enable it for all configure tests. If a configure test failed due # to -Werror this would just silently disable some features,
Clang 10 enables this by default with -Wtype-limit. All of the instances flagged by this Werror so far have been cases in which we really do want the compiler to optimize away the test completely. Disabling the warning will avoid having to add ifdefs to work around this. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- configure | 2 ++ 1 file changed, 2 insertions(+) -- 2.26.2