[PATCH] Properly register dead cgraph_nodes in passes.c.

[PATCH] Properly register dead cgraph_nodes in passes.c.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1092 bytes --]

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  <mliska@suse.cz>

	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(-)



[-- Attachment #2: 0001-Properly-register-dead-cgraph_nodes-in-passes.c.patch --]
[-- Type: text/x-patch, Size: 4131 bytes --]

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<int_hash <int, 0, -1> >
-
 /* 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<cgraph_node *> *removed_nodes = (hash_set<cgraph_node *> *)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<cgraph_node *> *removed_nodes = (hash_set<cgraph_node *> *)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<cgraph_node *> *removed_nodes = (hash_set<cgraph_node *> *)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<cgraph_node *> (symtab->cgraph_count);
+      cgraph_node_hook_list *removal_hook;
+      cgraph_node_hook_list *insertion_hook;
+      cgraph_2node_hook_list *duplication_hook;
+      hash_set<cgraph_node *> 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.  */
@@ -3046,5 +3067,3 @@ function_called_by_processed_nodes_p (void)
     }
   return e != NULL;
 }
-
-#include "gt-passes.h"

Re: [PATCH] Properly register dead cgraph_nodes in passes.c.

[Martin Sebor]

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  <mliska@suse.cz>
> 
> 	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(-)
> 
> 

Re: [PATCH] Properly register dead cgraph_nodes in passes.c.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]

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  <mliska@suse.cz>
>>
>>     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(-)
>>
>>
> 


[-- Attachment #2: 0001-Properly-register-dead-cgraph_nodes-in-passes.c.patch --]
[-- Type: text/x-patch, Size: 4833 bytes --]

From 1700d3b86e3acd3d6603d9d804f5b0c8938af56e Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
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  <mliska@suse.cz>

	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<int_hash <int, 0, -1> >
-
 /* 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<cgraph_node *> *removed_nodes = (hash_set<cgraph_node *> *)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<cgraph_node *> *removed_nodes = (hash_set<cgraph_node *> *)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<cgraph_node *> *removed_nodes = (hash_set<cgraph_node *> *)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<cgraph_node *> (symtab->cgraph_count);
+      hash_set<cgraph_node *> 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

Re: [PATCH] Properly register dead cgraph_nodes in passes.c.

[Jeff Law]

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  <mliska@suse.cz>
> 
> 	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