From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26258 invoked by alias); 28 Apr 2015 20:29:54 -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 26231 invoked by uid 89); 28 Apr 2015 20:29:53 -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; Tue, 28 Apr 2015 20:29:51 +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 1YnC8Y-0005rZ-MS from Tom_deVries@mentor.com ; Tue, 28 Apr 2015 13:29:47 -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; Tue, 28 Apr 2015 21:29:44 +0100 Message-ID: <553FEDB6.20509@mentor.com> Date: Tue, 28 Apr 2015 21:59:00 -0000 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 , John David Anglin Subject: Re: [PATCH][PR65818][bootstrap,hppa] Return side-effect free result in gimplify_va_arg_internal References: <553B89FB.7000009@mentor.com> <553E3451.5010300@mentor.com> <553E500B.2060505@mentor.com> In-Reply-To: Content-Type: multipart/mixed; boundary="------------070404040300060309060803" X-SW-Source: 2015-04/txt/msg01793.txt.bz2 --------------070404040300060309060803 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-length: 5191 On 28-04-15 12:34, Richard Biener wrote: > On Mon, Apr 27, 2015 at 5:04 PM, Tom de Vries wrote: >> On 27-04-15 15:40, Richard Biener wrote: >>> >>> On Mon, Apr 27, 2015 at 3:06 PM, Tom de Vries >>> wrote: >>>> >>>> On 27-04-15 10:17, Richard Biener wrote: >>>>>> >>>>>> >>>>>> This patch fixes that by gimplifying the address expression of the >>>>>> mem-ref >>>>>>> >>>>>>> returned by the target hook (borrowing code from gimplify_expr, case >>>>>>> MEM_REF). >>>>>>> >>>>>>> Bootstrapped and reg-tested on x86_64. >>>>>>> >>>>>>> Bootstrapped and reg-tested on hppa2.0w-hp-hpux11.11. >>>>>>> >>>>>>> OK for trunk? >>>>> >>>>> >>>>> Hmm, that assert looks suspicious... >>>>> >>>>> Can't you simply always do >>>>> >>>>> gimplify_expr (expr, pre_p, post_p, is_gimple_lvalue, fb_lvalue); >>>> >>>> >>>> >>>> It's a bit counter-intuitive for me, using is_gimple_lvalue for something >>>> (the result of va_arg) we use as rvalue. >>> >>> >>> Yeah, choosing that was done because you could assert it's a MEM_REF >>> and tree-stdarg eventually builds a WITH_SIZE_EXPR around it. >>> >>> It would possibly be easier to simply "inline" gimplify_va_arg_internal >>> at its use and only gimplify the assignment. Though in that case the >>> original code probably worked - just in the lhs == NULL case it didn't, >>> which hints at a better place for the fix - in expand_ifn_va_arg_1 do >>> >>> if (lhs != NULL_TREE) >>> { >>> ... >>> } >>> else >>> gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either); >>> >>> So ... if you can re-try that one it's pre-approved. >>> >> >> Using that proposed patch, we run into the following problem. >> >> Consider this testcase with ignored-result-va_arg: >> ... >> #include >> >> void >> f2 (int i, ...) >> { >> va_list ap; >> va_start (ap, i); >> va_arg (ap, long); >> va_end (ap); >> } >> ... >> >> after gimplify_va_arg_internal we have: >> ... >> (gdb) call debug_generic_expr (expr) >> *(long int * {ref-all}) addr.2 >> ... >> >> In a bit more detail: >> ... >> (gdb) call debug_tree (expr) >> > type > size >> unit size >> align 64 symtab 0 alias set -1 canonical type 0x7ffff64a77e0 >> precision 64 min max >> >> pointer_to_this > >> >> arg 0 > type > long int> >> public static unsigned DI size >> unit size >> align 64 symtab 0 alias set -1 canonical type 0x7ffff65e0498> >> >> arg 0 > 0x7ffff64c2150> >> used unsigned ignored DI file stdarg-1.c line 4 col 1 size >> unit size >> align 64 context >> >> arg 1 >> constant 0>> >> ... >> >> During 'gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either)' we ICE >> here: >> ... >> 8886 gcc_assert ((*gimple_test_f) (*expr_p)); >> ... >> >> At this point expr is: >> ... >> (gdb) call debug_generic_expr (*expr_p) >> *addr.2 >> ... >> >> In more detail: >> ... >> (gdb) call debug_tree (*expr_p) >> > type > align 8 symtab 0 alias set -1 canonical type 0x7ffff64c2000 >> pointer_to_this > >> >> arg 0 > type > void> >> sizes-gimplified public unsigned DI >> size >> unit size >> align 64 symtab 0 alias set -1 canonical type 0x7ffff64c2150 >> pointer_to_this > >> used unsigned ignored DI file stdarg-1.c line 4 col 1 size >> unit size >> align 64 context > >> arg 1 >> constant 0>> >> ... >> >> The memref is now VOID type. And that's not a gimple_val: >> ... >> (gdb) p is_gimple_val (*expr_p) >> $1 = false >> ... > > On which target? I can't seem to reproduce on x86_64 or i?86. I also > can't see how this > could happen. > Reproduced using attached patch on top of r222537, for target x86_64, with and without -m32. Thanks, - Tom > Richard. > >> Attached patch uses is_gimple_lvalue, but in expand_ifn_va_arg_1. >> >> I'll bootstrap and reg-test on x86_64 and commit, unless further comments of >> course. >> >> Thanks, >> - Tom --------------070404040300060309060803 Content-Type: text/x-patch; name="0001-Reproduce-ICE-in-gimplify_expr-gcc_assert-gimple_tes.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0001-Reproduce-ICE-in-gimplify_expr-gcc_assert-gimple_tes.pa"; filename*1="tch" Content-length: 875 Reproduce ICE in 'gimplify_expr: gcc_assert ((*gimple_test_f) (*expr_p))' diff --git a/gcc/testsuite/gcc.dg/reproduce.c b/gcc/testsuite/gcc.dg/reproduce.c new file mode 100644 index 0000000..c0c8e0b --- /dev/null +++ b/gcc/testsuite/gcc.dg/reproduce.c @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +#include + +void +f2 (int i, ...) +{ + va_list ap; + va_start (ap, i); + va_arg (ap, long); + va_end (ap); +} diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c index 1356374..829cbc1 100644 --- a/gcc/tree-stdarg.c +++ b/gcc/tree-stdarg.c @@ -1079,7 +1079,7 @@ expand_ifn_va_arg_1 (function *fun) gimplify_assign (lhs, expr, &pre); } else - gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); + gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either); pop_gimplify_context (NULL); -- 1.9.1 --------------070404040300060309060803--