diff mbox

Fix partition WRT static variables taking address of labels

Message ID 20150330073201.GA42058@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka March 30, 2015, 7:32 a.m. UTC 
Hi,
this patch fixes problem with partitioning WRT named labels.  This problem
reproduces with linux kernel LTO and also on firefox with -fno-toplevel-reorder
at least.

Bootstrapped/regtested x86_64-linux and tested it fixes the problem on Firefx,
intend to commit it tomorrow after some more testing on chromium

The patch may need a bit more tweaking WRT nested functions contains nonlocal
adresses taken, but that is even more exotic than the problem I am trying
to solve so I think it can wait for next stage1. This patch should be backportable
to earlier releases after reversing the C++ APi changes.

Honza

	PR lto/50676
	* symtab.c (symtab_node::get_partitioning_class): Local static
	taking named label inherits partitioning property of the function
	it belong to.
	* lto-cgraph.c (lto_output_varpool_node, input_varpool_node): Stream
	contains_named_label.
	* tree.c (decl_address_ip_invariant_p): LABEL_DECL is not IP
	invariant.
	* cgraph.h (varpool_node): Add contains_named_label.
	* cgraphbuild.c (record_reference): Record contains_named_label.
	* lto-partition.c (add_references_to_partition,
	add_symbol_to_partition_1): Local static var should stay in
	same partition as its containing function when it reffer to
	named label.
	(add_sorted_nodes): Skip static var referring named label.
	(lto_balanced_map): Likewise.
	* varpool.c (varpool_node::dump): Dump contains_named_label.
	* ipa.c (symbol_table::remove_unreachable_nodes): Variable
	referring named label keeps its function alive.

	* gcc.dg/lto/named-label.c: New testcase.

Comments

Jan Hubicka March 30, 2015, 9:07 a.m. UTC | #1 
Hi,
unforutnately the patch ICE on:
int i;
struct C
{
  C();
};

C::C()  
{
  static void *labelref = &&label;
  goto *labelref;
 label: i = 1;
}

int main()
{
  C c;
  return (i != 1);
}

The problem is that decl_function_context of labelref is not C::C
itself but an abstract function that is abstract origin of C::C.
Jaosn, I suppose this is an artefact of constructor clonning?
Function like this can not be clonned without duplicating label,
is the decl_function_context really intended to be the abstract one?

Honza
Richard Biener March 30, 2015, 9:32 a.m. UTC | #2 
On Mon, Mar 30, 2015 at 11:07 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> unforutnately the patch ICE on:
> int i;
> struct C
> {
>   C();
> };
>
> C::C()
> {
>   static void *labelref = &&label;
>   goto *labelref;
>  label: i = 1;
> }
>
> int main()
> {
>   C c;
>   return (i != 1);
> }
>
> The problem is that decl_function_context of labelref is not C::C
> itself but an abstract function that is abstract origin of C::C.
> Jaosn, I suppose this is an artefact of constructor clonning?
> Function like this can not be clonned without duplicating label,
> is the decl_function_context really intended to be the abstract one?

IMHO that's bogus.  But I thought we decided that functions with
non-local labels shouldn't be cloned?  ISTR people collecting
such labels in some global array (kernel folks?) and thus definitely
don't expect labels to multiply.  And cloning static vars looks bogus
as well as the addresses of the vars need to compare equal
(return &labelref, call the fn two times, reaching different clones and
abort if the return value isn't equal).

Richard.

> Honza
Jason Merrill March 30, 2015, 2:04 p.m. UTC | #3 
On 03/30/2015 05:32 AM, Richard Biener wrote:
> IMHO that's bogus.  But I thought we decided that functions with
> non-local labels shouldn't be cloned?

Yes, why isn't copy_forbidden flagging this function?  It has a check 
for exactly this situation.

> And cloning static vars looks bogus
> as well as the addresses of the vars need to compare equal

We aren't cloning the static var, that's why its decl_function_context 
is the abstract function.

Jason
Jan Hubicka March 30, 2015, 5:19 p.m. UTC | #4 
> On 03/30/2015 05:32 AM, Richard Biener wrote:
> >IMHO that's bogus.  But I thought we decided that functions with
> >non-local labels shouldn't be cloned?
> 
> Yes, why isn't copy_forbidden flagging this function?  It has a
> check for exactly this situation.

I think the problem is that C++ FE does not ask about copy_forbidden and clone
ctor anyway?

Honza
> 
> >And cloning static vars looks bogus
> >as well as the addresses of the vars need to compare equal
> 
> We aren't cloning the static var, that's why its
> decl_function_context is the abstract function.
> 
> Jason
Jason Merrill March 30, 2015, 6:33 p.m. UTC | #5 
On 03/30/2015 01:19 PM, Jan Hubicka wrote:
> I think the problem is that C++ FE does not ask about copy_forbidden and clone
> ctor anyway?

maybe_clone_body checks tree_versionable_function_p and doesn't clone if 
it's false.

...ok, now I've actually checked the testcase.  In this case there is a 
single clone and an alias, so there is only one version of the label.

Jason
Jan Hubicka March 30, 2015, 6:36 p.m. UTC | #6 
> On 03/30/2015 01:19 PM, Jan Hubicka wrote:
> >I think the problem is that C++ FE does not ask about copy_forbidden and clone
> >ctor anyway?
> 
> maybe_clone_body checks tree_versionable_function_p and doesn't
> clone if it's false.
> 
> ...ok, now I've actually checked the testcase.  In this case there
> is a single clone and an alias, so there is only one version of the
> label.

Will there be only one copy even on targets that do not support aliases?
Still, it would be great if we can make context to be the actual function.
Otherwise I will need to figure out some way to represent the link in between
static variable and function it takes label of...

Thanks!
Honza
> 
> Jason
Jason Merrill March 30, 2015, 6:44 p.m. UTC | #7 
On 03/30/2015 02:36 PM, Jan Hubicka wrote:
> Will there be only one copy even on targets that do not support aliases?

Yes, on those targets we use thunks.

> Still, it would be great if we can make context to be the actual function.

That sounds messy.

> Otherwise I will need to figure out some way to represent the link in between
> static variable and function it takes label of...

Check DECL_ORIGIN of the label's function?

Jason
Jan Hubicka March 30, 2015, 6:53 p.m. UTC | #8 
> On 03/30/2015 02:36 PM, Jan Hubicka wrote:
> >Will there be only one copy even on targets that do not support aliases?
> 
> Yes, on those targets we use thunks.

I see, and only if thinks fails on variadic arguments we lose, right?

> 
> >Still, it would be great if we can make context to be the actual function.
> 
> That sounds messy.
> 
> >Otherwise I will need to figure out some way to represent the link in between
> >static variable and function it takes label of...
> 
> Check DECL_ORIGIN of the label's function?

What I am trying to do is  to easily get a way IPA code can group the function
with static variables referring its initializers without actually needing to
introduce new representation by ipa-ref.

So I added a flag to static variables and when walking symbol table I only have
the static variable decl at hand (no constructor) and want to get back to the
containing function that define its label.

I hoped we are safe to use the decl_function_context, because these functions are
never cloned, but here we just get ready for cloning without actually doing it ;).
Can you thnk of way of doing so easily?

I suppose I may just go with patch as it is and silecne the ICE for this stage4
or develop more involved fix with the extra reference represented somehow
which may be better to wait for next stage1 and be backported.  I will try to think
about it bit more today.  

Honza
> 
> Jason
diff mbox

Patch

Index: symtab.c
===================================================================
--- symtab.c	(revision 221757)
+++ symtab.c	(working copy)
@@ -1682,6 +1682,11 @@  symtab_node::get_partitioning_class (voi
 
   if (varpool_node *vnode = dyn_cast <varpool_node *> (this))
     {
+      /* Static variables referring named label are always handled the same way
+	 as the function they belong to.  */
+      if (vnode->contains_named_label)
+	return cgraph_node::get (decl_function_context (decl))
+		->get_partitioning_class ();
       if (alias && definition && !ultimate_alias_target ()->definition)
 	return SYMBOL_EXTERNAL;
       /* Constant pool references use local symbol names that can not
Index: lto-cgraph.c
===================================================================
--- lto-cgraph.c	(revision 221757)
+++ lto-cgraph.c	(working copy)
@@ -662,6 +663,7 @@  lto_output_varpool_node (struct lto_simp
   bp_pack_value (&bp, node->tls_model, 3);
   bp_pack_value (&bp, node->used_by_single_function, 1);
   bp_pack_value (&bp, node->need_bounds_init, 1);
+  bp_pack_value (&bp, node->contains_named_label, 1);
   streamer_write_bitpack (&bp);
 
   group = node->get_comdat_group ();
@@ -1413,6 +1416,7 @@  input_varpool_node (struct lto_file_decl
   node->tls_model = (enum tls_model)bp_unpack_value (&bp, 3);
   node->used_by_single_function = (enum tls_model)bp_unpack_value (&bp, 1);
   node->need_bounds_init = bp_unpack_value (&bp, 1);
+  node->contains_named_label = bp_unpack_value (&bp, 1);
   group = read_identifier (ib);
   if (group)
     {
Index: tree.c
===================================================================
--- tree.c	(revision 221757)
+++ tree.c	(working copy)
@@ -3149,7 +3149,12 @@  decl_address_ip_invariant_p (const_tree
 
   switch (TREE_CODE (op))
     {
+    /* Theoretically label decls can be considered IP invariants, but until
+       we are able to handle them as regular symbols (i.e. output with
+       non-local visibility), we really want them to be contained in a function
+       they belong to.  */
     case LABEL_DECL:
+      return false;
     case FUNCTION_DECL:
     case STRING_CST:
       return true;
Index: cgraph.h
===================================================================
--- cgraph.h	(revision 221757)
+++ cgraph.h	(working copy)
@@ -1778,6 +1780,9 @@  public:
      if we did not do any inter-procedural code movement.  */
   unsigned used_by_single_function : 1;
 
+  /* Set if te variable is contains a named label.  */
+  unsigned contains_named_label : 1;
+
 private:
   /* Assemble thunks and aliases associated to varpool node.  */
   void assemble_aliases (void);
Index: cgraphbuild.c
===================================================================
--- cgraphbuild.c	(revision 221757)
+++ cgraphbuild.c	(working copy)
@@ -112,6 +112,8 @@  record_reference (tree *tp, int *walk_su
 	  varpool_node *vnode = varpool_node::get_create (decl);
 	  ctx->varpool_node->create_reference (vnode, IPA_REF_ADDR);
 	}
+      if (TREE_CODE (decl) == LABEL_DECL)
+	ctx->varpool_node->contains_named_label = true;
       *walk_subtrees = 0;
       break;
 
Index: lto/lto-partition.c
===================================================================
--- lto/lto-partition.c	(revision 221757)
+++ lto/lto-partition.c	(working copy)
@@ -113,6 +113,14 @@  add_references_to_partition (ltrans_part
   for (i = 0; node->iterate_reference (i, ref); i++)
     if (ref->referred->get_partitioning_class () == SYMBOL_DUPLICATE)
       add_symbol_to_partition (part, ref->referred);
+    /* A static variable referring to named labels must go to the same
+       partition as a function it belongs to.  */
+    else if (is_a <varpool_node *> (ref->referred)
+	     && (dyn_cast <varpool_node *> (ref->referred)
+		 ->contains_named_label)
+	     && node->decl == decl_function_context (ref->referred->decl)
+	     && !lto_symtab_encoder_in_partition_p (part->encoder, ref->referred))
+      add_symbol_to_partition (part, ref->referred);
     /* References to a readonly variable may be constant foled into its value.
        Recursively look into the initializers of the constant variable and add
        references, too.  */
@@ -193,6 +201,14 @@  add_symbol_to_partition_1 (ltrans_partit
 	add_symbol_to_partition_1 (part, cnode->instrumented_version);
     }
 
+  /* Variables referring named labels must always go to the same section
+     as the function they belong to.  */
+  if (is_a <varpool_node *> (node)
+      && dyn_cast <varpool_node *> (node)->contains_named_label)
+    add_symbol_to_partition_1 (part,
+			         cgraph_node::get
+				    (decl_function_context (node->decl)));
+
   add_references_to_partition (part, node);
 
   /* Add all aliases associated with the symbol.  */
@@ -409,7 +425,12 @@  add_sorted_nodes (vec<symtab_node *> &ne
 
   next_nodes.qsort (varpool_node_cmp);
   FOR_EACH_VEC_ELT (next_nodes, i, node)
-    if (!symbol_partitioned_p (node))
+    if (!symbol_partitioned_p (node)
+	/* Local statics containing named labels always go after their
+	   functions.  Do not partition on them and wait for them to be
+	   dragged in, so we do not force reordering of the functions.  */
+	&& (!is_a <varpool_node *> (node)
+	    || !dyn_cast <varpool_node *> (node)->contains_named_label))
       add_symbol_to_partition (partition, node);
 }
 
@@ -636,6 +657,7 @@  lto_balanced_map (int n_lto_partitions)
 		  continue;
 		if (!symbol_partitioned_p (vnode) && flag_toplevel_reorder
 		    && !vnode->no_reorder
+		    && !vnode->contains_named_label
 		    && vnode->get_partitioning_class () == SYMBOL_PARTITION)
 		  add_symbol_to_partition (partition, vnode);
 		index = lto_symtab_encoder_lookup (partition->encoder,
@@ -674,6 +696,7 @@  lto_balanced_map (int n_lto_partitions)
 		if (!symbol_partitioned_p (vnode) && flag_toplevel_reorder
 		    && !vnode->no_reorder
 		    && !vnode->can_remove_if_no_refs_p ()
+		    && !vnode->contains_named_label
 		    && vnode->get_partitioning_class () == SYMBOL_PARTITION)
 		  add_symbol_to_partition (partition, vnode);
 		index = lto_symtab_encoder_lookup (partition->encoder,
Index: varpool.c
===================================================================
--- varpool.c	(revision 221757)
+++ varpool.c	(working copy)
@@ -256,6 +256,8 @@  varpool_node::dump (FILE *f)
     fprintf (f, " const-value-known");
   if (writeonly)
     fprintf (f, " write-only");
+  if (contains_named_label)
+    fprintf (f, " contains-named-label");
   if (tls_model)
     fprintf (f, " tls-%s", tls_model_names [tls_model]);
   fprintf (f, "\n");
Index: ipa.c
===================================================================
--- ipa.c	(revision 221757)
+++ ipa.c	(working copy)
@@ -406,6 +406,18 @@  symbol_table::remove_unreachable_nodes (
 		      n->used_as_abstract_origin = true;
 		}
 	    }
+
+	  /* Static variables referring local label needs the containing
+	     function to be output.  */
+	  if (is_a <varpool_node *> (node)
+	      && dyn_cast <varpool_node *> (node)->contains_named_label)
+	    {
+	      cgraph_node *cn
+		 = cgraph_node::get (decl_function_context (node->decl));
+	      if (!reachable.add (cn))
+		enqueue_node (cn, &first, &reachable);
+	    }
+	      
 	  /* If any symbol in a comdat group is reachable, force
 	     all externally visible symbols in the same comdat
 	     group to be reachable as well.  Comdat-local symbols
Index: testsuite/gcc.dg/lto/named-label.c
===================================================================
--- testsuite/gcc.dg/lto/named-label.c	(revision 0)
+++ testsuite/gcc.dg/lto/named-label.c	(revision 0)
@@ -0,0 +1,29 @@ 
+/* { dg-lto-do link } */
+/* { dg-lto-options { { -fPIC -flto -flto-partition=max } } } */
+void *p0,*p1;
+void test (void **p)
+{
+  if (p[0]==p[1])
+    __builtin_abort ();
+  p0=p[0];
+  p1=p[1];
+}
+void test2 (void **p)
+{
+  if (p[0]!=p0|| p[1] !=p1)
+    __builtin_abort ();
+}
+void ** t()
+{
+  static void * labelptr[2]={&&label, &&label2};
+label:
+  test (labelptr);
+label2:
+  return labelptr;
+}
+int
+main()
+{
+  void **labelptr = t();
+  test2 (labelptr);
+}