From: Tejas Belagod <tejas.belagod@arm.com>
To: "Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com>,
gcc-patches@gcc.gnu.org
Cc: rearnsha@arm.com
Subject: Re: [PATCH] Arm: Fix incorrect tailcall-generation for indirect calls [PR113780]
Date: Thu, 15 Feb 2024 15:23:08 +0530 [thread overview]
Message-ID: <0420a460-aedf-486f-ae04-07095f2ff506@arm.com> (raw)
In-Reply-To: <7d52db28-0d85-4008-91a5-977d19551290@arm.com>
[-- Attachment #1: Type: text/plain, Size: 4611 bytes --]
On 2/14/24 3:55 PM, Richard Earnshaw (lists) wrote:
> On 14/02/2024 09:20, Tejas Belagod wrote:
>> On 2/7/24 11:41 PM, Richard Earnshaw (lists) wrote:
>>> On 07/02/2024 07:59, Tejas Belagod wrote:
>>>> This patch fixes a bug that causes indirect calls in PAC-enabled functions
>>>> to be tailcalled incorrectly when all argument registers R0-R3 are used.
>>>>
>>>> Tested on arm-none-eabi for armv8.1-m.main. OK for trunk?
>>>>
>>>> 2024-02-07 Tejas Belagod <tejas.belagod@arm.com>
>>>>
>>>> PR target/113780
>>>> * gcc/config/arm.cc (arm_function_ok_for_sibcall): Don't allow tailcalls
>>>> for indirect calls with 4 or more arguments in pac-enabled functions.
>>>>
>>>> * gcc.target/arm/pac-sibcall.c: New.
>>>> ---
>>>> gcc/config/arm/arm.cc | 12 ++++++++----
>>>> gcc/testsuite/gcc.target/arm/pac-sibcall.c | 11 +++++++++++
>>>> 2 files changed, 19 insertions(+), 4 deletions(-)
>>>> create mode 100644 gcc/testsuite/gcc.target/arm/pac-sibcall.c
>>>>
>>>> diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
>>>> index c44047c377a..c1f8286a4d4 100644
>>>> --- a/gcc/config/arm/arm.cc
>>>> +++ b/gcc/config/arm/arm.cc
>>>> @@ -7980,10 +7980,14 @@ arm_function_ok_for_sibcall (tree decl, tree exp)
>>>> && DECL_WEAK (decl))
>>>> return false;
>>>> - /* We cannot do a tailcall for an indirect call by descriptor if all the
>>>> - argument registers are used because the only register left to load the
>>>> - address is IP and it will already contain the static chain. */
>>>> - if (!decl && CALL_EXPR_BY_DESCRIPTOR (exp) && !flag_trampolines)
>>>> + /* We cannot do a tailcall for an indirect call by descriptor or for an
>>>> + indirect call in a pac-enabled function if all the argument registers
>>>> + are used because the only register left to load the address is IP and
>>>> + it will already contain the static chain or the PAC signature in the
>>>> + case of PAC-enabled functions. */
>>>
>>> This comment is becoming a bit unwieldy. I suggest restructuring it as:
>>>
>>> We cannot tailcall an indirect call by descriptor if all the call-clobbered
>>> general registers are live (r0-r3 and ip). This can happen when:
>>> - IP contains the static chain, or
>>> - IP is needed for validating the PAC signature.
>>>
>>>
>>>> + if (!decl
>>>> + && ((CALL_EXPR_BY_DESCRIPTOR (exp) && !flag_trampolines)
>>>> + || arm_current_function_pac_enabled_p()))
>>>> {
>>>> tree fntype = TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp)));
>>>> CUMULATIVE_ARGS cum;
>>>> diff --git a/gcc/testsuite/gcc.target/arm/pac-sibcall.c b/gcc/testsuite/gcc.target/arm/pac-sibcall.c
>>>> new file mode 100644
>>>> index 00000000000..c57bf7a952c
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/arm/pac-sibcall.c
>>>> @@ -0,0 +1,11 @@
>>>> +/* Testing return address signing. */
>>>> +/* { dg-do compile } */
>>>> +/* { dg-require-effective-target mbranch_protection_ok } */
>>>> +/* { dg-options " -mcpu=cortex-m85 -mbranch-protection=pac-ret+leaf -O2" } */
>>>
>>> No, you can't just add options like this, you need to first check that they won't result in conflicts with other options on the command line. See https://gcc.gnu.org/pipermail/gcc-patches/2024-January/644077.html for an example of how to handle this.
>>>
>> Thanks for the review, Richard. Respin attached.
>>
>> Thanks,
>> Tejas.
>>
>>>> +
>>>> +void fail(void (*f)(int, int, int, int))
>>>> +{
>>>> + f(1, 2, 3, 4);
>>>> +}
>>>> +
>>>> +/* { dg-final { scan-assembler-not "bx\tip\t@ indirect register sibling call" } } */
>>>
>>> R.
>>>
> +++ b/gcc/testsuite/gcc.target/arm/pac-sibcall.c
> @@ -0,0 +1,14 @@
> +/* If all call-clobbered general registers are live (r0-r3, ip), disable
> + indirect tail-call for a PAC-enabled function. */
> +
> +/* { dg-do compile } */
> +/* { dg-require-effective-target mbranch_protection_ok } */
> This only checks if -mbranch-protection can work with the existing architecture/cpu; not with the flags you're about to add below. You should check for arm_arch_v8_1m_main_pacbti_ok instead; then you can assume that -mbranch-protection can be added.
>
Indeed! Thanks for catching that.
> +/* { dg-add-options arm_arch_v8_1m_main_pacbti } */
> +/* { dg-additional-options "-mbranch-protection=pac-ret+leaf -O2" } */
>
> Otherwise this is OK if you fix the above.
>
Thanks Richard. Respin attached. Will apply.
Thanks,
Tejas.
> R.
[-- Attachment #2: tail-call-m-1.txt --]
[-- Type: text/plain, Size: 2724 bytes --]
diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index c44047c377a802d0c1dc1406df1b88a6b079607b..1cd69268ee986a0953cc85ab259355d2191250ac 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -7980,10 +7980,13 @@ arm_function_ok_for_sibcall (tree decl, tree exp)
&& DECL_WEAK (decl))
return false;
- /* We cannot do a tailcall for an indirect call by descriptor if all the
- argument registers are used because the only register left to load the
- address is IP and it will already contain the static chain. */
- if (!decl && CALL_EXPR_BY_DESCRIPTOR (exp) && !flag_trampolines)
+ /* We cannot tailcall an indirect call by descriptor if all the call-clobbered
+ general registers are live (r0-r3 and ip). This can happen when:
+ - IP contains the static chain, or
+ - IP is needed for validating the PAC signature. */
+ if (!decl
+ && ((CALL_EXPR_BY_DESCRIPTOR (exp) && !flag_trampolines)
+ || arm_current_function_pac_enabled_p()))
{
tree fntype = TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp)));
CUMULATIVE_ARGS cum;
diff --git a/gcc/testsuite/gcc.target/arm/pac-sibcall.c b/gcc/testsuite/gcc.target/arm/pac-sibcall.c
new file mode 100644
index 0000000000000000000000000000000000000000..e15bd2f478d1c9aae5870dd572e5fe043fd4df1f
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pac-sibcall.c
@@ -0,0 +1,14 @@
+/* If all call-clobbered general registers are live (r0-r3, ip), disable
+ indirect tail-call for a PAC-enabled function. */
+
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_arch_v8_1m_main_pacbti_ok } */
+/* { dg-add-options arm_arch_v8_1m_main_pacbti } */
+/* { dg-additional-options "-mbranch-protection=pac-ret+leaf -O2" } */
+
+void fail(void (*f)(int, int, int, int))
+{
+ f(1, 2, 3, 4);
+}
+
+/* { dg-final { scan-assembler-not "bx\tip\t@ indirect register sibling call" } } */
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index b1faaf4aa955efdb78cdc13a139a0a11b24012a7..0af2d3ea80fb1fd8966f2806fe87e1f89da261f3 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -5534,7 +5534,7 @@ foreach { armfunc armflag armdefs } {
v8m_main "-march=armv8-m.main+fp -mthumb" __ARM_ARCH_8M_MAIN__
v8_1m_main "-march=armv8.1-m.main+fp -mthumb" __ARM_ARCH_8M_MAIN__
v8_1m_main_pacbti "-march=armv8.1-m.main+pacbti+fp -mthumb"
- "__ARM_ARCH_8M_MAIN__ && __ARM_FEATURE_BTI"
+ "__ARM_ARCH_8M_MAIN__ && __ARM_FEATURE_BTI && __ARM_FEATURE_PAUTH"
v9a "-march=armv9-a+simd" __ARM_ARCH_9A__ } {
eval [string map [list FUNC $armfunc FLAG $armflag DEFS $armdefs ] {
proc check_effective_target_arm_arch_FUNC_ok { } {
prev parent reply other threads:[~2024-02-15 9:53 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-07 7:59 Tejas Belagod
2024-02-07 18:11 ` Richard Earnshaw (lists)
2024-02-14 9:20 ` Tejas Belagod
2024-02-14 10:25 ` Richard Earnshaw (lists)
2024-02-15 9:53 ` Tejas Belagod [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0420a460-aedf-486f-ae04-07095f2ff506@arm.com \
--to=tejas.belagod@arm.com \
--cc=Richard.Earnshaw@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=rearnsha@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).