compiler: use correct init order for multi-value initialization

Use the correct initialization order for

var a = c
var b, c = x.(bool)

The global c is initialized by the preinit of b, but were missing a
dependency of c on b, so a would be initialized to the zero value of c
rather than the correct value.

Simply adding the dependency of c on b didn't work because the preinit
of b refers to c, so that appeared circular.  So this patch changes
the init order to skip dependencies that only appear on the left hand
side of assignments in preinit blocks.

Doing that didn't work because the write barrier pass can transform "a
= b" into code like "gcWriteBarrier(&a, b)" that is not obviously a
simple assigment.  So this patch moves the collection of dependencies
to just after lowering, before the write barriers are inserted.

Making those changes permit relaxing the requirement that we don't
warn about self-dependency in preinit blocks, so now we correctly warn
for

var a, b any = b.(bool)

The test case is https://go.dev/cl/415238.

Fixes golang/go#53619

Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/415594

5 files changed, 150 insertions, 98 deletions

diff --git a/gcc/go/gofrontend/MERGE b/gcc/go/gofrontend/MERGE
index 65f64e0f..7b1d301 100644
--- a/gcc/go/gofrontend/MERGE
+++ b/gcc/go/gofrontend/MERGE
@@ -1,4 +1,4 @@
-ac438edc5335f69c95df9342f43712ad2f61ad66
+6479d5976c5d848ec6f5843041275723a00006b0
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
diff --git a/gcc/go/gofrontend/go.cc b/gcc/go/gofrontend/go.cc
index 404cb12..1512770 100644
--- a/gcc/go/gofrontend/go.cc
+++ b/gcc/go/gofrontend/go.cc
@@ -146,6 +146,9 @@ go_parse_input_files(const char** filenames, unsigned int filename_count,
   if (only_check_syntax)
     return;
 
+  // Record global variable initializer dependencies.
+  ::gogo->record_global_init_refs();
+
   // Do simple deadcode elimination.
   ::gogo->remove_deadcode();
 
diff --git a/gcc/go/gofrontend/gogo.cc b/gcc/go/gofrontend/gogo.cc
index 67b91fa..9197eef 100644
--- a/gcc/go/gofrontend/gogo.cc
+++ b/gcc/go/gofrontend/gogo.cc
@@ -1086,8 +1086,8 @@ class Find_vars : public Traverse
 
  public:
   Find_vars()
-    : Traverse(traverse_expressions),
-      vars_(), seen_objects_()
+    : Traverse(traverse_expressions | traverse_statements),
+      vars_(), seen_objects_(), lhs_is_ref_(false)
   { }
 
   // An iterator through the variables found, after the traversal.
@@ -1104,11 +1104,16 @@ class Find_vars : public Traverse
   int
   expression(Expression**);
 
+  int
+  statement(Block*, size_t* index, Statement*);
+
  private:
   // Accumulated variables.
   Vars vars_;
   // Objects we have already seen.
   Seen_objects seen_objects_;
+  // Whether an assignment to a variable counts as a reference.
+  bool lhs_is_ref_;
 };
 
 // Collect global variables referenced by EXPR.  Look through function
@@ -1164,7 +1169,11 @@ Find_vars::expression(Expression** pexpr)
 	  if (ins.second)
 	    {
 	      // This is the first time we have seen this name.
-	      if (f->func_value()->block()->traverse(this) == TRAVERSE_EXIT)
+	      bool hold = this->lhs_is_ref_;
+	      this->lhs_is_ref_ = true;
+	      int r = f->func_value()->block()->traverse(this);
+	      this->lhs_is_ref_ = hold;
+	      if (r == TRAVERSE_EXIT)
 		return TRAVERSE_EXIT;
 	    }
 	}
@@ -1192,6 +1201,29 @@ Find_vars::expression(Expression** pexpr)
   return TRAVERSE_CONTINUE;
 }
 
+// Check a statement while searching for variables.  This is where we
+// skip variables on the left hand side of assigments if appropriate.
+
+int
+Find_vars::statement(Block*, size_t*, Statement* s)
+{
+  if (this->lhs_is_ref_)
+    return TRAVERSE_CONTINUE;
+  Assignment_statement* as = s->assignment_statement();
+  if (as == NULL)
+    return TRAVERSE_CONTINUE;
+
+  // Only traverse subexpressions of the LHS.
+  if (as->lhs()->traverse_subexpressions(this) == TRAVERSE_EXIT)
+    return TRAVERSE_EXIT;
+
+  Expression* rhs = as->rhs();
+  if (Expression::traverse(&rhs, this) == TRAVERSE_EXIT)
+    return TRAVERSE_EXIT;
+
+  return TRAVERSE_SKIP_COMPONENTS;
+}
+
 // Return true if EXPR, PREINIT, or DEP refers to VAR.
 
 static bool
@@ -1230,11 +1262,11 @@ class Var_init
 {
  public:
   Var_init()
-    : var_(NULL), init_(NULL), refs_(NULL), dep_count_(0)
+    : var_(NULL), init_(NULL), dep_count_(0)
   { }
 
   Var_init(Named_object* var, Bstatement* init)
-    : var_(var), init_(init), refs_(NULL), dep_count_(0)
+    : var_(var), init_(init), dep_count_(0)
   { }
 
   // Return the variable.
@@ -1247,19 +1279,6 @@ class Var_init
   init() const
   { return this->init_; }
 
-  // Add a reference.
-  void
-  add_ref(Named_object* var);
-
-  // The variables which this variable's initializers refer to.
-  const std::vector<Named_object*>*
-  refs()
-  { return this->refs_; }
-
-  // Clear the references, if any.
-  void
-  clear_refs();
-
   // Return the number of remaining dependencies.
   size_t
   dep_count() const
@@ -1280,36 +1299,12 @@ class Var_init
   Named_object* var_;
   // The backend initialization statement.
   Bstatement* init_;
-  // Variables this refers to.
-  std::vector<Named_object*>* refs_;
   // The number of initializations this is dependent on.  A variable
   // initialization should not be emitted if any of its dependencies
   // have not yet been resolved.
   size_t dep_count_;
 };
 
-// Add a reference.
-
-void
-Var_init::add_ref(Named_object* var)
-{
-  if (this->refs_ == NULL)
-    this->refs_ = new std::vector<Named_object*>;
-  this->refs_->push_back(var);
-}
-
-// Clear the references, if any.
-
-void
-Var_init::clear_refs()
-{
-  if (this->refs_ != NULL)
-    {
-      delete this->refs_;
-      this->refs_ = NULL;
-    }
-}
-
 // For comparing Var_init keys in a map.
 
 inline bool
@@ -1324,7 +1319,7 @@ typedef std::list<Var_init> Var_inits;
 // variable V2 then we initialize V1 after V2.
 
 static void
-sort_var_inits(Gogo* gogo, Var_inits* var_inits)
+sort_var_inits(Var_inits* var_inits)
 {
   if (var_inits->empty())
     return;
@@ -1337,33 +1332,13 @@ sort_var_inits(Gogo* gogo, Var_inits* var_inits)
   Init_deps init_deps;
   bool init_loop = false;
 
-  // Compute all variable references.
+  // Map from variable to Var_init.
   for (Var_inits::iterator pvar = var_inits->begin();
        pvar != var_inits->end();
        ++pvar)
     {
       Named_object* var = pvar->var();
       var_to_init[var] = &*pvar;
-
-      Find_vars find_vars;
-      Expression* init = var->var_value()->init();
-      if (init != NULL)
-	Expression::traverse(&init, &find_vars);
-      if (var->var_value()->has_pre_init())
-	var->var_value()->preinit()->traverse(&find_vars);
-      Named_object* dep = gogo->var_depends_on(var->var_value());
-      if (dep != NULL)
-	{
-	  Expression* dinit = dep->var_value()->init();
-	  if (dinit != NULL)
-	    Expression::traverse(&dinit, &find_vars);
-	  if (dep->var_value()->has_pre_init())
-	    dep->var_value()->preinit()->traverse(&find_vars);
-	}
-      for (Find_vars::const_iterator p = find_vars.begin();
-	   p != find_vars.end();
-	   ++p)
-	pvar->add_ref(*p);
     }
 
   // Add dependencies to init_deps, and check for cycles.
@@ -1373,7 +1348,8 @@ sort_var_inits(Gogo* gogo, Var_inits* var_inits)
     {
       Named_object* var = pvar->var();
 
-      const std::vector<Named_object*>* refs = pvar->refs();
+      const std::vector<Named_object*>* refs =
+	pvar->var()->var_value()->init_refs();
       if (refs == NULL)
 	continue;
       for (std::vector<Named_object*>::const_iterator pdep = refs->begin();
@@ -1383,19 +1359,11 @@ sort_var_inits(Gogo* gogo, Var_inits* var_inits)
 	  Named_object* dep = *pdep;
 	  if (var == dep)
 	    {
-	      // This is a reference from a variable to itself, which
-	      // may indicate a loop.  We only report an error if
-	      // there is an initializer and there is no dependency.
-	      // When there is no initializer, it means that the
-	      // preinitializer sets the variable, which will appear
-	      // to be a loop here.
-	      if (var->var_value()->init() != NULL
-		  && gogo->var_depends_on(var->var_value()) == NULL)
-		go_error_at(var->location(),
-			    ("initialization expression for %qs "
-			     "depends upon itself"),
-			    var->message_name().c_str());
-
+	      // This is a reference from a variable to itself.
+	      go_error_at(var->location(),
+			  ("initialization expression for %qs "
+			   "depends upon itself"),
+			  var->message_name().c_str());
 	      continue;
 	    }
 
@@ -1412,7 +1380,8 @@ sort_var_inits(Gogo* gogo, Var_inits* var_inits)
 	  pvar->add_dependency();
 
 	  // Check for cycles.
-	  const std::vector<Named_object*>* deprefs = dep_init->refs();
+	  const std::vector<Named_object*>* deprefs =
+	    dep_init->var()->var_value()->init_refs();
 	  if (deprefs == NULL)
 	    continue;
 	  for (std::vector<Named_object*>::const_iterator pdepdep =
@@ -1437,10 +1406,6 @@ sort_var_inits(Gogo* gogo, Var_inits* var_inits)
     }
 
   var_to_init.clear();
-  for (Var_inits::iterator pvar = var_inits->begin();
-       pvar != var_inits->end();
-       ++pvar)
-    pvar->clear_refs();
 
   // If there are no dependencies then the declaration order is sorted.
   if (!init_deps.empty() && !init_loop)
@@ -1748,7 +1713,7 @@ Gogo::write_globals()
   // workable order.
   if (!var_inits.empty())
     {
-      sort_var_inits(this, &var_inits);
+      sort_var_inits(&var_inits);
       for (Var_inits::const_iterator p = var_inits.begin();
            p != var_inits.end();
            ++p)
@@ -3840,6 +3805,51 @@ Gogo::check_types_in_block(Block* block)
   block->traverse(&traverse);
 }
 
+// For each global variable defined in the current package, record the
+// set of variables that its initializer depends on.  We do this after
+// lowering so that all unknown names are resolved to their final
+// locations.  We do this before write barrier insertion because that
+// makes it harder to distinguish references from assignments in
+// preinit blocks.
+
+void
+Gogo::record_global_init_refs()
+{
+  Bindings* bindings = this->package_->bindings();
+  for (Bindings::const_definitions_iterator pb = bindings->begin_definitions();
+       pb != bindings->end_definitions();
+       pb++)
+    {
+      Named_object* no = *pb;
+      if (!no->is_variable())
+	continue;
+
+      Variable* var = no->var_value();
+      go_assert(var->is_global());
+
+      Find_vars find_vars;
+      Expression* init = var->init();
+      if (init != NULL)
+	Expression::traverse(&init, &find_vars);
+      if (var->has_pre_init())
+	var->preinit()->traverse(&find_vars);
+      Named_object* dep = this->var_depends_on(var);
+      if (dep != NULL)
+	{
+	  Expression* dinit = dep->var_value()->init();
+	  if (dinit != NULL)
+	    Expression::traverse(&dinit, &find_vars);
+	  if (dep->var_value()->has_pre_init())
+	    dep->var_value()->preinit()->traverse(&find_vars);
+	}
+
+      for (Find_vars::const_iterator pv = find_vars.begin();
+	   pv != find_vars.end();
+	   ++pv)
+	var->add_init_ref(*pv);
+    }
+}
+
 // A traversal class which finds all the expressions which must be
 // evaluated in order within a statement or larger expression.  This
 // is used to implement the rules about order of evaluation.
@@ -7422,16 +7432,16 @@ Variable::Variable(Type* type, Expression* init, bool is_global,
 		   bool is_parameter, bool is_receiver,
 		   Location location)
   : type_(type), init_(init), preinit_(NULL), location_(location),
-    embeds_(NULL), backend_(NULL), is_global_(is_global),
-    is_parameter_(is_parameter), is_closure_(false), is_receiver_(is_receiver),
-    is_varargs_parameter_(false), is_global_sink_(false), is_used_(false),
-    is_address_taken_(false), is_non_escaping_address_taken_(false),
-    seen_(false), init_is_lowered_(false), init_is_flattened_(false),
+    toplevel_decl_(NULL), init_refs_(NULL), embeds_(NULL), backend_(NULL),
+    is_global_(is_global), is_parameter_(is_parameter), is_closure_(false),
+    is_receiver_(is_receiver), is_varargs_parameter_(false),
+    is_global_sink_(false), is_used_(false), is_address_taken_(false),
+    is_non_escaping_address_taken_(false), seen_(false),
+    init_is_lowered_(false), init_is_flattened_(false),
     type_from_init_tuple_(false), type_from_range_index_(false),
     type_from_range_value_(false), type_from_chan_element_(false),
     is_type_switch_var_(false), determined_type_(false),
-    in_unique_section_(false), is_referenced_by_inline_(false),
-    toplevel_decl_(NULL)
+    in_unique_section_(false), is_referenced_by_inline_(false)
 {
   go_assert(type != NULL || init != NULL);
   go_assert(!is_parameter || init == NULL);
@@ -7921,6 +7931,16 @@ Variable::get_init_block(Gogo* gogo, Named_object* function,
   return block_stmt;
 }
 
+// Add an initializer reference.
+
+void
+Variable::add_init_ref(Named_object* var)
+{
+  if (this->init_refs_ == NULL)
+    this->init_refs_ = new std::vector<Named_object*>;
+  this->init_refs_->push_back(var);
+}
+
 // Export the variable
 
 void
diff --git a/gcc/go/gofrontend/gogo.h b/gcc/go/gofrontend/gogo.h
index 2ee0fda..433fdae 100644
--- a/gcc/go/gofrontend/gogo.h
+++ b/gcc/go/gofrontend/gogo.h
@@ -842,6 +842,11 @@ class Gogo
   void
   check_return_statements();
 
+  // Gather references from global variables initializers to other
+  // variables.
+  void
+  record_global_init_refs();
+
   // Remove deadcode.
   void
   remove_deadcode();
@@ -2333,6 +2338,15 @@ class Variable
     this->toplevel_decl_ = s;
   }
 
+  // Note that the initializer of this global variable refers to VAR.
+  void
+  add_init_ref(Named_object* var);
+
+  // The variables that this variable's initializers refer to.
+  const std::vector<Named_object*>*
+  init_refs() const
+  { return this->init_refs_; }
+
   // Traverse the initializer expression.
   int
   traverse_expression(Traverse*, unsigned int traverse_mask);
@@ -2389,6 +2403,12 @@ class Variable
   Block* preinit_;
   // Location of variable definition.
   Location location_;
+  // The top-level declaration for this variable. Only used for local
+  // variables. Must be a Temporary_statement if not NULL.
+  Statement* toplevel_decl_;
+  // Variables that the initializer of a global variable refers to.
+  // Used for initializer ordering.
+  std::vector<Named_object*>* init_refs_;
   // Any associated go:embed comments.
   std::vector<std::string>* embeds_;
   // Backend representation.
@@ -2439,9 +2459,6 @@ class Variable
   // True if this variable is referenced from an inlined body that
   // will be put into the export data.
   bool is_referenced_by_inline_ : 1;
-  // The top-level declaration for this variable. Only used for local
-  // variables. Must be a Temporary_statement if not NULL.
-  Statement* toplevel_decl_;
 };
 
 // A variable which is really the name for a function return value, or
diff --git a/gcc/go/gofrontend/parse.cc b/gcc/go/gofrontend/parse.cc
index e388261..a3c6f63 100644
--- a/gcc/go/gofrontend/parse.cc
+++ b/gcc/go/gofrontend/parse.cc
@@ -1977,7 +1977,11 @@ Parse::init_vars_from_map(const Typed_identifier_list* vars, Type* type,
   else if (!val_no->is_sink())
     {
       if (val_no->is_variable())
-	val_no->var_value()->add_preinit_statement(this->gogo_, s);
+	{
+	  val_no->var_value()->add_preinit_statement(this->gogo_, s);
+	  if (no->is_variable())
+	    this->gogo_->record_var_depends_on(no->var_value(), val_no);
+	}
     }
   else if (!no->is_sink())
     {
@@ -2044,7 +2048,11 @@ Parse::init_vars_from_receive(const Typed_identifier_list* vars, Type* type,
   else if (!val_no->is_sink())
     {
       if (val_no->is_variable())
-	val_no->var_value()->add_preinit_statement(this->gogo_, s);
+	{
+	  val_no->var_value()->add_preinit_statement(this->gogo_, s);
+	  if (no->is_variable())
+	    this->gogo_->record_var_depends_on(no->var_value(), val_no);
+	}
     }
   else if (!no->is_sink())
     {
@@ -2110,7 +2118,11 @@ Parse::init_vars_from_type_guard(const Typed_identifier_list* vars,
   else if (!val_no->is_sink())
     {
       if (val_no->is_variable())
-	val_no->var_value()->add_preinit_statement(this->gogo_, s);
+	{
+	  val_no->var_value()->add_preinit_statement(this->gogo_, s);
+	  if (no->is_variable())
+	    this->gogo_->record_var_depends_on(no->var_value(), val_no);
+	}
     }
   else if (!no->is_sink())
     {