[debugger] tuned StepNext and StepOut at return position

src/debug/debug.cc

Index: src/debug/debug.cc
diff --git a/src/debug/debug.cc b/src/debug/debug.cc
index feca580bb86085ea09beca9b0046c8a3404c11d4..d5c2a316ec1c144e842e3f8f66b6893bce5962fb 100644
--- a/src/debug/debug.cc
+++ b/src/debug/debug.cc
@@ -389,6 +389,9 @@ void Debug::ThreadInit() {
   thread_local_.last_step_action_ = StepNone;
   thread_local_.last_statement_position_ = kNoSourcePosition;
   thread_local_.last_frame_count_ = -1;
+  thread_local_.ignore_next_stepping_break_ = false;
+  thread_local_.ignore_step_into_function_ = Smi::kZero;
+  thread_local_.step_into_function_call_requested_ = false;
   thread_local_.target_frame_count_ = -1;
   thread_local_.return_value_ = Smi::kZero;
   thread_local_.async_task_count_ = 0;
@@ -553,6 +556,7 @@ void Debug::Break(JavaScriptFrame* frame) {
       break;
     }
   }
+  step_break = step_break && !thread_local_.ignore_next_stepping_break_;
 
   // Clear all current stepping setup.
   ClearStepping();
@@ -814,7 +818,8 @@ void Debug::ClearAllBreakPoints() {
   }
 }
 
-void Debug::FloodWithOneShot(Handle<SharedFunctionInfo> shared) {
+void Debug::FloodWithOneShot(Handle<SharedFunctionInfo> shared,
+                             bool returns_only) {
   if (IsBlackboxed(shared)) return;
   // Make sure the function is compiled and has set up the debug info.
   if (!EnsureDebugInfo(shared)) return;
@@ -822,11 +827,13 @@ void Debug::FloodWithOneShot(Handle<SharedFunctionInfo> shared) {
   // Flood the function with break points.
   if (debug_info->HasDebugCode()) {
     for (CodeBreakIterator it(debug_info); !it.Done(); it.Next()) {
+      if (returns_only && !it.GetBreakLocation().IsReturn()) continue;
       it.SetDebugBreak();
     }
   }
   if (debug_info->HasDebugBytecodeArray()) {
     for (BytecodeArrayBreakIterator it(debug_info); !it.Done(); it.Next()) {
+      if (returns_only && !it.GetBreakLocation().IsReturn()) continue;
       it.SetDebugBreak();
     }
   }
@@ -876,10 +883,15 @@ MaybeHandle<FixedArray> Debug::GetHitBreakPointObjects(
 }
 
 void Debug::PrepareStepIn(Handle<JSFunction> function) {
-  CHECK(last_step_action() >= StepIn);
+  CHECK(step_into_function_call_requested());
   if (ignore_events()) return;
   if (in_debug_scope()) return;
   if (break_disabled()) return;
+  Handle<SharedFunctionInfo> shared(function->shared());
+  if (IsBlackboxed(shared)) return;
+  if (*function == thread_local_.ignore_step_into_function_) return;
+  thread_local_.ignore_step_into_function_ = Smi::kZero;
+  thread_local_.last_step_action_ = StepIn;
   FloodWithOneShot(Handle<SharedFunctionInfo>(function->shared(), isolate_));
 }
 
@@ -985,7 +997,6 @@ void Debug::PrepareStep(StepAction step_action) {
   feature_tracker()->Track(DebugFeatureTracker::kStepping);
 
   thread_local_.last_step_action_ = step_action;
-  UpdateHookOnFunctionCall();
 
   StackTraceFrameIterator frames_it(isolate_, frame_id);
   StandardFrame* frame = frames_it.frame();
@@ -1012,6 +1023,16 @@ void Debug::PrepareStep(StepAction step_action) {
 
   BreakLocation location = BreakLocation::FromFrame(debug_info, js_frame);
 
+  thread_local_.step_into_function_call_requested_ = step_action == StepIn;
+  if (location.IsReturn()) {

[Yang, 2017/03/21 13:13:52]

Please add comments!

How about this slight change:
- If we perform StepOut, and we are not at a return, we set a
fast_forward_to_return_ flag, and instrument all returns of the current
function. (yes, let's rename ignore_next_stepping_break_).
- In Debug::Break, if the fast_forward_to_return flag is set, we simply DCHECK
that we are indeed at a return, and the target frame count is the same as the
current frame count (to prevent breaking in a recursive call), and call
PrepareStep to StepOut.

- If we perform StepOut, and we are at a return, we store StepIn to
last_step_action_, remember ignore_step_into_function_, and also instrument the
caller. When then UpdateHookInFunctionCall.
- Afterwards, we could break either in the caller, or we could break through
DebugOnFunctionCall. There, we check that we are not stepping into
ignore_step_info_function_.

This way, I don't think we have to introduce a
step_into_function_call_requested_ flag.

[kozy, 2017/03/21 16:03:11]

Done! My goals with current solutions were: doesn't change last_step_action_
since I think about this as a last user requested step action and have minimum
changes in Break. But at the same time I like idea to introduce less additional
state.

+    thread_local_.step_into_function_call_requested_ = true;
+    if (last_step_action() == StepOut) {
+      thread_local_.ignore_step_into_function_ = *function;
+    }
+    step_action = StepOut;
+  }
+  UpdateHookOnFunctionCall();
+
   // Any step at a return is a step-out.
   if (location.IsReturn()) step_action = StepOut;

[Yang, 2017/03/21 13:13:52]

We already have this here.

[kozy, 2017/03/21 16:03:11]

removed.

   // A step-next at a tail call is a step-out.
@@ -1034,6 +1055,12 @@ void Debug::PrepareStep(StepAction step_action) {
       // Clear last position info. For stepping out it does not matter.
       thread_local_.last_statement_position_ = kNoSourcePosition;
       thread_local_.last_frame_count_ = -1;
+      if (!location.IsReturn() && !IsBlackboxed(shared)) {

[Yang, 2017/03/21 13:13:52]

How can IsBlackboxed return true?

[kozy, 2017/03/21 16:03:11]

we can break in blackboxed frame in case when we have not blackboxed frames (not
top one) and call V8Inspector::breakProgram, in line 1041 we do:
if (step_action == StepNext && IsBlackboxed(shared)) step_action = StepOut;

+        thread_local_.target_frame_count_ = current_frame_count;
+        thread_local_.ignore_next_stepping_break_ = true;
+        FloodWithOneShot(shared, true);
+        return;
+      }
       // Skip the current frame, find the first frame we want to step out to
       // and deoptimize every frame along the way.
       bool in_current_frame = true;
@@ -1124,6 +1151,9 @@ void Debug::ClearStepping() {
 
   thread_local_.last_step_action_ = StepNone;
   thread_local_.last_statement_position_ = kNoSourcePosition;
+  thread_local_.ignore_step_into_function_ = Smi::kZero;
+  thread_local_.step_into_function_call_requested_ = false;
+  thread_local_.ignore_next_stepping_break_ = false;
   thread_local_.last_frame_count_ = -1;
   thread_local_.target_frame_count_ = -1;
   UpdateHookOnFunctionCall();
@@ -2108,7 +2138,7 @@ void Debug::UpdateState() {
 
 void Debug::UpdateHookOnFunctionCall() {
   STATIC_ASSERT(LastStepAction == StepIn);
-  hook_on_function_call_ = thread_local_.last_step_action_ == StepIn ||
+  hook_on_function_call_ = thread_local_.step_into_function_call_requested_ ||
                            isolate_->needs_side_effect_check();
 }