From f706cfa30cbdd45d13a49c0b256cd463a88feda0 Mon Sep 17 00:00:00 2001 From: "ager@chromium.org" Date: Fri, 12 Jun 2009 11:24:13 +0000 Subject: [PATCH] Fix debugger after inlined keyed store change. Make sure that the IC is always hit when debugging and make sure to restore the fast case when leaving the debugger. Review URL: http://codereview.chromium.org/125044 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@2152 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/ic-arm.cc | 6 ++++++ src/debug.cc | 14 ++++++++++++++ src/ia32/codegen-ia32.cc | 27 +++++++++++++++++++++++++-- src/ia32/ic-ia32.cc | 27 ++++++++++++++++++++++++++- src/ic.h | 11 +++++++++++ src/x64/ic-x64.cc | 13 +++++++++++++ test/cctest/test-debug.cc | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 141 insertions(+), 3 deletions(-) diff --git a/src/arm/ic-arm.cc b/src/arm/ic-arm.cc index 58e061a..8b4e087 100644 --- a/src/arm/ic-arm.cc +++ b/src/arm/ic-arm.cc @@ -503,6 +503,12 @@ bool KeyedLoadIC::PatchInlinedLoad(Address address, Object* map) { return false; } +void KeyedStoreIC::ClearInlinedVersion(Address address) {} +void KeyedStoreIC::RestoreInlinedVersion(Address address) {} +bool KeyedStoreIC::PatchInlinedStore(Address address, Object* map) { + return false; +} + Object* KeyedLoadIC_Miss(Arguments args); diff --git a/src/debug.cc b/src/debug.cc index 0daf564..e37bfb7 100644 --- a/src/debug.cc +++ b/src/debug.cc @@ -382,6 +382,7 @@ void BreakLocationIterator::SetDebugBreakAtIC() { // the code copy and will therefore have no effect on the running code // keeping it from using the inlined code. if (code->is_keyed_load_stub()) KeyedLoadIC::ClearInlinedVersion(pc()); + if (code->is_keyed_store_stub()) KeyedStoreIC::ClearInlinedVersion(pc()); } } @@ -389,6 +390,19 @@ void BreakLocationIterator::SetDebugBreakAtIC() { void BreakLocationIterator::ClearDebugBreakAtIC() { // Patch the code to the original invoke. rinfo()->set_target_address(original_rinfo()->target_address()); + + RelocInfo::Mode mode = rmode(); + if (RelocInfo::IsCodeTarget(mode)) { + Address target = original_rinfo()->target_address(); + Handle code(Code::GetCodeFromTargetAddress(target)); + + // Restore the inlined version of keyed stores to get back to the + // fast case. We need to patch back the keyed store because no + // patching happens when running normally. For keyed loads, the + // map check will get patched back when running normally after ICs + // have been cleared at GC. + if (code->is_keyed_store_stub()) KeyedStoreIC::RestoreInlinedVersion(pc()); + } } diff --git a/src/ia32/codegen-ia32.cc b/src/ia32/codegen-ia32.cc index c7a35e8..bd27af7 100644 --- a/src/ia32/codegen-ia32.cc +++ b/src/ia32/codegen-ia32.cc @@ -5749,10 +5749,13 @@ class DeferredReferenceSetKeyedValue: public DeferredCode { virtual void Generate(); + Label* patch_site() { return &patch_site_; } + private: Register value_; Register key_; Register receiver_; + Label patch_site_; }; @@ -5766,6 +5769,15 @@ void DeferredReferenceSetKeyedValue::Generate() { // Call the IC stub. Handle ic(Builtins::builtin(Builtins::KeyedStoreIC_Initialize)); __ call(ic, RelocInfo::CODE_TARGET); + // The delta from the start of the map-compare instruction to the + // test instruction. We use masm_-> directly here instead of the + // __ macro because the macro sometimes uses macro expansion to turn + // into something that can't return a value. This is encountered + // when doing generated code coverage tests. + int delta_to_patch_site = masm_->SizeOfCodeGeneratedSince(patch_site()); + // Here we use masm_-> instead of the __ macro because this is the + // instruction that gets patched and coverage code gets in the way. + masm_->test(eax, Immediate(-delta_to_patch_site)); // Restore value (returned from store IC), key and receiver // registers. if (!value_.is(eax)) __ mov(value_, eax); @@ -6122,9 +6134,15 @@ void Reference::SetValue(InitState init_state) { // is not a dictionary. __ mov(tmp.reg(), FieldOperand(receiver.reg(), JSObject::kElementsOffset)); + // Bind the deferred code patch site to be able to locate the + // fixed array map comparison. When debugging, we patch this + // comparison to always fail so that we will hit the IC call + // in the deferred code which will allow the debugger to + // break for fast case stores. + __ bind(deferred->patch_site()); __ cmp(FieldOperand(tmp.reg(), HeapObject::kMapOffset), - Immediate(Factory::hash_table_map())); - deferred->Branch(equal); + Immediate(Factory::fixed_array_map())); + deferred->Branch(not_equal); // Store the value. __ mov(Operand(tmp.reg(), @@ -6141,6 +6159,11 @@ void Reference::SetValue(InitState init_state) { cgen_->frame()->Push(&value); } else { Result answer = cgen_->frame()->CallKeyedStoreIC(); + // Make sure that we do not have a test instruction after the + // call. A test instruction after the call is used to + // indicate that we have generated an inline version of the + // keyed store. + __ nop(); cgen_->frame()->Push(&answer); } break; diff --git a/src/ia32/ic-ia32.cc b/src/ia32/ic-ia32.cc index d7f264d..5da9b2f 100644 --- a/src/ia32/ic-ia32.cc +++ b/src/ia32/ic-ia32.cc @@ -747,6 +747,21 @@ void KeyedLoadIC::ClearInlinedVersion(Address address) { } +void KeyedStoreIC::ClearInlinedVersion(Address address) { + // Insert null as the elements map to check for. This will make + // sure that the elements fast-case map check fails so that control + // flows to the IC instead of the inlined version. + PatchInlinedStore(address, Heap::null_value()); +} + + +void KeyedStoreIC::RestoreInlinedVersion(Address address) { + // Restore the fast-case elements map check so that the inlined + // version can be used again. + PatchInlinedStore(address, Heap::fixed_array_map()); +} + + bool LoadIC::PatchInlinedLoad(Address address, Object* map, int offset) { // The address of the instruction following the call. Address test_instruction_address = address + 4; @@ -774,7 +789,7 @@ bool LoadIC::PatchInlinedLoad(Address address, Object* map, int offset) { } -bool KeyedLoadIC::PatchInlinedLoad(Address address, Object* map) { +static bool PatchInlinedMapCheck(Address address, Object* map) { Address test_instruction_address = address + 4; // 4 = stub address // The keyed load has a fast inlined case if the IC call instruction // is immediately followed by a test instruction. @@ -795,6 +810,16 @@ bool KeyedLoadIC::PatchInlinedLoad(Address address, Object* map) { } +bool KeyedLoadIC::PatchInlinedLoad(Address address, Object* map) { + return PatchInlinedMapCheck(address, map); +} + + +bool KeyedStoreIC::PatchInlinedStore(Address address, Object* map) { + return PatchInlinedMapCheck(address, map); +} + + // Defined in ic.cc. Object* KeyedLoadIC_Miss(Arguments args); diff --git a/src/ic.h b/src/ic.h index bd94fd8..9c96ba2 100644 --- a/src/ic.h +++ b/src/ic.h @@ -356,6 +356,12 @@ class KeyedStoreIC: public IC { static void GenerateGeneric(MacroAssembler* masm); static void GenerateExtendStorage(MacroAssembler* masm); + // Clear the inlined version so the IC is always hit. + static void ClearInlinedVersion(Address address); + + // Restore the inlined version so the fast case can get hit. + static void RestoreInlinedVersion(Address address); + private: static void Generate(MacroAssembler* masm, const ExternalReference& f); @@ -378,6 +384,11 @@ class KeyedStoreIC: public IC { } static void Clear(Address address, Code* target); + + // Support for patching the map that is checked in an inlined + // version of keyed store. + static bool PatchInlinedStore(Address address, Object* map); + friend class IC; }; diff --git a/src/x64/ic-x64.cc b/src/x64/ic-x64.cc index 71a3a9a..858f3a2 100644 --- a/src/x64/ic-x64.cc +++ b/src/x64/ic-x64.cc @@ -40,6 +40,14 @@ void KeyedLoadIC::ClearInlinedVersion(Address address) { UNIMPLEMENTED(); } +void KeyedStoreIC::ClearInlinedVersion(Address address) { + UNIMPLEMENTED(); +} + +void KeyedStoreIC::RestoreInlinedVersion(Address address) { + UNIMPLEMENTED(); +} + void KeyedLoadIC::Generate(MacroAssembler* masm, ExternalReference const& f) { masm->int3(); // UNIMPLEMENTED. @@ -58,6 +66,11 @@ bool KeyedLoadIC::PatchInlinedLoad(Address address, Object* map) { return false; } +bool KeyedStoreIC::PatchInlinedStore(Address address, Object* map) { + UNIMPLEMENTED(); + return false; +} + Object* KeyedLoadStubCompiler::CompileLoadArrayLength(String* name) { UNIMPLEMENTED(); return NULL; diff --git a/test/cctest/test-debug.cc b/test/cctest/test-debug.cc index 92f48e1..7669b43 100644 --- a/test/cctest/test-debug.cc +++ b/test/cctest/test-debug.cc @@ -2237,6 +2237,52 @@ TEST(DebugStepKeyedLoadLoop) { } +// Test of the stepping mechanism for keyed store in a loop. +TEST(DebugStepKeyedStoreLoop) { + v8::HandleScope scope; + DebugLocalContext env; + + // Create a function for testing stepping of keyed store. The statement 'y=1' + // is there to have more than one breakable statement in the loop, TODO(315). + v8::Local foo = CompileFunction( + &env, + "function foo(a) {\n" + " var len = a.length;\n" + " for (var i = 0; i < len; i++) {\n" + " y = 1;\n" + " a[i] = 42;\n" + " }\n" + "}\n", + "foo"); + + // Create array [0,1,2,3,4,5,6,7,8,9] + v8::Local a = v8::Array::New(10); + for (int i = 0; i < 10; i++) { + a->Set(v8::Number::New(i), v8::Number::New(i)); + } + + // Call function without any break points to ensure inlining is in place. + const int kArgc = 1; + v8::Handle args[kArgc] = { a }; + foo->Call(env->Global(), kArgc, args); + + // Register a debug event listener which steps and counts. + v8::Debug::SetDebugEventListener(DebugEventStep); + + // Setup break point and step through the function. + SetBreakPoint(foo, 3); + step_action = StepNext; + break_point_hit_count = 0; + foo->Call(env->Global(), kArgc, args); + + // With stepping all break locations are hit. + CHECK_EQ(22, break_point_hit_count); + + v8::Debug::SetDebugEventListener(NULL); + CheckDebuggerUnloaded(); +} + + // Test the stepping mechanism with different ICs. TEST(DebugStepLinearMixedICs) { v8::HandleScope scope; -- 2.7.4