From patchwork Tue Nov 17 19:56:05 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kugan Vivekanandarajah X-Patchwork-Id: 56864 Delivered-To: patch@linaro.org Received: by 10.112.155.196 with SMTP id vy4csp2146988lbb; Tue, 17 Nov 2015 11:56:46 -0800 (PST) X-Received: by 10.68.251.170 with SMTP id zl10mr65136409pbc.106.1447790206673; Tue, 17 Nov 2015 11:56:46 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id hm2si1788539pac.186.2015.11.17.11.56.46 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 17 Nov 2015 11:56:46 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-414421-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-414421-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-414421-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 :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=r4jdZslgJWQ9oWS0o ifnQFE4MarDZBQHITNQhfh4DpFpL0IYZA9KkLIrHFU+w1xiQDQTB2jQYdKOaSO4X 3DtIpNdvX0GDMpx/1enNdRzgcZlWt9USCls1RWv6ZIy00cgijT4n7xaugypsRPdT TT9F5MMeE68nO5eK2hXHOZCc3w= 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 :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=JraHjXjTHnHI8G/K+6LZgDR AK18=; b=EaFwR8VzoSaT5J0903DYsvRX1tCNDnaEIttZgPTs6dRZHc1+6PBKpkK lgX2DFGcq/9GvtHK+iSmpgOJvGx9oNyCKoLDh0G8UB9n8zoPG3bSXBraT2IUuZwt 7DUYRUGCoR/nTUoTMLEHhX77hyMRy7AVR0wmRsNTNp/sRnW4ESIQ= Received: (qmail 9184 invoked by alias); 17 Nov 2015 19:56:32 -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 9171 invoked by uid 89); 17 Nov 2015 19:56:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f50.google.com Received: from mail-pa0-f50.google.com (HELO mail-pa0-f50.google.com) (209.85.220.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 17 Nov 2015 19:56:30 +0000 Received: by pabfh17 with SMTP id fh17so19039672pab.0 for ; Tue, 17 Nov 2015 11:56:28 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-type; bh=AE7dIqCU0jTV4keDKUrU9PtPFZZNjzi+VJ4vJvG+hjA=; b=YqsoixdHMCUGOXcbuTIZ2cwtukiBje9FzDIrAkzpaG9jl0w75sSlo1yFzEKqeZ6G4y 3dHeGQ+RW+D7RdZXwDfX6gY8A/7+eux8M74Hpg5GfvsEm4H2RhsRoMAT2/Q+7w93Jo03 miuToItqgX7qR2layemoaitxUPOfrrPMdXEXcUi8Oa0k70OPn1Uka6KafN5VyRgLNfwt 5ewmA5cSa980suHyDX2P6mVWCgcak9MVzKIzkqA6Tn0Y14fqt/kMmOLkNCinQgSBnYR5 mLU2BjZnGQnS2lBJXLmtV6Roixo1YYzN5vrQnkgyhvl1BGY+v7FEcJKVIxLkOrbln4ps +8Mg== X-Gm-Message-State: ALoCoQnvuIlUkOoHS4KewljXvuVjZBOyUUAMrGE1pXrxO9SbXuTLK/fgy7H76umcxVx5hZNRRZkH X-Received: by 10.66.141.73 with SMTP id rm9mr149292pab.96.1447790188589; Tue, 17 Nov 2015 11:56:28 -0800 (PST) Received: from [10.1.1.10] (58-6-183-210.dyn.iinet.net.au. [58.6.183.210]) by smtp.googlemail.com with ESMTPSA id yz3sm44471820pbb.37.2015.11.17.11.56.24 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 17 Nov 2015 11:56:27 -0800 (PST) Subject: Re: Incorrect code due to indirect tail call of varargs function with hard float ABI To: Ramana Radhakrishnan , Charles Baylis References: <564A57BA.7050504@linaro.org> <564A9327.8070607@linaro.org> <564AFBEE.9050801@foss.arm.com> Cc: "gcc-patches@gcc.gnu.org" , Richard Earnshaw , Ramana Radhakrishnan , Kyrill Tkachov From: Kugan Message-ID: <564B8655.1060509@linaro.org> Date: Wed, 18 Nov 2015 06:56:05 +1100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <564AFBEE.9050801@foss.arm.com> X-IsSubscribed: yes On 17/11/15 21:05, Ramana Radhakrishnan wrote: > Hi Kugan, > > It does look like an issue. > > Please open a bug report. > >> >> >> On 17/11/15 12:00, Charles Baylis wrote: >>> On 16 November 2015 at 22:24, Kugan wrote: >>> >>>> Please note that we have a sibcall from "broken" to "indirect". >>>> >>>> "direct" is variadic function so it is conforming to AAPCS base standard. >>>> >>>> "broken" is a non-variadic function and will return the value in >>>> floating point register for TARGET_HARD_FLOAT. Thus we should not be >>>> doing sibcall here. >>>> >>>> Attached patch fixes this. Bootstrap and regression testing is ongoing. >>>> Is this OK if no issues with the testing? >>> >>> Hi Kugan, >>> >>> It looks like this patch should work, but I think this is an overly >>> conservative fix, as it prevents all sibcalls for hardfloat targets. >>> It would be better if only variadic sibcalls were prevented on >>> hardfloat. You can check for variadic calls by checking the >>> function_type in the call expression (exp) using stdarg_p(). >>> >>> As an example to show how to test for variadic function calls, this is >>> how to test it in gdb: >>> >>> (gdb) b arm_function_ok_for_sibcall >>> Breakpoint 1 at 0xdae59c: file >>> /home/cbaylis/srcarea/gcc/gcc-git/gcc/config/arm/arm.c, line 6634. >>> (gdb) r >>> ... >>> Breakpoint 1, arm_function_ok_for_sibcall (decl=0x0, exp=0x7ffff6104ce8) >>> at /home/cbaylis/srcarea/gcc/gcc-git/gcc/config/arm/arm.c:6634 >>> 6634 if (cfun->machine->sibcall_blocked) >>> (gdb) print debug_tree(exp) >>> >> type >> size >>> unit size >>> align 64 symtab 0 alias set -1 canonical type 0x7ffff62835e8 >>> precision 64 >>> pointer_to_this > >>> side-effects addressable >>> fn >> type >>> ... >>> (gdb) print stdarg_p((tree)0x7ffff60e9348) <--- from function_type ^^^^^ >>> $2 = true >>> >> >> How about: > > > > A run time testcase and a changelog would also be needed. > >> >> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c >> index a379121..2376d66 100644 >> --- a/gcc/config/arm/arm.c >> +++ b/gcc/config/arm/arm.c >> @@ -6681,6 +6681,13 @@ arm_function_ok_for_sibcall (tree decl, tree exp) >> register. */ >> rtx a, b; >> >> + /* If it is an indirect function pointer, get the function type. */ >> + if (!decl >> + && POINTER_TYPE_P (TREE_TYPE (CALL_EXPR_FN (exp))) >> + && (TREE_CODE (TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp)))) >> + == FUNCTION_TYPE)) >> + decl = TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp))); >> + > > If decl is null it's guaranteed to be an indirect function call - drop the additional checks in the if clause. > > >> a = arm_function_value (TREE_TYPE (exp), decl, false); >> b = arm_function_value (TREE_TYPE (DECL_RESULT (cfun->decl)), >> cfun->decl, false); >> > > > Please resubmit with a testcase, Changelog and after testing. Hi Ramana, Thanks for the review. I have opened a gcc bug-report for this. I tested the attached patch for arm-none-linux-gnueabihf and arm-none-linux-gnueabi with no new regressions. Is this OK? Thanks, Kugan gcc/ChangeLog: 2015-11-18 Kugan Vivekanandarajah PR target/68390 * config/arm/arm.c (arm_function_ok_for_sibcall): Get function type for indirect function call. gcc/testsuite/ChangeLog: 2015-11-18 Kugan Vivekanandarajah PR target/68390 * gcc.target/arm/PR68390.c: New test. diff --git a/gcc/testsuite/gcc.target/arm/variadic_sibcall.c b/gcc/testsuite/gcc.target/arm/variadic_sibcall.c deleted file mode 100644 index e69de29..0000000 diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index a379121..a4509f4 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -6681,6 +6681,10 @@ arm_function_ok_for_sibcall (tree decl, tree exp) register. */ rtx a, b; + /* If it is an indirect function pointer, get the function type. */ + if (!decl) + decl = TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp))); + a = arm_function_value (TREE_TYPE (exp), decl, false); b = arm_function_value (TREE_TYPE (DECL_RESULT (cfun->decl)), cfun->decl, false); diff --git a/gcc/testsuite/gcc.target/arm/PR68390.c b/gcc/testsuite/gcc.target/arm/PR68390.c index e69de29..86f07fe 100644 --- a/gcc/testsuite/gcc.target/arm/PR68390.c +++ b/gcc/testsuite/gcc.target/arm/PR68390.c @@ -0,0 +1,27 @@ +/* { dg-do run } */ +/* { dg-options "-O2" } */ + +__attribute__ ((noinline)) +double direct(int x, ...) +{ + return x*x; +} + +__attribute__ ((noinline)) +double broken(double (*indirect)(int x, ...), int v) +{ + return indirect(v); +} + +int main () +{ + double d1, d2; + int i = 2; + d1 = broken (direct, i); + if (d1 != i*i) + { + __builtin_abort (); + } + return 0; +} +