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 <mjam...@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 <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 247165 (AG München)
