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