Debugger: clear ICs on activating step-in to correctly flood accessor pairs.

If we compile handlers to call accessors, Debug::HandleStepIn won't get
called. Therefore we need to clear ICs each time. This has not been
necessary before because we used to patch ICs for breaking, and restored
them with cleared ICs. This is no longer the case. We do not use ICs
for breaking anymore, so they are not implicitly cleared any longer.

R=mvstanton@chromium.org
BUG=v8:4269
LOG=N

Review URL: https://codereview.chromium.org/1212253009

Cr-Commit-Position: refs/heads/master@{#29518}

src/debug.cc

@@ -1351,7 +1351,7 @@ void Debug::PrepareStep(StepAction step_action,
       }
     }
 
-    ActivateStepIn(frame);
+    ActivateStepIn(function, frame);
   }
 
   // Fill the current function with one-shot break points even for step in on
@@ -1455,30 +1455,27 @@ Handle<Object> Debug::GetSourceBreakLocations(
 
 
 // Handle stepping into a function.
-void Debug::HandleStepIn(Handle<Object> function_obj, Handle<Object> holder,
-                         Address fp, bool is_constructor) {
+void Debug::HandleStepIn(Handle<Object> function_obj, bool is_constructor) {
   // Flood getter/setter if we either step in or step to another frame.
   bool step_frame = thread_local_.last_step_action_ == StepFrame;
   if (!StepInActive() && !step_frame) return;
   if (!function_obj->IsJSFunction()) return;
   Handle<JSFunction> function = Handle<JSFunction>::cast(function_obj);
   Isolate* isolate = function->GetIsolate();
-  // If the frame pointer is not supplied by the caller find it.
-  if (fp == 0) {
-    StackFrameIterator it(isolate);
+
+  StackFrameIterator it(isolate);
+  it.Advance();
+  // For constructor functions skip another frame.
+  if (is_constructor) {
+    DCHECK(it.frame()->is_construct());
     it.Advance();
-    // For constructor functions skip another frame.
-    if (is_constructor) {
-      DCHECK(it.frame()->is_construct());
-      it.Advance();
-    }
-    fp = it.frame()->fp();
   }
+  Address fp = it.frame()->fp();
 
   // Flood the function with one-shot break points if it is called from where
   // step into was requested, or when stepping into a new frame.
   if (fp == thread_local_.step_into_fp_ || step_frame) {
-    FloodWithOneShotGeneric(function, holder);
+    FloodWithOneShotGeneric(function, Handle<Object>());
   }
 }
 
@@ -1512,8 +1509,12 @@ void Debug::ClearOneShot() {
 }
 
 
-void Debug::ActivateStepIn(StackFrame* frame) {
+void Debug::ActivateStepIn(Handle<JSFunction> function, StackFrame* frame) {
   DCHECK(!StepOutActive());
+  // Make sure IC state is clean. This is so that we correct flood
+  // accessor pairs when stepping in.
+  function->code()->ClearInlineCaches();
+  function->shared()->feedback_vector()->ClearICSlots(function->shared());
   thread_local_.step_into_fp_ = frame->UnpaddedFP();
 }
 
@@ -2069,10 +2070,6 @@ bool Debug::EnsureDebugInfo(Handle<SharedFunctionInfo> shared,
     return false;
   }
 
-  // Make sure IC state is clean.
-  shared->code()->ClearInlineCaches();
-  shared->feedback_vector()->ClearICSlots(*shared);
-
   // Create the debug info object.
   Handle<DebugInfo> debug_info = isolate->factory()->NewDebugInfo(shared);
 

src/debug.h

@@ -465,8 +465,7 @@ class Debug {
   bool IsStepping() { return thread_local_.step_count_ > 0; }
   bool StepNextContinue(BreakLocation* location, JavaScriptFrame* frame);
   bool StepInActive() { return thread_local_.step_into_fp_ != 0; }
-  void HandleStepIn(Handle<Object> function_obj, Handle<Object> holder,
-                    Address fp, bool is_constructor);
+  void HandleStepIn(Handle<Object> function_obj, bool is_constructor);
   bool StepOutActive() { return thread_local_.step_out_fp_ != 0; }
 
   // Purge all code objects that have no debug break slots.
@@ -625,7 +624,7 @@ class Debug {
 
   static bool CompileDebuggerScript(Isolate* isolate, int index);
   void ClearOneShot();
-  void ActivateStepIn(StackFrame* frame);
+  void ActivateStepIn(Handle<JSFunction> function, StackFrame* frame);
   void ClearStepIn();
   void ActivateStepOut(StackFrame* frame);
   void ClearStepNext();

src/objects.cc

@@ -440,9 +440,7 @@ MaybeHandle<Object> Object::GetPropertyWithDefinedGetter(
   Debug* debug = isolate->debug();
   // Handle stepping into a getter if step into is active.
   // TODO(rossberg): should this apply to getters that are function proxies?
-  if (debug->is_active()) {
-    debug->HandleStepIn(getter, Handle<Object>::null(), 0, false);
-  }
+  if (debug->is_active()) debug->HandleStepIn(getter, false);
 
   return Execution::Call(isolate, getter, receiver, 0, NULL, true);
 }
@@ -457,9 +455,7 @@ MaybeHandle<Object> Object::SetPropertyWithDefinedSetter(
   Debug* debug = isolate->debug();
   // Handle stepping into a setter if step into is active.
   // TODO(rossberg): should this apply to getters that are function proxies?
-  if (debug->is_active()) {
-    debug->HandleStepIn(setter, Handle<Object>::null(), 0, false);
-  }
+  if (debug->is_active()) debug->HandleStepIn(setter, false);
 
   Handle<Object> argv[] = { value };
   RETURN_ON_EXCEPTION(isolate, Execution::Call(isolate, setter, receiver,

src/runtime/runtime-classes.cc

@@ -470,9 +470,7 @@ RUNTIME_FUNCTION(Runtime_HandleStepInForDerivedConstructors) {
   CONVERT_ARG_HANDLE_CHECKED(JSFunction, function, 0);
   Debug* debug = isolate->debug();
   // Handle stepping into constructors if step into is active.
-  if (debug->StepInActive()) {
-    debug->HandleStepIn(function, Handle<Object>::null(), 0, true);
-  }
+  if (debug->StepInActive()) debug->HandleStepIn(function, true);
   return *isolate->factory()->undefined_value();
 }
 

src/runtime/runtime-debug.cc

@@ -3212,6 +3212,17 @@ RUNTIME_FUNCTION(Runtime_DebugIsActive) {
 }
 
 
+RUNTIME_FUNCTION(Runtime_DebugHandleStepIntoAccessor) {
+  HandleScope scope(isolate);
+  DCHECK(args.length() == 2);
+  CONVERT_ARG_HANDLE_CHECKED(JSFunction, function, 0);
+  Debug* debug = isolate->debug();
+  // Handle stepping into constructors if step into is active.
+  if (debug->StepInActive()) debug->HandleStepIn(function, false);
+  return *isolate->factory()->undefined_value();
+}
+
+
 RUNTIME_FUNCTION(Runtime_DebugBreakInOptimizedCode) {
   UNIMPLEMENTED();
   return NULL;

src/runtime/runtime-object.cc

@@ -1072,9 +1072,7 @@ static Object* Runtime_NewObjectHelper(Isolate* isolate,
 
   Debug* debug = isolate->debug();
   // Handle stepping into constructors if step into is active.
-  if (debug->StepInActive()) {
-    debug->HandleStepIn(function, Handle<Object>::null(), 0, true);
-  }
+  if (debug->StepInActive()) debug->HandleStepIn(function, true);
 
   if (function->has_initial_map()) {
     if (function->initial_map()->instance_type() == JS_FUNCTION_TYPE) {

test/mjsunit/debug-stepin-accessor-ic.js

@@ -0,0 +1,49 @@
+// Copyright 2014 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --expose-debug-as debug
+
+function get() {
+  return 3;                       // Break
+}                                 // Break
+
+function set(x) {
+  this.x = x;                     // Break
+}                                 // Break
+
+var o = {};
+Object.defineProperty(o, "get", { get : get });
+Object.defineProperty(o, "set", { set : set });
+
+function f() {
+  for (var i = 0; i < 10; i++) {  // Break
+    o.get;                        // Break
+    o.set = 1;                    // Break
+  }
+}                                 // Break
+
+var break_count = 0;
+var exception = null;
+
+function listener(event, exec_state, event_data, data) {
+  if (event != Debug.DebugEvent.Break) return;
+  try {
+    var source_line = exec_state.frame(0).sourceLineText();
+    assertTrue(source_line.indexOf("// Break") > 0);
+    exec_state.prepareStep(Debug.StepAction.StepIn, 1);
+    break_count++;
+  } catch (e) {
+    exception = e;
+  }
+}
+
+var Debug = debug.Debug;
+Debug.setListener(listener);
+
+debugger;                         // Break
+f();                              // Break
+
+Debug.setListener(null);          // Break
+assertEquals(86, break_count);
+assertNull(exception);