From patchwork Tue Nov 24 15:12:32 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrylo Tkachov X-Patchwork-Id: 57258 Delivered-To: patch@linaro.org Received: by 10.112.155.196 with SMTP id vy4csp2157465lbb; Tue, 24 Nov 2015 07:25:58 -0800 (PST) X-Received: by 10.66.228.225 with SMTP id sl1mr3205175pac.63.1448378758271; Tue, 24 Nov 2015 07:25:58 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id c5si27083383pas.41.2015.11.24.07.25.57 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 24 Nov 2015 07:25:58 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-415208-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; spf=pass (google.com: domain of gcc-patches-return-415208-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-415208-patch=linaro.org@gcc.gnu.org; dkim=pass header.i=@gcc.gnu.org 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:subject:content-type; q= dns; s=default; b=wcIE5Pc5xg/9yi6ccjMzMfa9rEYpgyRr56vF9iBAo+H6yP o4jz9XSoeRhy68g2DfzqawE4M4XmqKJGkrJRGl6s/sWp6fxe1pWweIzYdIQNAb2J iwzegdCzpCzHdAVxLklnQB4FH776zTK2YayBN6Lz+sFWSPoPWt3K0GoxFpvhk= 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:subject:content-type; s= default; bh=CyqGFpV5g0nNtDsbNLEAvfLkgSE=; b=N0v7sEHEoIuw8fuboVyE 08gTIyUxteLOXM0araHRt7DobLgGnabkfJa12EJKmwSLK8fZB2lOsjBg0ku2sveK 63vIwywMUANR8A2GueAn5GM6kMRf62oAyw1zegC3YQy/8LQM94088cCJDjbOoqb/ rUNIibjVgW7OjUdKIWRhhfg= Received: (qmail 2742 invoked by alias); 24 Nov 2015 15:25: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 2729 invoked by uid 89); 24 Nov 2015 15:25:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 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) (207.82.80.143) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 24 Nov 2015 15:25:43 +0000 Received: from cam-owa2.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by eu-smtp-1.mimecast.com with ESMTP id uk-mta-12-tnYubK4jQQqsFwsF97hKfQ-1; Tue, 24 Nov 2015 15:23:33 +0000 Received: from [10.2.206.200] ([10.1.2.79]) by cam-owa2.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Tue, 24 Nov 2015 15:12:33 +0000 Message-ID: <56547E60.6010702@arm.com> Date: Tue, 24 Nov 2015 15:12:32 +0000 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: GCC Patches Subject: [PATCH][calls.c] PR rtl-optimization/67226: Take into account pretend_args_size when checking stack offsets for sibcall optimisation X-MC-Unique: tnYubK4jQQqsFwsF97hKfQ-1 X-IsSubscribed: yes Hi all, This is a wrong-code PR similar to PR 65358. It occurs on targets/ABIs that can pass struct parameter partially in registers and partially on the stack. In this instance the target is arm. In this bug we hit a problem during sibcall optimisation at call expansion time where we have a partial argument to a function being passed down to another function partially and so the incoming and outgoing arguments pointer are the same. We miscompile the code if the incoming and outgoing stack slots overlap but are not identical. In this particular testcase for the call to store_one_arg we want to store the parameter in arg->value to the stack frame at argblock. arg->value is: (mem/c:BLK (plus:SI (reg/f:SI 104 virtual-incoming-args) (const_int 20 [0x14])) [1 from+0 S20 A32]) and argblock is: (plus:SI (reg/f:SI 104 virtual-incoming-args) (const_int 16 [0x10])) and the size of arg->value that we want to push to argblock is 12 bytes. Of course, a write of 12 bytes from arg->value to argblock would overwrite arg->value itself, so calls.c has logic to avoid it. However, it doesn't work properly in this case. What concerned me was that argblock has the value (crtl->args.internal_arg_pointer + 16) rather than just crtl->args.internal_arg_pointer. The code above the emit_push_insn call in store_one_arg that does the pushing of the rogue argument that is supposed to check for the argument overlap case seems to assume that argblock is just crtl->args.internal_arg_pointer Looking around, that '16' is crtl->args.pretend_args_size. From what I understand, this is non-zero when passing an argument partially in registers and we need to take it into account when detecting these overlaps just above the call to emit_push_insn. This patch fixes the issue by looking at the sum of arg->locate.offset.constant and crtl->args.pretend_args_size rather than just arg->locate.offset.constant. With this patch the sibcall is rejected as expected. Bootstrapped and tested on arm, aarch64, x86_64. I didn't see any codegen difference for arm-none-linux-gnueabihf on the whole of SPEC2006 so I hope this is not a high-impact change and it shouldn't reject any legitimate sibcall cases. I'm not terribly familiar with this code and the exact purposes of the various arg_data and crtl->args fields, so please let me know if this is the wrong direction. If this is right, ok for trunk? The wrong-code has been reported the GCC 5 branch as well and I suspect 4.9 is also affected, but I haven't checked. If this is ok, I'd like to test it on those branches for backporting as well. Thanks, Kyrill 2015-11-24 Kyrylo Tkachov PR rtl-optimization/67226 * calls.c (store_one_arg): Take into account crtl->args.pretend_args_size when checking for overlap between arg->value and argblock + arg->locate.offset during sibcall optimization. 2015-11-24 Kyrylo Tkachov PR rtl-optimization/67226 * gcc.c-torture/execute/pr67226.c: New test. commit 28b13fd99664480bfb7f5a606dd89719a0aa0b6f Author: Kyrylo Tkachov Date: Mon Nov 23 13:19:57 2015 +0000 PR rtl-optimization/67226: Take into account pretend_args_size when checking stack offsets for sibcall optimisation diff --git a/gcc/calls.c b/gcc/calls.c index b56556a..233e1e6 100644 --- a/gcc/calls.c +++ b/gcc/calls.c @@ -4944,17 +4944,19 @@ store_one_arg (struct arg_data *arg, rtx argblock, int flags, && arg->locate.size.var == 0 && CONST_INT_P (size_rtx)); - if (arg->locate.offset.constant > i) + int argblock_offset = arg->locate.offset.constant + + crtl->args.pretend_args_size; + if (argblock_offset > i) { - if (arg->locate.offset.constant < i + INTVAL (size_rtx)) + if (argblock_offset < i + INTVAL (size_rtx)) sibcall_failure = 1; } - else if (arg->locate.offset.constant < i) + else if (argblock_offset < i) { /* Use arg->locate.size.constant instead of size_rtx because we only care about the part of the argument on the stack. */ - if (i < (arg->locate.offset.constant + if (i < (argblock_offset + arg->locate.size.constant)) sibcall_failure = 1; } diff --git a/gcc/testsuite/gcc.c-torture/execute/pr67226.c b/gcc/testsuite/gcc.c-torture/execute/pr67226.c new file mode 100644 index 0000000..c533496 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr67226.c @@ -0,0 +1,42 @@ +struct assembly_operand +{ + int type, value, symtype, symflags, marker; +}; + +struct assembly_operand to_input, from_input; + +void __attribute__ ((__noinline__, __noclone__)) +assemblez_1 (int internal_number, struct assembly_operand o1) +{ + if (o1.type != from_input.type) + __builtin_abort (); +} + +void __attribute__ ((__noinline__, __noclone__)) +t0 (struct assembly_operand to, struct assembly_operand from) +{ + if (to.value == 0) + assemblez_1 (32, from); + else + __builtin_abort (); +} + +int +main (void) +{ + to_input.value = 0; + to_input.type = 1; + to_input.symtype = 2; + to_input.symflags = 3; + to_input.marker = 4; + + from_input.value = 5; + from_input.type = 6; + from_input.symtype = 7; + from_input.symflags = 8; + from_input.marker = 9; + + t0 (to_input, from_input); + + return 0; +}