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

runtime/vm/flow_graph_builder.cc

 // Copyright (c) 2012, 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 "vm/flow_graph_builder.h"
 
 #include "lib/invocation_mirror.h"
 #include "vm/ast_printer.h"
 #include "vm/code_descriptors.h"
 #include "vm/dart_entry.h"
@@ skipping 1822 matching lines @@
   return kFirstLocalSlotFromFp
       - owner()->num_stack_locals()
       - owner()->num_copied_params()
       - owner()->args_pushed()
       - temp_index() + 1;
 }
 
 
 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];
     OS::SNPrint(name, 64, ":tmp_local%"Pd, index);
     var_ = new LocalVariable(0, String::ZoneHandle(Symbols::New(name)),
                              Type::ZoneHandle(Type::DynamicType()));
     var_->set_index(index);
     visitor->Do(new PushTempInstr(value));
     visitor->AllocateTempIndex();
   }
 
   LocalVariable* var() const { return var_; }
 
+  Value* CloseAndReturnValue() {
+    ASSERT(!return_definition_);
+    Value* tmp = visitor_->Bind(new LoadLocalInstr(*var_));
+    visitor_->DeallocateTempIndex(1);
+    return visitor_->Bind(new DropTempsInstr(1, tmp));
+  }
+
   ~TempLocalScope() {
-    Value* result = visitor_->Bind(new LoadLocalInstr(*var_));
-    visitor_->DeallocateTempIndex(1);
-    visitor_->ReturnDefinition(new DropTempsInstr(1, result));
+    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.

 };
 
 
 void EffectGraphVisitor::BuildLetTempExpressions(LetNode* node) {
   intptr_t num_temps = node->num_temps();
   for (intptr_t i = 0; i < num_temps; ++i) {
     ValueGraphVisitor for_value(owner(), temp_index());
     node->InitializerAt(i)->Visit(&for_value);
     Append(for_value);
     Value* temp_val = for_value.value();
@@ skipping 698 matching lines @@
       new ZoneGrowableArray<PushArgumentInstr*>();
   Function& getter_function = Function::ZoneHandle();
   if (node->is_super_getter()) {
     // Statically resolved instance getter, i.e. "super getter".
     ASSERT(node->receiver() != NULL);
     getter_function = Resolver::ResolveDynamicAnyArgs(node->cls(), getter_name);
     if (getter_function.IsNull()) {
       // 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 {
       ValueGraphVisitor receiver_value(owner(), temp_index());
       node->receiver()->Visit(&receiver_value);
       Append(receiver_value);
       arguments->Add(PushArgument(receiver_value.value()));
     }
   } else {
     getter_function = node->cls().LookupStaticFunction(getter_name);
@@ skipping 50 matching lines @@
           ? Resolver::ResolveDynamicAnyArgs(node->cls(), setter_name)
           : node->cls().LookupStaticFunction(setter_name));
   StaticCallInstr* call;
   if (setter_function.IsNull()) {
     if (is_super_setter) {
       ASSERT(node->receiver() != NULL);
       // Resolve and call noSuchMethod.
       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(
           node->token_pos(),
           node->cls(),
           setter_name,
           InvocationMirror::EncodeType(
             node->cls().IsTopLevel() ?
                 InvocationMirror::kTopLevel :
                 InvocationMirror::kStatic,
@@ skipping 211 matching lines @@
     if (super_function->IsNull()) {
       // Could not resolve super operator. Generate call noSuchMethod() of the
       // super class instead.
       ArgumentListNode* arguments = new ArgumentListNode(node->token_pos());
       arguments->Add(node->array());
       arguments->Add(node->index_expr());
       StaticCallInstr* call =
           BuildStaticNoSuchMethodCall(node->super_class(),
                                       node->array(),
                                       Symbols::IndexToken(),
-                                      arguments);
+                                      arguments,
+                                      false);  // Don't save last arg.
       ReturnDefinition(call);
       return;
     }
   }
   ZoneGrowableArray<PushArgumentInstr*>* arguments =
       new ZoneGrowableArray<PushArgumentInstr*>(2);
   ValueGraphVisitor for_array(owner(), temp_index());
   node->array()->Visit(&for_array);
   Append(for_array);
   arguments->Add(PushArgument(for_array.value()));
@@ skipping 33 matching lines @@
     // Resolve the store indexed operator in the super class.
     super_function = &Function::ZoneHandle(
         Resolver::ResolveDynamicAnyArgs(node->super_class(),
                                         Symbols::AssignIndexToken()));
     if (super_function->IsNull()) {
       // Could not resolve super operator. Generate call noSuchMethod() of the
       // super class instead.
       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.
-      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);
+      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) {
         Do(call);
+        // BuildStaticNoSuchMethodCall stores the value in expression_temp.
         return BuildLoadExprTemp();
       } else {
         return call;
       }
     }
   }
 
   ZoneGrowableArray<PushArgumentInstr*>* arguments =
       new ZoneGrowableArray<PushArgumentInstr*>(3);
   ValueGraphVisitor for_array(owner(), temp_index());
@@ skipping 329 matching lines @@
   }
 }
 
 
 // Looks up dynamic method noSuchMethod in target_class
 // (including its super class chain) and builds a static call to it.
 StaticCallInstr* EffectGraphVisitor::BuildStaticNoSuchMethodCall(
     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());
   const Class& mirror_class = Class::Handle(
       corelib.LookupClassAllowPrivate(Symbols::InvocationMirror()));
   ASSERT(!mirror_class.IsNull());
   const Function& allocation_function = Function::ZoneHandle(
       Resolver::ResolveStaticByName(
           mirror_class,
           PrivateCoreLibName(Symbols::AllocateInvocationMirror()),
           Resolver::kIsQualified));
   ASSERT(!allocation_function.IsNull());
 
   // Evaluate the receiver before the arguments. This will be used
   // as an argument to the noSuchMethod call.
   ValueGraphVisitor for_receiver(owner(), temp_index());
   receiver->Visit(&for_receiver);
   Append(for_receiver);
   PushArgumentInstr* push_receiver = PushArgument(for_receiver.value());
 
   // 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(),
                                                     allocation_args);
   Value* invocation_mirror = Bind(allocation);
   PushArgumentInstr* push_invocation_mirror = PushArgument(invocation_mirror);
   // Lookup noSuchMethod and call it with the receiver and the InvocationMirror.
   const Function& no_such_method_func = Function::ZoneHandle(
       Resolver::ResolveDynamicAnyArgs(target_class, Symbols::NoSuchMethod()));
   // We are guaranteed to find noSuchMethod of class Object.
@@ skipping 167 matching lines @@
   intptr_t len = OS::SNPrint(NULL, 0, kFormat, function_name, reason) + 1;
   char* chars = Isolate::Current()->current_zone()->Alloc<char>(len);
   OS::SNPrint(chars, len, kFormat, function_name, reason);
   const Error& error = Error::Handle(
       LanguageError::New(String::Handle(String::New(chars))));
   Isolate::Current()->long_jump_base()->Jump(1, error);
 }
 
 
 }  // namespace dart