Add support for converted closures with explicit contexts to VM

runtime/vm/kernel_binary_flowgraph.cc

Index: runtime/vm/kernel_binary_flowgraph.cc
diff --git a/runtime/vm/kernel_binary_flowgraph.cc b/runtime/vm/kernel_binary_flowgraph.cc
index 37f596c4fa2535ec5c15ee3e6198bac1a1ef6388..5db62d49d71b54397e2b5c0955947daec5fcd9ac 100644
--- a/runtime/vm/kernel_binary_flowgraph.cc
+++ b/runtime/vm/kernel_binary_flowgraph.cc
@@ -121,6 +121,7 @@ ScopeBuildingResult* StreamingScopeBuilder::BuildScopes() {
 
   switch (function.kind()) {
     case RawFunction::kClosureFunction:
+    case RawFunction::kConvertedClosureFunction:
     case RawFunction::kRegularFunction:
     case RawFunction::kGetterFunction:
     case RawFunction::kSetterFunction:
@@ -359,10 +360,10 @@ void StreamingScopeBuilder::VisitProcedure() {
 void StreamingScopeBuilder::VisitField() {
   builder_->ReadFieldUntilAnnotation(
       &unused_nameindex, &unused_tokenposition, &unused_tokenposition,
-      &unused_intptr, &unused_word);       // read first part of field.
-  builder_->SkipListOfExpressions();       // read annotations.
-  VisitDartType();                         // read type.
-  Tag tag = builder_->ReadTag();           // read initializer (part 1).
+      &unused_intptr, &unused_word);  // read first part of field.
+  builder_->SkipListOfExpressions();  // read annotations.
+  VisitDartType();                    // read type.
+  Tag tag = builder_->ReadTag();      // read initializer (part 1).
   if (tag == kSomething) {
     VisitExpression();  // read initializer (part 2).
   }
@@ -693,6 +694,26 @@ void StreamingScopeBuilder::VisitExpression() {
       return;
     case kNullLiteral:
       return;
+    case kVectorCreation:
+      builder_->ReadUInt();  // read size.
+      return;
+    case kVectorGet:
+      VisitExpression();     // read expression.
+      builder_->ReadUInt();  // read index.
+      return;
+    case kVectorSet:
+      VisitExpression();     // read vector expression.
+      builder_->ReadUInt();  // read index.
+      VisitExpression();     // read value.
+      return;
+    case kVectorCopy:
+      VisitExpression();  // read vector expression.
+      return;
+    case kClosureCreation:
+      builder_->SkipCanonicalNameReference();  // read function reference.
+      VisitExpression();                       // read context vector.
+      VisitDartType();  // read function type of the closure.
+      return;
     default:
       UNREACHABLE();
   }
@@ -1032,6 +1053,7 @@ void StreamingScopeBuilder::VisitDartType() {
     case kDynamicType:
     case kVoidType:
     case kBottomType:
+    case kVectorType:
       // those contain nothing.
       return;
     case kInterfaceType:
@@ -1443,6 +1465,9 @@ void StreamingDartTypeTranslator::BuildTypeInternal() {
     case kVoidType:
       result_ = Object::void_type().raw();
       break;
+    case kVectorType:
+      result_ = Object::vector_type().raw();
+      break;
     case kBottomType:
       result_ = dart::Class::Handle(Z, I->object_store()->null_class())
                     .CanonicalType();
@@ -3141,6 +3166,83 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfImplicitClosureFunction(
                 flow_graph_builder_->next_block_id_ - 1);
 }
 
+
+// This method follows the logic similar to that of

[kustermann, 2017/06/20 12:13:14]

s/follows the logic .... of/follows the logic of/

[Dmitry Stefantsov, 2017/06/22 14:12:52]

Agreed.  It reads better.  Thanks!

+// StreamingFlowGraphBuilder::BuildGraphOfImplicitClosureFunction.  For
+// additional details on converted closure functions, please, see the comment on
+// the method Function::ConvertedClosureFunction.
+FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfConvertedClosureFunction(
+    const Function& function) {

[kustermann, 2017/06/20 12:13:13]

Given the significant duplication of code, maybe consider just parametrizing the
old code?

[Dmitry Stefantsov, 2017/06/22 14:12:52]

I agree, we may benefit from the parametrization.  But I would rather do it in a
separate CL.  What are your thoughts, Martin?

[kustermann, 2017/06/26 10:32:36]

Just keep it then.

+  const Function& target = Function::ZoneHandle(Z, function.parent_function());
+
+  TargetEntryInstr* normal_entry = flow_graph_builder_->BuildTargetEntry();
+  flow_graph_builder_->graph_entry_ = new (Z) GraphEntryInstr(
+      *parsed_function(), normal_entry, Compiler::kNoOSRDeoptId);
+  SetupDefaultParameterValues();
+
+  Fragment body(normal_entry);
+  body += flow_graph_builder_->CheckStackOverflowInPrologue();
+
+  // Load all the arguments.
+  ASSERT(target.is_static());
+
+  TokenPosition end_position;
+  ReadFunctionNodeUntilTypeParameters(
+      &unused_tokenposition, &end_position, &unused_word,
+      &unused_word);         // read first part of function node.
+  SkipTypeParametersList();  // read type parameter list.
+  ReadUInt();                // read total parameter count.
+  ReadUInt();                // read required_parameter_count.
+
+  // Positional.
+  intptr_t positional_argument_count = ReadListLength();

[kustermann, 2017/06/20 12:13:14]

Usually we try to use "const" where possible. (also in other places in the CL)

[Dmitry Stefantsov, 2017/06/22 14:12:52]

Do you mean something like `const intptr_t positional_argument_count ...`?  I
can make this change, but this variable declaration is the same like the similar
one from `BuildGraphOfImplicitClosureConversion` that I took as the basis. 
Should I then also make the changes there?

[kustermann, 2017/06/26 10:32:36]

It's fine to just change your code. The benefit of const (or 'final' in dart) is
that one knows there's no other assignment to the variable.

+
+  // The first argument is the instance of the closure class.  For converted
+  // closures its context field contains the context vector that is used by the
+  // converted top-level function (target) explicitly and that should be passed
+  // to that function as the first parameter.
+  body += LoadLocal(LookupVariable(ReaderOffset()));  // 0th variable offset.
+  body += flow_graph_builder_->LoadField(Closure::context_offset());
+  body += PushArgument();
+  SkipVariableDeclaration();  // read 0th variable.
+
+  // The rest of the parameters are the same for the method of the Closure class
+  // being invoked and the top-level function (target).
+  for (intptr_t i = 1; i < positional_argument_count; i++) {
+    body += LoadLocal(LookupVariable(ReaderOffset()));  // ith variable offset.
+    body += PushArgument();
+    SkipVariableDeclaration();  // read ith variable.
+  }
+
+  // Named.
+  intptr_t named_argument_count = ReadListLength();
+  Array& argument_names = Array::ZoneHandle(Z);
+  if (named_argument_count > 0) {
+    argument_names = Array::New(named_argument_count);
+    for (intptr_t i = 0; i < named_argument_count; i++) {
+      body +=
+          LoadLocal(LookupVariable(ReaderOffset()));  // ith variable offset.
+      body += PushArgument();
+      argument_names.SetAt(
+          i, H.DartSymbol(GetNameFromVariableDeclaration(ReaderOffset())));
+      SkipVariableDeclaration();  // read ith variable.
+    }
+  }
+
+  // Forward them to the target.
+  intptr_t argument_count = positional_argument_count + named_argument_count;
+  body += StaticCall(TokenPosition::kNoSource, target, argument_count,
+                     argument_names);
+
+  // Return the result.
+  body += Return(end_position);
+
+  return new (Z)
+      FlowGraph(*parsed_function(), flow_graph_builder_->graph_entry_,
+                flow_graph_builder_->next_block_id_ - 1);
+}
+
+
 static bool IsGetMainClosure(const String& name) {
   if (name.Length() < 16) return false;
   const char* cstr = "_getMainClosure@";
@@ -3564,13 +3666,17 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraph(intptr_t kernel_offset) {
 
   switch (function.kind()) {
     case RawFunction::kClosureFunction:
+    case RawFunction::kConvertedClosureFunction:
     case RawFunction::kRegularFunction:
     case RawFunction::kGetterFunction:
     case RawFunction::kSetterFunction: {
       ReadUntilFunctionNode();  // read until function node.
-      return function.IsImplicitClosureFunction()
-                 ? BuildGraphOfImplicitClosureFunction(function)
-                 : BuildGraphOfFunction(is_in_builtin_library_toplevel);
+      if (function.IsImplicitClosureFunction()) {
+        return BuildGraphOfImplicitClosureFunction(function);
+      } else if (function.IsConvertedClosureFunction()) {
+        return BuildGraphOfConvertedClosureFunction(function);
+      }
+      return BuildGraphOfFunction(is_in_builtin_library_toplevel);
     }
     case RawFunction::kConstructor: {
       bool is_factory = function.IsFactory();
@@ -3701,6 +3807,16 @@ Fragment StreamingFlowGraphBuilder::BuildExpression(TokenPosition* position) {
       return BuildBoolLiteral(false, position);
     case kNullLiteral:
       return BuildNullLiteral(position);
+    case kVectorCreation:
+      return BuildVectorCreation(position);
+    case kVectorGet:
+      return BuildVectorGet(position);
+    case kVectorSet:
+      return BuildVectorSet(position);
+    case kVectorCopy:
+      return BuildVectorCopy(position);
+    case kClosureCreation:
+      return BuildClosureCreation(position);
     default:
       UNREACHABLE();
   }
@@ -3856,6 +3972,7 @@ void StreamingFlowGraphBuilder::SkipDartType() {
     case kDynamicType:
     case kVoidType:
     case kBottomType:
+    case kVectorType:
       // those contain nothing.
       return;
     case kInterfaceType:
@@ -3900,8 +4017,8 @@ void StreamingFlowGraphBuilder::SkipInterfaceType(bool simple) {
 void StreamingFlowGraphBuilder::SkipFunctionType(bool simple) {
   if (!simple) {
     SkipTypeParametersList();  // read type_parameters.
-    ReadUInt();  // read required parameter count.
-    ReadUInt();  // read total parameter count.
+    ReadUInt();                // read required parameter count.
+    ReadUInt();                // read total parameter count.
   }
 
   SkipListOfDartTypes();  // read positional_parameters types.
@@ -4050,8 +4167,8 @@ void StreamingFlowGraphBuilder::SkipExpression() {
       SkipOptionalDartType();  // read unused static type.
       return;
     case kStringConcatenation:
-      ReadPosition();                           // read position.
-      SkipListOfExpressions();                  // read list of expressions.
+      ReadPosition();           // read position.
+      SkipListOfExpressions();  // read list of expressions.
       return;
     case kIsExpression:
       ReadPosition();    // read position.
@@ -4080,9 +4197,9 @@ void StreamingFlowGraphBuilder::SkipExpression() {
       return;
     case kListLiteral:
     case kConstListLiteral:
-      ReadPosition();                           // read position.
-      SkipDartType();                           // read type.
-      SkipListOfExpressions();                  // read list of expressions.
+      ReadPosition();           // read position.
+      SkipDartType();           // read type.
+      SkipListOfExpressions();  // read list of expressions.
       return;
     case kMapLiteral:
     case kConstMapLiteral: {
@@ -4103,6 +4220,26 @@ void StreamingFlowGraphBuilder::SkipExpression() {
       SkipVariableDeclaration();  // read variable declaration.
       SkipExpression();           // read expression.
       return;
+    case kVectorCreation:
+      ReadUInt();  // read value.
+      return;
+    case kVectorGet:
+      SkipExpression();  // read vector expression.
+      ReadUInt();        // read index.
+      return;
+    case kVectorSet:
+      SkipExpression();  // read vector expression.
+      ReadUInt();        // read index.
+      SkipExpression();  // read value.
+      return;
+    case kVectorCopy:
+      SkipExpression();  // read vector expression.
+      return;
+    case kClosureCreation:
+      SkipCanonicalNameReference();  // read top-level function reference.
+      SkipExpression();              // read context vector.
+      SkipDartType();                // read function type.
+      return;
     case kBigIntLiteral:
       SkipStringReference();  // read string reference.
       return;
@@ -4175,12 +4312,12 @@ void StreamingFlowGraphBuilder::SkipStatement() {
       return;
     case kForStatement: {
       SkipListOfVariableDeclarations();  // read variables.
-      Tag tag = ReadTag();  // Read first part of condition.
+      Tag tag = ReadTag();               // Read first part of condition.
       if (tag == kSomething) {
         SkipExpression();  // read rest of condition.
       }
       SkipListOfExpressions();  // read updates.
-      SkipStatement();  // read body.
+      SkipStatement();          // read body.
       return;
     }
     case kForInStatement:
@@ -4423,6 +4560,10 @@ Value* StreamingFlowGraphBuilder::stack() {
   return flow_graph_builder_->stack_;
 }
 
+void StreamingFlowGraphBuilder::Push(Definition* definition) {
+  flow_graph_builder_->Push(definition);
+}
+
 Value* StreamingFlowGraphBuilder::Pop() {
   return flow_graph_builder_->Pop();
 }
@@ -4459,12 +4600,12 @@ intptr_t StreamingFlowGraphBuilder::PeekArgumentsCount() {
 
 intptr_t StreamingFlowGraphBuilder::PeekArgumentsTypeCount() {
   AlternativeReadingScope alt(reader_);
-  ReadUInt();                               // read arguments count.
-  return ReadListLength();                  // read length of types list.
+  ReadUInt();               // read arguments count.
+  return ReadListLength();  // read length of types list.
 }
 
 void StreamingFlowGraphBuilder::SkipArgumentsBeforeActualArguments() {
-  ReadUInt();  // read arguments count.
+  ReadUInt();             // read arguments count.
   SkipListOfDartTypes();  // read list of types.
 }
 
@@ -4589,6 +4730,20 @@ Fragment StreamingFlowGraphBuilder::AllocateObject(const dart::Class& klass,
   return flow_graph_builder_->AllocateObject(klass, argument_count);
 }
 
+Fragment StreamingFlowGraphBuilder::AllocateObject(
+    const dart::Class& klass,
+    const Function& closure_function) {
+  return flow_graph_builder_->AllocateObject(klass, closure_function);
+}
+
+Fragment StreamingFlowGraphBuilder::AllocateContext(int size) {
+  return flow_graph_builder_->AllocateContext(size);
+}
+
+Fragment StreamingFlowGraphBuilder::LoadField(intptr_t offset) {
+  return flow_graph_builder_->LoadField(offset);
+}
+
 Fragment StreamingFlowGraphBuilder::InstanceCall(TokenPosition position,
                                                  const dart::String& name,
                                                  Token::Kind kind,
@@ -4609,6 +4764,11 @@ Fragment StreamingFlowGraphBuilder::StoreStaticField(TokenPosition position,
   return flow_graph_builder_->StoreStaticField(position, field);
 }
 
+Fragment StreamingFlowGraphBuilder::StoreInstanceField(TokenPosition position,
+                                                       intptr_t offset) {
+  return flow_graph_builder_->StoreInstanceField(position, offset);
+}
+
 Fragment StreamingFlowGraphBuilder::StringInterpolate(TokenPosition position) {
   return flow_graph_builder_->StringInterpolate(position);
 }
@@ -5519,9 +5679,10 @@ Fragment StreamingFlowGraphBuilder::BuildIsExpression(TokenPosition* p) {
     if (dart::FlowGraphBuilder::SimpleInstanceOfType(type)) {
       instructions += Constant(type);
       instructions += PushArgument();  // Type.
-      instructions += InstanceCall(position, dart::Library::PrivateCoreLibName(
-                                                 Symbols::_simpleInstanceOf()),
-                                   Token::kIS, 2, 2);  // 2 checked arguments.
+      instructions += InstanceCall(
+          position,
+          dart::Library::PrivateCoreLibName(Symbols::_simpleInstanceOf()),
+          Token::kIS, 2, 2);  // 2 checked arguments.
       return instructions;
     }
 
@@ -5789,8 +5950,8 @@ Fragment StreamingFlowGraphBuilder::BuildFunctionExpression() {
 Fragment StreamingFlowGraphBuilder::BuildLet(TokenPosition* position) {
   if (position != NULL) *position = TokenPosition::kNoSource;
 
-  Fragment instructions = BuildVariableDeclaration();       // read variable.
-  instructions += BuildExpression();                        // read body.
+  Fragment instructions = BuildVariableDeclaration();  // read variable.
+  instructions += BuildExpression();                   // read body.
   return instructions;
 }
 
@@ -5851,6 +6012,93 @@ Fragment StreamingFlowGraphBuilder::BuildNullLiteral(TokenPosition* position) {
   return Constant(Instance::ZoneHandle(Z, Instance::null()));
 }
 
+Fragment StreamingFlowGraphBuilder::BuildVectorCreation(
+    TokenPosition* position) {
+  if (position != NULL) *position = TokenPosition::kNoSource;
+
+  intptr_t size = ReadUInt();  // read size.
+  return AllocateContext(size);
+}
+
+Fragment StreamingFlowGraphBuilder::BuildVectorGet(TokenPosition* position) {
+  if (position != NULL) *position = TokenPosition::kNoSource;
+
+  Fragment instructions = BuildExpression();  // read expression.
+  intptr_t index = ReadUInt();                // read index.
+  instructions += LoadField(Context::variable_offset(index));
+  return instructions;
+}
+
+Fragment StreamingFlowGraphBuilder::BuildVectorSet(TokenPosition* position) {
+  if (position != NULL) *position = TokenPosition::kNoSource;
+
+  Fragment instructions = BuildExpression();  // read vector expression.
+  intptr_t index = ReadUInt();                // read index.
+  instructions += BuildExpression();          // read value expression.
+
+  // The assigned value is the result of the expression.
+  LocalVariable* result = MakeTemporary();

[kustermann, 2017/06/20 12:13:14]

I suspect this is unsafe:

The MakeTemporary() makes the top-of-stack value addressable relative to the
frame pointer. You can use [result] (e.g. via LoadLocal(result)) as long as it's
on the stack.

But the StoreInstanceFieldInstr below will consume 2 elements from the stack
(value/context: both are Pop()ed) and will push the result. So the [result] got
Pop()ed from the stack and you can't use it anymore.

This means that [result] will point to one item past top-of-stack.

Take a look at kernel_to_il.cc:StoreLocal() to see how you can fix it.

[Dmitry Stefantsov, 2017/06/22 14:12:52]

Thank you so much for pointing me to that!  Indeed this code should be
rewritten.  I'll upload a Patch Set in this CL fixing this.

+
+  Value* value = Pop();
+  StoreInstanceFieldInstr* store = new (Z)
+      StoreInstanceFieldInstr(Context::variable_offset(index), Pop(), value,
+                              kNoStoreBarrier, TokenPosition::kNoSource);
+  instructions <<= store;
+
+  // Load the result that is stored in the temporary variable.
+  instructions += LoadLocal(result);
+
+  return instructions;
+}
+
+Fragment StreamingFlowGraphBuilder::BuildVectorCopy(TokenPosition* position) {
+  if (position != NULL) *position = TokenPosition::kNoSource;
+
+  Fragment instructions = BuildExpression();  // read vector expression.
+  CloneContextInstr* clone_instruction = new (Z) CloneContextInstr(
+      TokenPosition::kNoSource, Pop(), Thread::Current()->GetNextDeoptId());

[kustermann, 2017/06/20 12:13:13]

Maybe introduce local variables, so one knows what gets Pop()ed.

Value* context_to_copy = Pop();

[Dmitry Stefantsov, 2017/06/22 14:12:52]

Thanks!  That would read better indeed.  Done.

+  instructions <<= clone_instruction;
+  Push(clone_instruction);
+
+  return instructions;
+}
+
+Fragment StreamingFlowGraphBuilder::BuildClosureCreation(
+    TokenPosition* position) {
+  if (position != NULL) *position = TokenPosition::kNoSource;
+
+  NameIndex function_reference =
+      ReadCanonicalNameReference();  // read function reference.
+  Function& function = Function::ZoneHandle(
+      Z, H.LookupStaticMethodByKernelProcedure(function_reference));
+  function = function.ConvertedClosureFunction();
+  ASSERT(!function.IsNull());
+
+  const dart::Class& closure_class =
+      dart::Class::ZoneHandle(Z, I->object_store()->closure_class());
+  Fragment instructions = AllocateObject(closure_class, function);
+  LocalVariable* closure = MakeTemporary();

[kustermann, 2017/06/20 12:13:14]

Here for example (as opposed to above), the [closure] will stay on the stack
until the very end, so it's safe to make it addressable via [MakeTemporary] and
do several LoadLocal(closure)s.

[Dmitry Stefantsov, 2017/06/22 14:12:52]

Yep.  Now I see it.  Thanks!

+
+  instructions += BuildExpression();  // read context vector.
+  LocalVariable* context = MakeTemporary();
+
+  instructions += LoadLocal(closure);
+  instructions += Constant(function);
+  instructions +=
+      StoreInstanceField(TokenPosition::kNoSource, Closure::function_offset());
+
+  instructions += LoadLocal(closure);
+  instructions += LoadLocal(context);
+  instructions +=
+      StoreInstanceField(TokenPosition::kNoSource, Closure::context_offset());
+
+  instructions += Drop();
+
+  SkipDartType();  // skip function type of the closure.
+
+  return instructions;
+}
+
 Fragment StreamingFlowGraphBuilder::BuildInvalidStatement() {
   H.ReportError("Invalid statements not implemented yet!");
   return Fragment();