From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 108840 invoked by alias); 12 Aug 2019 07:34:39 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 108831 invoked by uid 89); 12 Aug 2019 07:34:39 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-17.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.1 spammy=U*mliska, Liska, mliskasusecz, liska X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 12 Aug 2019 07:34:37 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 7AC35ABD6; Mon, 12 Aug 2019 07:34:35 +0000 (UTC) Subject: Re: [PATCH] Properly register dead cgraph_nodes in passes.c. To: Martin Sebor , gcc-patches@gcc.gnu.org Cc: Jan Hubicka References: From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: <3172eee4-3cc9-1f48-8e12-20994d337baf@suse.cz> Date: Mon, 12 Aug 2019 08:51:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------44C7E959764D84C6369673F9" X-IsSubscribed: yes X-SW-Source: 2019-08/txt/msg00695.txt.bz2 This is a multi-part message in MIME format. --------------44C7E959764D84C6369673F9 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-length: 1973 On 8/9/19 4:39 PM, Martin Sebor wrote: > On 8/9/19 6:41 AM, Martin Liška wrote: >> Hi. >> >> The patch prevents crashes caused by fact that do_per_function_toporder >> uses get_uid () to register all dead cgraph_nodes. That does not work >> now as cgraph_nodes are directly released via ggc_free and so that one >> will see a garbage here. Second steps is to register all cgraph hooks >> and correctly hold add removed nodes. Doing that we'll not need the GGC nodes >> array. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> I can also build xalancbmk with -O2 -ffast-math where I previously saw >> the ICE. > > Just a comment on "style:" to make code more readable, GCC > coding conventions call for variables to be defined at the same > time as they're initialized (if possible).  There's still lots > of legacy C89 code that defines variables at the beginning of > a function and initializes them much later, but in new C and > C++ code we have the opportunity to follow it. > > Martin Sure, I'm sending updated version of the patch. Martin > > PS For some additional background see DCL19-C in the CERT C > Coding Standard.  A warning that helped find opportunities to > reduce the scope of variables would be quite useful. > >> >> Ready to be installed? >> Thanks, >> Martin >> >> gcc/ChangeLog: >> >> 2019-08-09  Martin Liska  >> >>     PR ipa/91404 >>     * passes.c (order): Remove. >>     (uid_hash_t): Likewise). >>     (remove_cgraph_node_from_order): Remove from set >>     of pointers (cgraph_node *). >>     (insert_cgraph_node_to_order): New. >>     (duplicate_cgraph_node_to_order): New. >>     (do_per_function_toporder): Register all 3 cgraph hooks. >>     Skip removed_nodes now as we know about all of them. >> --- >>   gcc/passes.c | 69 +++++++++++++++++++++++++++++++++------------------- >>   1 file changed, 44 insertions(+), 25 deletions(-) >> >> > --------------44C7E959764D84C6369673F9 Content-Type: text/x-patch; name="0001-Properly-register-dead-cgraph_nodes-in-passes.c.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0001-Properly-register-dead-cgraph_nodes-in-passes.c.patch" Content-length: 4834 >From 1700d3b86e3acd3d6603d9d804f5b0c8938af56e Mon Sep 17 00:00:00 2001 From: Martin Liska Date: Fri, 9 Aug 2019 13:50:31 +0200 Subject: [PATCH] Properly register dead cgraph_nodes in passes.c. gcc/ChangeLog: 2019-08-09 Martin Liska PR ipa/91404 * passes.c (order): Remove. (uid_hash_t): Likewise). (remove_cgraph_node_from_order): Remove from set of pointers (cgraph_node *). (insert_cgraph_node_to_order): New. (duplicate_cgraph_node_to_order): New. (do_per_function_toporder): Register all 3 cgraph hooks. Skip removed_nodes now as we know about all of them. --- gcc/passes.c | 68 +++++++++++++++++++++++++++++++++------------------- 1 file changed, 43 insertions(+), 25 deletions(-) diff --git a/gcc/passes.c b/gcc/passes.c index bd56004d909..f715c67ab65 100644 --- a/gcc/passes.c +++ b/gcc/passes.c @@ -1646,24 +1646,39 @@ do_per_function (void (*callback) (function *, void *data), void *data) } } -/* Because inlining might remove no-longer reachable nodes, we need to - keep the array visible to garbage collector to avoid reading collected - out nodes. */ -static int nnodes; -static GTY ((length ("nnodes"))) cgraph_node **order; - -#define uid_hash_t hash_set > - /* Hook called when NODE is removed and therefore should be excluded from order vector. DATA is a hash set with removed nodes. */ static void remove_cgraph_node_from_order (cgraph_node *node, void *data) { - uid_hash_t *removed_nodes = (uid_hash_t *)data; - removed_nodes->add (node->get_uid ()); + hash_set *removed_nodes = (hash_set *)data; + removed_nodes->add (node); +} + +/* Hook called when NODE is insert and therefore should be + excluded from removed_nodes. DATA is a hash set with removed nodes. */ + +static void +insert_cgraph_node_to_order (cgraph_node *node, void *data) +{ + hash_set *removed_nodes = (hash_set *)data; + removed_nodes->remove (node); } +/* Hook called when NODE is duplicated and therefore should be + excluded from removed_nodes. DATA is a hash set with removed nodes. */ + +static void +duplicate_cgraph_node_to_order (cgraph_node *node, cgraph_node *node2, + void *data) +{ + hash_set *removed_nodes = (hash_set *)data; + gcc_checking_assert (!removed_nodes->contains (node)); + removed_nodes->remove (node2); +} + + /* If we are in IPA mode (i.e., current_function_decl is NULL), call function CALLBACK for every function in the call graph. Otherwise, call CALLBACK on the current function. @@ -1677,26 +1692,30 @@ do_per_function_toporder (void (*callback) (function *, void *data), void *data) callback (cfun, data); else { - cgraph_node_hook_list *hook; - uid_hash_t removed_nodes; - gcc_assert (!order); - order = ggc_vec_alloc (symtab->cgraph_count); + hash_set removed_nodes; + unsigned nnodes = symtab->cgraph_count; + cgraph_node **order = XNEWVEC (cgraph_node *, nnodes); nnodes = ipa_reverse_postorder (order); for (i = nnodes - 1; i >= 0; i--) order[i]->process = 1; - hook = symtab->add_cgraph_removal_hook (remove_cgraph_node_from_order, - &removed_nodes); + cgraph_node_hook_list *removal_hook + = symtab->add_cgraph_removal_hook (remove_cgraph_node_from_order, + &removed_nodes); + cgraph_node_hook_list *insertion_hook + = symtab->add_cgraph_insertion_hook (insert_cgraph_node_to_order, + &removed_nodes); + cgraph_2node_hook_list *duplication_hook + = symtab->add_cgraph_duplication_hook (duplicate_cgraph_node_to_order, + &removed_nodes); for (i = nnodes - 1; i >= 0; i--) { cgraph_node *node = order[i]; /* Function could be inlined and removed as unreachable. */ - if (node == NULL || removed_nodes.contains (node->get_uid ())) + if (node == NULL || removed_nodes.contains (node)) continue; - /* Allow possibly removed nodes to be garbage collected. */ - order[i] = NULL; node->process = 0; if (node->has_gimple_body_p ()) { @@ -1706,11 +1725,12 @@ do_per_function_toporder (void (*callback) (function *, void *data), void *data) pop_cfun (); } } - symtab->remove_cgraph_removal_hook (hook); + symtab->remove_cgraph_removal_hook (removal_hook); + symtab->remove_cgraph_insertion_hook (insertion_hook); + symtab->remove_cgraph_duplication_hook (duplication_hook); + + free (order); } - ggc_free (order); - order = NULL; - nnodes = 0; } /* Helper function to perform function body dump. */ @@ -3046,5 +3066,3 @@ function_called_by_processed_nodes_p (void) } return e != NULL; } - -#include "gt-passes.h" -- 2.22.0 --------------44C7E959764D84C6369673F9--