| Message ID | 20191116010731.3jdxozzfpsqsrcc4@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix incorrect int->float conversions caught by clang -Wimplicit-int-float-conversion | expand |
Fangrui Song <i@maskray.me> writes: > 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 | 4 ++-- > util/cutils.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 354ad072fa..ac3ea2934a 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -53,6 +53,7 @@ > #include "monitor/monitor.h" > #include "net/announce.h" > #include "qemu/queue.h" > +#include <math.h> > > #define MAX_THROTTLE (32 << 20) /* Migration transfer speed throttling */ > > @@ -2035,11 +2036,10 @@ void qmp_migrate_set_downtime(double value, Error **errp) if (value < 0 || value > MAX_MIGRATE_DOWNTIME_SECONDS) { error_setg(errp, "Parameter 'downtime_limit' expects an integer in " "the range of 0 to %d seconds", MAX_MIGRATE_DOWNTIME_SECONDS); return; > } @value is now in [0,2000]. > > value *= 1000; /* Convert to milliseconds */ @value is in [0,2000000] > - value = MAX(0, MIN(INT64_MAX, value)); This does nothing. > > MigrateSetParameters p = { > .has_downtime_limit = true, > - .downtime_limit = value, > + .downtime_limit = (int64_t)fmin(value, nextafter(0x1p63, 0)), This does nothing and is hard to read :) Can we simply drop the offending line statement instead? > }; > > 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). > */ > - if ((val * mul >= 0xfffffffffffffc00) || val < 0) { > + if ((val * mul > nextafter(0x1p64, 0)) || val < 0) { > retval = -ERANGE; > goto out; > } *result = val * mul; I figure this one is correct and hard to read. 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. nextafter(0x1p64, 0) is a clever way to write 0xfffffffffffff800, the largest uint64_t exactly representable as double. With your patch, val * mul in [0,0xfffffffffffff800] will be accepted. The first val * mul above this range is 0x1p64. Rejecting it is correct, because it overflows yint64_t.
> Fangrui Song <address@hidden> writes: > > > 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 <address@hidden> > > --- > > migration/migration.c | 4 ++-- > > util/cutils.c | 4 ++-- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/migration/migration.c b/migration/migration.c > > index 354ad072fa..ac3ea2934a 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -53,6 +53,7 @@ > > #include "monitor/monitor.h" > > #include "net/announce.h" > > #include "qemu/queue.h" > > +#include <math.h> > > > > #define MAX_THROTTLE (32 << 20) /* Migration transfer speed > > throttling */ > > > > @@ -2035,11 +2036,10 @@ void qmp_migrate_set_downtime(double value, Error > > **errp) > if (value < 0 || value > MAX_MIGRATE_DOWNTIME_SECONDS) { > error_setg(errp, "Parameter 'downtime_limit' expects an integer in " > "the range of 0 to %d seconds", > MAX_MIGRATE_DOWNTIME_SECONDS); > return; > > } > > @value is now in [0,2000]. > > > > > value *= 1000; /* Convert to milliseconds */ > > @value is in [0,2000000] > > > - value = MAX(0, MIN(INT64_MAX, value)); > > This does nothing. > > > > > MigrateSetParameters p = { > > .has_downtime_limit = true, > > - .downtime_limit = value, > > + .downtime_limit = (int64_t)fmin(value, nextafter(0x1p63, 0)), > > This does nothing and is hard to read :) > > Can we simply drop the offending line statement instead? Fixed in the new patch. > > }; > > > > 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). > > */ > > - if ((val * mul >= 0xfffffffffffffc00) || val < 0) { > > + if ((val * mul > nextafter(0x1p64, 0)) || val < 0) { > > retval = -ERANGE; > > goto out; > > } > *result = val * mul; > > I figure this one is correct and hard to read. > > 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. > > nextafter(0x1p64, 0) is a clever way to write 0xfffffffffffff800, the > largest uint64_t exactly representable as double. > > With your patch, val * mul in [0,0xfffffffffffff800] will be accepted. > > 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.)
Markus Armbruster <armbru@redhat.com> wrote: > Fangrui Song <i@maskray.me> writes: > >> 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 | 4 ++-- >> util/cutils.c | 4 ++-- >> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index 354ad072fa..ac3ea2934a 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -53,6 +53,7 @@ >> #include "monitor/monitor.h" >> #include "net/announce.h" >> #include "qemu/queue.h" >> +#include <math.h> >> >> #define MAX_THROTTLE (32 << 20) /* Migration transfer speed throttling */ >> >> @@ -2035,11 +2036,10 @@ void qmp_migrate_set_downtime(double value, Error **errp) > if (value < 0 || value > MAX_MIGRATE_DOWNTIME_SECONDS) { > error_setg(errp, "Parameter 'downtime_limit' expects an integer in " > "the range of 0 to %d seconds", > MAX_MIGRATE_DOWNTIME_SECONDS); > return; >> } > > @value is now in [0,2000]. > >> >> value *= 1000; /* Convert to milliseconds */ > > @value is in [0,2000000] > >> - value = MAX(0, MIN(INT64_MAX, value)); > > This does nothing. > >> >> MigrateSetParameters p = { >> .has_downtime_limit = true, >> - .downtime_limit = value, >> + .downtime_limit = (int64_t)fmin(value, nextafter(0x1p63, 0)), > > This does nothing and is hard to read :) > > Can we simply drop the offending line statement instead? Agreed aboutdropping the whole bussines for migration. >> }; >> >> 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). >> */ >> - 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? Thanks, Juan.
On 2019-11-20, Juan Quintela wrote: >Markus Armbruster <armbru@redhat.com> wrote: >> Fangrui Song <i@maskray.me> writes: >> >>> 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 | 4 ++-- >>> util/cutils.c | 4 ++-- >>> 2 files changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/migration/migration.c b/migration/migration.c >>> index 354ad072fa..ac3ea2934a 100644 >>> --- a/migration/migration.c >>> +++ b/migration/migration.c >>> @@ -53,6 +53,7 @@ >>> #include "monitor/monitor.h" >>> #include "net/announce.h" >>> #include "qemu/queue.h" >>> +#include <math.h> >>> >>> #define MAX_THROTTLE (32 << 20) /* Migration transfer speed throttling */ >>> >>> @@ -2035,11 +2036,10 @@ void qmp_migrate_set_downtime(double value, Error **errp) >> if (value < 0 || value > MAX_MIGRATE_DOWNTIME_SECONDS) { >> error_setg(errp, "Parameter 'downtime_limit' expects an integer in " >> "the range of 0 to %d seconds", >> MAX_MIGRATE_DOWNTIME_SECONDS); >> return; >>> } >> >> @value is now in [0,2000]. >> >>> >>> value *= 1000; /* Convert to milliseconds */ >> >> @value is in [0,2000000] >> >>> - value = MAX(0, MIN(INT64_MAX, value)); >> >> This does nothing. >> >>> >>> MigrateSetParameters p = { >>> .has_downtime_limit = true, >>> - .downtime_limit = value, >>> + .downtime_limit = (int64_t)fmin(value, nextafter(0x1p63, 0)), >> >> This does nothing and is hard to read :) >> >> Can we simply drop the offending line statement instead? > >Agreed aboutdropping the whole bussines for migration. > > >>> }; >>> >>> 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). >>> */ >>> - 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.)
On 11/20/19 6:30 PM, Fangrui Song wrote: > On 2019-11-20, Juan Quintela wrote: >> Markus Armbruster <armbru@redhat.com> wrote: >>> Fangrui Song <i@maskray.me> writes: >>> >>>> 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 | 4 ++-- >>>> util/cutils.c | 4 ++-- >>>> 2 files changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/migration/migration.c b/migration/migration.c >>>> index 354ad072fa..ac3ea2934a 100644 >>>> --- a/migration/migration.c >>>> +++ b/migration/migration.c >>>> @@ -53,6 +53,7 @@ >>>> #include "monitor/monitor.h" >>>> #include "net/announce.h" >>>> #include "qemu/queue.h" >>>> +#include <math.h> >>>> >>>> #define MAX_THROTTLE (32 << 20) /* Migration transfer speed >>>> throttling */ >>>> >>>> @@ -2035,11 +2036,10 @@ void qmp_migrate_set_downtime(double value, Error >>>> **errp) >>> if (value < 0 || value > MAX_MIGRATE_DOWNTIME_SECONDS) { >>> error_setg(errp, "Parameter 'downtime_limit' expects an integer >>> in " >>> "the range of 0 to %d seconds", >>> MAX_MIGRATE_DOWNTIME_SECONDS); >>> return; >>>> } >>> >>> @value is now in [0,2000]. >>> >>>> >>>> value *= 1000; /* Convert to milliseconds */ >>> >>> @value is in [0,2000000] >>> >>>> - value = MAX(0, MIN(INT64_MAX, value)); >>> >>> This does nothing. >>> >>>> >>>> MigrateSetParameters p = { >>>> .has_downtime_limit = true, >>>> - .downtime_limit = value, >>>> + .downtime_limit = (int64_t)fmin(value, nextafter(0x1p63, 0)), >>> >>> This does nothing and is hard to read :) >>> >>> Can we simply drop the offending line statement instead? >> >> Agreed aboutdropping the whole bussines for migration. >> >> >>>> }; >>>> >>>> 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). >>>> */ >>>> - 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". */ r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 11/20/19 6:30 PM, Fangrui Song wrote: >> On 2019-11-20, Juan Quintela wrote: >>> Markus Armbruster <armbru@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.
On 2019-11-21, Markus Armbruster wrote: >Richard Henderson <richard.henderson@linaro.org> writes: > >> On 11/20/19 6:30 PM, Fangrui Song wrote: >>> On 2019-11-20, Juan Quintela wrote: >>>> Markus Armbruster <armbru@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.
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 <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) > > 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
diff --git a/migration/migration.c b/migration/migration.c index 354ad072fa..ac3ea2934a 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -53,6 +53,7 @@ #include "monitor/monitor.h" #include "net/announce.h" #include "qemu/queue.h" +#include <math.h> #define MAX_THROTTLE (32 << 20) /* Migration transfer speed throttling */ @@ -2035,11 +2036,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)fmin(value, nextafter(0x1p63, 0)), }; 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). */ - if ((val * mul >= 0xfffffffffffffc00) || val < 0) { + if ((val * mul > nextafter(0x1p64, 0)) || val < 0) { retval = -ERANGE; goto out; }
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 | 4 ++-- util/cutils.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)