On 2019-11-21, Markus Armbruster wrote:
Richard Henderson <richard.hender...@linaro.org> writes:
On 11/20/19 6:30 PM, Fangrui Song wrote:
On 2019-11-20, Juan Quintela wrote:
Markus Armbruster <arm...@redhat.com> wrote:
Fangrui Song <i...@maskray.me> writes:
[...]
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).
*/
- if ((val * mul >= 0xfffffffffffffc00) || val < 0) {
+ if ((val * mul > nextafter(0x1p64, 0)) || val < 0) {
retval = -ERANGE;
goto out;
}
This comment was really bad (it says the same that the code).
On the other hand, I can *kind of* understand what does 0xffff<more
f's here>.
But I am at a complete loss about what value is:
nextafter(0x1p64, 0).
Can we put what value is that instead?
It is a C99 hexadecimal floating-point literal.
0x1p64 represents hex fraction 1.0 scaled by 2**64, that is 2**64.
We can write this as `val * mul > 0xfffffffffffff800p0`, but I feel that
counting the number of f's is error-prone and is not fun.
(We cannot use val * mul >= 0x1p64.
If FLT_EVAL_METHOD == 2, the intermediate computation val * mul will be
performed at long double precision, val * mul may not by representable
by a double and will overflow as (double)0x1p64.)
I agree about not spelling out the f's, or the 0x800 at the end. That's
something that the compiler can do for us, resolving this standard library
function at compile-time.
We just need a better comment. Perhaps:
/*
* 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".
*/
Yes, please.
Thanks for the suggestion. Attached a new patch.
>From 13312e91e5565a6bed8c394d5711603c7a8f8a3c 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
