Fix invalid usage of StoreIC_ArrayLength optimization.

This introduces an additional check into the StoreIC_ArrayLength builtin
checking that the array still has fast properties. Redifinitions of the
length property that would cause it's type or attributes to change, will
switch to slow properties, thereby invalidating said optimization.

R=svenpanne@chromium.org
BUG=v8:1756
TEST=test262

Review URL: http://codereview.chromium.org/8895025

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

diff --git a/src/arm/ic-arm.cc b/src/arm/ic-arm.cc
index abbac99..219d354 100644 (file)
--- a/src/arm/ic-arm.cc
+++ b/src/arm/ic-arm.cc
@@ -1469,11 +1469,10 @@ void StoreIC::GenerateArrayLength(MacroAssembler* masm) {
   //  -- lr    : return address
   // -----------------------------------
   //
-  // This accepts as a receiver anything JSObject::SetElementsLength accepts
-  // (currently anything except for external and pixel arrays which means
-  // anything with elements of FixedArray type.), but currently is restricted
-  // to JSArray.
-  // Value must be a number, but only smis are accepted as the most common case.
+  // This accepts as a receiver anything JSArray::SetElementsLength accepts
+  // (currently anything except for external arrays which means anything with
+  // elements of FixedArray type).  Value must be a number, but only smis are
+  // accepted as the most common case.
 
   Label miss;
 
@@ -1495,6 +1494,13 @@ void StoreIC::GenerateArrayLength(MacroAssembler* masm) {
   __ CompareObjectType(scratch, scratch, scratch, FIXED_ARRAY_TYPE);
   __ b(ne, &miss);
 
+  // Check that the array has fast properties, otherwise the length
+  // property might have been redefined.
+  __ ldr(scratch, FieldMemOperand(receiver, JSArray::kPropertiesOffset));
+  __ ldr(scratch, FieldMemOperand(scratch, FixedArray::kMapOffset));
+  __ CompareRoot(scratch, Heap::kHashTableMapRootIndex);
+  __ b(eq, &miss);
+
   // Check that value is a smi.
   __ JumpIfNotSmi(value, &miss);
 

diff --git a/src/ia32/ic-ia32.cc b/src/ia32/ic-ia32.cc
index a83db12..82bb02a 100644 (file)
--- a/src/ia32/ic-ia32.cc
+++ b/src/ia32/ic-ia32.cc
@@ -1374,10 +1374,10 @@ void StoreIC::GenerateArrayLength(MacroAssembler* masm) {
   //  -- esp[0] : return address
   // -----------------------------------
   //
-  // This accepts as a receiver anything JSObject::SetElementsLength accepts
+  // This accepts as a receiver anything JSArray::SetElementsLength accepts
   // (currently anything except for external arrays which means anything with
-  // elements of FixedArray type.), but currently is restricted to JSArray.
-  // Value must be a number, but only smis are accepted as the most common case.
+  // elements of FixedArray type).  Value must be a number, but only smis are
+  // accepted as the most common case.
 
   Label miss;
 
@@ -1399,6 +1399,13 @@ void StoreIC::GenerateArrayLength(MacroAssembler* masm) {
   __ CmpObjectType(scratch, FIXED_ARRAY_TYPE, scratch);
   __ j(not_equal, &miss);
 
+  // Check that the array has fast properties, otherwise the length
+  // property might have been redefined.
+  __ mov(scratch, FieldOperand(receiver, JSArray::kPropertiesOffset));
+  __ CompareRoot(FieldOperand(scratch, FixedArray::kMapOffset),
+                 Heap::kHashTableMapRootIndex);
+  __ j(equal, &miss);
+
   // Check that value is a smi.
   __ JumpIfNotSmi(value, &miss);
 

diff --git a/src/ia32/macro-assembler-ia32.cc b/src/ia32/macro-assembler-ia32.cc
index 2e4cfa4..3356e81 100644 (file)
--- a/src/ia32/macro-assembler-ia32.cc
+++ b/src/ia32/macro-assembler-ia32.cc
@@ -357,6 +357,14 @@ void MacroAssembler::CompareRoot(Register with, Heap::RootListIndex index) {
 }
 
 
+void MacroAssembler::CompareRoot(const Operand& with,
+                                 Heap::RootListIndex index) {
+  // see ROOT_ACCESSOR macro in factory.h
+  Handle<Object> value(&isolate()->heap()->roots_array_start()[index]);
+  cmp(with, value);
+}
+
+
 void MacroAssembler::CmpObjectType(Register heap_object,
                                    InstanceType type,
                                    Register map) {

diff --git a/src/ia32/macro-assembler-ia32.h b/src/ia32/macro-assembler-ia32.h
index 46f99be..47214ea 100644 (file)
--- a/src/ia32/macro-assembler-ia32.h
+++ b/src/ia32/macro-assembler-ia32.h
@@ -308,8 +308,9 @@ class MacroAssembler: public Assembler {
   void SafeSet(Register dst, const Immediate& x);
   void SafePush(const Immediate& x);
 
-  // Compare a register against a known root, e.g. undefined, null, true, ...
+  // Compare against a known root, e.g. undefined, null, true, ...
   void CompareRoot(Register with, Heap::RootListIndex index);
+  void CompareRoot(const Operand& with, Heap::RootListIndex index);
 
   // Compare object type for heap object.
   // Incoming register is heap_object and outgoing register is map.

diff --git a/src/ic.cc b/src/ic.cc
index ad447cc..624ecd7 100644 (file)
--- a/src/ic.cc
+++ b/src/ic.cc
@@ -1272,10 +1272,13 @@ MaybeObject* StoreIC::Store(State state,
     return *value;
   }
 
-  // Use specialized code for setting the length of arrays.
-  if (receiver->IsJSArray()
-      && name->Equals(isolate()->heap()->length_symbol())
-      && Handle<JSArray>::cast(receiver)->AllowsSetElementsLength()) {
+  // Use specialized code for setting the length of arrays with fast
+  // properties.  Slow properties might indicate redefinition of the
+  // length property.
+  if (receiver->IsJSArray() &&
+      name->Equals(isolate()->heap()->length_symbol()) &&
+      Handle<JSArray>::cast(receiver)->AllowsSetElementsLength() &&
+      receiver->HasFastProperties()) {
 #ifdef DEBUG
     if (FLAG_trace_ic) PrintF("[StoreIC : +#length /array]\n");
 #endif
@@ -1879,12 +1882,19 @@ RUNTIME_FUNCTION(MaybeObject*, StoreIC_ArrayLength) {
   NoHandleAllocation nha;
 
   ASSERT(args.length() == 2);
-  JSObject* receiver = JSObject::cast(args[0]);
+  JSArray* receiver = JSArray::cast(args[0]);
   Object* len = args[1];
 
   // The generated code should filter out non-Smis before we get here.
   ASSERT(len->IsSmi());
 
+#ifdef DEBUG
+  // The length property has to be a writable callback property.
+  LookupResult debug_lookup(isolate);
+  receiver->LocalLookup(isolate->heap()->length_symbol(), &debug_lookup);
+  ASSERT(debug_lookup.type() == CALLBACKS && !debug_lookup.IsReadOnly());
+#endif
+
   Object* result;
   { MaybeObject* maybe_result = receiver->SetElementsLength(len);
     if (!maybe_result->ToObject(&result)) return maybe_result;

diff --git a/src/mips/ic-mips.cc b/src/mips/ic-mips.cc
index c240125..1eef55a 100644 (file)
--- a/src/mips/ic-mips.cc
+++ b/src/mips/ic-mips.cc
@@ -1470,11 +1470,10 @@ void StoreIC::GenerateArrayLength(MacroAssembler* masm) {
   //  -- ra    : return address
   // -----------------------------------
   //
-  // This accepts as a receiver anything JSObject::SetElementsLength accepts
-  // (currently anything except for external and pixel arrays which means
-  // anything with elements of FixedArray type.), but currently is restricted
-  // to JSArray.
-  // Value must be a number, but only smis are accepted as the most common case.
+  // This accepts as a receiver anything JSArray::SetElementsLength accepts
+  // (currently anything except for external arrays which means anything with
+  // elements of FixedArray type).  Value must be a number, but only smis are
+  // accepted as the most common case.
 
   Label miss;
 
@@ -1496,6 +1495,10 @@ void StoreIC::GenerateArrayLength(MacroAssembler* masm) {
   __ GetObjectType(scratch, scratch, scratch);
   __ Branch(&miss, ne, scratch, Operand(FIXED_ARRAY_TYPE));
 
+  // Check that the array has fast properties, otherwise the length
+  // property might have been redefined.
+  // TODO(mstarzinger): Port this check to MIPS.
+
   // Check that value is a smi.
   __ JumpIfNotSmi(value, &miss);
 

diff --git a/src/objects-inl.h b/src/objects-inl.h
index d6d6571..7c7f094 100644 (file)
--- a/src/objects-inl.h
+++ b/src/objects-inl.h
@@ -4256,14 +4256,6 @@ bool JSObject::HasIndexedInterceptor() {
 }
 
 
-bool JSObject::AllowsSetElementsLength() {
-  bool result = elements()->IsFixedArray() ||
-      elements()->IsFixedDoubleArray();
-  ASSERT(result == !HasExternalArrayElements());
-  return result;
-}
-
-
 MaybeObject* JSObject::EnsureWritableFastElements() {
   ASSERT(HasFastTypeElements());
   FixedArray* elems = FixedArray::cast(elements());
@@ -4624,6 +4616,13 @@ void JSArray::set_length(Smi* length) {
 }
 
 
+bool JSArray::AllowsSetElementsLength() {
+  bool result = elements()->IsFixedArray() || elements()->IsFixedDoubleArray();
+  ASSERT(result == !HasExternalArrayElements());
+  return result;
+}
+
+
 MaybeObject* JSArray::SetContent(FixedArrayBase* storage) {
   MaybeObject* maybe_result = EnsureCanContainElements(
       storage, ALLOW_COPIED_DOUBLE_ELEMENTS);

diff --git a/src/objects.cc b/src/objects.cc
index 2a56797..2b6f235 100644 (file)
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -8376,7 +8376,7 @@ void JSArray::Expand(int required_size) {
 }
 
 
-MaybeObject* JSObject::SetElementsLength(Object* len) {
+MaybeObject* JSArray::SetElementsLength(Object* len) {
   // We should never end in here with a pixel or external array.
   ASSERT(AllowsSetElementsLength());
   return GetElementsAccessor()->SetLength(this, len);

diff --git a/src/objects.h b/src/objects.h
index 2b18e67..5346585 100644 (file)
--- a/src/objects.h
+++ b/src/objects.h
@@ -1471,7 +1471,6 @@ class JSObject: public JSReceiver {
   inline bool HasExternalDoubleElements();
   bool HasFastArgumentsElements();
   bool HasDictionaryArgumentsElements();
-  inline bool AllowsSetElementsLength();
   inline NumberDictionary* element_dictionary();  // Gets slow elements.
 
   // Requires: HasFastElements().
@@ -1733,9 +1732,6 @@ class JSObject: public JSReceiver {
   bool HasRealElementProperty(uint32_t index);
   bool HasRealNamedCallbackProperty(String* key);
 
-  // Initializes the array to a certain length
-  MUST_USE_RESULT MaybeObject* SetElementsLength(Object* length);
-
   // Get the header size for a JSObject.  Used to compute the index of
   // internal fields as well as the number of internal fields.
   inline int GetHeaderSize();
@@ -7398,6 +7394,10 @@ class JSArray: public JSObject {
   // capacity is non-zero.
   MUST_USE_RESULT MaybeObject* Initialize(int capacity);
 
+  // Initializes the array to a certain length.
+  inline bool AllowsSetElementsLength();
+  MUST_USE_RESULT MaybeObject* SetElementsLength(Object* length);
+
   // Set the content of the array to the content of storage.
   inline MaybeObject* SetContent(FixedArrayBase* storage);
 

diff --git a/src/x64/ic-x64.cc b/src/x64/ic-x64.cc
index b3a9422..0204460 100644 (file)
--- a/src/x64/ic-x64.cc
+++ b/src/x64/ic-x64.cc
@@ -1397,11 +1397,10 @@ void StoreIC::GenerateArrayLength(MacroAssembler* masm) {
   //  -- rsp[0] : return address
   // -----------------------------------
   //
-  // This accepts as a receiver anything JSObject::SetElementsLength accepts
-  // (currently anything except for external and pixel arrays which means
-  // anything with elements of FixedArray type.), but currently is restricted
-  // to JSArray.
-  // Value must be a number, but only smis are accepted as the most common case.
+  // This accepts as a receiver anything JSArray::SetElementsLength accepts
+  // (currently anything except for external arrays which means anything with
+  // elements of FixedArray type).  Value must be a number, but only smis are
+  // accepted as the most common case.
 
   Label miss;
 
@@ -1423,6 +1422,13 @@ void StoreIC::GenerateArrayLength(MacroAssembler* masm) {
   __ CmpObjectType(scratch, FIXED_ARRAY_TYPE, scratch);
   __ j(not_equal, &miss);
 
+  // Check that the array has fast properties, otherwise the length
+  // property might have been redefined.
+  __ movq(scratch, FieldOperand(receiver, JSArray::kPropertiesOffset));
+  __ CompareRoot(FieldOperand(scratch, FixedArray::kMapOffset),
+                 Heap::kHashTableMapRootIndex);
+  __ j(equal, &miss);
+
   // Check that value is a smi.
   __ JumpIfNotSmi(value, &miss);
 

diff --git a/test/test262/test262.status b/test/test262/test262.status
index c7d363d..6024218 100644 (file)
--- a/test/test262/test262.status
+++ b/test/test262/test262.status
@@ -42,14 +42,6 @@ S10.4.2.1_A1: FAIL
 # V8 Bug: http://code.google.com/p/v8/issues/detail?id=1530
 S15.3.3.1_A4: FAIL
 
-# V8 Bug: http://code.google.com/p/v8/issues/detail?id=1756
-15.2.3.6-4-167: FAIL || PASS
-15.2.3.6-4-181: FAIL || PASS
-15.2.3.7-6-a-163: FAIL || PASS
-15.2.3.7-6-a-164: FAIL || PASS
-15.2.3.7-6-a-176: FAIL || PASS
-15.2.3.7-6-a-177: FAIL || PASS
-
 # V8 Bug: http://code.google.com/p/v8/issues/detail?id=1772
 15.2.3.6-4-292-1: FAIL
 15.2.3.6-4-293-2: FAIL