[GOOGLE] More strict checking for call args

[GOOGLE] More strict checking for call args

[Dehao Chen]

This patch makes more strict check of call args to make sure the
number of args match.

Bootstrapped and passed regression tests.

OK for google branches?

Thanks,
Dehao

Index: gcc/gimple-low.c
===================================================================
--- gcc/gimple-low.c (revision 199414)
+++ gcc/gimple-low.c (working copy)
@@ -254,9 +254,13 @@ gimple_check_call_args (gimple stmt, tree fndecl)
   && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
             return false;
  }
+      if (p != NULL)
+ return false;
     }
   else if (parms)
     {
+      if (list_length (parms) - nargs != 1)
+ return false;
       for (i = 0, p = parms; i < nargs; i++, p = TREE_CHAIN (p))
  {
   tree arg;

Re: [GOOGLE] More strict checking for call args

[Xinliang David Li]

On Thu, May 30, 2013 at 3:47 PM, Dehao Chen <dehao@google.com> wrote:
> This patch makes more strict check of call args to make sure the
> number of args match.
>
> Bootstrapped and passed regression tests.
>
> OK for google branches?
>
> Thanks,
> Dehao
>
> Index: gcc/gimple-low.c
> ===================================================================
> --- gcc/gimple-low.c (revision 199414)
> +++ gcc/gimple-low.c (working copy)
> @@ -254,9 +254,13 @@ gimple_check_call_args (gimple stmt, tree fndecl)
>    && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
>              return false;
>   }
> +      if (p != NULL)
> + return false;
>      }
>    else if (parms)
>      {
> +      if (list_length (parms) - nargs != 1)
> + return false;

This does not seem to be correct for vararg functions.

David


>        for (i = 0, p = parms; i < nargs; i++, p = TREE_CHAIN (p))
>   {
>    tree arg;

Re: [GOOGLE] More strict checking for call args

[Dehao Chen]

Yes, patch updated:

Testing on-going.

Dehao

Index: gimple-low.c
===================================================================
--- gimple-low.c (revision 199414)
+++ gimple-low.c (working copy)
@@ -254,6 +254,8 @@
   && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
             return false;
  }
+      if (p != NULL)
+ return false;
     }
   else if (parms)
     {

On Thu, May 30, 2013 at 4:10 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Thu, May 30, 2013 at 3:47 PM, Dehao Chen <dehao@google.com> wrote:
>> This patch makes more strict check of call args to make sure the
>> number of args match.
>>
>> Bootstrapped and passed regression tests.
>>
>> OK for google branches?
>>
>> Thanks,
>> Dehao
>>
>> Index: gcc/gimple-low.c
>> ===================================================================
>> --- gcc/gimple-low.c (revision 199414)
>> +++ gcc/gimple-low.c (working copy)
>> @@ -254,9 +254,13 @@ gimple_check_call_args (gimple stmt, tree fndecl)
>>    && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
>>              return false;
>>   }
>> +      if (p != NULL)
>> + return false;
>>      }
>>    else if (parms)
>>      {
>> +      if (list_length (parms) - nargs != 1)
>> + return false;
>
> This does not seem to be correct for vararg functions.
>
> David
>
>
>>        for (i = 0, p = parms; i < nargs; i++, p = TREE_CHAIN (p))
>>   {
>>    tree arg;

Re: [GOOGLE] More strict checking for call args

[Duncan Sands]

Hi Dehao,

On 31/05/13 00:47, Dehao Chen wrote:
> This patch makes more strict check of call args to make sure the
> number of args match.
>
> Bootstrapped and passed regression tests.

did you thoroughly test Fortran?  The Fortran front-end has long had an
unfortunate tendency to eg declare a function as taking 4 int arguments,
but in the call pass it one argument (an array of length 4, consisting
of ints).  It would be great if all such nastiness has been fixed.  There
are also a few cases in which it declares a builtin as taking, say, an
int,float pair, but passes a float,int pair in the call.  I fixed a couple
of instances of this a while back, but I still have one outstanding patch.

Ciao, Duncan.

>
> OK for google branches?
>
> Thanks,
> Dehao
>
> Index: gcc/gimple-low.c
> ===================================================================
> --- gcc/gimple-low.c (revision 199414)
> +++ gcc/gimple-low.c (working copy)
> @@ -254,9 +254,13 @@ gimple_check_call_args (gimple stmt, tree fndecl)
>     && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
>               return false;
>    }
> +      if (p != NULL)
> + return false;
>       }
>     else if (parms)
>       {
> +      if (list_length (parms) - nargs != 1)
> + return false;
>         for (i = 0, p = parms; i < nargs; i++, p = TREE_CHAIN (p))
>    {
>     tree arg;
>

Re: [GOOGLE] More strict checking for call args

[Xinliang David Li]

Those cases you mentioned may lead to problems in inlining, indirect
target promotion transformations etc too, so it is better to use the
new guard against them.

David

On Fri, May 31, 2013 at 1:15 AM, Duncan Sands <baldrick@free.fr> wrote:
> Hi Dehao,
>
>
> On 31/05/13 00:47, Dehao Chen wrote:
>>
>> This patch makes more strict check of call args to make sure the
>> number of args match.
>>
>> Bootstrapped and passed regression tests.
>
>
> did you thoroughly test Fortran?  The Fortran front-end has long had an
> unfortunate tendency to eg declare a function as taking 4 int arguments,
> but in the call pass it one argument (an array of length 4, consisting
> of ints).  It would be great if all such nastiness has been fixed.  There
> are also a few cases in which it declares a builtin as taking, say, an
> int,float pair, but passes a float,int pair in the call.  I fixed a couple
> of instances of this a while back, but I still have one outstanding patch.
>
> Ciao, Duncan.
>
>
>>
>> OK for google branches?
>>
>> Thanks,
>> Dehao
>>
>> Index: gcc/gimple-low.c
>> ===================================================================
>> --- gcc/gimple-low.c (revision 199414)
>> +++ gcc/gimple-low.c (working copy)
>> @@ -254,9 +254,13 @@ gimple_check_call_args (gimple stmt, tree fndecl)
>>     && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
>>               return false;
>>    }
>> +      if (p != NULL)
>> + return false;
>>       }
>>     else if (parms)
>>       {
>> +      if (list_length (parms) - nargs != 1)
>> + return false;
>>         for (i = 0, p = parms; i < nargs; i++, p = TREE_CHAIN (p))
>>    {
>>     tree arg;
>>
>

Re: [GOOGLE] More strict checking for call args

[Dehao Chen]

Hi,

This patch was committed to google branch. But I think it is of
general interest. So is it ok for trunk?

Thanks,
Dehao

gcc/ChangeLog:

2013-06-03  Dehao Chen  <dehao@google.com>

*gimple-low.c (gimple_check_call_args): Restrict the call_arg check to
contain same number of args.

Index: gcc/gimple-low.c
===================================================================
--- gcc/gimple-low.c (revision 199570)
+++ gcc/gimple-low.c (working copy)
@@ -243,6 +243,8 @@ gimple_check_call_args (gimple stmt, tree fndecl)
   && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
             return false;
  }
+      if (p != NULL)
+ return false;
     }
   else if (parms)
     {

Re: [GOOGLE] More strict checking for call args

[Richard Biener]

On Tue, Jun 4, 2013 at 2:55 AM, Dehao Chen <dehao@google.com> wrote:
> Hi,
>
> This patch was committed to google branch. But I think it is of
> general interest. So is it ok for trunk?
>
> Thanks,
> Dehao
>
> gcc/ChangeLog:
>
> 2013-06-03  Dehao Chen  <dehao@google.com>
>
> *gimple-low.c (gimple_check_call_args): Restrict the call_arg check to
> contain same number of args.
>
> Index: gcc/gimple-low.c
> ===================================================================
> --- gcc/gimple-low.c (revision 199570)
> +++ gcc/gimple-low.c (working copy)
> @@ -243,6 +243,8 @@ gimple_check_call_args (gimple stmt, tree fndecl)
>    && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
>              return false;
>   }
> +      if (p != NULL)
> + return false;

Please add a comment here, like

         /* Not enough parameters to the function call.  */
         if (p != NULL)
           return false;

note that I believe we can deal with this situation just fine during inlining,
we just leave the parameters uninitialized.

So - why do you think the test is a good idea?  The whole function should
ideally be not necessary and is just there to avoid situations we cannot
deal with during inlining.

Richard.

>      }
>    else if (parms)
>      {

Re: [GOOGLE] More strict checking for call args

[Dehao Chen]

On Tue, Jun 4, 2013 at 3:56 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Tue, Jun 4, 2013 at 2:55 AM, Dehao Chen <dehao@google.com> wrote:
> > Hi,
> >
> > This patch was committed to google branch. But I think it is of
> > general interest. So is it ok for trunk?
> >
> > Thanks,
> > Dehao
> >
> > gcc/ChangeLog:
> >
> > 2013-06-03  Dehao Chen  <dehao@google.com>
> >
> > *gimple-low.c (gimple_check_call_args): Restrict the call_arg check to
> > contain same number of args.
> >
> > Index: gcc/gimple-low.c
> > ===================================================================
> > --- gcc/gimple-low.c (revision 199570)
> > +++ gcc/gimple-low.c (working copy)
> > @@ -243,6 +243,8 @@ gimple_check_call_args (gimple stmt, tree fndecl)
> >    && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
> >              return false;
> >   }
> > +      if (p != NULL)
> > + return false;
>
> Please add a comment here, like
>
>          /* Not enough parameters to the function call.  */
>          if (p != NULL)
>            return false;
>
> note that I believe we can deal with this situation just fine during inlining,
> we just leave the parameters uninitialized.
>
> So - why do you think the test is a good idea?  The whole function should
> ideally be not necessary and is just there to avoid situations we cannot
> deal with during inlining.

This check is necessary when profile does not match. E.g. if an
indirect call target profile is targeting to an incorrect callee, this
patch can make sure it's verified before actually transforming code.
This could happen is you use an out-of-date profile to optimize for
new code.

Dehao



>
> Richard.
>
> >      }
> >    else if (parms)
> >      {

Re: [GOOGLE] More strict checking for call args

[Xinliang David Li]

Richard's question is that inlining should deal with extra arguments
just fine -- those paths (the insane profile case) won't be executed
anyway. Do you have a case showing otherwise (i.e., the mismatch
upsets the compiler?)

David


On Tue, Jun 4, 2013 at 8:07 AM, Dehao Chen <dehao@google.com> wrote:
> On Tue, Jun 4, 2013 at 3:56 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>>
>> On Tue, Jun 4, 2013 at 2:55 AM, Dehao Chen <dehao@google.com> wrote:
>> > Hi,
>> >
>> > This patch was committed to google branch. But I think it is of
>> > general interest. So is it ok for trunk?
>> >
>> > Thanks,
>> > Dehao
>> >
>> > gcc/ChangeLog:
>> >
>> > 2013-06-03  Dehao Chen  <dehao@google.com>
>> >
>> > *gimple-low.c (gimple_check_call_args): Restrict the call_arg check to
>> > contain same number of args.
>> >
>> > Index: gcc/gimple-low.c
>> > ===================================================================
>> > --- gcc/gimple-low.c (revision 199570)
>> > +++ gcc/gimple-low.c (working copy)
>> > @@ -243,6 +243,8 @@ gimple_check_call_args (gimple stmt, tree fndecl)
>> >    && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
>> >              return false;
>> >   }
>> > +      if (p != NULL)
>> > + return false;
>>
>> Please add a comment here, like
>>
>>          /* Not enough parameters to the function call.  */
>>          if (p != NULL)
>>            return false;
>>
>> note that I believe we can deal with this situation just fine during inlining,
>> we just leave the parameters uninitialized.
>>
>> So - why do you think the test is a good idea?  The whole function should
>> ideally be not necessary and is just there to avoid situations we cannot
>> deal with during inlining.
>
> This check is necessary when profile does not match. E.g. if an
> indirect call target profile is targeting to an incorrect callee, this
> patch can make sure it's verified before actually transforming code.
> This could happen is you use an out-of-date profile to optimize for
> new code.
>
> Dehao
>
>
>
>>
>> Richard.
>>
>> >      }
>> >    else if (parms)
>> >      {

Re: [GOOGLE] More strict checking for call args

[Dehao Chen]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]

attached is a testcase that would cause problem when source has changed:

$ g++ test.cc -O2 -fprofile-generate -DOLD
$ ./a.out
$ g++ test.cc -O2 -fprofile-use
test.cc:34:1: internal compiler error: in operator[], at vec.h:815
 }
 ^
0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
../../gcc/vec.h:815
0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
../../gcc/vec.h:1244
0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
../../gcc/vec.h:815
0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
../../gcc/vec.h:1244
0xf24464 ipa_get_indirect_edge_target_1
../../gcc/ipa-cp.c:1535
0x971b9a estimate_edge_devirt_benefit
../../gcc/ipa-inline-analysis.c:2757
0x973f59 estimate_edge_size_and_time
../../gcc/ipa-inline-analysis.c:2789
0x973f59 estimate_calls_size_and_time
../../gcc/ipa-inline-analysis.c:2842
0x97429f estimate_node_size_and_time
../../gcc/ipa-inline-analysis.c:2929
0x976077 do_estimate_edge_size(cgraph_edge*)
../../gcc/ipa-inline-analysis.c:3472
0x97614f estimate_edge_size
../../gcc/ipa-inline.h:274
0x97614f estimate_edge_growth
../../gcc/ipa-inline.h:286
0x97614f do_estimate_growth_1
../../gcc/ipa-inline-analysis.c:3582
0x7e41df cgraph_for_node_and_aliases
../../gcc/cgraph.c:1777
0x976675 do_estimate_growth(cgraph_node*)
../../gcc/ipa-inline-analysis.c:3596
0xf314ea estimate_growth
../../gcc/ipa-inline.h:261
0xf314ea inline_small_functions
../../gcc/ipa-inline.c:1432
0xf314ea ipa_inline
../../gcc/ipa-inline.c:1797
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <http://gcc.gnu.org/bugs.html> for instructions.

[-- Attachment #2: test.cc --]
[-- Type: application/octet-stream, Size: 565 bytes --]

struct base {
  virtual int bar(int a)=0;
};

struct derived : base {
  derived() {}
  virtual int bar(int a) { return a + 1; }
};

int func_old(int j, int k, base *ptr) {
  return j + k + ptr->bar(10);
}

int func_new(base *ptr) {
  return ptr->bar(10);
}

int (*fptr_old)(int j, int k, base *ptr);
int (*fptr_new)(base *ptr);

int main() {
  derived d;
  base *ptr = &d;
  fptr_old = func_old;
  fptr_new = func_new;
  int ret = 0;
  for (int i = 0; i < 1000; i++)
#ifdef OLD
    ret += fptr_old(i, i, ptr);
#else
    ret += fptr_new(ptr);
#endif
  return ret;
}

Re: [GOOGLE] More strict checking for call args

[Richard Biener]

On Wed, Jun 5, 2013 at 2:19 AM, Dehao Chen <dehao@google.com> wrote:
> attached is a testcase that would cause problem when source has changed:
>
> $ g++ test.cc -O2 -fprofile-generate -DOLD
> $ ./a.out
> $ g++ test.cc -O2 -fprofile-use
> test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>  }
>  ^
> 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
> ../../gcc/vec.h:815
> 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
> ../../gcc/vec.h:1244
> 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
> ../../gcc/vec.h:815
> 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
> ../../gcc/vec.h:1244
> 0xf24464 ipa_get_indirect_edge_target_1
> ../../gcc/ipa-cp.c:1535

This use needs to be properly guarded.  We can perfectly well have
mismatching fndecl nodes in gimple calls.  If we start with

void fn(int, int, int);

...
void (*x)(float, double, struct X, int) = fn;
(*x)(1., 2., {}, 1);

the GIMPLE_CALL receives the function type effective for the call
from the source (gimple_call_fntype).  Then CCP happily propagates the
'fn' decl and we end up with

  fn (1., 2., {}, 1);

that is, gimple_call_fndecl is 'fn' but gimple_call_fntype is still
void (*x)(float, double, struct X, int)!

So the solution is not to fix the argument verification predicate but to make
code aware of the fact that for the call statement gimple_call_fntype is
relevant for what is a valid call (that's also what is verified against in
verify_stmts) even though the ultimate called function-decl 'fn' has a
different prototype.  Thus any code propagating from a call site to
the callee has to deal with mismatches.

Richard.

> 0x971b9a estimate_edge_devirt_benefit
> ../../gcc/ipa-inline-analysis.c:2757
> 0x973f59 estimate_edge_size_and_time
> ../../gcc/ipa-inline-analysis.c:2789
> 0x973f59 estimate_calls_size_and_time
> ../../gcc/ipa-inline-analysis.c:2842
> 0x97429f estimate_node_size_and_time
> ../../gcc/ipa-inline-analysis.c:2929
> 0x976077 do_estimate_edge_size(cgraph_edge*)
> ../../gcc/ipa-inline-analysis.c:3472
> 0x97614f estimate_edge_size
> ../../gcc/ipa-inline.h:274
> 0x97614f estimate_edge_growth
> ../../gcc/ipa-inline.h:286
> 0x97614f do_estimate_growth_1
> ../../gcc/ipa-inline-analysis.c:3582
> 0x7e41df cgraph_for_node_and_aliases
> ../../gcc/cgraph.c:1777
> 0x976675 do_estimate_growth(cgraph_node*)
> ../../gcc/ipa-inline-analysis.c:3596
> 0xf314ea estimate_growth
> ../../gcc/ipa-inline.h:261
> 0xf314ea inline_small_functions
> ../../gcc/ipa-inline.c:1432
> 0xf314ea ipa_inline
> ../../gcc/ipa-inline.c:1797
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See <http://gcc.gnu.org/bugs.html> for instructions.

Re: [GOOGLE] More strict checking for call args

[Xinliang David Li]

Right, except that in the context of FDO/autoFDO, where this happens
the most (note in FDO case, it can happen with fresh profile too for
multi-threaded programs), it is not that important to handle -- the
mismatch path will never be executed, so why bother to inline and
bloat the code for it?

if (fptr_new == func_old) {
      func_old (ptr);             <--- do not want to inline.
}
else
   fptr_new(ptr);

David


On Wed, Jun 5, 2013 at 1:31 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, Jun 5, 2013 at 2:19 AM, Dehao Chen <dehao@google.com> wrote:
>> attached is a testcase that would cause problem when source has changed:
>>
>> $ g++ test.cc -O2 -fprofile-generate -DOLD
>> $ ./a.out
>> $ g++ test.cc -O2 -fprofile-use
>> test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>  }
>>  ^
>> 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>> ../../gcc/vec.h:815
>> 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>> ../../gcc/vec.h:1244
>> 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>> ../../gcc/vec.h:815
>> 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>> ../../gcc/vec.h:1244
>> 0xf24464 ipa_get_indirect_edge_target_1
>> ../../gcc/ipa-cp.c:1535
>
> This use needs to be properly guarded.  We can perfectly well have
> mismatching fndecl nodes in gimple calls.  If we start with
>
> void fn(int, int, int);
>
> ...
> void (*x)(float, double, struct X, int) = fn;
> (*x)(1., 2., {}, 1);
>
> the GIMPLE_CALL receives the function type effective for the call
> from the source (gimple_call_fntype).  Then CCP happily propagates the
> 'fn' decl and we end up with
>
>   fn (1., 2., {}, 1);
>
> that is, gimple_call_fndecl is 'fn' but gimple_call_fntype is still
> void (*x)(float, double, struct X, int)!
>
> So the solution is not to fix the argument verification predicate but to make
> code aware of the fact that for the call statement gimple_call_fntype is
> relevant for what is a valid call (that's also what is verified against in
> verify_stmts) even though the ultimate called function-decl 'fn' has a
> different prototype.  Thus any code propagating from a call site to
> the callee has to deal with mismatches.
>
> Richard.
>
>> 0x971b9a estimate_edge_devirt_benefit
>> ../../gcc/ipa-inline-analysis.c:2757
>> 0x973f59 estimate_edge_size_and_time
>> ../../gcc/ipa-inline-analysis.c:2789
>> 0x973f59 estimate_calls_size_and_time
>> ../../gcc/ipa-inline-analysis.c:2842
>> 0x97429f estimate_node_size_and_time
>> ../../gcc/ipa-inline-analysis.c:2929
>> 0x976077 do_estimate_edge_size(cgraph_edge*)
>> ../../gcc/ipa-inline-analysis.c:3472
>> 0x97614f estimate_edge_size
>> ../../gcc/ipa-inline.h:274
>> 0x97614f estimate_edge_growth
>> ../../gcc/ipa-inline.h:286
>> 0x97614f do_estimate_growth_1
>> ../../gcc/ipa-inline-analysis.c:3582
>> 0x7e41df cgraph_for_node_and_aliases
>> ../../gcc/cgraph.c:1777
>> 0x976675 do_estimate_growth(cgraph_node*)
>> ../../gcc/ipa-inline-analysis.c:3596
>> 0xf314ea estimate_growth
>> ../../gcc/ipa-inline.h:261
>> 0xf314ea inline_small_functions
>> ../../gcc/ipa-inline.c:1432
>> 0xf314ea ipa_inline
>> ../../gcc/ipa-inline.c:1797
>> Please submit a full bug report,
>> with preprocessed source if appropriate.
>> Please include the complete backtrace with any bug report.
>> See <http://gcc.gnu.org/bugs.html> for instructions.

Re: [GOOGLE] More strict checking for call args

[Richard Biener]

On Wed, Jun 5, 2013 at 6:11 PM, Xinliang David Li <davidxl@google.com> wrote:
> Right, except that in the context of FDO/autoFDO, where this happens
> the most (note in FDO case, it can happen with fresh profile too for
> multi-threaded programs), it is not that important to handle -- the
> mismatch path will never be executed, so why bother to inline and
> bloat the code for it?

It's about being able to IPA-CP through such calls.  Consider

void foo (int);
void bar (int i, float)
{
  foo (i);
}
void foobar ()
{
  bar (1);  // mismatched # of arguments but we can see foo() is
called with constant 1
}

that's important if we can prove that all calls to foo () are called
with a constant 1
argument for example and thus we can replace it without cloning.

If the compatibility predicate guards even the IPA-CP case (not just the
inlining itself) then the predicate is used in a bogus way.

So your symptom is not properly fixed with the patch but papered over.

Richard.

> if (fptr_new == func_old) {
>       func_old (ptr);             <--- do not want to inline.
> }
> else
>    fptr_new(ptr);
>
> David
>
>
> On Wed, Jun 5, 2013 at 1:31 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Jun 5, 2013 at 2:19 AM, Dehao Chen <dehao@google.com> wrote:
>>> attached is a testcase that would cause problem when source has changed:
>>>
>>> $ g++ test.cc -O2 -fprofile-generate -DOLD
>>> $ ./a.out
>>> $ g++ test.cc -O2 -fprofile-use
>>> test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>>  }
>>>  ^
>>> 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>> ../../gcc/vec.h:815
>>> 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>> ../../gcc/vec.h:1244
>>> 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>> ../../gcc/vec.h:815
>>> 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>> ../../gcc/vec.h:1244
>>> 0xf24464 ipa_get_indirect_edge_target_1
>>> ../../gcc/ipa-cp.c:1535
>>
>> This use needs to be properly guarded.  We can perfectly well have
>> mismatching fndecl nodes in gimple calls.  If we start with
>>
>> void fn(int, int, int);
>>
>> ...
>> void (*x)(float, double, struct X, int) = fn;
>> (*x)(1., 2., {}, 1);
>>
>> the GIMPLE_CALL receives the function type effective for the call
>> from the source (gimple_call_fntype).  Then CCP happily propagates the
>> 'fn' decl and we end up with
>>
>>   fn (1., 2., {}, 1);
>>
>> that is, gimple_call_fndecl is 'fn' but gimple_call_fntype is still
>> void (*x)(float, double, struct X, int)!
>>
>> So the solution is not to fix the argument verification predicate but to make
>> code aware of the fact that for the call statement gimple_call_fntype is
>> relevant for what is a valid call (that's also what is verified against in
>> verify_stmts) even though the ultimate called function-decl 'fn' has a
>> different prototype.  Thus any code propagating from a call site to
>> the callee has to deal with mismatches.
>>
>> Richard.
>>
>>> 0x971b9a estimate_edge_devirt_benefit
>>> ../../gcc/ipa-inline-analysis.c:2757
>>> 0x973f59 estimate_edge_size_and_time
>>> ../../gcc/ipa-inline-analysis.c:2789
>>> 0x973f59 estimate_calls_size_and_time
>>> ../../gcc/ipa-inline-analysis.c:2842
>>> 0x97429f estimate_node_size_and_time
>>> ../../gcc/ipa-inline-analysis.c:2929
>>> 0x976077 do_estimate_edge_size(cgraph_edge*)
>>> ../../gcc/ipa-inline-analysis.c:3472
>>> 0x97614f estimate_edge_size
>>> ../../gcc/ipa-inline.h:274
>>> 0x97614f estimate_edge_growth
>>> ../../gcc/ipa-inline.h:286
>>> 0x97614f do_estimate_growth_1
>>> ../../gcc/ipa-inline-analysis.c:3582
>>> 0x7e41df cgraph_for_node_and_aliases
>>> ../../gcc/cgraph.c:1777
>>> 0x976675 do_estimate_growth(cgraph_node*)
>>> ../../gcc/ipa-inline-analysis.c:3596
>>> 0xf314ea estimate_growth
>>> ../../gcc/ipa-inline.h:261
>>> 0xf314ea inline_small_functions
>>> ../../gcc/ipa-inline.c:1432
>>> 0xf314ea ipa_inline
>>> ../../gcc/ipa-inline.c:1797
>>> Please submit a full bug report,
>>> with preprocessed source if appropriate.
>>> Please include the complete backtrace with any bug report.
>>> See <http://gcc.gnu.org/bugs.html> for instructions.

Re: [GOOGLE] More strict checking for call args

[Martin Jambor]

Hi,

On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
> attached is a testcase that would cause problem when source has changed:
> 
> $ g++ test.cc -O2 -fprofile-generate -DOLD
> $ ./a.out
> $ g++ test.cc -O2 -fprofile-use
> test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>  }
>  ^
> 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
> ../../gcc/vec.h:815
> 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
> ../../gcc/vec.h:1244
> 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
> ../../gcc/vec.h:815
> 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
> ../../gcc/vec.h:1244
> 0xf24464 ipa_get_indirect_edge_target_1
> ../../gcc/ipa-cp.c:1535
> 0x971b9a estimate_edge_devirt_benefit
> ../../gcc/ipa-inline-analysis.c:2757

Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
Since it is called also from inlining, we can have parameter count
mismatches... and in fact in non-virtual paths of that function we do
check that we don't.  Because all callers have to pass known_vals
describing all formal parameters of the inline tree root, we should
apply the fix below (I've only just started running a bootstrap and
testsuite on x86_64, though).

OTOH, while I understand that FDO can change inlining sufficiently so
that this error occurs, IMHO this should not be caused by outdated
profiles but there is somewhere a parameter mismatch in the source.

Dehao, can you please check that this patch helps?

Richi, if it does and the patch passes bootstrap and tests, is it OK
for trunk and 4.8 branch?

Thanks and sorry for the trouble,

Martin


2013-06-06  Martin Jambor  <mjambor@suse.cz>

	* ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
	within bounds at the beginning of the function.

Index: src/gcc/ipa-cp.c
===================================================================
--- src.orig/gcc/ipa-cp.c
+++ src/gcc/ipa-cp.c
@@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
   tree otr_type;
   tree t;
 
-  if (param_index == -1)
+  if (param_index == -1
+      || known_vals.length () <= (unsigned int) param_index)
     return NULL_TREE;
 
   if (!ie->indirect_info->polymorphic)
@@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
 	    t = NULL;
 	}
       else
-	t = (known_vals.length () > (unsigned int) param_index
-	     ? known_vals[param_index] : NULL);
+	t = NULL;
 
       if (t &&
 	  TREE_CODE (t) == ADDR_EXPR

Re: [GOOGLE] More strict checking for call args

[Richard Biener]

On Thu, Jun 6, 2013 at 4:11 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
>> attached is a testcase that would cause problem when source has changed:
>>
>> $ g++ test.cc -O2 -fprofile-generate -DOLD
>> $ ./a.out
>> $ g++ test.cc -O2 -fprofile-use
>> test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>  }
>>  ^
>> 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>> ../../gcc/vec.h:815
>> 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>> ../../gcc/vec.h:1244
>> 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>> ../../gcc/vec.h:815
>> 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>> ../../gcc/vec.h:1244
>> 0xf24464 ipa_get_indirect_edge_target_1
>> ../../gcc/ipa-cp.c:1535
>> 0x971b9a estimate_edge_devirt_benefit
>> ../../gcc/ipa-inline-analysis.c:2757
>
> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
> Since it is called also from inlining, we can have parameter count
> mismatches... and in fact in non-virtual paths of that function we do
> check that we don't.  Because all callers have to pass known_vals
> describing all formal parameters of the inline tree root, we should
> apply the fix below (I've only just started running a bootstrap and
> testsuite on x86_64, though).
>
> OTOH, while I understand that FDO can change inlining sufficiently so
> that this error occurs, IMHO this should not be caused by outdated
> profiles but there is somewhere a parameter mismatch in the source.
>
> Dehao, can you please check that this patch helps?
>
> Richi, if it does and the patch passes bootstrap and tests, is it OK
> for trunk and 4.8 branch?

Yes.

Thanks,
Richard.

> Thanks and sorry for the trouble,
>
> Martin
>
>
> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>
>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>         within bounds at the beginning of the function.
>
> Index: src/gcc/ipa-cp.c
> ===================================================================
> --- src.orig/gcc/ipa-cp.c
> +++ src/gcc/ipa-cp.c
> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>    tree otr_type;
>    tree t;
>
> -  if (param_index == -1)
> +  if (param_index == -1
> +      || known_vals.length () <= (unsigned int) param_index)
>      return NULL_TREE;
>
>    if (!ie->indirect_info->polymorphic)
> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>             t = NULL;
>         }
>        else
> -       t = (known_vals.length () > (unsigned int) param_index
> -            ? known_vals[param_index] : NULL);
> +       t = NULL;
>
>        if (t &&
>           TREE_CODE (t) == ADDR_EXPR

Re: [GOOGLE] More strict checking for call args

[Dehao Chen]

Hi, Martin,

Yes, your patch can fix my case. Thanks a lot for the fix.

With the fix, value profiling will still promote the wrong indirect
call target. Though it will not be inlining, but it results in an
additional check. How about in check_ic_target, after calling
gimple_check_call_matching_types, we also check if number of args
match number of params in target->symbol.decl?

Thanks,
Dehao


On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjambor@suse.cz> wrote:
>
> Hi,
>
> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
> > attached is a testcase that would cause problem when source has changed:
> >
> > $ g++ test.cc -O2 -fprofile-generate -DOLD
> > $ ./a.out
> > $ g++ test.cc -O2 -fprofile-use
> > test.cc:34:1: internal compiler error: in operator[], at vec.h:815
> >  }
> >  ^
> > 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
> > ../../gcc/vec.h:815
> > 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
> > ../../gcc/vec.h:1244
> > 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
> > ../../gcc/vec.h:815
> > 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
> > ../../gcc/vec.h:1244
> > 0xf24464 ipa_get_indirect_edge_target_1
> > ../../gcc/ipa-cp.c:1535
> > 0x971b9a estimate_edge_devirt_benefit
> > ../../gcc/ipa-inline-analysis.c:2757
>
> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
> Since it is called also from inlining, we can have parameter count
> mismatches... and in fact in non-virtual paths of that function we do
> check that we don't.  Because all callers have to pass known_vals
> describing all formal parameters of the inline tree root, we should
> apply the fix below (I've only just started running a bootstrap and
> testsuite on x86_64, though).
>
> OTOH, while I understand that FDO can change inlining sufficiently so
> that this error occurs, IMHO this should not be caused by outdated
> profiles but there is somewhere a parameter mismatch in the source.
>
> Dehao, can you please check that this patch helps?
>
> Richi, if it does and the patch passes bootstrap and tests, is it OK
> for trunk and 4.8 branch?
>
> Thanks and sorry for the trouble,
>
> Martin
>
>
> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>
>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>         within bounds at the beginning of the function.
>
> Index: src/gcc/ipa-cp.c
> ===================================================================
> --- src.orig/gcc/ipa-cp.c
> +++ src/gcc/ipa-cp.c
> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>    tree otr_type;
>    tree t;
>
> -  if (param_index == -1)
> +  if (param_index == -1
> +      || known_vals.length () <= (unsigned int) param_index)
>      return NULL_TREE;
>
>    if (!ie->indirect_info->polymorphic)
> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>             t = NULL;
>         }
>        else
> -       t = (known_vals.length () > (unsigned int) param_index
> -            ? known_vals[param_index] : NULL);
> +       t = NULL;
>
>        if (t &&
>           TREE_CODE (t) == ADDR_EXPR

Re: [GOOGLE] More strict checking for call args

[Xinliang David Li]

gimple_check_call_matching_types is called by check_ic_target. Instead
of moving the check out of this guard function, may be enhancing the
interface to allow it to guard with different strictness?

David

On Thu, Jun 6, 2013 at 8:10 AM, Dehao Chen <dehao@google.com> wrote:
> Hi, Martin,
>
> Yes, your patch can fix my case. Thanks a lot for the fix.
>
> With the fix, value profiling will still promote the wrong indirect
> call target. Though it will not be inlining, but it results in an
> additional check. How about in check_ic_target, after calling
> gimple_check_call_matching_types, we also check if number of args
> match number of params in target->symbol.decl?
>
> Thanks,
> Dehao
>
>
> On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>
>> Hi,
>>
>> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
>> > attached is a testcase that would cause problem when source has changed:
>> >
>> > $ g++ test.cc -O2 -fprofile-generate -DOLD
>> > $ ./a.out
>> > $ g++ test.cc -O2 -fprofile-use
>> > test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>> >  }
>> >  ^
>> > 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>> > ../../gcc/vec.h:815
>> > 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>> > ../../gcc/vec.h:1244
>> > 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>> > ../../gcc/vec.h:815
>> > 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>> > ../../gcc/vec.h:1244
>> > 0xf24464 ipa_get_indirect_edge_target_1
>> > ../../gcc/ipa-cp.c:1535
>> > 0x971b9a estimate_edge_devirt_benefit
>> > ../../gcc/ipa-inline-analysis.c:2757
>>
>> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
>> Since it is called also from inlining, we can have parameter count
>> mismatches... and in fact in non-virtual paths of that function we do
>> check that we don't.  Because all callers have to pass known_vals
>> describing all formal parameters of the inline tree root, we should
>> apply the fix below (I've only just started running a bootstrap and
>> testsuite on x86_64, though).
>>
>> OTOH, while I understand that FDO can change inlining sufficiently so
>> that this error occurs, IMHO this should not be caused by outdated
>> profiles but there is somewhere a parameter mismatch in the source.
>>
>> Dehao, can you please check that this patch helps?
>>
>> Richi, if it does and the patch passes bootstrap and tests, is it OK
>> for trunk and 4.8 branch?
>>
>> Thanks and sorry for the trouble,
>>
>> Martin
>>
>>
>> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>>
>>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>>         within bounds at the beginning of the function.
>>
>> Index: src/gcc/ipa-cp.c
>> ===================================================================
>> --- src.orig/gcc/ipa-cp.c
>> +++ src/gcc/ipa-cp.c
>> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>>    tree otr_type;
>>    tree t;
>>
>> -  if (param_index == -1)
>> +  if (param_index == -1
>> +      || known_vals.length () <= (unsigned int) param_index)
>>      return NULL_TREE;
>>
>>    if (!ie->indirect_info->polymorphic)
>> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>>             t = NULL;
>>         }
>>        else
>> -       t = (known_vals.length () > (unsigned int) param_index
>> -            ? known_vals[param_index] : NULL);
>> +       t = NULL;
>>
>>        if (t &&
>>           TREE_CODE (t) == ADDR_EXPR

Re: [GOOGLE] More strict checking for call args

[Xinliang David Li]

On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
>> attached is a testcase that would cause problem when source has changed:
>>
>> $ g++ test.cc -O2 -fprofile-generate -DOLD
>> $ ./a.out
>> $ g++ test.cc -O2 -fprofile-use
>> test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>  }
>>  ^
>> 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>> ../../gcc/vec.h:815
>> 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>> ../../gcc/vec.h:1244
>> 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>> ../../gcc/vec.h:815
>> 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>> ../../gcc/vec.h:1244
>> 0xf24464 ipa_get_indirect_edge_target_1
>> ../../gcc/ipa-cp.c:1535
>> 0x971b9a estimate_edge_devirt_benefit
>> ../../gcc/ipa-inline-analysis.c:2757
>
> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
> Since it is called also from inlining, we can have parameter count
> mismatches... and in fact in non-virtual paths of that function we do
> check that we don't.  Because all callers have to pass known_vals
> describing all formal parameters of the inline tree root, we should
> apply the fix below (I've only just started running a bootstrap and
> testsuite on x86_64, though).
>
> OTOH, while I understand that FDO can change inlining sufficiently so
> that this error occurs, IMHO this should not be caused by outdated
> profiles but there is somewhere a parameter mismatch in the source.

Martin, what do you mean by the above?

thanks,

David


>
> Dehao, can you please check that this patch helps?
>
> Richi, if it does and the patch passes bootstrap and tests, is it OK
> for trunk and 4.8 branch?
>
> Thanks and sorry for the trouble,
>
> Martin
>
>
> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>
>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>         within bounds at the beginning of the function.
>
> Index: src/gcc/ipa-cp.c
> ===================================================================
> --- src.orig/gcc/ipa-cp.c
> +++ src/gcc/ipa-cp.c
> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>    tree otr_type;
>    tree t;
>
> -  if (param_index == -1)
> +  if (param_index == -1
> +      || known_vals.length () <= (unsigned int) param_index)
>      return NULL_TREE;
>
>    if (!ie->indirect_info->polymorphic)
> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>             t = NULL;
>         }
>        else
> -       t = (known_vals.length () > (unsigned int) param_index
> -            ? known_vals[param_index] : NULL);
> +       t = NULL;
>
>        if (t &&
>           TREE_CODE (t) == ADDR_EXPR

Re: [GOOGLE] More strict checking for call args

[Dehao Chen]

[-- Attachment #1: Type: text/plain, Size: 4390 bytes --]

I've prepared a patch check for args for indirect call value profiling.

Testing on-going. Is it ok for trunk if testing is good?

Thanks,
Dehao

gcc/ChangeLog:
2013-06-06  Dehao Chen  <dehao@google.com>

        * tree-flow.h (gimple_check_call_matching_types): Add new argument.
        * gimple-low.c (gimple_check_call_matching_types): Likewise.
        (gimple_check_call_args): Likewise.
        * value-prof.c (check_ic_target): Likewise.
        * ipa-inline.c (early_inliner): Likewise.
        * ipa-prop.c (update_indirect_edges_after_inlining): Likewise.
        * cgraph.c (cgraph_create_edge_1): Likewise.
        (cgraph_make_edge_direct): Likewise.



On Thu, Jun 6, 2013 at 9:14 AM, Xinliang David Li <davidxl@google.com> wrote:
> gimple_check_call_matching_types is called by check_ic_target. Instead
> of moving the check out of this guard function, may be enhancing the
> interface to allow it to guard with different strictness?
>
> David
>
> On Thu, Jun 6, 2013 at 8:10 AM, Dehao Chen <dehao@google.com> wrote:
>> Hi, Martin,
>>
>> Yes, your patch can fix my case. Thanks a lot for the fix.
>>
>> With the fix, value profiling will still promote the wrong indirect
>> call target. Though it will not be inlining, but it results in an
>> additional check. How about in check_ic_target, after calling
>> gimple_check_call_matching_types, we also check if number of args
>> match number of params in target->symbol.decl?
>>
>> Thanks,
>> Dehao
>>
>>
>> On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>>
>>> Hi,
>>>
>>> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
>>> > attached is a testcase that would cause problem when source has changed:
>>> >
>>> > $ g++ test.cc -O2 -fprofile-generate -DOLD
>>> > $ ./a.out
>>> > $ g++ test.cc -O2 -fprofile-use
>>> > test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>> >  }
>>> >  ^
>>> > 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>> > ../../gcc/vec.h:815
>>> > 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>> > ../../gcc/vec.h:1244
>>> > 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>> > ../../gcc/vec.h:815
>>> > 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>> > ../../gcc/vec.h:1244
>>> > 0xf24464 ipa_get_indirect_edge_target_1
>>> > ../../gcc/ipa-cp.c:1535
>>> > 0x971b9a estimate_edge_devirt_benefit
>>> > ../../gcc/ipa-inline-analysis.c:2757
>>>
>>> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
>>> Since it is called also from inlining, we can have parameter count
>>> mismatches... and in fact in non-virtual paths of that function we do
>>> check that we don't.  Because all callers have to pass known_vals
>>> describing all formal parameters of the inline tree root, we should
>>> apply the fix below (I've only just started running a bootstrap and
>>> testsuite on x86_64, though).
>>>
>>> OTOH, while I understand that FDO can change inlining sufficiently so
>>> that this error occurs, IMHO this should not be caused by outdated
>>> profiles but there is somewhere a parameter mismatch in the source.
>>>
>>> Dehao, can you please check that this patch helps?
>>>
>>> Richi, if it does and the patch passes bootstrap and tests, is it OK
>>> for trunk and 4.8 branch?
>>>
>>> Thanks and sorry for the trouble,
>>>
>>> Martin
>>>
>>>
>>> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>>>
>>>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>>>         within bounds at the beginning of the function.
>>>
>>> Index: src/gcc/ipa-cp.c
>>> ===================================================================
>>> --- src.orig/gcc/ipa-cp.c
>>> +++ src/gcc/ipa-cp.c
>>> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>>>    tree otr_type;
>>>    tree t;
>>>
>>> -  if (param_index == -1)
>>> +  if (param_index == -1
>>> +      || known_vals.length () <= (unsigned int) param_index)
>>>      return NULL_TREE;
>>>
>>>    if (!ie->indirect_info->polymorphic)
>>> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>>>             t = NULL;
>>>         }
>>>        else
>>> -       t = (known_vals.length () > (unsigned int) param_index
>>> -            ? known_vals[param_index] : NULL);
>>> +       t = NULL;
>>>
>>>        if (t &&
>>>           TREE_CODE (t) == ADDR_EXPR

[-- Attachment #2: diff.txt --]
[-- Type: text/plain, Size: 4888 bytes --]

Index: gcc/gimple-low.c
===================================================================
--- gcc/gimple-low.c	(revision 199780)
+++ gcc/gimple-low.c	(working copy)
@@ -204,7 +204,7 @@ struct gimple_opt_pass pass_lower_cf =
    return false.  */
 
 static bool
-gimple_check_call_args (gimple stmt, tree fndecl)
+gimple_check_call_args (gimple stmt, tree fndecl, bool args_count_match)
 {
   tree parms, p;
   unsigned int i, nargs;
@@ -243,6 +243,8 @@ static bool
 		  && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
             return false;
 	}
+      if (args_count_match && p)
+	return false;
     }
   else if (parms)
     {
@@ -271,11 +273,13 @@ static bool
 }
 
 /* Verify if the type of the argument and lhs of CALL_STMT matches
-   that of the function declaration CALLEE.
+   that of the function declaration CALLEE. If ARGS_COUNT_MATCH is
+   true, the arg count needs to be the same.
    If we cannot verify this or there is a mismatch, return false.  */
 
 bool
-gimple_check_call_matching_types (gimple call_stmt, tree callee)
+gimple_check_call_matching_types (gimple call_stmt, tree callee,
+				  bool args_count_match)
 {
   tree lhs;
 
@@ -285,7 +289,7 @@ bool
        && !useless_type_conversion_p (TREE_TYPE (DECL_RESULT (callee)),
                                       TREE_TYPE (lhs))
        && !fold_convertible_p (TREE_TYPE (DECL_RESULT (callee)), lhs))
-      || !gimple_check_call_args (call_stmt, callee))
+      || !gimple_check_call_args (call_stmt, callee, args_count_match))
     return false;
   return true;
 }
Index: gcc/ipa-inline.c
===================================================================
--- gcc/ipa-inline.c	(revision 199780)
+++ gcc/ipa-inline.c	(working copy)
@@ -2054,8 +2054,8 @@ early_inliner (void)
 	      es->call_stmt_time
 		= estimate_num_insns (edge->call_stmt, &eni_time_weights);
 	      if (edge->callee->symbol.decl
-		  && !gimple_check_call_matching_types (edge->call_stmt,
-							edge->callee->symbol.decl))
+		  && !gimple_check_call_matching_types (
+		      edge->call_stmt, edge->callee->symbol.decl, false))
 		edge->call_stmt_cannot_inline_p = true;
 	    }
 	  timevar_pop (TV_INTEGRATION);
Index: gcc/cgraph.c
===================================================================
--- gcc/cgraph.c	(revision 199780)
+++ gcc/cgraph.c	(working copy)
@@ -816,7 +816,8 @@ cgraph_create_edge_1 (struct cgraph_node *caller,
   pop_cfun ();
   if (call_stmt
       && callee && callee->symbol.decl
-      && !gimple_check_call_matching_types (call_stmt, callee->symbol.decl))
+      && !gimple_check_call_matching_types (call_stmt, callee->symbol.decl,
+					    false))
     edge->call_stmt_cannot_inline_p = true;
   else
     edge->call_stmt_cannot_inline_p = false;
@@ -1016,7 +1017,8 @@ cgraph_make_edge_direct (struct cgraph_edge *edge,
 
   if (edge->call_stmt)
     edge->call_stmt_cannot_inline_p
-      = !gimple_check_call_matching_types (edge->call_stmt, callee->symbol.decl);
+      = !gimple_check_call_matching_types (edge->call_stmt, callee->symbol.decl,
+					   false);
 
   /* We need to re-determine the inlining status of the edge.  */
   initialize_inline_failed (edge);
Index: gcc/tree-flow.h
===================================================================
--- gcc/tree-flow.h	(revision 199780)
+++ gcc/tree-flow.h	(working copy)
@@ -464,7 +464,7 @@ extern void record_vars_into (tree, tree);
 extern void record_vars (tree);
 extern bool gimple_seq_may_fallthru (gimple_seq);
 extern bool gimple_stmt_may_fallthru (gimple);
-extern bool gimple_check_call_matching_types (gimple, tree);
+extern bool gimple_check_call_matching_types (gimple, tree, bool);
 
 
 /* In tree-ssa.c  */
Index: gcc/value-prof.c
===================================================================
--- gcc/value-prof.c	(revision 199780)
+++ gcc/value-prof.c	(working copy)
@@ -1231,7 +1231,7 @@ static bool
 check_ic_target (gimple call_stmt, struct cgraph_node *target)
 {
    location_t locus;
-   if (gimple_check_call_matching_types (call_stmt, target->symbol.decl))
+   if (gimple_check_call_matching_types (call_stmt, target->symbol.decl, true))
      return true;
 
    locus =  gimple_location (call_stmt);
Index: gcc/ipa-prop.c
===================================================================
--- gcc/ipa-prop.c	(revision 199780)
+++ gcc/ipa-prop.c	(working copy)
@@ -2468,8 +2468,9 @@ update_indirect_edges_after_inlining (struct cgrap
 	  new_direct_edge->indirect_inlining_edge = 1;
 	  if (new_direct_edge->call_stmt)
 	    new_direct_edge->call_stmt_cannot_inline_p
-	      = !gimple_check_call_matching_types (new_direct_edge->call_stmt,
-						   new_direct_edge->callee->symbol.decl);
+	      = !gimple_check_call_matching_types (
+		  new_direct_edge->call_stmt,
+		  new_direct_edge->callee->symbol.decl, false);
 	  if (new_edges)
 	    {
 	      new_edges->safe_push (new_direct_edge);

Re: [GOOGLE] More strict checking for call args

[Xinliang David Li]

This one should be submitted and discussed in trunk.

thanks,

David

On Thu, Jun 6, 2013 at 9:39 PM, Dehao Chen <dehao@google.com> wrote:
> I've prepared a patch check for args for indirect call value profiling.
>
> Testing on-going. Is it ok for trunk if testing is good?
>
> Thanks,
> Dehao
>
> gcc/ChangeLog:
> 2013-06-06  Dehao Chen  <dehao@google.com>
>
>         * tree-flow.h (gimple_check_call_matching_types): Add new argument.
>         * gimple-low.c (gimple_check_call_matching_types): Likewise.
>         (gimple_check_call_args): Likewise.
>         * value-prof.c (check_ic_target): Likewise.
>         * ipa-inline.c (early_inliner): Likewise.
>         * ipa-prop.c (update_indirect_edges_after_inlining): Likewise.
>         * cgraph.c (cgraph_create_edge_1): Likewise.
>         (cgraph_make_edge_direct): Likewise.
>
>
>
> On Thu, Jun 6, 2013 at 9:14 AM, Xinliang David Li <davidxl@google.com> wrote:
>> gimple_check_call_matching_types is called by check_ic_target. Instead
>> of moving the check out of this guard function, may be enhancing the
>> interface to allow it to guard with different strictness?
>>
>> David
>>
>> On Thu, Jun 6, 2013 at 8:10 AM, Dehao Chen <dehao@google.com> wrote:
>>> Hi, Martin,
>>>
>>> Yes, your patch can fix my case. Thanks a lot for the fix.
>>>
>>> With the fix, value profiling will still promote the wrong indirect
>>> call target. Though it will not be inlining, but it results in an
>>> additional check. How about in check_ic_target, after calling
>>> gimple_check_call_matching_types, we also check if number of args
>>> match number of params in target->symbol.decl?
>>>
>>> Thanks,
>>> Dehao
>>>
>>>
>>> On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
>>>> > attached is a testcase that would cause problem when source has changed:
>>>> >
>>>> > $ g++ test.cc -O2 -fprofile-generate -DOLD
>>>> > $ ./a.out
>>>> > $ g++ test.cc -O2 -fprofile-use
>>>> > test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>>> >  }
>>>> >  ^
>>>> > 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>>> > ../../gcc/vec.h:815
>>>> > 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>>> > ../../gcc/vec.h:1244
>>>> > 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>>> > ../../gcc/vec.h:815
>>>> > 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>>> > ../../gcc/vec.h:1244
>>>> > 0xf24464 ipa_get_indirect_edge_target_1
>>>> > ../../gcc/ipa-cp.c:1535
>>>> > 0x971b9a estimate_edge_devirt_benefit
>>>> > ../../gcc/ipa-inline-analysis.c:2757
>>>>
>>>> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
>>>> Since it is called also from inlining, we can have parameter count
>>>> mismatches... and in fact in non-virtual paths of that function we do
>>>> check that we don't.  Because all callers have to pass known_vals
>>>> describing all formal parameters of the inline tree root, we should
>>>> apply the fix below (I've only just started running a bootstrap and
>>>> testsuite on x86_64, though).
>>>>
>>>> OTOH, while I understand that FDO can change inlining sufficiently so
>>>> that this error occurs, IMHO this should not be caused by outdated
>>>> profiles but there is somewhere a parameter mismatch in the source.
>>>>
>>>> Dehao, can you please check that this patch helps?
>>>>
>>>> Richi, if it does and the patch passes bootstrap and tests, is it OK
>>>> for trunk and 4.8 branch?
>>>>
>>>> Thanks and sorry for the trouble,
>>>>
>>>> Martin
>>>>
>>>>
>>>> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>>>>
>>>>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>>>>         within bounds at the beginning of the function.
>>>>
>>>> Index: src/gcc/ipa-cp.c
>>>> ===================================================================
>>>> --- src.orig/gcc/ipa-cp.c
>>>> +++ src/gcc/ipa-cp.c
>>>> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>>>>    tree otr_type;
>>>>    tree t;
>>>>
>>>> -  if (param_index == -1)
>>>> +  if (param_index == -1
>>>> +      || known_vals.length () <= (unsigned int) param_index)
>>>>      return NULL_TREE;
>>>>
>>>>    if (!ie->indirect_info->polymorphic)
>>>> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>>>>             t = NULL;
>>>>         }
>>>>        else
>>>> -       t = (known_vals.length () > (unsigned int) param_index
>>>> -            ? known_vals[param_index] : NULL);
>>>> +       t = NULL;
>>>>
>>>>        if (t &&
>>>>           TREE_CODE (t) == ADDR_EXPR

Re: [GOOGLE] More strict checking for call args

[Richard Biener]

On Thu, Jun 6, 2013 at 5:10 PM, Dehao Chen <dehao@google.com> wrote:
> Hi, Martin,
>
> Yes, your patch can fix my case. Thanks a lot for the fix.
>
> With the fix, value profiling will still promote the wrong indirect
> call target. Though it will not be inlining, but it results in an
> additional check. How about in check_ic_target, after calling
> gimple_check_call_matching_types, we also check if number of args
> match number of params in target->symbol.decl?

I wonder what's the point in the gimple_check_call_matching_types check
in the profiling case.  It's at least no longer

/* Perform sanity check on the indirect call target. Due to race conditions,
   false function target may be attributed to an indirect call site. If the
   call expression type mismatches with the target function's type, expand_call
   may ICE.

because since the introduction of gimple_call_fntype we will _not_ ICE.

Thus I argue that check_ic_target should be even removed instead of
enhancing it!

How does IC profiling determine the called target?  That is, what does it
do when the target is not always the same?  (because the checking code
talks about race conditions for example)

Richard.


> Thanks,
> Dehao
>
>
> On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>
>> Hi,
>>
>> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
>> > attached is a testcase that would cause problem when source has changed:
>> >
>> > $ g++ test.cc -O2 -fprofile-generate -DOLD
>> > $ ./a.out
>> > $ g++ test.cc -O2 -fprofile-use
>> > test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>> >  }
>> >  ^
>> > 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>> > ../../gcc/vec.h:815
>> > 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>> > ../../gcc/vec.h:1244
>> > 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>> > ../../gcc/vec.h:815
>> > 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>> > ../../gcc/vec.h:1244
>> > 0xf24464 ipa_get_indirect_edge_target_1
>> > ../../gcc/ipa-cp.c:1535
>> > 0x971b9a estimate_edge_devirt_benefit
>> > ../../gcc/ipa-inline-analysis.c:2757
>>
>> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
>> Since it is called also from inlining, we can have parameter count
>> mismatches... and in fact in non-virtual paths of that function we do
>> check that we don't.  Because all callers have to pass known_vals
>> describing all formal parameters of the inline tree root, we should
>> apply the fix below (I've only just started running a bootstrap and
>> testsuite on x86_64, though).
>>
>> OTOH, while I understand that FDO can change inlining sufficiently so
>> that this error occurs, IMHO this should not be caused by outdated
>> profiles but there is somewhere a parameter mismatch in the source.
>>
>> Dehao, can you please check that this patch helps?
>>
>> Richi, if it does and the patch passes bootstrap and tests, is it OK
>> for trunk and 4.8 branch?
>>
>> Thanks and sorry for the trouble,
>>
>> Martin
>>
>>
>> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>>
>>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>>         within bounds at the beginning of the function.
>>
>> Index: src/gcc/ipa-cp.c
>> ===================================================================
>> --- src.orig/gcc/ipa-cp.c
>> +++ src/gcc/ipa-cp.c
>> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>>    tree otr_type;
>>    tree t;
>>
>> -  if (param_index == -1)
>> +  if (param_index == -1
>> +      || known_vals.length () <= (unsigned int) param_index)
>>      return NULL_TREE;
>>
>>    if (!ie->indirect_info->polymorphic)
>> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>>             t = NULL;
>>         }
>>        else
>> -       t = (known_vals.length () > (unsigned int) param_index
>> -            ? known_vals[param_index] : NULL);
>> +       t = NULL;
>>
>>        if (t &&
>>           TREE_CODE (t) == ADDR_EXPR

Re: [GOOGLE] More strict checking for call args

[Xinliang David Li]

On Fri, Jun 7, 2013 at 2:05 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Jun 6, 2013 at 5:10 PM, Dehao Chen <dehao@google.com> wrote:
>> Hi, Martin,
>>
>> Yes, your patch can fix my case. Thanks a lot for the fix.
>>
>> With the fix, value profiling will still promote the wrong indirect
>> call target. Though it will not be inlining, but it results in an
>> additional check. How about in check_ic_target, after calling
>> gimple_check_call_matching_types, we also check if number of args
>> match number of params in target->symbol.decl?
>
> I wonder what's the point in the gimple_check_call_matching_types check
> in the profiling case.  It's at least no longer
>
> /* Perform sanity check on the indirect call target. Due to race conditions,
>    false function target may be attributed to an indirect call site. If the
>    call expression type mismatches with the target function's type, expand_call
>    may ICE.
>
> because since the introduction of gimple_call_fntype we will _not_ ICE.
>
> Thus I argue that check_ic_target should be even removed instead of
> enhancing it!
>

Another reason is what Dehao had mentioned -- wrong target leads to
useless transformation.

> How does IC profiling determine the called target?  That is, what does it
> do when the target is not always the same?  (because the checking code
> talks about race conditions for example)


The race condition is the happening at instrumentation time -- the
indirect call counters are not thread local. We have seen this a lot
in the past that a totally bogus target is attributed to a indirect
callsite.

thanks,

David
>
> Richard.
>
>
>> Thanks,
>> Dehao
>>
>>
>> On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>>
>>> Hi,
>>>
>>> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
>>> > attached is a testcase that would cause problem when source has changed:
>>> >
>>> > $ g++ test.cc -O2 -fprofile-generate -DOLD
>>> > $ ./a.out
>>> > $ g++ test.cc -O2 -fprofile-use
>>> > test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>> >  }
>>> >  ^
>>> > 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>> > ../../gcc/vec.h:815
>>> > 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>> > ../../gcc/vec.h:1244
>>> > 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>> > ../../gcc/vec.h:815
>>> > 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>> > ../../gcc/vec.h:1244
>>> > 0xf24464 ipa_get_indirect_edge_target_1
>>> > ../../gcc/ipa-cp.c:1535
>>> > 0x971b9a estimate_edge_devirt_benefit
>>> > ../../gcc/ipa-inline-analysis.c:2757
>>>
>>> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
>>> Since it is called also from inlining, we can have parameter count
>>> mismatches... and in fact in non-virtual paths of that function we do
>>> check that we don't.  Because all callers have to pass known_vals
>>> describing all formal parameters of the inline tree root, we should
>>> apply the fix below (I've only just started running a bootstrap and
>>> testsuite on x86_64, though).
>>>
>>> OTOH, while I understand that FDO can change inlining sufficiently so
>>> that this error occurs, IMHO this should not be caused by outdated
>>> profiles but there is somewhere a parameter mismatch in the source.
>>>
>>> Dehao, can you please check that this patch helps?
>>>
>>> Richi, if it does and the patch passes bootstrap and tests, is it OK
>>> for trunk and 4.8 branch?
>>>
>>> Thanks and sorry for the trouble,
>>>
>>> Martin
>>>
>>>
>>> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>>>
>>>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>>>         within bounds at the beginning of the function.
>>>
>>> Index: src/gcc/ipa-cp.c
>>> ===================================================================
>>> --- src.orig/gcc/ipa-cp.c
>>> +++ src/gcc/ipa-cp.c
>>> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>>>    tree otr_type;
>>>    tree t;
>>>
>>> -  if (param_index == -1)
>>> +  if (param_index == -1
>>> +      || known_vals.length () <= (unsigned int) param_index)
>>>      return NULL_TREE;
>>>
>>>    if (!ie->indirect_info->polymorphic)
>>> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>>>             t = NULL;
>>>         }
>>>        else
>>> -       t = (known_vals.length () > (unsigned int) param_index
>>> -            ? known_vals[param_index] : NULL);
>>> +       t = NULL;
>>>
>>>        if (t &&
>>>           TREE_CODE (t) == ADDR_EXPR

Re: [GOOGLE] More strict checking for call args

[Richard Biener]

On Fri, Jun 7, 2013 at 3:30 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Fri, Jun 7, 2013 at 2:05 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Thu, Jun 6, 2013 at 5:10 PM, Dehao Chen <dehao@google.com> wrote:
>>> Hi, Martin,
>>>
>>> Yes, your patch can fix my case. Thanks a lot for the fix.
>>>
>>> With the fix, value profiling will still promote the wrong indirect
>>> call target. Though it will not be inlining, but it results in an
>>> additional check. How about in check_ic_target, after calling
>>> gimple_check_call_matching_types, we also check if number of args
>>> match number of params in target->symbol.decl?
>>
>> I wonder what's the point in the gimple_check_call_matching_types check
>> in the profiling case.  It's at least no longer
>>
>> /* Perform sanity check on the indirect call target. Due to race conditions,
>>    false function target may be attributed to an indirect call site. If the
>>    call expression type mismatches with the target function's type, expand_call
>>    may ICE.
>>
>> because since the introduction of gimple_call_fntype we will _not_ ICE.
>>
>> Thus I argue that check_ic_target should be even removed instead of
>> enhancing it!
>>
>
> Another reason is what Dehao had mentioned -- wrong target leads to
> useless transformation.

Sure, but a not wrong in the sense of the predicate does not guarantee
a useful transformation either.

>> How does IC profiling determine the called target?  That is, what does it
>> do when the target is not always the same?  (because the checking code
>> talks about race conditions for example)
>
>
> The race condition is the happening at instrumentation time -- the
> indirect call counters are not thread local. We have seen this a lot
> in the past that a totally bogus target is attributed to a indirect
> callsite.

So it simply uses whatever function was called last?  Instead of
using the function that was called most of the time?

Richard.

> thanks,
>
> David
>>
>> Richard.
>>
>>
>>> Thanks,
>>> Dehao
>>>
>>>
>>> On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
>>>> > attached is a testcase that would cause problem when source has changed:
>>>> >
>>>> > $ g++ test.cc -O2 -fprofile-generate -DOLD
>>>> > $ ./a.out
>>>> > $ g++ test.cc -O2 -fprofile-use
>>>> > test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>>> >  }
>>>> >  ^
>>>> > 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>>> > ../../gcc/vec.h:815
>>>> > 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>>> > ../../gcc/vec.h:1244
>>>> > 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>>> > ../../gcc/vec.h:815
>>>> > 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>>> > ../../gcc/vec.h:1244
>>>> > 0xf24464 ipa_get_indirect_edge_target_1
>>>> > ../../gcc/ipa-cp.c:1535
>>>> > 0x971b9a estimate_edge_devirt_benefit
>>>> > ../../gcc/ipa-inline-analysis.c:2757
>>>>
>>>> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
>>>> Since it is called also from inlining, we can have parameter count
>>>> mismatches... and in fact in non-virtual paths of that function we do
>>>> check that we don't.  Because all callers have to pass known_vals
>>>> describing all formal parameters of the inline tree root, we should
>>>> apply the fix below (I've only just started running a bootstrap and
>>>> testsuite on x86_64, though).
>>>>
>>>> OTOH, while I understand that FDO can change inlining sufficiently so
>>>> that this error occurs, IMHO this should not be caused by outdated
>>>> profiles but there is somewhere a parameter mismatch in the source.
>>>>
>>>> Dehao, can you please check that this patch helps?
>>>>
>>>> Richi, if it does and the patch passes bootstrap and tests, is it OK
>>>> for trunk and 4.8 branch?
>>>>
>>>> Thanks and sorry for the trouble,
>>>>
>>>> Martin
>>>>
>>>>
>>>> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>>>>
>>>>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>>>>         within bounds at the beginning of the function.
>>>>
>>>> Index: src/gcc/ipa-cp.c
>>>> ===================================================================
>>>> --- src.orig/gcc/ipa-cp.c
>>>> +++ src/gcc/ipa-cp.c
>>>> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>>>>    tree otr_type;
>>>>    tree t;
>>>>
>>>> -  if (param_index == -1)
>>>> +  if (param_index == -1
>>>> +      || known_vals.length () <= (unsigned int) param_index)
>>>>      return NULL_TREE;
>>>>
>>>>    if (!ie->indirect_info->polymorphic)
>>>> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>>>>             t = NULL;
>>>>         }
>>>>        else
>>>> -       t = (known_vals.length () > (unsigned int) param_index
>>>> -            ? known_vals[param_index] : NULL);
>>>> +       t = NULL;
>>>>
>>>>        if (t &&
>>>>           TREE_CODE (t) == ADDR_EXPR

Re: [GOOGLE] More strict checking for call args

[Dehao Chen]

On Fri, Jun 7, 2013 at 6:47 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Fri, Jun 7, 2013 at 3:30 PM, Xinliang David Li <davidxl@google.com> wrote:
>> On Fri, Jun 7, 2013 at 2:05 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Thu, Jun 6, 2013 at 5:10 PM, Dehao Chen <dehao@google.com> wrote:
>>>> Hi, Martin,
>>>>
>>>> Yes, your patch can fix my case. Thanks a lot for the fix.
>>>>
>>>> With the fix, value profiling will still promote the wrong indirect
>>>> call target. Though it will not be inlining, but it results in an
>>>> additional check. How about in check_ic_target, after calling
>>>> gimple_check_call_matching_types, we also check if number of args
>>>> match number of params in target->symbol.decl?
>>>
>>> I wonder what's the point in the gimple_check_call_matching_types check
>>> in the profiling case.  It's at least no longer
>>>
>>> /* Perform sanity check on the indirect call target. Due to race conditions,
>>>    false function target may be attributed to an indirect call site. If the
>>>    call expression type mismatches with the target function's type, expand_call
>>>    may ICE.
>>>
>>> because since the introduction of gimple_call_fntype we will _not_ ICE.
>>>
>>> Thus I argue that check_ic_target should be even removed instead of
>>> enhancing it!
>>>
>>
>> Another reason is what Dehao had mentioned -- wrong target leads to
>> useless transformation.
>
> Sure, but a not wrong in the sense of the predicate does not guarantee
> a useful transformation either.

True, but at least it will filter some obvious bad transformations.

Another important factor is for AutoFDO, which uses source lineno to
map profile to IR. So when the source has changed, it could literally
map anything to an indirect call target. check_ic_target can actually
filter out most of the incorrect targets.

Thanks,
Dehao

>
>>> How does IC profiling determine the called target?  That is, what does it
>>> do when the target is not always the same?  (because the checking code
>>> talks about race conditions for example)
>>
>>
>> The race condition is the happening at instrumentation time -- the
>> indirect call counters are not thread local. We have seen this a lot
>> in the past that a totally bogus target is attributed to a indirect
>> callsite.
>
> So it simply uses whatever function was called last?  Instead of
> using the function that was called most of the time?
>
> Richard.
>
>> thanks,
>>
>> David
>>>
>>> Richard.
>>>
>>>
>>>> Thanks,
>>>> Dehao
>>>>
>>>>
>>>> On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
>>>>> > attached is a testcase that would cause problem when source has changed:
>>>>> >
>>>>> > $ g++ test.cc -O2 -fprofile-generate -DOLD
>>>>> > $ ./a.out
>>>>> > $ g++ test.cc -O2 -fprofile-use
>>>>> > test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>>>> >  }
>>>>> >  ^
>>>>> > 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>>>> > ../../gcc/vec.h:815
>>>>> > 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>>>> > ../../gcc/vec.h:1244
>>>>> > 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>>>> > ../../gcc/vec.h:815
>>>>> > 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>>>> > ../../gcc/vec.h:1244
>>>>> > 0xf24464 ipa_get_indirect_edge_target_1
>>>>> > ../../gcc/ipa-cp.c:1535
>>>>> > 0x971b9a estimate_edge_devirt_benefit
>>>>> > ../../gcc/ipa-inline-analysis.c:2757
>>>>>
>>>>> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
>>>>> Since it is called also from inlining, we can have parameter count
>>>>> mismatches... and in fact in non-virtual paths of that function we do
>>>>> check that we don't.  Because all callers have to pass known_vals
>>>>> describing all formal parameters of the inline tree root, we should
>>>>> apply the fix below (I've only just started running a bootstrap and
>>>>> testsuite on x86_64, though).
>>>>>
>>>>> OTOH, while I understand that FDO can change inlining sufficiently so
>>>>> that this error occurs, IMHO this should not be caused by outdated
>>>>> profiles but there is somewhere a parameter mismatch in the source.
>>>>>
>>>>> Dehao, can you please check that this patch helps?
>>>>>
>>>>> Richi, if it does and the patch passes bootstrap and tests, is it OK
>>>>> for trunk and 4.8 branch?
>>>>>
>>>>> Thanks and sorry for the trouble,
>>>>>
>>>>> Martin
>>>>>
>>>>>
>>>>> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>>>>>
>>>>>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>>>>>         within bounds at the beginning of the function.
>>>>>
>>>>> Index: src/gcc/ipa-cp.c
>>>>> ===================================================================
>>>>> --- src.orig/gcc/ipa-cp.c
>>>>> +++ src/gcc/ipa-cp.c
>>>>> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>>>>>    tree otr_type;
>>>>>    tree t;
>>>>>
>>>>> -  if (param_index == -1)
>>>>> +  if (param_index == -1
>>>>> +      || known_vals.length () <= (unsigned int) param_index)
>>>>>      return NULL_TREE;
>>>>>
>>>>>    if (!ie->indirect_info->polymorphic)
>>>>> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>>>>>             t = NULL;
>>>>>         }
>>>>>        else
>>>>> -       t = (known_vals.length () > (unsigned int) param_index
>>>>> -            ? known_vals[param_index] : NULL);
>>>>> +       t = NULL;
>>>>>
>>>>>        if (t &&
>>>>>           TREE_CODE (t) == ADDR_EXPR

Re: [GOOGLE] More strict checking for call args

[Xinliang David Li]

On Fri, Jun 7, 2013 at 6:47 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Fri, Jun 7, 2013 at 3:30 PM, Xinliang David Li <davidxl@google.com> wrote:
>> On Fri, Jun 7, 2013 at 2:05 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Thu, Jun 6, 2013 at 5:10 PM, Dehao Chen <dehao@google.com> wrote:
>>>> Hi, Martin,
>>>>
>>>> Yes, your patch can fix my case. Thanks a lot for the fix.
>>>>
>>>> With the fix, value profiling will still promote the wrong indirect
>>>> call target. Though it will not be inlining, but it results in an
>>>> additional check. How about in check_ic_target, after calling
>>>> gimple_check_call_matching_types, we also check if number of args
>>>> match number of params in target->symbol.decl?
>>>
>>> I wonder what's the point in the gimple_check_call_matching_types check
>>> in the profiling case.  It's at least no longer
>>>
>>> /* Perform sanity check on the indirect call target. Due to race conditions,
>>>    false function target may be attributed to an indirect call site. If the
>>>    call expression type mismatches with the target function's type, expand_call
>>>    may ICE.
>>>
>>> because since the introduction of gimple_call_fntype we will _not_ ICE.
>>>
>>> Thus I argue that check_ic_target should be even removed instead of
>>> enhancing it!
>>>
>>
>> Another reason is what Dehao had mentioned -- wrong target leads to
>> useless transformation.
>
> Sure, but a not wrong in the sense of the predicate does not guarantee
> a useful transformation either.

The case in reality is very rare -- most of the cases, the
transformation is good.

>
>>> How does IC profiling determine the called target?  That is, what does it
>>> do when the target is not always the same?  (because the checking code
>>> talks about race conditions for example)
>>
>>
>> The race condition is the happening at instrumentation time -- the
>> indirect call counters are not thread local. We have seen this a lot
>> in the past that a totally bogus target is attributed to a indirect
>> callsite.
>
> So it simply uses whatever function was called last?  Instead of
> using the function that was called most of the time?

It uses the most frequent target -- but the target id recorded for the
most frequent target might be corrupted and got mapped to a false
target during profile-use.

David

>
> Richard.
>
>> thanks,
>>
>> David
>>>
>>> Richard.
>>>
>>>
>>>> Thanks,
>>>> Dehao
>>>>
>>>>
>>>> On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
>>>>> > attached is a testcase that would cause problem when source has changed:
>>>>> >
>>>>> > $ g++ test.cc -O2 -fprofile-generate -DOLD
>>>>> > $ ./a.out
>>>>> > $ g++ test.cc -O2 -fprofile-use
>>>>> > test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>>>> >  }
>>>>> >  ^
>>>>> > 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>>>> > ../../gcc/vec.h:815
>>>>> > 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>>>> > ../../gcc/vec.h:1244
>>>>> > 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>>>> > ../../gcc/vec.h:815
>>>>> > 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>>>> > ../../gcc/vec.h:1244
>>>>> > 0xf24464 ipa_get_indirect_edge_target_1
>>>>> > ../../gcc/ipa-cp.c:1535
>>>>> > 0x971b9a estimate_edge_devirt_benefit
>>>>> > ../../gcc/ipa-inline-analysis.c:2757
>>>>>
>>>>> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
>>>>> Since it is called also from inlining, we can have parameter count
>>>>> mismatches... and in fact in non-virtual paths of that function we do
>>>>> check that we don't.  Because all callers have to pass known_vals
>>>>> describing all formal parameters of the inline tree root, we should
>>>>> apply the fix below (I've only just started running a bootstrap and
>>>>> testsuite on x86_64, though).
>>>>>
>>>>> OTOH, while I understand that FDO can change inlining sufficiently so
>>>>> that this error occurs, IMHO this should not be caused by outdated
>>>>> profiles but there is somewhere a parameter mismatch in the source.
>>>>>
>>>>> Dehao, can you please check that this patch helps?
>>>>>
>>>>> Richi, if it does and the patch passes bootstrap and tests, is it OK
>>>>> for trunk and 4.8 branch?
>>>>>
>>>>> Thanks and sorry for the trouble,
>>>>>
>>>>> Martin
>>>>>
>>>>>
>>>>> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>>>>>
>>>>>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>>>>>         within bounds at the beginning of the function.
>>>>>
>>>>> Index: src/gcc/ipa-cp.c
>>>>> ===================================================================
>>>>> --- src.orig/gcc/ipa-cp.c
>>>>> +++ src/gcc/ipa-cp.c
>>>>> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>>>>>    tree otr_type;
>>>>>    tree t;
>>>>>
>>>>> -  if (param_index == -1)
>>>>> +  if (param_index == -1
>>>>> +      || known_vals.length () <= (unsigned int) param_index)
>>>>>      return NULL_TREE;
>>>>>
>>>>>    if (!ie->indirect_info->polymorphic)
>>>>> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>>>>>             t = NULL;
>>>>>         }
>>>>>        else
>>>>> -       t = (known_vals.length () > (unsigned int) param_index
>>>>> -            ? known_vals[param_index] : NULL);
>>>>> +       t = NULL;
>>>>>
>>>>>        if (t &&
>>>>>           TREE_CODE (t) == ADDR_EXPR

Re: [GOOGLE] More strict checking for call args

[Martin Jambor]

Hi,

On Thu, Jun 06, 2013 at 08:10:13AM -0700, Dehao Chen wrote:
> Hi, Martin,
> 
> Yes, your patch can fix my case. Thanks a lot for the fix.

good.  However, as usual when I'm trying to do things too quickly, I
made a stupid mistaker and testing has revealed I picked exactly the
wrong branch in the second hunk.  So this is the correct patch, now
after proper bootstrapping and testing on x86_64-linux.  Since the
intent is clearly the same as the approved patch, I intend to commit
it later today/early tomorrow, unless there are any objections.

Thanks and sorry again,

Martin


2013-06-07  Martin Jambor  <mjambor@suse.cz>

	* ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
	within bounds at the beginning of the function.

Index: src/gcc/ipa-cp.c
===================================================================
--- src.orig/gcc/ipa-cp.c
+++ src/gcc/ipa-cp.c
@@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
   tree otr_type;
   tree t;
 
-  if (param_index == -1)
+  if (param_index == -1
+      || known_vals.length () <= (unsigned int) param_index)
     return NULL_TREE;
 
   if (!ie->indirect_info->polymorphic)
@@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
 	    t = NULL;
 	}
       else
-	t = (known_vals.length () > (unsigned int) param_index
-	     ? known_vals[param_index] : NULL);
+	t = known_vals[param_index];
 
       if (t &&
 	  TREE_CODE (t) == ADDR_EXPR

Re: [GOOGLE] More strict checking for call args

[Richard Biener]

On Sat, Jun 8, 2013 at 2:26 AM, Xinliang David Li <davidxl@google.com> wrote:
> On Fri, Jun 7, 2013 at 6:47 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Fri, Jun 7, 2013 at 3:30 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> On Fri, Jun 7, 2013 at 2:05 AM, Richard Biener
>>> <richard.guenther@gmail.com> wrote:
>>>> On Thu, Jun 6, 2013 at 5:10 PM, Dehao Chen <dehao@google.com> wrote:
>>>>> Hi, Martin,
>>>>>
>>>>> Yes, your patch can fix my case. Thanks a lot for the fix.
>>>>>
>>>>> With the fix, value profiling will still promote the wrong indirect
>>>>> call target. Though it will not be inlining, but it results in an
>>>>> additional check. How about in check_ic_target, after calling
>>>>> gimple_check_call_matching_types, we also check if number of args
>>>>> match number of params in target->symbol.decl?
>>>>
>>>> I wonder what's the point in the gimple_check_call_matching_types check
>>>> in the profiling case.  It's at least no longer
>>>>
>>>> /* Perform sanity check on the indirect call target. Due to race conditions,
>>>>    false function target may be attributed to an indirect call site. If the
>>>>    call expression type mismatches with the target function's type, expand_call
>>>>    may ICE.
>>>>
>>>> because since the introduction of gimple_call_fntype we will _not_ ICE.
>>>>
>>>> Thus I argue that check_ic_target should be even removed instead of
>>>> enhancing it!
>>>>
>>>
>>> Another reason is what Dehao had mentioned -- wrong target leads to
>>> useless transformation.
>>
>> Sure, but a not wrong in the sense of the predicate does not guarantee
>> a useful transformation either.
>
> The case in reality is very rare -- most of the cases, the
> transformation is good.
>
>>
>>>> How does IC profiling determine the called target?  That is, what does it
>>>> do when the target is not always the same?  (because the checking code
>>>> talks about race conditions for example)
>>>
>>>
>>> The race condition is the happening at instrumentation time -- the
>>> indirect call counters are not thread local. We have seen this a lot
>>> in the past that a totally bogus target is attributed to a indirect
>>> callsite.
>>
>> So it simply uses whatever function was called last?  Instead of
>> using the function that was called most of the time?
>
> It uses the most frequent target -- but the target id recorded for the
> most frequent target might be corrupted and got mapped to a false
> target during profile-use.

But that's not due to "race conditions" but rather AutoFDO which isn't even
in trunk, right?

Anyway, the discussion is probably moot - the patch is ok with me
and my argument would be we should use the function in less places.

Thanks,
Richard.

> David
>
>>
>> Richard.
>>
>>> thanks,
>>>
>>> David
>>>>
>>>> Richard.
>>>>
>>>>
>>>>> Thanks,
>>>>> Dehao
>>>>>
>>>>>
>>>>> On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
>>>>>> > attached is a testcase that would cause problem when source has changed:
>>>>>> >
>>>>>> > $ g++ test.cc -O2 -fprofile-generate -DOLD
>>>>>> > $ ./a.out
>>>>>> > $ g++ test.cc -O2 -fprofile-use
>>>>>> > test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>>>>> >  }
>>>>>> >  ^
>>>>>> > 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>>>>> > ../../gcc/vec.h:815
>>>>>> > 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>>>>> > ../../gcc/vec.h:1244
>>>>>> > 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>>>>> > ../../gcc/vec.h:815
>>>>>> > 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>>>>> > ../../gcc/vec.h:1244
>>>>>> > 0xf24464 ipa_get_indirect_edge_target_1
>>>>>> > ../../gcc/ipa-cp.c:1535
>>>>>> > 0x971b9a estimate_edge_devirt_benefit
>>>>>> > ../../gcc/ipa-inline-analysis.c:2757
>>>>>>
>>>>>> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
>>>>>> Since it is called also from inlining, we can have parameter count
>>>>>> mismatches... and in fact in non-virtual paths of that function we do
>>>>>> check that we don't.  Because all callers have to pass known_vals
>>>>>> describing all formal parameters of the inline tree root, we should
>>>>>> apply the fix below (I've only just started running a bootstrap and
>>>>>> testsuite on x86_64, though).
>>>>>>
>>>>>> OTOH, while I understand that FDO can change inlining sufficiently so
>>>>>> that this error occurs, IMHO this should not be caused by outdated
>>>>>> profiles but there is somewhere a parameter mismatch in the source.
>>>>>>
>>>>>> Dehao, can you please check that this patch helps?
>>>>>>
>>>>>> Richi, if it does and the patch passes bootstrap and tests, is it OK
>>>>>> for trunk and 4.8 branch?
>>>>>>
>>>>>> Thanks and sorry for the trouble,
>>>>>>
>>>>>> Martin
>>>>>>
>>>>>>
>>>>>> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>>>>>>
>>>>>>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>>>>>>         within bounds at the beginning of the function.
>>>>>>
>>>>>> Index: src/gcc/ipa-cp.c
>>>>>> ===================================================================
>>>>>> --- src.orig/gcc/ipa-cp.c
>>>>>> +++ src/gcc/ipa-cp.c
>>>>>> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>>>>>>    tree otr_type;
>>>>>>    tree t;
>>>>>>
>>>>>> -  if (param_index == -1)
>>>>>> +  if (param_index == -1
>>>>>> +      || known_vals.length () <= (unsigned int) param_index)
>>>>>>      return NULL_TREE;
>>>>>>
>>>>>>    if (!ie->indirect_info->polymorphic)
>>>>>> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>>>>>>             t = NULL;
>>>>>>         }
>>>>>>        else
>>>>>> -       t = (known_vals.length () > (unsigned int) param_index
>>>>>> -            ? known_vals[param_index] : NULL);
>>>>>> +       t = NULL;
>>>>>>
>>>>>>        if (t &&
>>>>>>           TREE_CODE (t) == ADDR_EXPR

Re: [GOOGLE] More strict checking for call args

[Xinliang David Li]

On Thu, Jun 13, 2013 at 1:43 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Sat, Jun 8, 2013 at 2:26 AM, Xinliang David Li <davidxl@google.com> wrote:
>> On Fri, Jun 7, 2013 at 6:47 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Fri, Jun 7, 2013 at 3:30 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Fri, Jun 7, 2013 at 2:05 AM, Richard Biener
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Thu, Jun 6, 2013 at 5:10 PM, Dehao Chen <dehao@google.com> wrote:
>>>>>> Hi, Martin,
>>>>>>
>>>>>> Yes, your patch can fix my case. Thanks a lot for the fix.
>>>>>>
>>>>>> With the fix, value profiling will still promote the wrong indirect
>>>>>> call target. Though it will not be inlining, but it results in an
>>>>>> additional check. How about in check_ic_target, after calling
>>>>>> gimple_check_call_matching_types, we also check if number of args
>>>>>> match number of params in target->symbol.decl?
>>>>>
>>>>> I wonder what's the point in the gimple_check_call_matching_types check
>>>>> in the profiling case.  It's at least no longer
>>>>>
>>>>> /* Perform sanity check on the indirect call target. Due to race conditions,
>>>>>    false function target may be attributed to an indirect call site. If the
>>>>>    call expression type mismatches with the target function's type, expand_call
>>>>>    may ICE.
>>>>>
>>>>> because since the introduction of gimple_call_fntype we will _not_ ICE.
>>>>>
>>>>> Thus I argue that check_ic_target should be even removed instead of
>>>>> enhancing it!
>>>>>
>>>>
>>>> Another reason is what Dehao had mentioned -- wrong target leads to
>>>> useless transformation.
>>>
>>> Sure, but a not wrong in the sense of the predicate does not guarantee
>>> a useful transformation either.
>>
>> The case in reality is very rare -- most of the cases, the
>> transformation is good.
>>
>>>
>>>>> How does IC profiling determine the called target?  That is, what does it
>>>>> do when the target is not always the same?  (because the checking code
>>>>> talks about race conditions for example)
>>>>
>>>>
>>>> The race condition is the happening at instrumentation time -- the
>>>> indirect call counters are not thread local. We have seen this a lot
>>>> in the past that a totally bogus target is attributed to a indirect
>>>> callsite.
>>>
>>> So it simply uses whatever function was called last?  Instead of
>>> using the function that was called most of the time?
>>
>> It uses the most frequent target -- but the target id recorded for the
>> most frequent target might be corrupted and got mapped to a false
>> target during profile-use.
>
> But that's not due to "race conditions" but rather AutoFDO which isn't even
> in trunk, right?

not yet. Dehao is working on it.

David

>
> Anyway, the discussion is probably moot - the patch is ok with me
> and my argument would be we should use the function in less places.
>
> Thanks,
> Richard.
>
>> David
>>
>>>
>>> Richard.
>>>
>>>> thanks,
>>>>
>>>> David
>>>>>
>>>>> Richard.
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> Dehao
>>>>>>
>>>>>>
>>>>>> On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
>>>>>>> > attached is a testcase that would cause problem when source has changed:
>>>>>>> >
>>>>>>> > $ g++ test.cc -O2 -fprofile-generate -DOLD
>>>>>>> > $ ./a.out
>>>>>>> > $ g++ test.cc -O2 -fprofile-use
>>>>>>> > test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>>>>>> >  }
>>>>>>> >  ^
>>>>>>> > 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>>>>>> > ../../gcc/vec.h:815
>>>>>>> > 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>>>>>> > ../../gcc/vec.h:1244
>>>>>>> > 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>>>>>> > ../../gcc/vec.h:815
>>>>>>> > 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>>>>>> > ../../gcc/vec.h:1244
>>>>>>> > 0xf24464 ipa_get_indirect_edge_target_1
>>>>>>> > ../../gcc/ipa-cp.c:1535
>>>>>>> > 0x971b9a estimate_edge_devirt_benefit
>>>>>>> > ../../gcc/ipa-inline-analysis.c:2757
>>>>>>>
>>>>>>> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
>>>>>>> Since it is called also from inlining, we can have parameter count
>>>>>>> mismatches... and in fact in non-virtual paths of that function we do
>>>>>>> check that we don't.  Because all callers have to pass known_vals
>>>>>>> describing all formal parameters of the inline tree root, we should
>>>>>>> apply the fix below (I've only just started running a bootstrap and
>>>>>>> testsuite on x86_64, though).
>>>>>>>
>>>>>>> OTOH, while I understand that FDO can change inlining sufficiently so
>>>>>>> that this error occurs, IMHO this should not be caused by outdated
>>>>>>> profiles but there is somewhere a parameter mismatch in the source.
>>>>>>>
>>>>>>> Dehao, can you please check that this patch helps?
>>>>>>>
>>>>>>> Richi, if it does and the patch passes bootstrap and tests, is it OK
>>>>>>> for trunk and 4.8 branch?
>>>>>>>
>>>>>>> Thanks and sorry for the trouble,
>>>>>>>
>>>>>>> Martin
>>>>>>>
>>>>>>>
>>>>>>> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>>>>>>>
>>>>>>>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>>>>>>>         within bounds at the beginning of the function.
>>>>>>>
>>>>>>> Index: src/gcc/ipa-cp.c
>>>>>>> ===================================================================
>>>>>>> --- src.orig/gcc/ipa-cp.c
>>>>>>> +++ src/gcc/ipa-cp.c
>>>>>>> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>>>>>>>    tree otr_type;
>>>>>>>    tree t;
>>>>>>>
>>>>>>> -  if (param_index == -1)
>>>>>>> +  if (param_index == -1
>>>>>>> +      || known_vals.length () <= (unsigned int) param_index)
>>>>>>>      return NULL_TREE;
>>>>>>>
>>>>>>>    if (!ie->indirect_info->polymorphic)
>>>>>>> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>>>>>>>             t = NULL;
>>>>>>>         }
>>>>>>>        else
>>>>>>> -       t = (known_vals.length () > (unsigned int) param_index
>>>>>>> -            ? known_vals[param_index] : NULL);
>>>>>>> +       t = NULL;
>>>>>>>
>>>>>>>        if (t &&
>>>>>>>           TREE_CODE (t) == ADDR_EXPR