This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [GOOGLE] More strict checking for call args
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Xinliang David Li <davidxl at google dot com>
- Cc: Dehao Chen <dehao at google dot com>, Duncan Sands <baldrick at free dot fr>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 13 Jun 2013 10:43:00 +0200
- Subject: Re: [GOOGLE] More strict checking for call args
- References: <CAO2gOZWs7maFPDo=EZUe1mPARfNFxxnA5Yg3z0Wo0WS1+2ji2Q at mail dot gmail dot com> <51A85C16 dot 1030505 at free dot fr> <CAAkRFZL7aPp9WSxsj1yaujdBWrs91D=H0d_+KxVcgnh=xnt7Ng at mail dot gmail dot com> <CAO2gOZUZ0xTXoMPmLyhsCRkyFQehu7A7EiuqS1E40hBvd8yvxQ at mail dot gmail dot com> <CAFiYyc2q4nGUWNqDbqcpMunEPr-jQN0f2==h-rmMUCjqTWtTUw at mail dot gmail dot com> <CAO2gOZXqae0ju2nMK=f_XSKZFMOscdmL__P=MYf7x01BBs=jOw at mail dot gmail dot com> <CAAkRFZK4rfGvcaJ2PH34TGXtGaC41Wk8SeJ5hEyiC_eU9LTiTA at mail dot gmail dot com> <CAO2gOZViTTifco=SJ8sFqGjdAzg2h8Dzs87xtLa0ZVGaAJzGLg at mail dot gmail dot com> <20130606141124 dot GB30912 at virgil dot suse> <CAO2gOZWYMEa5y9F++nrfZm9JapTP2kCSEeHv-ad_aPNvRdS4-A at mail dot gmail dot com> <CAFiYyc2GTpRWx0iAaqmT9vq-fqACzAHwG+wixkAbu9iiVst_+Q at mail dot gmail dot com> <CAAkRFZKzsLTo5djwh_Pyn7zSXVRvJFHjWjg54aHk_sqryUY-jA at mail dot gmail dot com> <CAFiYyc3qiMf1qTNFXhQdsYDUEoJh0WNDTAoD_TE0qBT1jbvaUg at mail dot gmail dot com> <CAAkRFZLJdGhOMdnOjgQd1EOJzL1CgS=gQD5FsYOgWZV+BwerGQ at mail dot gmail dot com>
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