Hi,
PR 77333 is a i686-windows target bug, which however has its root in
our general mechanism of adjusting gimple statements when redirecting
call graph edge. Basically, these three things trigger it:
1) IPA-CP figures out that the this parameter of a C++ class method is
unused and because the class is in an anonymous namespace, it can
be removed and all calls adjusted. That effectively changes a
normal method into a static method and so internally, its type
changes from METHOD_TYPE to FUNCTION_TYPE.
2) Since the fix of PR 57330, we do not update gimple_call_fntype to
match the new type, in fact we explicitely set it to the old, now
invalid, type (see redirect_call_stmt_to_callee in cgraph.c).
3) Function ix86_get_callcvt which decides on call ABI, ends with the
following condition:
if (ret != 0
|| is_stdarg
|| TREE_CODE (type) != METHOD_TYPE
|| ix86_function_type_abi (type) != MS_ABI)
return IX86_CALLCVT_CDECL | ret;
return IX86_CALLCVT_THISCALL;
...and since now the callee is no longer a METHOD_TYPE but callers
still think that they are, leading to calling convention mismatches
and subsequent crashes. It took me quite a lot of time to come up
with a small testcase (reproducible using wine) but eventually I
managed.
The fix is not to do 2) above, but doing so without re-introducing PR
57330, of course. There are two options. One is to use the
call_stmt_cannot_inline_p flag of call-graph edges and not do any
IPA-CP accross those edges. That is done in the patch below. The (so
far a bit hypothetical) problem with that approach is that the call
graph edge flag may not be 100% reliable in LTO, because incompatible
decls might get merged and then we wold re-introduce PR 57330 again -
only with on invalid code and with LTO but an ICE nevertheless.
So the alternative would be to re-check when doing the gimple
statement adjustment and if the types match, then set the correct new
gimple_fntype and if they don't... then we can either leave it be or
just run the same type transformation on it as we did on the callee,
though they would be bogus either way. That is implemented in the
attached patch.
I have successfully bootstrapped both patches on x86_64-linux and I
have also tested them both on a windows cross-compiler and wine (with
some noise but I believe it is just noise).
Honza, Richi, do you prefer any one approach over the other?
Actually, we can have both, would that be desirable?
Thanks,
Martin
2017-03-02 Martin Jambor <mjam...@suse.cz>
PR ipa/77333
* ipa-prop.h (ipa_node_params): New field call_stmt_type_mismatch.
(ipa_node_params::ipa_node_params): Initialize it to zero.
* cgraph.c (redirect_call_stmt_to_callee): Set gimple fntype to
the type of the new target.
* ipa-cp.c (propagate_constants_across_call): Set variable flag of
lattices and call_stmt_type_mismatch of the callee when
encountering an edge with mismatched types.
(estimate_local_effects): Do not clone for all contexts when
call_stmt_type_mismatch is set.
testsuite/
* g++.dg/ipa/pr77333.C: New test.
---
gcc/cgraph.c | 2 +-
gcc/ipa-cp.c | 11 ++++---
gcc/ipa-prop.h | 4 ++-
gcc/testsuite/g++.dg/ipa/pr77333.C | 65 ++++++++++++++++++++++++++++++++++++++
4 files changed, 76 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/ipa/pr77333.C
diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 839388496ee..642ff0bcfc2 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -1425,7 +1425,7 @@ cgraph_edge::redirect_call_stmt_to_callee (void)
new_stmt = chkp_copy_call_skip_bounds (new_stmt);
gimple_call_set_fndecl (new_stmt, e->callee->decl);
- gimple_call_set_fntype (new_stmt, gimple_call_fntype (e->call_stmt));
+ gimple_call_set_fntype (new_stmt, TREE_TYPE (e->callee->decl));
if (gimple_vdef (new_stmt)
&& TREE_CODE (gimple_vdef (new_stmt)) == SSA_NAME)
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index aa3c9973a66..d27151ffade 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -2231,9 +2231,11 @@ propagate_constants_across_call (struct cgraph_edge *cs)
checking instrumentation_clone flag for chain source and target.
Going through instrumentation thunks we always have it changed
from 0 to 1 and all other nodes do not change it. */
- if (!cs->callee->instrumentation_clone
- && callee->instrumentation_clone)
+ if (cs->call_stmt_cannot_inline_p
+ || (!cs->callee->instrumentation_clone
+ && callee->instrumentation_clone))
{
+ callee_info->call_stmt_type_mismatch = true;
for (i = 0; i < parms_count; i++)
ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info,
i));
@@ -2841,8 +2843,9 @@ estimate_local_effects (struct cgraph_node *node)
known_aggs_ptrs = agg_jmp_p_vec_for_t_vec (known_aggs);
int devirt_bonus = devirtualization_time_bonus (node, known_csts,
known_contexts, known_aggs_ptrs);
- if (always_const || devirt_bonus
- || (removable_params_cost && node->local.can_change_signature))
+ if (!info->call_stmt_type_mismatch
+ && (always_const || devirt_bonus
+ || (removable_params_cost && node->local.can_change_signature)))
{
struct caller_statistics stats;
inline_hints hints;
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
index 8f7eb088813..612268415ff 100644
--- a/gcc/ipa-prop.h
+++ b/gcc/ipa-prop.h
@@ -360,6 +360,8 @@ struct GTY((for_user)) ipa_node_params
unsigned node_calling_single_call : 1;
/* False when there is something makes versioning impossible. */
unsigned versionable : 1;
+ /* Node is called with a call statement with mismatched types. */
+ unsigned call_stmt_type_mismatch : 1;
};
inline
@@ -368,7 +370,7 @@ ipa_node_params::ipa_node_params ()
known_csts (vNULL), known_contexts (vNULL), analysis_done (0),
node_enqueued (0), do_clone_for_all_contexts (0), is_all_contexts_clone (0),
node_dead (0), node_within_scc (0), node_calling_single_call (0),
- versionable (0)
+ versionable (0), call_stmt_type_mismatch (0)
{
}
diff --git a/gcc/testsuite/g++.dg/ipa/pr77333.C
b/gcc/testsuite/g++.dg/ipa/pr77333.C
new file mode 100644
index 00000000000..1ef997f7a54
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr77333.C
@@ -0,0 +1,65 @@
+// { dg-do run }
+// { dg-options "-O2 -fno-ipa-sra" }
+
+volatile int global;
+int __attribute__((noinline, noclone))
+get_data (int i)
+{
+ global = i;
+ return i;
+}
+
+typedef int array[32];
+
+namespace {
+
+char buf[512];
+
+class A
+{
+public:
+ int field;
+ char *s;
+
+ A() : field(223344)
+ {
+ s = buf;
+ }
+
+ int __attribute__((noinline))
+ foo (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j,
+ int k, int l, int m, int n, int o, int p, int q, int r, int s, int t)
+ {
+ global = a+b+c+d+e+f+g+h+i+j+k+l+m+n+o+p+q+r+s+t;
+ return global;
+ }
+
+ int __attribute__((noinline))
+ bar()
+ {
+ int r = foo (get_data (1), get_data (1), get_data (1), get_data (1),
+ get_data (1), get_data (1), get_data (1), get_data (1),
+ get_data (1), get_data (1), get_data (1), get_data (1),
+ get_data (1), get_data (1), get_data (1), get_data (1),
+ get_data (1), get_data (1), get_data (1), get_data (1));
+
+ if (field != 223344)
+ __builtin_abort ();
+ return 0;
+ }
+};
+
+}
+
+int main (int argc, char **argv)
+{
+ A a;
+ int r = a.bar();
+ r = a.bar ();
+ if (a.field != 223344)
+ __builtin_abort ();
+ if (global != 20)
+ __builtin_abort ();
+
+ return r;
+}
--
2.11.1
[PR 77333] Fixup fntypes of gimple calls of clones
2017-03-02 Martin Jambor <mjam...@suse.cz>
PR ipa/77333
* cgraph.h (cgraph_build_function_type_skip_args): Declare.
* cgraph.c (redirect_call_stmt_to_callee): Set gimple fntype so that
it reflects the signature changes performed at the callee side.
* cgraphclones.c (build_function_type_skip_args): Make public, renamed
to cgraph_build_function_type_skip_args.
(build_function_decl_skip_args): Adjust call to the above function.
testsuite/
* g++.dg/ipa/pr77333.C: New test.
---
gcc/cgraph.c | 16 +++++++++-
gcc/cgraph.h | 2 ++
gcc/cgraphclones.c | 9 +++---
gcc/testsuite/g++.dg/ipa/pr77333.C | 65 ++++++++++++++++++++++++++++++++++++++
4 files changed, 87 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/ipa/pr77333.C
diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 839388496ee..5143eafa2a6 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -1424,8 +1424,22 @@ cgraph_edge::redirect_call_stmt_to_callee (void)
if (skip_bounds)
new_stmt = chkp_copy_call_skip_bounds (new_stmt);
+ tree old_fntype = gimple_call_fntype (e->call_stmt);
+ tree old_fndecl = gimple_call_fndecl (e->call_stmt);
gimple_call_set_fndecl (new_stmt, e->callee->decl);
- gimple_call_set_fntype (new_stmt, gimple_call_fntype (e->call_stmt));
+ if ((e->callee->former_clone_of
+ && old_fntype == TREE_TYPE (e->callee->former_clone_of))
+ || (old_fndecl
+ && gimple_check_call_matching_types (e->call_stmt,
+ old_fndecl, false)))
+ gimple_call_set_fntype (new_stmt, TREE_TYPE (e->callee->decl));
+ else
+ {
+ bitmap skip = e->callee->clone.combined_args_to_skip;
+ tree t = cgraph_build_function_type_skip_args (old_fntype, skip,
+ false);
+ gimple_call_set_fntype (new_stmt, t);
+ }
if (gimple_vdef (new_stmt)
&& TREE_CODE (gimple_vdef (new_stmt)) == SSA_NAME)
diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index 3889a3e1701..62cebd9e55a 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -2326,6 +2326,8 @@ void tree_function_versioning (tree, tree, vec<ipa_replace_map *, va_gc> *,
void dump_callgraph_transformation (const cgraph_node *original,
const cgraph_node *clone,
const char *suffix);
+tree cgraph_build_function_type_skip_args (tree orig_type, bitmap args_to_skip,
+ bool skip_return);
/* In cgraphbuild.c */
int compute_call_stmt_bb_frequency (tree, basic_block bb);
diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index c2337e84553..69572b926c4 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -152,9 +152,9 @@ cgraph_edge::clone (cgraph_node *n, gcall *call_stmt, unsigned stmt_uid,
/* Build variant of function type ORIG_TYPE skipping ARGS_TO_SKIP and the
return value if SKIP_RETURN is true. */
-static tree
-build_function_type_skip_args (tree orig_type, bitmap args_to_skip,
- bool skip_return)
+tree
+cgraph_build_function_type_skip_args (tree orig_type, bitmap args_to_skip,
+ bool skip_return)
{
tree new_type = NULL;
tree args, new_args = NULL;
@@ -219,7 +219,8 @@ build_function_decl_skip_args (tree orig_decl, bitmap args_to_skip,
if (prototype_p (new_type)
|| (skip_return && !VOID_TYPE_P (TREE_TYPE (new_type))))
new_type
- = build_function_type_skip_args (new_type, args_to_skip, skip_return);
+ = cgraph_build_function_type_skip_args (new_type, args_to_skip,
+ skip_return);
TREE_TYPE (new_decl) = new_type;
/* For declarations setting DECL_VINDEX (i.e. methods)
diff --git a/gcc/testsuite/g++.dg/ipa/pr77333.C b/gcc/testsuite/g++.dg/ipa/pr77333.C
new file mode 100644
index 00000000000..1ef997f7a54
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr77333.C
@@ -0,0 +1,65 @@
+// { dg-do run }
+// { dg-options "-O2 -fno-ipa-sra" }
+
+volatile int global;
+int __attribute__((noinline, noclone))
+get_data (int i)
+{
+ global = i;
+ return i;
+}
+
+typedef int array[32];
+
+namespace {
+
+char buf[512];
+
+class A
+{
+public:
+ int field;
+ char *s;
+
+ A() : field(223344)
+ {
+ s = buf;
+ }
+
+ int __attribute__((noinline))
+ foo (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j,
+ int k, int l, int m, int n, int o, int p, int q, int r, int s, int t)
+ {
+ global = a+b+c+d+e+f+g+h+i+j+k+l+m+n+o+p+q+r+s+t;
+ return global;
+ }
+
+ int __attribute__((noinline))
+ bar()
+ {
+ int r = foo (get_data (1), get_data (1), get_data (1), get_data (1),
+ get_data (1), get_data (1), get_data (1), get_data (1),
+ get_data (1), get_data (1), get_data (1), get_data (1),
+ get_data (1), get_data (1), get_data (1), get_data (1),
+ get_data (1), get_data (1), get_data (1), get_data (1));
+
+ if (field != 223344)
+ __builtin_abort ();
+ return 0;
+ }
+};
+
+}
+
+int main (int argc, char **argv)
+{
+ A a;
+ int r = a.bar();
+ r = a.bar ();
+ if (a.field != 223344)
+ __builtin_abort ();
+ if (global != 20)
+ __builtin_abort ();
+
+ return r;
+}
--
2.12.0
