[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