Go patch committed: Don't evaluate interface conversions multiple times

Go patch committed: Don't evaluate interface conversions multiple times

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

There were a few cases where the Go compiler was evaluating value
multiple times when they were converted to or from interface types.
This happened when the conversion occurred in a composite literal, or
even, when implicitly converting from one interface type to another,
in a variable declaration.  For this patch I added an assertion for
the cases where a value might be evaluated multiple times, and then
patched the compiler until it stopped crashing on the testsuite.
Bootstrapped and ran Go testsuite on x86_64-unknown-linux-gnu.
Committed to mainline.  This bug does not occur on the 4.9 branch.
I've committed a test case to the master Go repository
(https://go-review.googlesource.com/#/c/1710/).

Ian

[-- Attachment #2: foo.txt --]
[-- Type: text/plain, Size: 8840 bytes --]

diff -r 47792391c17a go/expressions.cc
--- a/go/expressions.cc	Wed Dec 17 06:26:14 2014 -0800
+++ b/go/expressions.cc	Thu Dec 18 19:29:08 2014 -0800
@@ -302,6 +302,9 @@
                                            bool for_type_guard,
                                            Location location)
 {
+  if (Type::are_identical(lhs_type, rhs->type(), false, NULL))
+    return rhs;
+
   Interface_type* lhs_interface_type = lhs_type->interface_type();
   bool lhs_is_empty = lhs_interface_type->is_empty();
 
@@ -313,6 +316,9 @@
   // to do a runtime check, although we still need to build a new
   // method table.
 
+  // We are going to evaluate RHS multiple times.
+  go_assert(rhs->is_variable());
+
   // Get the type descriptor for the right hand side.  This will be
   // NULL for a nil interface.
   Expression* rhs_type_expr = Expression::get_interface_type_descriptor(rhs);
@@ -355,6 +361,9 @@
 Expression::convert_interface_to_type(Type *lhs_type, Expression* rhs,
                                       Location location)
 {
+  // We are going to evaluate RHS multiple times.
+  go_assert(rhs->is_variable());
+
   // Call a function to check that the type is valid.  The function
   // will panic with an appropriate runtime type error if the type is
   // not valid.
@@ -3155,8 +3164,7 @@
 {
   if (((this->type()->is_string_type()
         && this->expr_->type()->is_slice_type())
-       || (this->type()->interface_type() != NULL
-           && this->expr_->type()->interface_type() != NULL))
+       || this->expr_->type()->interface_type() != NULL)
       && !this->expr_->is_variable())
     {
       Temporary_statement* temp =
@@ -8782,12 +8790,17 @@
 	  else
 	    {
 	      Location loc = (*pa)->location();
-	      Expression* arg =
-		Expression::convert_for_assignment(gogo, pp->type(), *pa, loc);
-	      Temporary_statement* temp =
-		Statement::make_temporary(pp->type(), arg, loc);
-	      inserter->insert(temp);
-	      args->push_back(Expression::make_temporary_reference(temp, loc));
+	      Expression* arg = *pa;
+	      if (!arg->is_variable())
+		{
+		  Temporary_statement *temp =
+		    Statement::make_temporary(NULL, arg, loc);
+		  inserter->insert(temp);
+		  arg = Expression::make_temporary_reference(temp, loc);
+		}
+	      arg = Expression::convert_for_assignment(gogo, pp->type(), arg,
+						       loc);
+	      args->push_back(arg);
 	    }
 	}
       delete this->args_;
@@ -11602,6 +11615,9 @@
     return ret;
   }
 
+  Expression*
+  do_flatten(Gogo*, Named_object*, Statement_inserter*);
+
   Bexpression*
   do_get_backend(Translate_context*);
 
@@ -11776,6 +11792,39 @@
   go_assert(pv == this->vals_->end());
 }
 
+// Flatten a struct construction expression.  Store the values into
+// temporaries in case they need interface conversion.
+
+Expression*
+Struct_construction_expression::do_flatten(Gogo*, Named_object*,
+					   Statement_inserter* inserter)
+{
+  if (this->vals_ == NULL)
+    return this;
+
+  // If this is a constant struct, we don't need temporaries.
+  if (this->is_constant_struct())
+    return this;
+
+  Location loc = this->location();
+  for (Expression_list::iterator pv = this->vals_->begin();
+       pv != this->vals_->end();
+       ++pv)
+    {
+      if (*pv != NULL)
+	{
+	  if (!(*pv)->is_variable())
+	    {
+	      Temporary_statement* temp =
+		Statement::make_temporary(NULL, *pv, loc);
+	      inserter->insert(temp);
+	      *pv = Expression::make_temporary_reference(temp, loc);
+	    }
+	}
+    }
+  return this;
+}
+
 // Return the backend representation for constructing a struct.
 
 Bexpression*
@@ -11909,6 +11958,9 @@
   vals()
   { return this->vals_; }
 
+  Expression*
+  do_flatten(Gogo*, Named_object*, Statement_inserter*);
+
   // Get the backend constructor for the array values.
   Bexpression*
   get_constructor(Translate_context* context, Btype* btype);
@@ -12024,6 +12076,39 @@
     }
 }
 
+// Flatten an array construction expression.  Store the values into
+// temporaries in case they need interface conversion.
+
+Expression*
+Array_construction_expression::do_flatten(Gogo*, Named_object*,
+					   Statement_inserter* inserter)
+{
+  if (this->vals_ == NULL)
+    return this;
+
+  // If this is a constant array, we don't need temporaries.
+  if (this->is_constant_array())
+    return this;
+
+  Location loc = this->location();
+  for (Expression_list::iterator pv = this->vals_->begin();
+       pv != this->vals_->end();
+       ++pv)
+    {
+      if (*pv != NULL)
+	{
+	  if (!(*pv)->is_variable())
+	    {
+	      Temporary_statement* temp =
+		Statement::make_temporary(NULL, *pv, loc);
+	      inserter->insert(temp);
+	      *pv = Expression::make_temporary_reference(temp, loc);
+	    }
+	}
+    }
+  return this;
+}
+
 // Get a constructor expression for the array values.
 
 Bexpression*
diff -r 47792391c17a go/gogo.cc
--- a/go/gogo.cc	Wed Dec 17 06:26:14 2014 -0800
+++ b/go/gogo.cc	Thu Dec 18 19:29:08 2014 -0800
@@ -5830,6 +5830,21 @@
 
       gogo->flatten_expression(function, inserter, &this->init_);
 
+      // If an interface conversion is needed, we need a temporary
+      // variable.
+      if (this->type_ != NULL
+	  && !Type::are_identical(this->type_, this->init_->type(), false,
+				  NULL)
+	  && this->init_->type()->interface_type() != NULL
+	  && !this->init_->is_variable())
+	{
+	  Temporary_statement* temp =
+	    Statement::make_temporary(NULL, this->init_, this->location_);
+	  inserter->insert(temp);
+	  this->init_ = Expression::make_temporary_reference(temp,
+							     this->location_);
+	}
+
       this->seen_ = false;
       this->init_is_flattened_ = true;
     }
diff -r 47792391c17a go/statements.cc
--- a/go/statements.cc	Wed Dec 17 06:26:14 2014 -0800
+++ b/go/statements.cc	Thu Dec 18 19:29:08 2014 -0800
@@ -521,6 +521,9 @@
   void
   do_check_types(Gogo*);
 
+  Statement*
+  do_flatten(Gogo*, Named_object*, Block*, Statement_inserter*);
+
   Bstatement*
   do_get_backend(Translate_context*);
 
@@ -606,6 +609,28 @@
     this->set_is_error();
 }
 
+// Flatten an assignment statement.  We may need a temporary for
+// interface conversion.
+
+Statement*
+Assignment_statement::do_flatten(Gogo*, Named_object*, Block*,
+				 Statement_inserter* inserter)
+{
+  if (!this->lhs_->is_sink_expression()
+      && !Type::are_identical(this->lhs_->type(), this->rhs_->type(),
+			      false, NULL)
+      && this->rhs_->type()->interface_type() != NULL
+      && !this->rhs_->is_variable())
+    {
+      Temporary_statement* temp =
+	Statement::make_temporary(NULL, this->rhs_, this->location());
+      inserter->insert(temp);
+      this->rhs_ = Expression::make_temporary_reference(temp,
+							this->location());
+    }
+  return this;
+}
+
 // Convert an assignment statement to the backend representation.
 
 Bstatement*
@@ -2480,12 +2505,13 @@
   gogo->add_block(b, location);
 
   gogo->lower_block(function, b);
-  gogo->flatten_block(function, b);
 
   // We already ran the determine_types pass, so we need to run it
   // just for the call statement now.  The other types are known.
   call_statement->determine_types();
 
+  gogo->flatten_block(function, b);
+
   if (may_call_recover || recover_arg != NULL)
     {
       // Dig up the call expression, which may have been changed
@@ -4412,6 +4438,27 @@
     }
 }
 
+// Flatten a send statement.  We may need a temporary for interface
+// conversion.
+
+Statement*
+Send_statement::do_flatten(Gogo*, Named_object*, Block*,
+			   Statement_inserter* inserter)
+{
+  Type* element_type = this->channel_->type()->channel_type()->element_type();
+  if (!Type::are_identical(element_type, this->val_->type(), false, NULL)
+      && this->val_->type()->interface_type() != NULL
+      && !this->val_->is_variable())
+    {
+      Temporary_statement* temp =
+	Statement::make_temporary(NULL, this->val_, this->location());
+      inserter->insert(temp);
+      this->val_ = Expression::make_temporary_reference(temp,
+							this->location());
+    }
+  return this;
+}
+
 // Convert a send statement to the backend representation.
 
 Bstatement*
@@ -4421,7 +4468,9 @@
 
   Channel_type* channel_type = this->channel_->type()->channel_type();
   Type* element_type = channel_type->element_type();
-  Expression* val = Expression::make_cast(element_type, this->val_, loc);
+  Expression* val = Expression::convert_for_assignment(context->gogo(),
+						       element_type,
+						       this->val_, loc);
 
   bool is_small;
   bool can_take_address;
diff -r 47792391c17a go/statements.h
--- a/go/statements.h	Wed Dec 17 06:26:14 2014 -0800
+++ b/go/statements.h	Thu Dec 18 19:29:08 2014 -0800
@@ -704,6 +704,9 @@
   void
   do_check_types(Gogo*);
 
+  Statement*
+  do_flatten(Gogo*, Named_object*, Block*, Statement_inserter*);
+
   Bstatement*
   do_get_backend(Translate_context*);