Kernel debugging; service tests

runtime/vm/kernel_to_il.cc

 // Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include <map>
 #include <set>
 #include <string>
 
 #include "vm/kernel_to_il.h"
 
@@ skipping 1310 matching lines @@
   if (named->length() == 0) return Array::ZoneHandle(Z);
 
   const Array& names =
       Array::ZoneHandle(Z, Array::New(named->length(), Heap::kOld));
   for (intptr_t i = 0; i < named->length(); ++i) {
     names.SetAt(i, DartSymbol((*named)[i]->name()));
   }
   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,
                                      TranslationHelper* h,
                                      DartTypeTranslator* type_translator)
     : builder_(builder),
       isolate_(Isolate::Current()),
       zone_(zone),
       translation_helper_(*h),
       type_translator_(*type_translator),
@@ skipping 719 matching lines @@
   return instructions;
 }
 
 
 Fragment FlowGraphBuilder::PushContext(int size) {
   ASSERT(size > 0);
   Fragment instructions = AllocateContext(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_;
   return instructions;
 }
 
 
 Fragment FlowGraphBuilder::PopContext() {
   return AdjustContextTo(context_depth_ - 1);
 }
@@ skipping 460 matching lines @@
 
 
 Fragment FlowGraphBuilder::Return(TokenPosition position) {
   Fragment instructions;
 
   instructions += CheckReturnTypeInCheckedMode();
 
   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);
   if (exit_collector_ != NULL) exit_collector_->AddExit(return_instr);
 
   instructions <<= return_instr;
 
   return instructions.closed();
 }
 
@@ skipping 97 matching lines @@
     instructions += GuardFieldClass(field_clone, H.thread()->GetNextDeoptId());
     instructions += LoadLocal(store_expression);
     instructions += GuardFieldLength(field_clone, H.thread()->GetNextDeoptId());
   }
   instructions += StoreInstanceField(field_clone, is_initialization_store);
   return instructions;
 }
 
 
 Fragment FlowGraphBuilder::StoreInstanceField(
+    TokenPosition position,
     intptr_t offset,
     StoreBarrierType emit_store_barrier) {
   Value* value = Pop();
   if (value->BindsToConstant()) {
     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);
 }
 
 
 Fragment FlowGraphBuilder::StoreLocal(TokenPosition position,
                                       LocalVariable* variable) {
   Fragment instructions;
   if (variable->is_captured()) {
     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;
     Push(store);
   }
   return instructions;
 }
 
 
-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));
 }
 
 
 Fragment FlowGraphBuilder::StringInterpolate(TokenPosition position) {
   Value* array = Pop();
   StringInterpolateInstr* interpolate =
       new (Z) StringInterpolateInstr(array, position);
   Push(interpolate);
   return Fragment(interpolate);
 }
@@ skipping 350 matching lines @@
         parameter->set_index(parameter_index);
         // Mark the stack variable so it will be ignored by the code for
         // try/catch.
         parameter->set_is_captured_parameter(true);
 
         // Copy the parameter from the stack to the context.  Overwrite it
         // with a null constant on the stack so the original value is
         // 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();
       }
     }
     body += Drop();  // The context.
   }
   if (constructor != NULL) {
     // TODO(27590): Currently the [VariableDeclaration]s from the
     // initializers will be visible inside the entire body of the constructor.
@@ skipping 120 matching lines @@
       TargetEntryInstr* otherwise;
       dispatch += LoadLocal(scopes_->switch_variable);
       dispatch += IntConstant(i);
       dispatch += BranchIfStrictEqual(&then, &otherwise);
 
       // True branch is linked to appropriate continuation point.
       // Note: continuations start with nop DropTemps instruction
       // 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;
     }
     body = dispatch;
 
     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
     // 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.
     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());
     }
-    Fragment check(
-        new (Z) DebugStepCheckInstr(check_pos, RawPcDescriptors::kRuntimeCall));
-    body = check + body;
+    body = H.GetDebugInstruction(check_pos) + body;
   }
 
   normal_entry->LinkTo(body.entry);
 
   // When compiling for OSR, use a depth first search to prune instructions
   // unreachable from the OSR entry. Catch entries are always considered
   // reachable, even if they become unreachable after OSR.
   if (osr_id_ != Compiler::kNoOSRDeoptId) {
     BitVector* block_marks = new (Z) BitVector(Z, next_block_id_);
     bool found = graph_entry_->PruneUnreachable(graph_entry_, NULL, osr_id_,
@@ skipping 84 matching lines @@
       break;
     case MethodRecognizer::kLinkedHashMap_getIndex:
       body += LoadLocal(scopes_->this_variable);
       body += LoadNativeField(kind, LinkedHashMap::index_offset(),
                               Object::dynamic_type(), kDynamicCid);
       break;
     case MethodRecognizer::kLinkedHashMap_setIndex:
       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:
       body += LoadLocal(scopes_->this_variable);
       body += LoadNativeField(kind, LinkedHashMap::data_offset(),
                               Object::dynamic_type(), kArrayCid);
       break;
     case MethodRecognizer::kLinkedHashMap_setData:
       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:
       body += LoadLocal(scopes_->this_variable);
       body += LoadNativeField(kind, LinkedHashMap::hash_mask_offset(),
                               Type::ZoneHandle(Z, Type::SmiType()), kSmiCid);
       break;
     case MethodRecognizer::kLinkedHashMap_setHashMask:
       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;
     case MethodRecognizer::kLinkedHashMap_getUsedData:
       body += LoadLocal(scopes_->this_variable);
       body += LoadNativeField(kind, LinkedHashMap::used_data_offset(),
                               Type::ZoneHandle(Z, Type::SmiType()), kSmiCid);
       break;
     case MethodRecognizer::kLinkedHashMap_setUsedData:
       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;
     case MethodRecognizer::kLinkedHashMap_getDeletedKeys:
       body += LoadLocal(scopes_->this_variable);
       body += LoadNativeField(kind, LinkedHashMap::deleted_keys_offset(),
                               Type::ZoneHandle(Z, Type::SmiType()), kSmiCid);
       break;
     case MethodRecognizer::kLinkedHashMap_setDeletedKeys:
       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;
     case MethodRecognizer::kBigint_getNeg:
       body += LoadLocal(scopes_->this_variable);
       body += LoadNativeField(kind, Bigint::neg_offset(),
                               Type::ZoneHandle(Z, Type::BoolType()), kBoolCid);
       break;
     default: {
       dart::String& name = dart::String::ZoneHandle(Z, function.native_name());
@@ skipping 20 matching lines @@
       GraphEntryInstr(*parsed_function_, normal_entry, Compiler::kNoOSRDeoptId);
 
   Fragment body(normal_entry);
   if (is_setter) {
     if (is_method) {
       body += LoadLocal(scopes_->this_variable);
       body += LoadLocal(setter_value);
       body += StoreInstanceFieldGuarded(field, false);
     } else {
       body += LoadLocal(setter_value);
-      body += StoreStaticField(field);
+      body += StoreStaticField(TokenPosition::kNoSource, field);
     }
     body += NullConstant();
   } else if (is_method) {
     body += LoadLocal(scopes_->this_variable);
     body += LoadField(field);
   } else if (field.is_const()) {
     // If the parser needs to know the value of an uninitialized constant field
     // it will set the value to the transition sentinel (used to detect circular
     // initialization) and then call the implicit getter.  Thus, the getter
     // cannot contain the InitStaticField instruction that normal static getters
@@ skipping 48 matching lines @@
   fragment += AllocateObject(closure_class, target);
   LocalVariable* closure = MakeTemporary();
 
   // Allocate a context that closes over `this`.
   fragment += AllocateContext(1);
   LocalVariable* context = MakeTemporary();
 
   // 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;
 }
 
 
 Fragment FlowGraphBuilder::GuardFieldLength(const dart::Field& field,
                                             intptr_t deopt_id) {
   return Fragment(new (Z) GuardFieldLengthInstr(Pop(), field, deopt_id));
 }
 
@@ skipping 928 matching lines @@
 
 
 void FlowGraphBuilder::VisitVariableGet(VariableGet* node) {
   fragment_ = LoadLocal(LookupVariable(node->variable()));
 }
 
 
 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;
 }
 
 
 void FlowGraphBuilder::VisitStaticGet(StaticGet* node) {
   Member* target = node->target();
   if (target->IsField()) {
     Field* kernel_field = Field::Cast(target);
@@ skipping 33 matching lines @@
 void FlowGraphBuilder::VisitStaticSet(StaticSet* node) {
   Member* target = node->target();
   if (target->IsField()) {
     Field* kernel_field = Field::Cast(target);
     const dart::Field& field =
         dart::Field::ZoneHandle(Z, H.LookupFieldByKernelField(kernel_field));
     const AbstractType& dst_type = AbstractType::ZoneHandle(Z, field.type());
     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());
 
     // Evaluate the expression on the right hand side.
     Fragment instructions = TranslateExpression(node->expression());
     LocalVariable* variable = MakeTemporary();
 
     // Prepare argument.
     instructions += LoadLocal(variable);
     instructions += PushArgument();
@@ skipping 405 matching lines @@
       instructions += LoadInstantiatorTypeArguments();
     } else {
       instructions += NullConstant();
     }
     instructions += PushArgument();  // Type arguments.
 
     instructions += Constant(type);
     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;
 }
 
 
 void FlowGraphBuilder::VisitConditionalExpression(ConditionalExpression* node) {
   bool negate;
   Fragment instructions = TranslateCondition(node->condition(), &negate);
 
@@ skipping 203 matching lines @@
   Fragment instructions = TranslateStatement(node->variable());
   instructions += TranslateExpression(node->body());
   fragment_ = instructions;
 }
 
 
 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());
 
   fragment_ = instructions;
 }
 
 
 void FlowGraphBuilder::VisitRethrow(Rethrow* node) {
   Fragment instructions;
@@ skipping 58 matching lines @@
 
 void FlowGraphBuilder::VisitReturnStatement(ReturnStatement* node) {
   bool inside_try_finally = try_finally_block_ != NULL;
 
   Fragment instructions = node->expression() == NULL
                               ? NullConstant()
                               : TranslateExpression(node->expression());
   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());
     }
   } else {
     Pop();
   }
   fragment_ = instructions;
 }
 
@@ skipping 16 matching lines @@
     if (node->IsConst()) {
       const Instance& constant_value =
           constant_evaluator_.EvaluateExpression(initializer);
       variable->SetConstValue(constant_value);
       instructions += Constant(constant_value);
     } else {
       instructions += TranslateExpression(initializer);
       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;
 }
 
 
 void FlowGraphBuilder::VisitIfStatement(IfStatement* node) {
   bool negate;
   Fragment instructions = TranslateCondition(node->condition(), &negate);
   TargetEntryInstr* then_entry;
   TargetEntryInstr* otherwise_entry;
@@ skipping 218 matching lines @@
 void FlowGraphBuilder::VisitBreakStatement(BreakStatement* node) {
   TryFinallyBlock* outer_finally = NULL;
   intptr_t target_context_depth = -1;
   JoinEntryInstr* destination = breakable_block_->BreakDestination(
       node->target(), &outer_finally, &target_context_depth);
 
   Fragment instructions;
   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;
 }
 
 
 void FlowGraphBuilder::VisitSwitchStatement(SwitchStatement* node) {
   SwitchBlock block(this, node);
 
   // Instead of using a variable we should reuse the expression on the stack,
@@ skipping 585 matching lines @@
       dart::Class::ZoneHandle(Z, I->object_store()->closure_class());
   ASSERT(!closure_class.IsNull());
   Fragment instructions = AllocateObject(closure_class, function);
   LocalVariable* closure = MakeTemporary();
 
   // TODO(27590): Generic closures need type arguments.
 
   // 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;
 }
 
 
 RawObject* EvaluateMetadata(TreeNode* const kernel_node) {
   LongJumpScope jump;
   if (setjmp(*jump.Set()) == 0) {
     Thread* thread = Thread::Current();
     Zone* zone_ = thread->zone();
@@ skipping 98 matching lines @@
     thread->clear_sticky_error();
     return error.raw();
   }
 }
 
 
 }  // namespace kernel
 }  // namespace dart
 
 #endif  // !defined(DART_PRECOMPILED_RUNTIME)