[PATCH v3 0/2] Fix incorrect integer->float conversion caught by clang

[PATCH v3 0/2] Fix incorrect integer->float conversion caught by clang

[Markus Armbruster]

Fangrui Song's "[PATCH v2] Fix incorrect integer->float conversions
caught by clang -Wimplicit-int-float-conversion" doesn't apply with
git-am, and it mixes a bug fix, which should be considered for 4.2,
with a cleanup, which should go into 5.0.  I took the liberty to
respin, hope that's alright.

I propose the migration maintainers pick up PATCH 1 for 5.0.

I can submit PATCH 2 for 4.2-rc3.

Fangrui Song (2):
  migration: Fix incorrect integer->float conversion caught by clang
  util/cutils: Fix incorrect integer->float conversion caught by clang

 migration/migration.c | 3 +--
 util/cutils.c         | 8 +++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.21.0

[PATCH v3 1/2] migration: Fix incorrect integer->float conversion caught by clang

[Markus Armbruster]

From: Fangrui Song <i@maskray.me>

Clang does not like qmp_migrate_set_downtime()'s code to clamp double
@value to 0..INT64_MAX:

    qemu/migration/migration.c:2038:24: error: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion]

The warning will be enabled by default in clang 10. It is not
available for clang <= 9.

The clamp is actually useless; @value is checked to be within
0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before.  Delete it.

While there, make the conversion from double to int64_t explicit.

Signed-off-by: Fangrui Song <i@maskray.me>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
[Patch split, commit message improved]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/migration.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 354ad072fa..09b150663f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2035,11 +2035,10 @@ void qmp_migrate_set_downtime(double value, Error **errp)
     }
 
     value *= 1000; /* Convert to milliseconds */
-    value = MAX(0, MIN(INT64_MAX, value));
 
     MigrateSetParameters p = {
         .has_downtime_limit = true,
-        .downtime_limit = value,
+        .downtime_limit = (int64_t)value,
     };
 
     qmp_migrate_set_parameters(&p, errp);
-- 
2.21.0

[PATCH v3 2/2] util/cutils: Fix incorrect integer->float conversion caught by clang

[Markus Armbruster]

From: Fangrui Song <i@maskray.me>

Clang does not like do_strtosz()'s code to guard against overflow:

    qemu/util/cutils.c:245:23: error: implicit conversion from 'unsigned long' to 'double' changes value from 18446744073709550592 to 18446744073709551616 [-Werror,-Wimplicit-int-float-conversion]

The warning will be enabled by default in clang 10. It is not
available for clang <= 9.

val * mul >= 0xfffffffffffffc00 is indeed wrong.  0xfffffffffffffc00
is not representable exactly as double.  It's half-way between the
representable values 0xfffffffffffff800 and 0x10000000000000000.
Which one we get is implementation-defined.  Bad.

We want val * mul > (the largest uint64_t exactly representable as
double).  That's 0xfffffffffffff800.  Write it as nextafter(0x1p64, 0)
with a suitable comment.

Signed-off-by: Fangrui Song <i@maskray.me>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
[Patch split, commit message improved]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/cutils.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/util/cutils.c b/util/cutils.c
index fd591cadf0..77acadc70a 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -239,10 +239,12 @@ static int do_strtosz(const char *nptr, const char **end,
         goto out;
     }
     /*
-     * Values >= 0xfffffffffffffc00 overflow uint64_t after their trip
-     * through double (53 bits of precision).
+     * Values near UINT64_MAX overflow to 2**64 when converting to double
+     * precision.  Compare against the maximum representable double precision
+     * value below 2**64, computed as "the next value after 2**64 (0x1p64) in
+     * the direction of 0".
      */
-    if ((val * mul >= 0xfffffffffffffc00) || val < 0) {
+    if ((val * mul > nextafter(0x1p64, 0)) || val < 0) {
         retval = -ERANGE;
         goto out;
     }
-- 
2.21.0

Re: [PATCH v3 1/2] migration: Fix incorrect integer->float conversion caught by clang

[Richard Henderson]

On 11/22/19 9:00 AM, Markus Armbruster wrote:
> From: Fangrui Song <i@maskray.me>
> 
> Clang does not like qmp_migrate_set_downtime()'s code to clamp double
> @value to 0..INT64_MAX:
> 
>     qemu/migration/migration.c:2038:24: error: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion]
> 
> The warning will be enabled by default in clang 10. It is not
> available for clang <= 9.
> 
> The clamp is actually useless; @value is checked to be within
> 0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before.  Delete it.
> 
> While there, make the conversion from double to int64_t explicit.
> 
> Signed-off-by: Fangrui Song <i@maskray.me>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> [Patch split, commit message improved]
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  migration/migration.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v3 2/2] util/cutils: Fix incorrect integer->float conversion caught by clang

[Richard Henderson]

On 11/22/19 9:00 AM, Markus Armbruster wrote:
> From: Fangrui Song <i@maskray.me>
> 
> Clang does not like do_strtosz()'s code to guard against overflow:
> 
>     qemu/util/cutils.c:245:23: error: implicit conversion from 'unsigned long' to 'double' changes value from 18446744073709550592 to 18446744073709551616 [-Werror,-Wimplicit-int-float-conversion]
> 
> The warning will be enabled by default in clang 10. It is not
> available for clang <= 9.
> 
> val * mul >= 0xfffffffffffffc00 is indeed wrong.  0xfffffffffffffc00
> is not representable exactly as double.  It's half-way between the
> representable values 0xfffffffffffff800 and 0x10000000000000000.
> Which one we get is implementation-defined.  Bad.
> 
> We want val * mul > (the largest uint64_t exactly representable as
> double).  That's 0xfffffffffffff800.  Write it as nextafter(0x1p64, 0)
> with a suitable comment.
> 
> Signed-off-by: Fangrui Song <i@maskray.me>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> [Patch split, commit message improved]
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/cutils.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v3 1/2] migration: Fix incorrect integer->float conversion caught by clang

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> From: Fangrui Song <i@maskray.me>
>
> Clang does not like qmp_migrate_set_downtime()'s code to clamp double
> @value to 0..INT64_MAX:
>
>     qemu/migration/migration.c:2038:24: error: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion]
>
> The warning will be enabled by default in clang 10. It is not
> available for clang <= 9.
>
> The clamp is actually useless; @value is checked to be within
> 0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before.  Delete it.
>
> While there, make the conversion from double to int64_t explicit.
>
> Signed-off-by: Fangrui Song <i@maskray.me>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> [Patch split, commit message improved]
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Should I get this through migration tree, or are you going to pull it?

Later, Juan.

Re: [PATCH v3 2/2] util/cutils: Fix incorrect integer->float conversion caught by clang

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> From: Fangrui Song <i@maskray.me>
>
> Clang does not like do_strtosz()'s code to guard against overflow:
>
>     qemu/util/cutils.c:245:23: error: implicit conversion from 'unsigned long' to 'double' changes value from 18446744073709550592 to 18446744073709551616 [-Werror,-Wimplicit-int-float-conversion]
>
> The warning will be enabled by default in clang 10. It is not
> available for clang <= 9.
>
> val * mul >= 0xfffffffffffffc00 is indeed wrong.  0xfffffffffffffc00
> is not representable exactly as double.  It's half-way between the
> representable values 0xfffffffffffff800 and 0x10000000000000000.
> Which one we get is implementation-defined.  Bad.
>
> We want val * mul > (the largest uint64_t exactly representable as
> double).  That's 0xfffffffffffff800.  Write it as nextafter(0x1p64, 0)
> with a suitable comment.
>
> Signed-off-by: Fangrui Song <i@maskray.me>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> [Patch split, commit message improved]
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Learning this *new* C99 float format O:-)

Re: [PATCH v3 1/2] migration: Fix incorrect integer->float conversion caught by clang

[Philippe Mathieu-Daudé]

On 11/22/19 9:00 AM, Markus Armbruster wrote:
> From: Fangrui Song <i@maskray.me>
> 
> Clang does not like qmp_migrate_set_downtime()'s code to clamp double
> @value to 0..INT64_MAX:
> 
>      qemu/migration/migration.c:2038:24: error: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion]
> 
> The warning will be enabled by default in clang 10. It is not
> available for clang <= 9.
> 
> The clamp is actually useless; @value is checked to be within
> 0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before.  Delete it.
> 
> While there, make the conversion from double to int64_t explicit.
> 
> Signed-off-by: Fangrui Song <i@maskray.me>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> [Patch split, commit message improved]
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   migration/migration.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 354ad072fa..09b150663f 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2035,11 +2035,10 @@ void qmp_migrate_set_downtime(double value, Error **errp)
>       }
>   
>       value *= 1000; /* Convert to milliseconds */
> -    value = MAX(0, MIN(INT64_MAX, value));
>   
>       MigrateSetParameters p = {
>           .has_downtime_limit = true,
> -        .downtime_limit = value,
> +        .downtime_limit = (int64_t)value,

I agree with Eric a one line comment about the explicit cast is 
welcomed. We can still use 'git blame', find the last commit sha, then 
look at the commit description. But having it along the code is 
straightforward.

Preferably with a comment (the maintainer queing this can amend it):
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>       };
>   
>       qmp_migrate_set_parameters(&p, errp);
> 

Re: [PATCH v3 1/2] migration: Fix incorrect integer->float conversion caught by clang

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> wrote:
>> From: Fangrui Song <i@maskray.me>
>>
>> Clang does not like qmp_migrate_set_downtime()'s code to clamp double
>> @value to 0..INT64_MAX:
>>
>>     qemu/migration/migration.c:2038:24: error: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion]
>>
>> The warning will be enabled by default in clang 10. It is not
>> available for clang <= 9.
>>
>> The clamp is actually useless; @value is checked to be within
>> 0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before.  Delete it.
>>
>> While there, make the conversion from double to int64_t explicit.
>>
>> Signed-off-by: Fangrui Song <i@maskray.me>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> [Patch split, commit message improved]
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
> Should I get this through migration tree, or are you going to pull it?

Plase take this patch through the migration tree.

Re: [PATCH v3 1/2] migration: Fix incorrect integer->float conversion caught by clang

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Juan Quintela <quintela@redhat.com> writes:
>
>> Markus Armbruster <armbru@redhat.com> wrote:
>>> From: Fangrui Song <i@maskray.me>
>>>
>>> Clang does not like qmp_migrate_set_downtime()'s code to clamp double
>>> @value to 0..INT64_MAX:
>>>
>>>     qemu/migration/migration.c:2038:24: error: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion]
>>>
>>> The warning will be enabled by default in clang 10. It is not
>>> available for clang <= 9.
>>>
>>> The clamp is actually useless; @value is checked to be within
>>> 0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before.  Delete it.
>>>
>>> While there, make the conversion from double to int64_t explicit.
>>>
>>> Signed-off-by: Fangrui Song <i@maskray.me>
>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>> [Patch split, commit message improved]
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>
>> Should I get this through migration tree, or are you going to pull it?
>
> Plase take this patch through the migration tree.

Ping...

Re: [PATCH v3 1/2] migration: Fix incorrect integer->float conversion caught by clang

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Markus Armbruster <armbru@redhat.com> writes:
>
>> Juan Quintela <quintela@redhat.com> writes:
>>
>>> Markus Armbruster <armbru@redhat.com> wrote:
>>>> From: Fangrui Song <i@maskray.me>
>>>>
>>>> Clang does not like qmp_migrate_set_downtime()'s code to clamp double
>>>> @value to 0..INT64_MAX:
>>>>
>>>>     qemu/migration/migration.c:2038:24: error: implicit conversion
>>>> from 'long' to 'double' changes value from 9223372036854775807 to
>>>> 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion]
>>>>
>>>> The warning will be enabled by default in clang 10. It is not
>>>> available for clang <= 9.
>>>>
>>>> The clamp is actually useless; @value is checked to be within
>>>> 0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before.  Delete it.
>>>>
>>>> While there, make the conversion from double to int64_t explicit.
>>>>
>>>> Signed-off-by: Fangrui Song <i@maskray.me>
>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>>> [Patch split, commit message improved]
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>
>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>>
>>> Should I get this through migration tree, or are you going to pull it?
>>
>> Plase take this patch through the migration tree.
>
> Ping...

It was on my last pull request (with didn't work due to ...)
And it is on the pull request sent 30 mins ago O:-)

Later, Juan.

Re: [PATCH v3 1/2] migration: Fix incorrect integer->float conversion caught by clang

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Juan Quintela <quintela@redhat.com> writes:
> >
> >> Markus Armbruster <armbru@redhat.com> wrote:
> >>> From: Fangrui Song <i@maskray.me>
> >>>
> >>> Clang does not like qmp_migrate_set_downtime()'s code to clamp double
> >>> @value to 0..INT64_MAX:
> >>>
> >>>     qemu/migration/migration.c:2038:24: error: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Werror,-Wimplicit-int-float-conversion]
> >>>
> >>> The warning will be enabled by default in clang 10. It is not
> >>> available for clang <= 9.
> >>>
> >>> The clamp is actually useless; @value is checked to be within
> >>> 0..MAX_MIGRATE_DOWNTIME_SECONDS immediately before.  Delete it.
> >>>
> >>> While there, make the conversion from double to int64_t explicit.
> >>>
> >>> Signed-off-by: Fangrui Song <i@maskray.me>
> >>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >>> [Patch split, commit message improved]
> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >>
> >> Reviewed-by: Juan Quintela <quintela@redhat.com>
> >>
> >> Should I get this through migration tree, or are you going to pull it?
> >
> > Plase take this patch through the migration tree.
> 
> Ping...

This looks like it went in in today's migration pull.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK