Correct handling of arrays with callbacks in the prototype chain.

src/ia32/ic-ia32.cc

Index: src/ia32/ic-ia32.cc
diff --git a/src/ia32/ic-ia32.cc b/src/ia32/ic-ia32.cc
index f8e4ea53d0f78cb32f63e86b30343f85c857f42c..97445d01f27424839b8c8ef669cee5a15c891c7f 100644
--- a/src/ia32/ic-ia32.cc
+++ b/src/ia32/ic-ia32.cc
@@ -711,6 +711,34 @@ void KeyedStoreIC::GenerateNonStrictArguments(MacroAssembler* masm) {
 }
 
 
+static void HasElementCallbacksInPrototypeChain(

[danno, 2013/10/23 12:22:11]

This (and similar code in other implementations) belongs in the
platform-specific macro assembler.

[mvstanton, 2013/10/30 10:42:42]

Done.

+    MacroAssembler* masm,
+    Label* found) {
+  Factory* factory = masm->isolate()->factory();
+  Label loop, not_found;
+
+  // ebx contained elements pointer.
+  __ mov(ebx, FieldOperand(edx, HeapObject::kMapOffset));
+
+  // Loop based on the map going up the prototype chain.
+  __ bind(&loop);
+  __ test(FieldOperand(ebx, Map::kBitField4Offset),
+          Immediate(Smi::FromInt(Map::HasElementCallbacks::kMask)));
+  __ j(not_zero, found);
+
+  // Next map
+  __ mov(ebx, FieldOperand(ebx, Map::kPrototypeOffset));
+  __ cmp(ebx, Immediate(factory->null_value()));
+  __ j(equal, &not_found);
+  __ mov(ebx, FieldOperand(ebx, HeapObject::kMapOffset));
+  __ jmp(&loop);
+
+  // Restore ebx
+  __ bind(&not_found);
+  __ mov(ebx, FieldOperand(edx, JSObject::kElementsOffset));
+}
+
+
 static void KeyedStoreGenerateGenericHelper(

[danno, 2013/10/23 12:22:11]

I wish *so* badly that we generaed this in Hydrogen. It would make this hackery
unnecessary.

[mvstanton, 2013/10/30 10:42:42]

I've added this to the TODO list.

     MacroAssembler* masm,
     Label* fast_object,
@@ -733,9 +761,22 @@ static void KeyedStoreGenerateGenericHelper(
     __ cmp(edi, masm->isolate()->factory()->fixed_array_map());
     __ j(not_equal, fast_double);
   }
+
+  // HOLECHECK: guards "A[i] = V"
+  // We have to go to the runtime if the current value is undefined because
+  // there may be a callback on the element
+  Label holecheck_passed1;
+  __ cmp(CodeGenerator::FixedArrayElementOperand(ebx, ecx),
+         masm->isolate()->factory()->the_hole_value());
+  __ j(not_equal, &holecheck_passed1);
+  HasElementCallbacksInPrototypeChain(masm, slow);
+
+  __ bind(&holecheck_passed1);
+
   // Smi stores don't require further checks.
   Label non_smi_value;
   __ JumpIfNotSmi(eax, &non_smi_value);
+

[danno, 2013/10/23 12:22:11]

nit: please remove the extraneous whitespace change

[mvstanton, 2013/10/30 10:42:42]

Done.

   if (increment_length == kIncrementLength) {
     // Add 1 to receiver->length.
     __ add(FieldOperand(edx, JSArray::kLengthOffset),
@@ -773,6 +814,15 @@ static void KeyedStoreGenerateGenericHelper(
     // If the value is a number, store it as a double in the FastDoubleElements
     // array.
   }
+
+  // HOLECHECK: guards "A[i] double hole?"
+  // We have to see if the double version of the hole is present. If so
+  // go to the runtime.
+  uint32_t offset = FixedDoubleArray::kHeaderSize + sizeof(kHoleNanLower32);
+  __ cmp(FieldOperand(ebx, ecx, times_4, offset), Immediate(kHoleNanUpper32));
+  __ j(not_equal, &fast_double_without_map_check);
+  HasElementCallbacksInPrototypeChain(masm, slow);
+
   __ bind(&fast_double_without_map_check);
   __ StoreNumberToDoubleElements(eax, ebx, ecx, edi, xmm0,
                                  &transition_double_elements, false);
@@ -794,6 +844,7 @@ static void KeyedStoreGenerateGenericHelper(
 
   // Value is a double. Transition FAST_SMI_ELEMENTS -> FAST_DOUBLE_ELEMENTS
   // and complete the store.
+

[danno, 2013/10/23 12:22:11]

nit: please remove the extraneous whitespace change

[mvstanton, 2013/10/30 10:42:42]

Done.

   __ LoadTransitionedArrayMapConditional(FAST_SMI_ELEMENTS,
                                          FAST_DOUBLE_ELEMENTS,
                                          ebx,
@@ -806,6 +857,7 @@ static void KeyedStoreGenerateGenericHelper(
   __ jmp(&fast_double_without_map_check);
 
   __ bind(&non_double_value);
+

[danno, 2013/10/23 12:22:11]

nit: please remove the extraneous whitespace change

[mvstanton, 2013/10/30 10:42:42]

Done.

   // Value is not a double, FAST_SMI_ELEMENTS -> FAST_ELEMENTS
   __ LoadTransitionedArrayMapConditional(FAST_SMI_ELEMENTS,
                                          FAST_ELEMENTS,