PR testsuite/45621 (indirect inlining related cgraph verifier ICE)

PR testsuite/45621 (indirect inlining related cgraph verifier ICE)

[Jan Hubicka]

Hi,
the testcase causes inliner confussion when folding statement that is call to a clone
of virtual method.  It thinks that folding folded call of builtin and re-creates an edge.
This is wrong since inline plan is lost and we then run into verifier ICE.

The patch makes inliner more cureful about deciding when is an original call and what
is new call introduced by builtin folding.

Bootstrapped/regtested x86_64-linux, comitted.
Index: ChangeLog
===================================================================
*** ChangeLog	(revision 165491)
--- ChangeLog	(working copy)
***************
*** 1,3 ****
--- 1,12 ----
+ 2010-10-14  Jan Hubicka  <jh@suse.cz>
+ 
+ 	PR middle-end/45621
+ 	* cgraph.c (cgraph_update_edges_for_call_stmt_node): When new call is
+ 	redirected to clone, be happy.
+ 	* cgraph.h (cgraph node): Enable former_clone_of unconditinally.
+ 	* cgraphunit.c (verify_cgraph_node, cgraph_materialize_clone): Handle
+ 	former_clone_of unconditinally.
+ 	
  2010-10-14  Iain Sandoe  <iains@gcc.gnu.org>
  
  	merge from FSF apple 'trunk' branch. 
Index: testsuite/ChangeLog
===================================================================
*** testsuite/ChangeLog	(revision 165491)
--- testsuite/ChangeLog	(working copy)
***************
*** 1,3 ****
--- 1,10 ----
+ 2010-10-14  Jan Hubicka  <jh@suse.cz>
+ 
+ 	PR middle-end/45621
+ 	* g++.dg/lto/pr45621.h : New.
+ 	* g++.dg/lto/pr45621_0.C: New.
+ 	* g++.dg/lto/pr45621_1.C: New.
+ 
  2010-10-14  Iain Sandoe  <iains@gcc.gnu.org>
  
  	* objc.dg/property: New.
Index: cgraph.c
===================================================================
*** cgraph.c	(revision 165491)
--- cgraph.c	(working copy)
*************** cgraph_update_edges_for_call_stmt_node (
*** 1241,1249 ****
  	{
  	  /* See if the edge is already there and has the correct callee.  It
  	     might be so because of indirect inlining has already updated
! 	     it.  */
! 	  if (new_call && e->callee && e->callee->decl == new_call)
! 	    return;
  
  	  /* Otherwise remove edge and create new one; we can't simply redirect
  	     since function has changed, so inline plan and other information
--- 1241,1258 ----
  	{
  	  /* See if the edge is already there and has the correct callee.  It
  	     might be so because of indirect inlining has already updated
! 	     it.  We also might've cloned and redirected the edge.  */
! 	  if (new_call && e->callee)
! 	    {
! 	      struct cgraph_node *callee = e->callee;
! 	      while (callee)
! 		{
! 		  if (callee->decl == new_call
! 		      || callee->former_clone_of == new_call)
! 		    return;
! 		  callee = callee->clone_of;
! 		}
! 	    }
  
  	  /* Otherwise remove edge and create new one; we can't simply redirect
  	     since function has changed, so inline plan and other information
Index: cgraph.h
===================================================================
*** cgraph.h	(revision 165491)
--- cgraph.h	(working copy)
*************** struct GTY((chain_next ("%h.next"), chai
*** 227,237 ****
    /* For functions with many calls sites it holds map from call expression
       to the edge to speed up cgraph_edge function.  */
    htab_t GTY((param_is (struct cgraph_edge))) call_site_hash;
! #ifdef ENABLE_CHECKING
!   /* Declaration node used to be clone of.  Used for checking only. 
!      We must skip it or we get references from release checking GGC files. */
!   tree GTY ((skip)) former_clone_of;
! #endif
  
    PTR GTY ((skip)) aux;
  
--- 227,234 ----
    /* For functions with many calls sites it holds map from call expression
       to the edge to speed up cgraph_edge function.  */
    htab_t GTY((param_is (struct cgraph_edge))) call_site_hash;
!   /* Declaration node used to be clone of. */
!   tree former_clone_of;
  
    PTR GTY ((skip)) aux;
  
Index: cgraphunit.c
===================================================================
*** cgraphunit.c	(revision 165491)
--- cgraphunit.c	(working copy)
*************** verify_cgraph_node (struct cgraph_node *
*** 656,662 ****
  				debug_tree (e->callee->decl);
  				error_found = true;
  			      }
- #ifdef ENABLE_CHECKING
  			    else if (!e->callee->global.inlined_to
  				     && decl
  				     && cgraph_get_node (decl)
--- 656,661 ----
*************** verify_cgraph_node (struct cgraph_node *
*** 671,677 ****
  				debug_tree (decl);
  				error_found = true;
  			      }
- #endif
  			  }
  			else if (decl)
  			  {
--- 670,675 ----
*************** static void
*** 2079,2089 ****
  cgraph_materialize_clone (struct cgraph_node *node)
  {
    bitmap_obstack_initialize (NULL);
- #ifdef ENABLE_CHECKING
    node->former_clone_of = node->clone_of->decl;
    if (node->clone_of->former_clone_of)
      node->former_clone_of = node->clone_of->former_clone_of;
- #endif
    /* Copy the OLD_VERSION_NODE function tree to the new version.  */
    tree_function_versioning (node->clone_of->decl, node->decl,
    			    node->clone.tree_map, true,
--- 2077,2085 ----
Index: testsuite/g++.dg/lto/pr45621.h
===================================================================
*** testsuite/g++.dg/lto/pr45621.h	(revision 0)
--- testsuite/g++.dg/lto/pr45621.h	(revision 0)
***************
*** 0 ****
--- 1,8 ----
+ struct S
+ {
+   void m ();
+   virtual void v1 ();
+   virtual void v2 ();
+ };
+ 
+ extern S s;
Index: testsuite/g++.dg/lto/pr45621_0.C
===================================================================
*** testsuite/g++.dg/lto/pr45621_0.C	(revision 0)
--- testsuite/g++.dg/lto/pr45621_0.C	(revision 0)
***************
*** 0 ****
--- 1,10 ----
+ // { dg-lto-do assemble }
+ // { dg-extra-ld-options "-O2 -fipa-cp-clone -flto -nostdlib -r" }
+ #include "pr45621.h"
+ 
+ void
+ foo ()
+ {
+   s.v1 ();
+   s.m ();
+ }
Index: testsuite/g++.dg/lto/pr45621_1.C
===================================================================
*** testsuite/g++.dg/lto/pr45621_1.C	(revision 0)
--- testsuite/g++.dg/lto/pr45621_1.C	(revision 0)
***************
*** 0 ****
--- 1,13 ----
+ #include "pr45621.h"
+ 
+ void
+ S::v1 ()
+ {
+   v2 ();
+ }
+ 
+ void
+ S::m ()
+ {
+   v1 ();
+ }

Re: PR testsuite/45621 (indirect inlining related cgraph verifier ICE)

[Michael Matz]

Hello,

On Fri, 15 Oct 2010, Jan Hubicka wrote:

> + 2010-10-14  Jan Hubicka  <jh@suse.cz>
> + 
> + 	PR middle-end/45621
> + 	* cgraph.c (cgraph_update_edges_for_call_stmt_node): When new call is
> + 	redirected to clone, be happy.
> + 	* cgraph.h (cgraph node): Enable former_clone_of unconditinally.
> + 	* cgraphunit.c (verify_cgraph_node, cgraph_materialize_clone): Handle
> + 	former_clone_of unconditinally.

With disable-checking this results in:

libbackend.a(cgraphunit.o): In function `verify_cgraph_node':
/gcc/spec/sb-frescobaldi.arch.suse.de-head-64/gcc/gcc/cgraphunit.c:659: 
undefined reference to `clone_of_p'

for which I'm checking in the below as obvious.


Ciao,
Michael.
-- 
	* cgraphunit.c (clone_of_p): Define unconditionally.

Index: cgraphunit.c
===================================================================
--- cgraphunit.c        (revision 165503)
+++ cgraphunit.c        (working copy)
@@ -396,7 +396,6 @@ cgraph_mark_if_needed (tree decl)
     cgraph_mark_needed_node (node);
 }

-#ifdef ENABLE_CHECKING
 /* Return TRUE if NODE2 is equivalent to NODE or its clone.  */
 static bool
 clone_of_p (struct cgraph_node *node, struct cgraph_node *node2)
@@ -405,7 +404,6 @@ clone_of_p (struct cgraph_node *node, st
     node2 = node2->clone_of;
   return node2 != NULL;
 }
-#endif

 /* Verify edge E count and frequency.  */

Re: PR testsuite/45621 (indirect inlining related cgraph verifier ICE)

[Jan Hubicka]

> Hello,
> 
> On Fri, 15 Oct 2010, Jan Hubicka wrote:
> 
> > + 2010-10-14  Jan Hubicka  <jh@suse.cz>
> > + 
> > + 	PR middle-end/45621
> > + 	* cgraph.c (cgraph_update_edges_for_call_stmt_node): When new call is
> > + 	redirected to clone, be happy.
> > + 	* cgraph.h (cgraph node): Enable former_clone_of unconditinally.
> > + 	* cgraphunit.c (verify_cgraph_node, cgraph_materialize_clone): Handle
> > + 	former_clone_of unconditinally.
> 
> With disable-checking this results in:
> 
> libbackend.a(cgraphunit.o): In function `verify_cgraph_node':
> /gcc/spec/sb-frescobaldi.arch.suse.de-head-64/gcc/gcc/cgraphunit.c:659: 
> undefined reference to `clone_of_p'
> 
> for which I'm checking in the below as obvious.

Ah, thanks.  Must've missed the hunk when separating patch out of my tree.

Honza
> 
> 
> Ciao,
> Michael.
> -- 
> 	* cgraphunit.c (clone_of_p): Define unconditionally.
> 
> Index: cgraphunit.c
> ===================================================================
> --- cgraphunit.c        (revision 165503)
> +++ cgraphunit.c        (working copy)
> @@ -396,7 +396,6 @@ cgraph_mark_if_needed (tree decl)
>      cgraph_mark_needed_node (node);
>  }
> 
> -#ifdef ENABLE_CHECKING
>  /* Return TRUE if NODE2 is equivalent to NODE or its clone.  */
>  static bool
>  clone_of_p (struct cgraph_node *node, struct cgraph_node *node2)
> @@ -405,7 +404,6 @@ clone_of_p (struct cgraph_node *node, st
>      node2 = node2->clone_of;
>    return node2 != NULL;
>  }
> -#endif
> 
>  /* Verify edge E count and frequency.  */
> 

Re: PR testsuite/45621 (indirect inlining related cgraph verifier ICE)

[Martin Jambor]

Hi,

On Fri, Oct 15, 2010 at 03:23:25AM +0200, Jan Hubicka wrote:
> Hi,
> the testcase causes inliner confussion when folding statement that is call to a clone
> of virtual method.  It thinks that folding folded call of builtin and re-creates an edge.
> This is wrong since inline plan is lost and we then run into verifier ICE.
> 
> The patch makes inliner more cureful about deciding when is an original call and what
> is new call introduced by builtin folding.
> 
> Bootstrapped/regtested x86_64-linux, comitted.
> Index: ChangeLog
> ===================================================================
> *** ChangeLog	(revision 165491)
> --- ChangeLog	(working copy)
> ***************
> *** 1,3 ****
> --- 1,12 ----
> + 2010-10-14  Jan Hubicka  <jh@suse.cz>
> + 
> + 	PR middle-end/45621
> + 	* cgraph.c (cgraph_update_edges_for_call_stmt_node): When new call is
> + 	redirected to clone, be happy.
> + 	* cgraph.h (cgraph node): Enable former_clone_of unconditinally.
> + 	* cgraphunit.c (verify_cgraph_node, cgraph_materialize_clone): Handle
> + 	former_clone_of unconditinally.
> + 	

I have just noticed that we still set former_clone_of in
cgraph_remove_unreachable_nodes only when checking is enabled:

#ifdef ENABLE_CHECKING
		      if (node->clone_of)
			node->former_clone_of = node->clone_of->decl;
#endif

That does not sound right if there are more uses of former_clone_of
chan checking now.  Should we remove that #ifdef?

Thanks,

Martin


>   2010-10-14  Iain Sandoe  <iains@gcc.gnu.org>
>   
>   	merge from FSF apple 'trunk' branch. 
> Index: testsuite/ChangeLog
> ===================================================================
> *** testsuite/ChangeLog	(revision 165491)
> --- testsuite/ChangeLog	(working copy)
> ***************
> *** 1,3 ****
> --- 1,10 ----
> + 2010-10-14  Jan Hubicka  <jh@suse.cz>
> + 
> + 	PR middle-end/45621
> + 	* g++.dg/lto/pr45621.h : New.
> + 	* g++.dg/lto/pr45621_0.C: New.
> + 	* g++.dg/lto/pr45621_1.C: New.
> + 
>   2010-10-14  Iain Sandoe  <iains@gcc.gnu.org>
>   
>   	* objc.dg/property: New.
> Index: cgraph.c
> ===================================================================
> *** cgraph.c	(revision 165491)
> --- cgraph.c	(working copy)
> *************** cgraph_update_edges_for_call_stmt_node (
> *** 1241,1249 ****
>   	{
>   	  /* See if the edge is already there and has the correct callee.  It
>   	     might be so because of indirect inlining has already updated
> ! 	     it.  */
> ! 	  if (new_call && e->callee && e->callee->decl == new_call)
> ! 	    return;
>   
>   	  /* Otherwise remove edge and create new one; we can't simply redirect
>   	     since function has changed, so inline plan and other information
> --- 1241,1258 ----
>   	{
>   	  /* See if the edge is already there and has the correct callee.  It
>   	     might be so because of indirect inlining has already updated
> ! 	     it.  We also might've cloned and redirected the edge.  */
> ! 	  if (new_call && e->callee)
> ! 	    {
> ! 	      struct cgraph_node *callee = e->callee;
> ! 	      while (callee)
> ! 		{
> ! 		  if (callee->decl == new_call
> ! 		      || callee->former_clone_of == new_call)
> ! 		    return;
> ! 		  callee = callee->clone_of;
> ! 		}
> ! 	    }
>   
>   	  /* Otherwise remove edge and create new one; we can't simply redirect
>   	     since function has changed, so inline plan and other information
> Index: cgraph.h
> ===================================================================
> *** cgraph.h	(revision 165491)
> --- cgraph.h	(working copy)
> *************** struct GTY((chain_next ("%h.next"), chai
> *** 227,237 ****
>     /* For functions with many calls sites it holds map from call expression
>        to the edge to speed up cgraph_edge function.  */
>     htab_t GTY((param_is (struct cgraph_edge))) call_site_hash;
> ! #ifdef ENABLE_CHECKING
> !   /* Declaration node used to be clone of.  Used for checking only. 
> !      We must skip it or we get references from release checking GGC files. */
> !   tree GTY ((skip)) former_clone_of;
> ! #endif
>   
>     PTR GTY ((skip)) aux;
>   
> --- 227,234 ----
>     /* For functions with many calls sites it holds map from call expression
>        to the edge to speed up cgraph_edge function.  */
>     htab_t GTY((param_is (struct cgraph_edge))) call_site_hash;
> !   /* Declaration node used to be clone of. */
> !   tree former_clone_of;
>   
>     PTR GTY ((skip)) aux;
>   
> Index: cgraphunit.c
> ===================================================================
> *** cgraphunit.c	(revision 165491)
> --- cgraphunit.c	(working copy)
> *************** verify_cgraph_node (struct cgraph_node *
> *** 656,662 ****
>   				debug_tree (e->callee->decl);
>   				error_found = true;
>   			      }
> - #ifdef ENABLE_CHECKING
>   			    else if (!e->callee->global.inlined_to
>   				     && decl
>   				     && cgraph_get_node (decl)
> --- 656,661 ----
> *************** verify_cgraph_node (struct cgraph_node *
> *** 671,677 ****
>   				debug_tree (decl);
>   				error_found = true;
>   			      }
> - #endif
>   			  }
>   			else if (decl)
>   			  {
> --- 670,675 ----
> *************** static void
> *** 2079,2089 ****
>   cgraph_materialize_clone (struct cgraph_node *node)
>   {
>     bitmap_obstack_initialize (NULL);
> - #ifdef ENABLE_CHECKING
>     node->former_clone_of = node->clone_of->decl;
>     if (node->clone_of->former_clone_of)
>       node->former_clone_of = node->clone_of->former_clone_of;
> - #endif
>     /* Copy the OLD_VERSION_NODE function tree to the new version.  */
>     tree_function_versioning (node->clone_of->decl, node->decl,
>     			    node->clone.tree_map, true,
> --- 2077,2085 ----
> Index: testsuite/g++.dg/lto/pr45621.h
> ===================================================================
> *** testsuite/g++.dg/lto/pr45621.h	(revision 0)
> --- testsuite/g++.dg/lto/pr45621.h	(revision 0)
> ***************
> *** 0 ****
> --- 1,8 ----
> + struct S
> + {
> +   void m ();
> +   virtual void v1 ();
> +   virtual void v2 ();
> + };
> + 
> + extern S s;
> Index: testsuite/g++.dg/lto/pr45621_0.C
> ===================================================================
> *** testsuite/g++.dg/lto/pr45621_0.C	(revision 0)
> --- testsuite/g++.dg/lto/pr45621_0.C	(revision 0)
> ***************
> *** 0 ****
> --- 1,10 ----
> + // { dg-lto-do assemble }
> + // { dg-extra-ld-options "-O2 -fipa-cp-clone -flto -nostdlib -r" }
> + #include "pr45621.h"
> + 
> + void
> + foo ()
> + {
> +   s.v1 ();
> +   s.m ();
> + }
> Index: testsuite/g++.dg/lto/pr45621_1.C
> ===================================================================
> *** testsuite/g++.dg/lto/pr45621_1.C	(revision 0)
> --- testsuite/g++.dg/lto/pr45621_1.C	(revision 0)
> ***************
> *** 0 ****
> --- 1,13 ----
> + #include "pr45621.h"
> + 
> + void
> + S::v1 ()
> + {
> +   v2 ();
> + }
> + 
> + void
> + S::m ()
> + {
> +   v1 ();
> + }

Re: PR testsuite/45621 (indirect inlining related cgraph verifier ICE)

[Jan Hubicka]

> Hi,
> 
> On Fri, Oct 15, 2010 at 03:23:25AM +0200, Jan Hubicka wrote:
> > Hi,
> > the testcase causes inliner confussion when folding statement that is call to a clone
> > of virtual method.  It thinks that folding folded call of builtin and re-creates an edge.
> > This is wrong since inline plan is lost and we then run into verifier ICE.
> > 
> > The patch makes inliner more cureful about deciding when is an original call and what
> > is new call introduced by builtin folding.
> > 
> > Bootstrapped/regtested x86_64-linux, comitted.
> > Index: ChangeLog
> > ===================================================================
> > *** ChangeLog	(revision 165491)
> > --- ChangeLog	(working copy)
> > ***************
> > *** 1,3 ****
> > --- 1,12 ----
> > + 2010-10-14  Jan Hubicka  <jh@suse.cz>
> > + 
> > + 	PR middle-end/45621
> > + 	* cgraph.c (cgraph_update_edges_for_call_stmt_node): When new call is
> > + 	redirected to clone, be happy.
> > + 	* cgraph.h (cgraph node): Enable former_clone_of unconditinally.
> > + 	* cgraphunit.c (verify_cgraph_node, cgraph_materialize_clone): Handle
> > + 	former_clone_of unconditinally.
> > + 	
> 
> I have just noticed that we still set former_clone_of in
> cgraph_remove_unreachable_nodes only when checking is enabled:
> 
> #ifdef ENABLE_CHECKING
> 		      if (node->clone_of)
> 			node->former_clone_of = node->clone_of->decl;
> #endif
> 
> That does not sound right if there are more uses of former_clone_of
> chan checking now.  Should we remove that #ifdef?

Ah yes, please do.
It should not matter here as we will not remove the original before materialization, but
it is good idea to keep datastructures consistent.

Honza

Re: PR testsuite/45621 (indirect inlining related cgraph verifier ICE)

[Martin Jambor]

Hi,

On Thu, Dec 16, 2010 at 06:37:04PM +0100, Jan Hubicka wrote:
> > 
> > I have just noticed that we still set former_clone_of in
> > cgraph_remove_unreachable_nodes only when checking is enabled:
> > 
> > #ifdef ENABLE_CHECKING
> > 		      if (node->clone_of)
> > 			node->former_clone_of = node->clone_of->decl;
> > #endif
> > 
> > That does not sound right if there are more uses of former_clone_of
> > chan checking now.  Should we remove that #ifdef?
> 
> Ah yes, please do.  It should not matter here as we will not remove
> the original before materialization, but it is good idea to keep
> datastructures consistent.
> 

OK, I have just committed the simple patch below as revision 168209
after the usual bootstrap and testing on x86_64-linux.

Thanks

2010-12-23  Martin Jambor  <mjambor@suse.cz>

	* ipa.c (cgraph_remove_unreachable_nodes): Update former_clone_of even
	when not checking.

Index: icln/gcc/ipa.c
===================================================================
--- icln.orig/gcc/ipa.c
+++ icln/gcc/ipa.c
@@ -428,10 +428,8 @@ cgraph_remove_unreachable_nodes (bool be
 			node->clone_of->clones = node->next_sibling_clone;
 		      if (node->next_sibling_clone)
 			node->next_sibling_clone->prev_sibling_clone = node->prev_sibling_clone;
-#ifdef ENABLE_CHECKING
 		      if (node->clone_of)
 			node->former_clone_of = node->clone_of->decl;
-#endif
 		      node->clone_of = NULL;
 		      node->next_sibling_clone = NULL;
 		      node->prev_sibling_clone = NULL;