Re: [PATCH] Fix incorrect int->float conversions caught by clang -Wimplicit-int-float-conversion

[Eric Blake]

On 11/19/19 2:49 PM, Fangrui Song wrote:

> > Can we simply drop the offending line statement instead?
>
> Fixed in the new patch.

> > The first val * mul above this range is 0x1p64.  Rejecting it is
> > correct, because it overflows yint64_t.
>
> I am not subscribed, so apologize that this email may be off the thread.
>
> (The binutils mailing list allows a user to download the raw email so I
> can still reply to a specific email, but this list does not provide such
> feature.)

Actually, it's better to post a v2 patch as a new top-level thread, 
rather than buried as an attachment to a reply to v1, because our CI 
tooling doesn't see through the attachment (nor was it easy for me to 
reply to the v2 patch - I had to open the attachment to paste its text 
inline below...).

More patch submission hints at https://wiki.qemu.org/Contribute/SubmitAPatch

> > From 5f1c5a42794ddcbabb63d9af920d9f437ea90a9f Mon Sep 17 00:00:00 2001
> From: Fangrui Song <address@hidden>
> Date: Fri, 15 Nov 2019 16:27:47 -0800
> Subject: [PATCH] Fix incorrect integer->float conversions caught by clang
>  -Wimplicit-int-float-conversion
> To: address@hidden
>
> The warning will be enabled by default in clang 10. It is not available for clang 
> <= 9.

> +++ 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,
>      };

The explicit cast looks odd without a comment (generally, we try to 
avoid casts, so a comment such as /* explicit cast to silence compiler 
*/ can be useful)

>  
>      qmp_migrate_set_parameters(&p, errp);
> diff --git a/util/cutils.c b/util/cutils.c
> index fd591cadf0..2b4484c015 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -239,10 +239,10 @@ static int do_strtosz(const char *nptr, const char **end,
>          goto out;
>      }
>      /*
> -     * Values >= 0xfffffffffffffc00 overflow uint64_t after their trip
> +     * Values > nextafter(0x1p64, 0) overflow uint64_t after their trip
>       * through double (53 bits of precision).

I thought we agreed on more text than just this (in particular, that the 
nextafter() call represents 2^64 rounded towards zero).

>       */
> -    if ((val * mul >= 0xfffffffffffffc00) || val < 0) {
> +    if ((val * mul > nextafter(0x1p64, 0)) || val < 0) {
>          retval = -ERANGE;
>          goto out;
>      }
> --
> 2.24.0


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org