diff mbox series

[PR,70929] IPA call type compatibility fix/workaround

Message ID ri67e5fyyf5.fsf@suse.cz
State New
Headers show
Series [PR,70929] IPA call type compatibility fix/workaround | expand

Commit Message

Martin Jambor Oct. 8, 2019, 4:50 p.m. UTC
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

Comments

Richard Biener Oct. 9, 2019, 8:01 a.m. UTC | #1
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;
> +}
>
Martin Jambor Oct. 10, 2019, 10:06 a.m. UTC | #2
Hi,

On Wed, Oct 09 2019, Richard Biener wrote:
>>

...

>> +  /* 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 :/

Right, and one hand I noticed id did not make much sense, on the other
there were few cases where it was necessary to make the new predicate as
permissive as the old one (not that any of those that I saw looked
interesting).

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

OK, this is a very nice idea, I have made the change in the patch.

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

The fndecl here is not the decl extracted from the gimple statement.  It
is received as a function parameter and two callers extract it from a
call graph edge callee and one - speculation resolution - even from the
ipa reference associated with the speculation.  So I don't think th
should be replaced.

So, is the following OK (bootstrapped and tested on x86_64-linux,  no
LTO bootstrap this time because of PR 92037)?

Martin


2019-10-09  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..4f7bfa28f37 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 (TREE_TYPE (gimple_call_fn (stmt)));
+  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 Oct. 10, 2019, 1:06 p.m. UTC | #3
Now also to the list...

On Thu, 10 Oct 2019, Richard Biener wrote:

> On Thu, 10 Oct 2019, Martin Jambor wrote:
> 
> > Hi,
> > 
> > On Wed, Oct 09 2019, Richard Biener wrote:
> > >>
> > 
> > ...
> > 
> > >> +  /* 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 :/
> > 
> > Right, and one hand I noticed id did not make much sense, on the other
> > there were few cases where it was necessary to make the new predicate as
> > permissive as the old one (not that any of those that I saw looked
> > interesting).
> > 
> > >
> > > 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.
> > 
> > OK, this is a very nice idea, I have made the change in the patch.
> > 
> > >
> > > 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).
> > >
> > 
> > The fndecl here is not the decl extracted from the gimple statement.  It
> > is received as a function parameter and two callers extract it from a
> > call graph edge callee and one - speculation resolution - even from the
> > ipa reference associated with the speculation.  So I don't think th
> > should be replaced.
> 
> Hmm, OK.  But then the code cares for fndecl == NULL which as far as
> I can see should not happen.  And in that case it does something
> completely different, so...
> 
> > So, is the following OK (bootstrapped and tested on x86_64-linux,  no
> > LTO bootstrap this time because of PR 92037)?
> > 
> > Martin
> > 
> > 
> > 2019-10-09  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..4f7bfa28f37 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)))
> >      {
> 
> You might want to check the result of this against the a simple
> types_compatible_p (TREE_TYPE (fndecl), 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;
> 
> The code below doeesn't make any sense to me for the !fndecl case.
> 
> I'm not sure if we really need to handle the apples-to-oranges
> case (comparing unpromoted types against promoted decls).
> 
> Btw, I questioned whether we need all this code anyways - we mostly
> have it for QOI (not break things if the ABI makes things magically
> "work").
> 
> > +  if (fndecl)
> > +    p = TYPE_ARG_TYPES (TREE_TYPE (fndecl));
> > +  else
> > +    p = TYPE_ARG_TYPES (TREE_TYPE (gimple_call_fn (stmt)));
> > +  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;
> > +}
> > 
> 
>
diff mbox series

Patch

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));
+  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;
+}