Reviewers: danno, danno-g,
Message:
Committed patchset #2 manually as r21712.
Description:
Re-land Clusterfuzz identified overflow check needed in dehoisting.
Overflow check needs to be smarter.
BUG=380092
[email protected], [email protected]
LOG=N
Committed: https://code.google.com/p/v8/source/detail?r=21712
Please review this at https://codereview.chromium.org/317963004/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files (+50, -33 lines):
M src/hydrogen-dehoist.cc
M src/hydrogen-instructions.h
A + test/mjsunit/regress/regress-380092.js
Index: src/hydrogen-dehoist.cc
diff --git a/src/hydrogen-dehoist.cc b/src/hydrogen-dehoist.cc
index
fe0ae764ad63dcf3fc6c3ca625ebd90cdea7fcf7..b377498cac3f7d58711fba6561f9c41ded13fbb8
100644
--- a/src/hydrogen-dehoist.cc
+++ b/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
- // overflows when the offset is added to the object header size.
- if (value >= 1 << array_operation->MaxBaseOffsetBits() || value < 0)
return;
+ if (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(static_cast<uint32_t>(value));
+ array_operation->IncreaseBaseOffset(value);
array_operation->SetDehoisted(true);
}
Index: src/hydrogen-instructions.h
diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h
index
d1f2372db689de8059d55f532d8ad26bf3bd661a..1fc800e6ffd6b6e8857c7b86bca00e1858566cf3
100644
--- a/src/hydrogen-instructions.h
+++ b/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;
- virtual int MaxBaseOffsetBits() = 0;
+ // increase_by_value should be non-negative
+ 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_); }
- void IncreaseBaseOffset(uint32_t base_offset) {
- base_offset += BaseOffsetField::decode(bit_field_);
- bit_field_ = BaseOffsetField::update(bit_field_, base_offset);
+ uint32_t base_offset() {
+ int32_t base_offset_value = BaseOffsetField::decode(bit_field_);
+ ASSERT(base_offset_value >= 0);
+ return static_cast<uint32_t>(base_offset_value);
}
- virtual int MaxBaseOffsetBits() {
- return kBitsForBaseOffset;
+ bool CanIncreaseBaseOffset(int32_t increase_by_value) {
+ 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_; }
- void IncreaseBaseOffset(uint32_t base_offset) {
- base_offset_ += base_offset;
+ uint32_t base_offset() {
+ ASSERT(base_offset_ >= 0);
+ return static_cast<uint32_t>(base_offset_);
+ }
+ bool CanIncreaseBaseOffset(int32_t increase_by_value) {
+ ASSERT(increase_by_value >= 0);
+ // Guard against overflow
+ return (increase_by_value + base_offset_) >= 0;
}
- virtual int MaxBaseOffsetBits() {
- return 31 - ElementsKindToShiftSize(elements_kind_);
+ 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;
Index: test/mjsunit/regress/regress-380092.js
diff --git
a/test/mjsunit/regress/regress-no-dummy-use-for-arguments-object.js
b/test/mjsunit/regress/regress-380092.js
similarity index 56%
copy from test/mjsunit/regress/regress-no-dummy-use-for-arguments-object.js
copy to test/mjsunit/regress/regress-380092.js
index
658d776ea3b80cd00d256a5514603bdb3df1f179..a7634355ca5d0db9a867cb691b05129b984911f8
100644
--- a/test/mjsunit/regress/regress-no-dummy-use-for-arguments-object.js
+++ b/test/mjsunit/regress/regress-380092.js
@@ -4,18 +4,11 @@
// Flags: --allow-natives-syntax
-function g() {
- arguments.length;
+function many_hoist(o, index) {
+ return o[index + 33554427];
}
-var global = "";
-
-function f() {
- global.dummy = this;
- g({});
-}
-
-f();
-f();
-%OptimizeFunctionOnNextCall(f);
-f();
+var obj = {};
+many_hoist(obj, 0);
+%OptimizeFunctionOnNextCall(many_hoist);
+many_hoist(obj, 5);
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
