Revert "[Layout] Make LayoutUnit::Min() be -LayoutUnit::Max()"

This reverts commit b8ad61c0.

Reason for revert:
  The operators for +,-,+=,-= used base::Clamp{Add,Sub} which didn't
  apply the new limits. As such it was possible for the code to get
  into a state where:
  LayoutUnit test = LayoutUnit::Min();
  test -= LayoutUnit(1);
  LOG(INFO) << "result: " << (test < LayoutUnit::Min());
  The above would log "true".

  Code which operated near these limits was now fragile.
  Causing crbug.com/989742.

Original change's description:
> [Layout] Make LayoutUnit::Min() be -LayoutUnit::Max()
> 
> Some table code adds positive size to negative padding to get final
> size. A pathological page with padding and size of equal magnitudes
> could saturate size to LayoutUnit::Max() and padding to
> LayoutUnit::Min(), causing the result to be -1 instead of 0.
> 
> Bug: 966564
> Change-Id: I1b48a13691c63c916d705e0fd62831c63f1a3363
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1639481
> Commit-Queue: David Grogan <dgrogan@chromium.org>
> Reviewed-by: Emil A Eklund <eae@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#670977}

TBR=dgrogan@chromium.org,eae@chromium.org,cavalcantii@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 966564, 989742
Change-Id: Ic363c34409eda992dbf1dc3b4b4b56d5ea715887
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1730527Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: David Grogan <dgrogan@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#684057}

third_party/blink/renderer/core/layout/layout_table_section_test.cc

@@ -398,20 +398,6 @@ TEST_F(LayoutTableSectionTest, RowCollapseNegativeHeightCrash) {
   )HTML");
 }
 
-TEST_F(LayoutTableSectionTest, RowCollapseSaturation) {
-  // When a collapsed row's height saturates LayoutUnit, we'd set the height to
-  // -1/64, triggering a downstream DCHECK(height >= 0). After a little trial
-  // and error, a huge line-height was the only way I could reproduce this.
-  SetBodyInnerHTML(R"HTML(
-    <div style="display:table-row; visibility:collapse; line-height:279999999%">
-      <div style="display:table-cell;">
-        <div style="position:fixed"></div>
-        No crash = pass.
-      </div>
-    </div>
-  )HTML");
-}
-
 }  // anonymous namespace
 
 }  // namespace blink

third_party/blink/renderer/platform/geometry/layout_unit.h

@@ -58,15 +58,31 @@ static const unsigned kLayoutUnitFractionalBits = 6;
 static const int kFixedPointDenominator = 1 << kLayoutUnitFractionalBits;
 
 const int kIntMaxForLayoutUnit = INT_MAX / kFixedPointDenominator;
-const int kIntMinForLayoutUnit = -kIntMaxForLayoutUnit;
+const int kIntMinForLayoutUnit = INT_MIN / kFixedPointDenominator;
 
+#if defined(ARCH_CPU_ARM_FAMILY) && defined(ARCH_CPU_32_BITS) && \
+    defined(COMPILER_GCC) && !defined(OS_NACL) && __OPTIMIZE__
+inline int GetMaxSaturatedSetResultForTesting() {
+  // For ARM Asm version the set function maxes out to the biggest
+  // possible integer part with the fractional part zero'd out.
+  // e.g. 0x7fffffc0.
+  return std::numeric_limits<int>::max() & ~(kFixedPointDenominator - 1);
+}
+
+inline int GetMinSaturatedSetResultForTesting() {
+  return std::numeric_limits<int>::min();
+}
+#else
 ALWAYS_INLINE int GetMaxSaturatedSetResultForTesting() {
+  // For C version the set function maxes out to max int, this differs from
+  // the ARM asm version.
   return std::numeric_limits<int>::max();
 }
 
 ALWAYS_INLINE int GetMinSaturatedSetResultForTesting() {
-  return -GetMaxSaturatedSetResultForTesting();
+  return std::numeric_limits<int>::min();
 }
+#endif  // CPU(ARM) && COMPILER(GCC)
 
 // TODO(thakis): Remove these two lines once http://llvm.org/PR26504 is resolved
 class PLATFORM_EXPORT LayoutUnit;
@@ -86,54 +102,34 @@ class LayoutUnit {
   }
   constexpr explicit LayoutUnit(uint64_t value)
       : value_(base::saturated_cast<int>(value * kFixedPointDenominator)) {}
-
-  template <typename T>
-  // As long as std::numeric_limits<T>::max() <= -std::numeric_limits<T>::min(),
-  // this class will make  min and max be of equal magnitude.
-  struct SymmetricLimits {
-    static constexpr T NaN() {
-      return std::numeric_limits<T>::has_quiet_NaN
-                 ? std::numeric_limits<T>::quiet_NaN()
-                 : T();
-    }
-    static constexpr T max() { return std::numeric_limits<T>::max(); }
-    static constexpr T Overflow() { return max(); }
-    static constexpr T lowest() { return -max(); }
-    static constexpr T Underflow() { return lowest(); }
-  };
-
   constexpr explicit LayoutUnit(float value)
-      : value_(base::saturated_cast<int, SymmetricLimits>(
-            value * kFixedPointDenominator)) {}
+      : value_(base::saturated_cast<int>(value * kFixedPointDenominator)) {}
   constexpr explicit LayoutUnit(double value)
-      : value_(base::saturated_cast<int, SymmetricLimits>(
-            value * kFixedPointDenominator)) {}
+      : value_(base::saturated_cast<int>(value * kFixedPointDenominator)) {}
 
   static LayoutUnit FromFloatCeil(float value) {
     LayoutUnit v;
-    v.value_ = base::saturated_cast<int, SymmetricLimits>(
-        ceilf(value * kFixedPointDenominator));
+    v.value_ = base::saturated_cast<int>(ceilf(value * kFixedPointDenominator));
     return v;
   }
 
   static LayoutUnit FromFloatFloor(float value) {
     LayoutUnit v;
-    v.value_ = base::saturated_cast<int, SymmetricLimits>(
-        floorf(value * kFixedPointDenominator));
+    v.value_ =
+        base::saturated_cast<int>(floorf(value * kFixedPointDenominator));
     return v;
   }
 
   static LayoutUnit FromFloatRound(float value) {
     LayoutUnit v;
-    v.value_ = base::saturated_cast<int, SymmetricLimits>(
-        roundf(value * kFixedPointDenominator));
+    v.value_ =
+        base::saturated_cast<int>(roundf(value * kFixedPointDenominator));
     return v;
   }
 
   static LayoutUnit FromDoubleRound(double value) {
     LayoutUnit v;
-    v.value_ = base::saturated_cast<int, SymmetricLimits>(
-        round(value * kFixedPointDenominator));
+    v.value_ = base::saturated_cast<int>(round(value * kFixedPointDenominator));
     return v;
   }
 
@@ -168,7 +164,7 @@ class LayoutUnit {
   constexpr int RawValue() const { return value_; }
   inline void SetRawValue(int value) { value_ = value; }
   void SetRawValue(int64_t value) {
-    REPORT_OVERFLOW(value > -std::numeric_limits<int>::max() &&
+    REPORT_OVERFLOW(value > std::numeric_limits<int>::min() &&
                     value < std::numeric_limits<int>::max());
     value_ = static_cast<int>(value);
   }
@@ -220,7 +216,7 @@ class LayoutUnit {
 
   bool MightBeSaturated() const {
     return RawValue() == std::numeric_limits<int>::max() ||
-           RawValue() == -std::numeric_limits<int>::max();
+           RawValue() == std::numeric_limits<int>::min();
   }
 
   static float Epsilon() { return 1.0f / kFixedPointDenominator; }
@@ -239,12 +235,12 @@ class LayoutUnit {
   }
   static constexpr LayoutUnit Min() {
     LayoutUnit m;
-    m.value_ = -std::numeric_limits<int>::max();
+    m.value_ = std::numeric_limits<int>::min();
     return m;
   }
 
   // Versions of max/min that are slightly smaller/larger than max/min() to
-  // allow for rounding without overflowing.
+  // allow for roinding without overflowing.
   static const LayoutUnit NearlyMax() {
     LayoutUnit m;
     m.value_ = std::numeric_limits<int>::max() - kFixedPointDenominator / 2;
@@ -252,7 +248,7 @@ class LayoutUnit {
   }
   static const LayoutUnit NearlyMin() {
     LayoutUnit m;
-    m.value_ = -std::numeric_limits<int>::max() + kFixedPointDenominator / 2;
+    m.value_ = std::numeric_limits<int>::min() + kFixedPointDenominator / 2;
     return m;
   }
 
@@ -274,11 +270,67 @@ class LayoutUnit {
            std::numeric_limits<int>::max() / kFixedPointDenominator;
   }
 
+#if defined(ARCH_CPU_ARM_FAMILY) && defined(ARCH_CPU_32_BITS) && \
+    defined(COMPILER_GCC) && !defined(OS_NACL) && __OPTIMIZE__
+  // If we're building ARM 32-bit on GCC we replace the C++ versions with some
+  // native ARM assembly for speed.
+  inline void SaturatedSet(int value) {
+    // Figure out how many bits are left for storing the integer part of
+    // the fixed point number, and saturate our input to that
+    enum { Saturate = 32 - kLayoutUnitFractionalBits };
+
+    int result;
+
+    // The following ARM code will Saturate the passed value to the number of
+    // bits used for the whole part of the fixed point representation, then
+    // shift it up into place. This will result in the low
+    // <kLayoutUnitFractionalBits> bits all being 0's. When the value saturates
+    // this gives a different result to from the C++ case; in the C++ code a
+    // saturated value has all the low bits set to 1 (for a +ve number at
+    // least). This cannot be done rapidly in ARM ... we live with the
+    // difference, for the sake of speed.
+
+    asm("ssat %[output],%[saturate],%[value]\n\t"
+        "lsl  %[output],%[shift]"
+        : [output] "=r"(result)
+        : [value] "r"(value), [saturate] "n"(Saturate),
+          [shift] "n"(kLayoutUnitFractionalBits));
+
+    value_ = result;
+  }
+
+  inline void SaturatedSet(unsigned value) {
+    // Here we are being passed an unsigned value to saturate,
+    // even though the result is returned as a signed integer. The ARM
+    // instruction for unsigned saturation therefore needs to be given one
+    // less bit (i.e. the sign bit) for the saturation to work correctly; hence
+    // the '31' below.
+    enum { Saturate = 31 - kLayoutUnitFractionalBits };
+
+    // The following ARM code will Saturate the passed value to the number of
+    // bits used for the whole part of the fixed point representation, then
+    // shift it up into place. This will result in the low
+    // <kLayoutUnitFractionalBits> bits all being 0's. When the value saturates
+    // this gives a different result to from the C++ case; in the C++ code a
+    // saturated value has all the low bits set to 1. This cannot be done
+    // rapidly in ARM, so we live with the difference, for the sake of speed.
+
+    int result;
+
+    asm("usat %[output],%[saturate],%[value]\n\t"
+        "lsl  %[output],%[shift]"
+        : [output] "=r"(result)
+        : [value] "r"(value), [saturate] "n"(Saturate),
+          [shift] "n"(kLayoutUnitFractionalBits));
+
+    value_ = result;
+  }
+#else
   ALWAYS_INLINE void SaturatedSet(int value) {
     if (value > kIntMaxForLayoutUnit)
       value_ = std::numeric_limits<int>::max();
     else if (value < kIntMinForLayoutUnit)
-      value_ = -std::numeric_limits<int>::max();
+      value_ = std::numeric_limits<int>::min();
     else
       value_ = static_cast<unsigned>(value) << kLayoutUnitFractionalBits;
   }
@@ -289,6 +341,7 @@ class LayoutUnit {
     else
       value_ = value << kLayoutUnitFractionalBits;
   }
+#endif  // CPU(ARM) && COMPILER(GCC)
 
   int value_;
 };
@@ -422,7 +475,7 @@ constexpr bool operator==(const float a, const LayoutUnit& b) {
 }
 
 // For multiplication that's prone to overflow, this bounds it to
-// LayoutUnit::Max() and ::Min()
+// LayoutUnit::max() and ::min()
 inline LayoutUnit BoundedMultiply(const LayoutUnit& a, const LayoutUnit& b) {
   int64_t result = static_cast<int64_t>(a.RawValue()) *
                    static_cast<int64_t>(b.RawValue()) / kFixedPointDenominator;

third_party/blink/renderer/platform/geometry/layout_unit_test.cc

@@ -75,7 +75,7 @@ TEST(LayoutUnitTest, LayoutUnitInt) {
             LayoutUnit(kIntMaxForLayoutUnit + 100).RawValue());
   EXPECT_EQ((kIntMaxForLayoutUnit - 100) << kLayoutUnitFractionalBits,
             LayoutUnit(kIntMaxForLayoutUnit - 100).RawValue());
-  EXPECT_EQ(-max_internal_representation,
+  EXPECT_EQ(GetMinSaturatedSetResultForTesting(),
             LayoutUnit(kIntMinForLayoutUnit).RawValue());
   EXPECT_EQ(GetMinSaturatedSetResultForTesting(),
             LayoutUnit(kIntMinForLayoutUnit - 100).RawValue());
@@ -83,10 +83,6 @@ TEST(LayoutUnitTest, LayoutUnitInt) {
   // multiplication instead of direct shifting here.
   EXPECT_EQ((kIntMinForLayoutUnit + 100) * (1 << kLayoutUnitFractionalBits),
             LayoutUnit(kIntMinForLayoutUnit + 100).RawValue());
-  // A positive overflowed LayoutUnit should be of equal magnitude to a negative
-  // overflowed LayoutUnit.
-  EXPECT_EQ(LayoutUnit(), LayoutUnit(kIntMaxForLayoutUnit + 100) +
-                              LayoutUnit(-kIntMaxForLayoutUnit - 100));
 }
 
 TEST(LayoutUnitTest, LayoutUnitUnsigned) {
@@ -141,9 +137,12 @@ TEST(LayoutUnitTest, LayoutUnitRounding) {
   // The fractional part of LayoutUnit::Max() is 0x3f, so it should round up.
   EXPECT_EQ(((std::numeric_limits<int>::max() / kFixedPointDenominator) + 1),
             LayoutUnit::Max().Round());
-  // Similarly, LayoutUnit::Min() should round down.
-  EXPECT_EQ((-std::numeric_limits<int>::max() / kFixedPointDenominator) - 1,
-            LayoutUnit::Min().Round());
+  // The fractional part of LayoutUnit::Min() is 0, so the next bigger possible
+  // value should round down.
+  LayoutUnit epsilon;
+  epsilon.SetRawValue(1);
+  EXPECT_EQ(((std::numeric_limits<int>::min() / kFixedPointDenominator)),
+            (LayoutUnit::Min() + epsilon).Round());
 }
 
 TEST(LayoutUnitTest, LayoutUnitSnapSizeToPixel) {
@@ -306,20 +305,6 @@ TEST(LayoutUnitTest, LayoutUnitFloatOverflow) {
   EXPECT_EQ(kIntMinForLayoutUnit, LayoutUnit(-176972000.0f).ToInt());
   EXPECT_EQ(kIntMaxForLayoutUnit, LayoutUnit(176972000.0).ToInt());
   EXPECT_EQ(kIntMinForLayoutUnit, LayoutUnit(-176972000.0).ToInt());
-
-  EXPECT_EQ(GetMinSaturatedSetResultForTesting(),
-            LayoutUnit::FromFloatCeil(-176972000.0f).RawValue());
-  EXPECT_EQ(GetMinSaturatedSetResultForTesting(),
-            LayoutUnit::FromFloatFloor(-176972000.0f).RawValue());
-  EXPECT_EQ(GetMinSaturatedSetResultForTesting(),
-            LayoutUnit::FromFloatRound(-176972000.0f).RawValue());
-
-  EXPECT_EQ(GetMinSaturatedSetResultForTesting(),
-            LayoutUnit::FromDoubleRound(-176972000.0).RawValue());
-
-  // A positive overflowed LayoutUnit should be of equal magnitude to a negative
-  // overflowed LayoutUnit.
-  EXPECT_EQ(LayoutUnit(), LayoutUnit(176972000.0) + LayoutUnit(-176972000.0));
 }
 
 TEST(LayoutUnitTest, UnaryMinus) {
@@ -327,8 +312,12 @@ TEST(LayoutUnitTest, UnaryMinus) {
   EXPECT_EQ(LayoutUnit(999), -LayoutUnit(-999));
   EXPECT_EQ(LayoutUnit(-999), -LayoutUnit(999));
 
-  // -LayoutUnit::Min() and LayoutUnit::Max() are equal.
-  EXPECT_EQ(LayoutUnit::Min(), -LayoutUnit::Max());
+  LayoutUnit negative_max;
+  negative_max.SetRawValue(LayoutUnit::Min().RawValue() + 1);
+  EXPECT_EQ(negative_max, -LayoutUnit::Max());
+  EXPECT_EQ(LayoutUnit::Max(), -negative_max);
+
+  // -LayoutUnit::min() is saturated to LayoutUnit::max()
   EXPECT_EQ(LayoutUnit::Max(), -LayoutUnit::Min());
 }