| Message ID | 20191122080039.12771-3-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix incorrect integer->float conversion caught by clang | expand |
On 11/22/19 9:00 AM, Markus Armbruster wrote: > From: Fangrui Song <i@maskray.me> > > Clang does not like do_strtosz()'s code to guard against overflow: > > 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] > > The warning will be enabled by default in clang 10. It is not > available for clang <= 9. > > val * mul >= 0xfffffffffffffc00 is indeed wrong. 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. > > We want val * mul > (the largest uint64_t exactly representable as > double). That's 0xfffffffffffff800. Write it as nextafter(0x1p64, 0) > with a suitable comment. > > Signed-off-by: Fangrui Song <i@maskray.me> > Reviewed-by: Markus Armbruster <armbru@redhat.com> > [Patch split, commit message improved] > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > util/cutils.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Markus Armbruster <armbru@redhat.com> wrote: > From: Fangrui Song <i@maskray.me> > > Clang does not like do_strtosz()'s code to guard against overflow: > > 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] > > The warning will be enabled by default in clang 10. It is not > available for clang <= 9. > > val * mul >= 0xfffffffffffffc00 is indeed wrong. 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. > > We want val * mul > (the largest uint64_t exactly representable as > double). That's 0xfffffffffffff800. Write it as nextafter(0x1p64, 0) > with a suitable comment. > > Signed-off-by: Fangrui Song <i@maskray.me> > Reviewed-by: Markus Armbruster <armbru@redhat.com> > [Patch split, commit message improved] > Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Learning this *new* C99 float format O:-)
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; }