Fix improved LoadICs for dictionaries with callbacks.

This fixes the positive lookup performed by these LoadICs, to use the
holder instead of the receiver to perfrom the lookup on. It also extends
this improvement to KeyedLoadICs. And it fixes a bug introduced for the
JavaScript getter case of a LoadIC.

R=erik.corry@gmail.com
BUG=chromium:142088
TEST=cctest/test-api/Regress142088,cctest/test-api/Regress137002b

Review URL: https://chromiumcodereview.appspot.com/10828303

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

src/arm/stub-cache-arm.cc

@@ -1238,8 +1238,12 @@ void StubCompiler::GenerateDictionaryLoadCallback(Register receiver,
                                                   Handle<AccessorInfo> callback,
                                                   Handle<String> name,
                                                   Label* miss) {
+  ASSERT(!receiver.is(scratch1));
+  ASSERT(!receiver.is(scratch2));
+  ASSERT(!receiver.is(scratch3));
+
+  // Load the properties dictionary.
   Register dictionary = scratch1;
-  Register index = scratch2;
   __ ldr(dictionary, FieldMemOperand(receiver, JSObject::kPropertiesOffset));
 
   // Probe the dictionary.
@@ -1249,19 +1253,18 @@ void StubCompiler::GenerateDictionaryLoadCallback(Register receiver,
                                                      &probe_done,
                                                      dictionary,
                                                      name_reg,
-                                                     index,  // Set if we hit.
+                                                     scratch2,
                                                      scratch3);
   __ bind(&probe_done);
 
-  // If probing finds an entry in the dictionary, check that the value is the
-  // callback.
-  const int kElementsStartOffset =
-      StringDictionary::kHeaderSize +
+  // If probing finds an entry in the dictionary, scratch3 contains the
+  // pointer into the dictionary. Check that the value is the callback.
+  Register pointer = scratch3;
+  const int kElementsStartOffset = StringDictionary::kHeaderSize +
       StringDictionary::kElementsStartIndex * kPointerSize;
   const int kValueOffset = kElementsStartOffset + kPointerSize;
-  __ add(scratch1, dictionary, Operand(kValueOffset - kHeapObjectTag));
-  __ ldr(scratch3, MemOperand(scratch1, index, LSL, kPointerSizeLog2));
-  __ cmp(scratch3, Operand(callback));
+  __ ldr(scratch2, FieldMemOperand(pointer, kValueOffset));
+  __ cmp(scratch2, Operand(callback));
   __ b(ne, miss);
 }
 
@@ -1273,6 +1276,7 @@ void StubCompiler::GenerateLoadCallback(Handle<JSObject> object,
                                         Register scratch1,
                                         Register scratch2,
                                         Register scratch3,
+                                        Register scratch4,
                                         Handle<AccessorInfo> callback,
                                         Handle<String> name,
                                         Label* miss) {
@@ -1285,7 +1289,7 @@ void StubCompiler::GenerateLoadCallback(Handle<JSObject> object,
 
   if (!holder->HasFastProperties() && !holder->IsJSGlobalObject()) {
     GenerateDictionaryLoadCallback(
-        receiver, name_reg, scratch1, scratch2, scratch3, callback, name, miss);
+        reg, name_reg, scratch2, scratch3, scratch4, callback, name, miss);
   }
 
   // Build AccessorInfo::args_ list on the stack and push property name below
@@ -2914,7 +2918,7 @@ Handle<Code> LoadStubCompiler::CompileLoadCallback(
   //  -- lr    : return address
   // -----------------------------------
   Label miss;
-  GenerateLoadCallback(object, holder, r0, r2, r3, r1, r4, callback, name,
+  GenerateLoadCallback(object, holder, r0, r2, r3, r1, r4, r5, callback, name,
                        &miss);
   __ bind(&miss);
   GenerateLoadMiss(masm(), Code::LOAD_IC);
@@ -3085,7 +3089,7 @@ Handle<Code> KeyedLoadStubCompiler::CompileLoadCallback(
   __ cmp(r0, Operand(name));
   __ b(ne, &miss);
 
-  GenerateLoadCallback(receiver, holder, r1, r0, r2, r3, r4, callback, name,
+  GenerateLoadCallback(receiver, holder, r1, r0, r2, r3, r4, r5, callback, name,
                        &miss);
   __ bind(&miss);
   GenerateLoadMiss(masm(), Code::KEYED_LOAD_IC);

src/ia32/stub-cache-ia32.cc

@@ -1060,18 +1060,31 @@ void StubCompiler::GenerateDictionaryLoadCallback(Register receiver,
                                                   Handle<AccessorInfo> callback,
                                                   Handle<String> name,
                                                   Label* miss) {
+  ASSERT(!receiver.is(scratch2));
+  ASSERT(!receiver.is(scratch3));
   Register dictionary = scratch1;
+  bool must_preserve_dictionary_reg = receiver.is(dictionary);
+
+  // Load the properties dictionary.
+  if (must_preserve_dictionary_reg) {
+    __ push(dictionary);
+  }
   __ mov(dictionary, FieldOperand(receiver, JSObject::kPropertiesOffset));
 
   // Probe the dictionary.
-  Label probe_done;
+  Label probe_done, pop_and_miss;
   StringDictionaryLookupStub::GeneratePositiveLookup(masm(),
-                                                     miss,
+                                                     &pop_and_miss,
                                                      &probe_done,
                                                      dictionary,
                                                      name_reg,
                                                      scratch2,
                                                      scratch3);
+  __ bind(&pop_and_miss);
+  if (must_preserve_dictionary_reg) {
+    __ pop(dictionary);
+  }
+  __ jmp(miss);
   __ bind(&probe_done);
 
   // If probing finds an entry in the dictionary, scratch2 contains the
@@ -1083,6 +1096,9 @@ void StubCompiler::GenerateDictionaryLoadCallback(Register receiver,
   const int kValueOffset = kElementsStartOffset + kPointerSize;
   __ mov(scratch3,
          Operand(dictionary, index, times_4, kValueOffset - kHeapObjectTag));
+  if (must_preserve_dictionary_reg) {
+    __ pop(dictionary);
+  }
   __ cmp(scratch3, callback);
   __ j(not_equal, miss);
 }
@@ -1095,6 +1111,7 @@ void StubCompiler::GenerateLoadCallback(Handle<JSObject> object,
                                         Register scratch1,
                                         Register scratch2,
                                         Register scratch3,
+                                        Register scratch4,
                                         Handle<AccessorInfo> callback,
                                         Handle<String> name,
                                         Label* miss) {
@@ -1107,7 +1124,7 @@ void StubCompiler::GenerateLoadCallback(Handle<JSObject> object,
 
   if (!holder->HasFastProperties() && !holder->IsJSGlobalObject()) {
     GenerateDictionaryLoadCallback(
-        receiver, name_reg, scratch1, scratch2, scratch3, callback, name, miss);
+        reg, name_reg, scratch1, scratch2, scratch3, callback, name, miss);
   }
 
   // Insert additional parameters into the stack frame above return address.
@@ -2945,8 +2962,8 @@ Handle<Code> LoadStubCompiler::CompileLoadCallback(
   // -----------------------------------
   Label miss;
 
-  GenerateLoadCallback(object, holder, edx, ecx, ebx, eax, edi, callback,
-                       name, &miss);
+  GenerateLoadCallback(object, holder, edx, ecx, ebx, eax, edi, no_reg,
+                       callback, name, &miss);
   __ bind(&miss);
   GenerateLoadMiss(masm(), Code::LOAD_IC);
 
@@ -3135,8 +3152,8 @@ Handle<Code> KeyedLoadStubCompiler::CompileLoadCallback(
   __ cmp(ecx, Immediate(name));
   __ j(not_equal, &miss);
 
-  GenerateLoadCallback(receiver, holder, edx, ecx, ebx, eax, edi, callback,
-                       name, &miss);
+  GenerateLoadCallback(receiver, holder, edx, ecx, ebx, eax, edi, no_reg,
+                       callback, name, &miss);
 
   __ bind(&miss);
   __ DecrementCounter(counters->keyed_load_callback(), 1);

src/ic.cc

@@ -996,6 +996,7 @@ void LoadIC::UpdateCaches(LookupResult* lookup,
           Handle<Object> getter(Handle<AccessorPair>::cast(callback)->getter());
           if (!getter->IsJSFunction()) return;
           if (holder->IsGlobalObject()) return;
+          if (!holder->HasFastProperties()) return;
           code = isolate()->stub_cache()->ComputeLoadViaGetter(
               name, receiver, holder, Handle<JSFunction>::cast(getter));
         } else {
@@ -1264,7 +1265,6 @@ void KeyedLoadIC::UpdateCaches(LookupResult* lookup,
         Handle<AccessorInfo> callback =
             Handle<AccessorInfo>::cast(callback_object);
         if (v8::ToCData<Address>(callback->getter()) == 0) return;
-        if (!holder->HasFastProperties()) return;
         if (!callback->IsCompatibleReceiver(*receiver)) return;
         code = isolate()->stub_cache()->ComputeKeyedLoadCallback(
             name, receiver, holder, callback);

src/stub-cache.h

@@ -550,6 +550,7 @@ class StubCompiler BASE_EMBEDDED {
                             Register scratch1,
                             Register scratch2,
                             Register scratch3,
+                            Register scratch4,
                             Handle<AccessorInfo> callback,
                             Handle<String> name,
                             Label* miss);

src/x64/ic-x64.cc

@@ -135,7 +135,7 @@ static void GenerateDictionaryLoad(MacroAssembler* masm,
                                                      r0,
                                                      r1);
 
-  // If probing finds an entry in the dictionary, r0 contains the
+  // If probing finds an entry in the dictionary, r1 contains the
   // index into the dictionary. Check that the value is a normal
   // property.
   __ bind(&done);
@@ -178,10 +178,9 @@ static void GenerateDictionaryStore(MacroAssembler* masm,
   //
   // value - holds the value to store and is unchanged.
   //
-  // scratch0 - used for index into the property dictionary and is clobbered.
+  // scratch0 - used during the positive dictionary lookup and is clobbered.
   //
-  // scratch1 - used to hold the capacity of the property dictionary and is
-  //            clobbered.
+  // scratch1 - used for index into the property dictionary and is clobbered.
   Label done;
 
   // Probe the dictionary.

src/x64/stub-cache-x64.cc

@@ -1037,6 +1037,11 @@ void StubCompiler::GenerateDictionaryLoadCallback(Register receiver,
                                                   Handle<AccessorInfo> callback,
                                                   Handle<String> name,
                                                   Label* miss) {
+  ASSERT(!receiver.is(scratch1));
+  ASSERT(!receiver.is(scratch2));
+  ASSERT(!receiver.is(scratch3));
+
+  // Load the properties dictionary.
   Register dictionary = scratch1;
   __ movq(dictionary, FieldOperand(receiver, JSObject::kPropertiesOffset));
 
@@ -1051,17 +1056,18 @@ void StubCompiler::GenerateDictionaryLoadCallback(Register receiver,
                                                      scratch3);
   __ bind(&probe_done);
 
-  // If probing finds an entry in the dictionary, scratch2 contains the
+  // If probing finds an entry in the dictionary, scratch3 contains the
   // index into the dictionary. Check that the value is the callback.
-  Register index = scratch2;
+  Register index = scratch3;
   const int kElementsStartOffset =
       StringDictionary::kHeaderSize +
       StringDictionary::kElementsStartIndex * kPointerSize;
   const int kValueOffset = kElementsStartOffset + kPointerSize;
-  __ movq(scratch3,
-          Operand(dictionary, index, times_8, kValueOffset - kHeapObjectTag));
-  __ movq(scratch2, callback, RelocInfo::EMBEDDED_OBJECT);
-  __ cmpq(scratch3, scratch2);
+  __ movq(scratch2,
+          Operand(dictionary, index, times_pointer_size,
+                  kValueOffset - kHeapObjectTag));
+  __ movq(scratch3, callback, RelocInfo::EMBEDDED_OBJECT);
+  __ cmpq(scratch2, scratch3);
   __ j(not_equal, miss);
 }
 
@@ -1073,6 +1079,7 @@ void StubCompiler::GenerateLoadCallback(Handle<JSObject> object,
                                         Register scratch1,
                                         Register scratch2,
                                         Register scratch3,
+                                        Register scratch4,
                                         Handle<AccessorInfo> callback,
                                         Handle<String> name,
                                         Label* miss) {
@@ -1085,7 +1092,7 @@ void StubCompiler::GenerateLoadCallback(Handle<JSObject> object,
 
   if (!holder->HasFastProperties() && !holder->IsJSGlobalObject()) {
     GenerateDictionaryLoadCallback(
-        receiver, name_reg, scratch1, scratch2, scratch3, callback, name, miss);
+        reg, name_reg, scratch2, scratch3, scratch4, callback, name, miss);
   }
 
   // Insert additional parameters into the stack frame above return address.
@@ -2781,7 +2788,7 @@ Handle<Code> LoadStubCompiler::CompileLoadCallback(
   //  -- rsp[0] : return address
   // -----------------------------------
   Label miss;
-  GenerateLoadCallback(object, holder, rax, rcx, rdx, rbx, rdi, callback,
+  GenerateLoadCallback(object, holder, rax, rcx, rdx, rbx, rdi, r8, callback,
                        name, &miss);
   __ bind(&miss);
   GenerateLoadMiss(masm(), Code::LOAD_IC);
@@ -2964,7 +2971,7 @@ Handle<Code> KeyedLoadStubCompiler::CompileLoadCallback(
   __ Cmp(rax, name);
   __ j(not_equal, &miss);
 
-  GenerateLoadCallback(receiver, holder, rdx, rax, rbx, rcx, rdi, callback,
+  GenerateLoadCallback(receiver, holder, rdx, rax, rbx, rcx, rdi, r8, callback,
                        name, &miss);
   __ bind(&miss);
   __ DecrementCounter(counters->keyed_load_callback(), 1);

test/cctest/test-api.cc

@@ -14726,6 +14726,8 @@ THREADED_TEST(FunctionGetScriptId) {
 
 static v8::Handle<Value> GetterWhichReturns42(Local<String> name,
                                               const AccessorInfo& info) {
+  CHECK(v8::Utils::OpenHandle(*info.This())->IsJSObject());
+  CHECK(v8::Utils::OpenHandle(*info.Holder())->IsJSObject());
   return v8_num(42);
 }
 
@@ -14733,12 +14735,16 @@ static v8::Handle<Value> GetterWhichReturns42(Local<String> name,
 static void SetterWhichSetsYOnThisTo23(Local<String> name,
                                        Local<Value> value,
                                        const AccessorInfo& info) {
+  CHECK(v8::Utils::OpenHandle(*info.This())->IsJSObject());
+  CHECK(v8::Utils::OpenHandle(*info.Holder())->IsJSObject());
   info.This()->Set(v8_str("y"), v8_num(23));
 }
 
 
 Handle<Value> FooGetInterceptor(Local<String> name,
                                 const AccessorInfo& info) {
+  CHECK(v8::Utils::OpenHandle(*info.This())->IsJSObject());
+  CHECK(v8::Utils::OpenHandle(*info.Holder())->IsJSObject());
   if (!name->Equals(v8_str("foo"))) return Handle<Value>();
   return v8_num(42);
 }
@@ -14747,6 +14753,8 @@ Handle<Value> FooGetInterceptor(Local<String> name,
 Handle<Value> FooSetInterceptor(Local<String> name,
                                 Local<Value> value,
                                 const AccessorInfo& info) {
+  CHECK(v8::Utils::OpenHandle(*info.This())->IsJSObject());
+  CHECK(v8::Utils::OpenHandle(*info.Holder())->IsJSObject());
   if (!name->Equals(v8_str("foo"))) return Handle<Value>();
   info.This()->Set(v8_str("y"), v8_num(23));
   return v8_num(23);
@@ -17068,19 +17076,30 @@ THREADED_TEST(Regress137002b) {
              "function store2(x) { void 0; x.foo = void 0; }"
              "function keyed_load2(x, key) { void 0; return x[key]; }"
 
+             "obj.y = void 0;"
              "obj.__proto__ = null;"
              "var subobj = {};"
+             "subobj.y = void 0;"
              "subobj.__proto__ = obj;"
              "%OptimizeObjectForAddingMultipleProperties(obj, 1);"
 
              // Make the ICs monomorphic.
              "load(obj); load(obj);"
              "load2(subobj); load2(subobj);"
-             "store(obj);"
-             "store2(subobj);"
+             "store(obj); store(obj);"
+             "store2(subobj); store2(subobj);"
              "keyed_load(obj, 'foo'); keyed_load(obj, 'foo');"
              "keyed_load2(subobj, 'foo'); keyed_load2(subobj, 'foo');"
 
+             // Actually test the shiny new ICs and better not crash. This
+             // serves as a regression test for issue 142088 as well.
+             "load(obj);"
+             "load2(subobj);"
+             "store(obj);"
+             "store2(subobj);"
+             "keyed_load(obj, 'foo');"
+             "keyed_load2(subobj, 'foo');"
+
              // Delete the accessor.  It better not be called any more now.
              "delete obj.foo;"
              "obj.y = void 0;"
@@ -17103,6 +17122,23 @@ THREADED_TEST(Regress137002b) {
 }
 
 
+THREADED_TEST(Regress142088) {
+  i::FLAG_allow_natives_syntax = true;
+  v8::HandleScope scope;
+  LocalContext context;
+  Local<ObjectTemplate> templ = ObjectTemplate::New();
+  templ->SetAccessor(v8_str("foo"),
+                     GetterWhichReturns42,
+                     SetterWhichSetsYOnThisTo23);
+  context->Global()->Set(v8_str("obj"), templ->NewInstance());
+
+  CompileRun("function load(x) { return x.foo; }"
+             "var o = Object.create(obj);"
+             "%OptimizeObjectForAddingMultipleProperties(obj, 1);"
+             "load(o); load(o); load(o); load(o);");
+}
+
+
 THREADED_TEST(Regress137496) {
   i::FLAG_expose_gc = true;
   v8::HandleScope scope;