Patchwork lto-symtab and cgraph aliases

login
register
mail settings
Submitter Jan Hubicka
Date July 5, 2010, 7:55 p.m.
Message ID <20100705195535.GB12132@kam.mff.cuni.cz>
Download mbox | patch
Permalink /patch/57927/
State New
Headers show

Comments

Jan Hubicka - July 5, 2010, 7:55 p.m.
Hi,
While compiling Mozilla, we end up with ICE becuase we replace thunk

for _ZN7nanojit9LirWriterD1Ev by ordinary node. WHile merging in
unmerged cgraph we get couple
__base_dtor /9369(-1) @0x7f2f66f8c2b0 (asm: _ZN7nanojit9LirWriterD2Ev) analyzed 1 time, 12 benefit 1 size, 3 benefit address_taken externally_visible finalized inlinable
  called by:
  calls:
  References:  var:_ZTVN7nanojit9LirWriterE (addr)
  Refering this function:  var:_ZTVN7nanojit9LirWriterE (addr) var:_ZTVN7nanojit9LirWriterE (addr) var:_ZTVN7nanojit9LirWriterE (addr)
  aliases & thunks: __comp_dtor /9370 (asm: _ZN7nanojit9LirWriterD1Ev)

We end p removing all and producing new cgraph node:

__comp_dtor /0(-1) @0x7f2f674f2408 (asm: _ZN7nanojit9LirWriterD1Ev) availability:not_available reachable
  called by:
  calls:
  References:
  Refering this function:  var:_ZTVN7nanojit9LirWriterE (addr) var:_ZTVN7nanojit9LirWriterE (addr) var:_ZTVN7nanojit9LirWriterE (addr)

That is bogys (it is alias node put into main linked list of cgraph nodes,
see UID of 0 etc.)

I ended up reorganizing lto-symtab code since the original did not make much
sense to me.  I also run into problem producing resonable testcase for this.
We need multiple files and the library linked into it. Moreover one needs
linker plugin (otherwise we do not ice because of different prevailing
decisions)

Our current code seems to be build around assumption that aliases will always
be the same (this is not neccesarily true with object files with different ABI
settings) and that we can thus merge en-masse first time we merge two symbols
from the alias list.

Thus entry->node points to function, even for DECLs that are actually thunks
or aliases.  This is different from varpool where entry->vnode points to variable
only for node prepresenting the variable and to alias for aliases.

This patch turns cgraph nodes into same logic as we do for varpool nodes.
entry->node points to node or alias (we need new accestor function for that
cgraph_get_node_or_alias) and when merging symbols we merge the corresponding
nodes and aliases only.

Bootstrapped/regtested x86_64-linux and also tested on whopr bootstrap + mozilla
build. Seems to make sense?
	* lto-symtab.c (lto_cgraph_replace_node): Handle aliases.
Richard Guenther - July 6, 2010, 9:59 a.m.
On Mon, Jul 5, 2010 at 9:55 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> While compiling Mozilla, we end up with ICE becuase we replace thunk
>
> for _ZN7nanojit9LirWriterD1Ev by ordinary node. WHile merging in
> unmerged cgraph we get couple
> __base_dtor /9369(-1) @0x7f2f66f8c2b0 (asm: _ZN7nanojit9LirWriterD2Ev) analyzed 1 time, 12 benefit 1 size, 3 benefit address_taken externally_visible finalized inlinable
>  called by:
>  calls:
>  References:  var:_ZTVN7nanojit9LirWriterE (addr)
>  Refering this function:  var:_ZTVN7nanojit9LirWriterE (addr) var:_ZTVN7nanojit9LirWriterE (addr) var:_ZTVN7nanojit9LirWriterE (addr)
>  aliases & thunks: __comp_dtor /9370 (asm: _ZN7nanojit9LirWriterD1Ev)
>
> We end p removing all and producing new cgraph node:
>
> __comp_dtor /0(-1) @0x7f2f674f2408 (asm: _ZN7nanojit9LirWriterD1Ev) availability:not_available reachable
>  called by:
>  calls:
>  References:
>  Refering this function:  var:_ZTVN7nanojit9LirWriterE (addr) var:_ZTVN7nanojit9LirWriterE (addr) var:_ZTVN7nanojit9LirWriterE (addr)
>
> That is bogys (it is alias node put into main linked list of cgraph nodes,
> see UID of 0 etc.)
>
> I ended up reorganizing lto-symtab code since the original did not make much
> sense to me.  I also run into problem producing resonable testcase for this.
> We need multiple files and the library linked into it. Moreover one needs
> linker plugin (otherwise we do not ice because of different prevailing
> decisions)
>
> Our current code seems to be build around assumption that aliases will always
> be the same (this is not neccesarily true with object files with different ABI
> settings) and that we can thus merge en-masse first time we merge two symbols
> from the alias list.
>
> Thus entry->node points to function, even for DECLs that are actually thunks
> or aliases.  This is different from varpool where entry->vnode points to variable
> only for node prepresenting the variable and to alias for aliases.
>
> This patch turns cgraph nodes into same logic as we do for varpool nodes.
> entry->node points to node or alias (we need new accestor function for that
> cgraph_get_node_or_alias) and when merging symbols we merge the corresponding
> nodes and aliases only.
>
> Bootstrapped/regtested x86_64-linux and also tested on whopr bootstrap + mozilla
> build. Seems to make sense?

Changelog is incomplete (missing cgraph.h change).

Otherwise it makes sense.

Thanks,
Richard.


>        * lto-symtab.c (lto_cgraph_replace_node): Handle aliases.
> Index: lto-symtab.c
> ===================================================================
> --- lto-symtab.c        (revision 161847)
> +++ lto-symtab.c        (working copy)
> @@ -206,6 +206,24 @@ lto_cgraph_replace_node (struct cgraph_n
>                         struct cgraph_node *prevailing_node)
>  {
>   struct cgraph_edge *e, *next;
> +  bool no_aliases_please = false;
> +
> +  if (cgraph_dump_file)
> +    {
> +      fprintf (cgraph_dump_file, "Replacing cgraph node %s/%i by %s/%i"
> +              " for symbol %s\n",
> +              cgraph_node_name (node), node->uid,
> +              cgraph_node_name (prevailing_node),
> +              prevailing_node->uid,
> +              IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (node->decl)));
> +    }
> +
> +  if (prevailing_node->same_body_alias)
> +    {
> +      if (prevailing_node->thunk.thunk_p)
> +       no_aliases_please = true;
> +      prevailing_node = prevailing_node->same_body;
> +    }
>
>   /* Merge node flags.  */
>   if (node->needed)
> @@ -227,27 +244,37 @@ lto_cgraph_replace_node (struct cgraph_n
>   /* Redirect incomming references.  */
>   ipa_clone_refering (prevailing_node, NULL, &node->ref_list);
>
> -  if (node->same_body)
> +  /* If we have aliases, redirect them to the prevailing node.  */
> +  if (!node->same_body_alias && node->same_body)
>     {
> -      struct cgraph_node *alias;
> +      struct cgraph_node *alias, *last;
> +      /* We prevail aliases/tunks by a thunk.  This is doable but
> +         would need thunk combination.  Hopefully no ABI changes will
> +         every be crazy enough.  */
> +      gcc_assert (!no_aliases_please);
>
>       for (alias = node->same_body; alias; alias = alias->next)
> -       if (DECL_ASSEMBLER_NAME_SET_P (alias->decl))
> -         {
> -           lto_symtab_entry_t se
> -             = lto_symtab_get (DECL_ASSEMBLER_NAME (alias->decl));
> -
> -           for (; se; se = se->next)
> -             if (se->node == node)
> -               {
> -                 se->node = NULL;
> -                 break;
> -               }
> -         }
> +       {
> +         last = alias;
> +         gcc_assert (alias->same_body_alias);
> +         alias->same_body = prevailing_node;
> +         alias->thunk.alias = prevailing_node->decl;
> +       }
> +      last->next = prevailing_node->same_body;
> +      /* Node with aliases is prevailed by alias.
> +        We could handle this, but combining thunks together will be tricky.
> +        Hopefully this does not happen.  */
> +      if (prevailing_node->same_body)
> +       prevailing_node->same_body->previous = last;
> +      prevailing_node->same_body = node->same_body;
> +      node->same_body = NULL;
>     }
>
>   /* Finally remove the replaced node.  */
> -  cgraph_remove_node (node);
> +  if (node->same_body_alias)
> +    cgraph_remove_same_body_alias (node);
> +  else
> +    cgraph_remove_node (node);
>  }
>
>  /* Replace the cgraph node NODE with PREVAILING_NODE in the cgraph, merging
> @@ -433,7 +460,9 @@ lto_symtab_resolve_can_prevail_p (lto_sy
>
>   /* For functions we need a non-discarded body.  */
>   if (TREE_CODE (e->decl) == FUNCTION_DECL)
> -    return (e->node && e->node->analyzed);
> +    return (e->node
> +           && (e->node->analyzed
> +               || (e->node->same_body_alias && e->node->same_body->analyzed)));
>
>   /* A variable should have a size.  */
>   else if (TREE_CODE (e->decl) == VAR_DECL)
> @@ -461,7 +490,7 @@ lto_symtab_resolve_symbols (void **slot)
>   for (e = (lto_symtab_entry_t) *slot; e; e = e->next)
>     {
>       if (TREE_CODE (e->decl) == FUNCTION_DECL)
> -       e->node = cgraph_get_node (e->decl);
> +       e->node = cgraph_get_node_or_alias (e->decl);
>       else if (TREE_CODE (e->decl) == VAR_DECL)
>        {
>          e->vnode = varpool_get_node (e->decl);
> @@ -751,22 +780,7 @@ lto_symtab_merge_cgraph_nodes_1 (void **
>   for (e = prevailing->next; e; e = e->next)
>     {
>       if (e->node != NULL)
> -       {
> -         if (e->node->decl != e->decl && e->node->same_body)
> -           {
> -             struct cgraph_node *alias;
> -
> -             for (alias = e->node->same_body; alias; alias = alias->next)
> -               if (alias->decl == e->decl)
> -                 break;
> -             if (alias)
> -               {
> -                 cgraph_remove_same_body_alias (alias);
> -                 continue;
> -               }
> -           }
> -         lto_cgraph_replace_node (e->node, prevailing->node);
> -       }
> +       lto_cgraph_replace_node (e->node, prevailing->node);
>       if (e->vnode != NULL)
>        lto_varpool_replace_node (e->vnode, prevailing->vnode);
>     }
> Index: cgraph.c
> ===================================================================
> --- cgraph.c    (revision 161847)
> +++ cgraph.c    (working copy)
> @@ -604,6 +604,29 @@ cgraph_add_thunk (tree alias, tree decl,
>    is assigned.  */
>
>  struct cgraph_node *
> +cgraph_get_node_or_alias (tree decl)
> +{
> +  struct cgraph_node key, *node = NULL, **slot;
> +
> +  gcc_assert (TREE_CODE (decl) == FUNCTION_DECL);
> +
> +  if (!cgraph_hash)
> +    return NULL;
> +
> +  key.decl = decl;
> +
> +  slot = (struct cgraph_node **) htab_find_slot (cgraph_hash, &key,
> +                                                NO_INSERT);
> +
> +  if (slot && *slot)
> +    node = *slot;
> +  return node;
> +}
> +
> +/* Returns the cgraph node assigned to DECL or NULL if no cgraph node
> +   is assigned.  */
> +
> +struct cgraph_node *
>  cgraph_get_node (tree decl)
>  {
>   struct cgraph_node key, *node = NULL, **slot;
>

Patch

Index: lto-symtab.c
===================================================================
--- lto-symtab.c	(revision 161847)
+++ lto-symtab.c	(working copy)
@@ -206,6 +206,24 @@  lto_cgraph_replace_node (struct cgraph_n
 			 struct cgraph_node *prevailing_node)
 {
   struct cgraph_edge *e, *next;
+  bool no_aliases_please = false;
+
+  if (cgraph_dump_file)
+    {
+      fprintf (cgraph_dump_file, "Replacing cgraph node %s/%i by %s/%i"
+ 	       " for symbol %s\n",
+	       cgraph_node_name (node), node->uid,
+	       cgraph_node_name (prevailing_node),
+	       prevailing_node->uid,
+	       IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (node->decl)));
+    }
+
+  if (prevailing_node->same_body_alias)
+    {
+      if (prevailing_node->thunk.thunk_p)
+	no_aliases_please = true;
+      prevailing_node = prevailing_node->same_body;
+    }
 
   /* Merge node flags.  */
   if (node->needed)
@@ -227,27 +244,37 @@  lto_cgraph_replace_node (struct cgraph_n
   /* Redirect incomming references.  */
   ipa_clone_refering (prevailing_node, NULL, &node->ref_list);
 
-  if (node->same_body)
+  /* If we have aliases, redirect them to the prevailing node.  */
+  if (!node->same_body_alias && node->same_body)
     {
-      struct cgraph_node *alias;
+      struct cgraph_node *alias, *last;
+      /* We prevail aliases/tunks by a thunk.  This is doable but
+         would need thunk combination.  Hopefully no ABI changes will
+         every be crazy enough.  */
+      gcc_assert (!no_aliases_please);
 
       for (alias = node->same_body; alias; alias = alias->next)
-	if (DECL_ASSEMBLER_NAME_SET_P (alias->decl))
-	  {
-	    lto_symtab_entry_t se
-	      = lto_symtab_get (DECL_ASSEMBLER_NAME (alias->decl));
-
-	    for (; se; se = se->next)
-	      if (se->node == node)
-		{
-		  se->node = NULL;
-		  break;
-		}
-	  }
+	{
+	  last = alias;
+	  gcc_assert (alias->same_body_alias);
+	  alias->same_body = prevailing_node;
+	  alias->thunk.alias = prevailing_node->decl;
+	}
+      last->next = prevailing_node->same_body;
+      /* Node with aliases is prevailed by alias.
+	 We could handle this, but combining thunks together will be tricky.
+	 Hopefully this does not happen.  */
+      if (prevailing_node->same_body)
+	prevailing_node->same_body->previous = last;
+      prevailing_node->same_body = node->same_body;
+      node->same_body = NULL;
     }
 
   /* Finally remove the replaced node.  */
-  cgraph_remove_node (node);
+  if (node->same_body_alias)
+    cgraph_remove_same_body_alias (node);
+  else
+    cgraph_remove_node (node);
 }
 
 /* Replace the cgraph node NODE with PREVAILING_NODE in the cgraph, merging
@@ -433,7 +460,9 @@  lto_symtab_resolve_can_prevail_p (lto_sy
 
   /* For functions we need a non-discarded body.  */
   if (TREE_CODE (e->decl) == FUNCTION_DECL)
-    return (e->node && e->node->analyzed);
+    return (e->node
+	    && (e->node->analyzed
+	        || (e->node->same_body_alias && e->node->same_body->analyzed)));
 
   /* A variable should have a size.  */
   else if (TREE_CODE (e->decl) == VAR_DECL)
@@ -461,7 +490,7 @@  lto_symtab_resolve_symbols (void **slot)
   for (e = (lto_symtab_entry_t) *slot; e; e = e->next)
     {
       if (TREE_CODE (e->decl) == FUNCTION_DECL)
-	e->node = cgraph_get_node (e->decl);
+	e->node = cgraph_get_node_or_alias (e->decl);
       else if (TREE_CODE (e->decl) == VAR_DECL)
 	{
 	  e->vnode = varpool_get_node (e->decl);
@@ -751,22 +780,7 @@  lto_symtab_merge_cgraph_nodes_1 (void **
   for (e = prevailing->next; e; e = e->next)
     {
       if (e->node != NULL)
-	{
-	  if (e->node->decl != e->decl && e->node->same_body)
-	    {
-	      struct cgraph_node *alias;
-
-	      for (alias = e->node->same_body; alias; alias = alias->next)
-		if (alias->decl == e->decl)
-		  break;
-	      if (alias)
-		{
-		  cgraph_remove_same_body_alias (alias);
-		  continue;
-		}
-	    }
-	  lto_cgraph_replace_node (e->node, prevailing->node);
-	}
+	lto_cgraph_replace_node (e->node, prevailing->node);
       if (e->vnode != NULL)
 	lto_varpool_replace_node (e->vnode, prevailing->vnode);
     }
Index: cgraph.c
===================================================================
--- cgraph.c	(revision 161847)
+++ cgraph.c	(working copy)
@@ -604,6 +604,29 @@  cgraph_add_thunk (tree alias, tree decl,
    is assigned.  */
 
 struct cgraph_node *
+cgraph_get_node_or_alias (tree decl)
+{
+  struct cgraph_node key, *node = NULL, **slot;
+
+  gcc_assert (TREE_CODE (decl) == FUNCTION_DECL);
+
+  if (!cgraph_hash)
+    return NULL;
+
+  key.decl = decl;
+
+  slot = (struct cgraph_node **) htab_find_slot (cgraph_hash, &key,
+						 NO_INSERT);
+
+  if (slot && *slot)
+    node = *slot;
+  return node;
+}
+
+/* Returns the cgraph node assigned to DECL or NULL if no cgraph node
+   is assigned.  */
+
+struct cgraph_node *
 cgraph_get_node (tree decl)
 {
   struct cgraph_node key, *node = NULL, **slot;