Kernel debugging; service tests

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 e1251535705c70a0680a041c6a93efe8cda51b43..8d794a4dc2d67ae2124a78b25826ca1ed23cc49c 100644
--- a/runtime/vm/kernel_to_il.cc
+++ b/runtime/vm/kernel_to_il.cc
@@ -1328,6 +1328,36 @@ const Array& TranslationHelper::ArgumentNames(List<NamedExpression>* named) {
   return names;
 }
 
+bool TranslationHelper::ShouldAddDebugInstruction(const Function& function,

[Kevin Millikin (Google), 2017/02/08 15:37:52]

Rename this to something like NeedsDebugStepCheck.

Why?  First it's just a predicate and it doesn't care how the caller will
interpret the result.  We could use the result for anything, not necessarily
adding an instruction.  It will end up looking really strange at the call site
if we end up using it for some other purpose.

Second, the "DebugInstruction" has a name which is DebugStepCheckInstr.  That's
not necessarily a great name, but it's confusing to arbitrarily use different
names at each level of abstraction.

[jensj, 2017/02/13 14:04:17]

Done.

+                                                  TokenPosition position) {
+  if (!(FLAG_support_debugger && position.IsDebugPause())) return false;

[Kevin Millikin (Google), 2017/02/08 15:37:53]

I think this reads better as:

return FLAG_support_debugger &&
    position.IsDebugPause() &&
    !function.is_native() &&
    function.is_debuggable();

[jensj, 2017/02/13 14:04:17]

Semi-done. git cl format runtime has a different idea about line-breaks.

+  return !function.is_native() && function.is_debuggable();
+}
+
+
+bool TranslationHelper::ShouldAddDebugInstruction(Value* value,

[Kevin Millikin (Google), 2017/02/08 15:37:52]

This function's implementation is cluttered.  Suggestions below.

[jensj, 2017/02/13 14:04:17]

Acknowledged.

+                                                  TokenPosition position) {
+  if (!(FLAG_support_debugger && position.IsDebugPause())) return false;

[Kevin Millikin (Google), 2017/02/08 15:37:52]

Push negation in (something like negation normal form).  It's easier to read:

if (!FLAG_support_debugger || !position.IsDebugPause()) return false;

[jensj, 2017/02/13 14:04:17]

Done.

+  if (value->definition()->IsConstant()) return true;

[Kevin Millikin (Google), 2017/02/08 15:37:52]

Name value->definition(), it will not change and we don't have to repeat it
everywhere:

Definition* definition = value->definition();

Put the simple cases first:

if (definition->IsConstant() || definition->IsLoadStaticField()) return true;

[jensj, 2017/02/13 14:04:17]

Done.

+  if (value->definition()->IsAllocateObject()) {
+    return !value->definition()
+                ->AsAllocateObject()
+                ->closure_function()
+                .IsNull();
+  }
+  if (value->definition()->IsLoadLocal() &&

[Kevin Millikin (Google), 2017/02/08 15:37:52]

Just return the value of the last test:

return definition->IsLoadLocal() &&
    !definition->AsLoadLocal()->local().IsInternal();

[jensj, 2017/02/13 14:04:17]

Done.

+      !value->definition()->AsLoadLocal()->local().IsInternal()) {
+    return true;
+  }
+  if (value->definition()->IsLoadStaticField()) return true;
+  return false;
+}
+
+Fragment TranslationHelper::GetDebugInstruction(TokenPosition position) {

[Kevin Millikin (Google), 2017/02/08 15:37:52]

Move this to the graph builder.  Use the same name as the flow graph instruction
otherwise it looks complicated when it's in fact simple.  (It's not "getting"
anything anyway, it's making it.)

TranslationHelper::GetDebugInstruction ==> FlowGraphBuilder::DebugStepCheck.

[jensj, 2017/02/13 14:04:17]

Done.

+  return Fragment(
+      new (Z) DebugStepCheckInstr(position, RawPcDescriptors::kRuntimeCall));
+}
+
 
 ConstantEvaluator::ConstantEvaluator(FlowGraphBuilder* builder,
                                      Zone* zone,
@@ -2067,7 +2097,8 @@ Fragment FlowGraphBuilder::PushContext(int size) {
   LocalVariable* context = MakeTemporary();
   instructions += LoadLocal(context);
   instructions += LoadLocal(parsed_function_->current_context_var());
-  instructions += StoreInstanceField(Context::parent_offset());
+  instructions +=
+      StoreInstanceField(TokenPosition::kNoSource, Context::parent_offset());
   instructions += StoreLocal(TokenPosition::kNoSource,
                              parsed_function_->current_context_var());
   ++context_depth_;
@@ -2548,11 +2579,8 @@ Fragment FlowGraphBuilder::Return(TokenPosition position) {
   Value* value = Pop();
   ASSERT(stack_ == NULL);
 
-  const Function& function = parsed_function_->function();
-  if (FLAG_support_debugger && position.IsDebugPause() &&
-      !function.is_native()) {
-    instructions <<=
-        new (Z) DebugStepCheckInstr(position, RawPcDescriptors::kRuntimeCall);
+  if (H.ShouldAddDebugInstruction(parsed_function_->function(), position)) {
+    instructions += H.GetDebugInstruction(position);
   }
 
   ReturnInstr* return_instr = new (Z) ReturnInstr(position, value);
@@ -2670,6 +2698,7 @@ Fragment FlowGraphBuilder::StoreInstanceFieldGuarded(
 
 
 Fragment FlowGraphBuilder::StoreInstanceField(
+    TokenPosition position,
     intptr_t offset,
     StoreBarrierType emit_store_barrier) {
   Value* value = Pop();
@@ -2677,7 +2706,7 @@ Fragment FlowGraphBuilder::StoreInstanceField(
     emit_store_barrier = kNoStoreBarrier;
   }
   StoreInstanceFieldInstr* store = new (Z) StoreInstanceFieldInstr(
-      offset, Pop(), value, emit_store_barrier, TokenPosition::kNoSource);
+      offset, Pop(), value, emit_store_barrier, position);
   return Fragment(store);
 }
 
@@ -2689,21 +2718,10 @@ Fragment FlowGraphBuilder::StoreLocal(TokenPosition position,
     LocalVariable* value = MakeTemporary();
     instructions += LoadContextAt(variable->owner()->context_level());
     instructions += LoadLocal(value);
-    instructions +=
-        StoreInstanceField(Context::variable_offset(variable->index()));
+    instructions += StoreInstanceField(
+        position, Context::variable_offset(variable->index()));
   } else {
     Value* value = Pop();
-    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;
@@ -2713,9 +2731,10 @@ Fragment FlowGraphBuilder::StoreLocal(TokenPosition position,
 }
 
 
-Fragment FlowGraphBuilder::StoreStaticField(const dart::Field& field) {
-  return Fragment(new (Z) StoreStaticFieldInstr(MayCloneField(Z, field), Pop(),
-                                                TokenPosition::kNoSource));
+Fragment FlowGraphBuilder::StoreStaticField(TokenPosition position,
+                                            const dart::Field& field) {
+  return Fragment(
+      new (Z) StoreStaticFieldInstr(MayCloneField(Z, field), Pop(), position));
 }
 
 
@@ -3086,7 +3105,8 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFunction(FunctionNode* function,
         // eligible for garbage collection.
         body += LoadLocal(context);
         body += LoadLocal(parameter);
-        body += StoreInstanceField(Context::variable_offset(variable->index()));
+        body += StoreInstanceField(TokenPosition::kNoSource,
+                                   Context::variable_offset(variable->index()));
         body += NullConstant();
         body += StoreLocal(TokenPosition::kNoSource, parameter);
         body += Drop();
@@ -3227,7 +3247,6 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFunction(FunctionNode* function,
       // which acts like an anchor, so we need to skip it.
       then->LinkTo(yield_continuations_[i].entry->next());
       then->set_try_index(yield_continuations_[i].try_index);
-
       // False branch will contain the next comparison.
       dispatch = Fragment(dispatch.entry, otherwise);
       block = otherwise;
@@ -3237,8 +3256,7 @@ 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 (H.ShouldAddDebugInstruction(dart_function, function->position())) {
     // 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
@@ -3258,9 +3276,7 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFunction(FunctionNode* function,
       check_pos = function->position();
       ASSERT(check_pos.IsDebugPause());
     }
-    Fragment check(
-        new (Z) DebugStepCheckInstr(check_pos, RawPcDescriptors::kRuntimeCall));
-    body = check + body;
+    body = H.GetDebugInstruction(check_pos) + body;
   }
 
   normal_entry->LinkTo(body.entry);
@@ -3365,7 +3381,8 @@ Fragment FlowGraphBuilder::NativeFunctionBody(FunctionNode* kernel_function,
       body += LoadLocal(scopes_->this_variable);
       body += LoadLocal(
           LookupVariable(kernel_function->positional_parameters()[0]));
-      body += StoreInstanceField(LinkedHashMap::index_offset());
+      body += StoreInstanceField(TokenPosition::kNoSource,
+                                 LinkedHashMap::index_offset());
       body += NullConstant();
       break;
     case MethodRecognizer::kLinkedHashMap_getData:
@@ -3377,7 +3394,8 @@ Fragment FlowGraphBuilder::NativeFunctionBody(FunctionNode* kernel_function,
       body += LoadLocal(scopes_->this_variable);
       body += LoadLocal(
           LookupVariable(kernel_function->positional_parameters()[0]));
-      body += StoreInstanceField(LinkedHashMap::data_offset());
+      body += StoreInstanceField(TokenPosition::kNoSource,
+                                 LinkedHashMap::data_offset());
       body += NullConstant();
       break;
     case MethodRecognizer::kLinkedHashMap_getHashMask:
@@ -3389,7 +3407,8 @@ Fragment FlowGraphBuilder::NativeFunctionBody(FunctionNode* kernel_function,
       body += LoadLocal(scopes_->this_variable);
       body += LoadLocal(
           LookupVariable(kernel_function->positional_parameters()[0]));
-      body += StoreInstanceField(LinkedHashMap::hash_mask_offset(),
+      body += StoreInstanceField(TokenPosition::kNoSource,
+                                 LinkedHashMap::hash_mask_offset(),
                                  kNoStoreBarrier);
       body += NullConstant();
       break;
@@ -3402,7 +3421,8 @@ Fragment FlowGraphBuilder::NativeFunctionBody(FunctionNode* kernel_function,
       body += LoadLocal(scopes_->this_variable);
       body += LoadLocal(
           LookupVariable(kernel_function->positional_parameters()[0]));
-      body += StoreInstanceField(LinkedHashMap::used_data_offset(),
+      body += StoreInstanceField(TokenPosition::kNoSource,
+                                 LinkedHashMap::used_data_offset(),
                                  kNoStoreBarrier);
       body += NullConstant();
       break;
@@ -3415,7 +3435,8 @@ Fragment FlowGraphBuilder::NativeFunctionBody(FunctionNode* kernel_function,
       body += LoadLocal(scopes_->this_variable);
       body += LoadLocal(
           LookupVariable(kernel_function->positional_parameters()[0]));
-      body += StoreInstanceField(LinkedHashMap::deleted_keys_offset(),
+      body += StoreInstanceField(TokenPosition::kNoSource,
+                                 LinkedHashMap::deleted_keys_offset(),
                                  kNoStoreBarrier);
       body += NullConstant();
       break;
@@ -3456,7 +3477,7 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFieldAccessor(
       body += StoreInstanceFieldGuarded(field, false);
     } else {
       body += LoadLocal(setter_value);
-      body += StoreStaticField(field);
+      body += StoreStaticField(TokenPosition::kNoSource, field);
     }
     body += NullConstant();
   } else if (is_method) {
@@ -3525,17 +3546,20 @@ Fragment FlowGraphBuilder::BuildImplicitClosureCreation(
   // Store the function and the context in the closure.
   fragment += LoadLocal(closure);
   fragment += Constant(target);
-  fragment += StoreInstanceField(Closure::function_offset());
+  fragment +=
+      StoreInstanceField(TokenPosition::kNoSource, Closure::function_offset());
 
   fragment += LoadLocal(closure);
   fragment += LoadLocal(context);
-  fragment += StoreInstanceField(Closure::context_offset());
+  fragment +=
+      StoreInstanceField(TokenPosition::kNoSource, Closure::context_offset());
 
   // The context is on top of the operand stack.  Store `this`.  The context
   // doesn't need a parent pointer because it doesn't close over anything
   // else.
   fragment += LoadLocal(scopes_->this_variable);
-  fragment += StoreInstanceField(Context::variable_offset(0));
+  fragment +=
+      StoreInstanceField(TokenPosition::kNoSource, Context::variable_offset(0));
 
   return fragment;
 }
@@ -4484,6 +4508,9 @@ void FlowGraphBuilder::VisitVariableGet(VariableGet* node) {
 void FlowGraphBuilder::VisitVariableSet(VariableSet* node) {
   Fragment instructions = TranslateExpression(node->expression());
   instructions += CheckVariableTypeInCheckedMode(node->variable());
+  if (H.ShouldAddDebugInstruction(stack_, node->position())) {

[Kevin Millikin (Google), 2017/02/08 15:37:52]

It's not usual that we build the graph by prepending a fragment to an existing
one.  It's unusual enough that I would do it immediately when it becomes
possible.  Call DebugStepCheck() after TranslateExpression, not in between
CheckVariableTypeInCheckedMode and StoreLocal, which seems kind of random.

[jensj, 2017/02/13 14:04:17]

You're right. I think it was somewhat of a 'bad merge'.

+    instructions = H.GetDebugInstruction(node->position()) + instructions;
+  }
   instructions +=
       StoreLocal(node->position(), LookupVariable(node->variable()));
   fragment_ = instructions;
@@ -4537,9 +4564,12 @@ void FlowGraphBuilder::VisitStaticSet(StaticSet* node) {
     Fragment instructions = TranslateExpression(node->expression());
     instructions += CheckAssignableInCheckedMode(
         dst_type, dart::String::ZoneHandle(Z, field.name()));
+    if (H.ShouldAddDebugInstruction(stack_, node->position())) {
+      instructions = H.GetDebugInstruction(node->position()) + instructions;

[Kevin Millikin (Google), 2017/02/08 15:37:53]

Call DebugStepCheck() after TranslateExpression, not here.

[jensj, 2017/02/13 14:04:17]

Done.

+    }
     LocalVariable* variable = MakeTemporary();
     instructions += LoadLocal(variable);
-    fragment_ = instructions + StoreStaticField(field);
+    fragment_ = instructions + StoreStaticField(node->position(), field);
   } else {
     ASSERT(target->IsProcedure());
 
@@ -4965,8 +4995,8 @@ void FlowGraphBuilder::VisitAsExpression(AsExpression* node) {
     instructions += PushArgument();  // Type.
 
     instructions += InstanceCall(
-        TokenPosition::kNoSource,
-        dart::Library::PrivateCoreLibName(Symbols::_as()), Token::kAS, 3);
+        node->position(), dart::Library::PrivateCoreLibName(Symbols::_as()),
+        Token::kAS, 3);
   }
 
   fragment_ = instructions;
@@ -5190,6 +5220,9 @@ void FlowGraphBuilder::VisitThrow(Throw* node) {
   Fragment instructions;
 
   instructions += TranslateExpression(node->expression());
+  if (H.ShouldAddDebugInstruction(stack_, node->position())) {
+    instructions = H.GetDebugInstruction(node->position()) + instructions;
+  }
   instructions += PushArgument();
   instructions += ThrowException(node->position());
   ASSERT(instructions.is_closed());
@@ -5268,13 +5301,17 @@ void FlowGraphBuilder::VisitReturnStatement(ReturnStatement* node) {
   if (instructions.is_open()) {
     if (inside_try_finally) {
       ASSERT(scopes_->finally_return_variable != NULL);
-      instructions += StoreLocal(TokenPosition::kNoSource,
-                                 scopes_->finally_return_variable);
+      const Function& function = parsed_function_->function();
+      if (H.ShouldAddDebugInstruction(function, node->position())) {
+        instructions += H.GetDebugInstruction(node->position());
+      }
+      instructions +=
+          StoreLocal(node->position(), scopes_->finally_return_variable);
       instructions += Drop();
       instructions += TranslateFinallyFinalizers(NULL, -1);
       if (instructions.is_open()) {
         instructions += LoadLocal(scopes_->finally_return_variable);
-        instructions += Return(node->position());
+        instructions += Return(TokenPosition::kNoSource);
       }
     } else {
       instructions += Return(node->position());
@@ -5311,16 +5348,27 @@ void FlowGraphBuilder::VisitVariableDeclaration(VariableDeclaration* node) {
       instructions += CheckVariableTypeInCheckedMode(node);
     }
   }
-  instructions += StoreLocal(variable->token_pos(), variable);
+  // In parser.cc's Parser::ParseVariableDeclaration the position is actually

[Kevin Millikin (Google), 2017/02/08 15:37:52]

Don't refer to the (other) producer(s) of this value, it's not important and it
will be an obsolete comment if (when?) parser.cc is deleted.

Instead, please refer to the consumer of the value, which is important.  Just
say that the debugger expects the position of the equals if there is an
initializer or the identifier if not.

[jensj, 2017/02/13 14:04:17]

Done.

+  // the position of the equal sign, or in the case of no initialization the
+  // identifier token position. Taking the max of position (the identifier
+  // position) and equals_position (the position of equals, or noSource)
+  // gives the same result.
+  TokenPosition debug_position =
+      Utils::Maximum(node->position(), node->equals_position());
+  if (H.ShouldAddDebugInstruction(stack_, debug_position)) {
+    instructions = H.GetDebugInstruction(debug_position) + instructions;
+  }
+  instructions += StoreLocal(node->position(), variable);
   instructions += Drop();
   fragment_ = instructions;
 }
 
 
 void FlowGraphBuilder::VisitFunctionDeclaration(FunctionDeclaration* node) {
-  Fragment instructions = TranslateFunctionNode(node->function(), node);
+  Fragment instructions = H.GetDebugInstruction(node->position());
+  instructions += TranslateFunctionNode(node->function(), node);
   instructions +=
-      StoreLocal(TokenPosition::kNoSource, LookupVariable(node->variable()));
+      StoreLocal(node->position(), LookupVariable(node->variable()));
   instructions += Drop();
   fragment_ = instructions;
 }
@@ -5559,6 +5607,10 @@ void FlowGraphBuilder::VisitBreakStatement(BreakStatement* node) {
   instructions +=
       TranslateFinallyFinalizers(outer_finally, target_context_depth);
   if (instructions.is_open()) {
+    if (H.ShouldAddDebugInstruction(parsed_function_->function(),
+                                    node->position())) {
+      instructions += H.GetDebugInstruction(node->position());
+    }
     instructions += Goto(destination);
   }
   fragment_ = instructions;
@@ -6164,11 +6216,13 @@ Fragment FlowGraphBuilder::TranslateFunctionNode(FunctionNode* node,
   // Store the function and the context in the closure.
   instructions += LoadLocal(closure);
   instructions += Constant(function);
-  instructions += StoreInstanceField(Closure::function_offset());
+  instructions +=
+      StoreInstanceField(TokenPosition::kNoSource, Closure::function_offset());
 
   instructions += LoadLocal(closure);
   instructions += LoadLocal(parsed_function_->current_context_var());
-  instructions += StoreInstanceField(Closure::context_offset());
+  instructions +=
+      StoreInstanceField(TokenPosition::kNoSource, Closure::context_offset());
 
   return instructions;
 }