Debugging in kernel shaping up.

runtime/vm/kernel_to_il.cc

Index: runtime/vm/kernel_to_il.cc
diff --git a/runtime/vm/kernel_to_il.cc b/runtime/vm/kernel_to_il.cc
index 22299a884a28e0ae894af03c3b33e69ebe3f63a8..e53f031015fde7f2b4fb4926346379100edc9ece 100644
--- a/runtime/vm/kernel_to_il.cc
+++ b/runtime/vm/kernel_to_il.cc
@@ -366,6 +366,20 @@ ScopeBuildingResult* ScopeBuilder::BuildScopes() {
       // will forward the call to the real function.
       //     -> see BuildGraphOfImplicitClosureFunction
       if (!function.IsImplicitClosureFunction()) {
+        // TODO(jensj): HACK: Push the begin token to after any parameters to
+        // avoid crash when breaking on definition line of async method in
+        // debugger. It seems that another scope needs to be added
+        // in which captures are made, but I can't make that work.
+        // This 'solution' doesn't crash, but I cannot see the parameters at
+        // that particular breakpoint either.
+        // Also push the end token to after the "}" to avoid crashing on
+        // stepping past the last line (to the "}" character).
+        if (node->body() != NULL && node->body()->position().IsReal()) {
+          scope_->set_begin_token_pos(node->body()->position());
+        }
+        if (scope_->end_token_pos().IsReal()) {
+          scope_->set_end_token_pos(scope_->end_token_pos().Next());
+        }
         node_->AcceptVisitor(this);
       }
       break;
@@ -2525,10 +2539,27 @@ Fragment FlowGraphBuilder::PushArgument() {
 Fragment FlowGraphBuilder::Return(TokenPosition position) {
   Value* value = Pop();
   ASSERT(stack_ == NULL);
-  ReturnInstr* return_instr =
-      new (Z) ReturnInstr(TokenPosition::kNoSource, value);
+
+  Fragment fragment;
+
+  // Doing what's done in EffectGraphVisitor::VisitReturnNode.

[Kevin Millikin (Google), 2017/01/24 13:44:13]

This kind of comment isn't useful.

It won't even make sense (at least might stop being true) if something changes
at a remote place.

[jensj, 2017/01/25 12:52:22]

I will remove the comment.

+
+  // Call to stub that checks whether the debugger is in single
+  // step mode. This call must happen before the contexts are
+  // unchained so that captured variables can be inspected.
+  // No debugger check is done in native functions or for return
+  // statements for which there is no associated source position.
+  const Function& function = parsed_function_->function();

[Kevin Millikin (Google), 2017/01/24 13:44:13]

Note that we are not doing the same thing as the classic flow graph builder's
VisitReturnNode, anyway.  There, we place the single-step check before any
finally blocks, and here we place it after.

That might explain why you are having trouble stepping past the end positions of
some functions.

[jensj, 2017/01/25 12:52:22]

I don't think that's the issue (in regards to the hack above), but I will
investigate.

+  if (FLAG_support_debugger && position.IsDebugPause() &&
+      !function.is_native()) {
+    fragment <<=
+        new (Z) DebugStepCheckInstr(position, RawPcDescriptors::kRuntimeCall);
+  }
+
+  ReturnInstr* return_instr = new (Z) ReturnInstr(position, value);
   if (exit_collector_ != NULL) exit_collector_->AddExit(return_instr);
-  return Fragment(return_instr).closed();
+  fragment <<= return_instr;
+  return fragment.closed();
 }
 
 
@@ -2648,6 +2679,18 @@ Fragment FlowGraphBuilder::StoreLocal(TokenPosition position,
         StoreInstanceField(Context::variable_offset(variable->index()));
   } else {
     Value* value = Pop();
+    // Do approximately as in EffectGraphVisitor::VisitStoreLocalNode.

[Kevin Millikin (Google), 2017/01/24 13:44:13]

Again, this comment isn't useful.

This seems somewhat surprisingly different from what the other flow graph
builder is doing.  There we are adding the single-step check before the code for
evaluating the right-hand side of some (but not all) source variable
assignments.  Here we are adding it for a possibly different set of assignments
that both includes and excludes some, and we're doing it after the code for
evaluating the right-hand side.

I think I'd be more comfortable if it was obviously the same.  If it's not, we
need a comment here that says more than just "approximately the same".  Ideally
we'd have the team come up with (1) a description of which positions need
single-step checks and whether it's a correctness or safety issue to have too
many or too few, (2) a characterization of the ones that differ between the two
front-ends, and (3) an argument that's it's OK that they differ.

[jensj, 2017/01/25 12:52:22]

I will remove the comment.

I would suggest going in detail with the insertions of DebugStepCheckInstr in
another CL though.

a) The insertion seems rather arbitrary anyway.
b) I have refactored it quite a bit for an upcomming CL anyways (where I have
done some comparison of the insertions via --print-flow-graph, though I could
probably do more)
c) it seems to me that doing too few will simply not allow us to stop at points
where we would want to stop (and as a note, having none we definitely have too
few). Whether having to many can do any harm I'm not sure. I can't really see
how though.

+    if (FLAG_support_debugger && position.IsDebugPause() &&
+        !variable->IsInternal()) {
+      if (value->definition()->IsConstant() ||
+          value->definition()->IsAllocateObject() ||
+          (value->definition()->IsLoadLocal() &&
+           !value->definition()->AsLoadLocal()->local().IsInternal())) {
+        instructions <<= new (Z)
+            DebugStepCheckInstr(position, RawPcDescriptors::kRuntimeCall);
+      }
+    }
+
     StoreLocalInstr* store =
         new (Z) StoreLocalInstr(*variable, value, position);
     instructions <<= store;
@@ -3161,6 +3204,35 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFunction(FunctionNode* function,
     context_depth_ = current_context_depth;
   }
 
+  if (FLAG_support_debugger && function->position().IsDebugPause() &&
+      !dart_function.is_native() && dart_function.is_debuggable()) {
+    // If a switch was added above: Start the switch by injecting a debugable
+    // safepoint so stepping over an await works.
+    // If not, still start the body with a debugable safepoint to ensure
+    // breaking on a method always happens, even if there are no
+    // assignments/calls/runtimecalls in the first basic block.
+    // Place this check at the last parameter to ensure parameters
+    // are in scope in the debugger at method entry.
+    // Basically copied from EffectGraphVisitor::VisitSequenceNode.
+    Fragment dispatch;
+    const int num_params = dart_function.NumParameters();
+    TokenPosition check_pos = TokenPosition::kNoSource;
+    if (num_params > 0) {
+      LocalScope* scope = parsed_function_->node_sequence()->scope();
+      const LocalVariable& parameter = *scope->VariableAt(num_params - 1);
+      check_pos = parameter.token_pos();
+    }
+    if (!check_pos.IsDebugPause()) {
+      // No parameters or synthetic parameters.
+      check_pos = function->position();
+      ASSERT(check_pos.IsDebugPause());
+    }
+    dispatch <<=

[Kevin Millikin (Google), 2017/01/24 13:44:13]

I'm not sure why this is called dispatch.  I don't think of it as dispatching. 
I'd probably just write:

Fragment check(new (Z) DebugStepCheckInstr(...));
body = check + body;

[jensj, 2017/01/25 12:52:22]

Done.

+        new (Z) DebugStepCheckInstr(check_pos, RawPcDescriptors::kRuntimeCall);
+    dispatch.entry->LinkTo(body.entry);
+    body = dispatch;
+  }
+
   normal_entry->LinkTo(body.entry);
 
   // When compiling for OSR, use a depth first search to prune instructions