From patchwork Wed Apr 22 11:51:48 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrylo Tkachov X-Patchwork-Id: 463650 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 4B73614007D for ; Wed, 22 Apr 2015 21:52:02 +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=kru5QZFa; 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=Q+JFGKe8IBrS57Hp+ eiLhZKO4qc2a6qCTbnAmJJ/FCuKzIYC+NLqC8hCGHuRYGShdLxO0ua0npOX9gunL FJS97EzcwCSNjIiCOT+WaGhhgxK5DwOuV5MUGAZOUnfx8bFuRDlLGJtbsYKKcnV6 nXTmRqHawkn7Q1UtRPwBqdjgOQ= 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=fcWbo9ClGF0vmhRykPgfyBm Y/dQ=; b=kru5QZFaKHi2+3LdVToaGqaegNSehlckJZKdqHPgQ+dSkn7n4Bntd2K xR7VnxvZ/+CnJECLjZXKB5kgKohgUs4msrgfgZDadDn8nUVXna6fSPR8pAprNbeF bowFYAkC0kdvngLR9C2TK3u2R8LZHIX/CpSaeMT/mKeHBmfvlDgk= Received: (qmail 78868 invoked by alias); 22 Apr 2015 11:51:54 -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 78859 invoked by uid 89); 22 Apr 2015 11:51:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00, SPF_PASS autolearn=ham version=3.3.2 X-HELO: eu-smtp-delivery-143.mimecast.com Received: from eu-smtp-delivery-143.mimecast.com (HELO eu-smtp-delivery-143.mimecast.com) (146.101.78.143) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 22 Apr 2015 11:51:52 +0000 Received: from cam-owa1.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by uk-mta-17.uk.mimecast.lan; Wed, 22 Apr 2015 12:51:49 +0100 Received: from [10.2.207.50] ([10.1.2.79]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Wed, 22 Apr 2015 12:51:49 +0100 Message-ID: <55378B54.2040904@arm.com> Date: Wed, 22 Apr 2015 12:51:48 +0100 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: 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> In-Reply-To: <553689F0.5090606@arm.com> X-MC-Unique: OTbNm-74RqSBjXyqeVu8FA-1 X-IsSubscribed: yes 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? 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 >> commit 39c9cb0bff1088cffaf31208b9a735d2bc0c505a Author: Kyrylo Tkachov Date: Wed Mar 18 13:42:37 2015 +0000 [expr.c] PR 65358 Avoid clobbering partial argument during sibcall diff --git a/gcc/expr.c b/gcc/expr.c index 530a944..ccf94e0 100644 --- a/gcc/expr.c +++ b/gcc/expr.c @@ -4121,6 +4121,36 @@ emit_single_push_insn (machine_mode mode, rtx x, tree type) } #endif +/* If reading SIZE bytes from X will end up reading from + Y return the number of bytes that overlap. Return -1 + in all other cases. */ + +static int +memory_load_overlap (rtx x, rtx y, HOST_WIDE_INT size) +{ + rtx tmp = plus_constant (Pmode, x, +#ifdef ARGS_GROW_DOWNWARD + -size); +#else + size); +#endif + + + rtx sub = simplify_gen_binary (MINUS, Pmode, tmp, y); + + if (!CONST_INT_P (sub)) + return -1; + + HOST_WIDE_INT val = +#ifdef ARGS_GROW_DOWNWARD + -INTVAL (sub); +#else + INTVAL (sub); +#endif + + return IN_RANGE (val, 1, size) ? val : -1; +} + /* Generate code to push X onto the stack, assuming it has mode MODE and type TYPE. MODE is redundant except when X is a CONST_INT (since they don't @@ -4179,6 +4209,10 @@ emit_push_insn (rtx x, machine_mode mode, tree type, rtx size, xinner = x; + int nregs = partial / UNITS_PER_WORD; + rtx *tmp_regs = NULL; + int overlapping = 0; + if (mode == BLKmode || (STRICT_ALIGNMENT && align < GET_MODE_ALIGNMENT (mode))) { @@ -4309,6 +4343,35 @@ emit_push_insn (rtx x, machine_mode mode, tree type, rtx size, PARM_BOUNDARY. Assume the caller isn't lying. */ set_mem_align (target, align); + /* If part should go in registers and pushing to that part would + overwrite some of the values that need to go into regs, load the + overlapping values into temporary pseudos to be moved into the hard + regs at the end after the stack pushing has completed. + We cannot load them directly into the hard regs here because + they can be clobbered by the block move expansions. + See PR 65358. */ + + if (partial > 0 && reg != 0 && mode == BLKmode + && GET_CODE (reg) != PARALLEL) + { + overlapping = memory_load_overlap (XEXP (x, 0), temp, partial); + if (overlapping > 0) + { + gcc_assert (overlapping % UNITS_PER_WORD == 0); + overlapping /= UNITS_PER_WORD; + + tmp_regs = XALLOCAVEC (rtx, overlapping); + + for (int i = 0; i < overlapping; i++) + tmp_regs[i] = gen_reg_rtx (word_mode); + + for (int i = 0; i < overlapping; i++) + emit_move_insn (tmp_regs[i], + operand_subword_force (target, i, mode)); + } + else + overlapping = 0; + } emit_block_move (target, xinner, size, BLOCK_OP_CALL_PARM); } } @@ -4411,9 +4474,8 @@ emit_push_insn (rtx x, machine_mode mode, tree type, rtx size, } } - /* If part should go in registers, copy that part - into the appropriate registers. Do this now, at the end, - since mem-to-mem copies above may do function calls. */ + /* Move the partial arguments into the registers and any overlapping + values that we moved into the pseudos in tmp_regs. */ if (partial > 0 && reg != 0) { /* Handle calls that pass values in multiple non-contiguous locations. @@ -4421,9 +4483,15 @@ emit_push_insn (rtx x, machine_mode mode, tree type, rtx size, if (GET_CODE (reg) == PARALLEL) emit_group_load (reg, x, type, -1); else - { + { gcc_assert (partial % UNITS_PER_WORD == 0); - move_block_to_reg (REGNO (reg), x, partial / UNITS_PER_WORD, mode); + move_block_to_reg (REGNO (reg), x, nregs - overlapping, mode); + + for (int i = 0; i < overlapping; i++) + emit_move_insn (gen_rtx_REG (word_mode, REGNO (reg) + + nregs - overlapping + i), + tmp_regs[i]); + } } diff --git a/gcc/testsuite/gcc.dg/pr65358.c b/gcc/testsuite/gcc.dg/pr65358.c new file mode 100644 index 0000000..ba89fd4 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr65358.c @@ -0,0 +1,33 @@ +/* { dg-do run } */ +/* { dg-options "-O2" } */ + +struct pack +{ + int fine; + int victim; + int killer; +}; + +int __attribute__ ((__noinline__, __noclone__)) +bar (int a, int b, struct pack p) +{ + if (a != 20 || b != 30) + __builtin_abort (); + if (p.fine != 40 || p.victim != 50 || p.killer != 60) + __builtin_abort (); + return 0; +} + +int __attribute__ ((__noinline__, __noclone__)) +foo (int arg1, int arg2, int arg3, struct pack p) +{ + return bar (arg2, arg3, p); +} + +int main (void) +{ + struct pack p = { 40, 50, 60 }; + + (void) foo (10, 20, 30, p); + return 0; +}