Re: [PATCH] Properly register dead cgraph_nodes in passes.c.
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. > > 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. So this turns out to fix the kernel build failure my tester was tripping over as well. OK for the trunk. jeff
Re: [PATCH] Properly register dead cgraph_nodes in passes.c.
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(-)
>>
>>
>
>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 (s
Re: [PATCH] Properly register dead cgraph_nodes in passes.c.
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 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(-)
[PATCH] Properly register dead cgraph_nodes in passes.c.
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.
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(-)
diff --git a/gcc/passes.c b/gcc/passes.c
index bd56004d909..934ae0b52ca 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,33 @@ 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);
+ cgraph_node_hook_list *removal_hook;
+ cgraph_node_hook_list *insertion_hook;
+ cgraph_2node_hook_list *duplication_hook;
+ 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);
+ removal_hook
+ = symtab->add_cgraph_removal_hook (remove_cgraph_node_from_order,
+ &removed_nodes);
+ insertion_hook
+ = symtab->add_cgraph_insertion_hook (insert_cgraph_node_to_order,
+ &removed_nodes);
+ 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 +1728,10 @@ 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);
}
- ggc_free (order);
- order = NULL;
- nnodes = 0;
}
/* Helper function to perform function body dump.
