| Message ID | 20120409191854.GA8966@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
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); } > >
On Mon, Apr 9, 2012 at 12:18 PM, Jan Hubicka <hubicka@ucw.cz> 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. > At mainline after this patch i would like to follow with series of cleanups > and API changes I have in queue for symtab work. > > 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. Hi Jan, Can you backport it to 4.7? It also fixes: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53768 Thanks.
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); }
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. At mainline after this patch i would like to follow with series of cleanups and API changes I have in queue for symtab work. 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