[PR 77333] Fix fntypes of calls calling clones

Martin Jambor mjambor@suse.cz
Fri Mar 10 15:07:00 GMT 2017


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  <mjambor@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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-PR-77333-Fixup-fntypes-of-gimple-calls-of-clones.patch
Type: text/x-patch
Size: 5090 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20170310/0cee9ad9/attachment.bin>


More information about the Gcc-patches mailing list