Allocate instance closures similarly to regular closures, i.e. without a

runtime/vm/flow_graph_builder.cc

Index: runtime/vm/flow_graph_builder.cc
===================================================================
--- runtime/vm/flow_graph_builder.cc	(revision 32988)
+++ runtime/vm/flow_graph_builder.cc	(working copy)
@@ -2321,21 +2321,16 @@
       ReturnDefinition(ExitTempLocalScope(tmp_var));
     }
   } else {
+    // TODO(regis): Merge this branch in the above one.
     ASSERT(function.IsImplicitInstanceClosureFunction());
-    ValueGraphVisitor for_receiver(owner());
-    node->receiver()->Visit(&for_receiver);
-    Append(for_receiver);
-    Value* receiver = for_receiver.value();
 
-    PushArgumentInstr* push_receiver = PushArgument(receiver);
     ZoneGrowableArray<PushArgumentInstr*>* arguments =
-        new ZoneGrowableArray<PushArgumentInstr*>(2);
-    arguments->Add(push_receiver);
+        new ZoneGrowableArray<PushArgumentInstr*>(2);  // 1? here and above too?
     ASSERT(function.context_scope() != ContextScope::null());
 
     // The function type of a closure may have type arguments. In that case,
     // pass the type arguments of the instantiator. Otherwise, pass null object.
-    const Class& cls = Class::Handle(function.signature_class());
+    const Class& cls = Class::ZoneHandle(function.signature_class());
     ASSERT(!cls.IsNull());
     const bool requires_type_arguments = cls.NumTypeArguments() > 0;
     Value* type_arguments = NULL;
@@ -2345,13 +2340,72 @@
       type_arguments = BuildInstantiatorTypeArguments(node->token_pos(),
                                                       instantiator_class,
                                                       NULL);
-    } else {
-      type_arguments = BuildNullValue();
+      arguments->Add(PushArgument(type_arguments));
     }
-    PushArgumentInstr* push_type_arguments = PushArgument(type_arguments);
-    arguments->Add(push_type_arguments);
-    ReturnDefinition(
-        new CreateClosureInstr(node->function(), arguments, node->token_pos()));
+    AllocateObjectInstr* alloc = new AllocateObjectInstr(node->token_pos(),
+                                                         cls,
+                                                         arguments);
+    alloc->set_closure_function(function);
+
+    // Create fake fields for function and context. Only the context field is
+    // stored at the allocation to be used later when inlining a closure call.
+    const Field& function_field =
+        Field::ZoneHandle(
+            Field::New(Symbols::ClosureFunctionField(),
+            false,  // !static
+            false,  // !final
+            false,  // !const
+            alloc->cls(),
+            0));  // No token position.
+    function_field.SetOffset(Closure::function_offset());
+    const Field& context_field =
+        Field::ZoneHandle(Field::New(
+            Symbols::ClosureContextField(),
+            false,  // !static
+            false,  // !final
+            false,  // !const
+            alloc->cls(),
+            0));  // No token position.
+    context_field.SetOffset(Closure::context_offset());
+    alloc->set_context_field(context_field);
+
+    Value* closure_val = Bind(alloc);
+    { LocalVariable* closure_tmp_var = EnterTempLocalScope(closure_val);
+      // Store function.
+      Value* closure_tmp_val = Bind(new LoadLocalInstr(*closure_tmp_var));
+      Value* func_val =
+          Bind(new ConstantInstr(Function::ZoneHandle(function.raw())));
+      Do(new StoreInstanceFieldInstr(function_field,
+                                     closure_tmp_val,
+                                     func_val,
+                                     kEmitStoreBarrier));
+      // Create new context containing the receiver.
+      const intptr_t kNumContextVariables = 1;  // The receiver.
+      Value* allocated_context =
+          Bind(new AllocateContextInstr(node->token_pos(),
+                                        kNumContextVariables));
+      { LocalVariable* context_tmp_var = EnterTempLocalScope(allocated_context);
+        // Store receiver in new context.
+        Value* context_tmp_val = Bind(new LoadLocalInstr(*context_tmp_var));

[Florian Schneider, 2014/02/25 09:21:15]

In StoreVMFieldInstr the order of arguments is reversed relative to
StoreInstanceField. It is an artifact of how we build stores to captured locals.
(and should probably be changed, since it is annoying to get right)

I think the bug is fixed by reordering evaluation of the input operands here:

// First operand (receiver).
ValueGraphVisitor for_receiver(owner());
node->receiver()->Visit(&for_receiver);
Append(for_receiver);
Value* receiver = for_receiver.value();

// Second operand (context)
Value* context_tmp_val = Bind(new LoadLocalInstr(*context_tmp_var));

+        ValueGraphVisitor for_receiver(owner());
+        node->receiver()->Visit(&for_receiver);
+        Append(for_receiver);
+        Value* receiver = for_receiver.value();
+        Do(new StoreVMFieldInstr(context_tmp_val,
+                                 Context::variable_offset(0),
+                                 receiver,
+                                 Type::ZoneHandle()));
+        // Store new context.
+        closure_tmp_val = Bind(new LoadLocalInstr(*closure_tmp_var));
+        context_tmp_val = Bind(new LoadLocalInstr(*context_tmp_var));
+        Do(new StoreInstanceFieldInstr(context_field,
+                                       closure_tmp_val,
+                                       context_tmp_val,
+                                       kEmitStoreBarrier));
+        Do(ExitTempLocalScope(context_tmp_var));
+      }
+      ReturnDefinition(ExitTempLocalScope(closure_tmp_var));
+    }
   }
 }