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

Message ID 20191122000045.vz3eq6s6aqkv6l6h@gmail.com
State New
Headers show
Series
  • [v2] Fix incorrect integer->float conversions caught by clang -Wimplicit-int-float-conversion
Related show

Commit Message

Fangrui Song Nov. 22, 2019, midnight UTC
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 <i@maskray.me>
>>Date: Fri, 15 Nov 2019 16:27:47 -0800
>>Subject: [PATCH] Fix incorrect integer->float conversions caught by clang
>> -Wimplicit-int-float-conversion
>>To: qemu-devel@nongnu.org
>>
>>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.

Comments

Markus Armbruster Nov. 22, 2019, 8:06 a.m. UTC | #1
Fangrui Song <i@maskray.me> writes:

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

There's <https://lists.gnu.org/archive/mbox/qemu-devel/>.

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

Good, such versioning is essential to avoid confusion.  Next time, start
a new top-level thread for v2.  Our patch submission processing
(automated as well as human) expects that.

I just did it for you: "[PATCH v3 0/2] Fix incorrect integer->float
conversion caught by clang".

Many thanks for your fixes!

Patch
diff mbox series

From d533585df39083e88adc50b881a4be74125c837e Mon Sep 17 00:00:00 2001
From: Fangrui Song <i@maskray.me>
Date: Fri, 15 Nov 2019 16:27:47 -0800
Subject: [PATCH] Fix incorrect integer->float conversion caught by clang
 -Wimplicit-int-float-conversion
To: qemu-devel@nongnu.org

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

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

Signed-off-by: Fangrui Song <i@maskray.me>
---
 migration/migration.c | 3 +--
 util/cutils.c         | 8 +++++---
 2 files changed, 6 insertions(+), 5 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);
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.24.0