[PATCH v2] Fix incorrect integer->float conversions caught by clang -Wimplicit-int-float-conversion

[Fangrui Song]

On 2019-11-21, Eric Blake wrote:
> 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

Retitled to [PATCH v2]

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

downtime_limit is an int64_t while value is a double.

There is a diagnostic (-Wfloat-conversion, included by -Wconversion)

  warning: conversion from ‘double’ to ‘int64_t’ {aka ‘long int’} may change 
value [-Wfloat-conversion]

but it is not enabled by -Wall or -Wextra.

I am not familiar with qemu coding style, but I strongly feel it is a good
thing to add an explicit cast. If it does not fit the style, I hope a
maintainer can delete that for me.

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

Sorry, I uploaded the wrong patch file. Attaching the correct one now.

qemu.patch
Description: Text Data