Re-land Clusterfuzz identified overflow check needed in dehoisting.

Overflow check needs to be smarter.

BUG=380092
R=danno@google.com, danno@chromium.org
LOG=N

Review URL: https://codereview.chromium.org/317963004

git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21712 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/hydrogen-dehoist.cc

@@ -28,15 +28,25 @@ static void DehoistArrayIndex(ArrayInstructionInterface* array_operation) {
   if (!constant->HasInteger32Value()) return;
   int32_t sign = binary_operation->IsSub() ? -1 : 1;
   int32_t value = constant->Integer32Value() * sign;
-  // We limit offset values to 30 bits because we want to avoid the risk of
+  if (value < 0) return;
-  // overflows when the offset is added to the object header size.
+
-  if (value >= 1 << array_operation->MaxBaseOffsetBits() || value < 0) return;
+  // Check for overflow.
+  // TODO(mvstanton): replace with safe_math.h operations when that code is
+  // integrated.
+  int32_t shift_amount =
+      1 << ElementsKindToShiftSize(array_operation->elements_kind());
+  int32_t multiplication_result = value * shift_amount;
+  if ((multiplication_result >> shift_amount) != value) return;
+  value = multiplication_result;
+
+  // Ensure that the array operation can add value to existing base offset
+  // without overflowing.
+  if (!array_operation->CanIncreaseBaseOffset(value)) return;
   array_operation->SetKey(subexpression);
   if (binary_operation->HasNoUses()) {
     binary_operation->DeleteAndReplaceWith(NULL);
   }
-  value <<= ElementsKindToShiftSize(array_operation->elements_kind());
+  array_operation->IncreaseBaseOffset(value);
-  array_operation->IncreaseBaseOffset(static_cast<uint32_t>(value));
   array_operation->SetDehoisted(true);
 }
 

src/hydrogen-instructions.h

@@ -6404,8 +6404,9 @@ class ArrayInstructionInterface {
   virtual HValue* GetKey() = 0;
   virtual void SetKey(HValue* key) = 0;
   virtual ElementsKind elements_kind() const = 0;
-  virtual void IncreaseBaseOffset(uint32_t base_offset) = 0;
+  // increase_by_value should be non-negative
-  virtual int MaxBaseOffsetBits() = 0;
+  virtual bool CanIncreaseBaseOffset(int32_t increase_by_value) = 0;
+  virtual void IncreaseBaseOffset(int32_t increase_by_value) = 0;
   virtual bool IsDehoisted() = 0;
   virtual void SetDehoisted(bool is_dehoisted) = 0;
   virtual ~ArrayInstructionInterface() { }
@@ -6451,13 +6452,20 @@ class HLoadKeyed V8_FINAL
     return OperandAt(2);
   }
   bool HasDependency() const { return OperandAt(0) != OperandAt(2); }
-  uint32_t base_offset() { return BaseOffsetField::decode(bit_field_); }
+  uint32_t base_offset() {
-  void IncreaseBaseOffset(uint32_t base_offset) {
+    int32_t base_offset_value = BaseOffsetField::decode(bit_field_);
-    base_offset += BaseOffsetField::decode(bit_field_);
+    ASSERT(base_offset_value >= 0);
-    bit_field_ = BaseOffsetField::update(bit_field_, base_offset);
+    return static_cast<uint32_t>(base_offset_value);
   }
-  virtual int MaxBaseOffsetBits() {
+  bool CanIncreaseBaseOffset(int32_t increase_by_value) {
-    return kBitsForBaseOffset;
+    ASSERT(increase_by_value >= 0);
+    int32_t new_value = BaseOffsetField::decode(bit_field_) + increase_by_value;
+    return (new_value >= 0 && BaseOffsetField::is_valid(new_value));
+  }
+  void IncreaseBaseOffset(int32_t increase_by_value) {
+    ASSERT(increase_by_value >= 0);
+    increase_by_value += BaseOffsetField::decode(bit_field_);
+    bit_field_ = BaseOffsetField::update(bit_field_, increase_by_value);
   }
   HValue* GetKey() { return key(); }
   void SetKey(HValue* key) { SetOperandAt(1, key); }
@@ -6607,7 +6615,7 @@ class HLoadKeyed V8_FINAL
     public BitField<LoadKeyedHoleMode, kStartHoleMode, kBitsForHoleMode>
     {};  // NOLINT
   class BaseOffsetField:
-    public BitField<uint32_t, kStartBaseOffset, kBitsForBaseOffset>
+    public BitField<int32_t, kStartBaseOffset, kBitsForBaseOffset>
     {};  // NOLINT
   class IsDehoistedField:
     public BitField<bool, kStartIsDehoisted, kBitsForIsDehoisted>
@@ -6921,12 +6929,18 @@ class HStoreKeyed V8_FINAL
   }
   StoreFieldOrKeyedMode store_mode() const { return store_mode_; }
   ElementsKind elements_kind() const { return elements_kind_; }
-  uint32_t base_offset() { return base_offset_; }
+  uint32_t base_offset() {
-  void IncreaseBaseOffset(uint32_t base_offset) {
+    ASSERT(base_offset_ >= 0);
-    base_offset_ += base_offset;
+    return static_cast<uint32_t>(base_offset_);
   }
-  virtual int MaxBaseOffsetBits() {
+  bool CanIncreaseBaseOffset(int32_t increase_by_value) {
-    return 31 - ElementsKindToShiftSize(elements_kind_);
+    ASSERT(increase_by_value >= 0);
+    // Guard against overflow
+    return (increase_by_value + base_offset_) >= 0;
+  }
+  void IncreaseBaseOffset(int32_t increase_by_value) {
+    ASSERT(increase_by_value >= 0);
+    base_offset_ += increase_by_value;
   }
   HValue* GetKey() { return key(); }
   void SetKey(HValue* key) { SetOperandAt(1, key); }
@@ -7017,7 +7031,7 @@ class HStoreKeyed V8_FINAL
   }
 
   ElementsKind elements_kind_;
-  uint32_t base_offset_;
+  int32_t base_offset_;
   bool is_dehoisted_ : 1;
   bool is_uninitialized_ : 1;
   StoreFieldOrKeyedMode store_mode_: 1;

test/mjsunit/regress/regress-380092.js

@@ -0,0 +1,14 @@
+// Copyright 2014 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+function many_hoist(o, index) {
+  return o[index + 33554427];
+}
+
+var obj = {};
+many_hoist(obj, 0);
+%OptimizeFunctionOnNextCall(many_hoist);
+many_hoist(obj, 5);