Revision: 12311
Author: [email protected]
Date: Tue Aug 14 06:17:47 2012
Log: 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.
[email protected]
BUG=chromium:142088
TEST=cctest/test-api/Regress142088,cctest/test-api/Regress137002b
Review URL: https://chromiumcodereview.appspot.com/10828303
http://code.google.com/p/v8/source/detail?r=12311
Modified:
/branches/bleeding_edge/src/arm/stub-cache-arm.cc
/branches/bleeding_edge/src/ia32/stub-cache-ia32.cc
/branches/bleeding_edge/src/ic.cc
/branches/bleeding_edge/src/stub-cache.h
/branches/bleeding_edge/src/x64/ic-x64.cc
/branches/bleeding_edge/src/x64/stub-cache-x64.cc
/branches/bleeding_edge/test/cctest/test-api.cc
=======================================
--- /branches/bleeding_edge/src/arm/stub-cache-arm.cc Mon Aug 6 07:25:19
2012
+++ /branches/bleeding_edge/src/arm/stub-cache-arm.cc Tue Aug 14 06:17:47
2012
@@ -1238,8 +1238,12 @@
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 @@
&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 @@
Register scratch1,
Register scratch2,
Register scratch3,
+ Register scratch4,
Handle<AccessorInfo> callback,
Handle<String> name,
Label* miss) {
@@ -1285,7 +1289,7 @@
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 @@
// -- 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 @@
__ 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);
=======================================
--- /branches/bleeding_edge/src/ia32/stub-cache-ia32.cc Mon Aug 6 07:25:19
2012
+++ /branches/bleeding_edge/src/ia32/stub-cache-ia32.cc Tue Aug 14 06:17:47
2012
@@ -1060,18 +1060,31 @@
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 @@
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 @@
Register scratch1,
Register scratch2,
Register scratch3,
+ Register scratch4,
Handle<AccessorInfo> callback,
Handle<String> name,
Label* miss) {
@@ -1107,7 +1124,7 @@
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 @@
// -----------------------------------
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 @@
__ 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);
=======================================
--- /branches/bleeding_edge/src/ic.cc Mon Aug 6 07:25:19 2012
+++ /branches/bleeding_edge/src/ic.cc Tue Aug 14 06:17:47 2012
@@ -996,6 +996,7 @@
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 @@
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);
=======================================
--- /branches/bleeding_edge/src/stub-cache.h Mon Aug 6 07:25:19 2012
+++ /branches/bleeding_edge/src/stub-cache.h Tue Aug 14 06:17:47 2012
@@ -550,6 +550,7 @@
Register scratch1,
Register scratch2,
Register scratch3,
+ Register scratch4,
Handle<AccessorInfo> callback,
Handle<String> name,
Label* miss);
=======================================
--- /branches/bleeding_edge/src/x64/ic-x64.cc Mon Jun 25 04:35:23 2012
+++ /branches/bleeding_edge/src/x64/ic-x64.cc Tue Aug 14 06:17:47 2012
@@ -135,7 +135,7 @@
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 @@
//
// 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.
=======================================
--- /branches/bleeding_edge/src/x64/stub-cache-x64.cc Mon Aug 6 07:25:19
2012
+++ /branches/bleeding_edge/src/x64/stub-cache-x64.cc Tue Aug 14 06:17:47
2012
@@ -1037,6 +1037,11 @@
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 @@
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 @@
Register scratch1,
Register scratch2,
Register scratch3,
+ Register scratch4,
Handle<AccessorInfo> callback,
Handle<String> name,
Label* miss) {
@@ -1085,7 +1092,7 @@
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 @@
// -- 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 @@
__ 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);
=======================================
--- /branches/bleeding_edge/test/cctest/test-api.cc Tue Aug 7 07:48:19 2012
+++ /branches/bleeding_edge/test/cctest/test-api.cc Tue Aug 14 06:17:47 2012
@@ -14726,6 +14726,8 @@
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 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> 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 @@
"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;"
@@ -17101,6 +17120,23 @@
CHECK(context->Global()->Get(v8_str("y_from_obj"))->IsUndefined());
CHECK(context->Global()->Get(v8_str("y_from_subobj"))->IsUndefined());
}
+
+
+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) {
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
