[PATCH] configure: Disable -Wtautological-type-limit-compare

[PATCH] configure: Disable -Wtautological-type-limit-compare

[Richard Henderson]

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,
-- 
2.26.2

Re: [PATCH] configure: Disable -Wtautological-type-limit-compare

[Thomas Huth]

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>

Re: [PATCH] configure: Disable -Wtautological-type-limit-compare

[Philippe Mathieu-Daudé]

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

Re: [PATCH] configure: Disable -Wtautological-type-limit-compare

[Eric Blake]

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

Re: [PATCH] configure: Disable -Wtautological-type-limit-compare

[Philippe Mathieu-Daudé]

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

Re: [PATCH] configure: Disable -Wtautological-type-limit-compare

[Thomas Huth]

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

Re: [PATCH] configure: Disable -Wtautological-type-limit-compare

[Alex Bennée]

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

Re: [PATCH] configure: Disable -Wtautological-type-limit-compare

[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

Re: [PATCH] configure: Disable -Wtautological-type-limit-compare

[Richard Henderson]

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

Re: [PATCH] configure: Disable -Wtautological-type-limit-compare

[Peter Maydell]

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

Re: [PATCH] configure: Disable -Wtautological-type-limit-compare

[Eric Blake]

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

Re: [PATCH] configure: Disable -Wtautological-type-limit-compare

[Aleksandar Markovic]

пет, 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
>
>