Handle breaks on keyed IC loads which can have an inlined version.

For keyed IC loads setting a break point now ensures that the inlined code is not used. When the break point is set the inlined map check is changed to fail causing the inlined code not to be used but the IC to be called. As long at the break point is set the map check will stay invalid.
Review URL: http://codereview.chromium.org/87025

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

src/debug.cc

@@ -35,6 +35,8 @@
 #include "debug.h"
 #include "execution.h"
 #include "global-handles.h"
+#include "ic.h"
+#include "ic-inl.h"
 #include "natives.h"
 #include "stub-cache.h"
 #include "log.h"
@@ -289,14 +291,8 @@ void BreakLocationIterator::SetDebugBreak() {
     // Patch the frame exit code with a break point.
     SetDebugBreakAtReturn();
   } else {
-    // Patch the original code with the current address as the current address
-    // might have changed by the inline caching since the code was copied.
-    original_rinfo()->set_target_address(rinfo()->target_address());
-
-    // Patch the code to invoke the builtin debug break function matching the
-    // calling convention used by the call site.
-    Handle<Code> dbgbrk_code(Debug::FindDebugBreak(rinfo()));
-    rinfo()->set_target_address(dbgbrk_code->entry());
+    // Patch the IC call.
+    SetDebugBreakAtIC();
   }
   ASSERT(IsDebugBreak());
 }
@@ -307,8 +303,8 @@ void BreakLocationIterator::ClearDebugBreak() {
     // Restore the frame exit code.
     ClearDebugBreakAtReturn();
   } else {
-    // Patch the code to the original invoke.
-    rinfo()->set_target_address(original_rinfo()->target_address());
+    // Patch the IC call.
+    ClearDebugBreakAtIC();
   }
   ASSERT(!IsDebugBreak());
 }
@@ -361,6 +357,39 @@ bool BreakLocationIterator::IsDebugBreak() {
 }
 
 
+void BreakLocationIterator::SetDebugBreakAtIC() {
+  // Patch the original code with the current address as the current address
+  // might have changed by the inline caching since the code was copied.
+  original_rinfo()->set_target_address(rinfo()->target_address());
+
+  RelocInfo::Mode mode = rmode();
+  if (RelocInfo::IsCodeTarget(mode)) {
+    Address target = rinfo()->target_address();
+    Handle<Code> code(Code::GetCodeFromTargetAddress(target));
+
+    // Patch the code to invoke the builtin debug break function matching the
+    // calling convention used by the call site.
+    Handle<Code> dbgbrk_code(Debug::FindDebugBreak(code, mode));
+    rinfo()->set_target_address(dbgbrk_code->entry());
+
+    // For stubs that refer back to an inlined version clear the cached map for
+    // the inlined case to always go through the IC. As long as the break point
+    // is set the patching performed by the runtime system will take place in
+    // 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::HasInlinedVersion(pc())) {
+      KeyedLoadIC::ClearInlinedVersion(pc());
+    }
+  }
+}
+
+
+void BreakLocationIterator::ClearDebugBreakAtIC() {
+  // Patch the code to the original invoke.
+  rinfo()->set_target_address(original_rinfo()->target_address());
+}
+
+
 Object* BreakLocationIterator::BreakPointObjects() {
   return debug_info_->GetBreakPointObjects(code_position());
 }
@@ -1056,48 +1085,42 @@ bool Debug::IsBreakStub(Code* code) {
 
 
 // Find the builtin to use for invoking the debug break
-Handle<Code> Debug::FindDebugBreak(RelocInfo* rinfo) {
+Handle<Code> Debug::FindDebugBreak(Handle<Code> code, RelocInfo::Mode mode) {
   // Find the builtin debug break function matching the calling convention
   // used by the call site.
-  RelocInfo::Mode mode = rinfo->rmode();
-
-  if (RelocInfo::IsCodeTarget(mode)) {
-    Address target = rinfo->target_address();
-    Code* code = Code::GetCodeFromTargetAddress(target);
-    if (code->is_inline_cache_stub()) {
-      if (code->is_call_stub()) {
-        return ComputeCallDebugBreak(code->arguments_count());
-      }
-      if (code->is_load_stub()) {
-        return Handle<Code>(Builtins::builtin(Builtins::LoadIC_DebugBreak));
-      }
-      if (code->is_store_stub()) {
-        return Handle<Code>(Builtins::builtin(Builtins::StoreIC_DebugBreak));
-      }
-      if (code->is_keyed_load_stub()) {
-        Handle<Code> result =
-            Handle<Code>(Builtins::builtin(Builtins::KeyedLoadIC_DebugBreak));
-        return result;
-      }
-      if (code->is_keyed_store_stub()) {
-        Handle<Code> result =
-            Handle<Code>(Builtins::builtin(Builtins::KeyedStoreIC_DebugBreak));
-        return result;
-      }
+  if (code->is_inline_cache_stub()) {
+    if (code->is_call_stub()) {
+      return ComputeCallDebugBreak(code->arguments_count());
+    }
+    if (code->is_load_stub()) {
+      return Handle<Code>(Builtins::builtin(Builtins::LoadIC_DebugBreak));
+    }
+    if (code->is_store_stub()) {
+      return Handle<Code>(Builtins::builtin(Builtins::StoreIC_DebugBreak));
     }
-    if (RelocInfo::IsConstructCall(mode)) {
+    if (code->is_keyed_load_stub()) {
       Handle<Code> result =
-          Handle<Code>(Builtins::builtin(Builtins::ConstructCall_DebugBreak));
+          Handle<Code>(Builtins::builtin(Builtins::KeyedLoadIC_DebugBreak));
       return result;
     }
-    if (code->kind() == Code::STUB) {
-      ASSERT(code->major_key() == CodeStub::CallFunction ||
-             code->major_key() == CodeStub::StackCheck);
+    if (code->is_keyed_store_stub()) {
       Handle<Code> result =
-          Handle<Code>(Builtins::builtin(Builtins::StubNoRegisters_DebugBreak));
+          Handle<Code>(Builtins::builtin(Builtins::KeyedStoreIC_DebugBreak));
       return result;
     }
   }
+  if (RelocInfo::IsConstructCall(mode)) {
+    Handle<Code> result =
+        Handle<Code>(Builtins::builtin(Builtins::ConstructCall_DebugBreak));
+    return result;
+  }
+  if (code->kind() == Code::STUB) {
+    ASSERT(code->major_key() == CodeStub::CallFunction ||
+           code->major_key() == CodeStub::StackCheck);
+    Handle<Code> result =
+        Handle<Code>(Builtins::builtin(Builtins::StubNoRegisters_DebugBreak));
+    return result;
+  }
 
   UNREACHABLE();
   return Handle<Code>::null();
@@ -1839,7 +1862,7 @@ void Debugger::NotifyMessageHandler(v8::DebugEvent event,
     v8::TryCatch try_catch;
     fun_name = v8::String::New("processDebugRequest");
     fun = v8::Function::Cast(*cmd_processor->Get(fun_name));
-    
+
     request = v8::String::New(command.text().start(),
                               command.text().length());
     command.text().Dispose();
@@ -2208,7 +2231,7 @@ MessageQueue::MessageQueue(int size) : start_(0), end_(0), size_(size) {
 
 
 MessageQueue::~MessageQueue() {
-  while(!IsEmpty()) {
+  while (!IsEmpty()) {
     Message m = Get();
     m.Dispose();
   }

src/debug.h

@@ -132,6 +132,10 @@ class BreakLocationIterator {
  private:
   void SetDebugBreak();
   void ClearDebugBreak();
+
+  void SetDebugBreakAtIC();
+  void ClearDebugBreakAtIC();
+
   bool IsDebugBreakAtReturn();
   void SetDebugBreakAtReturn();
   void ClearDebugBreakAtReturn();
@@ -205,7 +209,7 @@ class Debug {
   static bool IsBreakStub(Code* code);
 
   // Find the builtin to use for invoking the debug break
-  static Handle<Code> FindDebugBreak(RelocInfo* rinfo);
+  static Handle<Code> FindDebugBreak(Handle<Code> code, RelocInfo::Mode mode);
 
   static Handle<Object> GetSourceBreakLocations(
       Handle<SharedFunctionInfo> shared);

src/ic-arm.cc

@@ -507,6 +507,8 @@ void LoadIC::Generate(MacroAssembler* masm, const ExternalReference& f) {
 // TODO(181): Implement map patching once loop nesting is tracked on
 // the ARM platform so we can generate inlined fast-case code for
 // array indexing in loops.
+bool KeyedLoadIC::HasInlinedVersion(Address address) { return false; }
+void KeyedLoadIC::ClearInlinedVersion(Address address) { }
 void KeyedLoadIC::PatchInlinedMapCheck(Address address, Object* value) { }
 
 

src/ic-ia32.cc

@@ -729,8 +729,24 @@ void LoadIC::Generate(MacroAssembler* masm, const ExternalReference& f) {
 }
 
 
+// One byte opcode for test eax,0xXXXXXXXX.
+static const byte kTestEaxByte = 0xA9;
+
+
+bool KeyedLoadIC::HasInlinedVersion(Address address) {
+  Address test_instruction_address = address + 4;  // 4 = stub address
+  return *test_instruction_address == kTestEaxByte;
+}
+
+
+void KeyedLoadIC::ClearInlinedVersion(Address address) {
+  // Insert null as the map to check for to make sure the map check fails
+  // sending control flow to the IC instead of the inlined version.
+  PatchInlinedMapCheck(address, Heap::null_value());
+}
+
+
 void KeyedLoadIC::PatchInlinedMapCheck(Address address, Object* value) {
-  static const byte kTestEaxByte = 0xA9;
   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.
@@ -744,7 +760,7 @@ void KeyedLoadIC::PatchInlinedMapCheck(Address address, Object* value) {
     // bytes of the 7-byte operand-immediate compare instruction, so
     // we add 3 to the offset to get the map address.
     Address map_address = test_instruction_address + offset_value + 3;
-    // patch the map check.
+    // Patch the map check.
     (*(reinterpret_cast<Object**>(map_address))) = value;
   }
 }

src/ic.cc

@@ -237,7 +237,7 @@ void KeyedLoadIC::Clear(Address address, Code* target) {
   // Make sure to also clear the map used in inline fast cases.  If we
   // do not clear these maps, cached code can keep objects alive
   // through the embedded maps.
-  PatchInlinedMapCheck(address, Heap::null_value());
+  ClearInlinedVersion(address);
   SetTargetAtAddress(address, initialize_stub());
 }
 

src/ic.h

@@ -254,6 +254,12 @@ class KeyedLoadIC: public IC {
   static void GeneratePreMonomorphic(MacroAssembler* masm);
   static void GenerateGeneric(MacroAssembler* masm);
 
+  // Check if this IC corresponds to an inlined version.
+  static bool HasInlinedVersion(Address address);
+
+  // Clear the use of the inlined version.
+  static void ClearInlinedVersion(Address address);
+
  private:
   static void Generate(MacroAssembler* masm, const ExternalReference& f);
 

test/cctest/test-debug.cc

@@ -2177,6 +2177,53 @@ TEST(DebugStepLinear) {
 }
 
 
+// Test of the stepping mechanism for keyed load in a loop.
+TEST(DebugStepKeyedLoadLoop) {
+  v8::HandleScope scope;
+  DebugLocalContext env;
+
+  // Create a function for testing stepping of keyed load. The statement 'y=1'
+  // is there to have more than one breakable statement in the loop, TODO(315).
+  v8::Local<v8::Function> foo = CompileFunction(
+      &env,
+      "function foo(a) {\n"
+      "  var x;\n"
+      "  var len = a.length;\n"
+      "  for (var i = 0; i < len; i++) {\n"
+      "    y = 1;\n"
+      "    x = a[i];\n"
+      "  }\n"
+      "}\n",
+      "foo");
+
+  // Create array [0,1,2,3,4,5,6,7,8,9]
+  v8::Local<v8::Array> 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<v8::Value> 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;