Patchwork [GOOGLE] More strict checking for call args

login
register
mail settings
Submitter Martin Jambor
Date June 6, 2013, 2:11 p.m.
Message ID <20130606141124.GB30912@virgil.suse>
Download mbox | patch
Permalink /patch/249429/
State New
Headers show

Comments

Martin Jambor - June 6, 2013, 2:11 p.m.
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.
Richard Guenther - June 6, 2013, 2:20 p.m.
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
Dehao Chen - June 6, 2013, 3:10 p.m.
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
Xinliang David Li - June 6, 2013, 4:14 p.m.
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
Xinliang David Li - June 6, 2013, 4:15 p.m.
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
Richard Guenther - June 7, 2013, 9:05 a.m.
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
Xinliang David Li - June 7, 2013, 1:30 p.m.
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
Richard Guenther - June 7, 2013, 1:47 p.m.
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
Dehao Chen - June 7, 2013, 3:05 p.m.
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
Xinliang David Li - June 8, 2013, 12:26 a.m.
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
Richard Guenther - June 13, 2013, 8:43 a.m.
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
Xinliang David Li - June 13, 2013, 3:50 p.m.
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

Patch

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