Re: [PR 70929] IPA call type compatibility fix/workaround

[Richard Biener <rguenther at suse dot de>]

On Tue, 8 Oct 2019, Martin Jambor wrote:

> Hi,
> 
> I've been looking at PR 70929 and at the newly reported duplicate PR
> 91988 and would like to propose the following patch to address them.
> Basically, the idea is that if the types or arguments in TYPE_ARG_TYPES
> (as opposed to DECL_ARGUMENTS) from both the type from the target fndecl
> and from call statement fntype match, then we can assume that whatever
> promotions happened to the arguments they are the same in all
> compilation units and the calls will be compatible.  I inserted this
> test in the middle of gimple_check_call_args and it works for testcase
> from both bugs.
> 
> The new test of course can be fooled with programs with clearly
> undefined behavior, for example by having an indirect call which early
> optimizations would discover to call an incompatible functions - but the
> change considered in comment #5 of the bug would be too.  Moreover,
> unless we stream argument types one will always be able to fool the
> compiler and I find being careful about those and not inlining valid
> calls with references to TREE_ADDRESSABLE classes a bad tradeoff.
> 
> I verified that - at least on gcc/libstdc++ testsuites - the new
> gimple_check_call_args never returns false when the old one would return
> true.  I can imagine us not doing the
> 
>   fold_convertible_p (TREE_VALUE (f), arg)
> 
> bit or returning false whenever reach the line
> 
>   tree p;
> 
> and the function has any parameters at all.  This would make the
> function return false for on un-prototyped and/or K&R function
> declarations, but perhaps we don't care too much?
> 
> In any event, I have bootstrapped and tested this on x86_64-linux, is it
> perhaps OK for trunk?
> 
> Martin
> 
> 
> 2019-10-03  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR lto/70929
> 	* cgraph.c (gimple_check_call_args): Also compare types of argumen
> 	types and call statement fntype types.
> 
> 	testsuite/
> 	* g++.dg/lto/pr70929_[01].C: New test.
> ---
>  gcc/cgraph.c                         | 83 ++++++++++++++++++++++------
>  gcc/testsuite/g++.dg/lto/pr70929_0.C | 18 ++++++
>  gcc/testsuite/g++.dg/lto/pr70929_1.C | 10 ++++
>  3 files changed, 95 insertions(+), 16 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/lto/pr70929_0.C
>  create mode 100644 gcc/testsuite/g++.dg/lto/pr70929_1.C
> 
> diff --git a/gcc/cgraph.c b/gcc/cgraph.c
> index 0c3c6e7cac4..5a4c5253b49 100644
> --- a/gcc/cgraph.c
> +++ b/gcc/cgraph.c
> @@ -3636,26 +3636,19 @@ cgraph_node::get_fun () const
>  static bool
>  gimple_check_call_args (gimple *stmt, tree fndecl, bool args_count_match)
>  {
> -  tree parms, p;
> -  unsigned int i, nargs;
> -
>    /* Calls to internal functions always match their signature.  */
>    if (gimple_call_internal_p (stmt))
>      return true;
>  
> -  nargs = gimple_call_num_args (stmt);
> +  unsigned int nargs = gimple_call_num_args (stmt);
>  
> -  /* Get argument types for verification.  */
> -  if (fndecl)
> -    parms = TYPE_ARG_TYPES (TREE_TYPE (fndecl));
> -  else
> -    parms = TYPE_ARG_TYPES (gimple_call_fntype (stmt));
> -
> -  /* Verify if the type of the argument matches that of the function
> -     declaration.  If we cannot verify this or there is a mismatch,
> -     return false.  */
> +  /* If we have access to the declarations of formal parameters, match against
> +     those.  */
>    if (fndecl && DECL_ARGUMENTS (fndecl))
>      {
> +      unsigned int i;
> +      tree p;
> +
>        for (i = 0, p = DECL_ARGUMENTS (fndecl);
>  	   i < nargs;
>  	   i++, p = DECL_CHAIN (p))
> @@ -3676,17 +3669,75 @@ gimple_check_call_args (gimple *stmt, tree fndecl, bool args_count_match)
>  	}
>        if (args_count_match && p)
>  	return false;
> +
> +      return true;
>      }
> -  else if (parms)
> +
> +  /* If we don't have decls of formal parameters, see if we have both the type
> +     of the callee arguments in the fntype of the call statement and compare
> +     those.  We rely on the fact that whatever promotions happened to
> +     declarations for exactly the same sequence of types of parameters also
> +     happened on the callee side.  */
> +  if (fndecl
> +      && TYPE_ARG_TYPES (TREE_TYPE (fndecl))
> +      && TYPE_ARG_TYPES (gimple_call_fntype (stmt)))
>      {
> -      for (i = 0, p = parms; i < nargs; i++, p = TREE_CHAIN (p))
> +      unsigned int arg_idx = 0;
> +      for (tree f = TYPE_ARG_TYPES (TREE_TYPE (fndecl)),
> +	     s = TYPE_ARG_TYPES (gimple_call_fntype (stmt));
> +	   f || s;
> +	   f = TREE_CHAIN (f), s = TREE_CHAIN (s), arg_idx++)
>  	{
> +	  if (!f
> +	      || !s
> +	      || TREE_VALUE (f) == error_mark_node
> +	      || TREE_VALUE (s) == error_mark_node)
> +	    return false;
> +	  if (TREE_CODE (TREE_VALUE (f)) == VOID_TYPE)
> +	    {
> +	      if (TREE_CODE (TREE_VALUE (s)) != VOID_TYPE
> +		  || arg_idx != nargs)
> +		return false;
> +	      else
> +		break;
> +	    }
> +
>  	  tree arg;
> +
> +	  if (arg_idx >= nargs
> +	      || (arg = gimple_call_arg (stmt, arg_idx)) == error_mark_node)
> +	    return false;
> +
> +	  if (TREE_CODE (TREE_VALUE (s)) == VOID_TYPE
> +	      || (!types_compatible_p (TREE_VALUE (f), TREE_VALUE (s))
> +		  && !fold_convertible_p (TREE_VALUE (f), arg)))
> +	    return false;
> +	}
> +
> +      if (args_count_match && arg_idx != nargs)
> +	return false;
> +
> +      return true;
> +    }
> +
> +  /* If we only have the fntype extracted from the call statement, check it
> +     against the type of declarations while being pessimistic about
> +     promotions.  */
> +  tree p;
> +
> +  if (fndecl)
> +    p = TYPE_ARG_TYPES (TREE_TYPE (fndecl));
> +  else
> +    p = TYPE_ARG_TYPES (gimple_call_fntype (stmt));

This else case is bougs - you are then comparing the call arguments
against the call arguments...  Oh, I see it was there before :/

So it is that the FEs are expected to promote function arguments
according to the originally called function and that "ABI" is
recorded in gimple_call_fntype.  That means that we can either
look at the actual arguments or at TYPE_ARG_TYPES of
gimple_call_fntype.  But the fndecl ABI we want to verify
against is either its DECL_ARGUMENTS or TYPE_ARG_TYPEs of its type.

Verifying gimple_call_arg () against gimple_call_fntype ()
is pointless.  What should have been used here is

   else
     p = TYPE_ARG_TYPES (TREE_TYPE (gimple_call_fn (stmt)));

so, gimple_call_fn is the function called (if no fndecl then
this is a function pointer), thus look at the pointed-to type
and then its arguments.

Maybe you can test/fix that as independent commit.

Your second case

> +  if (fndecl
> +      && TYPE_ARG_TYPES (TREE_TYPE (fndecl))
> +      && TYPE_ARG_TYPES (gimple_call_fntype (stmt)))

then collapses with this and is also the better fallback IMHO
(so enter this case by using TYPE_ARG_TYPES (TREE_TYPE (gimple_call_fn 
(...))) instead of the fndecl).

Richard.

> +  if (p)
> +    {
> +      for (unsigned i = 0; i < nargs; i++, p = TREE_CHAIN (p))
> +	{
>  	  /* If this is a varargs function defer inlining decision
>  	     to callee.  */
>  	  if (!p)
>  	    break;
> -	  arg = gimple_call_arg (stmt, i);
> +	  tree arg = gimple_call_arg (stmt, i);
>  	  if (TREE_VALUE (p) == error_mark_node
>  	      || arg == error_mark_node
>  	      || TREE_CODE (TREE_VALUE (p)) == VOID_TYPE
> diff --git a/gcc/testsuite/g++.dg/lto/pr70929_0.C b/gcc/testsuite/g++.dg/lto/pr70929_0.C
> new file mode 100644
> index 00000000000..c96fb1c743a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lto/pr70929_0.C
> @@ -0,0 +1,18 @@
> +// { dg-lto-do run }
> +// { dg-lto-options { "-O3 -flto" } }
> +
> +struct s
> +{
> +  int a;
> +  s() {a=1;}
> +  ~s() {}
> +};
> +int t(struct s s);
> +int main()
> +{
> +  s s;
> +  int v=t(s);
> +  if (!__builtin_constant_p (v))
> +    __builtin_abort ();
> +  return 0;
> +}
> diff --git a/gcc/testsuite/g++.dg/lto/pr70929_1.C b/gcc/testsuite/g++.dg/lto/pr70929_1.C
> new file mode 100644
> index 00000000000..b33aa8f35f0
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lto/pr70929_1.C
> @@ -0,0 +1,10 @@
> +struct s
> +{
> +  int a;
> +  s() {a=1;}
> +  ~s() {}
> +};
> +int t(struct s s)
> +{
> +  return s.a;
> +}
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 247165 (AG München)