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);
+}
+
