[debugger] tuned StepNext and StepOut at return position

src/debug/debug.cc

 // Copyright 2012 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.
 
 #include "src/debug/debug.h"
 
 #include <memory>
 
 #include "src/api.h"
 #include "src/arguments.h"
@@ skipping 371 matching lines @@
 
 
 // Threading support.
 void Debug::ThreadInit() {
   thread_local_.break_count_ = 0;
   thread_local_.break_id_ = 0;
   thread_local_.break_frame_id_ = StackFrame::NO_ID;
   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;
   clear_suspended_generator();
   thread_local_.restart_fp_ = nullptr;
   base::NoBarrier_Store(&thread_local_.current_debug_scope_,
                         static_cast<base::AtomicWord>(0));
   UpdateHookOnFunctionCall();
 }
 
 
 char* Debug::ArchiveDebug(char* storage) {
   // Simply reset state. Don't archive anything.
   ThreadInit();
   return storage + ArchiveSpacePerThread();
 }
 
 
 char* Debug::RestoreDebug(char* storage) {
   // Simply reset state. Don't restore anything.
   ThreadInit();
   return storage + ArchiveSpacePerThread();
 }
 
 int Debug::ArchiveSpacePerThread() { return 0; }
 
 void Debug::Iterate(ObjectVisitor* v) {
   v->VisitPointer(&thread_local_.return_value_);
   v->VisitPointer(&thread_local_.suspended_generator_);

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

You need to iterate ignore_step_into_function_, or otherwise GC will not update
the pointer correctly.

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

Thanks! done.

 }
 
 DebugInfoListNode::DebugInfoListNode(DebugInfo* debug_info): next_(NULL) {
   // Globalize the request debug info object and make it weak.
   GlobalHandles* global_handles = debug_info->GetIsolate()->global_handles();
   debug_info_ =
       Handle<DebugInfo>::cast(global_handles->Create(debug_info)).location();
 }
 
 
@@ skipping 115 matching lines @@
     // Fall through.
     case StepIn: {
       FrameSummary summary = FrameSummary::GetTop(frame);
       step_break = step_break || location.IsReturn() ||
                    current_frame_count != last_frame_count ||
                    thread_local_.last_statement_position_ !=
                        summary.SourceStatementPosition();
       break;
     }
   }
+  step_break = step_break && !thread_local_.ignore_next_stepping_break_;
 
   // Clear all current stepping setup.
   ClearStepping();
 
   if (step_break) {
     // Notify the debug event listeners.
     OnDebugBreak(isolate_->factory()->undefined_value());
   } else {
     // Re-prepare to continue.
     PrepareStep(step_action);
@@ skipping 241 matching lines @@
   for (DebugInfoListNode* node = debug_info_list_; node != NULL;
        node = node->next()) {
     ClearBreakPoints(node->debug_info());
   }
   // Remove all debug info.
   while (debug_info_list_ != NULL) {
     RemoveDebugInfoAndClearFromShared(debug_info_list_->debug_info());
   }
 }
 
-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;
   Handle<DebugInfo> debug_info(shared->GetDebugInfo());
   // 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();
     }
   }
 }
 
 void Debug::ChangeBreakOnException(ExceptionBreakType type, bool enable) {
   if (type == BreakUncaughtException) {
     break_on_uncaught_exception_ = enable;
   } else {
     break_on_exception_ = enable;
@@ skipping 29 matching lines @@
     if (CheckBreakPoint(break_point_object)) {
       break_points_hit->set(break_points_hit_count++, *break_point_object);
     }
   }
   if (break_points_hit_count == 0) return {};
   break_points_hit->Shrink(break_points_hit_count);
   return break_points_hit;
 }
 
 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_));
 }
 
 void Debug::PrepareStepInSuspendedGenerator() {
   CHECK(has_suspended_generator());
   if (ignore_events()) return;
   if (in_debug_scope()) return;
   if (break_disabled()) return;
   thread_local_.last_step_action_ = StepIn;
   UpdateHookOnFunctionCall();
@@ skipping 85 matching lines @@
   // any. The debug frame will only be present if execution was stopped due to
   // hitting a break point. In other situations (e.g. unhandled exception) the
   // debug frame is not present.
   StackFrame::Id frame_id = break_frame_id();
   // If there is no JavaScript stack don't do anything.
   if (frame_id == StackFrame::NO_ID) return;
 
   feature_tracker()->Track(DebugFeatureTracker::kStepping);
 
   thread_local_.last_step_action_ = step_action;
-  UpdateHookOnFunctionCall();
 
   StackTraceFrameIterator frames_it(isolate_, frame_id);
   StandardFrame* frame = frames_it.frame();
 
   // Handle stepping in wasm functions via the wasm interpreter.
   if (frame->is_wasm()) {
     // If the top frame is compiled, we cannot step.
     if (frame->is_wasm_compiled()) return;
     WasmInterpreterEntryFrame* wasm_frame =
         WasmInterpreterEntryFrame::cast(frame);
     wasm_frame->wasm_instance()->debug_info()->PrepareStep(step_action);
     return;
   }
 
   JavaScriptFrame* js_frame = JavaScriptFrame::cast(frame);
   DCHECK(js_frame->function()->IsJSFunction());
 
   // Get the debug info (create it if it does not exist).
   auto summary = FrameSummary::GetTop(frame).AsJavaScript();
   Handle<JSFunction> function(summary.function());
   Handle<SharedFunctionInfo> shared(function->shared());
   if (!EnsureDebugInfo(shared)) return;
   Handle<DebugInfo> debug_info(shared->GetDebugInfo());
 
   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.
   if (location.IsTailCall() && step_action == StepNext) step_action = StepOut;
   // A step-next in blackboxed function is a step-out.
   if (step_action == StepNext && IsBlackboxed(shared)) step_action = StepOut;
 
   thread_local_.last_statement_position_ =
       summary.abstract_code()->SourceStatementPosition(summary.code_offset());
   int current_frame_count = CurrentFrameCount();
   thread_local_.last_frame_count_ = current_frame_count;
   // No longer perform the current async step.
   clear_suspended_generator();
 
   switch (step_action) {
     case StepNone:
       UNREACHABLE();
       break;
     case StepOut: {
       // 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;
       for (; !frames_it.done(); frames_it.Advance()) {
         // TODO(clemensh): Implement stepping out from JS to WASM.
         if (frames_it.frame()->is_wasm()) continue;
         JavaScriptFrame* frame = JavaScriptFrame::cast(frames_it.frame());
         if (last_step_action() == StepIn) {
           // Deoptimize frame to ensure calls are checked for step-in.
           Deoptimizer::DeoptimizeFunction(frame->function());
@@ skipping 70 matching lines @@
   }
   return locations;
 }
 
 void Debug::ClearStepping() {
   // Clear the various stepping setup.
   ClearOneShot();
 
   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();
 }
 
 
 // Clears all the one-shot break points that are currently set. Normally this
 // function is called each time a break point is hit as one shot break points
 // are used to support stepping.
 void Debug::ClearOneShot() {
@@ skipping 964 matching lines @@
   } else if (is_loaded()) {
     isolate_->compilation_cache()->Enable();
     Unload();
   }
   is_active_ = is_active;
   isolate_->DebugStateUpdated();
 }
 
 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();
 }
 
 MaybeHandle<Object> Debug::Call(Handle<Object> fun, Handle<Object> data) {
   DebugScope debug_scope(this);
   if (debug_scope.failed()) return isolate_->factory()->undefined_value();
 
   // Create the execution state.
   Handle<Object> exec_state;
   if (!MakeExecutionState().ToHandle(&exec_state)) {
@@ skipping 334 matching lines @@
     isolate_->Throw(*isolate_->factory()->NewEvalError(
         MessageTemplate::kNoSideEffectDebugEvaluate));
   }
   isolate_->set_needs_side_effect_check(old_needs_side_effect_check_);
   isolate_->debug()->UpdateHookOnFunctionCall();
   isolate_->debug()->side_effect_check_failed_ = false;
 }
 
 }  // namespace internal
 }  // namespace v8