From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2116) id 3534A3858C51; Fri, 1 Jul 2022 22:45:44 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 3534A3858C51 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" From: Ian Lance Taylor To: gcc-cvs@gcc.gnu.org Subject: [gcc r13-1402] compiler: use correct init order for multi-value initialization X-Act-Checkin: gcc X-Git-Author: Ian Lance Taylor X-Git-Refname: refs/heads/master X-Git-Oldrev: 1697806fdf25285b924251b0d785324775e9b905 X-Git-Newrev: fbd7665360d259434f378f68cb2680b17d6cab57 Message-Id: <20220701224544.3534A3858C51@sourceware.org> Date: Fri, 1 Jul 2022 22:45:44 +0000 (GMT) X-BeenThere: gcc-cvs@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-cvs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Jul 2022 22:45:44 -0000 https://gcc.gnu.org/g:fbd7665360d259434f378f68cb2680b17d6cab57 commit r13-1402-gfbd7665360d259434f378f68cb2680b17d6cab57 Author: Ian Lance Taylor Date: Thu Jun 30 17:40:00 2022 -0700 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 Diff: --- gcc/go/gofrontend/MERGE | 2 +- gcc/go/gofrontend/go.cc | 3 + gcc/go/gofrontend/gogo.cc | 202 +++++++++++++++++++++++++-------------------- gcc/go/gofrontend/gogo.h | 23 +++++- gcc/go/gofrontend/parse.cc | 18 +++- 5 files changed, 150 insertions(+), 98 deletions(-) diff --git a/gcc/go/gofrontend/MERGE b/gcc/go/gofrontend/MERGE index 65f64e0fbfb..7b1d3011fff 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 404cb124549..1512770af29 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 67b91fab4ca..9197eef3e38 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* - 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* 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; - 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_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* refs = pvar->refs(); + const std::vector* refs = + pvar->var()->var_value()->init_refs(); if (refs == NULL) continue; for (std::vector::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* deprefs = dep_init->refs(); + const std::vector* deprefs = + dep_init->var()->var_value()->init_refs(); if (deprefs == NULL) continue; for (std::vector::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; + 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 2ee0fda00ae..433fdaebb66 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* + 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* init_refs_; // Any associated go:embed comments. std::vector* 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 e388261f494..a3c6f630a09 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()) {