Re: [PATCH, PR 49886] Prevent fnsplit from changing signature when there are type attributes

[Martin Jambor <mjambor at suse dot cz>]

Ping.  Re-bootstrapped and re-tested yesterday on x86_64-linux.

THanks,

Martin


On Fri, Jul 29, 2011 at 10:55:31PM +0200, Martin Jambor wrote:
> Hi,
> 
> On Thu, Jul 28, 2011 at 06:52:05PM +0200, Martin Jambor wrote:
> > pass_split_functions is happy to split functions which have type
> > attributes but cannot update them if the new clone has in any way
> > different parameters than the original.  This can lead to
> > miscompilations in cases like the testcase.
> > 
> > This patch solves it by 1) making the inliner set the
> > can_change_signature flag to false for them because their signature
> > cannot be changed (this step is also necessary to make IPA-CP operate
> > on them and handle them correctly), and 2) make the splitting pass
> > keep all parameters if the flag is set.  The second step might involve
> > inventing some default definitions if the parameters did not really
> > have any.
> > 
> > I spoke about this with Honza and he claimed that the new function is
> > really an entirely different thing and that the parameters may
> > correspond only very loosely and thus the type attributes should be
> > cleared.  I'm not sure I agree, but in any case I needed this to work
> > to allow me continue with promised IPA-CP polishing and so I decided
> > to do this because it was easier.  (My own opinion is that the current
> > representation of parameter-describing function type attributes is
> > evil and will cause harm no matter hat we do.)
> > 
> 
> Actually, I'd like to commit the patch below which also clears
> can_change_signature for BUILT_IN_VA_START.  It is not really
> necessary for this fix but fixes some problems in a followup patch and
> is also the correct thing to do because if we clone a function calling
> it and pass non-NULL for args_to_skip, the new clone would not have a
> stdarg_p type and fold_builtin_next_arg could error when dealing with
> it.
> 
> Also bootstrapped and tested on x86_64-linux.  OK for trunk?
> 
> Thanks,
> 
> Martin
> 
> 
> 2011-07-29  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR middle-end/49886
> 	* ipa-inline-analysis.c (compute_inline_parameters): Set
> 	can_change_signature of noes with typde attributes.
> 	* ipa-split.c (split_function): Do not skip any arguments if
> 	can_change_signature is set.
> 
> 	* testsuite/gcc.c-torture/execute/pr49886.c: New testcase.
> 
> Index: src/gcc/ipa-inline-analysis.c
> ===================================================================
> --- src.orig/gcc/ipa-inline-analysis.c
> +++ src/gcc/ipa-inline-analysis.c
> @@ -1658,18 +1658,28 @@ compute_inline_parameters (struct cgraph
>    /* Can this function be inlined at all?  */
>    info->inlinable = tree_inlinable_function_p (node->decl);
>  
> -  /* Inlinable functions always can change signature.  */
> -  if (info->inlinable)
> -    node->local.can_change_signature = true;
> +  /* Type attributes can use parameter indices to describe them.  */
> +  if (TYPE_ATTRIBUTES (TREE_TYPE (node->decl)))
> +    node->local.can_change_signature = false;
>    else
>      {
> -      /* Functions calling builtin_apply can not change signature.  */
> -      for (e = node->callees; e; e = e->next_callee)
> -	if (DECL_BUILT_IN (e->callee->decl)
> -	    && DECL_BUILT_IN_CLASS (e->callee->decl) == BUILT_IN_NORMAL
> -	    && DECL_FUNCTION_CODE (e->callee->decl) == BUILT_IN_APPLY_ARGS)
> -	  break;
> -      node->local.can_change_signature = !e;
> +      /* Otherwise, inlinable functions always can change signature.  */
> +      if (info->inlinable)
> +	node->local.can_change_signature = true;
> +      else
> +	{
> +	  /* Functions calling builtin_apply can not change signature.  */
> +	  for (e = node->callees; e; e = e->next_callee)
> +	    {
> +	      tree cdecl = e->callee->decl;
> +	      if (DECL_BUILT_IN (cdecl)
> +		  && DECL_BUILT_IN_CLASS (cdecl) == BUILT_IN_NORMAL
> +		  && (DECL_FUNCTION_CODE (cdecl) == BUILT_IN_APPLY_ARGS
> +		      || DECL_FUNCTION_CODE (cdecl) == BUILT_IN_VA_START))
> +		break;
> +	    }
> +	  node->local.can_change_signature = !e;
> +	}
>      }
>    estimate_function_body_sizes (node, early);
>  
> Index: src/gcc/ipa-split.c
> ===================================================================
> --- src.orig/gcc/ipa-split.c
> +++ src/gcc/ipa-split.c
> @@ -945,10 +945,10 @@ static void
>  split_function (struct split_point *split_point)
>  {
>    VEC (tree, heap) *args_to_pass = NULL;
> -  bitmap args_to_skip = BITMAP_ALLOC (NULL);
> +  bitmap args_to_skip;
>    tree parm;
>    int num = 0;
> -  struct cgraph_node *node;
> +  struct cgraph_node *node, *cur_node = cgraph_get_node (current_function_decl);
>    basic_block return_bb = find_return_bb ();
>    basic_block call_bb;
>    gimple_stmt_iterator gsi;
> @@ -968,17 +968,30 @@ split_function (struct split_point *spli
>        dump_split_point (dump_file, split_point);
>      }
>  
> +  if (cur_node->local.can_change_signature)
> +    args_to_skip = BITMAP_ALLOC (NULL);
> +  else
> +    args_to_skip = NULL;
> +
>    /* Collect the parameters of new function and args_to_skip bitmap.  */
>    for (parm = DECL_ARGUMENTS (current_function_decl);
>         parm; parm = DECL_CHAIN (parm), num++)
> -    if (!is_gimple_reg (parm)
> -	|| !gimple_default_def (cfun, parm)
> -	|| !bitmap_bit_p (split_point->ssa_names_to_pass,
> -			  SSA_NAME_VERSION (gimple_default_def (cfun, parm))))
> +    if (args_to_skip
> +	&& (!is_gimple_reg (parm)
> +	    || !gimple_default_def (cfun, parm)
> +	    || !bitmap_bit_p (split_point->ssa_names_to_pass,
> +			      SSA_NAME_VERSION (gimple_default_def (cfun,
> +								    parm)))))
>        bitmap_set_bit (args_to_skip, num);
>      else
>        {
>  	arg = gimple_default_def (cfun, parm);
> +	if (!arg)
> +	  {
> +	    arg = make_ssa_name (parm, gimple_build_nop ());
> +	    set_default_def (parm, arg);
> +	  }
> +
>  	if (TYPE_MAIN_VARIANT (DECL_ARG_TYPE (parm))
>  	    != TYPE_MAIN_VARIANT (TREE_TYPE (arg)))
>  	  {
> @@ -1081,9 +1094,7 @@ split_function (struct split_point *spli
>  
>    /* Now create the actual clone.  */
>    rebuild_cgraph_edges ();
> -  node = cgraph_function_versioning (cgraph_get_node (current_function_decl),
> -				     NULL, NULL,
> -				     args_to_skip,
> +  node = cgraph_function_versioning (cur_node, NULL, NULL, args_to_skip,
>  				     split_point->split_bbs,
>  				     split_point->entry_bb, "part");
>    /* For usual cloning it is enough to clear builtin only when signature
> @@ -1094,7 +1105,7 @@ split_function (struct split_point *spli
>        DECL_BUILT_IN_CLASS (node->decl) = NOT_BUILT_IN;
>        DECL_FUNCTION_CODE (node->decl) = (enum built_in_function) 0;
>      }
> -  cgraph_node_remove_callees (cgraph_get_node (current_function_decl));
> +  cgraph_node_remove_callees (cur_node);
>    if (!split_part_return_p)
>      TREE_THIS_VOLATILE (node->decl) = 1;
>    if (dump_file)
> Index: src/gcc/testsuite/gcc.c-torture/execute/pr49886.c
> ===================================================================
> --- /dev/null
> +++ src/gcc/testsuite/gcc.c-torture/execute/pr49886.c
> @@ -0,0 +1,100 @@
> +struct PMC {
> +    unsigned flags;
> +};
> +
> +typedef struct Pcc_cell
> +{
> +    struct PMC *p;
> +    long bla;
> +    long type;
> +} Pcc_cell;
> +
> +int gi;
> +int cond;
> +
> +extern void abort ();
> +extern void never_ever(int interp, struct PMC *pmc)
> +  __attribute__((noinline,noclone));
> +
> +void never_ever (int interp, struct PMC *pmc)
> +{
> +  abort ();
> +}
> +
> +static void mark_cell(int * interp, Pcc_cell *c)
> +  __attribute__((__nonnull__(1)));
> +
> +static void
> +mark_cell(int * interp, Pcc_cell *c)
> +{
> +  if (!cond)
> +    return;
> +
> +  if (c && c->type == 4 && c->p
> +      && !(c->p->flags & (1<<18)))
> +    never_ever(gi + 1, c->p);
> +  if (c && c->type == 4 && c->p
> +      && !(c->p->flags & (1<<17)))
> +    never_ever(gi + 2, c->p);
> +  if (c && c->type == 4 && c->p
> +      && !(c->p->flags & (1<<16)))
> +    never_ever(gi + 3, c->p);
> +  if (c && c->type == 4 && c->p
> +      && !(c->p->flags & (1<<15)))
> +    never_ever(gi + 4, c->p);
> +  if (c && c->type == 4 && c->p
> +      && !(c->p->flags & (1<<14)))
> +    never_ever(gi + 5, c->p);
> +  if (c && c->type == 4 && c->p
> +      && !(c->p->flags & (1<<13)))
> +    never_ever(gi + 6, c->p);
> +  if (c && c->type == 4 && c->p
> +      && !(c->p->flags & (1<<12)))
> +    never_ever(gi + 7, c->p);
> +  if (c && c->type == 4 && c->p
> +      && !(c->p->flags & (1<<11)))
> +    never_ever(gi + 8, c->p);
> +  if (c && c->type == 4 && c->p
> +      && !(c->p->flags & (1<<10)))
> +    never_ever(gi + 9, c->p);
> +}
> +
> +static void
> +foo(int * interp, Pcc_cell *c)
> +{
> +  mark_cell(interp, c);
> +}
> +
> +static struct Pcc_cell *
> +__attribute__((noinline,noclone))
> +getnull(void)
> +{
> +  return (struct Pcc_cell *) 0;
> +}
> +
> +
> +int main()
> +{
> +  int i;
> +
> +  cond = 1;
> +  for (i = 0; i < 100; i++)
> +    foo (&gi, getnull ());
> +  return 0;
> +}
> +
> +
> +void
> +bar_1 (int * interp, Pcc_cell *c)
> +{
> +  c->bla += 1;
> +  mark_cell(interp, c);
> +}
> +
> +void
> +bar_2 (int * interp, Pcc_cell *c)
> +{
> +  c->bla += 2;
> +  mark_cell(interp, c);
> +}
> +