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

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);
 

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);
 

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) {

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.

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;

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);
 

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);

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);

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);
 

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);
 

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