Correct handling of arrays with callbacks in the prototype chain.

src/ia32/ic-ia32.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 693 matching lines @@
   __ mov(unmapped_location, eax);
   __ lea(edi, unmapped_location);
   __ mov(edx, eax);
   __ RecordWrite(ebx, edi, edx, kDontSaveFPRegs);
   __ Ret();
   __ bind(&slow);
   GenerateMiss(masm, MISS);
 }
 
 
+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,
     Label* fast_double,
     Label* slow,
     KeyedStoreCheckMap check_map,
     KeyedStoreIncrementLength increment_length) {
   Label transition_smi_elements;
   Label finish_object_store, non_double_value, transition_double_elements;
   Label fast_double_without_map_check;
   // eax: value
   // ecx: key (a smi)
   // edx: receiver
   // ebx: FixedArray receiver->elements
   // edi: receiver map
   // Fast case: Do the store, could either Object or double.
   __ bind(fast_object);
   if (check_map == kCheckMap) {
     __ mov(edi, FieldOperand(ebx, HeapObject::kMapOffset));
     __ 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),
            Immediate(Smi::FromInt(1)));
   }
   // It's irrelevant whether array is smi-only or not when writing a smi.
   __ mov(CodeGenerator::FixedArrayElementOperand(ebx, ecx), eax);
   __ ret(0);
 
   __ bind(&non_smi_value);
@@ skipping 17 matching lines @@
 
   __ bind(fast_double);
   if (check_map == kCheckMap) {
     // Check for fast double array case. If this fails, call through to the
     // runtime.
     __ cmp(edi, masm->isolate()->factory()->fixed_double_array_map());
     __ j(not_equal, slow);
     // 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);
   if (increment_length == kIncrementLength) {
     // Add 1 to receiver->length.
     __ add(FieldOperand(edx, JSArray::kLengthOffset),
            Immediate(Smi::FromInt(1)));
   }
   __ ret(0);
 
   __ bind(&transition_smi_elements);
   __ mov(ebx, FieldOperand(edx, HeapObject::kMapOffset));
 
   // Transition the array appropriately depending on the value type.
   __ CheckMap(eax,
               masm->isolate()->factory()->heap_number_map(),
               &non_double_value,
               DONT_DO_SMI_CHECK);
 
   // 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,
                                          edi,
                                          slow);
   AllocationSiteMode mode = AllocationSite::GetMode(FAST_SMI_ELEMENTS,
                                                     FAST_DOUBLE_ELEMENTS);
   ElementsTransitionGenerator::GenerateSmiToDouble(masm, mode, slow);
   __ mov(ebx, FieldOperand(edx, JSObject::kElementsOffset));
   __ 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,
                                          ebx,
                                          edi,
                                          slow);
   mode = AllocationSite::GetMode(FAST_SMI_ELEMENTS, FAST_ELEMENTS);
   ElementsTransitionGenerator::GenerateMapChangeElementsTransition(masm, mode,
                                                                    slow);
   __ mov(ebx, FieldOperand(edx, JSObject::kElementsOffset));
@@ skipping 839 matching lines @@
   Condition cc = (check == ENABLE_INLINED_SMI_CHECK)
       ? (*jmp_address == Assembler::kJncShortOpcode ? not_zero : zero)
       : (*jmp_address == Assembler::kJnzShortOpcode ? not_carry : carry);
   *jmp_address = static_cast<byte>(Assembler::kJccShortPrefix | cc);
 }
 
 
 } }  // namespace v8::internal
 
 #endif  // V8_TARGET_ARCH_IA32