Better single stepping in VM debugger

runtime/vm/debugger.cc

Index: runtime/vm/debugger.cc
===================================================================
--- runtime/vm/debugger.cc	(revision 24600)
+++ runtime/vm/debugger.cc	(working copy)
@@ -773,6 +773,7 @@
       code_breakpoints_(NULL),
       resume_action_(kContinue),
       ignore_breakpoints_(false),
+      in_event_notification_(false),
       exc_pause_info_(kNoPauseOnExceptions) {
 }
 
@@ -780,6 +781,7 @@
 Debugger::~Debugger() {
   PortMap::ClosePort(isolate_id_);
   isolate_id_ = ILLEGAL_ISOLATE_ID;
+  ASSERT(!in_event_notification_);
   ASSERT(src_breakpoints_ == NULL);
   ASSERT(code_breakpoints_ == NULL);
   ASSERT(stack_trace_ == NULL);
@@ -815,7 +817,21 @@
   return Function::null();
 }
 
+void Debugger::SetSingleStep() {
+  isolate_->set_single_step(true);
+  resume_action_ = kSingleStep;
+}
 
+void Debugger::SetStepOver() {
+  isolate_->set_single_step(false);
+  resume_action_ = kStepOver;
+}
+
+void Debugger::SetStepOut() {
+  isolate_->set_single_step(false);
+  resume_action_ = kStepOut;
+}
+
 RawFunction* Debugger::ResolveFunction(const Library& library,
                                        const String& class_name,
                                        const String& function_name) {
@@ -869,6 +885,10 @@
 
 
 void Debugger::InstrumentForStepping(const Function& target_function) {
+  if (target_function.is_native()) {
+    // Can't instrument native functions.
+    return;
+  }
   if (!target_function.HasCode()) {
     Compiler::CompileFunction(target_function);
     // If there were any errors, ignore them silently and return without
@@ -966,6 +986,24 @@
 }
 
 
+ActivationFrame* Debugger::TopDartFrame() {
+  StackFrameIterator iterator(false);
+  StackFrame* frame = iterator.NextFrame();
+  while ((frame != NULL) && !frame->IsDartFrame()) {
+    frame = iterator.NextFrame();
+  }
+  Code& code = Code::Handle(isolate_, frame->LookupDartCode());
+  ActivationFrame* activation =
+      new ActivationFrame(frame->pc(), frame->fp(), frame->sp(), code);
+  return activation;
+}
+
+
+DebuggerStackTrace* Debugger::StackTrace() {
+  return (stack_trace_ != NULL) ? stack_trace_ : CollectStackTrace();
+}
+
+
 void Debugger::SetExceptionPauseInfo(Dart_ExceptionPauseInfo pause_info) {
   ASSERT((pause_info == kNoPauseOnExceptions) ||
          (pause_info == kPauseOnUnhandledExceptions) ||
@@ -1007,7 +1045,7 @@
   // breakpoint or exception event, or if the debugger is not
   // interested in exception events.
   if (ignore_breakpoints_ ||
-      (stack_trace_ != NULL) ||
+      in_event_notification_ ||
       (event_handler_ == NULL) ||
       (exc_pause_info_ == kNoPauseOnExceptions)) {
     return;
@@ -1023,7 +1061,6 @@
   DebuggerEvent event;
   event.type = kExceptionThrown;
   event.exception = &exc;
-  ASSERT(event_handler_ != NULL);
   (*event_handler_)(&event);
   stack_trace_ = NULL;
   obj_cache_ = NULL;  // Remote object cache is zone allocated.
@@ -1506,11 +1543,69 @@
 }
 
 
+void Debugger::SignalPausedEvent(ActivationFrame* top_frame) {
+  resume_action_ = kContinue;
+  isolate_->set_single_step(false);
+  ASSERT(!in_event_notification_);
+  ASSERT(obj_cache_ == NULL);
+  in_event_notification_ = true;
+  obj_cache_ = new RemoteObjectCache(64);
+  DebuggerEvent event;
+  event.type = kBreakpointReached;
+  event.top_frame = top_frame;
+  (*event_handler_)(&event);
+  in_event_notification_ = false;
+  obj_cache_ = NULL;  // Remote object cache is zone allocated.
+}
+
+
+void Debugger::SingleStepCallback() {
+  ASSERT(resume_action_ == kSingleStep);
+  ASSERT(isolate_->single_step());
+  // We can't get here unless the debugger event handler enabled
+  // single stepping.
+  ASSERT(event_handler_ != NULL);
+  // Don't pause recursively.
+  if (in_event_notification_) return;
+
+  // Check whether we are in a Dart function that the user is
+  // interested in.
+  ActivationFrame* frame = TopDartFrame();
+  ASSERT(frame != NULL);
+  const Function& func = frame->function();
+  if (!IsDebuggable(func)) {
+    return;
+  }
+
+  if (FLAG_verbose_debug) {
+    OS::Print(">>> single step break at %s:%"Pd" (func %s token %"Pd")\n",
+              String::Handle(frame->SourceUrl()).ToCString(),
+              frame->LineNumber(),
+              String::Handle(frame->QualifiedFunctionName()).ToCString(),
+              frame->TokenPos());
+  }
+
+  stack_trace_ = CollectStackTrace();
+  SignalPausedEvent(frame);
+
+  RemoveInternalBreakpoints();
+  if (resume_action_ == kStepOver) {
+    InstrumentForStepping(func);
+  } else if (resume_action_ == kStepOut) {
+    if (stack_trace_->Length() > 1) {
+      ActivationFrame* caller_frame = stack_trace_->ActivationFrameAt(1);
+      InstrumentForStepping(caller_frame->function());
+    }
+  }

[siva, 2013/07/01 17:04:48]

I might have asked this question before, how does this work when one is
currently in a function and is trying to step out or step over a recursive
invocation of the same function that we are currently stopped in.

[hausner, 2013/08/09 09:59:02]

Stepping out of a function works by setting temporary breakpoints in the caller,
then resume. In the case of recursive calls, this is indeed not a very good
solution since the function that gets instrumented with temp breakpoints is the
same function we're currently stopped in.

I could walk the stack and instrument the first function that is different from
the current function. But that would not allow the user to step out one
recursive activation frame at a time.

Maybe we can discuss this offline again.

+  stack_trace_ = NULL;
+}
+
+
 void Debugger::SignalBpReached() {
   // We ignore this breakpoint when the VM is executing code invoked
   // by the debugger to evaluate variables values, or when we see a nested
   // breakpoint or exception event.
-  if (ignore_breakpoints_ || (stack_trace_ != NULL)) {
+  if (ignore_breakpoints_ || in_event_notification_) {
     return;
   }
   DebuggerStackTrace* stack_trace = CollectStackTrace();
@@ -1519,9 +1614,15 @@
   ASSERT(top_frame != NULL);
   CodeBreakpoint* bpt = GetCodeBreakpoint(top_frame->pc());
   ASSERT(bpt != NULL);
+
+  bool report_bp = true;
+  if (bpt->IsInternal() && !IsDebuggable(top_frame->function())) {
+    report_bp = false;
+  }
   if (FLAG_verbose_debug) {
-    OS::Print(">>> hit %s breakpoint at %s:%"Pd" "
+    OS::Print(">>> %s %s breakpoint at %s:%"Pd" "
               "(token %"Pd") (address %#"Px")\n",
+              report_bp ? "hit" : "ignore",
               bpt->IsInternal() ? "internal" : "user",
               String::Handle(bpt->SourceUrl()).ToCString(),
               bpt->LineNumber(),
@@ -1529,117 +1630,34 @@
               top_frame->pc());
   }
 
-  resume_action_ = kContinue;
-  if (event_handler_ != NULL) {
-    ASSERT(stack_trace_ == NULL);
-    ASSERT(obj_cache_ == NULL);
-    obj_cache_ = new RemoteObjectCache(64);
+  if (report_bp && (event_handler_ != NULL)) {
     stack_trace_ = stack_trace;
-    DebuggerEvent event;
-    event.type = kBreakpointReached;
-    ASSERT(stack_trace->Length() > 0);
-    event.top_frame = stack_trace->ActivationFrameAt(0);
-    (*event_handler_)(&event);
+    SignalPausedEvent(top_frame);
     stack_trace_ = NULL;
-    obj_cache_ = NULL;  // Remote object cache is zone allocated.
   }
 
-  Function& currently_instrumented_func = Function::Handle();
-  if (bpt->IsInternal()) {
-    currently_instrumented_func = bpt->function();
-  }
   Function& func_to_instrument = Function::Handle();
-  if (resume_action_ == kContinue) {
-    // Nothing to do here, any potential instrumentation will be removed
-    // below.
-  } else if (resume_action_ == kStepOver) {
-    func_to_instrument = bpt->function();
+  if (resume_action_ == kStepOver) {
     if (bpt->breakpoint_kind_ == PcDescriptors::kReturn) {
-      // If we are at the function return, do a StepOut action.
-      if (stack_trace->Length() > 1) {
-        ActivationFrame* caller_frame = stack_trace->ActivationFrameAt(1);
-        func_to_instrument = caller_frame->function().raw();
-      }
-    }
-  } else if (resume_action_ == kStepInto) {
-    // If the call target is not debuggable, we treat StepInto like
-    // a StepOver, that is we instrument the current function.
-    if (bpt->breakpoint_kind_ == PcDescriptors::kIcCall) {
+      // Step over return is converted into a single step so we break at
+      // the caller.
+      SetSingleStep();
+    } else {
       func_to_instrument = bpt->function();
-      ICData& ic_data = ICData::Handle();
-      const Code& code =
-          Code::Handle(Function::Handle(bpt->function_).unoptimized_code());
-      CodePatcher::GetInstanceCallAt(bpt->pc_, code, &ic_data);
-      ArgumentsDescriptor
-          args_descriptor(Array::Handle(ic_data.arguments_descriptor()));
-      ActivationFrame* top_frame = stack_trace->ActivationFrameAt(0);
-      intptr_t num_args = args_descriptor.Count();
-      Instance& receiver =
-          Instance::Handle(top_frame->GetInstanceCallReceiver(num_args));
-      Code& target_code =
-          Code::Handle(ResolveCompileInstanceCallTarget(receiver, ic_data));
-      if (!target_code.IsNull()) {
-        Function& callee = Function::Handle(target_code.function());
-        if (IsDebuggable(callee)) {
-          func_to_instrument = callee.raw();
-        }
-      }
-    } else if (bpt->breakpoint_kind_ == PcDescriptors::kUnoptStaticCall) {
-      func_to_instrument = bpt->function();
-      const Code& code = Code::Handle(func_to_instrument.CurrentCode());
-      ASSERT(!code.is_optimized());
-      const Function& callee = Function::Handle(
-          CodePatcher::GetUnoptimizedStaticCallAt(bpt->pc_, code, NULL));
-      ASSERT(!callee.IsNull());
-      if (IsDebuggable(callee)) {
-        func_to_instrument = callee.raw();
-      }
-    } else if (bpt->breakpoint_kind_ == PcDescriptors::kClosureCall) {
-      func_to_instrument = bpt->function();
-      const Code& code = Code::Handle(func_to_instrument.CurrentCode());
-      ArgumentsDescriptor args_desc(Array::Handle(
-          CodePatcher::GetClosureArgDescAt(bpt->pc_, code)));
-      ActivationFrame* top_frame = stack_trace->ActivationFrameAt(0);
-      const Object& receiver =
-          Object::Handle(top_frame->GetClosureObject(args_desc.Count()));
-      if (!receiver.IsNull()) {
-        // Verify that the class of receiver is a closure class
-        // by checking that signature_function() is not null.
-        const Class& receiver_class = Class::Handle(receiver.clazz());
-        if (receiver_class.IsSignatureClass()) {
-          Function& closure_func =
-              Function::Handle(Closure::function(Instance::Cast(receiver)));
-          if (IsDebuggable(closure_func)) {
-            func_to_instrument = closure_func.raw();
-          }
-        }
-        // If the receiver is not a closure, then the runtime will attempt
-        // to invoke the "call" method on the object if one exists.
-        // TODO(hausner): find call method and intrument it for stepping.
-      }
-    } else if (bpt->breakpoint_kind_ == PcDescriptors::kRuntimeCall) {
-      // This is just a call to the runtime, not Dart code. Stepping
-      // into not possible, just treat like StepOver.
-      func_to_instrument = bpt->function();
-    } else {
-      ASSERT(bpt->breakpoint_kind_ == PcDescriptors::kReturn);
-      // Treat like stepping out to caller.
-      if (stack_trace->Length() > 1) {
-        ActivationFrame* caller_frame = stack_trace->ActivationFrameAt(1);
-        func_to_instrument = caller_frame->function().raw();
-      }
     }
-  } else {
-    ASSERT(resume_action_ == kStepOut);
-    // Set stepping breakpoints in the caller.
+  } else if (resume_action_ == kStepOut) {
     if (stack_trace->Length() > 1) {
       ActivationFrame* caller_frame = stack_trace->ActivationFrameAt(1);
       func_to_instrument = caller_frame->function().raw();
     }
+  } else {
+    ASSERT((resume_action_ == kContinue) || (resume_action_ == kSingleStep));
+    // Nothing to do here. Any potential instrumentation will be removed
+    // below. Single stepping is handled by the single step callback.
   }
 
   if (func_to_instrument.IsNull() ||
-      (func_to_instrument.raw() != currently_instrumented_func.raw())) {
+      (func_to_instrument.raw() != bpt->function())) {
     RemoveInternalBreakpoints();  // *bpt is now invalid.
     if (!func_to_instrument.IsNull()) {
       InstrumentForStepping(func_to_instrument);