Re: [PATCH] Fix IPA flattening ICE (PR ipa/82801)

[Jeff Law]

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

Re: [PATCH] Fix IPA flattening ICE (PR ipa/82801)

[Jakub Jelinek]

On Tue, Dec 12, 2017 at 12:11:05PM -0800, Andi Kleen wrote:
> Jakub Jelinek  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

Re: [PATCH] Fix IPA flattening ICE (PR ipa/82801)

[Andi Kleen]

Jakub Jelinek  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

[PATCH] Fix IPA flattening ICE (PR ipa/82801)

[Jakub Jelinek]

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  

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.

--- gcc/ipa-inline.c.jj 2017-12-05 10:21:18.0 +0100
+++ gcc/ipa-inline.c2017-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 *removed
+= (hash_set *) 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 *flatten_removed_nodes = NULL;
+  if (j < nnodes - 2)
+{
+  flatten_removed_nodes = new hash_set;
+  node_removal_hook_holder
+   = symtab->add_cgraph_removal_hook (_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.0 +0100
@@ -0,0 +1,20 @@
+// PR ipa/82801
+// { dg-do compile }
+// { dg-options