From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26739 invoked by alias); 7 Jun 2013 13:47:58 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 26709 invoked by uid 89); 7 Jun 2013 13:47:54 -0000 X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,SPF_PASS autolearn=ham version=3.3.1 Received: from mail-wg0-f46.google.com (HELO mail-wg0-f46.google.com) (74.125.82.46) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 07 Jun 2013 13:47:54 +0000 Received: by mail-wg0-f46.google.com with SMTP id l18so3079250wgh.13 for ; Fri, 07 Jun 2013 06:47:51 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.194.119.195 with SMTP id kw3mr4379562wjb.64.1370612871810; Fri, 07 Jun 2013 06:47:51 -0700 (PDT) Received: by 10.194.19.168 with HTTP; Fri, 7 Jun 2013 06:47:51 -0700 (PDT) In-Reply-To: References: <51A85C16.1030505@free.fr> <20130606141124.GB30912@virgil.suse> Date: Fri, 07 Jun 2013 13:47:00 -0000 Message-ID: Subject: Re: [GOOGLE] More strict checking for call args From: Richard Biener To: Xinliang David Li Cc: Dehao Chen , Duncan Sands , GCC Patches Content-Type: text/plain; charset=ISO-8859-1 X-SW-Source: 2013-06/txt/msg00401.txt.bz2 On Fri, Jun 7, 2013 at 3:30 PM, Xinliang David Li wrote: > On Fri, Jun 7, 2013 at 2:05 AM, Richard Biener > wrote: >> On Thu, Jun 6, 2013 at 5:10 PM, Dehao Chen 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 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::operator[](unsigned int) >>>> > ../../gcc/vec.h:815 >>>> > 0x512740 vec::operator[](unsigned int) >>>> > ../../gcc/vec.h:1244 >>>> > 0xf24464 vec::operator[](unsigned int) >>>> > ../../gcc/vec.h:815 >>>> > 0xf24464 vec::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 >>>> >>>> * 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