Fix IPA flattening ICE (PR ipa/82801)

Message ID 20171212185758.GX2353@tucnak
State New
Headers show
Series
  • Fix IPA flattening ICE (PR ipa/82801)
Related show

Commit Message

Jakub Jelinek Dec. 12, 2017, 6:57 p.m.
Hi!

We ICE on the following testcase, because we first gather all cgraph nodes
in an array and then we walk it backwards and flatten_function anything
that has "flatten" attribute.  But flatten_function can result in cgraph
node removal and so we then try to dereference removed cgraph nodes.

From the array we are only interested in nodes with flatten attribute, so
the patch ignores all other nodes (thus the common case of no flatten
attributes in the TU is fast) by keeping them at the end of the order array
only, and if we have at least 2 nodes to flatten, we additionally register
a removal hook just in case a flatten node would be removed (the testcase
has a non-flatten node removed only).  

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2017-12-12  Jakub Jelinek  <jakub@redhat.com>

	PR ipa/82801
	* ipa-inline.c (flatten_remove_node_hook): New function.
	(ipa_inline): Keep only nodes with flatten attribute at the end of
	the array in the order from ipa_reverse_postorder, only walk that
	portion of array for flattening, if there is more than one such
	node, temporarily register a removal hook and ignore removed nodes.

	* g++.dg/ipa/pr82801.C: New test.


	Jakub

Comments

Andi Kleen Dec. 12, 2017, 8:11 p.m. | #1
Jakub Jelinek <jakub@redhat.com> writes:

> Hi!

>

> We ICE on the following testcase, because we first gather all cgraph nodes

> in an array and then we walk it backwards and flatten_function anything

> that has "flatten" attribute.  But flatten_function can result in cgraph

> node removal and so we then try to dereference removed cgraph nodes.


I wonder if this fixes PR83346 too?

This is also about a flatten crash.

-Andi
Jakub Jelinek Dec. 12, 2017, 8:26 p.m. | #2
On Tue, Dec 12, 2017 at 12:11:05PM -0800, Andi Kleen wrote:
> Jakub Jelinek <jakub@redhat.com> writes:

> 

> > Hi!

> >

> > We ICE on the following testcase, because we first gather all cgraph nodes

> > in an array and then we walk it backwards and flatten_function anything

> > that has "flatten" attribute.  But flatten_function can result in cgraph

> > node removal and so we then try to dereference removed cgraph nodes.

> 

> I wonder if this fixes PR83346 too?


Yes, it does.

	Jakub
Jeff Law Dec. 19, 2017, 4:48 a.m. | #3
On 12/12/2017 11:57 AM, Jakub Jelinek wrote:
> Hi!

> 

> We ICE on the following testcase, because we first gather all cgraph nodes

> in an array and then we walk it backwards and flatten_function anything

> that has "flatten" attribute.  But flatten_function can result in cgraph

> node removal and so we then try to dereference removed cgraph nodes.

> 

> From the array we are only interested in nodes with flatten attribute, so

> the patch ignores all other nodes (thus the common case of no flatten

> attributes in the TU is fast) by keeping them at the end of the order array

> only, and if we have at least 2 nodes to flatten, we additionally register

> a removal hook just in case a flatten node would be removed (the testcase

> has a non-flatten node removed only).  

> 

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

> 

> 2017-12-12  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR ipa/82801

> 	* ipa-inline.c (flatten_remove_node_hook): New function.

> 	(ipa_inline): Keep only nodes with flatten attribute at the end of

> 	the array in the order from ipa_reverse_postorder, only walk that

> 	portion of array for flattening, if there is more than one such

> 	node, temporarily register a removal hook and ignore removed nodes.

> 

> 	* g++.dg/ipa/pr82801.C: New test.

OK.  Make sure to reference 83346 in the ChangeLog entry.

jeff

Patch

--- gcc/ipa-inline.c.jj	2017-12-05 10:21:18.000000000 +0100
+++ gcc/ipa-inline.c	2017-12-12 11:40:37.396036390 +0100
@@ -2338,6 +2338,19 @@  dump_inline_stats (void)
 	       (int) reason[i][1], reason_freq[i].to_double (), reason[i][0]);
 }
 
+/* Called when node is removed.  */
+
+static void
+flatten_remove_node_hook (struct cgraph_node *node, void *data)
+{
+  if (lookup_attribute ("flatten", DECL_ATTRIBUTES (node->decl)) == NULL)
+    return;
+
+  hash_set<struct cgraph_node *> *removed
+    = (hash_set<struct cgraph_node *> *) data;
+  removed->add (node);
+}
+
 /* Decide on the inlining.  We do so in the topological order to avoid
    expenses on updating data structures.  */
 
@@ -2347,7 +2360,7 @@  ipa_inline (void)
   struct cgraph_node *node;
   int nnodes;
   struct cgraph_node **order;
-  int i;
+  int i, j;
   int cold;
   bool remove_functions = false;
 
@@ -2380,26 +2393,56 @@  ipa_inline (void)
   if (dump_file)
     fprintf (dump_file, "\nFlattening functions:\n");
 
+  /* First shrink order array, so that it only contains nodes with
+     flatten attribute.  */
+  for (i = nnodes - 1, j = i; i >= 0; i--)
+    {
+      node = order[i];
+      if (lookup_attribute ("flatten",
+			    DECL_ATTRIBUTES (node->decl)) != NULL)
+	order[j--] = order[i];
+    }
+
+  /* After this loop, order[j + 1] ... order[nnodes - 1] contain
+     nodes with flatten attribute.  If there is more than one such
+     node, we need to register a node removal hook, as flatten_function
+     could remove other nodes with flatten attribute.  See PR82801.  */
+  struct cgraph_node_hook_list *node_removal_hook_holder = NULL;
+  hash_set<struct cgraph_node *> *flatten_removed_nodes = NULL;
+  if (j < nnodes - 2)
+    {
+      flatten_removed_nodes = new hash_set<struct cgraph_node *>;
+      node_removal_hook_holder
+	= symtab->add_cgraph_removal_hook (&flatten_remove_node_hook,
+					   flatten_removed_nodes);
+    }
+
   /* In the first pass handle functions to be flattened.  Do this with
      a priority so none of our later choices will make this impossible.  */
-  for (i = nnodes - 1; i >= 0; i--)
+  for (i = nnodes - 1; i > j; i--)
     {
       node = order[i];
+      if (flatten_removed_nodes
+	  && flatten_removed_nodes->contains (node))
+	continue;
 
       /* Handle nodes to be flattened.
 	 Ideally when processing callees we stop inlining at the
 	 entry of cycles, possibly cloning that entry point and
 	 try to flatten itself turning it into a self-recursive
 	 function.  */
-      if (lookup_attribute ("flatten",
-			    DECL_ATTRIBUTES (node->decl)) != NULL)
-	{
-	  if (dump_file)
-	    fprintf (dump_file,
-		     "Flattening %s\n", node->name ());
-	  flatten_function (node, false);
-	}
+      if (dump_file)
+	fprintf (dump_file, "Flattening %s\n", node->name ());
+      flatten_function (node, false);
     }
+
+  if (j < nnodes - 2)
+    {
+      symtab->remove_cgraph_removal_hook (node_removal_hook_holder);
+      delete flatten_removed_nodes;
+    }
+  free (order);
+
   if (dump_file)
     dump_overall_stats ();
 
@@ -2411,7 +2454,6 @@  ipa_inline (void)
      inline functions and virtual functions so we really know what is called
      once.  */
   symtab->remove_unreachable_nodes (dump_file);
-  free (order);
 
   /* Inline functions with a property that after inlining into all callers the
      code size will shrink because the out-of-line copy is eliminated. 
--- gcc/testsuite/g++.dg/ipa/pr82801.C.jj	2017-12-12 11:55:56.879092669 +0100
+++ gcc/testsuite/g++.dg/ipa/pr82801.C	2017-12-12 11:52:36.000000000 +0100
@@ -0,0 +1,20 @@ 
+// PR ipa/82801
+// { dg-do compile }
+// { dg-options "-O2 -Wno-attributes" }
+
+template<int>
+struct A { A () {} };
+struct B { double foo () const; };
+
+__attribute__((always_inline, flatten))
+double B::foo () const
+{
+  A<1> v;
+  return 0.0;
+}
+
+int
+main ()
+{
+  return 0;
+}