Bug on casting double to uint64_t

[lbianc]

Taking a closer look into the cast function, I could identify a behavior that would not be the correct one when checking if the double value fits in uint64_t.
See code below (https://github.com/facebook/hhvm/blob/master/hphp/runtime/base/type-conversions.h#L69):

inline int64_t toInt64(double  v) {
  // If v >= 0 is false and v < 0 is false, then v is NaN. In that case, on
  // Intel, you get 0x800..00, a.k.a. the minimum int64_t. We mimic that on all
  // platforms, though this makes us sad.
  return (v >= 0
          ? (v > std::numeric_limits<uint64_t>::max() ? 0u : (uint64_t)v)
          : (v < 0 ? (int64_t)v : std::numeric_limits<int64_t>::min()));
}

Let 18446744073709551616.0 be 2^64 represented as double. Let’s call it 64b_maxd.
If v has a value in the range from (64b_maxd - 1024) to (64b_maxd + 2048) the statement v > std::numeric_limits<uint64_t>::max() returns false, resulting in a cast of v to uint64_t. There is no error running the code, because coincidentally the cast for that values returns "0", what is expected for a value that does not fit in a "uint64_t". This bug does not generate an error to x86_64, but may to other platforms.
I coded a small test here: https://gist.github.com/lbianc/3d320a1a3f75f487e21b.

Based on that, the correct way to check if v value fits in a uint64_t wouldn't be verifying if it is smaller than max of "uint64_t"?
The change would be:

  return (v >= 0
          ? (v < std::numeric_limits<uint64_t>::max() ? (uint64_t)v : 0u)
          : (v < 0 ? (int64_t)v : std::numeric_limits<int64_t>::min()));

[jwatzman]

If v has a value in the range from (64b_maxd - 1024) to (64b_maxd + 2048) the statement v > std::numeric_limits<uint64_t>::max() returns false, resulting in a cast of v to uint64_t.

Can you elaborate on this? Since 64b_maxd is the maximum uint64_t, wouldn't that be the value of std::numeric_limits<uint64_t>::max()? Why is it from the range of 1024 below to 2048 above?

Also, your proposed change is, in essence, changing a > b ? c : d into a < b ? d : c, which I'd expect to be the same unless a == b which I don't think is the problem here?

What subtlety of doubles, rounding, and overflow/edge cases am I missing here?

[lbianc]

You got it right: the bug is exactly on the case that a == b. But it’s not just one value because of rounding and lose precision of double to uint64_t. See below:

Let 18446744073709551616.0 be 2^64 represented as double. Let’s call it 64b_maxd.
Consider a double value (64b_maxd - 1024) being converted to uint64_t, the result is 18446744073709551616 (64b_maxd).
It happens because of double representation, see bellow:

Double representation (64 bits):

sign  Exponent   Mantissa
| 1  |    11    |    52    |

A double value casted to uint_64 cannot represent a value between the range from (64b_maxd - 1024) to (64b_maxd + 2048) and the reason is that double have only 52 bits to represent the number as floating point, losing 11 bits of precision. See bellow:

(64b_maxd - 1025) = 0x43EFFFFFFFFFFFFF

The next number that double can represent is:

((64b_maxd + 2048)) = 0x43F0000000000001

For all numbers between (64b_maxd - 1024) and (64b_maxd + 2048) it is rounded to 64b_maxd.

So, based on that, would be correct to check if the number is less than 64b_maxd, otherwise, if the number is equal, there is no way to know if the original number was from (64b_maxd - 1024) and (64b_maxd + 2048).

[jwatzman]

Ah, thanks for the explanation. Is this actually triggerable in PHP code, that would show a different result to PHP5? Or did you say this only shows up on non-x86 platforms, since x86 just so happens to do a conversion that makes this work out anyways?

If you want to put up a PR, we'd love to take a look at it.

[lbianc]

Second option is the correct, this only shows up on non-x86 platforms, since for overflow it returns 0 (the expected result). I could identify this issue running it in a PPC64 machine, which for overflow returns -1.
My change will not change x86 behavior.

Thanks for commenting, I will create the PR.

[gut]

Taking a closer look at this problem I noticed a strange design:

Why is the result of toInt64 converting a double as big as 15956433623758761984.0 to -2490310449950789632? a direct conversion would give a more appropriate value: -9223372036854775808

Also: is there a special reason not to truncate the maximum/minimum value? Currently, on x64, the value goes to zero.

source code

[jwatzman]

@gut do we differ from PHP5/PHP7 in our visible behavior in PHP code?

[gut]

@jwatzman , no, you're right. That red region also happens on PHP5. However not the overflow cases:

<?php
var_dump((int)  4611686018427387904.0 );
var_dump((int)  6640827866535438336.0 );
var_dump((int)  7978216811879380992.0 );
var_dump((int)  9223372036854775808.0 );
var_dump((int) 13281655733070876672.0 );
var_dump((int) 15956433623758761984.0 );
var_dump((int) 18446744073709551616.0 ); // not representable on uint64_t
var_dump((int) 26563311466141753344.0 ); // not representable on uint64_t
var_dump((int) 31912867247517523968.0 ); // not representable on uint64_t
?>

HipHop VM 3.2.0 (rel) output:

int(4611686018427387904)
int(6640827866535438336)
int(7978216811879380992)
int(-9223372036854775808)
int(-5165088340638674944)
int(-2490310449950789632)
int(0)
int(0)
int(0)

php 5.6.11-1 output:

int(4611686018427387904)
int(6640827866535438336)
int(7978216811879380992)
int(-9223372036854775808)
int(-5165088340638674944)
int(-2490310449950789632)
int(0)
int(8116567392432201728)
int(-4980620899901579264)

test based on https://github.com/facebook/hhvm/blob/master/hphp/test/quick/castDbl.php

[jwatzman]

https://3v4l.org/YlkBj -- looks like we match older versions of PHP5, but the output changed as of 5.5 or so.