From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 111647 invoked by alias); 27 Apr 2015 13:16:24 -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 111637 invoked by uid 89); 27 Apr 2015 13:16:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.3 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,RCVD_IN_JMF_BR,SPF_PASS,UNSUBSCRIBE_BODY autolearn=no version=3.3.2 X-HELO: BLU004-OMC3S13.hotmail.com Received: from blu004-omc3s13.hotmail.com (HELO BLU004-OMC3S13.hotmail.com) (65.55.116.88) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA256 encrypted) ESMTPS; Mon, 27 Apr 2015 13:16:21 +0000 Received: from BLU436-SMTP83 ([65.55.116.74]) by BLU004-OMC3S13.hotmail.com over TLS secured channel with Microsoft SMTPSVC(7.5.7601.22751); Mon, 27 Apr 2015 06:16:19 -0700 X-TMN: [MPInrliOWEzOae3DO6A23BCa6XKRWK7D] Message-ID: Date: Mon, 27 Apr 2015 13:16:00 -0000 From: John David Anglin User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Kyrill Tkachov , Jeff Law , GCC Patches CC: Honggyu Kim Subject: Re: [PATCH][expr.c] PR 65358 Avoid clobbering partial argument during sibcall References: <550ADF8F.7030300@arm.com> <55314236.2000806@redhat.com> <5534B811.2020107@arm.com> <55353F25.9010906@redhat.com> <55360AA0.9070106@arm.com> <55365A06.7010408@redhat.com> <553689F0.5090606@arm.com> <55378B54.2040904@arm.com> <553E0B9B.4040902@arm.com> In-Reply-To: <553E0B9B.4040902@arm.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2015-04/txt/msg01615.txt.bz2 On 2015-04-27 6:12 AM, Kyrill Tkachov wrote: > > On 22/04/15 12:51, Kyrill Tkachov wrote: >> On 21/04/15 18:33, Kyrill Tkachov wrote: >>> On 21/04/15 15:09, Jeff Law wrote: >>>> On 04/21/2015 02:30 AM, Kyrill Tkachov wrote: >>>>> From reading config/stormy16/stormy-abi it seems to me that we >>>>> don't >>>>> pass arguments partially in stormy16, so this code would never be >>>>> called >>>>> there. That leaves pa as the potential problematic target. >>>>> I don't suppose there's an easy way to test on pa? My checkout of >>>>> binutils >>>>> doesn't seem to include a sim target for it. >>>> No simulator, no machines in the testfarm, the box I had access to via >>>> parisc-linux.org seems dead and my ancient PA overheats well before a >>>> bootstrap could complete. I often regret knowing about the backwards >>>> way many things were done on the PA because it makes me think about >>>> cases that only matter on dead architectures. >>> So what should be the action plan here? I can't add an assert on >>> positive result as a negative result is valid. >>> >>> We want to catch the case where this would cause trouble on >>> pa, or change the patch until we're confident that it's fine >>> for pa. >>> >>> That being said, reading the documentation of STACK_GROWS_UPWARD >>> and ARGS_GROW_DOWNWARD I'm having a hard time visualising a case >>> where this would cause trouble on pa. >>> >>> Is the problem that in the function: >>> >>> +/* Add SIZE to X and check whether it's greater than Y. >>> + If it is, return the constant amount by which it's greater or >>> smaller. >>> + If the two are not statically comparable (for example, X and Y >>> contain >>> + different registers) return -1. This is used in >>> expand_push_insn to >>> + figure out if reading SIZE bytes from location X will end up >>> reading from >>> + location Y. */ >>> +static int >>> +memory_load_overlap (rtx x, rtx y, HOST_WIDE_INT size) >>> +{ >>> + rtx tmp = plus_constant (Pmode, x, size); >>> + rtx sub = simplify_gen_binary (MINUS, Pmode, tmp, y); >>> + >>> + if (!CONST_INT_P (sub)) >>> + return -1; >>> + >>> + return INTVAL (sub); >>> +} >>> >>> for ARGS_GROW_DOWNWARD we would be reading 'backwards' from x, >>> so the function should something like the following? >>> >>> static int >>> memory_load_overlap (rtx x, rtx y, HOST_WIDE_INT size) >>> { >>> #ifdef ARGS_GROW_DOWNWARD >>> rtx tmp = plus_constant (Pmode, x, -size); >>> #else >>> rtx tmp = plus_constant (Pmode, x, size); >>> #endif >>> rtx sub = simplify_gen_binary (MINUS, Pmode, tmp, y); >>> >>> if (!CONST_INT_P (sub)) >>> return -1; >>> >>> #ifdef ARGS_GROW_DOWNWARD >>> return INTVAL (-sub); >>> #else >>> return INTVAL (sub); >>> #endif >>> } >>> >>> now, say for x == sp + 4, y == sp + 8, size == 16: >>> This would be a problematic case for arm, so this code on arm >>> (where ARGS_GROW_DOWNWARD is *not* defined) would return >>> 12, which is the number of bytes that overlap. >>> >>> On a target where ARGS_GROW_DOWNWARD is defined this would return >>> -20, meaning that no overlap occurs (because we read in the descending >>> direction from x, IIUC). >> Hi Jeff, >> >> Here's an attempt to make this more concrete. >> Only the memory_load_overlap function has changed. >> This time I tried to take into account the case when >> ARGS_GROW_DOWNWARD. >> >> Take the case where x == sp, y == sp + 8, size == 16. >> For arm, this would return 8 as that is the number of bytes >> that overlap. On pa, since ARGS_GROW_DOWNWARD is defined it >> would return -1 as we're reading down from x rather than up >> towards y. >> >> In the case when x == sp + 8, y == sp, size == 16 >> This would return -1 on arm since we're reading upwards from x >> and thefore no overlap would happen. >> >> On pa, this would return 8, which I think is the right thing. >> But again, I don't have access to any pa means of testing. >> >> What do you think of this approach? > > Hi Dave, > > Would it be possible for you to test this patch on a 64-bit hppa > or at least bootstrap it? > https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01288.html I started a build and test with your patch on hppa64-hp-hpux11.11 this morning. > > There is a concern that it may potentially affect the passing of > complex arguments partially on the stack and partially in regs > on pa because of the way the args and stack grow on that target. > > Unfortunately I don't have access to any hardware or simulators. > It would help a lot with getting this patch in. If you write to linux-parisc@vger.kernel.org, arrangements can be made for an account on a Debian parisc linux machine for development testing. Helge Deller has arranged for some new machines since we took over the Debian buildd infrastructure for parisc. More info is here: https://parisc.wiki.kernel.org/index.php/Main_Page > > Thanks, > Kyrill > > >> >> Thanks, >> Kyrill >> >> P.S. I've included the testcase from Honggyu in the patch. >> >> 2015-04-22 Kyrylo Tkachov >> >> PR target/65358 >> * expr.c (memory_load_overlap): New function. >> (emit_push_insn): When pushing partial args to the stack would >> clobber the register part load the overlapping part into a pseudo >> and put it into the hard reg after pushing. >> >> 2015-04-22 Honggyu Kim >> >> PR target/65358 >> * gcc.dg/pr65358.c: New test. >> >>> >>> Thanks, >>> Kyrill >>> >>>> Jeff >>>> > > > Regards, Dave -- John David Anglin dave.anglin@bell.net