Bug 744965 - Implement mozilla::NumberEqualsInt32 in a way that doesn't depend on undefined behavior casting an out-of-range floating point number to int32_t. r=froydnj
☠☠ backed out by 742aab314d75 ☠ ☠
authorJeff Walden <jwalden@mit.edu>
Thu, 15 Feb 2018 17:22:14 -0800
changeset 404638 1fcc972d445b035a86907702d6d53c8430d6b6b8
parent 404617 671b0faa7112e687712dff4c99e9e5e788e78f01
child 404639 66c1c1596bea7cfb2316e78c940e52679b595efa
push id33485
push userrgurzau@mozilla.com
push dateWed, 21 Feb 2018 16:46:16 +0000
treeherdermozilla-central@3904c3f9314f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs744965
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 744965 - Implement mozilla::NumberEqualsInt32 in a way that doesn't depend on undefined behavior casting an out-of-range floating point number to int32_t. r=froydnj
mfbt/FloatingPoint.h
mfbt/MathAlgorithms.h
mfbt/tests/TestFloatingPoint.cpp
--- a/mfbt/FloatingPoint.h
+++ b/mfbt/FloatingPoint.h
@@ -8,18 +8,21 @@
 
 #ifndef mozilla_FloatingPoint_h
 #define mozilla_FloatingPoint_h
 
 #include "mozilla/Assertions.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/Casting.h"
 #include "mozilla/MathAlgorithms.h"
+#include "mozilla/MemoryChecking.h"
 #include "mozilla/Types.h"
+#include "mozilla/TypeTraits.h"
 
+#include <limits>
 #include <stdint.h>
 
 namespace mozilla {
 
 /*
  * It's reasonable to ask why we have this header at all.  Don't isnan,
  * copysign, the built-in comparison operators, and the like solve these
  * problems?  Unfortunately, they don't.  We've found that various compilers
@@ -31,36 +34,36 @@ namespace mozilla {
  *
  * For the aforementioned reasons, be very wary of making changes to any of
  * these algorithms.  If you must make changes, keep a careful eye out for
  * compiler bustage, particularly PGO-specific bustage.
  */
 
 struct FloatTypeTraits
 {
-  typedef uint32_t Bits;
+  using Bits = uint32_t;
 
-  static const unsigned kExponentBias = 127;
-  static const unsigned kExponentShift = 23;
+  static constexpr unsigned kExponentBias = 127;
+  static constexpr unsigned kExponentShift = 23;
 
-  static const Bits kSignBit         = 0x80000000UL;
-  static const Bits kExponentBits    = 0x7F800000UL;
-  static const Bits kSignificandBits = 0x007FFFFFUL;
+  static constexpr Bits kSignBit         = 0x80000000UL;
+  static constexpr Bits kExponentBits    = 0x7F800000UL;
+  static constexpr Bits kSignificandBits = 0x007FFFFFUL;
 };
 
 struct DoubleTypeTraits
 {
-  typedef uint64_t Bits;
+  using Bits = uint64_t;
 
-  static const unsigned kExponentBias = 1023;
-  static const unsigned kExponentShift = 52;
+  static constexpr unsigned kExponentBias = 1023;
+  static constexpr unsigned kExponentShift = 52;
 
-  static const Bits kSignBit         = 0x8000000000000000ULL;
-  static const Bits kExponentBits    = 0x7ff0000000000000ULL;
-  static const Bits kSignificandBits = 0x000fffffffffffffULL;
+  static constexpr Bits kSignBit         = 0x8000000000000000ULL;
+  static constexpr Bits kExponentBits    = 0x7ff0000000000000ULL;
+  static constexpr Bits kSignificandBits = 0x000fffffffffffffULL;
 };
 
 template<typename T> struct SelectTrait;
 template<> struct SelectTrait<float> : public FloatTypeTraits {};
 template<> struct SelectTrait<double> : public DoubleTypeTraits {};
 
 /*
  *  This struct contains details regarding the encoding of floating-point
@@ -86,18 +89,18 @@ template<> struct SelectTrait<double> : 
  *  Full details of how floating point number formats are encoded are beyond
  *  the scope of this comment. For more information, see
  *  http://en.wikipedia.org/wiki/IEEE_floating_point
  *  http://en.wikipedia.org/wiki/Floating_point#IEEE_754:_floating_point_in_modern_computers
  */
 template<typename T>
 struct FloatingPoint : public SelectTrait<T>
 {
-  typedef SelectTrait<T> Base;
-  typedef typename Base::Bits Bits;
+  using Base = SelectTrait<T>;
+  using Bits = typename Base::Bits;
 
   static_assert((Base::kSignBit & Base::kExponentBits) == 0,
                 "sign bit shouldn't overlap exponent bits");
   static_assert((Base::kSignBit & Base::kSignificandBits) == 0,
                 "sign bit shouldn't overlap significand bits");
   static_assert((Base::kExponentBits & Base::kSignificandBits) == 0,
                 "exponent bits shouldn't overlap significand bits");
 
@@ -323,48 +326,144 @@ template<typename T>
 static MOZ_ALWAYS_INLINE T
 MinNumberValue()
 {
   typedef FloatingPoint<T> Traits;
   typedef typename Traits::Bits Bits;
   return BitwiseCast<T>(Bits(1));
 }
 
+namespace detail {
+
+template<typename Float, typename SignedInteger>
+inline bool
+NumberEqualsSignedInteger(Float aValue, SignedInteger* aInteger)
+{
+  static_assert(IsSame<Float, float>::value || IsSame<Float, double>::value,
+                "Float must be an IEEE-754 floating point type");
+  static_assert(IsSigned<SignedInteger>::value,
+                "this algorithm only works for signed types: a different one "
+                "will be required for unsigned types");
+  static_assert(sizeof(SignedInteger) >= sizeof(int),
+                "this function *might* require some finessing for signed types "
+                "subject to integral promotion before it can be used on them");
+
+  MOZ_MAKE_MEM_UNDEFINED(aInteger, sizeof(*aInteger));
+
+  // NaNs and infinities are not integers.
+  if (!IsFinite(aValue)) {
+    return false;
+  }
+
+  // Otherwise do direct comparisons against the minimum/maximum |SignedInteger|
+  // values that can be encoded in |Float|.
+
+  constexpr SignedInteger MaxIntValue =
+    std::numeric_limits<SignedInteger>::max(); // e.g. INT32_MAX
+  constexpr SignedInteger MinValue =
+    std::numeric_limits<SignedInteger>::min(); // e.g. INT32_MIN
+
+  static_assert(IsPowerOfTwo(Abs(MinValue)),
+                "MinValue should be is a small power of two, thus exactly "
+                "representable in float/double both");
+
+  constexpr unsigned SignedIntegerWidth = CHAR_BIT * sizeof(SignedInteger);
+  constexpr unsigned ExponentShift = FloatingPoint<Float>::kExponentShift;
+
+  // Careful!  |MaxIntValue| may not be the maximum |SignedInteger| value that
+  // can be encoded in |Float|.  Its |SignedIntegerWidth - 1| bits of precision
+  // may exceed |Float|'s |ExponentShift + 1| bits of precision.  If necessary,
+  // compute the maximum |SignedInteger| that fits in |Float| from IEEE-754
+  // first principles.  (|MinValue| doesn't have this problem because as a
+  // [relatively] small power of two it's always representable in |Float|.)
+
+  // Per C++11 [expr.const]p2, unevaluated subexpressions of logical AND/OR and
+  // conditional expressions *may* contain non-constant expressions, without
+  // making the enclosing expression not constexpr.  MSVC implements this -- but
+  // it sometimes warns about undefined behavior in unevaluated subexpressions.
+  // This bites us if we initialize |MaxValue| the obvious way including an
+  // |uint64_t(1) << (SignedIntegerWidth - 2 - ExponentShift)| subexpression.
+  // Pull that shift-amount out and give it a not-too-huge value when it's in an
+  // unevaluated subexpression.  🙄
+  constexpr unsigned PrecisionExceededShiftAmount =
+    ExponentShift > SignedIntegerWidth - 1
+    ? 0
+    : SignedIntegerWidth - 2 - ExponentShift;
+
+  constexpr SignedInteger MaxValue =
+   ExponentShift > SignedIntegerWidth - 1
+    ? MaxIntValue
+    : SignedInteger((uint64_t(1) << (SignedIntegerWidth - 1)) -
+                    (uint64_t(1) << PrecisionExceededShiftAmount));
+
+  if (static_cast<Float>(MinValue) <= aValue &&
+      aValue <= static_cast<Float>(MaxValue))
+  {
+    auto possible = static_cast<SignedInteger>(aValue);
+    if (static_cast<Float>(possible) == aValue) {
+      *aInteger = possible;
+      return true;
+    }
+  }
+
+  return false;
+}
+
+template<typename Float, typename SignedInteger>
+inline bool
+NumberIsSignedInteger(Float aValue, SignedInteger* aInteger)
+{
+  static_assert(IsSame<Float, float>::value || IsSame<Float, double>::value,
+                "Float must be an IEEE-754 floating point type");
+  static_assert(IsSigned<SignedInteger>::value,
+                "this algorithm only works for signed types: a different one "
+                "will be required for unsigned types");
+  static_assert(sizeof(SignedInteger) >= sizeof(int),
+                "this function *might* require some finessing for signed types "
+                "subject to integral promotion before it can be used on them");
+
+  MOZ_MAKE_MEM_UNDEFINED(aInteger, sizeof(*aInteger));
+
+  if (IsNegativeZero(aValue)) {
+    return false;
+  }
+
+  return NumberEqualsSignedInteger(aValue, aInteger);
+}
+
+} // namespace detail
+
 /**
- * If aValue is equal to some int32_t value, set *aInt32 to that value and
- * return true; otherwise return false.
+ * If |aValue| is identical to some |int32_t| value, set |*aInt32| to that value
+ * and return true.  Otherwise return false, leaving |*aInt32| in an
+ * indeterminate state.
  *
- * Note that negative zero is "equal" to zero here. To test whether a value can
- * be losslessly converted to int32_t and back, use NumberIsInt32 instead.
+ * This method returns false for negative zero.  If you want to consider -0 to
+ * be 0, use NumberEqualsInt32 below.
+ */
+template<typename T>
+static MOZ_ALWAYS_INLINE bool
+NumberIsInt32(T aValue, int32_t* aInt32)
+{
+  return detail::NumberIsSignedInteger(aValue, aInt32);
+}
+
+/**
+ * If |aValue| is equal to some int32_t value (where -0 and +0 are considered
+ * equal), set |*aInt32| to that value and return true.  Otherwise return false,
+ * leaving |*aInt32| in an indeterminate state.
+ *
+ * |NumberEqualsInt32(-0.0, ...)| will return true.  To test whether a value can
+ * be losslessly converted to |int32_t| and back, use NumberIsInt32 above.
  */
 template<typename T>
 static MOZ_ALWAYS_INLINE bool
 NumberEqualsInt32(T aValue, int32_t* aInt32)
 {
-  /*
-   * XXX Casting a floating-point value that doesn't truncate to int32_t, to
-   *     int32_t, induces undefined behavior.  We should definitely fix this
-   *     (bug 744965), but as apparently it "works" in practice, it's not a
-   *     pressing concern now.
-   */
-  return aValue == (*aInt32 = int32_t(aValue));
-}
-
-/**
- * If d can be converted to int32_t and back to an identical double value,
- * set *aInt32 to that value and return true; otherwise return false.
- *
- * The difference between this and NumberEqualsInt32 is that this method returns
- * false for negative zero.
- */
-template<typename T>
-static MOZ_ALWAYS_INLINE bool
-NumberIsInt32(T aValue, int32_t* aInt32)
-{
-  return !IsNegativeZero(aValue) && NumberEqualsInt32(aValue, aInt32);
+  return detail::NumberEqualsSignedInteger(aValue, aInt32);
 }
 
 /**
  * Computes a NaN value.  Do not use this method if you depend upon a particular
  * NaN value being returned.
  */
 template<typename T>
 static MOZ_ALWAYS_INLINE T
--- a/mfbt/MathAlgorithms.h
+++ b/mfbt/MathAlgorithms.h
@@ -111,20 +111,20 @@ template<> struct AbsReturnType<long> { 
 template<> struct AbsReturnType<long long> { typedef unsigned long long Type; };
 template<> struct AbsReturnType<float> { typedef float Type; };
 template<> struct AbsReturnType<double> { typedef double Type; };
 template<> struct AbsReturnType<long double> { typedef long double Type; };
 
 } // namespace detail
 
 template<typename T>
-inline typename detail::AbsReturnType<T>::Type
+inline constexpr typename detail::AbsReturnType<T>::Type
 Abs(const T aValue)
 {
-  typedef typename detail::AbsReturnType<T>::Type ReturnType;
+  using ReturnType = typename detail::AbsReturnType<T>::Type;
   return aValue >= 0 ? ReturnType(aValue) : ~ReturnType(aValue) + 1;
 }
 
 template<>
 inline float
 Abs<float>(const float aFloat)
 {
   return std::fabs(aFloat);
--- a/mfbt/tests/TestFloatingPoint.cpp
+++ b/mfbt/tests/TestFloatingPoint.cpp
@@ -1,16 +1,17 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/FloatingPoint.h"
 
+#include <float.h>
 #include <math.h>
 
 using mozilla::ExponentComponent;
 using mozilla::FloatingPoint;
 using mozilla::FuzzyEqualsAdditive;
 using mozilla::FuzzyEqualsMultiplicative;
 using mozilla::IsFinite;
 using mozilla::IsInfinite;
@@ -314,23 +315,44 @@ TestDoublesPredicates()
   A(NumberIsInt32(double(INT32_MIN), &i));
   A(i == INT32_MIN);
   A(NumberIsInt32(double(INT32_MAX), &i));
   A(i == INT32_MAX);
   A(NumberEqualsInt32(double(INT32_MIN), &i));
   A(i == INT32_MIN);
   A(NumberEqualsInt32(double(INT32_MAX), &i));
   A(i == INT32_MAX);
+  A(pow(2.0, -1075.0) == 0.0);
+  A(pow(2.0, -1074.0) != 0.0);
+  A(!NumberIsInt32(pow(2.0, -1074.0), &i));
+  A(!NumberIsInt32(2 * pow(2.0, -1074.0), &i));
   A(!NumberIsInt32(0.5, &i));
+  A(1.0 - pow(2.0, -54.0) == 1.0);
+  A(1.0 - pow(2.0, -53.0) != 1.0);
+  A(!NumberIsInt32(1.0 - pow(2.0, -53.0), &i));
+  A(!NumberIsInt32(1.0 - pow(2.0, -52.0), &i));
+  A(1.0 + pow(2.0, -53.0) == 1.0f);
+  A(1.0 + pow(2.0, -52.0) != 1.0f);
+  A(!NumberIsInt32(1.0 + pow(2.0, -52.0), &i));
+  A(!NumberIsInt32(1.5f, &i));
+  A(!NumberIsInt32(-double(2147483649), &i));
+  A(!NumberIsInt32(double(2147483648), &i));
+  A(!NumberIsInt32(-double(1ULL << 52) + 0.5, &i));
+  A(!NumberIsInt32(double(1ULL << 52) - 0.5, &i));
+  A(!NumberIsInt32(double(2147483648), &i));
   A(!NumberIsInt32(double(INT32_MAX) + 0.1, &i));
   A(!NumberIsInt32(double(INT32_MIN) - 0.1, &i));
   A(!NumberIsInt32(NegativeInfinity<double>(), &i));
   A(!NumberIsInt32(PositiveInfinity<double>(), &i));
   A(!NumberIsInt32(UnspecifiedNaN<double>(), &i));
   A(!NumberEqualsInt32(0.5, &i));
+  A(!NumberEqualsInt32(-double(2147483649), &i));
+  A(!NumberEqualsInt32(double(2147483648), &i));
+  A(!NumberEqualsInt32(-double(1ULL << 52) + 0.5, &i));
+  A(!NumberEqualsInt32(double(1ULL << 52) - 0.5, &i));
   A(!NumberEqualsInt32(double(INT32_MAX) + 0.1, &i));
   A(!NumberEqualsInt32(double(INT32_MIN) - 0.1, &i));
   A(!NumberEqualsInt32(NegativeInfinity<double>(), &i));
   A(!NumberEqualsInt32(PositiveInfinity<double>(), &i));
   A(!NumberEqualsInt32(UnspecifiedNaN<double>(), &i));
 }
 
 static void
@@ -396,28 +418,48 @@ TestFloatsPredicates()
   A(i == 0);
   A(!NumberIsInt32(-0.0f, &i));
   A(NumberEqualsInt32(0.0f, &i));
   A(i == 0);
   A(NumberEqualsInt32(-0.0f, &i));
   A(i == 0);
   A(NumberIsInt32(float(INT32_MIN), &i));
   A(i == INT32_MIN);
+  A(NumberIsInt32(float(2147483648 - 128), &i)); // max int32_t fitting in float
+  A(i == 2147483648 - 128);
   A(NumberIsInt32(float(BIG), &i));
   A(i == BIG);
   A(NumberEqualsInt32(float(INT32_MIN), &i));
   A(i == INT32_MIN);
   A(NumberEqualsInt32(float(BIG), &i));
   A(i == BIG);
+  A(powf(2.0f, -150.0f) == 0.0f);
+  A(powf(2.0f, -149.0f) != 0.0f);
+  A(!NumberIsInt32(powf(2.0f, -149.0f), &i));
+  A(!NumberIsInt32(2 * powf(2.0f, -149.0f), &i));
   A(!NumberIsInt32(0.5f, &i));
+  A(1.0f - powf(2.0f, -25.0f) == 1.0f);
+  A(1.0f - powf(2.0f, -24.0f) != 1.0f);
+  A(!NumberIsInt32(1.0f - powf(2.0f, -24.0f), &i));
+  A(!NumberIsInt32(1.0f - powf(2.0f, -23.0f), &i));
+  A(1.0f + powf(2.0f, -24.0f) == 1.0f);
+  A(1.0f + powf(2.0f, -23.0f) != 1.0f);
+  A(!NumberIsInt32(1.0f + powf(2.0f, -23.0f), &i));
+  A(!NumberIsInt32(1.5f, &i));
+  A(!NumberIsInt32(-float(2147483648) - 256, &i));
+  A(!NumberIsInt32(float(2147483648), &i));
+  A(!NumberIsInt32(float(2147483648) + 256, &i));
   A(!NumberIsInt32(float(BIG) + 0.1f, &i));
   A(!NumberIsInt32(NegativeInfinity<float>(), &i));
   A(!NumberIsInt32(PositiveInfinity<float>(), &i));
   A(!NumberIsInt32(UnspecifiedNaN<float>(), &i));
   A(!NumberEqualsInt32(0.5f, &i));
+  A(!NumberEqualsInt32(-float(2147483648 + 256), &i));
+  A(!NumberEqualsInt32(float(2147483648), &i));
+  A(!NumberEqualsInt32(float(2147483648 + 256), &i));
   A(!NumberEqualsInt32(float(BIG) + 0.1f, &i));
   A(!NumberEqualsInt32(NegativeInfinity<float>(), &i));
   A(!NumberEqualsInt32(PositiveInfinity<float>(), &i));
   A(!NumberEqualsInt32(UnspecifiedNaN<float>(), &i));
 }
 
 static void
 TestPredicates()