On Tue, Aug 30, 2011 at 6:50 PM, Martin Jambor <mjam...@suse.cz> wrote:
> Ping. Re-bootstrapped and re-tested yesterday on x86_64-linux.
Ok. Does this also apply (maybe in modifed form) to the 4.6 branch?
Thanks,
Richard.
> 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 <mjam...@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);
>> +}
>> +
>
