Re: [PATCH] Arm: Fix incorrect tailcall-generation for indirect calls [PR113780]
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
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 000..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.
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
Re: [PATCH] Arm: Fix incorrect tailcall-generation for indirect calls [PR113780]
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
>>>
>>> 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 000..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.
+/* { 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.
R.
Re: [PATCH] Arm: Fix incorrect tailcall-generation for indirect calls [PR113780]
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
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 000..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.
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
..29686ad8ecbbb1eeff862d827c27ff3721bfa8ea
--- /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 mbranch_protection_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
Re: [PATCH] Arm: Fix incorrect tailcall-generation for indirect calls [PR113780]
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
>
> 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 000..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.
> +
> +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.
[PATCH] Arm: Fix incorrect tailcall-generation for indirect calls [PR113780]
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
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. */
+ 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 000..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" } */
+
+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"
} } */
--
2.25.1
