From patchwork Fri Jun 7 04:39:35 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dehao Chen X-Patchwork-Id: 249596 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "localhost", Issuer "www.qmailtoaster.com" (not verified)) by ozlabs.org (Postfix) with ESMTPS id 8DED82C02C5 for ; Fri, 7 Jun 2013 14:39:53 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=jsWU3k9aXPuvM5V0vS UkTYdZr9Q6vPD3E4WLHN2gApgZJNTrcqzItUgg0q0tHjtMgfs104nSi0d/4leDKA ArqlUIprPqcsouSGR9fOHSkDROefD3AEPdrh/r1/Lyn99F1D0kBkwTS0uog3x66G ms47YYXWyuiPaVyhvd3UQZSEw= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=tdTqTZflfjnY0+8YPHugFfMO 7QA=; b=OvbhY+U4wViy04dHVphobr315mCsRGidQng/Wk6zyFRFtq0JJyvB43NM yXgS22XePrMW3krgWfOuPPL16uvqwzo81hYAYcC6abcwQ27MMFtsxADbgcScLY19 Iw451H9A50UMsYtl6Nv+LVDdd1qRuoHqcqMg6sie2AAKQVJ0DPs= Received: (qmail 29936 invoked by alias); 7 Jun 2013 04:39:46 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 29906 invoked by uid 89); 7 Jun 2013 04:39:40 -0000 X-Spam-SWARE-Status: No, score=-4.3 required=5.0 tests=AWL, BAYES_00, KHOP_THREADED, RCVD_IN_DNSWL_LOW, RCVD_IN_HOSTKARMA_YE, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.1 Received: from mail-ea0-f173.google.com (HELO mail-ea0-f173.google.com) (209.85.215.173) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 07 Jun 2013 04:39:38 +0000 Received: by mail-ea0-f173.google.com with SMTP id g15so3054009eak.4 for ; Thu, 06 Jun 2013 21:39:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:x-gm-message-state; bh=dXxcjkksfxH28bWkuZ2SauP8vs1w23QngscSz69KAx8=; b=ArMTIBIShi8ibYTFp+CIuCDpoBRwRup9VCaA5bA0rJRA1vwp5HMIx68KEouZaMxH0y ZMYBDwqRIC8jLUkQPdvz+Yj6SuU+46IPV1Qlc8iZT+2WplkaHW0BjVHSq73Rjdnga1uz 5ve9mt8W3kIwQbviPpEFKZgSNZ8RJzvsJ2y56SNBFNZP7j0pjE9K58inN466PHqcFiGk z7YX7wXi3i+xTCDaoJgs5GVTp6At1H8w/2bvll8tF84ESCN5sm5IvyRfZ7FdS/s/+qqu Sy5bAEuqHxa4TDG+2zTMoD22E0fJ0LuhKo03YhKqIOROc8F8Wi6vG+Mzr2iyq4issHss qaYg== MIME-Version: 1.0 X-Received: by 10.14.206.136 with SMTP id l8mr36685476eeo.26.1370579976013; Thu, 06 Jun 2013 21:39:36 -0700 (PDT) Received: by 10.14.220.9 with HTTP; Thu, 6 Jun 2013 21:39:35 -0700 (PDT) In-Reply-To: References: <51A85C16.1030505@free.fr> <20130606141124.GB30912@virgil.suse> Date: Thu, 6 Jun 2013 21:39:35 -0700 Message-ID: Subject: Re: [GOOGLE] More strict checking for call args From: Dehao Chen To: Xinliang David Li Cc: Richard Biener , Duncan Sands , GCC Patches X-Gm-Message-State: ALoCoQmtG4YYg7ZV+M+1mNq83UnxaZ0XFLGyNZEk88Jmi7S9A7NwFUm5J6kRlc0ZPhEyr4JO6J87AgX/wcpYpTwE6QHNQgwQD/dxe48LH6g/HYk4LTscCcfS92DWQJbuQYQ2XCXuUKKi9YldGdBFmD8ChGAJ5Y+4tgc59CPrJ6pe+goJTdZWiVd2yAtKreinolevn9qTuVhJ X-Virus-Found: No 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 * 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 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 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 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 Index: gcc/gimple-low.c =================================================================== --- gcc/gimple-low.c (revision 199780) +++ gcc/gimple-low.c (working copy) @@ -204,7 +204,7 @@ struct gimple_opt_pass pass_lower_cf = return false. */ static bool -gimple_check_call_args (gimple stmt, tree fndecl) +gimple_check_call_args (gimple stmt, tree fndecl, bool args_count_match) { tree parms, p; unsigned int i, nargs; @@ -243,6 +243,8 @@ static bool && !fold_convertible_p (DECL_ARG_TYPE (p), arg))) return false; } + if (args_count_match && p) + return false; } else if (parms) { @@ -271,11 +273,13 @@ static bool } /* Verify if the type of the argument and lhs of CALL_STMT matches - that of the function declaration CALLEE. + that of the function declaration CALLEE. If ARGS_COUNT_MATCH is + true, the arg count needs to be the same. If we cannot verify this or there is a mismatch, return false. */ bool -gimple_check_call_matching_types (gimple call_stmt, tree callee) +gimple_check_call_matching_types (gimple call_stmt, tree callee, + bool args_count_match) { tree lhs; @@ -285,7 +289,7 @@ bool && !useless_type_conversion_p (TREE_TYPE (DECL_RESULT (callee)), TREE_TYPE (lhs)) && !fold_convertible_p (TREE_TYPE (DECL_RESULT (callee)), lhs)) - || !gimple_check_call_args (call_stmt, callee)) + || !gimple_check_call_args (call_stmt, callee, args_count_match)) return false; return true; } Index: gcc/ipa-inline.c =================================================================== --- gcc/ipa-inline.c (revision 199780) +++ gcc/ipa-inline.c (working copy) @@ -2054,8 +2054,8 @@ early_inliner (void) es->call_stmt_time = estimate_num_insns (edge->call_stmt, &eni_time_weights); if (edge->callee->symbol.decl - && !gimple_check_call_matching_types (edge->call_stmt, - edge->callee->symbol.decl)) + && !gimple_check_call_matching_types ( + edge->call_stmt, edge->callee->symbol.decl, false)) edge->call_stmt_cannot_inline_p = true; } timevar_pop (TV_INTEGRATION); Index: gcc/cgraph.c =================================================================== --- gcc/cgraph.c (revision 199780) +++ gcc/cgraph.c (working copy) @@ -816,7 +816,8 @@ cgraph_create_edge_1 (struct cgraph_node *caller, pop_cfun (); if (call_stmt && callee && callee->symbol.decl - && !gimple_check_call_matching_types (call_stmt, callee->symbol.decl)) + && !gimple_check_call_matching_types (call_stmt, callee->symbol.decl, + false)) edge->call_stmt_cannot_inline_p = true; else edge->call_stmt_cannot_inline_p = false; @@ -1016,7 +1017,8 @@ cgraph_make_edge_direct (struct cgraph_edge *edge, if (edge->call_stmt) edge->call_stmt_cannot_inline_p - = !gimple_check_call_matching_types (edge->call_stmt, callee->symbol.decl); + = !gimple_check_call_matching_types (edge->call_stmt, callee->symbol.decl, + false); /* We need to re-determine the inlining status of the edge. */ initialize_inline_failed (edge); Index: gcc/tree-flow.h =================================================================== --- gcc/tree-flow.h (revision 199780) +++ gcc/tree-flow.h (working copy) @@ -464,7 +464,7 @@ extern void record_vars_into (tree, tree); extern void record_vars (tree); extern bool gimple_seq_may_fallthru (gimple_seq); extern bool gimple_stmt_may_fallthru (gimple); -extern bool gimple_check_call_matching_types (gimple, tree); +extern bool gimple_check_call_matching_types (gimple, tree, bool); /* In tree-ssa.c */ Index: gcc/value-prof.c =================================================================== --- gcc/value-prof.c (revision 199780) +++ gcc/value-prof.c (working copy) @@ -1231,7 +1231,7 @@ static bool check_ic_target (gimple call_stmt, struct cgraph_node *target) { location_t locus; - if (gimple_check_call_matching_types (call_stmt, target->symbol.decl)) + if (gimple_check_call_matching_types (call_stmt, target->symbol.decl, true)) return true; locus = gimple_location (call_stmt); Index: gcc/ipa-prop.c =================================================================== --- gcc/ipa-prop.c (revision 199780) +++ gcc/ipa-prop.c (working copy) @@ -2468,8 +2468,9 @@ update_indirect_edges_after_inlining (struct cgrap new_direct_edge->indirect_inlining_edge = 1; if (new_direct_edge->call_stmt) new_direct_edge->call_stmt_cannot_inline_p - = !gimple_check_call_matching_types (new_direct_edge->call_stmt, - new_direct_edge->callee->symbol.decl); + = !gimple_check_call_matching_types ( + new_direct_edge->call_stmt, + new_direct_edge->callee->symbol.decl, false); if (new_edges) { new_edges->safe_push (new_direct_edge);