[PR,77333] Fix fntypes of calls calling clones

Hi,

On Thu, Mar 16, 2017 at 05:57:51PM +0100, Martin Jambor wrote:
> Hi,
> 
> On Mon, Mar 13, 2017 at 01:46:47PM +0100, Richard Biener wrote:
> > 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.

...

> > 
> > 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.
> > 

...

> After talking to Honza today, we decided to probably go this route and
> use the patch doing the type conversion at acall-sites when necessary.
> Honza promised to review the patch soon (he wants to figure out why
> former_clone_of can be NULL, something I decided not to bother about
> since at that time I thought the other approach was going to be
> preferable).
> 

and this is a slightly adjusted patch that is a result of what we
talked about.  I know that it is potentially disruptive change, so I
have tested it with:
  - bootstrap and testing and LTO-bootstrap and testing on x86_64-linux,
  - bootstrap and testing on i686-linux, ppc64le-linux and ia64-linux
  - bootstrap on aarch64-linux (no testing because there is no dejagnu
    installed on gcc117.fsffrance.org),
  - testing on i686-w64-mingw32 on Linux+wine, and
  - testing on powerpc-aix is underway.

OK for trunk (and subsequently to backport to gcc 6 and 5)?

Thanks,

Martin

2017-03-24  Martin Jambor  <mjambor@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                       | 17 +++++++++-
 gcc/cgraph.h                       |  2 ++
 gcc/cgraphclones.c                 |  9 +++---
 gcc/testsuite/g++.dg/ipa/pr77333.C | 65 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 88 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr77333.C

On Wed, 29 Mar 2017, Martin Jambor wrote:

> Hi,
> 
> On Thu, Mar 16, 2017 at 05:57:51PM +0100, Martin Jambor wrote:
> > Hi,
> > 
> > On Mon, Mar 13, 2017 at 01:46:47PM +0100, Richard Biener wrote:
> > > 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.
> 
> ...
> 
> > > 
> > > 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.
> > > 
> 
> ...
> 
> > After talking to Honza today, we decided to probably go this route and
> > use the patch doing the type conversion at acall-sites when necessary.
> > Honza promised to review the patch soon (he wants to figure out why
> > former_clone_of can be NULL, something I decided not to bother about
> > since at that time I thought the other approach was going to be
> > preferable).
> > 
> 
> and this is a slightly adjusted patch that is a result of what we
> talked about.  I know that it is potentially disruptive change, so I
> have tested it with:
>   - bootstrap and testing and LTO-bootstrap and testing on x86_64-linux,
>   - bootstrap and testing on i686-linux, ppc64le-linux and ia64-linux
>   - bootstrap on aarch64-linux (no testing because there is no dejagnu
>     installed on gcc117.fsffrance.org),
>   - testing on i686-w64-mingw32 on Linux+wine, and
>   - testing on powerpc-aix is underway.
> 
> OK for trunk (and subsequently to backport to gcc 6 and 5)?

Ok.  Ok to backport after some burn-in.

Richard.

> Thanks,
> 
> Martin
> 
> 2017-03-24  Martin Jambor  <mjambor@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                       | 17 +++++++++-
>  gcc/cgraph.h                       |  2 ++
>  gcc/cgraphclones.c                 |  9 +++---
>  gcc/testsuite/g++.dg/ipa/pr77333.C | 65 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 88 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..92ae0910c60 100644
> --- a/gcc/cgraph.c
> +++ b/gcc/cgraph.c
> @@ -1424,8 +1424,23 @@ 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);
>        gimple_call_set_fndecl (new_stmt, e->callee->decl);
> -      gimple_call_set_fntype (new_stmt, gimple_call_fntype (e->call_stmt));
> +      cgraph_node *origin = e->callee;
> +      while (origin->clone_of)
> +	origin = origin->clone_of;
> +
> +      if ((origin->former_clone_of
> +	   && old_fntype == TREE_TYPE (origin->former_clone_of))
> +	  || old_fntype == TREE_TYPE (origin->decl))
> +	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;
> +}
>