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.bela...@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.
+/* { 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.