[PATCH v2 0/7] aarch64 GCS preliminary patches

[PATCH v2 0/7] aarch64 GCS preliminary patches

[Szabolcs Nagy]

I'm working on Guarded Control Stack support for aarch64 and have a
set of patches that are needed for GCS but seem useful without it so
makes sense to review them separately from the rest of the GCS work.

previous version:
https://gcc.gnu.org/pipermail/gcc-patches/2023-August/628123.html

Szabolcs Nagy (7):
  aarch64: Use br instead of ret for eh_return
  aarch64: Do not force a stack frame for EH returns
  aarch64: Add eh_return compile tests
  aarch64: Disable branch-protection for pcs tests
  aarch64,arm: Remove accepted_branch_protection_string
  aarch64,arm: Fix branch-protection= parsing
  aarch64,arm: Move branch-protection data to targets

 gcc/config/aarch64/aarch64-opts.h             |   6 +-
 gcc/config/aarch64/aarch64-protos.h           |   1 -
 gcc/config/aarch64/aarch64.cc                 | 193 +++++++--------
 gcc/config/aarch64/aarch64.h                  |   9 +-
 gcc/config/arm/aarch-common-protos.h          |   5 +-
 gcc/config/arm/aarch-common.cc                | 229 +++++-------------
 gcc/config/arm/aarch-common.h                 |  25 +-
 gcc/config/arm/arm-c.cc                       |   2 -
 gcc/config/arm/arm.cc                         |  57 ++++-
 gcc/config/arm/arm.opt                        |   3 -
 gcc/df-scan.cc                                |  10 +
 gcc/doc/tm.texi                               |  12 +
 gcc/doc/tm.texi.in                            |  12 +
 gcc/except.cc                                 |  20 ++
 .../gcc.target/aarch64/aapcs64/func-ret-1.c   |   1 +
 .../gcc.target/aarch64/aapcs64/func-ret-2.c   |   1 +
 .../gcc.target/aarch64/aapcs64/func-ret-3.c   |   1 +
 .../gcc.target/aarch64/aapcs64/func-ret-4.c   |   1 +
 .../aarch64/aapcs64/func-ret-64x1_1.c         |   1 +
 .../aarch64/branch-protection-attr.c          |   6 +-
 .../aarch64/branch-protection-option.c        |   2 +-
 .../gcc.target/aarch64/eh_return-2.c          |   9 +
 .../gcc.target/aarch64/eh_return-3.c          |  30 +++
 .../aarch64/return_address_sign_1.c           |  13 +-
 .../aarch64/return_address_sign_2.c           |  17 +-
 .../aarch64/return_address_sign_b_1.c         |  11 -
 .../aarch64/return_address_sign_b_2.c         |  17 +-
 27 files changed, 356 insertions(+), 338 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/eh_return-2.c
 create mode 100644 gcc/testsuite/gcc.target/aarch64/eh_return-3.c

-- 
2.25.1

[PATCH v2 1/7] aarch64: Use br instead of ret for eh_return

[Szabolcs Nagy]

The expected way to handle eh_return is to pass the stack adjustment
offset and landing pad address via

  EH_RETURN_STACKADJ_RTX
  EH_RETURN_HANDLER_RTX

to the epilogue that is shared between normal return paths and the
eh_return paths.  EH_RETURN_HANDLER_RTX is the stack slot of the
return address that is overwritten with the landing pad in the
eh_return case and EH_RETURN_STACKADJ_RTX is a register added to sp
right before return and it is set to 0 in the normal return case.

The issue with this design is that eh_return and normal return may
require different return sequence but there is no way to distinguish
the two cases in the epilogue (the stack adjustment may be 0 in the
eh_return case too).

The reason eh_return and normal return requires different return
sequence is that control flow integrity hardening may need to treat
eh_return as a forward-edge transfer (it is not returning to the
previous stack frame) and normal return as a backward-edge one.
In case of AArch64 forward-edge is protected by BTI and requires br
instruction and backward-edge is protected by PAUTH or GCS and
requires ret (or authenticated ret) instruction.

This patch resolves the issue by introducing EH_RETURN_TAKEN_RTX that
is a flag set to 1 in the eh_return path and 0 in normal return paths.
Branching on the EH_RETURN_TAKEN_RTX flag, the right return sequence
can be used in the epilogue.

The handler could be passed the old way via clobbering the return
address, but since now the eh_return case can be distinguished, the
handler can be in a different register than x30 and no stack frame
is needed for eh_return.

This patch fixes a return to anywhere gadget in the unwinder with
existing standard branch protection as well as makes EH return
compatible with the Guarded Control Stack (GCS) extension.

Some tests are adjusted because eh_return no longer prevents pac-ret
in the normal return path.

gcc/ChangeLog:

	* config/aarch64/aarch64-protos.h (aarch64_eh_return_handler_rtx):
	Remove.
	* config/aarch64/aarch64.cc (aarch64_return_address_signing_enabled):
	Sign return address even in functions with eh_return.
	(aarch64_expand_epilogue): Conditionally return with br or ret.
	(aarch64_eh_return_handler_rtx): Remove.
	* config/aarch64/aarch64.h (EH_RETURN_TAKEN_RTX): Define.
	(EH_RETURN_STACKADJ_RTX): Change to R5.
	(EH_RETURN_HANDLER_RTX): Change to R6.
	* df-scan.cc: Handle EH_RETURN_TAKEN_RTX.
	* doc/tm.texi: Regenerate.
	* doc/tm.texi.in: Document EH_RETURN_TAKEN_RTX.
	* except.cc (expand_eh_return): Handle EH_RETURN_TAKEN_RTX.

gcc/testsuite/ChangeLog:

	* gcc.target/aarch64/return_address_sign_1.c: Move func4 to ...
	* gcc.target/aarch64/return_address_sign_2.c: ... here and fix the
	scan asm check.
	* gcc.target/aarch64/return_address_sign_b_1.c: Move func4 to ...
	* gcc.target/aarch64/return_address_sign_b_2.c: ... here and fix the
	scan asm check.

---
v2:
- Introduce EH_RETURN_TAKEN_RTX instead of abusing EH_RETURN_STACKADJ_RTX.
- Merge test fixes.
---
 gcc/config/aarch64/aarch64-protos.h           |  1 -
 gcc/config/aarch64/aarch64.cc                 | 88 ++++++-------------
 gcc/config/aarch64/aarch64.h                  |  9 +-
 gcc/df-scan.cc                                | 10 +++
 gcc/doc/tm.texi                               | 12 +++
 gcc/doc/tm.texi.in                            | 12 +++
 gcc/except.cc                                 | 20 +++++
 .../aarch64/return_address_sign_1.c           | 13 +--
 .../aarch64/return_address_sign_2.c           | 17 +++-
 .../aarch64/return_address_sign_b_1.c         | 11 ---
 .../aarch64/return_address_sign_b_2.c         | 17 +++-
 11 files changed, 116 insertions(+), 94 deletions(-)

diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index 60a55f4bc19..80296024f04 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -859,7 +859,6 @@ machine_mode aarch64_hard_regno_caller_save_mode (unsigned, unsigned,
 						       machine_mode);
 int aarch64_uxt_size (int, HOST_WIDE_INT);
 int aarch64_vec_fpconst_pow_of_2 (rtx);
-rtx aarch64_eh_return_handler_rtx (void);
 rtx aarch64_mask_from_zextract_ops (rtx, rtx);
 const char *aarch64_output_move_struct (rtx *operands);
 rtx aarch64_return_addr_rtx (void);
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index a28b66acf6a..5cdb33dd3dc 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -9113,17 +9113,6 @@ aarch64_return_address_signing_enabled (void)
   /* This function should only be called after frame laid out.   */
   gcc_assert (cfun->machine->frame.laid_out);
 
-  /* Turn return address signing off in any function that uses
-     __builtin_eh_return.  The address passed to __builtin_eh_return
-     is not signed so either it has to be signed (with original sp)
-     or the code path that uses it has to avoid authenticating it.
-     Currently eh return introduces a return to anywhere gadget, no
-     matter what we do here since it uses ret with user provided
-     address. An ideal fix for that is to use indirect branch which
-     can be protected with BTI j (to some extent).  */
-  if (crtl->calls_eh_return)
-    return false;
-
   /* If signing scope is AARCH_FUNCTION_NON_LEAF, we only sign a leaf function
      if its LR is pushed onto stack.  */
   return (aarch_ra_sign_scope == AARCH_FUNCTION_ALL
@@ -10060,11 +10049,7 @@ aarch64_allocate_and_probe_stack_space (rtx temp1, rtx temp2,
 /* Return 1 if the register is used by the epilogue.  We need to say the
    return register is used, but only after epilogue generation is complete.
    Note that in the case of sibcalls, the values "used by the epilogue" are
-   considered live at the start of the called function.
-
-   For SIMD functions we need to return 1 for FP registers that are saved and
-   restored by a function but are not zero in call_used_regs.  If we do not do 
-   this optimizations may remove the restore of the register.  */
+   considered live at the start of the called function.  */
 
 int
 aarch64_epilogue_uses (int regno)
@@ -10468,6 +10453,30 @@ aarch64_expand_epilogue (bool for_sibcall)
       RTX_FRAME_RELATED_P (insn) = 1;
     }
 
+  /* Stack adjustment for exception handler.  */
+  if (crtl->calls_eh_return && !for_sibcall)
+    {
+      /* If the EH_RETURN_TAKEN_RTX flag is set then we need
+	 to unwind the stack and jump to the handler, otherwise
+	 skip this eh_return logic and continue with normal
+	 return after the label.  We have already reset the CFA
+	 to be SP; letting the CFA move during this adjustment
+	 is just as correct as retaining the CFA from the body
+	 of the function.  Therefore, do nothing special.  */
+      rtx label = gen_label_rtx ();
+      rtx x = gen_rtx_EQ (VOIDmode, EH_RETURN_TAKEN_RTX, const0_rtx);
+      x = gen_rtx_IF_THEN_ELSE (VOIDmode, x,
+				gen_rtx_LABEL_REF (Pmode, label), pc_rtx);
+      rtx jump = emit_jump_insn (gen_rtx_SET (pc_rtx, x));
+      JUMP_LABEL (jump) = label;
+      LABEL_NUSES (label)++;
+      emit_insn (gen_add2_insn (stack_pointer_rtx,
+				EH_RETURN_STACKADJ_RTX));
+      emit_jump_insn (gen_indirect_jump (EH_RETURN_HANDLER_RTX));
+      emit_barrier ();
+      emit_label (label);
+    }
+
   /* We prefer to emit the combined return/authenticate instruction RETAA,
      however there are three cases in which we must instead emit an explicit
      authentication instruction.
@@ -10497,58 +10506,11 @@ aarch64_expand_epilogue (bool for_sibcall)
       RTX_FRAME_RELATED_P (insn) = 1;
     }
 
-  /* Stack adjustment for exception handler.  */
-  if (crtl->calls_eh_return && !for_sibcall)
-    {
-      /* We need to unwind the stack by the offset computed by
-	 EH_RETURN_STACKADJ_RTX.  We have already reset the CFA
-	 to be SP; letting the CFA move during this adjustment
-	 is just as correct as retaining the CFA from the body
-	 of the function.  Therefore, do nothing special.  */
-      emit_insn (gen_add2_insn (stack_pointer_rtx, EH_RETURN_STACKADJ_RTX));
-    }
-
   emit_use (gen_rtx_REG (DImode, LR_REGNUM));
   if (!for_sibcall)
     emit_jump_insn (ret_rtx);
 }
 
-/* Implement EH_RETURN_HANDLER_RTX.  EH returns need to either return
-   normally or return to a previous frame after unwinding.
-
-   An EH return uses a single shared return sequence.  The epilogue is
-   exactly like a normal epilogue except that it has an extra input
-   register (EH_RETURN_STACKADJ_RTX) which contains the stack adjustment
-   that must be applied after the frame has been destroyed.  An extra label
-   is inserted before the epilogue which initializes this register to zero,
-   and this is the entry point for a normal return.
-
-   An actual EH return updates the return address, initializes the stack
-   adjustment and jumps directly into the epilogue (bypassing the zeroing
-   of the adjustment).  Since the return address is typically saved on the
-   stack when a function makes a call, the saved LR must be updated outside
-   the epilogue.
-
-   This poses problems as the store is generated well before the epilogue,
-   so the offset of LR is not known yet.  Also optimizations will remove the
-   store as it appears dead, even after the epilogue is generated (as the
-   base or offset for loading LR is different in many cases).
-
-   To avoid these problems this implementation forces the frame pointer
-   in eh_return functions so that the location of LR is fixed and known early.
-   It also marks the store volatile, so no optimization is permitted to
-   remove the store.  */
-rtx
-aarch64_eh_return_handler_rtx (void)
-{
-  rtx tmp = gen_frame_mem (Pmode,
-    plus_constant (Pmode, hard_frame_pointer_rtx, UNITS_PER_WORD));
-
-  /* Mark the store volatile, so no optimization is permitted to remove it.  */
-  MEM_VOLATILE_P (tmp) = true;
-  return tmp;
-}
-
 /* Output code to add DELTA to the first argument, and then jump
    to FUNCTION.  Used for C++ multiple inheritance.  */
 static void
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 2f0777a37ac..58f7eeb565f 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -583,9 +583,12 @@ enum class aarch64_feature : unsigned char {
 /* Output assembly strings after .cfi_startproc is emitted.  */
 #define ASM_POST_CFI_STARTPROC  aarch64_post_cfi_startproc
 
-/* For EH returns X4 contains the stack adjustment.  */
-#define EH_RETURN_STACKADJ_RTX	gen_rtx_REG (Pmode, R4_REGNUM)
-#define EH_RETURN_HANDLER_RTX  aarch64_eh_return_handler_rtx ()
+/* For EH returns X4 is a flag that is set in the EH return
+   code paths and then X5 and X6 contain the stack adjustment
+   and return address respectively.  */
+#define EH_RETURN_TAKEN_RTX	gen_rtx_REG (Pmode, R4_REGNUM)
+#define EH_RETURN_STACKADJ_RTX	gen_rtx_REG (Pmode, R5_REGNUM)
+#define EH_RETURN_HANDLER_RTX	gen_rtx_REG (Pmode, R6_REGNUM)
 
 #undef TARGET_COMPUTE_FRAME_LAYOUT
 #define TARGET_COMPUTE_FRAME_LAYOUT aarch64_layout_frame
diff --git a/gcc/df-scan.cc b/gcc/df-scan.cc
index 9515740728c..934c9ca2d81 100644
--- a/gcc/df-scan.cc
+++ b/gcc/df-scan.cc
@@ -3720,6 +3720,16 @@ df_get_exit_block_use_set (bitmap exit_block_uses)
     }
 #endif
 
+#ifdef EH_RETURN_TAKEN_RTX
+  if ((!targetm.have_epilogue () || ! epilogue_completed)
+      && crtl->calls_eh_return)
+    {
+      rtx tmp = EH_RETURN_TAKEN_RTX;
+      if (tmp && REG_P (tmp))
+	df_mark_reg (tmp, exit_block_uses);
+    }
+#endif
+
   if ((!targetm.have_epilogue () || ! epilogue_completed)
       && crtl->calls_eh_return)
     {
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index f7ac806ff15..21bfe160a8b 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -3506,6 +3506,18 @@ If you want to support call frame exception handling, you must
 define either this macro or the @code{eh_return} instruction pattern.
 @end defmac
 
+@defmac EH_RETURN_TAKEN_RTX
+A C expression whose value is RTL representing a location in which
+to store if the EH return path was taken instead of a normal return.
+This macro allows conditionally executing different code in the
+epilogue for the EH and normal return cases.
+
+When this macro is defined, the macros @code{EH_RETURN_STACKADJ_RTX}
+and @code{EH_RETURN_HANDLER_RTX} are only meaningful in the epilogue
+when 1 is stored to the specified location. The value 0 means normal
+return.
+@end defmac
+
 @defmac RETURN_ADDR_OFFSET
 If defined, an integer-valued C expression for which rtl will be generated
 to add it to the exception handler address before it is searched in the
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 141027e0bb4..ee9dc5c5fc3 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -2742,6 +2742,18 @@ If you want to support call frame exception handling, you must
 define either this macro or the @code{eh_return} instruction pattern.
 @end defmac
 
+@defmac EH_RETURN_TAKEN_RTX
+A C expression whose value is RTL representing a location in which
+to store if the EH return path was taken instead of a normal return.
+This macro allows conditionally executing different code in the
+epilogue for the EH and normal return cases.
+
+When this macro is defined, the macros @code{EH_RETURN_STACKADJ_RTX}
+and @code{EH_RETURN_HANDLER_RTX} are only meaningful in the epilogue
+when 1 is stored to the specified location. The value 0 means normal
+return.
+@end defmac
+
 @defmac RETURN_ADDR_OFFSET
 If defined, an integer-valued C expression for which rtl will be generated
 to add it to the exception handler address before it is searched in the
diff --git a/gcc/except.cc b/gcc/except.cc
index e728aa43b6a..508f8bb3e26 100644
--- a/gcc/except.cc
+++ b/gcc/except.cc
@@ -2281,6 +2281,10 @@ expand_eh_return (void)
   emit_move_insn (EH_RETURN_STACKADJ_RTX, const0_rtx);
 #endif
 
+#ifdef EH_RETURN_TAKEN_RTX
+  emit_move_insn (EH_RETURN_TAKEN_RTX, const0_rtx);
+#endif
+
   around_label = gen_label_rtx ();
   emit_jump (around_label);
 
@@ -2291,6 +2295,10 @@ expand_eh_return (void)
   emit_move_insn (EH_RETURN_STACKADJ_RTX, crtl->eh.ehr_stackadj);
 #endif
 
+#ifdef EH_RETURN_TAKEN_RTX
+  emit_move_insn (EH_RETURN_TAKEN_RTX, const1_rtx);
+#endif
+
   if (targetm.have_eh_return ())
     emit_insn (targetm.gen_eh_return (crtl->eh.ehr_handler));
   else
@@ -2301,7 +2309,19 @@ expand_eh_return (void)
 	error ("%<__builtin_eh_return%> not supported on this target");
     }
 
+#ifdef EH_RETURN_TAKEN_RTX
+  rtx_code_label *eh_done_label = gen_label_rtx ();
+  emit_jump (eh_done_label);
+#endif
+
   emit_label (around_label);
+
+#ifdef EH_RETURN_TAKEN_RTX
+  for (rtx tmp : { EH_RETURN_STACKADJ_RTX, EH_RETURN_HANDLER_RTX })
+    if (tmp && REG_P (tmp))
+      emit_clobber (tmp);
+  emit_label (eh_done_label);
+#endif
 }
 
 /* Convert a ptr_mode address ADDR_TREE to a Pmode address controlled by
diff --git a/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c b/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c
index 232ba67ade0..114a9dacb3f 100644
--- a/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c
@@ -37,16 +37,5 @@ func3 (int a, int b, int c)
   /* autiasp */
 }
 
-/* eh_return.  */
-void __attribute__ ((target ("arch=armv8.3-a")))
-func4 (long offset, void *handler, int *ptr, int imm1, int imm2)
-{
-  /* no paciasp */
-  *ptr = imm1 + foo (imm1) + imm2;
-  __builtin_eh_return (offset, handler);
-  /* no autiasp */
-  return;
-}
-
-/* { dg-final { scan-assembler-times "autiasp" 3 } } */
 /* { dg-final { scan-assembler-times "paciasp" 3 } } */
+/* { dg-final { scan-assembler-times "autiasp" 3 } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/return_address_sign_2.c b/gcc/testsuite/gcc.target/aarch64/return_address_sign_2.c
index a4bc5b45333..d93492c3c43 100644
--- a/gcc/testsuite/gcc.target/aarch64/return_address_sign_2.c
+++ b/gcc/testsuite/gcc.target/aarch64/return_address_sign_2.c
@@ -14,5 +14,18 @@ func1 (int a, int b, int c)
   /* retaa */
 }
 
-/* { dg-final { scan-assembler-times "paciasp" 1 } } */
-/* { dg-final { scan-assembler-times "retaa" 1 } } */
+/* eh_return.  */
+void __attribute__ ((target ("arch=armv8.3-a")))
+func4 (long offset, void *handler, int *ptr, int imm1, int imm2)
+{
+  /* paciasp */
+  *ptr = imm1 + foo (imm1) + imm2;
+  if (handler)
+    /* br */
+    __builtin_eh_return (offset, handler);
+  /* retaa */
+  return;
+}
+
+/* { dg-final { scan-assembler-times "paciasp" 2 } } */
+/* { dg-final { scan-assembler-times "retaa" 2 } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_1.c b/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_1.c
index 43e32ab6cb7..697fa30dc5a 100644
--- a/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_1.c
@@ -37,16 +37,5 @@ func3 (int a, int b, int c)
   /* autibsp */
 }
 
-/* eh_return.  */
-void __attribute__ ((target ("arch=armv8.3-a")))
-func4 (long offset, void *handler, int *ptr, int imm1, int imm2)
-{
-  /* no pacibsp */
-  *ptr = imm1 + foo (imm1) + imm2;
-  __builtin_eh_return (offset, handler);
-  /* no autibsp */
-  return;
-}
-
 /* { dg-final { scan-assembler-times "pacibsp" 3 } } */
 /* { dg-final { scan-assembler-times "autibsp" 3 } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_2.c b/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_2.c
index 9ed64ce0591..452240b731e 100644
--- a/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_2.c
+++ b/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_2.c
@@ -14,5 +14,18 @@ func1 (int a, int b, int c)
   /* retab */
 }
 
-/* { dg-final { scan-assembler-times "pacibsp" 1 } } */
-/* { dg-final { scan-assembler-times "retab" 1 } } */
+/* eh_return.  */
+void __attribute__ ((target ("arch=armv8.3-a")))
+func4 (long offset, void *handler, int *ptr, int imm1, int imm2)
+{
+  /* pacibsp */
+  *ptr = imm1 + foo (imm1) + imm2;
+  if (handler)
+    /* br */
+    __builtin_eh_return (offset, handler);
+  /* retab */
+  return;
+}
+
+/* { dg-final { scan-assembler-times "pacibsp" 2 } } */
+/* { dg-final { scan-assembler-times "retab" 2 } } */
-- 
2.25.1

Re: [PATCH v2 1/7] aarch64: Use br instead of ret for eh_return

[Hans-Peter Nilsson]

> From: Szabolcs Nagy <szabolcs.nagy@arm.com>
> Date: Fri, 3 Nov 2023 15:36:08 +0000

I don't see others commenting on this patch, and you're not
mentioning this aspect, so I wonder:

> 	* config/aarch64/aarch64.h (EH_RETURN_TAKEN_RTX): Define.
> 	(EH_RETURN_STACKADJ_RTX): Change to R5.
> 	(EH_RETURN_HANDLER_RTX): Change to R6.

Isn't this an ABI change?

(I've forgotten relevant bits of the exception machinery; if
throw and catch are always in the same object and everything
in between register-number-agnostic then the only flaw would
be not mentioning that in the commit message.)

brgds, H-P

Re: [PATCH v2 1/7] aarch64: Use br instead of ret for eh_return

[Szabolcs Nagy]

The 11/13/2023 01:27, Hans-Peter Nilsson wrote:
> > From: Szabolcs Nagy <szabolcs.nagy@arm.com>
> > Date: Fri, 3 Nov 2023 15:36:08 +0000
> 
> I don't see others commenting on this patch, and you're not
> mentioning this aspect, so I wonder:
> 
> > 	* config/aarch64/aarch64.h (EH_RETURN_TAKEN_RTX): Define.
> > 	(EH_RETURN_STACKADJ_RTX): Change to R5.
> > 	(EH_RETURN_HANDLER_RTX): Change to R6.
> 
> Isn't this an ABI change?

not really: this is interface between the function body
and the epilogue, so all within the code of a single
function doing eh return, not a public abi boundary.

(e.g. R0..R3 are preserved from the function throwing
the exception to the exception handler, so that's abi.
R4..R6 are just an internal detail of the function doing
the eh return in the unwinder.)

> 
> (I've forgotten relevant bits of the exception machinery; if
> throw and catch are always in the same object and everything
> in between register-number-agnostic then the only flaw would
> be not mentioning that in the commit message.)
> 
> brgds, H-P

Re: [PATCH v2 1/7] aarch64: Use br instead of ret for eh_return

[Richard Sandiford]

Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> The expected way to handle eh_return is to pass the stack adjustment
> offset and landing pad address via
>
>   EH_RETURN_STACKADJ_RTX
>   EH_RETURN_HANDLER_RTX
>
> to the epilogue that is shared between normal return paths and the
> eh_return paths.  EH_RETURN_HANDLER_RTX is the stack slot of the
> return address that is overwritten with the landing pad in the
> eh_return case and EH_RETURN_STACKADJ_RTX is a register added to sp
> right before return and it is set to 0 in the normal return case.
>
> The issue with this design is that eh_return and normal return may
> require different return sequence but there is no way to distinguish
> the two cases in the epilogue (the stack adjustment may be 0 in the
> eh_return case too).
>
> The reason eh_return and normal return requires different return
> sequence is that control flow integrity hardening may need to treat
> eh_return as a forward-edge transfer (it is not returning to the
> previous stack frame) and normal return as a backward-edge one.
> In case of AArch64 forward-edge is protected by BTI and requires br
> instruction and backward-edge is protected by PAUTH or GCS and
> requires ret (or authenticated ret) instruction.
>
> This patch resolves the issue by introducing EH_RETURN_TAKEN_RTX that
> is a flag set to 1 in the eh_return path and 0 in normal return paths.
> Branching on the EH_RETURN_TAKEN_RTX flag, the right return sequence
> can be used in the epilogue.
>
> The handler could be passed the old way via clobbering the return
> address, but since now the eh_return case can be distinguished, the
> handler can be in a different register than x30 and no stack frame
> is needed for eh_return.
>
> This patch fixes a return to anywhere gadget in the unwinder with
> existing standard branch protection as well as makes EH return
> compatible with the Guarded Control Stack (GCS) extension.
>
> Some tests are adjusted because eh_return no longer prevents pac-ret
> in the normal return path.
>
> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64-protos.h (aarch64_eh_return_handler_rtx):
> 	Remove.
> 	* config/aarch64/aarch64.cc (aarch64_return_address_signing_enabled):
> 	Sign return address even in functions with eh_return.
> 	(aarch64_expand_epilogue): Conditionally return with br or ret.
> 	(aarch64_eh_return_handler_rtx): Remove.
> 	* config/aarch64/aarch64.h (EH_RETURN_TAKEN_RTX): Define.
> 	(EH_RETURN_STACKADJ_RTX): Change to R5.
> 	(EH_RETURN_HANDLER_RTX): Change to R6.
> 	* df-scan.cc: Handle EH_RETURN_TAKEN_RTX.
> 	* doc/tm.texi: Regenerate.
> 	* doc/tm.texi.in: Document EH_RETURN_TAKEN_RTX.
> 	* except.cc (expand_eh_return): Handle EH_RETURN_TAKEN_RTX.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/aarch64/return_address_sign_1.c: Move func4 to ...
> 	* gcc.target/aarch64/return_address_sign_2.c: ... here and fix the
> 	scan asm check.
> 	* gcc.target/aarch64/return_address_sign_b_1.c: Move func4 to ...
> 	* gcc.target/aarch64/return_address_sign_b_2.c: ... here and fix the
> 	scan asm check.

OK, thanks!

Richard

> ---
> v2:
> - Introduce EH_RETURN_TAKEN_RTX instead of abusing EH_RETURN_STACKADJ_RTX.
> - Merge test fixes.
> ---
>  gcc/config/aarch64/aarch64-protos.h           |  1 -
>  gcc/config/aarch64/aarch64.cc                 | 88 ++++++-------------
>  gcc/config/aarch64/aarch64.h                  |  9 +-
>  gcc/df-scan.cc                                | 10 +++
>  gcc/doc/tm.texi                               | 12 +++
>  gcc/doc/tm.texi.in                            | 12 +++
>  gcc/except.cc                                 | 20 +++++
>  .../aarch64/return_address_sign_1.c           | 13 +--
>  .../aarch64/return_address_sign_2.c           | 17 +++-
>  .../aarch64/return_address_sign_b_1.c         | 11 ---
>  .../aarch64/return_address_sign_b_2.c         | 17 +++-
>  11 files changed, 116 insertions(+), 94 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
> index 60a55f4bc19..80296024f04 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -859,7 +859,6 @@ machine_mode aarch64_hard_regno_caller_save_mode (unsigned, unsigned,
>  						       machine_mode);
>  int aarch64_uxt_size (int, HOST_WIDE_INT);
>  int aarch64_vec_fpconst_pow_of_2 (rtx);
> -rtx aarch64_eh_return_handler_rtx (void);
>  rtx aarch64_mask_from_zextract_ops (rtx, rtx);
>  const char *aarch64_output_move_struct (rtx *operands);
>  rtx aarch64_return_addr_rtx (void);
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index a28b66acf6a..5cdb33dd3dc 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -9113,17 +9113,6 @@ aarch64_return_address_signing_enabled (void)
>    /* This function should only be called after frame laid out.   */
>    gcc_assert (cfun->machine->frame.laid_out);
>  
> -  /* Turn return address signing off in any function that uses
> -     __builtin_eh_return.  The address passed to __builtin_eh_return
> -     is not signed so either it has to be signed (with original sp)
> -     or the code path that uses it has to avoid authenticating it.
> -     Currently eh return introduces a return to anywhere gadget, no
> -     matter what we do here since it uses ret with user provided
> -     address. An ideal fix for that is to use indirect branch which
> -     can be protected with BTI j (to some extent).  */
> -  if (crtl->calls_eh_return)
> -    return false;
> -
>    /* If signing scope is AARCH_FUNCTION_NON_LEAF, we only sign a leaf function
>       if its LR is pushed onto stack.  */
>    return (aarch_ra_sign_scope == AARCH_FUNCTION_ALL
> @@ -10060,11 +10049,7 @@ aarch64_allocate_and_probe_stack_space (rtx temp1, rtx temp2,
>  /* Return 1 if the register is used by the epilogue.  We need to say the
>     return register is used, but only after epilogue generation is complete.
>     Note that in the case of sibcalls, the values "used by the epilogue" are
> -   considered live at the start of the called function.
> -
> -   For SIMD functions we need to return 1 for FP registers that are saved and
> -   restored by a function but are not zero in call_used_regs.  If we do not do 
> -   this optimizations may remove the restore of the register.  */
> +   considered live at the start of the called function.  */
>  
>  int
>  aarch64_epilogue_uses (int regno)
> @@ -10468,6 +10453,30 @@ aarch64_expand_epilogue (bool for_sibcall)
>        RTX_FRAME_RELATED_P (insn) = 1;
>      }
>  
> +  /* Stack adjustment for exception handler.  */
> +  if (crtl->calls_eh_return && !for_sibcall)
> +    {
> +      /* If the EH_RETURN_TAKEN_RTX flag is set then we need
> +	 to unwind the stack and jump to the handler, otherwise
> +	 skip this eh_return logic and continue with normal
> +	 return after the label.  We have already reset the CFA
> +	 to be SP; letting the CFA move during this adjustment
> +	 is just as correct as retaining the CFA from the body
> +	 of the function.  Therefore, do nothing special.  */
> +      rtx label = gen_label_rtx ();
> +      rtx x = gen_rtx_EQ (VOIDmode, EH_RETURN_TAKEN_RTX, const0_rtx);
> +      x = gen_rtx_IF_THEN_ELSE (VOIDmode, x,
> +				gen_rtx_LABEL_REF (Pmode, label), pc_rtx);
> +      rtx jump = emit_jump_insn (gen_rtx_SET (pc_rtx, x));
> +      JUMP_LABEL (jump) = label;
> +      LABEL_NUSES (label)++;
> +      emit_insn (gen_add2_insn (stack_pointer_rtx,
> +				EH_RETURN_STACKADJ_RTX));
> +      emit_jump_insn (gen_indirect_jump (EH_RETURN_HANDLER_RTX));
> +      emit_barrier ();
> +      emit_label (label);
> +    }
> +
>    /* We prefer to emit the combined return/authenticate instruction RETAA,
>       however there are three cases in which we must instead emit an explicit
>       authentication instruction.
> @@ -10497,58 +10506,11 @@ aarch64_expand_epilogue (bool for_sibcall)
>        RTX_FRAME_RELATED_P (insn) = 1;
>      }
>  
> -  /* Stack adjustment for exception handler.  */
> -  if (crtl->calls_eh_return && !for_sibcall)
> -    {
> -      /* We need to unwind the stack by the offset computed by
> -	 EH_RETURN_STACKADJ_RTX.  We have already reset the CFA
> -	 to be SP; letting the CFA move during this adjustment
> -	 is just as correct as retaining the CFA from the body
> -	 of the function.  Therefore, do nothing special.  */
> -      emit_insn (gen_add2_insn (stack_pointer_rtx, EH_RETURN_STACKADJ_RTX));
> -    }
> -
>    emit_use (gen_rtx_REG (DImode, LR_REGNUM));
>    if (!for_sibcall)
>      emit_jump_insn (ret_rtx);
>  }
>  
> -/* Implement EH_RETURN_HANDLER_RTX.  EH returns need to either return
> -   normally or return to a previous frame after unwinding.
> -
> -   An EH return uses a single shared return sequence.  The epilogue is
> -   exactly like a normal epilogue except that it has an extra input
> -   register (EH_RETURN_STACKADJ_RTX) which contains the stack adjustment
> -   that must be applied after the frame has been destroyed.  An extra label
> -   is inserted before the epilogue which initializes this register to zero,
> -   and this is the entry point for a normal return.
> -
> -   An actual EH return updates the return address, initializes the stack
> -   adjustment and jumps directly into the epilogue (bypassing the zeroing
> -   of the adjustment).  Since the return address is typically saved on the
> -   stack when a function makes a call, the saved LR must be updated outside
> -   the epilogue.
> -
> -   This poses problems as the store is generated well before the epilogue,
> -   so the offset of LR is not known yet.  Also optimizations will remove the
> -   store as it appears dead, even after the epilogue is generated (as the
> -   base or offset for loading LR is different in many cases).
> -
> -   To avoid these problems this implementation forces the frame pointer
> -   in eh_return functions so that the location of LR is fixed and known early.
> -   It also marks the store volatile, so no optimization is permitted to
> -   remove the store.  */
> -rtx
> -aarch64_eh_return_handler_rtx (void)
> -{
> -  rtx tmp = gen_frame_mem (Pmode,
> -    plus_constant (Pmode, hard_frame_pointer_rtx, UNITS_PER_WORD));
> -
> -  /* Mark the store volatile, so no optimization is permitted to remove it.  */
> -  MEM_VOLATILE_P (tmp) = true;
> -  return tmp;
> -}
> -
>  /* Output code to add DELTA to the first argument, and then jump
>     to FUNCTION.  Used for C++ multiple inheritance.  */
>  static void
> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
> index 2f0777a37ac..58f7eeb565f 100644
> --- a/gcc/config/aarch64/aarch64.h
> +++ b/gcc/config/aarch64/aarch64.h
> @@ -583,9 +583,12 @@ enum class aarch64_feature : unsigned char {
>  /* Output assembly strings after .cfi_startproc is emitted.  */
>  #define ASM_POST_CFI_STARTPROC  aarch64_post_cfi_startproc
>  
> -/* For EH returns X4 contains the stack adjustment.  */
> -#define EH_RETURN_STACKADJ_RTX	gen_rtx_REG (Pmode, R4_REGNUM)
> -#define EH_RETURN_HANDLER_RTX  aarch64_eh_return_handler_rtx ()
> +/* For EH returns X4 is a flag that is set in the EH return
> +   code paths and then X5 and X6 contain the stack adjustment
> +   and return address respectively.  */
> +#define EH_RETURN_TAKEN_RTX	gen_rtx_REG (Pmode, R4_REGNUM)
> +#define EH_RETURN_STACKADJ_RTX	gen_rtx_REG (Pmode, R5_REGNUM)
> +#define EH_RETURN_HANDLER_RTX	gen_rtx_REG (Pmode, R6_REGNUM)
>  
>  #undef TARGET_COMPUTE_FRAME_LAYOUT
>  #define TARGET_COMPUTE_FRAME_LAYOUT aarch64_layout_frame
> diff --git a/gcc/df-scan.cc b/gcc/df-scan.cc
> index 9515740728c..934c9ca2d81 100644
> --- a/gcc/df-scan.cc
> +++ b/gcc/df-scan.cc
> @@ -3720,6 +3720,16 @@ df_get_exit_block_use_set (bitmap exit_block_uses)
>      }
>  #endif
>  
> +#ifdef EH_RETURN_TAKEN_RTX
> +  if ((!targetm.have_epilogue () || ! epilogue_completed)
> +      && crtl->calls_eh_return)
> +    {
> +      rtx tmp = EH_RETURN_TAKEN_RTX;
> +      if (tmp && REG_P (tmp))
> +	df_mark_reg (tmp, exit_block_uses);
> +    }
> +#endif
> +
>    if ((!targetm.have_epilogue () || ! epilogue_completed)
>        && crtl->calls_eh_return)
>      {
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index f7ac806ff15..21bfe160a8b 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -3506,6 +3506,18 @@ If you want to support call frame exception handling, you must
>  define either this macro or the @code{eh_return} instruction pattern.
>  @end defmac
>  
> +@defmac EH_RETURN_TAKEN_RTX
> +A C expression whose value is RTL representing a location in which
> +to store if the EH return path was taken instead of a normal return.
> +This macro allows conditionally executing different code in the
> +epilogue for the EH and normal return cases.
> +
> +When this macro is defined, the macros @code{EH_RETURN_STACKADJ_RTX}
> +and @code{EH_RETURN_HANDLER_RTX} are only meaningful in the epilogue
> +when 1 is stored to the specified location. The value 0 means normal
> +return.
> +@end defmac
> +
>  @defmac RETURN_ADDR_OFFSET
>  If defined, an integer-valued C expression for which rtl will be generated
>  to add it to the exception handler address before it is searched in the
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index 141027e0bb4..ee9dc5c5fc3 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -2742,6 +2742,18 @@ If you want to support call frame exception handling, you must
>  define either this macro or the @code{eh_return} instruction pattern.
>  @end defmac
>  
> +@defmac EH_RETURN_TAKEN_RTX
> +A C expression whose value is RTL representing a location in which
> +to store if the EH return path was taken instead of a normal return.
> +This macro allows conditionally executing different code in the
> +epilogue for the EH and normal return cases.
> +
> +When this macro is defined, the macros @code{EH_RETURN_STACKADJ_RTX}
> +and @code{EH_RETURN_HANDLER_RTX} are only meaningful in the epilogue
> +when 1 is stored to the specified location. The value 0 means normal
> +return.
> +@end defmac
> +
>  @defmac RETURN_ADDR_OFFSET
>  If defined, an integer-valued C expression for which rtl will be generated
>  to add it to the exception handler address before it is searched in the
> diff --git a/gcc/except.cc b/gcc/except.cc
> index e728aa43b6a..508f8bb3e26 100644
> --- a/gcc/except.cc
> +++ b/gcc/except.cc
> @@ -2281,6 +2281,10 @@ expand_eh_return (void)
>    emit_move_insn (EH_RETURN_STACKADJ_RTX, const0_rtx);
>  #endif
>  
> +#ifdef EH_RETURN_TAKEN_RTX
> +  emit_move_insn (EH_RETURN_TAKEN_RTX, const0_rtx);
> +#endif
> +
>    around_label = gen_label_rtx ();
>    emit_jump (around_label);
>  
> @@ -2291,6 +2295,10 @@ expand_eh_return (void)
>    emit_move_insn (EH_RETURN_STACKADJ_RTX, crtl->eh.ehr_stackadj);
>  #endif
>  
> +#ifdef EH_RETURN_TAKEN_RTX
> +  emit_move_insn (EH_RETURN_TAKEN_RTX, const1_rtx);
> +#endif
> +
>    if (targetm.have_eh_return ())
>      emit_insn (targetm.gen_eh_return (crtl->eh.ehr_handler));
>    else
> @@ -2301,7 +2309,19 @@ expand_eh_return (void)
>  	error ("%<__builtin_eh_return%> not supported on this target");
>      }
>  
> +#ifdef EH_RETURN_TAKEN_RTX
> +  rtx_code_label *eh_done_label = gen_label_rtx ();
> +  emit_jump (eh_done_label);
> +#endif
> +
>    emit_label (around_label);
> +
> +#ifdef EH_RETURN_TAKEN_RTX
> +  for (rtx tmp : { EH_RETURN_STACKADJ_RTX, EH_RETURN_HANDLER_RTX })
> +    if (tmp && REG_P (tmp))
> +      emit_clobber (tmp);
> +  emit_label (eh_done_label);
> +#endif
>  }
>  
>  /* Convert a ptr_mode address ADDR_TREE to a Pmode address controlled by
> diff --git a/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c b/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c
> index 232ba67ade0..114a9dacb3f 100644
> --- a/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c
> +++ b/gcc/testsuite/gcc.target/aarch64/return_address_sign_1.c
> @@ -37,16 +37,5 @@ func3 (int a, int b, int c)
>    /* autiasp */
>  }
>  
> -/* eh_return.  */
> -void __attribute__ ((target ("arch=armv8.3-a")))
> -func4 (long offset, void *handler, int *ptr, int imm1, int imm2)
> -{
> -  /* no paciasp */
> -  *ptr = imm1 + foo (imm1) + imm2;
> -  __builtin_eh_return (offset, handler);
> -  /* no autiasp */
> -  return;
> -}
> -
> -/* { dg-final { scan-assembler-times "autiasp" 3 } } */
>  /* { dg-final { scan-assembler-times "paciasp" 3 } } */
> +/* { dg-final { scan-assembler-times "autiasp" 3 } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/return_address_sign_2.c b/gcc/testsuite/gcc.target/aarch64/return_address_sign_2.c
> index a4bc5b45333..d93492c3c43 100644
> --- a/gcc/testsuite/gcc.target/aarch64/return_address_sign_2.c
> +++ b/gcc/testsuite/gcc.target/aarch64/return_address_sign_2.c
> @@ -14,5 +14,18 @@ func1 (int a, int b, int c)
>    /* retaa */
>  }
>  
> -/* { dg-final { scan-assembler-times "paciasp" 1 } } */
> -/* { dg-final { scan-assembler-times "retaa" 1 } } */
> +/* eh_return.  */
> +void __attribute__ ((target ("arch=armv8.3-a")))
> +func4 (long offset, void *handler, int *ptr, int imm1, int imm2)
> +{
> +  /* paciasp */
> +  *ptr = imm1 + foo (imm1) + imm2;
> +  if (handler)
> +    /* br */
> +    __builtin_eh_return (offset, handler);
> +  /* retaa */
> +  return;
> +}
> +
> +/* { dg-final { scan-assembler-times "paciasp" 2 } } */
> +/* { dg-final { scan-assembler-times "retaa" 2 } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_1.c b/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_1.c
> index 43e32ab6cb7..697fa30dc5a 100644
> --- a/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_1.c
> +++ b/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_1.c
> @@ -37,16 +37,5 @@ func3 (int a, int b, int c)
>    /* autibsp */
>  }
>  
> -/* eh_return.  */
> -void __attribute__ ((target ("arch=armv8.3-a")))
> -func4 (long offset, void *handler, int *ptr, int imm1, int imm2)
> -{
> -  /* no pacibsp */
> -  *ptr = imm1 + foo (imm1) + imm2;
> -  __builtin_eh_return (offset, handler);
> -  /* no autibsp */
> -  return;
> -}
> -
>  /* { dg-final { scan-assembler-times "pacibsp" 3 } } */
>  /* { dg-final { scan-assembler-times "autibsp" 3 } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_2.c b/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_2.c
> index 9ed64ce0591..452240b731e 100644
> --- a/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_2.c
> +++ b/gcc/testsuite/gcc.target/aarch64/return_address_sign_b_2.c
> @@ -14,5 +14,18 @@ func1 (int a, int b, int c)
>    /* retab */
>  }
>  
> -/* { dg-final { scan-assembler-times "pacibsp" 1 } } */
> -/* { dg-final { scan-assembler-times "retab" 1 } } */
> +/* eh_return.  */
> +void __attribute__ ((target ("arch=armv8.3-a")))
> +func4 (long offset, void *handler, int *ptr, int imm1, int imm2)
> +{
> +  /* pacibsp */
> +  *ptr = imm1 + foo (imm1) + imm2;
> +  if (handler)
> +    /* br */
> +    __builtin_eh_return (offset, handler);
> +  /* retab */
> +  return;
> +}
> +
> +/* { dg-final { scan-assembler-times "pacibsp" 2 } } */
> +/* { dg-final { scan-assembler-times "retab" 2 } } */

[PATCH v2 2/7] aarch64: Do not force a stack frame for EH returns

[Szabolcs Nagy]

EH returns no longer rely on clobbering the return address on the stack
so forcing a stack frame is not necessary.

This does not actually change the code gen for the unwinder since there
are calls before the EH return.

gcc/ChangeLog:

	* config/aarch64/aarch64.cc (aarch64_needs_frame_chain): Do not
	force frame chain for eh_return.
---
unchanged compared to v1
already approved at:
https://gcc.gnu.org/pipermail/gcc-patches/2023-September/629346.html
---
 gcc/config/aarch64/aarch64.cc | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 5cdb33dd3dc..88594bed8ce 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -8492,8 +8492,7 @@ aarch64_output_probe_sve_stack_clash (rtx base, rtx adjustment,
 static bool
 aarch64_needs_frame_chain (void)
 {
-  /* Force a frame chain for EH returns so the return address is at FP+8.  */
-  if (frame_pointer_needed || crtl->calls_eh_return)
+  if (frame_pointer_needed)
     return true;
 
   /* A leaf function cannot have calls or write LR.  */
-- 
2.25.1

Re: [PATCH v2 2/7] aarch64: Do not force a stack frame for EH returns

[Richard Sandiford]

Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> EH returns no longer rely on clobbering the return address on the stack
> so forcing a stack frame is not necessary.
>
> This does not actually change the code gen for the unwinder since there
> are calls before the EH return.
>
> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64.cc (aarch64_needs_frame_chain): Do not
> 	force frame chain for eh_return.

OK, thanks.

Richard

> ---
> unchanged compared to v1
> already approved at:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-September/629346.html
> ---
>  gcc/config/aarch64/aarch64.cc | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 5cdb33dd3dc..88594bed8ce 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -8492,8 +8492,7 @@ aarch64_output_probe_sve_stack_clash (rtx base, rtx adjustment,
>  static bool
>  aarch64_needs_frame_chain (void)
>  {
> -  /* Force a frame chain for EH returns so the return address is at FP+8.  */
> -  if (frame_pointer_needed || crtl->calls_eh_return)
> +  if (frame_pointer_needed)
>      return true;
>  
>    /* A leaf function cannot have calls or write LR.  */

[PATCH v2 3/7] aarch64: Add eh_return compile tests

[Szabolcs Nagy]

gcc/testsuite/ChangeLog:

	* gcc.target/aarch64/eh_return-2.c: New test.
	* gcc.target/aarch64/eh_return-3.c: New test.

---
v2: check-function-bodies in eh_return-3.c
(this is not very robust, but easier to read)
---
 .../gcc.target/aarch64/eh_return-2.c          |  9 ++++++
 .../gcc.target/aarch64/eh_return-3.c          | 30 +++++++++++++++++++
 2 files changed, 39 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/eh_return-2.c
 create mode 100644 gcc/testsuite/gcc.target/aarch64/eh_return-3.c

diff --git a/gcc/testsuite/gcc.target/aarch64/eh_return-2.c b/gcc/testsuite/gcc.target/aarch64/eh_return-2.c
new file mode 100644
index 00000000000..4a9d124e891
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/eh_return-2.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-final { scan-assembler "add\tsp, sp, x5" } } */
+/* { dg-final { scan-assembler "br\tx6" } } */
+
+void
+foo (unsigned long off, void *handler)
+{
+  __builtin_eh_return (off, handler);
+}
diff --git a/gcc/testsuite/gcc.target/aarch64/eh_return-3.c b/gcc/testsuite/gcc.target/aarch64/eh_return-3.c
new file mode 100644
index 00000000000..bfbe92af427
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/eh_return-3.c
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mbranch-protection=pac-ret+leaf" } */
+/* { dg-final { check-function-bodies "**" "" "" } } */
+
+/*
+**foo:
+**	hint	25 // paciasp
+**	stp	x0, x1, .*
+**	stp	x2, x3, .*
+**	cbz	w2, .*
+**	mov	x4, 0
+**	ldp	x2, x3, .*
+**	ldp	x0, x1, .*
+**	cbz	x4, .*
+**	add	sp, sp, x5
+**	br	x6
+**	hint	29 // autiasp
+**	ret
+**	mov	x5, x0
+**	mov	x6, x1
+**	mov	x4, 1
+**	b	.*
+*/
+void
+foo (unsigned long off, void *handler, int c)
+{
+  if (c)
+    return;
+  __builtin_eh_return (off, handler);
+}
-- 
2.25.1

Re: [PATCH v2 3/7] aarch64: Add eh_return compile tests

[Richard Sandiford]

Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/aarch64/eh_return-2.c: New test.
> 	* gcc.target/aarch64/eh_return-3.c: New test.
>
> ---
> v2: check-function-bodies in eh_return-3.c
> (this is not very robust, but easier to read)
> ---
>  .../gcc.target/aarch64/eh_return-2.c          |  9 ++++++
>  .../gcc.target/aarch64/eh_return-3.c          | 30 +++++++++++++++++++
>  2 files changed, 39 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/eh_return-2.c
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/eh_return-3.c
>
> diff --git a/gcc/testsuite/gcc.target/aarch64/eh_return-2.c b/gcc/testsuite/gcc.target/aarch64/eh_return-2.c
> new file mode 100644
> index 00000000000..4a9d124e891
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/eh_return-2.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-final { scan-assembler "add\tsp, sp, x5" } } */
> +/* { dg-final { scan-assembler "br\tx6" } } */
> +
> +void
> +foo (unsigned long off, void *handler)
> +{
> +  __builtin_eh_return (off, handler);
> +}
> diff --git a/gcc/testsuite/gcc.target/aarch64/eh_return-3.c b/gcc/testsuite/gcc.target/aarch64/eh_return-3.c
> new file mode 100644
> index 00000000000..bfbe92af427
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/eh_return-3.c
> @@ -0,0 +1,30 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mbranch-protection=pac-ret+leaf" } */

Probably best to add -fno-schedule-insns -fno-schedule-insns2, so that the
instructions in the check-function-bodies are in a more predictable order.

> +/* { dg-final { check-function-bodies "**" "" "" } } */
> +
> +/*
> +**foo:
> +**	hint	25 // paciasp
> +**	stp	x0, x1, .*
> +**	stp	x2, x3, .*
> +**	cbz	w2, .*
> +**	mov	x4, 0
> +**	ldp	x2, x3, .*
> +**	ldp	x0, x1, .*
> +**	cbz	x4, .*
> +**	add	sp, sp, x5
> +**	br	x6
> +**	hint	29 // autiasp
> +**	ret
> +**	mov	x5, x0
> +**	mov	x6, x1
> +**	mov	x4, 1
> +**	b	.*
> +*/

What's the significance of x3 here?  It looks from the function definition
like it should be undefined.  And what are the stps and ldps doing?

If those aren't an important part of the test, it might be better
to stub them out with "...", e.g.:

/*
**foo:
**	hint	25 // paciasp
**	...
**	cbz	w2, .*
**	mov	x4, 0
**	...
**	cbz	x4, .*
**	add	sp, sp, x5
**	br	x6
**	hint	29 // autiasp
**	ret
**	mov	x5, x0
**	mov	x6, x1
**	mov	x4, 1
**	b	.*
*/

LGTM otherwise.

Thanks,
Richard

> +void
> +foo (unsigned long off, void *handler, int c)
> +{
> +  if (c)
> +    return;
> +  __builtin_eh_return (off, handler);
> +}

Re: [PATCH v2 3/7] aarch64: Add eh_return compile tests

[Szabolcs Nagy]

The 11/26/2023 14:37, Richard Sandiford wrote:
> Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> > +++ b/gcc/testsuite/gcc.target/aarch64/eh_return-3.c
> > @@ -0,0 +1,30 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -mbranch-protection=pac-ret+leaf" } */
> 
> Probably best to add -fno-schedule-insns -fno-schedule-insns2, so that the
> instructions in the check-function-bodies are in a more predictable order.
> 
> > +/* { dg-final { check-function-bodies "**" "" "" } } */
> > +
> > +/*
> > +**foo:
> > +**	hint	25 // paciasp
> > +**	stp	x0, x1, .*
> > +**	stp	x2, x3, .*
> > +**	cbz	w2, .*
> > +**	mov	x4, 0
> > +**	ldp	x2, x3, .*
> > +**	ldp	x0, x1, .*
> > +**	cbz	x4, .*
> > +**	add	sp, sp, x5
> > +**	br	x6
> > +**	hint	29 // autiasp
> > +**	ret
> > +**	mov	x5, x0
> > +**	mov	x6, x1
> > +**	mov	x4, 1
> > +**	b	.*
> > +*/
> 
> What's the significance of x3 here?  It looks from the function definition
> like it should be undefined.  And what are the stps and ldps doing?

x0,..,x3 are preserved registers for eh (EH_RETURN_DATA_REGNO).

they are saved in the prologue and restored in the epilogue so
they can pass arguments to eh, which i think is relevant to an
eh_return test, although if the compiler knows they are not
clobbered then it could eliminate the save/restore.

> If those aren't an important part of the test, it might be better
> to stub them out with "...", e.g.:

i can do that.

> /*
> **foo:
> **	hint	25 // paciasp
> **	...
> **	cbz	w2, .*
> **	mov	x4, 0
> **	...
> **	cbz	x4, .*
> **	add	sp, sp, x5
> **	br	x6
> **	hint	29 // autiasp
> **	ret
> **	mov	x5, x0
> **	mov	x6, x1
> **	mov	x4, 1
> **	b	.*
> */
> 
> LGTM otherwise.

thanks.

Re: [PATCH v2 3/7] aarch64: Add eh_return compile tests

[Szabolcs Nagy]

The 11/26/2023 14:37, Richard Sandiford wrote:
> Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> > +++ b/gcc/testsuite/gcc.target/aarch64/eh_return-3.c
> > @@ -0,0 +1,30 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -mbranch-protection=pac-ret+leaf" } */
> 
> Probably best to add -fno-schedule-insns -fno-schedule-insns2, so that the
> instructions in the check-function-bodies are in a more predictable order.
> 
> > +/* { dg-final { check-function-bodies "**" "" "" } } */
> > +
> > +/*
> > +**foo:
> > +**	hint	25 // paciasp
> > +**	stp	x0, x1, .*
> > +**	stp	x2, x3, .*
> > +**	cbz	w2, .*
> > +**	mov	x4, 0
> > +**	ldp	x2, x3, .*
> > +**	ldp	x0, x1, .*
> > +**	cbz	x4, .*
> > +**	add	sp, sp, x5
> > +**	br	x6
> > +**	hint	29 // autiasp
> > +**	ret
> > +**	mov	x5, x0
> > +**	mov	x6, x1
> > +**	mov	x4, 1
> > +**	b	.*
> > +*/
> 
> What's the significance of x3 here?  It looks from the function definition
> like it should be undefined.  And what are the stps and ldps doing?
> 
> If those aren't an important part of the test, it might be better
> to stub them out with "...", e.g.:
> 
> /*
> **foo:
> **	hint	25 // paciasp
> **	...
> **	cbz	w2, .*
> **	mov	x4, 0
> **	...
> **	cbz	x4, .*
> **	add	sp, sp, x5
> **	br	x6
> **	hint	29 // autiasp
> **	ret
> **	mov	x5, x0
> **	mov	x6, x1
> **	mov	x4, 1
> **	b	.*
> */
> 
> LGTM otherwise.

committed as

From cad7e1e3e0dea1922f89290bbbc27b4c44f53bf5 Mon Sep 17 00:00:00 2001
From: Szabolcs Nagy <szabolcs.nagy@arm.com>
Date: Fri, 2 Jun 2023 14:17:02 +0100
Subject: [PATCH] aarch64: Add eh_return compile tests

gcc/testsuite/ChangeLog:

	* gcc.target/aarch64/eh_return-2.c: New test.
	* gcc.target/aarch64/eh_return-3.c: New test.
---
 .../gcc.target/aarch64/eh_return-2.c          |  9 ++++++
 .../gcc.target/aarch64/eh_return-3.c          | 28 +++++++++++++++++++
 2 files changed, 37 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/eh_return-2.c
 create mode 100644 gcc/testsuite/gcc.target/aarch64/eh_return-3.c

diff --git a/gcc/testsuite/gcc.target/aarch64/eh_return-2.c b/gcc/testsuite/gcc.target/aarch64/eh_return-2.c
new file mode 100644
index 00000000000..4a9d124e891
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/eh_return-2.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-final { scan-assembler "add\tsp, sp, x5" } } */
+/* { dg-final { scan-assembler "br\tx6" } } */
+
+void
+foo (unsigned long off, void *handler)
+{
+  __builtin_eh_return (off, handler);
+}
diff --git a/gcc/testsuite/gcc.target/aarch64/eh_return-3.c b/gcc/testsuite/gcc.target/aarch64/eh_return-3.c
new file mode 100644
index 00000000000..a17baa86501
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/eh_return-3.c
@@ -0,0 +1,28 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mbranch-protection=pac-ret+leaf -fno-schedule-insns -fno-schedule-insns2" } */
+/* { dg-final { check-function-bodies "**" "" "" } } */
+
+/*
+**foo:
+**	hint	25 // paciasp
+**	...
+**	cbz	w2, .*
+**	mov	x4, 0
+**	...
+**	cbz	x4, .*
+**	add	sp, sp, x5
+**	br	x6
+**	hint	29 // autiasp
+**	ret
+**	mov	x5, x0
+**	mov	x4, 1
+**	mov	x6, x1
+**	b	.*
+*/
+void
+foo (unsigned long off, void *handler, int c)
+{
+  if (c)
+    return;
+  __builtin_eh_return (off, handler);
+}
-- 
2.25.1

Re: [PATCH v2 3/7] aarch64: Add eh_return compile tests

[Andrew Pinski]

On Fri, Nov 3, 2023 at 8:37 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/aarch64/eh_return-2.c: New test.
>         * gcc.target/aarch64/eh_return-3.c: New test.


gcc.target/aarch64/eh_return-3.c fails when running the testsuite with
`-march=armv9-a+sve` . I think it is a good idea to try to keep the
testsuite clean when running with different -march=/-mcpu= options
even. I know there are many failures due to -march=/-mcpu option right
now but this is a new testcase and all.

Thanks,
Andrew

>
> ---
> v2: check-function-bodies in eh_return-3.c
> (this is not very robust, but easier to read)
> ---
>  .../gcc.target/aarch64/eh_return-2.c          |  9 ++++++
>  .../gcc.target/aarch64/eh_return-3.c          | 30 +++++++++++++++++++
>  2 files changed, 39 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/eh_return-2.c
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/eh_return-3.c
>
> diff --git a/gcc/testsuite/gcc.target/aarch64/eh_return-2.c b/gcc/testsuite/gcc.target/aarch64/eh_return-2.c
> new file mode 100644
> index 00000000000..4a9d124e891
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/eh_return-2.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-final { scan-assembler "add\tsp, sp, x5" } } */
> +/* { dg-final { scan-assembler "br\tx6" } } */
> +
> +void
> +foo (unsigned long off, void *handler)
> +{
> +  __builtin_eh_return (off, handler);
> +}
> diff --git a/gcc/testsuite/gcc.target/aarch64/eh_return-3.c b/gcc/testsuite/gcc.target/aarch64/eh_return-3.c
> new file mode 100644
> index 00000000000..bfbe92af427
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/eh_return-3.c
> @@ -0,0 +1,30 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mbranch-protection=pac-ret+leaf" } */
> +/* { dg-final { check-function-bodies "**" "" "" } } */
> +
> +/*
> +**foo:
> +**     hint    25 // paciasp
> +**     stp     x0, x1, .*
> +**     stp     x2, x3, .*
> +**     cbz     w2, .*
> +**     mov     x4, 0
> +**     ldp     x2, x3, .*
> +**     ldp     x0, x1, .*
> +**     cbz     x4, .*
> +**     add     sp, sp, x5
> +**     br      x6
> +**     hint    29 // autiasp
> +**     ret
> +**     mov     x5, x0
> +**     mov     x6, x1
> +**     mov     x4, 1
> +**     b       .*
> +*/
> +void
> +foo (unsigned long off, void *handler, int c)
> +{
> +  if (c)
> +    return;
> +  __builtin_eh_return (off, handler);
> +}
> --
> 2.25.1
>

[PATCH v2 4/7] aarch64: Disable branch-protection for pcs tests

[Szabolcs Nagy]

The tests manipulate the return address in abitest-2.h and thus not
compatible with -mbranch-protection=pac-ret+leaf or
-mbranch-protection=gcs.

gcc/testsuite/ChangeLog:

	* gcc.target/aarch64/aapcs64/func-ret-1.c: Disable branch-protection.
	* gcc.target/aarch64/aapcs64/func-ret-2.c: Likewise.
	* gcc.target/aarch64/aapcs64/func-ret-3.c: Likewise.
	* gcc.target/aarch64/aapcs64/func-ret-4.c: Likewise.
	* gcc.target/aarch64/aapcs64/func-ret-64x1_1.c: Likewise.
---
unchanged compared to v1
already approved at
https://gcc.gnu.org/pipermail/gcc-patches/2023-September/629353.html
---
 gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-1.c      | 1 +
 gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-2.c      | 1 +
 gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-3.c      | 1 +
 gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-4.c      | 1 +
 gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-64x1_1.c | 1 +
 5 files changed, 5 insertions(+)

diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-1.c b/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-1.c
index 5405e1e4920..7bd7757efe6 100644
--- a/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-1.c
+++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-1.c
@@ -4,6 +4,7 @@
    AAPCS64 \S 4.1.  */
 
 /* { dg-do run { target aarch64*-*-* } } */
+/* { dg-additional-options "-mbranch-protection=none" } */
 /* { dg-additional-sources "abitest.S" } */
 
 #ifndef IN_FRAMEWORK
diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-2.c b/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-2.c
index 6b171c46fbb..85a822ace4a 100644
--- a/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-2.c
+++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-2.c
@@ -4,6 +4,7 @@
    Homogeneous floating-point aggregate types are covered in func-ret-3.c.  */
 
 /* { dg-do run { target aarch64*-*-* } } */
+/* { dg-additional-options "-mbranch-protection=none" } */
 /* { dg-additional-sources "abitest.S" } */
 
 #ifndef IN_FRAMEWORK
diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-3.c b/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-3.c
index ad312b675b9..1d35ebf14b4 100644
--- a/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-3.c
+++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-3.c
@@ -4,6 +4,7 @@
    in AAPCS64 \S 4.3.5.  */
 
 /* { dg-do run { target aarch64-*-* } } */
+/* { dg-additional-options "-mbranch-protection=none" } */
 /* { dg-additional-sources "abitest.S" } */
 /* { dg-require-effective-target aarch64_big_endian } */
 
diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-4.c b/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-4.c
index af05fbe9fdf..15e1408c62d 100644
--- a/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-4.c
+++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-4.c
@@ -5,6 +5,7 @@
    are treated as general composite types.  */
 
 /* { dg-do run { target aarch64*-*-* } } */
+/* { dg-additional-options "-mbranch-protection=none" } */
 /* { dg-additional-sources "abitest.S" } */
 /* { dg-require-effective-target aarch64_big_endian } */
 
diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-64x1_1.c b/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-64x1_1.c
index 05957e2dcae..fe7bbb6a835 100644
--- a/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-64x1_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/func-ret-64x1_1.c
@@ -3,6 +3,7 @@
   Test 64-bit singleton vector types which should be in FP/SIMD registers.  */
 
 /* { dg-do run { target aarch64*-*-* } } */
+/* { dg-additional-options "-mbranch-protection=none" } */
 /* { dg-additional-sources "abitest.S" } */
 
 #ifndef IN_FRAMEWORK
-- 
2.25.1

[PATCH v2 5/7] aarch64,arm: Remove accepted_branch_protection_string

[Szabolcs Nagy]

On aarch64 this caused ICE with pragma push_options since

  commit ae54c1b09963779c5c3914782324ff48af32e2f1
  Author:     Wilco Dijkstra <wilco.dijkstra@arm.com>
  CommitDate: 2022-06-01 18:13:57 +0100

  AArch64: Cleanup option processing code

The failure is at pop_options:

internal compiler error: ‘global_options’ are modified in local context

On arm the variable was unused.

gcc/ChangeLog:

	* config/aarch64/aarch64.cc (aarch64_override_options_after_change_1):
	Do not override branch_protection options.
	(aarch64_override_options): Remove accepted_branch_protection_string.
	* config/arm/aarch-common.cc (BRANCH_PROTECT_STR_MAX): Remove.
	(aarch_parse_branch_protection): Remove
	accepted_branch_protection_string.
	* config/arm/arm.cc: Likewise.
---
unchanged from v1
---
 gcc/config/aarch64/aarch64.cc  | 10 +---------
 gcc/config/arm/aarch-common.cc | 16 ----------------
 gcc/config/arm/arm.cc          |  2 --
 3 files changed, 1 insertion(+), 27 deletions(-)

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 88594bed8ce..f8e8fefc8d8 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -323,8 +323,6 @@ bool aarch64_pcrelative_literal_loads;
 /* Global flag for whether frame pointer is enabled.  */
 bool aarch64_use_frame_pointer;
 
-char *accepted_branch_protection_string = NULL;
-
 /* Support for command line parsing of boolean flags in the tuning
    structures.  */
 struct aarch64_flag_desc
@@ -18101,12 +18099,6 @@ aarch64_adjust_generic_arch_tuning (struct tune_params &current_tune)
 static void
 aarch64_override_options_after_change_1 (struct gcc_options *opts)
 {
-  if (accepted_branch_protection_string)
-    {
-      opts->x_aarch64_branch_protection_string
-	= xstrdup (accepted_branch_protection_string);
-    }
-
   /* PR 70044: We have to be careful about being called multiple times for the
      same function.  This means all changes should be repeatable.  */
 
@@ -18715,7 +18707,7 @@ aarch64_override_options (void)
   /* Return address signing is currently not supported for ILP32 targets.  For
      LP64 targets use the configured option in the absence of a command-line
      option for -mbranch-protection.  */
-  if (!TARGET_ILP32 && accepted_branch_protection_string == NULL)
+  if (!TARGET_ILP32 && aarch64_branch_protection_string == NULL)
     {
 #ifdef TARGET_ENABLE_PAC_RET
       aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
diff --git a/gcc/config/arm/aarch-common.cc b/gcc/config/arm/aarch-common.cc
index 5b96ff4c2e8..cbc7f68a8bf 100644
--- a/gcc/config/arm/aarch-common.cc
+++ b/gcc/config/arm/aarch-common.cc
@@ -659,9 +659,6 @@ arm_md_asm_adjust (vec<rtx> &outputs, vec<rtx> & /*inputs*/,
   return saw_asm_flag ? seq : NULL;
 }
 
-#define BRANCH_PROTECT_STR_MAX 255
-extern char *accepted_branch_protection_string;
-
 static enum aarch_parse_opt_result
 aarch_handle_no_branch_protection (char* str, char* rest)
 {
@@ -812,19 +809,6 @@ aarch_parse_branch_protection (const char *const_str, char** last_str)
       else
 	*last_str = NULL;
     }
-
-  if (res == AARCH_PARSE_OK)
-    {
-      /* If needed, alloc the accepted string then copy in const_str.
-	Used by override_option_after_change_1.  */
-      if (!accepted_branch_protection_string)
-	accepted_branch_protection_string
-	  = (char *) xmalloc (BRANCH_PROTECT_STR_MAX + 1);
-      strncpy (accepted_branch_protection_string, const_str,
-	       BRANCH_PROTECT_STR_MAX + 1);
-      /* Forcibly null-terminate.  */
-      accepted_branch_protection_string[BRANCH_PROTECT_STR_MAX] = '\0';
-    }
   return res;
 }
 
diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index 6e933c80183..f49312cace0 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -2424,8 +2424,6 @@ const struct tune_params arm_fa726te_tune =
   tune_params::SCHED_AUTOPREF_OFF
 };
 
-char *accepted_branch_protection_string = NULL;
-
 /* Auto-generated CPU, FPU and architecture tables.  */
 #include "arm-cpu-data.h"
 
-- 
2.25.1

Re: [PATCH v2 5/7] aarch64,arm: Remove accepted_branch_protection_string

[Richard Sandiford]

Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> On aarch64 this caused ICE with pragma push_options since
>
>   commit ae54c1b09963779c5c3914782324ff48af32e2f1
>   Author:     Wilco Dijkstra <wilco.dijkstra@arm.com>
>   CommitDate: 2022-06-01 18:13:57 +0100
>
>   AArch64: Cleanup option processing code
>
> The failure is at pop_options:
>
> internal compiler error: ‘global_options’ are modified in local context
>
> On arm the variable was unused.
>
> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64.cc (aarch64_override_options_after_change_1):
> 	Do not override branch_protection options.
> 	(aarch64_override_options): Remove accepted_branch_protection_string.
> 	* config/arm/aarch-common.cc (BRANCH_PROTECT_STR_MAX): Remove.
> 	(aarch_parse_branch_protection): Remove
> 	accepted_branch_protection_string.
> 	* config/arm/arm.cc: Likewise.

OK.  Not sure I fully understand the subtlety of how things worked
before Wilco's patch, but I agree it looks like all the results of
the parsing are correctly captured by Target options or TargetVariables
in aarch64.opt, and so no special handling is needed.

Thanks,
Richard

> ---
> unchanged from v1
> ---
>  gcc/config/aarch64/aarch64.cc  | 10 +---------
>  gcc/config/arm/aarch-common.cc | 16 ----------------
>  gcc/config/arm/arm.cc          |  2 --
>  3 files changed, 1 insertion(+), 27 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 88594bed8ce..f8e8fefc8d8 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -323,8 +323,6 @@ bool aarch64_pcrelative_literal_loads;
>  /* Global flag for whether frame pointer is enabled.  */
>  bool aarch64_use_frame_pointer;
>  
> -char *accepted_branch_protection_string = NULL;
> -
>  /* Support for command line parsing of boolean flags in the tuning
>     structures.  */
>  struct aarch64_flag_desc
> @@ -18101,12 +18099,6 @@ aarch64_adjust_generic_arch_tuning (struct tune_params &current_tune)
>  static void
>  aarch64_override_options_after_change_1 (struct gcc_options *opts)
>  {
> -  if (accepted_branch_protection_string)
> -    {
> -      opts->x_aarch64_branch_protection_string
> -	= xstrdup (accepted_branch_protection_string);
> -    }
> -
>    /* PR 70044: We have to be careful about being called multiple times for the
>       same function.  This means all changes should be repeatable.  */
>  
> @@ -18715,7 +18707,7 @@ aarch64_override_options (void)
>    /* Return address signing is currently not supported for ILP32 targets.  For
>       LP64 targets use the configured option in the absence of a command-line
>       option for -mbranch-protection.  */
> -  if (!TARGET_ILP32 && accepted_branch_protection_string == NULL)
> +  if (!TARGET_ILP32 && aarch64_branch_protection_string == NULL)
>      {
>  #ifdef TARGET_ENABLE_PAC_RET
>        aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
> diff --git a/gcc/config/arm/aarch-common.cc b/gcc/config/arm/aarch-common.cc
> index 5b96ff4c2e8..cbc7f68a8bf 100644
> --- a/gcc/config/arm/aarch-common.cc
> +++ b/gcc/config/arm/aarch-common.cc
> @@ -659,9 +659,6 @@ arm_md_asm_adjust (vec<rtx> &outputs, vec<rtx> & /*inputs*/,
>    return saw_asm_flag ? seq : NULL;
>  }
>  
> -#define BRANCH_PROTECT_STR_MAX 255
> -extern char *accepted_branch_protection_string;
> -
>  static enum aarch_parse_opt_result
>  aarch_handle_no_branch_protection (char* str, char* rest)
>  {
> @@ -812,19 +809,6 @@ aarch_parse_branch_protection (const char *const_str, char** last_str)
>        else
>  	*last_str = NULL;
>      }
> -
> -  if (res == AARCH_PARSE_OK)
> -    {
> -      /* If needed, alloc the accepted string then copy in const_str.
> -	Used by override_option_after_change_1.  */
> -      if (!accepted_branch_protection_string)
> -	accepted_branch_protection_string
> -	  = (char *) xmalloc (BRANCH_PROTECT_STR_MAX + 1);
> -      strncpy (accepted_branch_protection_string, const_str,
> -	       BRANCH_PROTECT_STR_MAX + 1);
> -      /* Forcibly null-terminate.  */
> -      accepted_branch_protection_string[BRANCH_PROTECT_STR_MAX] = '\0';
> -    }
>    return res;
>  }
>  
> diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
> index 6e933c80183..f49312cace0 100644
> --- a/gcc/config/arm/arm.cc
> +++ b/gcc/config/arm/arm.cc
> @@ -2424,8 +2424,6 @@ const struct tune_params arm_fa726te_tune =
>    tune_params::SCHED_AUTOPREF_OFF
>  };
>  
> -char *accepted_branch_protection_string = NULL;
> -
>  /* Auto-generated CPU, FPU and architecture tables.  */
>  #include "arm-cpu-data.h"

[PATCH v2 6/7] aarch64,arm: Fix branch-protection= parsing

[Szabolcs Nagy]

Refactor the parsing to have a single API and fix a few parsing issues:

- Different handling of "bti+none" and "none+bti": these should be
  rejected because "none" can only appear alone.

- Accepted empty strings such as "bti++pac-ret" or "bti+", this bug
  was caused by using strtok_r.

- Memory got leaked (str_root was never freed). And two buffers got
  allocated when one is enough.

The callbacks now have no failure mode, only parsing can fail and
all failures are handled locally.  The "-mbranch-protection=" vs
"target("branch-protection=")" difference in the error message is
handled by a separate argument to aarch_validate_mbranch_protection.

gcc/ChangeLog:

	* config/aarch64/aarch64.cc (aarch64_override_options): Update.
	(aarch64_handle_attr_branch_protection): Update.
	* config/arm/aarch-common-protos.h (aarch_parse_branch_protection):
	Remove.
	(aarch_validate_mbranch_protection): Add new argument.
	* config/arm/aarch-common.cc (aarch_handle_no_branch_protection):
	Update.
	(aarch_handle_standard_branch_protection): Update.
	(aarch_handle_pac_ret_protection): Update.
	(aarch_handle_pac_ret_leaf): Update.
	(aarch_handle_pac_ret_b_key): Update.
	(aarch_handle_bti_protection): Update.
	(aarch_parse_branch_protection): Remove.
	(next_tok): New.
	(aarch_validate_mbranch_protection): Rewrite.
	* config/arm/aarch-common.h (struct aarch_branch_protect_type):
	Add field "alone".
	* config/arm/arm.cc (arm_configure_build_target): Update.

gcc/testsuite/ChangeLog:

	* gcc.target/aarch64/branch-protection-attr.c: Update.
	* gcc.target/aarch64/branch-protection-option.c: Update.
---
v2: merge tests updates into the patch
error message is not changed, see previous discussion:
https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633945.html
---
 gcc/config/aarch64/aarch64.cc                 |  37 +--
 gcc/config/arm/aarch-common-protos.h          |   5 +-
 gcc/config/arm/aarch-common.cc                | 214 ++++++++----------
 gcc/config/arm/aarch-common.h                 |  14 +-
 gcc/config/arm/arm.cc                         |   3 +-
 .../aarch64/branch-protection-attr.c          |   6 +-
 .../aarch64/branch-protection-option.c        |   2 +-
 7 files changed, 113 insertions(+), 168 deletions(-)

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index f8e8fefc8d8..4f7f707b675 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -18642,7 +18642,8 @@ aarch64_override_options (void)
     aarch64_validate_sls_mitigation (aarch64_harden_sls_string);
 
   if (aarch64_branch_protection_string)
-    aarch_validate_mbranch_protection (aarch64_branch_protection_string);
+    aarch_validate_mbranch_protection (aarch64_branch_protection_string,
+				       "-mbranch-protection=");
 
   /* -mcpu=CPU is shorthand for -march=ARCH_FOR_CPU, -mtune=CPU.
      If either of -march or -mtune is given, they override their
@@ -19016,34 +19017,12 @@ aarch64_handle_attr_cpu (const char *str)
 
 /* Handle the argument STR to the branch-protection= attribute.  */
 
- static bool
- aarch64_handle_attr_branch_protection (const char* str)
- {
-  char *err_str = (char *) xmalloc (strlen (str) + 1);
-  enum aarch_parse_opt_result res = aarch_parse_branch_protection (str,
-								   &err_str);
-  bool success = false;
-  switch (res)
-    {
-     case AARCH_PARSE_MISSING_ARG:
-       error ("missing argument to %<target(\"branch-protection=\")%> pragma or"
-	      " attribute");
-       break;
-     case AARCH_PARSE_INVALID_ARG:
-       error ("invalid protection type %qs in %<target(\"branch-protection"
-	      "=\")%> pragma or attribute", err_str);
-       break;
-     case AARCH_PARSE_OK:
-       success = true;
-      /* Fall through.  */
-     case AARCH_PARSE_INVALID_FEATURE:
-       break;
-     default:
-       gcc_unreachable ();
-    }
-  free (err_str);
-  return success;
- }
+static bool
+aarch64_handle_attr_branch_protection (const char* str)
+{
+  return aarch_validate_mbranch_protection (str,
+					    "target(\"branch-protection=\")");
+}
 
 /* Handle the argument STR to the tune= target attribute.  */
 
diff --git a/gcc/config/arm/aarch-common-protos.h b/gcc/config/arm/aarch-common-protos.h
index f8cb6562096..75ffdfbb050 100644
--- a/gcc/config/arm/aarch-common-protos.h
+++ b/gcc/config/arm/aarch-common-protos.h
@@ -159,10 +159,7 @@ rtx_insn *arm_md_asm_adjust (vec<rtx> &outputs, vec<rtx> & /*inputs*/,
 			     vec<rtx> &clobbers, HARD_REG_SET &clobbered_regs,
 			     location_t loc);
 
-/* Parsing routine for branch-protection common to AArch64 and Arm.  */
-enum aarch_parse_opt_result aarch_parse_branch_protection (const char*, char**);
-
 /* Validation routine for branch-protection common to AArch64 and Arm.  */
-bool aarch_validate_mbranch_protection (const char *);
+bool aarch_validate_mbranch_protection (const char *, const char *);
 
 #endif /* GCC_AARCH_COMMON_PROTOS_H */
diff --git a/gcc/config/arm/aarch-common.cc b/gcc/config/arm/aarch-common.cc
index cbc7f68a8bf..159c61b786c 100644
--- a/gcc/config/arm/aarch-common.cc
+++ b/gcc/config/arm/aarch-common.cc
@@ -659,169 +659,143 @@ arm_md_asm_adjust (vec<rtx> &outputs, vec<rtx> & /*inputs*/,
   return saw_asm_flag ? seq : NULL;
 }
 
-static enum aarch_parse_opt_result
-aarch_handle_no_branch_protection (char* str, char* rest)
+static void
+aarch_handle_no_branch_protection (void)
 {
   aarch_ra_sign_scope = AARCH_FUNCTION_NONE;
   aarch_enable_bti = 0;
-  if (rest)
-    {
-      error ("unexpected %<%s%> after %<%s%>", rest, str);
-      return AARCH_PARSE_INVALID_FEATURE;
-    }
-  return AARCH_PARSE_OK;
 }
 
-static enum aarch_parse_opt_result
-aarch_handle_standard_branch_protection (char* str, char* rest)
+static void
+aarch_handle_standard_branch_protection (void)
 {
   aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
   aarch_ra_sign_key = AARCH_KEY_A;
   aarch_enable_bti = 1;
-  if (rest)
-    {
-      error ("unexpected %<%s%> after %<%s%>", rest, str);
-      return AARCH_PARSE_INVALID_FEATURE;
-    }
-  return AARCH_PARSE_OK;
 }
 
-static enum aarch_parse_opt_result
-aarch_handle_pac_ret_protection (char* str ATTRIBUTE_UNUSED,
-				 char* rest ATTRIBUTE_UNUSED)
+static void
+aarch_handle_pac_ret_protection (void)
 {
   aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
   aarch_ra_sign_key = AARCH_KEY_A;
-  return AARCH_PARSE_OK;
 }
 
-static enum aarch_parse_opt_result
-aarch_handle_pac_ret_leaf (char* str ATTRIBUTE_UNUSED,
-			   char* rest ATTRIBUTE_UNUSED)
+static void
+aarch_handle_pac_ret_leaf (void)
 {
   aarch_ra_sign_scope = AARCH_FUNCTION_ALL;
-  return AARCH_PARSE_OK;
 }
 
-static enum aarch_parse_opt_result
-aarch_handle_pac_ret_b_key (char* str ATTRIBUTE_UNUSED,
-			    char* rest ATTRIBUTE_UNUSED)
+static void
+aarch_handle_pac_ret_b_key (void)
 {
   aarch_ra_sign_key = AARCH_KEY_B;
-  return AARCH_PARSE_OK;
 }
 
-static enum aarch_parse_opt_result
-aarch_handle_bti_protection (char* str ATTRIBUTE_UNUSED,
-			     char* rest ATTRIBUTE_UNUSED)
+static void
+aarch_handle_bti_protection (void)
 {
   aarch_enable_bti = 1;
-  return AARCH_PARSE_OK;
 }
 
 static const struct aarch_branch_protect_type aarch_pac_ret_subtypes[] = {
-  { "leaf", aarch_handle_pac_ret_leaf, NULL, 0 },
-  { "b-key", aarch_handle_pac_ret_b_key, NULL, 0 },
-  { NULL, NULL, NULL, 0 }
+  { "leaf", false, aarch_handle_pac_ret_leaf, NULL, 0 },
+  { "b-key", false, aarch_handle_pac_ret_b_key, NULL, 0 },
+  { NULL, false, NULL, NULL, 0 }
 };
 
 static const struct aarch_branch_protect_type aarch_branch_protect_types[] = {
-  { "none", aarch_handle_no_branch_protection, NULL, 0 },
-  { "standard", aarch_handle_standard_branch_protection, NULL, 0 },
-  { "pac-ret", aarch_handle_pac_ret_protection, aarch_pac_ret_subtypes,
+  { "none", true, aarch_handle_no_branch_protection, NULL, 0 },
+  { "standard", true, aarch_handle_standard_branch_protection, NULL, 0 },
+  { "pac-ret", false, aarch_handle_pac_ret_protection, aarch_pac_ret_subtypes,
     ARRAY_SIZE (aarch_pac_ret_subtypes) },
-  { "bti", aarch_handle_bti_protection, NULL, 0 },
-  { NULL, NULL, NULL, 0 }
+  { "bti", false, aarch_handle_bti_protection, NULL, 0 },
+  { NULL, false, NULL, NULL, 0 }
 };
 
-/* Parses CONST_STR for branch protection features specified in
-   aarch64_branch_protect_types, and set any global variables required.  Returns
-   the parsing result and assigns LAST_STR to the last processed token from
-   CONST_STR so that it can be used for error reporting.  */
+/* In-place split *str at delim, return *str and set *str to the tail
+   of the string or NULL if the end is reached.  */
 
-enum aarch_parse_opt_result
-aarch_parse_branch_protection (const char *const_str, char** last_str)
+static char *
+next_tok (char **str, int delim)
 {
-  char *str_root = xstrdup (const_str);
-  char* token_save = NULL;
-  char *str = strtok_r (str_root, "+", &token_save);
-  enum aarch_parse_opt_result res = AARCH_PARSE_OK;
-  if (!str)
-    res = AARCH_PARSE_MISSING_ARG;
-  else
+  char *tok = *str;
+  for (char *p = tok; p && *p != '\0'; p++)
     {
-      char *next_str = strtok_r (NULL, "+", &token_save);
-      /* Reset the branch protection features to their defaults.  */
-      aarch_handle_no_branch_protection (NULL, NULL);
-
-      while (str && res == AARCH_PARSE_OK)
+      if (*p == delim)
 	{
-	  const aarch_branch_protect_type* type = aarch_branch_protect_types;
-	  bool found = false;
-	  /* Search for this type.  */
-	  while (type && type->name && !found && res == AARCH_PARSE_OK)
-	    {
-	      if (strcmp (str, type->name) == 0)
-		{
-		  found = true;
-		  res = type->handler (str, next_str);
-		  str = next_str;
-		  next_str = strtok_r (NULL, "+", &token_save);
-		}
-	      else
-		type++;
-	    }
-	  if (found && res == AARCH_PARSE_OK)
-	    {
-	      bool found_subtype = true;
-	      /* Loop through each token until we find one that isn't a
-		 subtype.  */
-	      while (found_subtype)
-		{
-		  found_subtype = false;
-		  const aarch_branch_protect_type *subtype = type->subtypes;
-		  /* Search for the subtype.  */
-		  while (str && subtype && subtype->name && !found_subtype
-			  && res == AARCH_PARSE_OK)
-		    {
-		      if (strcmp (str, subtype->name) == 0)
-			{
-			  found_subtype = true;
-			  res = subtype->handler (str, next_str);
-			  str = next_str;
-			  next_str = strtok_r (NULL, "+", &token_save);
-			}
-		      else
-			subtype++;
-		    }
-		}
-	    }
-	  else if (!found)
-	    res = AARCH_PARSE_INVALID_ARG;
+	  *p = '\0';
+	  *str = p + 1;
+	  return tok;
 	}
     }
-  /* Copy the last processed token into the argument to pass it back.
-    Used by option and attribute validation to print the offending token.  */
-  if (last_str)
-    {
-      if (str)
-	strcpy (*last_str, str);
-      else
-	*last_str = NULL;
-    }
-  return res;
+  *str = NULL;
+  return tok;
 }
 
+/* Parses CONST_STR for branch protection features specified in
+   aarch64_branch_protect_types, and set any global variables required.
+   Returns true on success.  */
+
 bool
-aarch_validate_mbranch_protection (const char *const_str)
+aarch_validate_mbranch_protection (const char *const_str, const char *opt)
 {
-  char *str = (char *) xmalloc (strlen (const_str));
-  enum aarch_parse_opt_result res =
-    aarch_parse_branch_protection (const_str, &str);
-  if (res == AARCH_PARSE_INVALID_ARG)
-    error ("invalid argument %<%s%> for %<-mbranch-protection=%>", str);
-  else if (res == AARCH_PARSE_MISSING_ARG)
-    error ("missing argument for %<-mbranch-protection=%>");
-  free (str);
-  return res == AARCH_PARSE_OK;
+  char *str_root = xstrdup (const_str);
+  char *next_str = str_root;
+  char *str = next_tok (&next_str, '+');
+  char *alone_str = NULL;
+  bool reject_alone = false;
+  bool res = true;
+
+  /* First entry is "none" and it is used to reset the state.  */
+  aarch_branch_protect_types[0].handler ();
+
+  while (str)
+    {
+      const aarch_branch_protect_type *type = aarch_branch_protect_types;
+      for (; type->name; type++)
+	if (strcmp (str, type->name) == 0)
+	  break;
+      if (type->name == NULL)
+	{
+	  res = false;
+	  error ("invalid argument %<%s%> for %<%s%>", str, opt);
+	  break;
+	}
+
+      if (type->alone && alone_str == NULL)
+	alone_str = str;
+      else
+	reject_alone = true;
+      if (reject_alone && alone_str != NULL)
+	{
+	  res = false;
+	  error ("argument %<%s%> can only appear alone in %<%s%>",
+		 alone_str, opt);
+	  break;
+	}
+
+      type->handler ();
+      str = next_tok (&next_str, '+');
+      if (type->subtypes == NULL)
+	continue;
+
+      /* Loop through tokens until we find one that isn't a subtype.  */
+      while (str)
+	{
+	  const aarch_branch_protect_type *subtype = type->subtypes;
+	  for (; subtype->name; subtype++)
+	    if (strcmp (str, subtype->name) == 0)
+	      break;
+	  if (subtype->name == NULL)
+	    break;
+
+	  subtype->handler ();
+	  str = next_tok (&next_str, '+');
+	}
+    }
+
+  free (str_root);
+  return res;
 }
diff --git a/gcc/config/arm/aarch-common.h b/gcc/config/arm/aarch-common.h
index c6a67f0d05c..f72e21127fc 100644
--- a/gcc/config/arm/aarch-common.h
+++ b/gcc/config/arm/aarch-common.h
@@ -55,16 +55,10 @@ struct aarch_branch_protect_type
   /* The type's name that the user passes to the branch-protection option
      string.  */
   const char* name;
-  /* Function to handle the protection type and set global variables.
-     First argument is the string token corresponding with this type and the
-     second argument is the next token in the option string.
-     Return values:
-     * AARCH_PARSE_OK: Handling was sucessful.
-     * AARCH_INVALID_ARG: The type is invalid in this context and the caller
-     should print an error.
-     * AARCH_INVALID_FEATURE: The type is invalid and the handler prints its
-     own error.  */
-  enum aarch_parse_opt_result (*handler)(char*, char*);
+  /* The type can only appear alone, other types should be rejected.  */
+  int alone;
+  /* Function to handle the protection type and set global variables.  */
+  void (*handler)(void);
   /* A list of types that can follow this type in the option string.  */
   const struct aarch_branch_protect_type* subtypes;
   unsigned int num_subtypes;
diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index f49312cace0..5681073a948 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -3297,7 +3297,8 @@ arm_configure_build_target (struct arm_build_target *target,
 
   if (opts->x_arm_branch_protection_string)
     {
-      aarch_validate_mbranch_protection (opts->x_arm_branch_protection_string);
+      aarch_validate_mbranch_protection (opts->x_arm_branch_protection_string,
+					 "-mbranch-protection=");
 
       if (aarch_ra_sign_key != AARCH_KEY_A)
 	{
diff --git a/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c b/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
index 272000c2747..dae2a758a56 100644
--- a/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
+++ b/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
@@ -4,19 +4,19 @@ void __attribute__ ((target("branch-protection=leaf")))
 foo1 ()
 {
 }
-/* { dg-error {invalid protection type 'leaf' in 'target\("branch-protection="\)' pragma or attribute} "" { target *-*-* } 5 } */
+/* { dg-error {invalid argument 'leaf' for 'target\("branch-protection="\)'} "" { target *-*-* } 5 } */
 /* { dg-error {pragma or attribute 'target\("branch-protection=leaf"\)' is not valid} "" { target *-*-* } 5 } */
 
 void __attribute__ ((target("branch-protection=none+pac-ret")))
 foo2 ()
 {
 }
-/* { dg-error "unexpected 'pac-ret' after 'none'" "" { target *-*-* } 12 } */
+/* { dg-error {argument 'none' can only appear alone in 'target\("branch-protection="\)'} "" { target *-*-* } 12 } */
 /* { dg-error {pragma or attribute 'target\("branch-protection=none\+pac-ret"\)' is not valid} "" { target *-*-* } 12 } */
 
 void __attribute__ ((target("branch-protection=")))
 foo3 ()
 {
 }
-/* { dg-error {missing argument to 'target\("branch-protection="\)' pragma or attribute} "" { target *-*-* } 19 } */
+/* { dg-error {invalid argument '' for 'target\("branch-protection="\)'} "" { target *-*-* } 19 } */
 /* { dg-error {pragma or attribute 'target\("branch-protection="\)' is not valid} "" { target *-*-* } 19 } */
diff --git a/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c b/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
index 1b3bf4ee2b8..e2f847a31c4 100644
--- a/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
+++ b/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
@@ -1,4 +1,4 @@
 /* { dg-do "compile" } */
 /* { dg-options "-mbranch-protection=leaf -mbranch-protection=none+pac-ret" } */
 
-/* { dg-error "unexpected 'pac-ret' after 'none'"  "" { target *-*-* } 0 } */
+/* { dg-error "argument 'none' can only appear alone in '-mbranch-protection='" "" { target *-*-* } 0 } */
-- 
2.25.1

Re: [PATCH v2 6/7] aarch64,arm: Fix branch-protection= parsing

[Richard Earnshaw]

On 03/11/2023 15:36, Szabolcs Nagy wrote:
> Refactor the parsing to have a single API and fix a few parsing issues:
> 
> - Different handling of "bti+none" and "none+bti": these should be
>    rejected because "none" can only appear alone.
> 
> - Accepted empty strings such as "bti++pac-ret" or "bti+", this bug
>    was caused by using strtok_r.
> 

These now print
   error: invalid argument ‘’ for ‘-mbranch-protection=’

which is OK, but might be a bit confusing.  Perhaps we could change this 
specific case to "missing feature or flag for '-mbranch-protection'".

The ideal solution (IMO) would be if we could print something like

   in option
   -mbranch-protection=+bti
                       ^
                       |
   missing feature or flag

much like we do for source code diagnostics now.

However, I don't know if our framework could handle that for things from 
the command line, and it's not important enough to do now.

> - Memory got leaked (str_root was never freed). And two buffers got
>    allocated when one is enough.
> 
> The callbacks now have no failure mode, only parsing can fail and
> all failures are handled locally.  The "-mbranch-protection=" vs
> "target("branch-protection=")" difference in the error message is
> handled by a separate argument to aarch_validate_mbranch_protection.
> 
> gcc/ChangeLog:
> 
> 	* config/aarch64/aarch64.cc (aarch64_override_options): Update.
> 	(aarch64_handle_attr_branch_protection): Update.
> 	* config/arm/aarch-common-protos.h (aarch_parse_branch_protection):
> 	Remove.
> 	(aarch_validate_mbranch_protection): Add new argument.
> 	* config/arm/aarch-common.cc (aarch_handle_no_branch_protection):
> 	Update.
> 	(aarch_handle_standard_branch_protection): Update.
> 	(aarch_handle_pac_ret_protection): Update.
> 	(aarch_handle_pac_ret_leaf): Update.
> 	(aarch_handle_pac_ret_b_key): Update.
> 	(aarch_handle_bti_protection): Update.
> 	(aarch_parse_branch_protection): Remove.
> 	(next_tok): New.
> 	(aarch_validate_mbranch_protection): Rewrite.
> 	* config/arm/aarch-common.h (struct aarch_branch_protect_type):
> 	Add field "alone".
> 	* config/arm/arm.cc (arm_configure_build_target): Update.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/aarch64/branch-protection-attr.c: Update.
> 	* gcc.target/aarch64/branch-protection-option.c: Update.

This is OK.  If you want to do the simple tweak for the error message 
for the case I mention above, consider that pre-approved.

R.

> ---
> v2: merge tests updates into the patch
> error message is not changed, see previous discussion:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633945.html
> ---
>   gcc/config/aarch64/aarch64.cc                 |  37 +--
>   gcc/config/arm/aarch-common-protos.h          |   5 +-
>   gcc/config/arm/aarch-common.cc                | 214 ++++++++----------
>   gcc/config/arm/aarch-common.h                 |  14 +-
>   gcc/config/arm/arm.cc                         |   3 +-
>   .../aarch64/branch-protection-attr.c          |   6 +-
>   .../aarch64/branch-protection-option.c        |   2 +-
>   7 files changed, 113 insertions(+), 168 deletions(-)
> 
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index f8e8fefc8d8..4f7f707b675 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -18642,7 +18642,8 @@ aarch64_override_options (void)
>       aarch64_validate_sls_mitigation (aarch64_harden_sls_string);
>   
>     if (aarch64_branch_protection_string)
> -    aarch_validate_mbranch_protection (aarch64_branch_protection_string);
> +    aarch_validate_mbranch_protection (aarch64_branch_protection_string,
> +				       "-mbranch-protection=");
>   
>     /* -mcpu=CPU is shorthand for -march=ARCH_FOR_CPU, -mtune=CPU.
>        If either of -march or -mtune is given, they override their
> @@ -19016,34 +19017,12 @@ aarch64_handle_attr_cpu (const char *str)
>   
>   /* Handle the argument STR to the branch-protection= attribute.  */
>   
> - static bool
> - aarch64_handle_attr_branch_protection (const char* str)
> - {
> -  char *err_str = (char *) xmalloc (strlen (str) + 1);
> -  enum aarch_parse_opt_result res = aarch_parse_branch_protection (str,
> -								   &err_str);
> -  bool success = false;
> -  switch (res)
> -    {
> -     case AARCH_PARSE_MISSING_ARG:
> -       error ("missing argument to %<target(\"branch-protection=\")%> pragma or"
> -	      " attribute");
> -       break;
> -     case AARCH_PARSE_INVALID_ARG:
> -       error ("invalid protection type %qs in %<target(\"branch-protection"
> -	      "=\")%> pragma or attribute", err_str);
> -       break;
> -     case AARCH_PARSE_OK:
> -       success = true;
> -      /* Fall through.  */
> -     case AARCH_PARSE_INVALID_FEATURE:
> -       break;
> -     default:
> -       gcc_unreachable ();
> -    }
> -  free (err_str);
> -  return success;
> - }
> +static bool
> +aarch64_handle_attr_branch_protection (const char* str)
> +{
> +  return aarch_validate_mbranch_protection (str,
> +					    "target(\"branch-protection=\")");
> +}
>   
>   /* Handle the argument STR to the tune= target attribute.  */
>   
> diff --git a/gcc/config/arm/aarch-common-protos.h b/gcc/config/arm/aarch-common-protos.h
> index f8cb6562096..75ffdfbb050 100644
> --- a/gcc/config/arm/aarch-common-protos.h
> +++ b/gcc/config/arm/aarch-common-protos.h
> @@ -159,10 +159,7 @@ rtx_insn *arm_md_asm_adjust (vec<rtx> &outputs, vec<rtx> & /*inputs*/,
>   			     vec<rtx> &clobbers, HARD_REG_SET &clobbered_regs,
>   			     location_t loc);
>   
> -/* Parsing routine for branch-protection common to AArch64 and Arm.  */
> -enum aarch_parse_opt_result aarch_parse_branch_protection (const char*, char**);
> -
>   /* Validation routine for branch-protection common to AArch64 and Arm.  */
> -bool aarch_validate_mbranch_protection (const char *);
> +bool aarch_validate_mbranch_protection (const char *, const char *);
>   
>   #endif /* GCC_AARCH_COMMON_PROTOS_H */
> diff --git a/gcc/config/arm/aarch-common.cc b/gcc/config/arm/aarch-common.cc
> index cbc7f68a8bf..159c61b786c 100644
> --- a/gcc/config/arm/aarch-common.cc
> +++ b/gcc/config/arm/aarch-common.cc
> @@ -659,169 +659,143 @@ arm_md_asm_adjust (vec<rtx> &outputs, vec<rtx> & /*inputs*/,
>     return saw_asm_flag ? seq : NULL;
>   }
>   
> -static enum aarch_parse_opt_result
> -aarch_handle_no_branch_protection (char* str, char* rest)
> +static void
> +aarch_handle_no_branch_protection (void)
>   {
>     aarch_ra_sign_scope = AARCH_FUNCTION_NONE;
>     aarch_enable_bti = 0;
> -  if (rest)
> -    {
> -      error ("unexpected %<%s%> after %<%s%>", rest, str);
> -      return AARCH_PARSE_INVALID_FEATURE;
> -    }
> -  return AARCH_PARSE_OK;
>   }
>   
> -static enum aarch_parse_opt_result
> -aarch_handle_standard_branch_protection (char* str, char* rest)
> +static void
> +aarch_handle_standard_branch_protection (void)
>   {
>     aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
>     aarch_ra_sign_key = AARCH_KEY_A;
>     aarch_enable_bti = 1;
> -  if (rest)
> -    {
> -      error ("unexpected %<%s%> after %<%s%>", rest, str);
> -      return AARCH_PARSE_INVALID_FEATURE;
> -    }
> -  return AARCH_PARSE_OK;
>   }
>   
> -static enum aarch_parse_opt_result
> -aarch_handle_pac_ret_protection (char* str ATTRIBUTE_UNUSED,
> -				 char* rest ATTRIBUTE_UNUSED)
> +static void
> +aarch_handle_pac_ret_protection (void)
>   {
>     aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
>     aarch_ra_sign_key = AARCH_KEY_A;
> -  return AARCH_PARSE_OK;
>   }
>   
> -static enum aarch_parse_opt_result
> -aarch_handle_pac_ret_leaf (char* str ATTRIBUTE_UNUSED,
> -			   char* rest ATTRIBUTE_UNUSED)
> +static void
> +aarch_handle_pac_ret_leaf (void)
>   {
>     aarch_ra_sign_scope = AARCH_FUNCTION_ALL;
> -  return AARCH_PARSE_OK;
>   }
>   
> -static enum aarch_parse_opt_result
> -aarch_handle_pac_ret_b_key (char* str ATTRIBUTE_UNUSED,
> -			    char* rest ATTRIBUTE_UNUSED)
> +static void
> +aarch_handle_pac_ret_b_key (void)
>   {
>     aarch_ra_sign_key = AARCH_KEY_B;
> -  return AARCH_PARSE_OK;
>   }
>   
> -static enum aarch_parse_opt_result
> -aarch_handle_bti_protection (char* str ATTRIBUTE_UNUSED,
> -			     char* rest ATTRIBUTE_UNUSED)
> +static void
> +aarch_handle_bti_protection (void)
>   {
>     aarch_enable_bti = 1;
> -  return AARCH_PARSE_OK;
>   }
>   
>   static const struct aarch_branch_protect_type aarch_pac_ret_subtypes[] = {
> -  { "leaf", aarch_handle_pac_ret_leaf, NULL, 0 },
> -  { "b-key", aarch_handle_pac_ret_b_key, NULL, 0 },
> -  { NULL, NULL, NULL, 0 }
> +  { "leaf", false, aarch_handle_pac_ret_leaf, NULL, 0 },
> +  { "b-key", false, aarch_handle_pac_ret_b_key, NULL, 0 },
> +  { NULL, false, NULL, NULL, 0 }
>   };
>   
>   static const struct aarch_branch_protect_type aarch_branch_protect_types[] = {
> -  { "none", aarch_handle_no_branch_protection, NULL, 0 },
> -  { "standard", aarch_handle_standard_branch_protection, NULL, 0 },
> -  { "pac-ret", aarch_handle_pac_ret_protection, aarch_pac_ret_subtypes,
> +  { "none", true, aarch_handle_no_branch_protection, NULL, 0 },
> +  { "standard", true, aarch_handle_standard_branch_protection, NULL, 0 },
> +  { "pac-ret", false, aarch_handle_pac_ret_protection, aarch_pac_ret_subtypes,
>       ARRAY_SIZE (aarch_pac_ret_subtypes) },
> -  { "bti", aarch_handle_bti_protection, NULL, 0 },
> -  { NULL, NULL, NULL, 0 }
> +  { "bti", false, aarch_handle_bti_protection, NULL, 0 },
> +  { NULL, false, NULL, NULL, 0 }
>   };
>   
> -/* Parses CONST_STR for branch protection features specified in
> -   aarch64_branch_protect_types, and set any global variables required.  Returns
> -   the parsing result and assigns LAST_STR to the last processed token from
> -   CONST_STR so that it can be used for error reporting.  */
> +/* In-place split *str at delim, return *str and set *str to the tail
> +   of the string or NULL if the end is reached.  */
>   
> -enum aarch_parse_opt_result
> -aarch_parse_branch_protection (const char *const_str, char** last_str)
> +static char *
> +next_tok (char **str, int delim)
>   {
> -  char *str_root = xstrdup (const_str);
> -  char* token_save = NULL;
> -  char *str = strtok_r (str_root, "+", &token_save);
> -  enum aarch_parse_opt_result res = AARCH_PARSE_OK;
> -  if (!str)
> -    res = AARCH_PARSE_MISSING_ARG;
> -  else
> +  char *tok = *str;
> +  for (char *p = tok; p && *p != '\0'; p++)
>       {
> -      char *next_str = strtok_r (NULL, "+", &token_save);
> -      /* Reset the branch protection features to their defaults.  */
> -      aarch_handle_no_branch_protection (NULL, NULL);
> -
> -      while (str && res == AARCH_PARSE_OK)
> +      if (*p == delim)
>   	{
> -	  const aarch_branch_protect_type* type = aarch_branch_protect_types;
> -	  bool found = false;
> -	  /* Search for this type.  */
> -	  while (type && type->name && !found && res == AARCH_PARSE_OK)
> -	    {
> -	      if (strcmp (str, type->name) == 0)
> -		{
> -		  found = true;
> -		  res = type->handler (str, next_str);
> -		  str = next_str;
> -		  next_str = strtok_r (NULL, "+", &token_save);
> -		}
> -	      else
> -		type++;
> -	    }
> -	  if (found && res == AARCH_PARSE_OK)
> -	    {
> -	      bool found_subtype = true;
> -	      /* Loop through each token until we find one that isn't a
> -		 subtype.  */
> -	      while (found_subtype)
> -		{
> -		  found_subtype = false;
> -		  const aarch_branch_protect_type *subtype = type->subtypes;
> -		  /* Search for the subtype.  */
> -		  while (str && subtype && subtype->name && !found_subtype
> -			  && res == AARCH_PARSE_OK)
> -		    {
> -		      if (strcmp (str, subtype->name) == 0)
> -			{
> -			  found_subtype = true;
> -			  res = subtype->handler (str, next_str);
> -			  str = next_str;
> -			  next_str = strtok_r (NULL, "+", &token_save);
> -			}
> -		      else
> -			subtype++;
> -		    }
> -		}
> -	    }
> -	  else if (!found)
> -	    res = AARCH_PARSE_INVALID_ARG;
> +	  *p = '\0';
> +	  *str = p + 1;
> +	  return tok;
>   	}
>       }
> -  /* Copy the last processed token into the argument to pass it back.
> -    Used by option and attribute validation to print the offending token.  */
> -  if (last_str)
> -    {
> -      if (str)
> -	strcpy (*last_str, str);
> -      else
> -	*last_str = NULL;
> -    }
> -  return res;
> +  *str = NULL;
> +  return tok;
>   }
>   
> +/* Parses CONST_STR for branch protection features specified in
> +   aarch64_branch_protect_types, and set any global variables required.
> +   Returns true on success.  */
> +
>   bool
> -aarch_validate_mbranch_protection (const char *const_str)
> +aarch_validate_mbranch_protection (const char *const_str, const char *opt)
>   {
> -  char *str = (char *) xmalloc (strlen (const_str));
> -  enum aarch_parse_opt_result res =
> -    aarch_parse_branch_protection (const_str, &str);
> -  if (res == AARCH_PARSE_INVALID_ARG)
> -    error ("invalid argument %<%s%> for %<-mbranch-protection=%>", str);
> -  else if (res == AARCH_PARSE_MISSING_ARG)
> -    error ("missing argument for %<-mbranch-protection=%>");
> -  free (str);
> -  return res == AARCH_PARSE_OK;
> +  char *str_root = xstrdup (const_str);
> +  char *next_str = str_root;
> +  char *str = next_tok (&next_str, '+');
> +  char *alone_str = NULL;
> +  bool reject_alone = false;
> +  bool res = true;
> +
> +  /* First entry is "none" and it is used to reset the state.  */
> +  aarch_branch_protect_types[0].handler ();
> +
> +  while (str)
> +    {
> +      const aarch_branch_protect_type *type = aarch_branch_protect_types;
> +      for (; type->name; type++)
> +	if (strcmp (str, type->name) == 0)
> +	  break;
> +      if (type->name == NULL)
> +	{
> +	  res = false;
> +	  error ("invalid argument %<%s%> for %<%s%>", str, opt);
> +	  break;
> +	}
> +
> +      if (type->alone && alone_str == NULL)
> +	alone_str = str;
> +      else
> +	reject_alone = true;
> +      if (reject_alone && alone_str != NULL)
> +	{
> +	  res = false;
> +	  error ("argument %<%s%> can only appear alone in %<%s%>",
> +		 alone_str, opt);
> +	  break;
> +	}
> +
> +      type->handler ();
> +      str = next_tok (&next_str, '+');
> +      if (type->subtypes == NULL)
> +	continue;
> +
> +      /* Loop through tokens until we find one that isn't a subtype.  */
> +      while (str)
> +	{
> +	  const aarch_branch_protect_type *subtype = type->subtypes;
> +	  for (; subtype->name; subtype++)
> +	    if (strcmp (str, subtype->name) == 0)
> +	      break;
> +	  if (subtype->name == NULL)
> +	    break;
> +
> +	  subtype->handler ();
> +	  str = next_tok (&next_str, '+');
> +	}
> +    }
> +
> +  free (str_root);
> +  return res;
>   }
> diff --git a/gcc/config/arm/aarch-common.h b/gcc/config/arm/aarch-common.h
> index c6a67f0d05c..f72e21127fc 100644
> --- a/gcc/config/arm/aarch-common.h
> +++ b/gcc/config/arm/aarch-common.h
> @@ -55,16 +55,10 @@ struct aarch_branch_protect_type
>     /* The type's name that the user passes to the branch-protection option
>        string.  */
>     const char* name;
> -  /* Function to handle the protection type and set global variables.
> -     First argument is the string token corresponding with this type and the
> -     second argument is the next token in the option string.
> -     Return values:
> -     * AARCH_PARSE_OK: Handling was sucessful.
> -     * AARCH_INVALID_ARG: The type is invalid in this context and the caller
> -     should print an error.
> -     * AARCH_INVALID_FEATURE: The type is invalid and the handler prints its
> -     own error.  */
> -  enum aarch_parse_opt_result (*handler)(char*, char*);
> +  /* The type can only appear alone, other types should be rejected.  */
> +  int alone;
> +  /* Function to handle the protection type and set global variables.  */
> +  void (*handler)(void);
>     /* A list of types that can follow this type in the option string.  */
>     const struct aarch_branch_protect_type* subtypes;
>     unsigned int num_subtypes;
> diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
> index f49312cace0..5681073a948 100644
> --- a/gcc/config/arm/arm.cc
> +++ b/gcc/config/arm/arm.cc
> @@ -3297,7 +3297,8 @@ arm_configure_build_target (struct arm_build_target *target,
>   
>     if (opts->x_arm_branch_protection_string)
>       {
> -      aarch_validate_mbranch_protection (opts->x_arm_branch_protection_string);
> +      aarch_validate_mbranch_protection (opts->x_arm_branch_protection_string,
> +					 "-mbranch-protection=");
>   
>         if (aarch_ra_sign_key != AARCH_KEY_A)
>   	{
> diff --git a/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c b/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
> index 272000c2747..dae2a758a56 100644
> --- a/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
> +++ b/gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c
> @@ -4,19 +4,19 @@ void __attribute__ ((target("branch-protection=leaf")))
>   foo1 ()
>   {
>   }
> -/* { dg-error {invalid protection type 'leaf' in 'target\("branch-protection="\)' pragma or attribute} "" { target *-*-* } 5 } */
> +/* { dg-error {invalid argument 'leaf' for 'target\("branch-protection="\)'} "" { target *-*-* } 5 } */
>   /* { dg-error {pragma or attribute 'target\("branch-protection=leaf"\)' is not valid} "" { target *-*-* } 5 } */
>   
>   void __attribute__ ((target("branch-protection=none+pac-ret")))
>   foo2 ()
>   {
>   }
> -/* { dg-error "unexpected 'pac-ret' after 'none'" "" { target *-*-* } 12 } */
> +/* { dg-error {argument 'none' can only appear alone in 'target\("branch-protection="\)'} "" { target *-*-* } 12 } */
>   /* { dg-error {pragma or attribute 'target\("branch-protection=none\+pac-ret"\)' is not valid} "" { target *-*-* } 12 } */
>   
>   void __attribute__ ((target("branch-protection=")))
>   foo3 ()
>   {
>   }
> -/* { dg-error {missing argument to 'target\("branch-protection="\)' pragma or attribute} "" { target *-*-* } 19 } */
> +/* { dg-error {invalid argument '' for 'target\("branch-protection="\)'} "" { target *-*-* } 19 } */
>   /* { dg-error {pragma or attribute 'target\("branch-protection="\)' is not valid} "" { target *-*-* } 19 } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c b/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
> index 1b3bf4ee2b8..e2f847a31c4 100644
> --- a/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
> +++ b/gcc/testsuite/gcc.target/aarch64/branch-protection-option.c
> @@ -1,4 +1,4 @@
>   /* { dg-do "compile" } */
>   /* { dg-options "-mbranch-protection=leaf -mbranch-protection=none+pac-ret" } */
>   
> -/* { dg-error "unexpected 'pac-ret' after 'none'"  "" { target *-*-* } 0 } */
> +/* { dg-error "argument 'none' can only appear alone in '-mbranch-protection='" "" { target *-*-* } 0 } */

[PATCH v2 7/7] aarch64,arm: Move branch-protection data to targets

[Szabolcs Nagy]

The branch-protection types are target specific, not the same on arm
and aarch64.  This currently affects pac-ret+b-key, but there will be
a new type on aarch64 that is not relevant for arm.

gcc/ChangeLog:

	* config/aarch64/aarch64-opts.h (enum aarch64_key_type): Rename to ...
	(enum aarch_key_type): ... this.
	* config/aarch64/aarch64.cc (aarch_handle_no_branch_protection): Copy.
	(aarch_handle_standard_branch_protection): Copy.
	(aarch_handle_pac_ret_protection): Copy.
	(aarch_handle_pac_ret_leaf): Copy.
	(aarch_handle_pac_ret_b_key): Copy.
	(aarch_handle_bti_protection): Copy.
	* config/arm/aarch-common.cc (aarch_handle_no_branch_protection):
	Remove.
	(aarch_handle_standard_branch_protection): Remove.
	(aarch_handle_pac_ret_protection): Remove.
	(aarch_handle_pac_ret_leaf): Remove.
	(aarch_handle_pac_ret_b_key): Remove.
	(aarch_handle_bti_protection): Remove.
	* config/arm/aarch-common.h (enum aarch_key_type): Remove.
	(struct aarch_branch_protect_type): Declare.
	* config/arm/arm-c.cc (arm_cpu_builtins): Remove aarch_ra_sign_key.
	* config/arm/arm.cc (aarch_handle_no_branch_protection): Copy.
	(aarch_handle_standard_branch_protection): Copy.
	(aarch_handle_pac_ret_protection): Copy.
	(aarch_handle_pac_ret_leaf): Copy.
	(aarch_handle_bti_protection): Copy.
	(arm_configure_build_target): Copy.
	* config/arm/arm.opt: Remove aarch_ra_sign_key.
---
unchanged compared to v1.
---
 gcc/config/aarch64/aarch64-opts.h |  6 ++--
 gcc/config/aarch64/aarch64.cc     | 55 +++++++++++++++++++++++++++++++
 gcc/config/arm/aarch-common.cc    | 55 -------------------------------
 gcc/config/arm/aarch-common.h     | 11 +++----
 gcc/config/arm/arm-c.cc           |  2 --
 gcc/config/arm/arm.cc             | 52 +++++++++++++++++++++++++----
 gcc/config/arm/arm.opt            |  3 --
 7 files changed, 109 insertions(+), 75 deletions(-)

diff --git a/gcc/config/aarch64/aarch64-opts.h b/gcc/config/aarch64/aarch64-opts.h
index 831e28ab52a..1abae1442b5 100644
--- a/gcc/config/aarch64/aarch64-opts.h
+++ b/gcc/config/aarch64/aarch64-opts.h
@@ -103,9 +103,9 @@ enum stack_protector_guard {
 };
 
 /* The key type that -msign-return-address should use.  */
-enum aarch64_key_type {
-  AARCH64_KEY_A,
-  AARCH64_KEY_B
+enum aarch_key_type {
+  AARCH_KEY_A,
+  AARCH_KEY_B
 };
 
 /* An enum specifying how to handle load and store pairs using
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 4f7f707b675..9739223831f 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -18620,6 +18620,61 @@ aarch64_set_asm_isa_flags (aarch64_feature_flags flags)
   aarch64_set_asm_isa_flags (&global_options, flags);
 }
 
+static void
+aarch_handle_no_branch_protection (void)
+{
+  aarch_ra_sign_scope = AARCH_FUNCTION_NONE;
+  aarch_enable_bti = 0;
+}
+
+static void
+aarch_handle_standard_branch_protection (void)
+{
+  aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
+  aarch_ra_sign_key = AARCH_KEY_A;
+  aarch_enable_bti = 1;
+}
+
+static void
+aarch_handle_pac_ret_protection (void)
+{
+  aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
+  aarch_ra_sign_key = AARCH_KEY_A;
+}
+
+static void
+aarch_handle_pac_ret_leaf (void)
+{
+  aarch_ra_sign_scope = AARCH_FUNCTION_ALL;
+}
+
+static void
+aarch_handle_pac_ret_b_key (void)
+{
+  aarch_ra_sign_key = AARCH_KEY_B;
+}
+
+static void
+aarch_handle_bti_protection (void)
+{
+  aarch_enable_bti = 1;
+}
+
+static const struct aarch_branch_protect_type aarch_pac_ret_subtypes[] = {
+  { "leaf", false, aarch_handle_pac_ret_leaf, NULL, 0 },
+  { "b-key", false, aarch_handle_pac_ret_b_key, NULL, 0 },
+  { NULL, false, NULL, NULL, 0 }
+};
+
+const struct aarch_branch_protect_type aarch_branch_protect_types[] = {
+  { "none", true, aarch_handle_no_branch_protection, NULL, 0 },
+  { "standard", true, aarch_handle_standard_branch_protection, NULL, 0 },
+  { "pac-ret", false, aarch_handle_pac_ret_protection, aarch_pac_ret_subtypes,
+    ARRAY_SIZE (aarch_pac_ret_subtypes) },
+  { "bti", false, aarch_handle_bti_protection, NULL, 0 },
+  { NULL, false, NULL, NULL, 0 }
+};
+
 /* Implement TARGET_OPTION_OVERRIDE.  This is called once in the beginning
    and is used to parse the -m{cpu,tune,arch} strings and setup the initial
    tuning structs.  In particular it must set selected_tune and
diff --git a/gcc/config/arm/aarch-common.cc b/gcc/config/arm/aarch-common.cc
index 159c61b786c..92e1248f83f 100644
--- a/gcc/config/arm/aarch-common.cc
+++ b/gcc/config/arm/aarch-common.cc
@@ -659,61 +659,6 @@ arm_md_asm_adjust (vec<rtx> &outputs, vec<rtx> & /*inputs*/,
   return saw_asm_flag ? seq : NULL;
 }
 
-static void
-aarch_handle_no_branch_protection (void)
-{
-  aarch_ra_sign_scope = AARCH_FUNCTION_NONE;
-  aarch_enable_bti = 0;
-}
-
-static void
-aarch_handle_standard_branch_protection (void)
-{
-  aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
-  aarch_ra_sign_key = AARCH_KEY_A;
-  aarch_enable_bti = 1;
-}
-
-static void
-aarch_handle_pac_ret_protection (void)
-{
-  aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
-  aarch_ra_sign_key = AARCH_KEY_A;
-}
-
-static void
-aarch_handle_pac_ret_leaf (void)
-{
-  aarch_ra_sign_scope = AARCH_FUNCTION_ALL;
-}
-
-static void
-aarch_handle_pac_ret_b_key (void)
-{
-  aarch_ra_sign_key = AARCH_KEY_B;
-}
-
-static void
-aarch_handle_bti_protection (void)
-{
-  aarch_enable_bti = 1;
-}
-
-static const struct aarch_branch_protect_type aarch_pac_ret_subtypes[] = {
-  { "leaf", false, aarch_handle_pac_ret_leaf, NULL, 0 },
-  { "b-key", false, aarch_handle_pac_ret_b_key, NULL, 0 },
-  { NULL, false, NULL, NULL, 0 }
-};
-
-static const struct aarch_branch_protect_type aarch_branch_protect_types[] = {
-  { "none", true, aarch_handle_no_branch_protection, NULL, 0 },
-  { "standard", true, aarch_handle_standard_branch_protection, NULL, 0 },
-  { "pac-ret", false, aarch_handle_pac_ret_protection, aarch_pac_ret_subtypes,
-    ARRAY_SIZE (aarch_pac_ret_subtypes) },
-  { "bti", false, aarch_handle_bti_protection, NULL, 0 },
-  { NULL, false, NULL, NULL, 0 }
-};
-
 /* In-place split *str at delim, return *str and set *str to the tail
    of the string or NULL if the end is reached.  */
 
diff --git a/gcc/config/arm/aarch-common.h b/gcc/config/arm/aarch-common.h
index f72e21127fc..90d2112408f 100644
--- a/gcc/config/arm/aarch-common.h
+++ b/gcc/config/arm/aarch-common.h
@@ -44,12 +44,7 @@ enum aarch_function_type {
   AARCH_FUNCTION_ALL
 };
 
-/* The key type that -msign-return-address should use.  */
-enum aarch_key_type {
-  AARCH_KEY_A,
-  AARCH_KEY_B
-};
-
+/* Specifies a -mbranch-protection= argument.  */
 struct aarch_branch_protect_type
 {
   /* The type's name that the user passes to the branch-protection option
@@ -64,4 +59,8 @@ struct aarch_branch_protect_type
   unsigned int num_subtypes;
 };
 
+/* Target specific data and callbacks for parsing -mbranch-protection=.
+   The first item is used to reset the branch-protection settings.  */
+extern const struct aarch_branch_protect_type aarch_branch_protect_types[];
+
 #endif /* GCC_AARCH_COMMON_H */
diff --git a/gcc/config/arm/arm-c.cc b/gcc/config/arm/arm-c.cc
index d3d93ceba00..204403b3ff4 100644
--- a/gcc/config/arm/arm-c.cc
+++ b/gcc/config/arm/arm-c.cc
@@ -248,8 +248,6 @@ arm_cpu_builtins (struct cpp_reader* pfile)
   {
     unsigned int pac = 1;
 
-    gcc_assert (aarch_ra_sign_key == AARCH_KEY_A);
-
     if (aarch_ra_sign_scope == AARCH_FUNCTION_ALL)
       pac |= 0x4;
 
diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index 5681073a948..5cb0f2858b7 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -3250,6 +3250,52 @@ static sbitmap isa_all_fpubits_internal;
 static sbitmap isa_all_fpbits;
 static sbitmap isa_quirkbits;
 
+static void
+aarch_handle_no_branch_protection (void)
+{
+  aarch_ra_sign_scope = AARCH_FUNCTION_NONE;
+  aarch_enable_bti = 0;
+}
+
+static void
+aarch_handle_standard_branch_protection (void)
+{
+  aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
+  aarch_enable_bti = 1;
+}
+
+static void
+aarch_handle_pac_ret_protection (void)
+{
+  aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
+}
+
+static void
+aarch_handle_pac_ret_leaf (void)
+{
+  aarch_ra_sign_scope = AARCH_FUNCTION_ALL;
+}
+
+static void
+aarch_handle_bti_protection (void)
+{
+  aarch_enable_bti = 1;
+}
+
+static const struct aarch_branch_protect_type aarch_pac_ret_subtypes[] = {
+  { "leaf", false, aarch_handle_pac_ret_leaf, NULL, 0 },
+  { NULL, false, NULL, NULL, 0 }
+};
+
+const struct aarch_branch_protect_type aarch_branch_protect_types[] = {
+  { "none", true, aarch_handle_no_branch_protection, NULL, 0 },
+  { "standard", true, aarch_handle_standard_branch_protection, NULL, 0 },
+  { "pac-ret", false, aarch_handle_pac_ret_protection, aarch_pac_ret_subtypes,
+    ARRAY_SIZE (aarch_pac_ret_subtypes) },
+  { "bti", false, aarch_handle_bti_protection, NULL, 0 },
+  { NULL, false, NULL, NULL, 0 }
+};
+
 /* Configure a build target TARGET from the user-specified options OPTS and
    OPTS_SET.  If WARN_COMPATIBLE, emit a diagnostic if both the CPU and
    architecture have been specified, but the two are not identical.  */
@@ -3299,12 +3345,6 @@ arm_configure_build_target (struct arm_build_target *target,
     {
       aarch_validate_mbranch_protection (opts->x_arm_branch_protection_string,
 					 "-mbranch-protection=");
-
-      if (aarch_ra_sign_key != AARCH_KEY_A)
-	{
-	  warning (0, "invalid key type for %<-mbranch-protection=%>");
-	  aarch_ra_sign_key = AARCH_KEY_A;
-	}
     }
 
   if (arm_selected_arch)
diff --git a/gcc/config/arm/arm.opt b/gcc/config/arm/arm.opt
index 88299dabc3a..bd8bb00d035 100644
--- a/gcc/config/arm/arm.opt
+++ b/gcc/config/arm/arm.opt
@@ -30,9 +30,6 @@ enum aarch_function_type aarch_ra_sign_scope = AARCH_FUNCTION_NONE
 TargetVariable
 unsigned aarch_enable_bti = 0
 
-TargetVariable
-enum aarch_key_type aarch_ra_sign_key = AARCH_KEY_A
-
 Enum
 Name(tls_type) Type(enum arm_tls_type)
 TLS dialect to use:
-- 
2.25.1

Re: [PATCH v2 7/7] aarch64,arm: Move branch-protection data to targets

[Richard Earnshaw]

On 03/11/2023 15:36, Szabolcs Nagy wrote:
> The branch-protection types are target specific, not the same on arm
> and aarch64.  This currently affects pac-ret+b-key, but there will be
> a new type on aarch64 that is not relevant for arm.
> 
> gcc/ChangeLog:
> 
> 	* config/aarch64/aarch64-opts.h (enum aarch64_key_type): Rename to ...
> 	(enum aarch_key_type): ... this.
> 	* config/aarch64/aarch64.cc (aarch_handle_no_branch_protection): Copy.
> 	(aarch_handle_standard_branch_protection): Copy.
> 	(aarch_handle_pac_ret_protection): Copy.
> 	(aarch_handle_pac_ret_leaf): Copy.
> 	(aarch_handle_pac_ret_b_key): Copy.
> 	(aarch_handle_bti_protection): Copy.

I think all of the above functions that have been moved back from 
aarch-common should be renamed back to aarch64_..., unless they are 
directly referenced statically by code in aarch-common.c.
> 	* config/arm/aarch-common.cc (aarch_handle_no_branch_protection):
> 	Remove.
> 	(aarch_handle_standard_branch_protection): Remove.
> 	(aarch_handle_pac_ret_protection): Remove.
> 	(aarch_handle_pac_ret_leaf): Remove.
> 	(aarch_handle_pac_ret_b_key): Remove.
> 	(aarch_handle_bti_protection): Remove.
> 	* config/arm/aarch-common.h (enum aarch_key_type): Remove.
> 	(struct aarch_branch_protect_type): Declare.
> 	* config/arm/arm-c.cc (arm_cpu_builtins): Remove aarch_ra_sign_key.
> 	* config/arm/arm.cc (aarch_handle_no_branch_protection): Copy.
> 	(aarch_handle_standard_branch_protection): Copy.
> 	(aarch_handle_pac_ret_protection): Copy.
> 	(aarch_handle_pac_ret_leaf): Copy.
> 	(aarch_handle_bti_protection): Copy.
> 	(arm_configure_build_target): Copy.

And the same here.

> 	* config/arm/arm.opt: Remove aarch_ra_sign_key.
> ---
> unchanged compared to v1.
> ---
>   gcc/config/aarch64/aarch64-opts.h |  6 ++--
>   gcc/config/aarch64/aarch64.cc     | 55 +++++++++++++++++++++++++++++++
>   gcc/config/arm/aarch-common.cc    | 55 -------------------------------
>   gcc/config/arm/aarch-common.h     | 11 +++----
>   gcc/config/arm/arm-c.cc           |  2 --
>   gcc/config/arm/arm.cc             | 52 +++++++++++++++++++++++++----
>   gcc/config/arm/arm.opt            |  3 --
>   7 files changed, 109 insertions(+), 75 deletions(-)
> 
> diff --git a/gcc/config/aarch64/aarch64-opts.h b/gcc/config/aarch64/aarch64-opts.h
> index 831e28ab52a..1abae1442b5 100644
> --- a/gcc/config/aarch64/aarch64-opts.h
> +++ b/gcc/config/aarch64/aarch64-opts.h
> @@ -103,9 +103,9 @@ enum stack_protector_guard {
>   };
>   
>   /* The key type that -msign-return-address should use.  */
> -enum aarch64_key_type {
> -  AARCH64_KEY_A,
> -  AARCH64_KEY_B
> +enum aarch_key_type {
> +  AARCH_KEY_A,
> +  AARCH_KEY_B
>   };
>   
>   /* An enum specifying how to handle load and store pairs using
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 4f7f707b675..9739223831f 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -18620,6 +18620,61 @@ aarch64_set_asm_isa_flags (aarch64_feature_flags flags)
>     aarch64_set_asm_isa_flags (&global_options, flags);
>   }
>   
> +static void
> +aarch_handle_no_branch_protection (void)
> +{
> +  aarch_ra_sign_scope = AARCH_FUNCTION_NONE;
> +  aarch_enable_bti = 0;
> +}
> +
> +static void
> +aarch_handle_standard_branch_protection (void)
> +{
> +  aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
> +  aarch_ra_sign_key = AARCH_KEY_A;
> +  aarch_enable_bti = 1;
> +}
> +
> +static void
> +aarch_handle_pac_ret_protection (void)
> +{
> +  aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
> +  aarch_ra_sign_key = AARCH_KEY_A;
> +}
> +
> +static void
> +aarch_handle_pac_ret_leaf (void)
> +{
> +  aarch_ra_sign_scope = AARCH_FUNCTION_ALL;
> +}
> +
> +static void
> +aarch_handle_pac_ret_b_key (void)
> +{
> +  aarch_ra_sign_key = AARCH_KEY_B;
> +}
> +
> +static void
> +aarch_handle_bti_protection (void)
> +{
> +  aarch_enable_bti = 1;
> +}
> +
> +static const struct aarch_branch_protect_type aarch_pac_ret_subtypes[] = {
> +  { "leaf", false, aarch_handle_pac_ret_leaf, NULL, 0 },
> +  { "b-key", false, aarch_handle_pac_ret_b_key, NULL, 0 },
> +  { NULL, false, NULL, NULL, 0 }
> +};
> +
> +const struct aarch_branch_protect_type aarch_branch_protect_types[] = {

can this be made static now?  And maybe pass the structure as a 
parameter if that's not done already.


> +  { "none", true, aarch_handle_no_branch_protection, NULL, 0 },
> +  { "standard", true, aarch_handle_standard_branch_protection, NULL, 0 },
> +  { "pac-ret", false, aarch_handle_pac_ret_protection, aarch_pac_ret_subtypes,
> +    ARRAY_SIZE (aarch_pac_ret_subtypes) },
> +  { "bti", false, aarch_handle_bti_protection, NULL, 0 },
> +  { NULL, false, NULL, NULL, 0 }
> +};
> +
>   /* Implement TARGET_OPTION_OVERRIDE.  This is called once in the beginning
>      and is used to parse the -m{cpu,tune,arch} strings and setup the initial
>      tuning structs.  In particular it must set selected_tune and
> diff --git a/gcc/config/arm/aarch-common.cc b/gcc/config/arm/aarch-common.cc
> index 159c61b786c..92e1248f83f 100644
> --- a/gcc/config/arm/aarch-common.cc
> +++ b/gcc/config/arm/aarch-common.cc
> @@ -659,61 +659,6 @@ arm_md_asm_adjust (vec<rtx> &outputs, vec<rtx> & /*inputs*/,
>     return saw_asm_flag ? seq : NULL;
>   }
>   
> -static void
> -aarch_handle_no_branch_protection (void)
> -{
> -  aarch_ra_sign_scope = AARCH_FUNCTION_NONE;
> -  aarch_enable_bti = 0;
> -}
> -
> -static void
> -aarch_handle_standard_branch_protection (void)
> -{
> -  aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
> -  aarch_ra_sign_key = AARCH_KEY_A;
> -  aarch_enable_bti = 1;
> -}
> -
> -static void
> -aarch_handle_pac_ret_protection (void)
> -{
> -  aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
> -  aarch_ra_sign_key = AARCH_KEY_A;
> -}
> -
> -static void
> -aarch_handle_pac_ret_leaf (void)
> -{
> -  aarch_ra_sign_scope = AARCH_FUNCTION_ALL;
> -}
> -
> -static void
> -aarch_handle_pac_ret_b_key (void)
> -{
> -  aarch_ra_sign_key = AARCH_KEY_B;
> -}
> -
> -static void
> -aarch_handle_bti_protection (void)
> -{
> -  aarch_enable_bti = 1;
> -}
> -
> -static const struct aarch_branch_protect_type aarch_pac_ret_subtypes[] = {
> -  { "leaf", false, aarch_handle_pac_ret_leaf, NULL, 0 },
> -  { "b-key", false, aarch_handle_pac_ret_b_key, NULL, 0 },
> -  { NULL, false, NULL, NULL, 0 }
> -};
> -
> -static const struct aarch_branch_protect_type aarch_branch_protect_types[] = {
> -  { "none", true, aarch_handle_no_branch_protection, NULL, 0 },
> -  { "standard", true, aarch_handle_standard_branch_protection, NULL, 0 },
> -  { "pac-ret", false, aarch_handle_pac_ret_protection, aarch_pac_ret_subtypes,
> -    ARRAY_SIZE (aarch_pac_ret_subtypes) },
> -  { "bti", false, aarch_handle_bti_protection, NULL, 0 },
> -  { NULL, false, NULL, NULL, 0 }
> -};
> -
>   /* In-place split *str at delim, return *str and set *str to the tail
>      of the string or NULL if the end is reached.  */
>   
> diff --git a/gcc/config/arm/aarch-common.h b/gcc/config/arm/aarch-common.h
> index f72e21127fc..90d2112408f 100644
> --- a/gcc/config/arm/aarch-common.h
> +++ b/gcc/config/arm/aarch-common.h
> @@ -44,12 +44,7 @@ enum aarch_function_type {
>     AARCH_FUNCTION_ALL
>   };
>   
> -/* The key type that -msign-return-address should use.  */
> -enum aarch_key_type {
> -  AARCH_KEY_A,
> -  AARCH_KEY_B
> -};
> -
> +/* Specifies a -mbranch-protection= argument.  */
>   struct aarch_branch_protect_type
>   {
>     /* The type's name that the user passes to the branch-protection option
> @@ -64,4 +59,8 @@ struct aarch_branch_protect_type
>     unsigned int num_subtypes;
>   };
>   
> +/* Target specific data and callbacks for parsing -mbranch-protection=.
> +   The first item is used to reset the branch-protection settings.  */
> +extern const struct aarch_branch_protect_type aarch_branch_protect_types[];
> +
>   #endif /* GCC_AARCH_COMMON_H */
> diff --git a/gcc/config/arm/arm-c.cc b/gcc/config/arm/arm-c.cc
> index d3d93ceba00..204403b3ff4 100644
> --- a/gcc/config/arm/arm-c.cc
> +++ b/gcc/config/arm/arm-c.cc
> @@ -248,8 +248,6 @@ arm_cpu_builtins (struct cpp_reader* pfile)
>     {
>       unsigned int pac = 1;
>   
> -    gcc_assert (aarch_ra_sign_key == AARCH_KEY_A);
> -
>       if (aarch_ra_sign_scope == AARCH_FUNCTION_ALL)
>         pac |= 0x4;
>   
> diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
> index 5681073a948..5cb0f2858b7 100644
> --- a/gcc/config/arm/arm.cc
> +++ b/gcc/config/arm/arm.cc
> @@ -3250,6 +3250,52 @@ static sbitmap isa_all_fpubits_internal;
>   static sbitmap isa_all_fpbits;
>   static sbitmap isa_quirkbits;
>   
> +static void
> +aarch_handle_no_branch_protection (void)
> +{
> +  aarch_ra_sign_scope = AARCH_FUNCTION_NONE;
> +  aarch_enable_bti = 0;
> +}
> +
> +static void
> +aarch_handle_standard_branch_protection (void)
> +{
> +  aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
> +  aarch_enable_bti = 1;
> +}
> +
> +static void
> +aarch_handle_pac_ret_protection (void)
> +{
> +  aarch_ra_sign_scope = AARCH_FUNCTION_NON_LEAF;
> +}
> +
> +static void
> +aarch_handle_pac_ret_leaf (void)
> +{
> +  aarch_ra_sign_scope = AARCH_FUNCTION_ALL;
> +}
> +
> +static void
> +aarch_handle_bti_protection (void)
> +{
> +  aarch_enable_bti = 1;
> +}
> +
> +static const struct aarch_branch_protect_type aarch_pac_ret_subtypes[] = {
> +  { "leaf", false, aarch_handle_pac_ret_leaf, NULL, 0 },
> +  { NULL, false, NULL, NULL, 0 }
> +};
> +
> +const struct aarch_branch_protect_type aarch_branch_protect_types[] = {
> +  { "none", true, aarch_handle_no_branch_protection, NULL, 0 },
> +  { "standard", true, aarch_handle_standard_branch_protection, NULL, 0 },
> +  { "pac-ret", false, aarch_handle_pac_ret_protection, aarch_pac_ret_subtypes,
> +    ARRAY_SIZE (aarch_pac_ret_subtypes) },
> +  { "bti", false, aarch_handle_bti_protection, NULL, 0 },
> +  { NULL, false, NULL, NULL, 0 }
> +};
> +
>   /* Configure a build target TARGET from the user-specified options OPTS and
>      OPTS_SET.  If WARN_COMPATIBLE, emit a diagnostic if both the CPU and
>      architecture have been specified, but the two are not identical.  */
> @@ -3299,12 +3345,6 @@ arm_configure_build_target (struct arm_build_target *target,
>       {
>         aarch_validate_mbranch_protection (opts->x_arm_branch_protection_string,
>   					 "-mbranch-protection=");
> -
> -      if (aarch_ra_sign_key != AARCH_KEY_A)
> -	{
> -	  warning (0, "invalid key type for %<-mbranch-protection=%>");
> -	  aarch_ra_sign_key = AARCH_KEY_A;
> -	}
>       }
>   
>     if (arm_selected_arch)
> diff --git a/gcc/config/arm/arm.opt b/gcc/config/arm/arm.opt
> index 88299dabc3a..bd8bb00d035 100644
> --- a/gcc/config/arm/arm.opt
> +++ b/gcc/config/arm/arm.opt
> @@ -30,9 +30,6 @@ enum aarch_function_type aarch_ra_sign_scope = AARCH_FUNCTION_NONE
>   TargetVariable
>   unsigned aarch_enable_bti = 0
>   
> -TargetVariable
> -enum aarch_key_type aarch_ra_sign_key = AARCH_KEY_A
> -
>   Enum
>   Name(tls_type) Type(enum arm_tls_type)
>   TLS dialect to use:

It would be nice if, when we raise an error, we could print out the list 
of valid options (and modifiers), much like we do on Arm for -march/-mcpu.

eg.
$ gcc -mcpu=crotex-a8
cc1: error: unrecognised -mcpu target: crotex-a8
cc1: note: valid arguments are: arm8 arm810 strongarm strongarm110 fa526 
[...rest of list]; did you mean ‘cortex-a8’?

R.

Re: [PATCH v2 7/7] aarch64,arm: Move branch-protection data to targets

[Szabolcs Nagy]

The 12/07/2023 13:13, Richard Earnshaw wrote:
> On 03/11/2023 15:36, Szabolcs Nagy wrote:
> > 	* config/aarch64/aarch64.cc (aarch_handle_no_branch_protection): Copy.
> > 	(aarch_handle_standard_branch_protection): Copy.
> > 	(aarch_handle_pac_ret_protection): Copy.
> > 	(aarch_handle_pac_ret_leaf): Copy.
> > 	(aarch_handle_pac_ret_b_key): Copy.
> > 	(aarch_handle_bti_protection): Copy.
> 
> I think all of the above functions that have been moved back from
> aarch-common should be renamed back to aarch64_..., unless they are directly
> referenced statically by code in aarch-common.c.

done.

> > +const struct aarch_branch_protect_type aarch_branch_protect_types[] = {
> 
> can this be made static now?  And maybe pass the structure as a parameter if
> that's not done already.

done in v4.

> It would be nice if, when we raise an error, we could print out the list of
> valid options (and modifiers), much like we do on Arm for -march/-mcpu.
> 
> eg.
> $ gcc -mcpu=crotex-a8
> cc1: error: unrecognised -mcpu target: crotex-a8
> cc1: note: valid arguments are: arm8 arm810 strongarm strongarm110 fa526
> [...rest of list]; did you mean ‘cortex-a8’?

i implemented this with candidates_list_and_hint but it does
not work very well if the typo is in a subtype, so i think
this should be done in a separate patch if at all.

Re: [PATCH v2 7/7] aarch64,arm: Move branch-protection data to targets

[Richard Earnshaw (lists)]

On 11/01/2024 14:43, Szabolcs Nagy wrote:
> The 12/07/2023 13:13, Richard Earnshaw wrote:
>> On 03/11/2023 15:36, Szabolcs Nagy wrote:
>>> 	* config/aarch64/aarch64.cc (aarch_handle_no_branch_protection): Copy.
>>> 	(aarch_handle_standard_branch_protection): Copy.
>>> 	(aarch_handle_pac_ret_protection): Copy.
>>> 	(aarch_handle_pac_ret_leaf): Copy.
>>> 	(aarch_handle_pac_ret_b_key): Copy.
>>> 	(aarch_handle_bti_protection): Copy.
>>
>> I think all of the above functions that have been moved back from
>> aarch-common should be renamed back to aarch64_..., unless they are directly
>> referenced statically by code in aarch-common.c.
> 
> done.
> 
>>> +const struct aarch_branch_protect_type aarch_branch_protect_types[] = {
>>
>> can this be made static now?  And maybe pass the structure as a parameter if
>> that's not done already.
> 
> done in v4.
> 
>> It would be nice if, when we raise an error, we could print out the list of
>> valid options (and modifiers), much like we do on Arm for -march/-mcpu.
>>
>> eg.
>> $ gcc -mcpu=crotex-a8
>> cc1: error: unrecognised -mcpu target: crotex-a8
>> cc1: note: valid arguments are: arm8 arm810 strongarm strongarm110 fa526
>> [...rest of list]; did you mean ‘cortex-a8’?
> 
> i implemented this with candidates_list_and_hint but it does
> not work very well if the typo is in a subtype, so i think
> this should be done in a separate patch if at all.
> 

I'd build the candidates list from all the types + subtypes, so that the suggestion code has a full list to pick from; but fair enough.

R.