Re: Fix partitioning of aliases

[Richard Guenther <rguenther at suse dot de>]

On Mon, 9 Apr 2012, Jan Hubicka wrote:

> Hi,
> this patch fixes several different ICEs related to handling aliases in WHOPR
> partitioning.  It took me over week debug this, but when variable alias
> is added to a boundary and its destination is not added, we get queue of
> unforutnate events where the destinatoin gets analyzed and added at ltrans time
> resulting in interesting miscompilation seen at Mozilla with some vtables.
> The problem is that constructor won't get streamed when the declaration is
> not in varpool at partitioning time and thus once the variable is re-added
> it has zero constructor.
> Of course the problem manifests itself in various weird ways depending
> on ordering of linker command maing it very difficult to reduce anything.
> 
> While working on this I also noticed that PR 52634 is about related problem
> where aliases are incorectly partitioned into multiple partitions.
> The patch also fixes the varpool ICEs mentioned in the other two PRs.
> I failed to produce testcase version of PR52722 testcase, since it does not
> link now either, but it won't ICE.
> 
> I will commit the patch and wait for some time, but I would like to backport
> it to 4.7, since it solves quite nasty misoptimization problem.

Yeah, it looks fine to me.

> At mainline after this patch i would like to follow with series of cleanups
> and API changes I have in queue for symtab work.

Thanks,
Richard.

> Honza
> 
> 	PR lto/52722
> 	PR lto/51765
> 	PR lto/52634	
> 	* lto-cgraph.c (compute_ltrans_boundary): When alias is in the boundary,
> 	add its target too.
> 	* lto.c (add_references_to_partition): Add also aliased nodes.
> 	(add_cgraph_node_to_partition,
> 	add_varpool_node_to_partition): Work on nodes, not functions/variables;
> 	when adding alias, add also the aliased object.
> 	* gcc.dg/lto/pr52634_1.c: New testcase.
> 	* gcc.dg/lto/pr52634_0.c: New testcase.
> Index: lto-cgraph.c
> ===================================================================
> *** lto-cgraph.c	(revision 185767)
> --- lto-cgraph.c	(working copy)
> *************** compute_ltrans_boundary (struct lto_out_
> *** 799,804 ****
> --- 799,806 ----
>   	  lto_set_varpool_encoder_encode_initializer (varpool_encoder, vnode);
>   	  add_references (encoder, varpool_encoder, &vnode->ref_list);
>   	}
> +       else if (vnode->alias || vnode->alias_of)
> +         add_references (encoder, varpool_encoder, &vnode->ref_list);
>       }
>   
>     /* Go over all the nodes again to include callees that are not in
> Index: lto/lto.c
> ===================================================================
> *** lto/lto.c	(revision 185767)
> --- lto/lto.c	(working copy)
> *************** free_ltrans_partitions (void)
> *** 1444,1450 ****
>     VEC_free (ltrans_partition, heap, ltrans_partitions);
>   }
>   
> ! /* See all references that go to comdat objects and bring them into partition too.  */
>   static void
>   add_references_to_partition (ltrans_partition part, struct ipa_ref_list *refs)
>   {
> --- 1444,1451 ----
>     VEC_free (ltrans_partition, heap, ltrans_partitions);
>   }
>   
> ! /* See all references that go to comdat objects and bring them into partition too.
> !    Also see all aliases of the newly added entry and bring them, too.  */
>   static void
>   add_references_to_partition (ltrans_partition part, struct ipa_ref_list *refs)
>   {
> *************** add_references_to_partition (ltrans_part
> *** 1453,1467 ****
>     for (i = 0; ipa_ref_list_reference_iterate (refs, i, ref); i++)
>       {
>         if (ref->refered_type == IPA_REF_CGRAPH
> ! 	  && DECL_COMDAT (cgraph_function_node (ipa_ref_node (ref), NULL)->decl)
>   	  && !cgraph_node_in_set_p (ipa_ref_node (ref), part->cgraph_set))
>   	add_cgraph_node_to_partition (part, ipa_ref_node (ref));
>         else
>   	if (ref->refered_type == IPA_REF_VARPOOL
> ! 	    && DECL_COMDAT (ipa_ref_varpool_node (ref)->decl)
> ! 	    && !varpool_node_in_set_p (ipa_ref_varpool_node (ref), part->varpool_set))
>   	  add_varpool_node_to_partition (part, ipa_ref_varpool_node (ref));
>       }
>   }
>   
>   /* Worker for add_cgraph_node_to_partition.  */
> --- 1454,1498 ----
>     for (i = 0; ipa_ref_list_reference_iterate (refs, i, ref); i++)
>       {
>         if (ref->refered_type == IPA_REF_CGRAPH
> ! 	  && (DECL_COMDAT (cgraph_function_node (ipa_ref_node (ref),
> ! 			   NULL)->decl)
> ! 	      || (ref->use == IPA_REF_ALIAS
> ! 		  && lookup_attribute
> ! 		       ("weakref", DECL_ATTRIBUTES (ipa_ref_node (ref)->decl))))
>   	  && !cgraph_node_in_set_p (ipa_ref_node (ref), part->cgraph_set))
>   	add_cgraph_node_to_partition (part, ipa_ref_node (ref));
>         else
>   	if (ref->refered_type == IPA_REF_VARPOOL
> ! 	    && (DECL_COMDAT (ipa_ref_varpool_node (ref)->decl)
> ! 	        || (ref->use == IPA_REF_ALIAS
> ! 		    && lookup_attribute
> ! 		         ("weakref",
> ! 			  DECL_ATTRIBUTES (ipa_ref_varpool_node (ref)->decl))))
> ! 	    && !varpool_node_in_set_p (ipa_ref_varpool_node (ref),
> ! 				       part->varpool_set))
>   	  add_varpool_node_to_partition (part, ipa_ref_varpool_node (ref));
>       }
> +   for (i = 0; ipa_ref_list_refering_iterate (refs, i, ref); i++)
> +     {
> +       if (ref->refering_type == IPA_REF_CGRAPH
> + 	  && ref->use == IPA_REF_ALIAS
> + 	  && !cgraph_node_in_set_p (ipa_ref_refering_node (ref),
> + 				    part->cgraph_set)
> + 	  && !lookup_attribute ("weakref",
> + 				DECL_ATTRIBUTES
> + 				  (ipa_ref_refering_node (ref)->decl)))
> + 	add_cgraph_node_to_partition (part, ipa_ref_refering_node (ref));
> +       else
> + 	if (ref->refering_type == IPA_REF_VARPOOL
> + 	    && ref->use == IPA_REF_ALIAS
> + 	    && !varpool_node_in_set_p (ipa_ref_refering_varpool_node (ref),
> + 				       part->varpool_set)
> + 	    && !lookup_attribute ("weakref",
> + 				  DECL_ATTRIBUTES
> + 				    (ipa_ref_refering_varpool_node (ref)->decl)))
> + 	  add_varpool_node_to_partition (part,
> + 					 ipa_ref_refering_varpool_node (ref));
> +     }
>   }
>   
>   /* Worker for add_cgraph_node_to_partition.  */
> *************** add_cgraph_node_to_partition (ltrans_par
> *** 1501,1509 ****
>     cgraph_node_set_iterator csi;
>     struct cgraph_node *n;
>   
> -   /* We always decide on functions, not associated thunks and aliases.  */
> -   node = cgraph_function_node (node, NULL);
> - 
>     /* If NODE is already there, we have nothing to do.  */
>     csi = cgraph_node_set_find (part->cgraph_set, node);
>     if (!csi_end_p (csi))
> --- 1532,1537 ----
> *************** add_cgraph_node_to_partition (ltrans_par
> *** 1522,1528 ****
> --- 1550,1563 ----
>   	&& !cgraph_node_in_set_p (e->callee, part->cgraph_set))
>         add_cgraph_node_to_partition (part, e->callee);
>   
> +   /* The only way to assemble non-weakref alias is to add the aliased object into
> +      the unit.  */
>     add_references_to_partition (part, &node->ref_list);
> +   n = cgraph_function_node (node, NULL);
> +   if (n != node
> +       && !lookup_attribute ("weakref",
> + 			    DECL_ATTRIBUTES (node->decl)))
> +     add_cgraph_node_to_partition (part, n);
>   
>     if (node->same_comdat_group)
>       for (n = node->same_comdat_group; n != node; n = n->same_comdat_group)
> *************** static void
> *** 1535,1542 ****
>   add_varpool_node_to_partition (ltrans_partition part, struct varpool_node *vnode)
>   {
>     varpool_node_set_iterator vsi;
> ! 
> !   vnode = varpool_variable_node (vnode, NULL);
>   
>     /* If NODE is already there, we have nothing to do.  */
>     vsi = varpool_node_set_find (part->varpool_set, vnode);
> --- 1570,1576 ----
>   add_varpool_node_to_partition (ltrans_partition part, struct varpool_node *vnode)
>   {
>     varpool_node_set_iterator vsi;
> !   struct varpool_node *v;
>   
>     /* If NODE is already there, we have nothing to do.  */
>     vsi = varpool_node_set_find (part->varpool_set, vnode);
> *************** add_varpool_node_to_partition (ltrans_pa
> *** 1554,1559 ****
> --- 1588,1601 ----
>       }
>     vnode->aux = (void *)((size_t)vnode->aux + 1);
>   
> +   /* The only way to assemble non-weakref alias is to add the aliased object into
> +      the unit.  */
> +   v = varpool_variable_node (vnode, NULL);
> +   if (v != vnode
> +       && !lookup_attribute ("weakref",
> + 			    DECL_ATTRIBUTES (vnode->decl)))
> +     add_varpool_node_to_partition (part, v);
> + 
>     add_references_to_partition (part, &vnode->ref_list);
>   
>     if (vnode->same_comdat_group
> Index: testsuite/gcc.dg/lto/pr52634_1.c
> ===================================================================
> --- testsuite/gcc.dg/lto/pr52634_1.c	(revision 0)
> +++ testsuite/gcc.dg/lto/pr52634_1.c	(revision 0)
> @@ -0,0 +1,2 @@
> +int cfliteKeyCallBacks = 5;
> +extern int cfliteValueCallBacks __attribute__((alias("cfliteKeyCallBacks")));
> Index: testsuite/gcc.dg/lto/pr52634_0.c
> ===================================================================
> --- testsuite/gcc.dg/lto/pr52634_0.c	(revision 0)
> +++ testsuite/gcc.dg/lto/pr52634_0.c	(revision 0)
> @@ -0,0 +1,5 @@
> +/* { dg-lto-do link } */
> +/* { dg-lto-options {{-flto -r -nostdlib -flto-partition=1to1}} */
> +extern int cfliteValueCallBacks;
> +void baz (int *);
> +int main () { baz(&cfliteValueCallBacks); }
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer