On Fri, 10 Mar 2017, Martin Jambor wrote:
> 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 you mean replacing a matching decl with a non-matching and thus
during symtab merge introduce the issue. You could detect whenever
merging incompatible symbols and set ->cannot_change_signature on
the prevailing node though?
But how do we deal with devirt here? That is propagation itself
may introduce (knowledge of) the incompatibility...
In general I am sympathetic with not doing any IPA propagation
across call stmt signature incompatibilties. Of course we may
be still too strict in those compatibility check...
> 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.
As we _do_ adjust the call (apply the transform to its actual
arguments) we should apply the same transform to
gimple_call_fntype. We can avoid doing duplicate work in case
the old fndecl (do we still have that around?) matched fntype.
I think the common case of "mismatches" is having them with
respect to the "extern" declaration but then the implementation
is actually ok (otherwise people would run into runtime issues).
I wonder what we do to IPA-SRA-kind of transforms ... (as it's early
it's much easier to just not do anything about those at this point).
> 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?
I think I'd like to see statistics for say, SPEC, how many
cgraph edges we disable transforms for. For correctness I'd simply
do the same transform to fntype as to the actual stmt (which of course
has some cost).
Richard.
> 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;
> +}
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
