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: Xinliang David Li <davidxl at google dot com>
- To: Dehao Chen <dehao at google dot com>
- Cc: Richard Biener <richard dot guenther at gmail dot com>, Duncan Sands <baldrick at free dot fr>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 6 Jun 2013 22:20:51 -0700
- 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> <CAAkRFZJabxDYrGcafa0iQELiiy6-waqrWvGT+GDqO3wXW+JQ=A at mail dot gmail dot com> <CAO2gOZXkaogJftpdDJMocQR2saNbKuMvbnT6+RuANnGnbSKQZQ at mail dot gmail dot com>
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