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
Change-Id: I095b5a330ab657fc3c7b366e85c881c2a80e5a1a
Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/415594
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
diff --git a/go/go.cc b/go/go.cc
index 404cb12..1512770 100644
--- a/go/go.cc
+++ b/go/go.cc
@@ -146,6 +146,9 @@
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/go/gogo.cc b/go/gogo.cc
index 67b91fa..9197eef 100644
--- a/go/gogo.cc
+++ b/go/gogo.cc
@@ -1086,8 +1086,8 @@
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 @@
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 @@
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 @@
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 @@
{
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 @@
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 @@
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 @@
// 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 @@
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 @@
{
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 @@
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 @@
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 @@
}
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 @@
// 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 @@
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 @@
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 @@
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/go/gogo.h b/go/gogo.h
index 2ee0fda..433fdae 100644
--- a/go/gogo.h
+++ b/go/gogo.h
@@ -842,6 +842,11 @@
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 @@
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 @@
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 @@
// 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/go/parse.cc b/go/parse.cc
index e388261..a3c6f63 100644
--- a/go/parse.cc
+++ b/go/parse.cc
@@ -1977,7 +1977,11 @@
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 @@
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 @@
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())
{
