From patchwork Wed Apr 22 13:38:00 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom de Vries X-Patchwork-Id: 463667 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.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id EA12B14012C for ; Wed, 22 Apr 2015 23:38:20 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass reason="1024-bit key; unprotected key" header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=duaHqfu7; dkim-adsp=none (unprotected policy); dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=v2TAJgnYV6JBHFLw0 ww6fRXkPsCJl7guYL7po+Cf5j1ATyzfybafxbOZuZwwd61sbu56Rhy7u+le9ruvV qkQv87tBbwhwG3/avm3p0L32jfYep9LPjvV8d09JYr0ZMK3l2fNe/S6WYHUhvek1 bDt5Cbd4n1es2Iacyb8CqRj528= 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 :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=Lm5KfW4wGd92B9ITyoIWUAf 5oPk=; b=duaHqfu7+sEOr4iG+zVV8P0mPsP7q49V1e9qxMZze/590mrQN8h6X2c FW2yPXFTpS4/nmnpg0eE0nF9Yhophrc+KVhMKpbKAsWa7IVcqJKIcLkk6wgn2F3J bzFDf+h6XNHjpiDeI3TRkIKY8CjcX9UaBhCdLi1+0Nw4cDMRcmbQ= Received: (qmail 58233 invoked by alias); 22 Apr 2015 13:38:11 -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 56313 invoked by uid 89); 22 Apr 2015 13:38:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 22 Apr 2015 13:38:09 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-FEM-01.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1Ykuqr-0003qA-5r from Tom_deVries@mentor.com ; Wed, 22 Apr 2015 06:38:05 -0700 Received: from [127.0.0.1] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.3.224.2; Wed, 22 Apr 2015 14:38:03 +0100 Message-ID: <5537A438.5030803@mentor.com> Date: Wed, 22 Apr 2015 15:38:00 +0200 From: Tom de Vries User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Richard Biener CC: GCC Patches Subject: Re: [PATCH][PR65823] Fix va_arg ap_copy nop detection References: <553750B2.8010209@mentor.com> In-Reply-To: On 22-04-15 10:06, Richard Biener wrote: > On Wed, Apr 22, 2015 at 9:41 AM, Tom de Vries wrote: >> Hi, >> >> this patch fixes PR65823. >> >> >> The patches fixes the problem by using operand_equal_p to do the equality >> test. >> >> >> Bootstrapped and reg-tested on x86_64. >> >> Did minimal non-bootstrap build on arm and reg-tested. >> >> OK for trunk? > > Hmm, ok for now. Committed. > But I wonder if we can't fix things to not require that > odd extra copy. Agreed, that would be good. > In fact that we introduce ap.1 looks completely bogus to me AFAICT, it's introduced by gimplify_arg ('&argp') because argp (a PARM_DECL) is not addressable. > (and we don't in this case for arm). Note that the pointer compare obviously > fails because we unshare the expression. > > So ... what breaks if we simply remove this odd "fixup"? > [ Originally mentioned at https://gcc.gnu.org/ml/gcc/2015-02/msg00011.html . ] I've committed gcc.target/x86_64/abi/callabi/vaarg-6.c specifically as a minimal version of this problem. If we remove the ap_copy fixup, at original we have: ... ;; Function do_cpy2 (null) { char * e; char * e; e = VA_ARG_EXPR ; e = VA_ARG_EXPR ; if (e != b) { abort (); } } ... and after gimplify we have: ... do_cpy2 (char * argp) { char * argp.1; char * argp.2; char * b.3; char * e; argp.1 = argp; e = VA_ARG (&argp.1, 0B); argp.2 = argp; e = VA_ARG (&argp.2, 0B); b.3 = b; if (e != b.3) goto ; else goto ; : abort (); : } ... The second VA_ARG uses &argp.2, which is a copy of argp, which is unmodified by the first VA_ARG. Using attached _proof-of-concept_ patch, I get callabi.exp working without the ap_copy, still at the cost of one 'argp.1 = argp' copy though: ... do_cpy2 (char * argp) { char * argp.1; char * b.2; char * e; argp.1 = argp; e = VA_ARG (&argp.1, 0B); e = VA_ARG (&argp.1, 0B); b.2 = b; if (e != b.2) goto ; else goto ; : abort (); : } ... But perhaps there's an easier way? Thanks, - Tom Add copy for va_list parameter --- gcc/function.c | 29 +++++++++++++++++++++++++++++ gcc/gimplify.c | 16 ---------------- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/gcc/function.c b/gcc/function.c index 7d4df92..2ebfec4 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -3855,6 +3855,24 @@ gimplify_parm_type (tree *tp, int *walk_subtrees, void *data) return NULL; } +static inline bool +is_va_list_type (tree type) +{ + tree id = TYPE_IDENTIFIER (type); + if (id == NULL_TREE) + return false; + const char *s = IDENTIFIER_POINTER (id); + if (s == NULL) + return false; + if (strcmp (s, "va_list") == 0) + return true; + if (strcmp (s, "__builtin_sysv_va_list") == 0) + return true; + if (strcmp (s, "__builtin_ms_va_list") == 0) + return true; + return false; +} + /* Gimplify the parameter list for current_function_decl. This involves evaluating SAVE_EXPRs of variable sized parameters and generating code to implement callee-copies reference parameters. Returns a sequence of @@ -3953,6 +3971,17 @@ gimplify_parameters (void) DECL_HAS_VALUE_EXPR_P (parm) = 1; } } + else if (is_va_list_type (TREE_TYPE (parm))) + { + tree cp = create_tmp_reg (data.nominal_type, get_name (parm)); + DECL_IGNORED_P (cp) = 0; + TREE_ADDRESSABLE (cp) = 1; + tree t = build2 (MODIFY_EXPR, TREE_TYPE (cp), cp, parm); + gimplify_and_add (t, &stmts); + + SET_DECL_VALUE_EXPR (parm, cp); + DECL_HAS_VALUE_EXPR_P (parm) = 1; + } } fnargs.release (); diff --git a/gcc/gimplify.c b/gcc/gimplify.c index 5f1dd1a..c922dc7 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -4569,7 +4569,6 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, gimple assign; location_t loc = EXPR_LOCATION (*expr_p); gimple_stmt_iterator gsi; - tree ap = NULL_TREE, ap_copy = NULL_TREE; gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR || TREE_CODE (*expr_p) == INIT_EXPR); @@ -4730,16 +4729,12 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, enum internal_fn ifn = CALL_EXPR_IFN (*from_p); auto_vec vargs (nargs); - if (ifn == IFN_VA_ARG) - ap = unshare_expr (CALL_EXPR_ARG (*from_p, 0)); for (i = 0; i < nargs; i++) { gimplify_arg (&CALL_EXPR_ARG (*from_p, i), pre_p, EXPR_LOCATION (*from_p)); vargs.quick_push (CALL_EXPR_ARG (*from_p, i)); } - if (ifn == IFN_VA_ARG) - ap_copy = CALL_EXPR_ARG (*from_p, 0); call_stmt = gimple_build_call_internal_vec (ifn, vargs); gimple_set_location (call_stmt, EXPR_LOCATION (*expr_p)); } @@ -4784,17 +4779,6 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, gsi = gsi_last (*pre_p); maybe_fold_stmt (&gsi); - /* When gimplifying the &ap argument of va_arg, we might end up with - ap.1 = ap - va_arg (&ap.1, 0B) - We need to assign ap.1 back to ap, otherwise va_arg has no effect on - ap. */ - if (ap != NULL_TREE - && TREE_CODE (ap) == ADDR_EXPR - && TREE_CODE (ap_copy) == ADDR_EXPR - && !operand_equal_p (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), 0)) - gimplify_assign (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), pre_p); - if (want_value) { *expr_p = TREE_THIS_VOLATILE (*to_p) ? *from_p : unshare_expr (*to_p); -- 1.9.1