From patchwork Thu Jul 18 05:05:39 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ian Lance Taylor X-Patchwork-Id: 1133532 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-505248-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=golang.org Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="oiYAAr6j"; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=golang-org.20150623.gappssmtp.com header.i=@golang-org.20150623.gappssmtp.com header.b="cY026B9L"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 45q2Cf5dzqz9s4Y for ; Thu, 18 Jul 2019 15:06:04 +1000 (AEST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:from:date:message-id:subject:to:content-type; q= dns; s=default; b=NUqZ6S5QLQcUqBEVaM6tgyEHfkQkl8wlQi61zJ0ic5+Bmw EXlktMOP+55K8kTfKUYXdaIVV/viVLvb9OIE+NlbgZypQZdKxSVcidrijLT5fonf fd2dy1Gpn+Fpo0waxlc3l0Tau+yazyOd38iAtvureyFYBHtQgRHiwRTK+bgVY= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:from:date:message-id:subject:to:content-type; s= default; bh=xAvdYjTt1ILLTqt2ptHkym8py+0=; b=oiYAAr6jMbOgR+QoX675 F2+UA1tcL3npIwc7/Zc+QhJNgfIMd6dm6qoPCTxXm8FSVimLJShSJ2SWYRCUmQ4B aoea0fw2uGmTveIKQbf3Yt/uJgqcszs6iuaQ9dMsbnX0pBIJAUvILWoy8Wl5DSHR g23iWmKkFIwe2HqxMR8Up/M= Received: (qmail 122300 invoked by alias); 18 Jul 2019 05:05:56 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 122133 invoked by uid 89); 18 Jul 2019 05:05:56 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-13.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_ASCII_DIVIDERS, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=discovery, exporter, sk:exports, exporting X-HELO: mail-lj1-f173.google.com Received: from mail-lj1-f173.google.com (HELO mail-lj1-f173.google.com) (209.85.208.173) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 18 Jul 2019 05:05:54 +0000 Received: by mail-lj1-f173.google.com with SMTP id z28so25976596ljn.4 for ; Wed, 17 Jul 2019 22:05:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=golang-org.20150623.gappssmtp.com; s=20150623; h=mime-version:from:date:message-id:subject:to; bh=R6SgayWrvXOdbe+tR9jehLUS2HcRiWB1xGqTsWqzd6U=; b=cY026B9L58f8Z79Sg+1qcgxG4vKySVjs17Fy5DMnEZylXZhtR8mIt3NZ/1py1OgpsX i5h0i39LaCvW9w52P2J0owUyRozQVeeR72r8p6RvErgpSVSn4OyOQBXB2fEDC4AGU/ke 04azL3mJU0405AVLzuugfN4RKglRZrOKJU0Y/QObnqdH+bbtWaPbksDyX3A79L2izAIi mp2teTHTZJ1jlYrD8g5AkVbRk9MbsIngYG5dQggUr9mAnOxzvkVpX44jmiTxsFD404tc 4kikudwj7Ofzw0h8SS6Y/+3Ysprs87J8/tDAvGvRaA+xcsy1rh/UE/I8tcTKHd490Omv am+w== MIME-Version: 1.0 From: Ian Lance Taylor Date: Wed, 17 Jul 2019 22:05:39 -0700 Message-ID: Subject: Go patch committed: Fix bug in unordered set when exporting To: gcc-patches , gofrontend-dev This Go frontend patch by Than McIntosh fixes a bug in the handling of unordered set during exporting. In https://golang.org/cl/183850 (https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00200.html) a change was made to combine tracking/discovery of exported types and imported packages during export data generation. As a result of this refactoring a bug was introduced: the new code can potentially insert items into the exports set (an unordered_set) while iterating through the same set, which is illegal according to the spec for std::unordered_set. This patch fixes the problem by changing the type discovery phase to iterate through a separate list of sorted exports, as opposed to iterating through the main unordered set. Also included is a change to fix the code that looks for variables that are referenced from inlined routine bodies (this code wasn't scanning all of the function that it needed to scan). There is a new test case for this problem in https://golang.org/cl/186697. This is for https://golang.org/issue/33020. Bootstrapped and ran Go testsuite on x86_64-pc-linux-gnu. Committed to mainline. Ian Index: gcc/go/gofrontend/MERGE =================================================================== --- gcc/go/gofrontend/MERGE (revision 273534) +++ gcc/go/gofrontend/MERGE (working copy) @@ -1,4 +1,4 @@ -0e51b7e9c03c6f6bc3d06343f2050f17349ccdc3 +19ed722fb3ae5e618c746da20efb79fc837337cd The first line of this file holds the git revision number of the last merge done from the gofrontend repository. Index: gcc/go/gofrontend/export.cc =================================================================== --- gcc/go/gofrontend/export.cc (revision 273534) +++ gcc/go/gofrontend/export.cc (working copy) @@ -111,7 +111,7 @@ class Collect_export_references : public : Traverse(traverse_expressions | traverse_types), exp_(exp), exports_(exports), imports_(imports), - inline_fcn_worklist_(NULL) + inline_fcn_worklist_(NULL), exports_finalized_(false) { } // Initial entry point; performs a walk to expand the exports set. @@ -121,7 +121,7 @@ class Collect_export_references : public // Second entry point (called after the method above), to find // all types referenced by exports. void - prepare_types(); + prepare_types(const std::vector& sorted_exports); protected: // Override of parent class method. @@ -141,6 +141,13 @@ class Collect_export_references : public traverse_named_type(Named_type*); private: + + // Add a named object to the exports set (during expand_exports()). + // Returns TRUE if a new object was added to the exports set, + // FALSE otherwise. + bool + add_to_exports(Named_object*); + // The exporter. Export* exp_; // The set of named objects to export. @@ -152,6 +159,8 @@ class Collect_export_references : public // Worklist of functions we are exporting with inline bodies that need // to be checked. std::vector* inline_fcn_worklist_; + // Set to true if expand_exports() has been called and is complete. + bool exports_finalized_; }; void @@ -172,6 +181,18 @@ Collect_export_references::expand_export } } this->inline_fcn_worklist_ = NULL; + this->exports_finalized_ = true; +} + +bool +Collect_export_references::add_to_exports(Named_object* no) +{ + std::pair ins = + this->exports_->insert(no); + // If the export list has been finalized, then we should not be + // adding anything new to the exports set. + go_assert(!this->exports_finalized_ || !ins.second); + return ins.second; } int @@ -189,7 +210,7 @@ Collect_export_references::expression(Ex if (var_package != NULL) this->imports_->insert(var_package); - this->exports_->insert(no); + this->add_to_exports(no); no->var_value()->set_is_referenced_by_inline(); } return TRAVERSE_CONTINUE; @@ -210,17 +231,16 @@ Collect_export_references::expression(Ex if (this->inline_fcn_worklist_ != NULL) { - std::pair ins = - this->exports_->insert(no); + bool added = this->add_to_exports(no); if (no->is_function()) no->func_value()->set_is_referenced_by_inline(); - // If ins.second is false then this object was already in + // If 'added' is false then this object was already in // exports_, in which case it was already added to // check_inline_refs_ the first time we added it to exports_, so // we don't need to add it again. - if (ins.second + if (added && no->is_function() && no->func_value()->export_for_inlining()) this->inline_fcn_worklist_->push_back(no); @@ -238,11 +258,11 @@ Collect_export_references::expression(Ex // exported inline function from another package). void -Collect_export_references::prepare_types() +Collect_export_references::prepare_types(const std::vector& sorted_exports) { // Iterate through the exported objects and traverse any types encountered. - for (Unordered_set(Named_object*)::iterator p = this->exports_->begin(); - p != this->exports_->end(); + for (std::vector::const_iterator p = sorted_exports.begin(); + p != sorted_exports.end(); ++p) { Named_object* no = *p; @@ -506,7 +526,8 @@ Export::export_globals(const std::string const std::map& imports, const std::string& import_init_fn, const Import_init_set& imported_init_fns, - const Bindings* bindings) + const Bindings* bindings, + Unordered_set(Named_object*)* functions_marked_inline) { // If there have been any errors so far, don't try to export // anything. That way the export code doesn't have to worry about @@ -520,35 +541,21 @@ Export::export_globals(const std::string // CHECK_INLINE_REFS is also on EXPORTS. Unordered_set(Named_object*) exports; std::vector check_inline_refs; + check_inline_refs.reserve(functions_marked_inline->size()); + + // Add all functions/methods from the "marked inlined" set to the + // CHECK_INLINE_REFS worklist. + for (Unordered_set(Named_object*)::const_iterator p = functions_marked_inline->begin(); + p != functions_marked_inline->end(); + ++p) + check_inline_refs.push_back(*p); for (Bindings::const_definitions_iterator p = bindings->begin_definitions(); p != bindings->end_definitions(); ++p) { if (should_export(*p)) - { - exports.insert(*p); - - if ((*p)->is_function() - && (*p)->func_value()->export_for_inlining()) - check_inline_refs.push_back(*p); - else if ((*p)->is_type()) - { - const Bindings* methods = (*p)->type_value()->local_methods(); - if (methods != NULL) - { - for (Bindings::const_definitions_iterator pm = - methods->begin_definitions(); - pm != methods->end_definitions(); - ++pm) - { - Function* fn = (*pm)->func_value(); - if (fn->export_for_inlining()) - check_inline_refs.push_back(*pm); - } - } - } - } + exports.insert(*p); } for (Bindings::const_declarations_iterator p = @@ -593,7 +600,7 @@ Export::export_globals(const std::string // Collect up the set of types mentioned in things we're exporting, // and any packages that may be referred to indirectly. - collect.prepare_types(); + collect.prepare_types(sorted_exports); // Assign indexes to all exported types and types referenced by // things we're exporting. Return value is index of first non-exported Index: gcc/go/gofrontend/export.h =================================================================== --- gcc/go/gofrontend/export.h (revision 273534) +++ gcc/go/gofrontend/export.h (working copy) @@ -158,7 +158,8 @@ class Export : public String_dump const std::map& imports, const std::string& import_init_fn, const Import_init_set& imported_init_fns, - const Bindings* bindings); + const Bindings* bindings, + Unordered_set(Named_object*)* marked_inline_functions); // Record a type that is mentioned in export data. Return value is // TRUE for newly visited types, FALSE for types that have been seen Index: gcc/go/gofrontend/gogo.cc =================================================================== --- gcc/go/gofrontend/gogo.cc (revision 273534) +++ gcc/go/gofrontend/gogo.cc (working copy) @@ -5078,9 +5078,10 @@ Inline_within_budget::expression(Express class Mark_inline_candidates : public Traverse { public: - Mark_inline_candidates() + Mark_inline_candidates(Unordered_set(Named_object*)* marked) : Traverse(traverse_functions - | traverse_types) + | traverse_types), + marked_functions_(marked) { } int @@ -5097,6 +5098,9 @@ class Mark_inline_candidates : public Tr // budget is a heuristic. In the usual GCC spirit, we could // consider setting this via a command line option. const int budget_heuristic = 80; + + // Set of named objects that are marked as inline candidates. + Unordered_set(Named_object*)* marked_functions_; }; // Mark a function if it is an inline candidate. @@ -5109,7 +5113,10 @@ Mark_inline_candidates::function(Named_o Inline_within_budget iwb(&budget); func->block()->traverse(&iwb); if (budget >= 0) - func->set_export_for_inlining(); + { + func->set_export_for_inlining(); + this->marked_functions_->insert(no); + } return TRAVERSE_CONTINUE; } @@ -5135,7 +5142,10 @@ Mark_inline_candidates::type(Type* t) Inline_within_budget iwb(&budget); func->block()->traverse(&iwb); if (budget >= 0) - func->set_export_for_inlining(); + { + func->set_export_for_inlining(); + this->marked_functions_->insert(no); + } } return TRAVERSE_CONTINUE; } @@ -5150,7 +5160,8 @@ Gogo::do_exports() // Mark any functions whose body should be exported for inlining by // other packages. - Mark_inline_candidates mic; + Unordered_set(Named_object*) marked_functions; + Mark_inline_candidates mic(&marked_functions); this->traverse(&mic); // For now we always stream to a section. Later we may want to @@ -5187,7 +5198,8 @@ Gogo::do_exports() this->imports_, init_fn_name, this->imported_init_fns_, - this->package_->bindings()); + this->package_->bindings(), + &marked_functions); if (!this->c_header_.empty() && !saw_errors()) this->write_c_header();