[PATCH 00/11] aarch64 GCS preliminary patches

[PATCH 00/11] 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.

GCS support will depend on the linux ABI that is under discussion at
https://lore.kernel.org/lkml/20230807-arm64-gcs-v4-0-68cfa37f9069@kernel.org/
so it will come later.

Richard Sandiford (1):
  Handle epilogues that contain jumps

Szabolcs Nagy (10):
  aarch64: AARCH64_ISA_RCPC was defined twice
  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: Fix pac-ret eh_return tests
  aarch64: Disable branch-protection for pcs tests
  aarch64,arm: Remove accepted_branch_protection_string
  aarch64,arm: Fix branch-protection= parsing
  aarch64: Fix branch-protection error message tests
  aarch64,arm: Move branch-protection data to targets

 gcc/config/aarch64/aarch64-opts.h             |   6 +-
 gcc/config/aarch64/aarch64-protos.h           |   2 +-
 gcc/config/aarch64/aarch64.cc                 | 211 +++++++++-------
 gcc/config/aarch64/aarch64.h                  |  12 +-
 gcc/config/aarch64/aarch64.md                 |   8 +
 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/function.cc                               |  10 +
 .../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          |  14 ++
 .../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 +-
 25 files changed, 338 insertions(+), 326 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 01/11] aarch64: AARCH64_ISA_RCPC was defined twice

[Szabolcs Nagy]

gcc/ChangeLog:

	* config/aarch64/aarch64.h (AARCH64_ISA_RCPC): Remove dup.
---
 gcc/config/aarch64/aarch64.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 2b0fc97bb71..c783cb96c48 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -222,7 +222,6 @@ enum class aarch64_feature : unsigned char {
 #define AARCH64_ISA_MOPS	   (aarch64_isa_flags & AARCH64_FL_MOPS)
 #define AARCH64_ISA_LS64	   (aarch64_isa_flags & AARCH64_FL_LS64)
 #define AARCH64_ISA_CSSC	   (aarch64_isa_flags & AARCH64_FL_CSSC)
-#define AARCH64_ISA_RCPC           (aarch64_isa_flags & AARCH64_FL_RCPC)
 
 /* Crypto is an optional extension to AdvSIMD.  */
 #define TARGET_CRYPTO (AARCH64_ISA_CRYPTO)
-- 
2.25.1

[PATCH 02/11] Handle epilogues that contain jumps

[Szabolcs Nagy]

From: Richard Sandiford <richard.sandiford@arm.com>

The prologue/epilogue pass allows the prologue sequence
to contain jumps.  The sequence is then partitioned into
basic blocks using find_many_sub_basic_blocks.

This patch treats epilogues in the same way.  It's needed for
a follow-on aarch64 patch that adds conditional code to both
the prologue and the epilogue.

Tested on aarch64-linux-gnu (including with a follow-on patch)
and x86_64-linux-gnu.  OK to install?

Richard

gcc/
	* function.cc (thread_prologue_and_epilogue_insns): Handle
	epilogues that contain jumps.
---

This is a previously approved patch that was not committed
because it was not needed at the time, but i'd like to commit
it as it is needed for the followup aarch64 eh_return changes:

https://gcc.gnu.org/pipermail/gcc-patches/2022-November/605769.html

---
 gcc/function.cc | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/gcc/function.cc b/gcc/function.cc
index dd2c1136e07..70d1cd65303 100644
--- a/gcc/function.cc
+++ b/gcc/function.cc
@@ -6120,6 +6120,11 @@ thread_prologue_and_epilogue_insns (void)
 		  && returnjump_p (BB_END (e->src)))
 		e->flags &= ~EDGE_FALLTHRU;
 	    }
+
+	  auto_sbitmap blocks (last_basic_block_for_fn (cfun));
+	  bitmap_clear (blocks);
+	    bitmap_set_bit (blocks, BLOCK_FOR_INSN (epilogue_seq)->index);
+	  find_many_sub_basic_blocks (blocks);
 	}
       else if (next_active_insn (BB_END (exit_fallthru_edge->src)))
 	{
@@ -6218,6 +6223,11 @@ thread_prologue_and_epilogue_insns (void)
 	  set_insn_locations (seq, epilogue_location);
 
 	  emit_insn_before (seq, insn);
+
+	  auto_sbitmap blocks (last_basic_block_for_fn (cfun));
+	  bitmap_clear (blocks);
+	  bitmap_set_bit (blocks, BLOCK_FOR_INSN (insn)->index);
+	  find_many_sub_basic_blocks (blocks);
 	}
     }
 
-- 
2.25.1

[PATCH 03/11] 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 using the EH_RETURN_STACKADJ_RTX
register only as a flag that is set to 1 in the eh_return paths
(it is 0 in normal return paths) and introduces

  AARCH64_EH_RETURN_STACKADJ_RTX
  AARCH64_EH_RETURN_HANDLER_RTX

to pass the actual stack adjustment and landing pad address to the
epilogue in the eh_return case. Then the epilogue can use the right
return sequence based on the EH_RETURN_STACKADJ_RTX flag.

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.

The new code generation for functions with eh_return is not amazing,
since x5 and x6 is assumed to be used by the epilogue even in the
normal return path, not just for eh_return.  But only the unwinder
is expected to use eh_return so this is fine.

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.

gcc/ChangeLog:

	* config/aarch64/aarch64-protos.h (aarch64_eh_return_handler_rtx):
	Remove.
	(aarch64_eh_return): New.
	* config/aarch64/aarch64.cc (aarch64_return_address_signing_enabled):
	Sign return address even in functions with eh_return.
	(aarch64_epilogue_uses): Mark two registers as used.
	(aarch64_expand_epilogue): Conditionally return with br or ret.
	(aarch64_eh_return_handler_rtx): Remove.
	(aarch64_eh_return): New.
	* config/aarch64/aarch64.h (EH_RETURN_HANDLER_RTX): Remove.
	(AARCH64_EH_RETURN_STACKADJ_REGNUM): Define.
	(AARCH64_EH_RETURN_STACKADJ_RTX): Define.
	(AARCH64_EH_RETURN_HANDLER_REGNUM): Define.
	(AARCH64_EH_RETURN_HANDLER_RTX): Define.
	* config/aarch64/aarch64.md (eh_return): New.
---
 gcc/config/aarch64/aarch64-protos.h |   2 +-
 gcc/config/aarch64/aarch64.cc       | 106 +++++++++++++++-------------
 gcc/config/aarch64/aarch64.h        |  11 ++-
 gcc/config/aarch64/aarch64.md       |   8 +++
 4 files changed, 73 insertions(+), 54 deletions(-)

diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index 70303d6fd95..5d1834162a4 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -855,7 +855,7 @@ 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);
+void aarch64_eh_return (rtx);
 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 eba5d4a7e04..36cd172d182 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -8972,17 +8972,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
@@ -9932,9 +9921,8 @@ aarch64_allocate_and_probe_stack_space (rtx temp1, rtx temp2,
    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.  */
+   For EH return we need to keep two registers alive for stack adjustment
+   and return address.  */
 
 int
 aarch64_epilogue_uses (int regno)
@@ -9944,6 +9932,13 @@ aarch64_epilogue_uses (int regno)
       if (regno == LR_REGNUM)
 	return 1;
     }
+
+  if (!epilogue_completed && crtl->calls_eh_return)
+    {
+      if (regno == AARCH64_EH_RETURN_STACKADJ_REGNUM
+	  || regno == AARCH64_EH_RETURN_HANDLER_REGNUM)
+	return 1;
+    }
   return 0;
 }
 
@@ -10342,6 +10337,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_STACKADJ_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_STACKADJ_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,
+				AARCH64_EH_RETURN_STACKADJ_RTX));
+      emit_jump_insn (gen_indirect_jump (AARCH64_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.
@@ -10371,56 +10390,41 @@ 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.
+/* Implement the eh_return instruction pattern.  Functions with EH returns
+   either return normally or return to a previous frame after unwinding.
 
-   An EH return uses a single shared return sequence.  The epilogue is
+   The two cases use 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));
+   An actual EH return initializes the stack adjustment then invokes this
+   target hook (which supposed to overwrite the return address) and then
+   jumps directly into the epilogue (bypassing the zeroing of the adjustment).
+
+   We depart from the intended EH return logic by using two additional
+   registers to pass the handler and stack adjustment to the epilogue
 
-  /* Mark the store volatile, so no optimization is permitted to remove it.  */
-  MEM_VOLATILE_P (tmp) = true;
-  return tmp;
+     AARCH64_EH_RETURN_HANDLER_RTX
+     AARCH64_EH_RETURN_STACKADJ_RTX
+
+   and set EH_RETURN_STACKADJ_RTX to 1 in the EH return path so it is a
+   flag that the epilogue can use to distinguish normal and EH returns.
+   This allows different return instructions in the two cases.  The return
+   address is not modified for EH returns.  */
+void
+aarch64_eh_return (rtx handler)
+{
+  emit_move_insn (AARCH64_EH_RETURN_HANDLER_RTX, handler);
+  emit_move_insn (AARCH64_EH_RETURN_STACKADJ_RTX, EH_RETURN_STACKADJ_RTX);
+  emit_move_insn (EH_RETURN_STACKADJ_RTX, const1_rtx);
 }
 
 /* Output code to add DELTA to the first argument, and then jump
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index c783cb96c48..fa68ef0057a 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -583,9 +583,16 @@ 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.  */
+/* 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_STACKADJ_RTX	gen_rtx_REG (Pmode, R4_REGNUM)
-#define EH_RETURN_HANDLER_RTX  aarch64_eh_return_handler_rtx ()
+#define AARCH64_EH_RETURN_STACKADJ_REGNUM	R5_REGNUM
+#define AARCH64_EH_RETURN_STACKADJ_RTX	\
+  gen_rtx_REG (Pmode, AARCH64_EH_RETURN_STACKADJ_REGNUM)
+#define AARCH64_EH_RETURN_HANDLER_REGNUM	R6_REGNUM
+#define AARCH64_EH_RETURN_HANDLER_RTX	\
+  gen_rtx_REG (Pmode, AARCH64_EH_RETURN_HANDLER_REGNUM)
 
 #undef TARGET_COMPUTE_FRAME_LAYOUT
 #define TARGET_COMPUTE_FRAME_LAYOUT aarch64_layout_frame
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 01cf989641f..0a3474776f0 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -877,6 +877,14 @@ (define_expand "sibcall_epilogue"
   "
 )
 
+(define_expand "eh_return"
+  [(use (match_operand 0 "general_operand"))]
+  ""
+{
+  aarch64_eh_return (operands[0]);
+  DONE;
+})
+
 (define_insn "*do_return"
   [(return)]
   ""
-- 
2.25.1

[PATCH 04/11] 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.
---
 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 36cd172d182..afdbf4213c1 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -8417,8 +8417,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

[PATCH 05/11] 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.
---
 gcc/testsuite/gcc.target/aarch64/eh_return-2.c |  9 +++++++++
 gcc/testsuite/gcc.target/aarch64/eh_return-3.c | 14 ++++++++++++++
 2 files changed, 23 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..35989eee806
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/eh_return-3.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mbranch-protection=pac-ret+leaf" } */
+/* { dg-final { scan-assembler "add\tsp, sp, x5" } } */
+/* { dg-final { scan-assembler "br\tx6" } } */
+/* { dg-final { scan-assembler "hint\t25 // paciasp" } } */
+/* { dg-final { scan-assembler "hint\t29 // autiasp" } } */
+
+void
+foo (unsigned long off, void *handler, int c)
+{
+  if (c)
+    return;
+  __builtin_eh_return (off, handler);
+}
-- 
2.25.1

[PATCH 06/11] aarch64: Fix pac-ret eh_return tests

[Szabolcs Nagy]

This is needed since eh_return no longer prevents pac-ret in the
normal return path.

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.
---
 .../gcc.target/aarch64/return_address_sign_1.c  | 13 +------------
 .../gcc.target/aarch64/return_address_sign_2.c  | 17 +++++++++++++++--
 .../aarch64/return_address_sign_b_1.c           | 11 -----------
 .../aarch64/return_address_sign_b_2.c           | 17 +++++++++++++++--
 4 files changed, 31 insertions(+), 27 deletions(-)

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..748924c72f3 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)
+{
+  /* paciasp */
+  *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

[PATCH 07/11] 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.
---
 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 08/11] 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.
---
 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 afdbf4213c1..7f0a22fae9c 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -322,8 +322,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
@@ -18004,12 +18002,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.  */
 
@@ -18612,7 +18604,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

[PATCH 09/11] 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/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 +-
 5 files changed, 109 insertions(+), 164 deletions(-)

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 7f0a22fae9c..661ac12cacc 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -18539,7 +18539,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
@@ -18913,34 +18914,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)
 	{
-- 
2.25.1

[PATCH 10/11] aarch64: Fix branch-protection error message tests

[Szabolcs Nagy]

Update tests for the new branch-protection parser errors.

gcc/testsuite/ChangeLog:

	* gcc.target/aarch64/branch-protection-attr.c: Update.
	* gcc.target/aarch64/branch-protection-option.c: Update.
---
 gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c   | 6 +++---
 gcc/testsuite/gcc.target/aarch64/branch-protection-option.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

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

[PATCH 11/11] 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.
---
 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 7e8f1babed8..75ef00b60d4 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
 };
 
 #endif
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 661ac12cacc..734980f78ec 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -18517,6 +18517,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 02/11] Handle epilogues that contain jumps

[Richard Biener]

On Tue, Aug 22, 2023 at 12:42 PM Szabolcs Nagy via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> From: Richard Sandiford <richard.sandiford@arm.com>
>
> The prologue/epilogue pass allows the prologue sequence
> to contain jumps.  The sequence is then partitioned into
> basic blocks using find_many_sub_basic_blocks.
>
> This patch treats epilogues in the same way.  It's needed for
> a follow-on aarch64 patch that adds conditional code to both
> the prologue and the epilogue.
>
> Tested on aarch64-linux-gnu (including with a follow-on patch)
> and x86_64-linux-gnu.  OK to install?
>
> Richard
>
> gcc/
>         * function.cc (thread_prologue_and_epilogue_insns): Handle
>         epilogues that contain jumps.
> ---
>
> This is a previously approved patch that was not committed
> because it was not needed at the time, but i'd like to commit
> it as it is needed for the followup aarch64 eh_return changes:
>
> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/605769.html
>
> ---
>  gcc/function.cc | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/gcc/function.cc b/gcc/function.cc
> index dd2c1136e07..70d1cd65303 100644
> --- a/gcc/function.cc
> +++ b/gcc/function.cc
> @@ -6120,6 +6120,11 @@ thread_prologue_and_epilogue_insns (void)
>                   && returnjump_p (BB_END (e->src)))
>                 e->flags &= ~EDGE_FALLTHRU;
>             }
> +
> +         auto_sbitmap blocks (last_basic_block_for_fn (cfun));
> +         bitmap_clear (blocks);
> +           bitmap_set_bit (blocks, BLOCK_FOR_INSN (epilogue_seq)->index);
> +         find_many_sub_basic_blocks (blocks);
>         }
>        else if (next_active_insn (BB_END (exit_fallthru_edge->src)))
>         {
> @@ -6218,6 +6223,11 @@ thread_prologue_and_epilogue_insns (void)
>           set_insn_locations (seq, epilogue_location);
>
>           emit_insn_before (seq, insn);
> +
> +         auto_sbitmap blocks (last_basic_block_for_fn (cfun));
> +         bitmap_clear (blocks);
> +         bitmap_set_bit (blocks, BLOCK_FOR_INSN (insn)->index);
> +         find_many_sub_basic_blocks (blocks);

I'll note that clearing a full sbitmap to pass down a single basic block
to find_many_sub_basic_blocks is a quite expensive operation.  May I suggest
to add an overload operating on a single basic block?  It's only

  FOR_EACH_BB_FN (bb, cfun)
    SET_STATE (bb,
               bitmap_bit_p (blocks, bb->index) ? BLOCK_TO_SPLIT :
BLOCK_ORIGINAL);

using the bitmap, so factoring the rest of the function and customizing this
walk would do the trick.  Note that the whole function could be refactored to
handle single blocks more efficiently.

>         }
>      }
>
> --
> 2.25.1
>

Re: [PATCH 03/11] 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 using the EH_RETURN_STACKADJ_RTX
> register only as a flag that is set to 1 in the eh_return paths
> (it is 0 in normal return paths) and introduces
>
>   AARCH64_EH_RETURN_STACKADJ_RTX
>   AARCH64_EH_RETURN_HANDLER_RTX
>
> to pass the actual stack adjustment and landing pad address to the
> epilogue in the eh_return case. Then the epilogue can use the right
> return sequence based on the EH_RETURN_STACKADJ_RTX flag.
>
> 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.

I don't think there's any specific target-independent requirement for
EH_RETURN_HANDLER_RTX to be a stack slot.  df-scan.cc has code to handle
registers.

So couldn't we just use EH_RETURN_HANDLER_RTX for this, rather than
making it AARCH64_EH_RETURN_HANDLER_RTX?

> The new code generation for functions with eh_return is not amazing,
> since x5 and x6 is assumed to be used by the epilogue even in the
> normal return path, not just for eh_return.  But only the unwinder
> is expected to use eh_return so this is fine.

I guess the problem here is that x5 and x6 are upwards-exposed on
the non-eh_return paths, and so are treated as live for most of the
function.  Is that right?

The patch seems to be using the existing interfaces to implement
a slightly different model.  E.g. if feels like a hack (but a neat hack)
that EH_RETURN_STACKADJ_RTX is now a flag rather than an adjustment,
with AARCH64_EH_RETURN_STACKADJ_RTX then being the "real" stack
adjustment.  And the reason for the upwards exposure of the new
registers on normal return paths is that the existing model has
no hook into the normal return path.

Rather than hiding this in target code, perhaps we should add a
target-independent concept of an "eh_return taken" flag, say
EH_RETURN_TAKEN_RTX.

We could define it so that, on targets that define EH_RETURN_TAKEN_RTX,
a register EH_RETURN_STACKADJ_RTX and a register EH_RETURN_HANDLER_RTX
are only meaningful when the flag is true.  E.g. we could have:

#ifdef EH_RETURN_HANDLER_RTX
  for (rtx tmp : { EH_RETURN_STACKADJ_RTX, EH_RETURN_HANDLER_RTX })
    if (tmp && REG_P (tmp))
      emit_clobber (tmp);
#endif

in the "normal return" part of expand_eh_return.  (If some other target
wants a flag with different semantics, it'd be up to them to add it.)

That should avoid most of the bad code-quality effects, since the
specialness of x4-x6 will be confined to the code immediately before
the pre-epilogue exit edges.

Thanks,
Richard

> 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.
>
> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64-protos.h (aarch64_eh_return_handler_rtx):
> 	Remove.
> 	(aarch64_eh_return): New.
> 	* config/aarch64/aarch64.cc (aarch64_return_address_signing_enabled):
> 	Sign return address even in functions with eh_return.
> 	(aarch64_epilogue_uses): Mark two registers as used.
> 	(aarch64_expand_epilogue): Conditionally return with br or ret.
> 	(aarch64_eh_return_handler_rtx): Remove.
> 	(aarch64_eh_return): New.
> 	* config/aarch64/aarch64.h (EH_RETURN_HANDLER_RTX): Remove.
> 	(AARCH64_EH_RETURN_STACKADJ_REGNUM): Define.
> 	(AARCH64_EH_RETURN_STACKADJ_RTX): Define.
> 	(AARCH64_EH_RETURN_HANDLER_REGNUM): Define.
> 	(AARCH64_EH_RETURN_HANDLER_RTX): Define.
> 	* config/aarch64/aarch64.md (eh_return): New.
> ---
>  gcc/config/aarch64/aarch64-protos.h |   2 +-
>  gcc/config/aarch64/aarch64.cc       | 106 +++++++++++++++-------------
>  gcc/config/aarch64/aarch64.h        |  11 ++-
>  gcc/config/aarch64/aarch64.md       |   8 +++
>  4 files changed, 73 insertions(+), 54 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
> index 70303d6fd95..5d1834162a4 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -855,7 +855,7 @@ 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);
> +void aarch64_eh_return (rtx);
>  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 eba5d4a7e04..36cd172d182 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -8972,17 +8972,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
> @@ -9932,9 +9921,8 @@ aarch64_allocate_and_probe_stack_space (rtx temp1, rtx temp2,
>     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.  */
> +   For EH return we need to keep two registers alive for stack adjustment
> +   and return address.  */
>  
>  int
>  aarch64_epilogue_uses (int regno)
> @@ -9944,6 +9932,13 @@ aarch64_epilogue_uses (int regno)
>        if (regno == LR_REGNUM)
>  	return 1;
>      }
> +
> +  if (!epilogue_completed && crtl->calls_eh_return)
> +    {
> +      if (regno == AARCH64_EH_RETURN_STACKADJ_REGNUM
> +	  || regno == AARCH64_EH_RETURN_HANDLER_REGNUM)
> +	return 1;
> +    }
>    return 0;
>  }
>  
> @@ -10342,6 +10337,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_STACKADJ_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_STACKADJ_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,
> +				AARCH64_EH_RETURN_STACKADJ_RTX));
> +      emit_jump_insn (gen_indirect_jump (AARCH64_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.
> @@ -10371,56 +10390,41 @@ 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.
> +/* Implement the eh_return instruction pattern.  Functions with EH returns
> +   either return normally or return to a previous frame after unwinding.
>  
> -   An EH return uses a single shared return sequence.  The epilogue is
> +   The two cases use 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));
> +   An actual EH return initializes the stack adjustment then invokes this
> +   target hook (which supposed to overwrite the return address) and then
> +   jumps directly into the epilogue (bypassing the zeroing of the adjustment).
> +
> +   We depart from the intended EH return logic by using two additional
> +   registers to pass the handler and stack adjustment to the epilogue
>  
> -  /* Mark the store volatile, so no optimization is permitted to remove it.  */
> -  MEM_VOLATILE_P (tmp) = true;
> -  return tmp;
> +     AARCH64_EH_RETURN_HANDLER_RTX
> +     AARCH64_EH_RETURN_STACKADJ_RTX
> +
> +   and set EH_RETURN_STACKADJ_RTX to 1 in the EH return path so it is a
> +   flag that the epilogue can use to distinguish normal and EH returns.
> +   This allows different return instructions in the two cases.  The return
> +   address is not modified for EH returns.  */
> +void
> +aarch64_eh_return (rtx handler)
> +{
> +  emit_move_insn (AARCH64_EH_RETURN_HANDLER_RTX, handler);
> +  emit_move_insn (AARCH64_EH_RETURN_STACKADJ_RTX, EH_RETURN_STACKADJ_RTX);
> +  emit_move_insn (EH_RETURN_STACKADJ_RTX, const1_rtx);
>  }
>  
>  /* Output code to add DELTA to the first argument, and then jump
> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
> index c783cb96c48..fa68ef0057a 100644
> --- a/gcc/config/aarch64/aarch64.h
> +++ b/gcc/config/aarch64/aarch64.h
> @@ -583,9 +583,16 @@ 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.  */
> +/* 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_STACKADJ_RTX	gen_rtx_REG (Pmode, R4_REGNUM)
> -#define EH_RETURN_HANDLER_RTX  aarch64_eh_return_handler_rtx ()
> +#define AARCH64_EH_RETURN_STACKADJ_REGNUM	R5_REGNUM
> +#define AARCH64_EH_RETURN_STACKADJ_RTX	\
> +  gen_rtx_REG (Pmode, AARCH64_EH_RETURN_STACKADJ_REGNUM)
> +#define AARCH64_EH_RETURN_HANDLER_REGNUM	R6_REGNUM
> +#define AARCH64_EH_RETURN_HANDLER_RTX	\
> +  gen_rtx_REG (Pmode, AARCH64_EH_RETURN_HANDLER_REGNUM)
>  
>  #undef TARGET_COMPUTE_FRAME_LAYOUT
>  #define TARGET_COMPUTE_FRAME_LAYOUT aarch64_layout_frame
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 01cf989641f..0a3474776f0 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -877,6 +877,14 @@ (define_expand "sibcall_epilogue"
>    "
>  )
>  
> +(define_expand "eh_return"
> +  [(use (match_operand 0 "general_operand"))]
> +  ""
> +{
> +  aarch64_eh_return (operands[0]);
> +  DONE;
> +})
> +
>  (define_insn "*do_return"
>    [(return)]
>    ""

Re: [PATCH 03/11] aarch64: Use br instead of ret for eh_return

[Richard Sandiford]

Richard Sandiford <richard.sandiford@arm.com> writes:
> Rather than hiding this in target code, perhaps we should add a
> target-independent concept of an "eh_return taken" flag, say
> EH_RETURN_TAKEN_RTX.
>
> We could define it so that, on targets that define EH_RETURN_TAKEN_RTX,
> a register EH_RETURN_STACKADJ_RTX and a register EH_RETURN_HANDLER_RTX
> are only meaningful when the flag is true.  E.g. we could have:
>
> #ifdef EH_RETURN_HANDLER_RTX

Gah, I meant #ifdef EH_RETURN_TAKEN_RTX here

>   for (rtx tmp : { EH_RETURN_STACKADJ_RTX, EH_RETURN_HANDLER_RTX })
>     if (tmp && REG_P (tmp))
>       emit_clobber (tmp);
> #endif
>
> in the "normal return" part of expand_eh_return.  (If some other target
> wants a flag with different semantics, it'd be up to them to add it.)
>
> That should avoid most of the bad code-quality effects, since the
> specialness of x4-x6 will be confined to the code immediately before
> the pre-epilogue exit edges.
>
> Thanks,
> Richard

Re: [PATCH 01/11] aarch64: AARCH64_ISA_RCPC was defined twice

[Richard Sandiford]

Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64.h (AARCH64_ISA_RCPC): Remove dup.

OK, thanks.

Richard

> ---
>  gcc/config/aarch64/aarch64.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
> index 2b0fc97bb71..c783cb96c48 100644
> --- a/gcc/config/aarch64/aarch64.h
> +++ b/gcc/config/aarch64/aarch64.h
> @@ -222,7 +222,6 @@ enum class aarch64_feature : unsigned char {
>  #define AARCH64_ISA_MOPS	   (aarch64_isa_flags & AARCH64_FL_MOPS)
>  #define AARCH64_ISA_LS64	   (aarch64_isa_flags & AARCH64_FL_LS64)
>  #define AARCH64_ISA_CSSC	   (aarch64_isa_flags & AARCH64_FL_CSSC)
> -#define AARCH64_ISA_RCPC           (aarch64_isa_flags & AARCH64_FL_RCPC)
>  
>  /* Crypto is an optional extension to AdvSIMD.  */
>  #define TARGET_CRYPTO (AARCH64_ISA_CRYPTO)

Re: [PATCH 04/11] 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 once we've agreed on something for 03/11.

Thanks,
Richard

> ---
>  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 36cd172d182..afdbf4213c1 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -8417,8 +8417,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.  */

Re: [PATCH 05/11] 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.

OK.

I wonder if it's worth using check-function-bodies for -3.c though.
It would then be easy to verify that the autiasp only occurs on the
normal return path.

Just a suggestion -- the current test is fine too.

Thanks,
Richard

> ---
>  gcc/testsuite/gcc.target/aarch64/eh_return-2.c |  9 +++++++++
>  gcc/testsuite/gcc.target/aarch64/eh_return-3.c | 14 ++++++++++++++
>  2 files changed, 23 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..35989eee806
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/eh_return-3.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mbranch-protection=pac-ret+leaf" } */
> +/* { dg-final { scan-assembler "add\tsp, sp, x5" } } */
> +/* { dg-final { scan-assembler "br\tx6" } } */
> +/* { dg-final { scan-assembler "hint\t25 // paciasp" } } */
> +/* { dg-final { scan-assembler "hint\t29 // autiasp" } } */
> +
> +void
> +foo (unsigned long off, void *handler, int c)
> +{
> +  if (c)
> +    return;
> +  __builtin_eh_return (off, handler);
> +}

Re: [PATCH 06/11] aarch64: Fix pac-ret eh_return tests

[Richard Sandiford]

Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> This is needed since eh_return no longer prevents pac-ret in the
> normal return path.
>
> 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.
> ---
>  .../gcc.target/aarch64/return_address_sign_1.c  | 13 +------------
>  .../gcc.target/aarch64/return_address_sign_2.c  | 17 +++++++++++++++--
>  .../aarch64/return_address_sign_b_1.c           | 11 -----------
>  .../aarch64/return_address_sign_b_2.c           | 17 +++++++++++++++--
>  4 files changed, 31 insertions(+), 27 deletions(-)
>
> 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 } } */

I suppose there is no normal return path here.  I don't know how quickly
we'd realise that though, in the sense that the flag register becomes known-1.
But a quick-and-dirty check would be whether the exit block has a single
predecessor, which in a function that calls eh_return should mean
that the eh_return is unconditional.

But that might not be worth worrying about, given the builtin's limited
use case.  And even if it is worth worrying about, keeping the test in
this file would mix correctness with optimisation, which isn't a good
thing for scan-assembler-times.

So yeah, I agree this is OK.  It should probably be part of 03 though,
so that no individual commit causes a regression.

Thanks,
Richard

> 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..748924c72f3 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)
> +{
> +  /* paciasp */
> +  *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 } } */

Re: [PATCH 07/11] aarch64: Disable branch-protection for pcs tests

[Richard Sandiford]

Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> 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.

OK, thanks.

Richard

> ---
>  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

Re: [PATCH 10/11] aarch64: Fix branch-protection error message tests

[Richard Sandiford]

Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> Update tests for the new branch-protection parser errors.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/aarch64/branch-protection-attr.c: Update.
> 	* gcc.target/aarch64/branch-protection-option.c: Update.

OK, thanks.  (And I agree these are better messages. :))

I think that's the last of the AArch64-specific ones.  The others
will need to be reviewed by Kyrill or Richard.

Richard

> ---
>  gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c   | 6 +++---
>  gcc/testsuite/gcc.target/aarch64/branch-protection-option.c | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> 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 } */

Re: [PATCH 02/11] Handle epilogues that contain jumps

[Richard Sandiford]

Richard Biener <richard.guenther@gmail.com> writes:
> On Tue, Aug 22, 2023 at 12:42 PM Szabolcs Nagy via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> From: Richard Sandiford <richard.sandiford@arm.com>
>>
>> The prologue/epilogue pass allows the prologue sequence
>> to contain jumps.  The sequence is then partitioned into
>> basic blocks using find_many_sub_basic_blocks.
>>
>> This patch treats epilogues in the same way.  It's needed for
>> a follow-on aarch64 patch that adds conditional code to both
>> the prologue and the epilogue.
>>
>> Tested on aarch64-linux-gnu (including with a follow-on patch)
>> and x86_64-linux-gnu.  OK to install?
>>
>> Richard
>>
>> gcc/
>>         * function.cc (thread_prologue_and_epilogue_insns): Handle
>>         epilogues that contain jumps.
>> ---
>>
>> This is a previously approved patch that was not committed
>> because it was not needed at the time, but i'd like to commit
>> it as it is needed for the followup aarch64 eh_return changes:
>>
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/605769.html
>>
>> ---
>>  gcc/function.cc | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/gcc/function.cc b/gcc/function.cc
>> index dd2c1136e07..70d1cd65303 100644
>> --- a/gcc/function.cc
>> +++ b/gcc/function.cc
>> @@ -6120,6 +6120,11 @@ thread_prologue_and_epilogue_insns (void)
>>                   && returnjump_p (BB_END (e->src)))
>>                 e->flags &= ~EDGE_FALLTHRU;
>>             }
>> +
>> +         auto_sbitmap blocks (last_basic_block_for_fn (cfun));
>> +         bitmap_clear (blocks);
>> +           bitmap_set_bit (blocks, BLOCK_FOR_INSN (epilogue_seq)->index);
>> +         find_many_sub_basic_blocks (blocks);
>>         }
>>        else if (next_active_insn (BB_END (exit_fallthru_edge->src)))
>>         {
>> @@ -6218,6 +6223,11 @@ thread_prologue_and_epilogue_insns (void)
>>           set_insn_locations (seq, epilogue_location);
>>
>>           emit_insn_before (seq, insn);
>> +
>> +         auto_sbitmap blocks (last_basic_block_for_fn (cfun));
>> +         bitmap_clear (blocks);
>> +         bitmap_set_bit (blocks, BLOCK_FOR_INSN (insn)->index);
>> +         find_many_sub_basic_blocks (blocks);
>
> I'll note that clearing a full sbitmap to pass down a single basic block
> to find_many_sub_basic_blocks is a quite expensive operation.  May I suggest
> to add an overload operating on a single basic block?  It's only
>
>   FOR_EACH_BB_FN (bb, cfun)
>     SET_STATE (bb,
>                bitmap_bit_p (blocks, bb->index) ? BLOCK_TO_SPLIT :
> BLOCK_ORIGINAL);
>
> using the bitmap, so factoring the rest of the function and customizing this
> walk would do the trick.  Note that the whole function could be refactored to
> handle single blocks more efficiently.

Sorry for the late reply, but does this look OK?  Tested on
aarch64-linux-gnu and x86_64-linux-gnu.

Thanks,
Richard

---

The prologue/epilogue pass allows the prologue sequence to contain
jumps.  The sequence is then partitioned into basic blocks using
find_many_sub_basic_blocks.

This patch treats epilogues in a similar way.  Since only one block
might need to be split, the patch (re)introduces a find_sub_basic_blocks
routine to handle a single block.

The new routine hard-codes the assumption that split_block will chain
the new block immediately after the original block.  The routine doesn't
try to replicate the fix for PR81030, since that was specific to
gimple->rtl expansion.

The patch is needed for follow-on aarch64 patches that add conditional
code to the epilogue.  The tests are part of those patches.

gcc/
	* cfgbuild.h (find_sub_basic_blocks): Declare.
	* cfgbuild.cc (update_profile_for_new_sub_basic_block): New function,
	split out from...
	(find_many_sub_basic_blocks): ...here.
	(find_sub_basic_blocks): New function.
	* function.cc (thread_prologue_and_epilogue_insns): Handle
	epilogues that contain jumps.
---
 gcc/cfgbuild.cc | 95 +++++++++++++++++++++++++++++++++----------------
 gcc/cfgbuild.h  |  1 +
 gcc/function.cc |  4 +++
 3 files changed, 70 insertions(+), 30 deletions(-)

diff --git a/gcc/cfgbuild.cc b/gcc/cfgbuild.cc
index 15ed4deb5f7..9a6b34fb4b1 100644
--- a/gcc/cfgbuild.cc
+++ b/gcc/cfgbuild.cc
@@ -693,6 +693,43 @@ compute_outgoing_frequencies (basic_block b)
     }
 }
 
+/* Update the profile information for BB, which was created by splitting
+   an RTL block that had a non-final jump.  */
+
+static void
+update_profile_for_new_sub_basic_block (basic_block bb)
+{
+  edge e;
+  edge_iterator ei;
+
+  bool initialized_src = false, uninitialized_src = false;
+  bb->count = profile_count::zero ();
+  FOR_EACH_EDGE (e, ei, bb->preds)
+    {
+      if (e->count ().initialized_p ())
+	{
+	  bb->count += e->count ();
+	  initialized_src = true;
+	}
+      else
+	uninitialized_src = true;
+    }
+  /* When some edges are missing with read profile, this is
+     most likely because RTL expansion introduced loop.
+     When profile is guessed we may have BB that is reachable
+     from unlikely path as well as from normal path.
+
+     TODO: We should handle loops created during BB expansion
+     correctly here.  For now we assume all those loop to cycle
+     precisely once.  */
+  if (!initialized_src
+      || (uninitialized_src
+	   && profile_status_for_fn (cfun) < PROFILE_GUESSED))
+    bb->count = profile_count::uninitialized ();
+
+  compute_outgoing_frequencies (bb);
+}
+
 /* Assume that some pass has inserted labels or control flow
    instructions within a basic block.  Split basic blocks as needed
    and create edges.  */
@@ -744,40 +781,15 @@ find_many_sub_basic_blocks (sbitmap blocks)
   if (profile_status_for_fn (cfun) != PROFILE_ABSENT)
     FOR_BB_BETWEEN (bb, min, max->next_bb, next_bb)
       {
-	edge e;
-	edge_iterator ei;
-
 	if (STATE (bb) == BLOCK_ORIGINAL)
 	  continue;
 	if (STATE (bb) == BLOCK_NEW)
 	  {
-	    bool initialized_src = false, uninitialized_src = false;
-	    bb->count = profile_count::zero ();
-	    FOR_EACH_EDGE (e, ei, bb->preds)
-	      {
-		if (e->count ().initialized_p ())
-		  {
-		    bb->count += e->count ();
-		    initialized_src = true;
-		  }
-		else
-		  uninitialized_src = true;
-	      }
-	    /* When some edges are missing with read profile, this is
-	       most likely because RTL expansion introduced loop.
-	       When profile is guessed we may have BB that is reachable
-	       from unlikely path as well as from normal path.
-
-	       TODO: We should handle loops created during BB expansion
-	       correctly here.  For now we assume all those loop to cycle
-	       precisely once.  */
-	    if (!initialized_src
-		|| (uninitialized_src
-		     && profile_status_for_fn (cfun) < PROFILE_GUESSED))
-	      bb->count = profile_count::uninitialized ();
+	    update_profile_for_new_sub_basic_block (bb);
+	    continue;
 	  }
- 	/* If nothing changed, there is no need to create new BBs.  */
-	else if (EDGE_COUNT (bb->succs) == n_succs[bb->index])
+	/* If nothing changed, there is no need to create new BBs.  */
+	if (EDGE_COUNT (bb->succs) == n_succs[bb->index])
 	  {
 	    /* In rare occassions RTL expansion might have mistakely assigned
 	       a probabilities different from what is in CFG.  This happens
@@ -788,10 +800,33 @@ find_many_sub_basic_blocks (sbitmap blocks)
 	      update_br_prob_note (bb);
 	    continue;
 	  }
-
 	compute_outgoing_frequencies (bb);
       }
 
   FOR_EACH_BB_FN (bb, cfun)
     SET_STATE (bb, 0);
 }
+
+/* Like find_many_sub_basic_blocks, but look only within BB.  */
+
+void
+find_sub_basic_blocks (basic_block bb)
+{
+  basic_block end_bb = bb->next_bb;
+  find_bb_boundaries (bb);
+  if (bb->next_bb == end_bb)
+    return;
+
+  /* Re-scan and wire in all edges.  This expects simple (conditional)
+     jumps at the end of each new basic blocks.  */
+  make_edges (bb, end_bb->prev_bb, 1);
+
+  /* Update branch probabilities.  Expect only (un)conditional jumps
+     to be created with only the forward edges.  */
+  if (profile_status_for_fn (cfun) != PROFILE_ABSENT)
+    {
+      compute_outgoing_frequencies (bb);
+      for (bb = bb->next_bb; bb != end_bb; bb = bb->next_bb)
+	update_profile_for_new_sub_basic_block (bb);
+    }
+}
diff --git a/gcc/cfgbuild.h b/gcc/cfgbuild.h
index 51d3eccb1ae..4191fb3fcba 100644
--- a/gcc/cfgbuild.h
+++ b/gcc/cfgbuild.h
@@ -24,5 +24,6 @@ extern bool inside_basic_block_p (const rtx_insn *);
 extern bool control_flow_insn_p (const rtx_insn *);
 extern void rtl_make_eh_edge (sbitmap, basic_block, rtx);
 extern void find_many_sub_basic_blocks (sbitmap);
+extern void find_sub_basic_blocks (basic_block);
 
 #endif /* GCC_CFGBUILD_H */
diff --git a/gcc/function.cc b/gcc/function.cc
index 336af28fb22..afb0b33da9e 100644
--- a/gcc/function.cc
+++ b/gcc/function.cc
@@ -6112,6 +6112,8 @@ thread_prologue_and_epilogue_insns (void)
 		  && returnjump_p (BB_END (e->src)))
 		e->flags &= ~EDGE_FALLTHRU;
 	    }
+
+	  find_sub_basic_blocks (BLOCK_FOR_INSN (epilogue_seq));
 	}
       else if (next_active_insn (BB_END (exit_fallthru_edge->src)))
 	{
@@ -6210,6 +6212,8 @@ thread_prologue_and_epilogue_insns (void)
 	  set_insn_locations (seq, epilogue_location);
 
 	  emit_insn_before (seq, insn);
+
+	  find_sub_basic_blocks (BLOCK_FOR_INSN (insn));
 	}
     }
 
-- 
2.25.1

Re: [PATCH 10/11] aarch64: Fix branch-protection error message tests

[Richard Earnshaw (lists)]

On 05/09/2023 16:00, Richard Sandiford via Gcc-patches wrote:
> Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
>> Update tests for the new branch-protection parser errors.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	* gcc.target/aarch64/branch-protection-attr.c: Update.
>> 	* gcc.target/aarch64/branch-protection-option.c: Update.
> 
> OK, thanks.  (And I agree these are better messages. :))
> 
> I think that's the last of the AArch64-specific ones.  The others
> will need to be reviewed by Kyrill or Richard.
> 
> Richard
> 
>> ---
>>  gcc/testsuite/gcc.target/aarch64/branch-protection-attr.c   | 6 +++---
>>  gcc/testsuite/gcc.target/aarch64/branch-protection-option.c | 2 +-
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> 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 } */

'leaf' is really a modifier for the other branch protection strategies; perhaps it would be better to describe it as that.

But this brings up another issue/question.  If the compiler has been configured with, say, '--enable-branch-protection=standard' or some other variety, is there (or do we want) a way to extend that to leaf functions without changing the underlying strategy?

>>  
>>  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 } */

Or maybe better still: "branch protection strategies 'none' and 'pac-ret' are incompatible".

>>  /* { 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 } */

But this is all a matter of taste.

However, this patch should be merged with the patch that changes the error messages.  Or has that already gone in?

R

Re: [PATCH 02/11] Handle epilogues that contain jumps

[Richard Biener]

On Thu, Oct 12, 2023 at 10:15 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Richard Biener <richard.guenther@gmail.com> writes:
> > On Tue, Aug 22, 2023 at 12:42 PM Szabolcs Nagy via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> From: Richard Sandiford <richard.sandiford@arm.com>
> >>
> >> The prologue/epilogue pass allows the prologue sequence
> >> to contain jumps.  The sequence is then partitioned into
> >> basic blocks using find_many_sub_basic_blocks.
> >>
> >> This patch treats epilogues in the same way.  It's needed for
> >> a follow-on aarch64 patch that adds conditional code to both
> >> the prologue and the epilogue.
> >>
> >> Tested on aarch64-linux-gnu (including with a follow-on patch)
> >> and x86_64-linux-gnu.  OK to install?
> >>
> >> Richard
> >>
> >> gcc/
> >>         * function.cc (thread_prologue_and_epilogue_insns): Handle
> >>         epilogues that contain jumps.
> >> ---
> >>
> >> This is a previously approved patch that was not committed
> >> because it was not needed at the time, but i'd like to commit
> >> it as it is needed for the followup aarch64 eh_return changes:
> >>
> >> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/605769.html
> >>
> >> ---
> >>  gcc/function.cc | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/gcc/function.cc b/gcc/function.cc
> >> index dd2c1136e07..70d1cd65303 100644
> >> --- a/gcc/function.cc
> >> +++ b/gcc/function.cc
> >> @@ -6120,6 +6120,11 @@ thread_prologue_and_epilogue_insns (void)
> >>                   && returnjump_p (BB_END (e->src)))
> >>                 e->flags &= ~EDGE_FALLTHRU;
> >>             }
> >> +
> >> +         auto_sbitmap blocks (last_basic_block_for_fn (cfun));
> >> +         bitmap_clear (blocks);
> >> +           bitmap_set_bit (blocks, BLOCK_FOR_INSN (epilogue_seq)->index);
> >> +         find_many_sub_basic_blocks (blocks);
> >>         }
> >>        else if (next_active_insn (BB_END (exit_fallthru_edge->src)))
> >>         {
> >> @@ -6218,6 +6223,11 @@ thread_prologue_and_epilogue_insns (void)
> >>           set_insn_locations (seq, epilogue_location);
> >>
> >>           emit_insn_before (seq, insn);
> >> +
> >> +         auto_sbitmap blocks (last_basic_block_for_fn (cfun));
> >> +         bitmap_clear (blocks);
> >> +         bitmap_set_bit (blocks, BLOCK_FOR_INSN (insn)->index);
> >> +         find_many_sub_basic_blocks (blocks);
> >
> > I'll note that clearing a full sbitmap to pass down a single basic block
> > to find_many_sub_basic_blocks is a quite expensive operation.  May I suggest
> > to add an overload operating on a single basic block?  It's only
> >
> >   FOR_EACH_BB_FN (bb, cfun)
> >     SET_STATE (bb,
> >                bitmap_bit_p (blocks, bb->index) ? BLOCK_TO_SPLIT :
> > BLOCK_ORIGINAL);
> >
> > using the bitmap, so factoring the rest of the function and customizing this
> > walk would do the trick.  Note that the whole function could be refactored to
> > handle single blocks more efficiently.
>
> Sorry for the late reply, but does this look OK?  Tested on
> aarch64-linux-gnu and x86_64-linux-gnu.

LGTM, not sure if I'm qualified enough to approve though (I think you
are more qualified here, so ..)

Thanks,
Richard.

> Thanks,
> Richard
>
> ---
>
> The prologue/epilogue pass allows the prologue sequence to contain
> jumps.  The sequence is then partitioned into basic blocks using
> find_many_sub_basic_blocks.
>
> This patch treats epilogues in a similar way.  Since only one block
> might need to be split, the patch (re)introduces a find_sub_basic_blocks
> routine to handle a single block.
>
> The new routine hard-codes the assumption that split_block will chain
> the new block immediately after the original block.  The routine doesn't
> try to replicate the fix for PR81030, since that was specific to
> gimple->rtl expansion.
>
> The patch is needed for follow-on aarch64 patches that add conditional
> code to the epilogue.  The tests are part of those patches.
>
> gcc/
>         * cfgbuild.h (find_sub_basic_blocks): Declare.
>         * cfgbuild.cc (update_profile_for_new_sub_basic_block): New function,
>         split out from...
>         (find_many_sub_basic_blocks): ...here.
>         (find_sub_basic_blocks): New function.
>         * function.cc (thread_prologue_and_epilogue_insns): Handle
>         epilogues that contain jumps.
> ---
>  gcc/cfgbuild.cc | 95 +++++++++++++++++++++++++++++++++----------------
>  gcc/cfgbuild.h  |  1 +
>  gcc/function.cc |  4 +++
>  3 files changed, 70 insertions(+), 30 deletions(-)
>
> diff --git a/gcc/cfgbuild.cc b/gcc/cfgbuild.cc
> index 15ed4deb5f7..9a6b34fb4b1 100644
> --- a/gcc/cfgbuild.cc
> +++ b/gcc/cfgbuild.cc
> @@ -693,6 +693,43 @@ compute_outgoing_frequencies (basic_block b)
>      }
>  }
>
> +/* Update the profile information for BB, which was created by splitting
> +   an RTL block that had a non-final jump.  */
> +
> +static void
> +update_profile_for_new_sub_basic_block (basic_block bb)
> +{
> +  edge e;
> +  edge_iterator ei;
> +
> +  bool initialized_src = false, uninitialized_src = false;
> +  bb->count = profile_count::zero ();
> +  FOR_EACH_EDGE (e, ei, bb->preds)
> +    {
> +      if (e->count ().initialized_p ())
> +       {
> +         bb->count += e->count ();
> +         initialized_src = true;
> +       }
> +      else
> +       uninitialized_src = true;
> +    }
> +  /* When some edges are missing with read profile, this is
> +     most likely because RTL expansion introduced loop.
> +     When profile is guessed we may have BB that is reachable
> +     from unlikely path as well as from normal path.
> +
> +     TODO: We should handle loops created during BB expansion
> +     correctly here.  For now we assume all those loop to cycle
> +     precisely once.  */
> +  if (!initialized_src
> +      || (uninitialized_src
> +          && profile_status_for_fn (cfun) < PROFILE_GUESSED))
> +    bb->count = profile_count::uninitialized ();
> +
> +  compute_outgoing_frequencies (bb);
> +}
> +
>  /* Assume that some pass has inserted labels or control flow
>     instructions within a basic block.  Split basic blocks as needed
>     and create edges.  */
> @@ -744,40 +781,15 @@ find_many_sub_basic_blocks (sbitmap blocks)
>    if (profile_status_for_fn (cfun) != PROFILE_ABSENT)
>      FOR_BB_BETWEEN (bb, min, max->next_bb, next_bb)
>        {
> -       edge e;
> -       edge_iterator ei;
> -
>         if (STATE (bb) == BLOCK_ORIGINAL)
>           continue;
>         if (STATE (bb) == BLOCK_NEW)
>           {
> -           bool initialized_src = false, uninitialized_src = false;
> -           bb->count = profile_count::zero ();
> -           FOR_EACH_EDGE (e, ei, bb->preds)
> -             {
> -               if (e->count ().initialized_p ())
> -                 {
> -                   bb->count += e->count ();
> -                   initialized_src = true;
> -                 }
> -               else
> -                 uninitialized_src = true;
> -             }
> -           /* When some edges are missing with read profile, this is
> -              most likely because RTL expansion introduced loop.
> -              When profile is guessed we may have BB that is reachable
> -              from unlikely path as well as from normal path.
> -
> -              TODO: We should handle loops created during BB expansion
> -              correctly here.  For now we assume all those loop to cycle
> -              precisely once.  */
> -           if (!initialized_src
> -               || (uninitialized_src
> -                    && profile_status_for_fn (cfun) < PROFILE_GUESSED))
> -             bb->count = profile_count::uninitialized ();
> +           update_profile_for_new_sub_basic_block (bb);
> +           continue;
>           }
> -       /* If nothing changed, there is no need to create new BBs.  */
> -       else if (EDGE_COUNT (bb->succs) == n_succs[bb->index])
> +       /* If nothing changed, there is no need to create new BBs.  */
> +       if (EDGE_COUNT (bb->succs) == n_succs[bb->index])
>           {
>             /* In rare occassions RTL expansion might have mistakely assigned
>                a probabilities different from what is in CFG.  This happens
> @@ -788,10 +800,33 @@ find_many_sub_basic_blocks (sbitmap blocks)
>               update_br_prob_note (bb);
>             continue;
>           }
> -
>         compute_outgoing_frequencies (bb);
>        }
>
>    FOR_EACH_BB_FN (bb, cfun)
>      SET_STATE (bb, 0);
>  }
> +
> +/* Like find_many_sub_basic_blocks, but look only within BB.  */
> +
> +void
> +find_sub_basic_blocks (basic_block bb)
> +{
> +  basic_block end_bb = bb->next_bb;
> +  find_bb_boundaries (bb);
> +  if (bb->next_bb == end_bb)
> +    return;
> +
> +  /* Re-scan and wire in all edges.  This expects simple (conditional)
> +     jumps at the end of each new basic blocks.  */
> +  make_edges (bb, end_bb->prev_bb, 1);
> +
> +  /* Update branch probabilities.  Expect only (un)conditional jumps
> +     to be created with only the forward edges.  */
> +  if (profile_status_for_fn (cfun) != PROFILE_ABSENT)
> +    {
> +      compute_outgoing_frequencies (bb);
> +      for (bb = bb->next_bb; bb != end_bb; bb = bb->next_bb)
> +       update_profile_for_new_sub_basic_block (bb);
> +    }
> +}
> diff --git a/gcc/cfgbuild.h b/gcc/cfgbuild.h
> index 51d3eccb1ae..4191fb3fcba 100644
> --- a/gcc/cfgbuild.h
> +++ b/gcc/cfgbuild.h
> @@ -24,5 +24,6 @@ extern bool inside_basic_block_p (const rtx_insn *);
>  extern bool control_flow_insn_p (const rtx_insn *);
>  extern void rtl_make_eh_edge (sbitmap, basic_block, rtx);
>  extern void find_many_sub_basic_blocks (sbitmap);
> +extern void find_sub_basic_blocks (basic_block);
>
>  #endif /* GCC_CFGBUILD_H */
> diff --git a/gcc/function.cc b/gcc/function.cc
> index 336af28fb22..afb0b33da9e 100644
> --- a/gcc/function.cc
> +++ b/gcc/function.cc
> @@ -6112,6 +6112,8 @@ thread_prologue_and_epilogue_insns (void)
>                   && returnjump_p (BB_END (e->src)))
>                 e->flags &= ~EDGE_FALLTHRU;
>             }
> +
> +         find_sub_basic_blocks (BLOCK_FOR_INSN (epilogue_seq));
>         }
>        else if (next_active_insn (BB_END (exit_fallthru_edge->src)))
>         {
> @@ -6210,6 +6212,8 @@ thread_prologue_and_epilogue_insns (void)
>           set_insn_locations (seq, epilogue_location);
>
>           emit_insn_before (seq, insn);
> +
> +         find_sub_basic_blocks (BLOCK_FOR_INSN (insn));
>         }
>      }
>
> --
> 2.25.1
>

Re: [PATCH 02/11] Handle epilogues that contain jumps

[Jeff Law]

On 10/17/23 03:19, Richard Biener wrote:
> On Thu, Oct 12, 2023 at 10:15 AM Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Richard Biener <richard.guenther@gmail.com> writes:
>>> On Tue, Aug 22, 2023 at 12:42 PM Szabolcs Nagy via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>
>>>> From: Richard Sandiford <richard.sandiford@arm.com>
>>>>
>>>> The prologue/epilogue pass allows the prologue sequence
>>>> to contain jumps.  The sequence is then partitioned into
>>>> basic blocks using find_many_sub_basic_blocks.
>>>>
>>>> This patch treats epilogues in the same way.  It's needed for
>>>> a follow-on aarch64 patch that adds conditional code to both
>>>> the prologue and the epilogue.
>>>>
>>>> Tested on aarch64-linux-gnu (including with a follow-on patch)
>>>> and x86_64-linux-gnu.  OK to install?
>>>>
>>>> Richard
>>>>
>>>> gcc/
>>>>          * function.cc (thread_prologue_and_epilogue_insns): Handle
>>>>          epilogues that contain jumps.
>>>> ---
>>>>
>>>> This is a previously approved patch that was not committed
>>>> because it was not needed at the time, but i'd like to commit
>>>> it as it is needed for the followup aarch64 eh_return changes:
>>>>
>>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/605769.html
>>>>
>>>> ---
>>>>   gcc/function.cc | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/gcc/function.cc b/gcc/function.cc
>>>> index dd2c1136e07..70d1cd65303 100644
>>>> --- a/gcc/function.cc
>>>> +++ b/gcc/function.cc
>>>> @@ -6120,6 +6120,11 @@ thread_prologue_and_epilogue_insns (void)
>>>>                    && returnjump_p (BB_END (e->src)))
>>>>                  e->flags &= ~EDGE_FALLTHRU;
>>>>              }
>>>> +
>>>> +         auto_sbitmap blocks (last_basic_block_for_fn (cfun));
>>>> +         bitmap_clear (blocks);
>>>> +           bitmap_set_bit (blocks, BLOCK_FOR_INSN (epilogue_seq)->index);
>>>> +         find_many_sub_basic_blocks (blocks);
>>>>          }
>>>>         else if (next_active_insn (BB_END (exit_fallthru_edge->src)))
>>>>          {
>>>> @@ -6218,6 +6223,11 @@ thread_prologue_and_epilogue_insns (void)
>>>>            set_insn_locations (seq, epilogue_location);
>>>>
>>>>            emit_insn_before (seq, insn);
>>>> +
>>>> +         auto_sbitmap blocks (last_basic_block_for_fn (cfun));
>>>> +         bitmap_clear (blocks);
>>>> +         bitmap_set_bit (blocks, BLOCK_FOR_INSN (insn)->index);
>>>> +         find_many_sub_basic_blocks (blocks);
>>>
>>> I'll note that clearing a full sbitmap to pass down a single basic block
>>> to find_many_sub_basic_blocks is a quite expensive operation.  May I suggest
>>> to add an overload operating on a single basic block?  It's only
>>>
>>>    FOR_EACH_BB_FN (bb, cfun)
>>>      SET_STATE (bb,
>>>                 bitmap_bit_p (blocks, bb->index) ? BLOCK_TO_SPLIT :
>>> BLOCK_ORIGINAL);
>>>
>>> using the bitmap, so factoring the rest of the function and customizing this
>>> walk would do the trick.  Note that the whole function could be refactored to
>>> handle single blocks more efficiently.
>>
>> Sorry for the late reply, but does this look OK?  Tested on
>> aarch64-linux-gnu and x86_64-linux-gnu.
> 
> LGTM, not sure if I'm qualified enough to approve though (I think you
> are more qualified here, so ..)
It looks quite sensible to me.

Jeff

Re: [PATCH 10/11] aarch64: Fix branch-protection error message tests

[Szabolcs Nagy]

The 10/13/2023 11:29, Richard Earnshaw (lists) wrote:
> On 05/09/2023 16:00, Richard Sandiford via Gcc-patches wrote:
> > Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> >> @@ -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 } */
> 
> 'leaf' is really a modifier for the other branch protection strategies; perhaps it would be better to describe it as that.

this error message is used for arbitrary strings, e.g.
branch-protection=foobar or branch-protection=bti+foo.

with further processing we can figure out that 'leaf'
is a valid modifier for pac-ret and change the error to

invalid placement of modifier 'leaf' in 'target("branch-protection=")'

otherwise fall back to

invalid argument 'foobar' for 'target("branch-protection=")'.

does that help?

(currently 'leaf' and 'b-key' are the only modifiers.)

> But this brings up another issue/question.  If the compiler has been configured with, say, '--enable-branch-protection=standard' or some other variety, is there (or do we want) a way to extend that to leaf functions without changing the underlying strategy?

there are several limitations in branch-protection handling,
i'm only fixing bugs and assumptions that don't work when arm
and aarch64 has different set of branch-protection options.

i think it can be useful to add/remove branch-protection options
incrementally in cflags instead of having one string, but it's
not obvious to me how to get there.

> >>  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 } */
> 
> Or maybe better still: "branch protection strategies 'none' and 'pac-ret' are incompatible".

i can make this change, but e.g.

in case of branch-protection=standard+bti+foo it would
say "'standard' and 'bti' are incompatible" which can be
surprising given that standard includes bti, meanwhile
"'standard' can only appear alone" explains the problem.

> But this is all a matter of taste.
> 
> However, this patch should be merged with the patch that changes the error messages.  Or has that already gone in?

i can do that merge.