Fix two bugs in the Dart VM's super-noSuchMethod invocation.

runtime/vm/flow_graph_builder.cc

Index: runtime/vm/flow_graph_builder.cc
===================================================================
--- runtime/vm/flow_graph_builder.cc	(revision 23405)
+++ runtime/vm/flow_graph_builder.cc	(working copy)
@@ -1840,8 +1840,10 @@
 
 class TempLocalScope : public ValueObject {
  public:
-  TempLocalScope(EffectGraphVisitor* visitor, Value* value)
-      : visitor_(visitor) {
+  TempLocalScope(EffectGraphVisitor* visitor,
+                 Value* value,
+                 bool return_definition = true)

[srdjan, 2013/05/30 17:42:30]

WOuld it be better to have a subclass instead of the flag return_definition?

[Florian Schneider, 2013/05/31 08:46:36]

I reconsidered using the constructor/destructor of a scoped object, but decided
to replace it with Enter- and Exit-helper functions.

The scoped object does not provide enough flexibility for the more complex graph
builder functions, like for example the BuildNoSuchMethod helper.

I think the result is as easy to read and use.

+      : visitor_(visitor), return_definition_(return_definition) {
     ASSERT(value->definition()->temp_index() == visitor->temp_index() - 1);
     intptr_t index = visitor->GetCurrentTempLocalIndex();
     char name[64];
@@ -1855,15 +1857,25 @@
 
   LocalVariable* var() const { return var_; }
 
-  ~TempLocalScope() {
-    Value* result = visitor_->Bind(new LoadLocalInstr(*var_));
+  Value* CloseAndReturnValue() {
+    ASSERT(!return_definition_);
+    Value* tmp = visitor_->Bind(new LoadLocalInstr(*var_));
     visitor_->DeallocateTempIndex(1);
-    visitor_->ReturnDefinition(new DropTempsInstr(1, result));
+    return visitor_->Bind(new DropTempsInstr(1, tmp));
   }
 
+  ~TempLocalScope() {
+    if (return_definition_) {
+      Value* result = visitor_->Bind(new LoadLocalInstr(*var_));
+      visitor_->DeallocateTempIndex(1);
+      visitor_->ReturnDefinition(new DropTempsInstr(1, result));
+    }
+  }
+
  private:
   EffectGraphVisitor* visitor_;
   LocalVariable* var_;
+  bool return_definition_;

[srdjan, 2013/05/30 17:42:30]

const

[Florian Schneider, 2013/05/31 08:46:36]

Replaced TempLocalScope with EnterScope/ExitScope functions.

 };
 
 
@@ -2582,10 +2594,12 @@
       // Resolve and call noSuchMethod.
       ArgumentListNode* arguments = new ArgumentListNode(node->token_pos());
       arguments->Add(node->receiver());
-      StaticCallInstr* call = BuildStaticNoSuchMethodCall(node->cls(),
-                                                          node->receiver(),
-                                                          getter_name,
-                                                          arguments);
+      StaticCallInstr* call =
+          BuildStaticNoSuchMethodCall(node->cls(),
+                                      node->receiver(),
+                                      getter_name,
+                                      arguments,
+                                      false);  // Don't save last argument.
       ReturnDefinition(call);
       return;
     } else {
@@ -2656,10 +2670,12 @@
       ArgumentListNode* arguments = new ArgumentListNode(node->token_pos());
       arguments->Add(node->receiver());
       arguments->Add(node->value());
-      call = BuildStaticNoSuchMethodCall(node->cls(),
-                                         node->receiver(),
-                                         setter_name,
-                                         arguments);
+      call = BuildStaticNoSuchMethodCall(
+          node->cls(),
+          node->receiver(),
+          setter_name,
+          arguments,
+          result_is_needed);  // Save last arg if result is needed.
     } else {
       // Throw a NoSuchMethodError.
       call = BuildThrowNoSuchMethodError(
@@ -2891,7 +2907,8 @@
           BuildStaticNoSuchMethodCall(node->super_class(),
                                       node->array(),
                                       Symbols::IndexToken(),
-                                      arguments);
+                                      arguments,
+                                      false);  // Don't save last arg.
       ReturnDefinition(call);
       return;
     }
@@ -2945,29 +2962,16 @@
       ArgumentListNode* arguments = new ArgumentListNode(node->token_pos());
       arguments->Add(node->array());
       arguments->Add(node->index_expr());
-
-      // Even though noSuchMethod most likely does not return,
-      // we save the stored value if the result is needed.
+      arguments->Add(node->value());
+      StaticCallInstr* call = BuildStaticNoSuchMethodCall(
+          node->super_class(),
+          node->array(),
+          Symbols::AssignIndexToken(),
+          arguments,
+          result_is_needed);  // Save last arg if result is needed.
       if (result_is_needed) {
-        ValueGraphVisitor for_value(owner(), temp_index());
-        node->value()->Visit(&for_value);
-        Append(for_value);
-        Do(BuildStoreExprTemp(for_value.value()));
-
-        const LocalVariable* temp =
-            owner()->parsed_function().expression_temp_var();
-        AstNode* value = new LoadLocalNode(node->token_pos(), temp);
-        arguments->Add(value);
-      } else {
-        arguments->Add(node->value());
-      }
-      StaticCallInstr* call =
-          BuildStaticNoSuchMethodCall(node->super_class(),
-                                      node->array(),
-                                      Symbols::AssignIndexToken(),
-                                      arguments);
-      if (result_is_needed) {
         Do(call);
+        // BuildStaticNoSuchMethodCall stores the value in expression_temp.
         return BuildLoadExprTemp();
       } else {
         return call;
@@ -3317,7 +3321,8 @@
     const Class& target_class,
     AstNode* receiver,
     const String& method_name,
-    ArgumentListNode* method_arguments) {
+    ArgumentListNode* method_arguments,
+    bool save_last_arg) {
   // Build the graph to allocate an InvocationMirror object by calling
   // the static allocation method.
   const Library& corelib = Library::Handle(Library::CoreLibrary());
@@ -3340,27 +3345,60 @@
 
   // Allocate the arguments and pass them into the construction
   // of the InvocationMirror.
-  const intptr_t args_pos = method_arguments->token_pos();
-  ArgumentListNode* arguments = new ArgumentListNode(args_pos);
+  ZoneGrowableArray<PushArgumentInstr*>* allocation_args =
+      new ZoneGrowableArray<PushArgumentInstr*>(3);
   // The first argument is the original method name.
-  arguments->Add(new LiteralNode(args_pos, method_name));
+  allocation_args->Add(PushArgument(Bind(new ConstantInstr(method_name))));
+
   // The second argument is the arguments descriptor of the original method.
   const Array& args_descriptor =
       Array::ZoneHandle(ArgumentsDescriptor::New(method_arguments->length(),
                                                  method_arguments->names()));
-  arguments->Add(new LiteralNode(args_pos, args_descriptor));
+  allocation_args->Add(PushArgument(Bind(new ConstantInstr(args_descriptor))));
+
   // The third argument is an array containing the original method arguments,
   // including the receiver.
-  ArrayNode* args_array = new ArrayNode(
-      args_pos,
-      Type::ZoneHandle(Type::ArrayType()));
-  for (intptr_t i = 0; i < method_arguments->length(); i++) {
-    args_array->AddElement(method_arguments->NodeAt(i));
+  intptr_t args_pos = method_arguments->token_pos();
+  Value* element_type =
+      Bind(new ConstantInstr(AbstractTypeArguments::ZoneHandle()));
+  CreateArrayInstr* create =
+      new CreateArrayInstr(args_pos,
+                           method_arguments->length(),
+                           Type::ZoneHandle(Type::ArrayType()),
+                           element_type);
+  Value* args_array = Bind(create);
+  { TempLocalScope tmp(this,
+                       args_array,
+                       false);  // Don't pass value to the visitor.
+    const intptr_t class_id = create->Type()->ToCid();
+    const intptr_t deopt_id = Isolate::kNoDeoptId;
+    for (int i = 0; i < method_arguments->length(); ++i) {
+      Value* array = Bind(new LoadLocalInstr(*tmp.var()));
+      Value* index = Bind(new ConstantInstr(Smi::ZoneHandle(Smi::New(i))));
+      ValueGraphVisitor for_value(owner(), temp_index());
+      method_arguments->NodeAt(i)->Visit(&for_value);
+      Append(for_value);
+      Value* value = for_value.value();
+      if (save_last_arg && i == method_arguments->length() - 1) {

[srdjan, 2013/05/30 17:42:30]

Add parenthesis.

[Florian Schneider, 2013/05/31 08:51:19]

Done.

+        // We can use the expression_temp_var here because this is always the
+        // last argument before the call.
+        value = Bind(BuildStoreExprTemp(value));
+      }
+      // No store barrier needed for constants.
+      const StoreBarrierType emit_store_barrier =
+          value->BindsToConstant()
+              ? kNoStoreBarrier
+              : kEmitStoreBarrier;
+      intptr_t index_scale = FlowGraphCompiler::ElementSizeFor(class_id);
+      StoreIndexedInstr* store = new StoreIndexedInstr(
+          array, index, value,
+          emit_store_barrier, index_scale, class_id, deopt_id);
+      Do(store);
+    }
+    args_array = tmp.CloseAndReturnValue();
   }
-  arguments->Add(args_array);
-  ZoneGrowableArray<PushArgumentInstr*>* allocation_args =
-      new ZoneGrowableArray<PushArgumentInstr*>(arguments->length());
-  BuildPushArguments(*arguments, allocation_args);
+  allocation_args->Add(PushArgument(args_array));
+
   StaticCallInstr* allocation = new StaticCallInstr(args_pos,
                                                     allocation_function,
                                                     Array::ZoneHandle(),