[PATCH v1 0/3][libgcc] store signing key and signing method in DWARF _Unwind_FrameState

[PATCH v1 0/3][libgcc] store signing key and signing method in DWARF _Unwind_FrameState

[Matthieu Longo]

This patch series is only a refactoring of the existing implementation of PAuth and returned-address signing. The existing behavior is preserved.

1. aarch64: store signing key and signing method in DWARF _Unwind_FrameState

_Unwind_FrameState already contains several CIE and FDE information (see the attributes below the comment "The information we care about from the CIE/FDE" in libgcc/unwind-dw2.h).
The patch aims at moving the information from DWARF CIE (signing key stored in the augmentation string) and FDE (the used signing method) into _Unwind_FrameState along the already-stored CIE and FDE information.
Note: those information have to be saved in frame_state_reg_info instead of _Unwind_FrameState as they need to be savable by DW_CFA_remember_state and restorable by DW_CFA_restore_state, that both rely on the attribute "prev".
Those new information in _Unwind_FrameState simplifies the look-up of the signing key when the return address is demangled. It also allows future signing methods to be easily added.
_Unwind_FrameState is not a part of the public API of libunwind, so the change is backward compatible.

A new architecture-specific handler MD_ARCH_EXTENSION_FRAME_INIT allows to reset values in the frame state and unwind context if needed by the architecture extension before changing the frame state to the caller context.
A new architecture-specific handler MD_ARCH_EXTENSION_CIE_AUG_HANDLER isolates the architecture-specific augmentation strings in AArch64 backend, and allows others architectures to reuse augmentation strings that would have clashed with AArch64 DWARF extensions.
aarch64_demangle_return_addr, DW_CFA_AARCH64_negate_ra_state and DW_CFA_val_expression cases in libgcc/unwind-dw2-execute_cfa.h were documented to clarify where the value of the RA state register is stored (FS and CONTEXT respectively).

2. libgcc: hide CIE and FDE data for DWARF architecture extensions behind a handler.

This patch provides a new handler MD_ARCH_FRAME_STATE_T to hide an architecture-specific structure containing CIE and FDE data related to DWARF architecture extensions.
Hiding the architecture-specific attributes behind a handler has the following benefits:
    1. isolating those data from the generic ones in _Unwind_FrameState
    2. avoiding casts to custom types.
    3. preserving typing information when debugging with GDB, and so facilitating their printing.

This approach required to add a new header md-unwind-def.h included at the top of libgcc/unwind-dw2.h, and redirecting to the corresponding architecture header via a symbolic link.
An obvious drawback is the increase in complexity with macros, and headers. It also caused a split of architecture definitions between md-unwind-def.h (types definitions used in unwind-dw2.h) and md-unwind.h (local types definitions and handlers implementations).
The naming of md-unwind.h with .h extension is a bit misleading as the file is only included in the middle of unwind-dw2.c. Changing this naming would require modification of others backends, which I prefered to abstain from.
Overall the benefits are worth the added complexity from my perspective.

3. libgcc: update configure (regenerated by autoreconf)

Regenerate the build files.


## Testing

Those changes were testing by covering the 3 following cases:
- backtracing.
- exception handling in a C++ program.
- gcc/testsuite/gcc.target/aarch64/pr104689.c: pac-ret with unusual DWARF [1]

Regression tested on aarch64-unknown-linux-gnu, and no regression found.

[1]: https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594414.html


Ok for master? I don't have commit access so I need someone to commit on my behalf.

Regards,
Matthieu.


Matthieu Longo (3):
  aarch64: store signing key and signing method in DWARF
    _Unwind_FrameState
  libgcc: hide CIE and FDE data for DWARF architecture extensions behind
    a handler.
  libgcc: update configure (regenerated by autoreconf)

 libgcc/Makefile.in                         |   6 +-
 libgcc/config.host                         |  13 +-
 libgcc/config/aarch64/aarch64-unwind-def.h |  41 ++++++
 libgcc/config/aarch64/aarch64-unwind.h     | 150 +++++++++++++++++----
 libgcc/configure                           |   2 +
 libgcc/configure.ac                        |   1 +
 libgcc/unwind-dw2-execute_cfa.h            |  34 +++--
 libgcc/unwind-dw2.c                        |  19 ++-
 libgcc/unwind-dw2.h                        |  19 ++-
 9 files changed, 233 insertions(+), 52 deletions(-)
 create mode 100644 libgcc/config/aarch64/aarch64-unwind-def.h

-- 
2.45.2

[PATCH v1 1/3] aarch64: store signing key and signing method in DWARF _Unwind_FrameState

[Matthieu Longo]

This patch is only a refactoring of the existing implementation
of PAuth and returned-address signing. The existing behavior is
preserved.

_Unwind_FrameState already contains several CIE and FDE information
(see the attributes below the comment "The information we care
about from the CIE/FDE" in libgcc/unwind-dw2.h).
The patch aims at moving the information from DWARF CIE (signing
key stored in the augmentation string) and FDE (the used signing
method) into _Unwind_FrameState along the already-stored CIE and
FDE information.
Note: those information have to be saved in frame_state_reg_info
instead of _Unwind_FrameState as they need to be savable by
DW_CFA_remember_state and restorable by DW_CFA_restore_state, that
both rely on the attribute "prev".

Those new information in _Unwind_FrameState simplifies the look-up
of the signing key when the return address is demangled. It also
allows future signing methods to be easily added.

_Unwind_FrameState is not a part of the public API of libunwind,
so the change is backward compatible.

A new architecture-specific handler MD_ARCH_EXTENSION_FRAME_INIT
allows to reset values (if needed) in the frame state and unwind
context before changing the frame state to the caller context.

A new architecture-specific handler MD_ARCH_EXTENSION_CIE_AUG_HANDLER
isolates the architecture-specific augmentation strings in AArch64
backend, and allows others architectures to reuse augmentation
strings that would have clashed with AArch64 DWARF extensions.

aarch64_demangle_return_addr, DW_CFA_AARCH64_negate_ra_state and
DW_CFA_val_expression cases in libgcc/unwind-dw2-execute_cfa.h
were documented to clarify where the value of the RA state register
is stored (FS and CONTEXT respectively).

libgcc/ChangeLog:

  * config/aarch64/aarch64-unwind.h
  (AARCH64_DWARF_RA_STATE_MASK): The mask for RA state register.
  (aarch64_RA_signing_method_t): The diversifiers used to sign a
  function's return address.
  (aarch64_pointer_auth_key): The key used to sign a function's
  return address.
  (aarch64_cie_signed_with_b_key): Deleted as the signing key is
  available now in _Unwind_FrameState.
  (MD_ARCH_EXTENSION_CIE_AUG_HANDLER): New CIE augmentation string
  handler for architecture extensions.
  (MD_ARCH_EXTENSION_FRAME_INIT): New architecture-extension
  initialization routine for DWARF frame state and context before
  execution of DWARF instructions.
  (aarch64_context_RA_state_get): Read RA state register from CONTEXT.
  (aarch64_RA_state_get): Read RA state register from FS.
  (aarch64_RA_state_set): Write RA state register into FS.
  (aarch64_RA_state_toggle): Toggle RA state register in FS.
  (aarch64_cie_aug_handler): Handler AArch64 augmentation strings.
  (aarch64_arch_extension_frame_init): Initialize defaults for the
  signing key (PAUTH_KEY_A), and RA state register (RA_no_signing).
  (aarch64_demangle_return_addr): Rely on the frame registers and
  the signing_key attribute in _Unwind_FrameState.
  * unwind-dw2-execute_cfa.h:
  Use the right alias DW_CFA_AARCH64_negate_ra_state for __aarch64__
  instead of DW_CFA_GNU_window_save.
  (DW_CFA_AARCH64_negate_ra_state): Save the signing method in RA
  state register. Toggle RA state register without resetting 'how'
  to REG_UNSAVED.
  * unwind-dw2.c:
  (extract_cie_info): Save the signing key in the current
  _Unwind_FrameState while parsing the augmentation data.
  (uw_frame_state_for): Reset some attributes related to architecture
  extensions in _Unwind_FrameState.
  (uw_update_context): Move authentication code to AArch64 unwinding.
  * unwind-dw2.h (enum register_rule): Give a name to the existing
  enum for the register rules, and replace 'unsigned char' by 'enum
  register_rule' to facilitate debugging in GDB.
  (_Unwind_FrameState): Add a new architecture-extension attribute
  to store the signing key.
---
 libgcc/config/aarch64/aarch64-unwind.h | 154 ++++++++++++++++++++-----
 libgcc/unwind-dw2-execute_cfa.h        |  34 ++++--
 libgcc/unwind-dw2.c                    |  19 ++-
 libgcc/unwind-dw2.h                    |  17 ++-
 4 files changed, 175 insertions(+), 49 deletions(-)

diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
index daf96624b5e..cc225a7e207 100644
--- a/libgcc/config/aarch64/aarch64-unwind.h
+++ b/libgcc/config/aarch64/aarch64-unwind.h
@@ -25,55 +25,155 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #if !defined (AARCH64_UNWIND_H) && !defined (__ILP32__)
 #define AARCH64_UNWIND_H
 
-#define DWARF_REGNUM_AARCH64_RA_STATE 34
+#include "ansidecl.h"
+#include <stdbool.h>
+
+#define AARCH64_DWARF_REGNUM_RA_STATE 34
+#define AARCH64_DWARF_RA_STATE_MASK   0x1
+
+/* The diversifiers used to sign a function's return address. */
+typedef enum
+{
+  AARCH64_RA_no_signing = 0x0,
+  AARCH64_RA_signing_SP = 0x1,
+} __attribute__((packed)) aarch64_RA_signing_method_t;
+
+/* The key used to sign a function's return address. */
+typedef enum {
+  AARCH64_PAUTH_KEY_A,
+  AARCH64_PAUTH_KEY_B,
+} __attribute__((packed)) aarch64_pointer_auth_key;
+
+#define MD_ARCH_EXTENSION_CIE_AUG_HANDLER(fs, aug) \
+  aarch64_cie_aug_handler(fs, aug)
+
+#define MD_ARCH_EXTENSION_FRAME_INIT(context, fs) \
+  aarch64_arch_extension_frame_init(context, fs)
 
 #define MD_DEMANGLE_RETURN_ADDR(context, fs, addr) \
   aarch64_demangle_return_addr (context, fs, addr)
 
-static inline int
-aarch64_cie_signed_with_b_key (struct _Unwind_Context *context)
+static inline aarch64_RA_signing_method_t
+aarch64_context_RA_state_get(struct _Unwind_Context *context)
+{
+  const int index = AARCH64_DWARF_REGNUM_RA_STATE;
+  return _Unwind_GetGR (context, index) & AARCH64_DWARF_RA_STATE_MASK;
+}
+
+static inline aarch64_RA_signing_method_t
+aarch64_fs_RA_state_get(_Unwind_FrameState const* fs)
 {
-  const struct dwarf_fde *fde = _Unwind_Find_FDE (context->bases.func,
-						  &context->bases);
-  if (fde != NULL)
+  const int index = AARCH64_DWARF_REGNUM_RA_STATE;
+  return fs->regs.reg[index].loc.offset & AARCH64_DWARF_RA_STATE_MASK;
+}
+
+static inline void
+aarch64_fs_RA_state_set(_Unwind_FrameState *fs,
+			aarch64_RA_signing_method_t signing_method)
+{
+  fs->regs.reg[AARCH64_DWARF_REGNUM_RA_STATE].loc.offset = signing_method;
+}
+
+static inline void
+aarch64_fs_RA_state_toggle(_Unwind_FrameState *fs)
+{
+  aarch64_RA_signing_method_t signing_method = aarch64_fs_RA_state_get(fs);
+  gcc_assert (signing_method == AARCH64_RA_no_signing ||
+	      signing_method == AARCH64_RA_signing_SP);
+  aarch64_fs_RA_state_set(fs, (signing_method == AARCH64_RA_no_signing)
+			  ? AARCH64_RA_signing_SP
+			  : AARCH64_RA_no_signing);
+}
+
+/* CIE handler for custom augmentation string.  */
+static inline bool
+aarch64_cie_aug_handler(_Unwind_FrameState *fs, unsigned char aug)
+{
+  // AArch64 B-key pointer authentication.
+  if (aug == 'B')
     {
-      const struct dwarf_cie *cie = get_cie (fde);
-      if (cie != NULL)
-	{
-	  const unsigned char *aug_str = cie->augmentation;
-	  return __builtin_strchr ((const char *) aug_str,
-				   'B') == NULL ? 0 : 1;
-	}
+      fs->regs.signing_key = AARCH64_PAUTH_KEY_B;
+      return true;
     }
-  return 0;
+  return false;
 }
 
-/* Do AArch64 private extraction on ADDR_WORD based on context info CONTEXT and
-   unwind frame info FS.  If ADDR_WORD is signed, we do address authentication
-   on it using CFA of current frame.  */
+/* At the entrance of a new frame, some cached information from the CIE/FDE,
+ * and registers values related to architectural extensions require a default
+ * initialization.
+ * If any of those values related to architecture extensions had to be saved
+ * for the next frame, it should be done via the architecture extensions handler
+ * MD_FROB_UPDATE_CONTEXT in uw_update_context_1 (libgcc/unwind-dw2.c).  */
+static inline void
+aarch64_arch_extension_frame_init (struct _Unwind_Context *context ATTRIBUTE_UNUSED,
+				   _Unwind_FrameState *fs)
+{
+  // Reset previously cached CIE/FDE information.
+  fs->regs.signing_key = AARCH64_PAUTH_KEY_A;
+
+  // Reset some registers.
+  // Note: the associated fs->how table is automatically reset to REG_UNSAVED in
+  // uw_frame_state_for (libgcc/unwind-dw2.c). REG_UNSAVED means that whatever
+  // was in the previous context is the current register value. In other words,
+  // the next context inherits by default the previous value for a register.
+  // To keep this logic coherent with some architecture extensions, we need to
+  // reinitialize fs->how for some registers to REG_ARCHEXT.
+  const int reg = AARCH64_DWARF_REGNUM_RA_STATE;
+  fs->regs.how[reg] = REG_ARCHEXT;
+  aarch64_fs_RA_state_set(fs, AARCH64_RA_no_signing);
+}
 
+/* Do AArch64 private extraction on ADDR_WORD based on context info CONTEXT and
+   unwind frame info FS. If ADDR_WORD is signed, we do address authentication
+   on it using CFA of current frame.
+   Note: this function is called after uw_update_context_1 in uw_update_context.
+   uw_update_context_1 is the function in which val expression are computed. Thus
+   if DW_CFA_val_expression is used, the value of the RA state register is stored
+   in CONTEXT, not FS. (more details about when DW_CFA_val_expression is used in
+   the next comment below)  */
 static inline void *
 aarch64_demangle_return_addr (struct _Unwind_Context *context,
 			      _Unwind_FrameState *fs,
 			      _Unwind_Word addr_word)
 {
   void *addr = (void *)addr_word;
-  const int reg = DWARF_REGNUM_AARCH64_RA_STATE;
-
-  if (fs->regs.how[reg] == REG_UNSAVED)
-    return addr;
+  const int reg = AARCH64_DWARF_REGNUM_RA_STATE;
+
+  // In libgcc, REG_ARCHEXT means that the RA state register was set by an AArch64
+  // DWARF instruction and contains a valid value.
+  // Return-address signing state is normally toggled by DW_CFA_AARCH64_negate_ra_state
+  // (also knwon by its alias as DW_CFA_GNU_window_save).
+  // However, RA state register can be set directly via DW_CFA_val_expression
+  // too. GCC does not generate such CFI but some other compilers reportedly do.
+  // (see PR104689 for more details).
+  // Any other value than REG_ARCHEXT should be interpreted as if the RA state register
+  // is set by another DWARF instruction, and the value is fetchable via _Unwind_GetGR.
+  aarch64_RA_signing_method_t signing_method = AARCH64_RA_no_signing;
+  if (fs->regs.how[reg] == REG_ARCHEXT)
+    {
+      signing_method = aarch64_fs_RA_state_get(fs);
+    }
+  else if (fs->regs.how[reg] != REG_UNSAVED)
+    {
+      signing_method = aarch64_context_RA_state_get(context);
+
+      // If the value was set in context, CONTEXT->by_value was set to 1.
+      // uw_install_context_1 contains an assert on by_value, to check that all
+      // registers values have been resolved before installing the context.
+      // This will make the unwinding crash if we don't reset CONTEXT->by_value,
+      // and CONTEXT->reg[reg].
+      context->reg[reg] = _Unwind_Get_Unwind_Context_Reg_Val(0x0);
+      context->by_value[reg] = 0;
+    }
 
-  /* Return-address signing state is toggled by DW_CFA_GNU_window_save (where
-     REG_UNSAVED/REG_UNSAVED_ARCHEXT means RA signing is disabled/enabled),
-     or set by a DW_CFA_expression.  */
-  if (fs->regs.how[reg] == REG_UNSAVED_ARCHEXT
-      || (_Unwind_GetGR (context, reg) & 0x1) != 0)
+  if (signing_method == AARCH64_RA_signing_SP)
     {
       _Unwind_Word salt = (_Unwind_Word) context->cfa;
-      if (aarch64_cie_signed_with_b_key (context) != 0)
+      if (fs->regs.signing_key == AARCH64_PAUTH_KEY_B)
 	return __builtin_aarch64_autib1716 (addr, salt);
       return __builtin_aarch64_autia1716 (addr, salt);
     }
+  // else {} // signing_method == AARCH64_RA_no_signing, nothing to do.
 
   return addr;
 }
diff --git a/libgcc/unwind-dw2-execute_cfa.h b/libgcc/unwind-dw2-execute_cfa.h
index a6b249f9ad6..2ea78c4ef8d 100644
--- a/libgcc/unwind-dw2-execute_cfa.h
+++ b/libgcc/unwind-dw2-execute_cfa.h
@@ -271,23 +271,33 @@
 	      fs->regs.how[reg] = REG_SAVED_VAL_EXP;
 	      fs->regs.reg[reg].loc.exp = insn_ptr;
 	    }
+	  // Don't execute the expression, but jump over it by adding
+	  // DW_FORM_block's size to insn_ptr.
 	  insn_ptr = read_uleb128 (insn_ptr, &utmp);
 	  insn_ptr += utmp;
 	  break;
 
-	case DW_CFA_GNU_window_save:
 #if defined (__aarch64__) && !defined (__ILP32__)
-	  /* This CFA is multiplexed with Sparc.  On AArch64 it's used to toggle
-	     return address signing status.  REG_UNSAVED/REG_UNSAVED_ARCHEXT
-	     mean RA signing is disabled/enabled.  */
-	  reg = DWARF_REGNUM_AARCH64_RA_STATE;
-	  gcc_assert (fs->regs.how[reg] == REG_UNSAVED
-		      || fs->regs.how[reg] == REG_UNSAVED_ARCHEXT);
-	  if (fs->regs.how[reg] == REG_UNSAVED)
-	    fs->regs.how[reg] = REG_UNSAVED_ARCHEXT;
-	  else
-	    fs->regs.how[reg] = REG_UNSAVED;
+	case DW_CFA_AARCH64_negate_ra_state:
+	  // This CFA is multiplexed with Sparc.
+	  // On AArch64 it's used to toggle the status of return address signing
+	  // with SP as a diversifier.
+	  // - REG_ARCHEXT means that the RA state register in FS contains a
+	  // valid value, and that no other DWARF directive has changed the value
+	  // of this register.
+	  // - any other value is not compatible with negating the RA state.
+	  reg = AARCH64_DWARF_REGNUM_RA_STATE;
+
+	  // /!\ Mixing DW_CFA_val_expression with DW_CFA_AARCH64_negate_ra_state
+	  // will result in an undefined behavior (likely an unwinding failure),
+	  // as the chronology of the DWARF directives will be broken.
+	  gcc_assert (fs->regs.how[reg] == REG_ARCHEXT);
+
+	  // Toggle current state
+	  aarch64_fs_RA_state_toggle(fs);
+	  break;
 #else
+	case DW_CFA_GNU_window_save:
 	  /* ??? Hardcoded for SPARC register window configuration.  */
 	  if (__LIBGCC_DWARF_FRAME_REGISTERS__ >= 32)
 	    for (reg = 16; reg < 32; ++reg)
@@ -295,8 +305,8 @@
 		fs->regs.how[reg] = REG_SAVED_OFFSET;
 		fs->regs.reg[reg].loc.offset = (reg - 16) * sizeof (void *);
 	      }
-#endif
 	  break;
+#endif
 
 	case DW_CFA_GNU_args_size:
 	  insn_ptr = read_uleb128 (insn_ptr, &utmp);
diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
index 0849e89cd34..fb60853825d 100644
--- a/libgcc/unwind-dw2.c
+++ b/libgcc/unwind-dw2.c
@@ -501,11 +501,11 @@ extract_cie_info (const struct dwarf_cie *cie, struct _Unwind_Context *context,
 	  fs->signal_frame = 1;
 	  aug += 1;
 	}
-      /* aarch64 B-key pointer authentication.  */
-      else if (aug[0] == 'B')
-	{
-	  aug += 1;
-      }
+
+#if defined(MD_ARCH_EXTENSION_CIE_AUG_HANDLER)
+      else if (MD_ARCH_EXTENSION_CIE_AUG_HANDLER(fs, aug[0]))
+	aug += 1;
+#endif
 
       /* Otherwise we have an unknown augmentation string.
 	 Bail unless we saw a 'z' prefix.  */
@@ -996,6 +996,9 @@ uw_frame_state_for (struct _Unwind_Context *context, _Unwind_FrameState *fs)
 
   memset (&fs->regs.how[0], 0,
 	  sizeof (*fs) - offsetof (_Unwind_FrameState, regs.how[0]));
+#if defined(MD_ARCH_EXTENSION_FRAME_INIT)
+  MD_ARCH_EXTENSION_FRAME_INIT(context, fs);
+#endif
   context->args_size = 0;
   context->lsda = 0;
 
@@ -1197,7 +1200,11 @@ uw_update_context_1 (struct _Unwind_Context *context, _Unwind_FrameState *fs)
       {
       case REG_UNSAVED:
       case REG_UNDEFINED:
-      case REG_UNSAVED_ARCHEXT:
+      case REG_ARCHEXT: // If the value depends on an augmenter, then there is
+			// no processing to do here, and the value computation
+			// should be delayed until the architecture handler
+			// computes the value correctly based on the augmenter
+			// information.
 	break;
 
       case REG_SAVED_OFFSET:
diff --git a/libgcc/unwind-dw2.h b/libgcc/unwind-dw2.h
index 0dd8611f697..b75f4c65f98 100644
--- a/libgcc/unwind-dw2.h
+++ b/libgcc/unwind-dw2.h
@@ -22,16 +22,17 @@
    see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
-enum {
+enum register_rule
+{
   REG_UNSAVED,
   REG_SAVED_OFFSET,
   REG_SAVED_REG,
   REG_SAVED_EXP,
   REG_SAVED_VAL_OFFSET,
   REG_SAVED_VAL_EXP,
-  REG_UNSAVED_ARCHEXT,		/* Target specific extension.  */
+  REG_ARCHEXT,		/* Target specific extension.  */
   REG_UNDEFINED
-};
+} __attribute__((packed));
 
 /* The result of interpreting the frame unwind info for a frame.
    This is all symbolic at this point, as none of the values can
@@ -49,7 +50,7 @@ typedef struct
 	const unsigned char *exp;
       } loc;
     } reg[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
-    unsigned char how[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
+    enum register_rule how[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
 
     enum {
       CFA_UNSET,
@@ -65,6 +66,14 @@ typedef struct
     _Unwind_Sword cfa_offset;
     _Unwind_Word cfa_reg;
     const unsigned char *cfa_exp;
+
+    /* Architecture extensions information from CIE/FDE.
+     * Note: those information have to be saved in struct frame_state_reg_info
+     * instead of _Unwind_FrameState as DW_CFA_restore_state has to be able to
+     * restore them.  */
+#if defined(__aarch64__) && !defined (__ILP32__)
+    unsigned char signing_key;
+#endif
   } regs;
 
   /* The PC described by the current frame state.  */
-- 
2.45.2

Re: [PATCH v1 1/3] aarch64: store signing key and signing method in DWARF _Unwind_FrameState

[Matthieu Longo]

On 2024-07-19 15:54, Matthieu Longo wrote:
> This patch is only a refactoring of the existing implementation
> of PAuth and returned-address signing. The existing behavior is
> preserved.
> 
> _Unwind_FrameState already contains several CIE and FDE information
> (see the attributes below the comment "The information we care
> about from the CIE/FDE" in libgcc/unwind-dw2.h).
> The patch aims at moving the information from DWARF CIE (signing
> key stored in the augmentation string) and FDE (the used signing
> method) into _Unwind_FrameState along the already-stored CIE and
> FDE information.
> Note: those information have to be saved in frame_state_reg_info
> instead of _Unwind_FrameState as they need to be savable by
> DW_CFA_remember_state and restorable by DW_CFA_restore_state, that
> both rely on the attribute "prev".
> 
> Those new information in _Unwind_FrameState simplifies the look-up
> of the signing key when the return address is demangled. It also
> allows future signing methods to be easily added.
> 
> _Unwind_FrameState is not a part of the public API of libunwind,
> so the change is backward compatible.
> 
> A new architecture-specific handler MD_ARCH_EXTENSION_FRAME_INIT
> allows to reset values (if needed) in the frame state and unwind
> context before changing the frame state to the caller context.
> 
> A new architecture-specific handler MD_ARCH_EXTENSION_CIE_AUG_HANDLER
> isolates the architecture-specific augmentation strings in AArch64
> backend, and allows others architectures to reuse augmentation
> strings that would have clashed with AArch64 DWARF extensions.
> 
> aarch64_demangle_return_addr, DW_CFA_AARCH64_negate_ra_state and
> DW_CFA_val_expression cases in libgcc/unwind-dw2-execute_cfa.h
> were documented to clarify where the value of the RA state register
> is stored (FS and CONTEXT respectively).
> 
> libgcc/ChangeLog:
> 
>    * config/aarch64/aarch64-unwind.h
>    (AARCH64_DWARF_RA_STATE_MASK): The mask for RA state register.
>    (aarch64_RA_signing_method_t): The diversifiers used to sign a
>    function's return address.
>    (aarch64_pointer_auth_key): The key used to sign a function's
>    return address.
>    (aarch64_cie_signed_with_b_key): Deleted as the signing key is
>    available now in _Unwind_FrameState.
>    (MD_ARCH_EXTENSION_CIE_AUG_HANDLER): New CIE augmentation string
>    handler for architecture extensions.
>    (MD_ARCH_EXTENSION_FRAME_INIT): New architecture-extension
>    initialization routine for DWARF frame state and context before
>    execution of DWARF instructions.
>    (aarch64_context_RA_state_get): Read RA state register from CONTEXT.
>    (aarch64_RA_state_get): Read RA state register from FS.
>    (aarch64_RA_state_set): Write RA state register into FS.
>    (aarch64_RA_state_toggle): Toggle RA state register in FS.
>    (aarch64_cie_aug_handler): Handler AArch64 augmentation strings.
>    (aarch64_arch_extension_frame_init): Initialize defaults for the
>    signing key (PAUTH_KEY_A), and RA state register (RA_no_signing).
>    (aarch64_demangle_return_addr): Rely on the frame registers and
>    the signing_key attribute in _Unwind_FrameState.
>    * unwind-dw2-execute_cfa.h:
>    Use the right alias DW_CFA_AARCH64_negate_ra_state for __aarch64__
>    instead of DW_CFA_GNU_window_save.
>    (DW_CFA_AARCH64_negate_ra_state): Save the signing method in RA
>    state register. Toggle RA state register without resetting 'how'
>    to REG_UNSAVED.
>    * unwind-dw2.c:
>    (extract_cie_info): Save the signing key in the current
>    _Unwind_FrameState while parsing the augmentation data.
>    (uw_frame_state_for): Reset some attributes related to architecture
>    extensions in _Unwind_FrameState.
>    (uw_update_context): Move authentication code to AArch64 unwinding.
>    * unwind-dw2.h (enum register_rule): Give a name to the existing
>    enum for the register rules, and replace 'unsigned char' by 'enum
>    register_rule' to facilitate debugging in GDB.
>    (_Unwind_FrameState): Add a new architecture-extension attribute
>    to store the signing key.
> ---
>   libgcc/config/aarch64/aarch64-unwind.h | 154 ++++++++++++++++++++-----
>   libgcc/unwind-dw2-execute_cfa.h        |  34 ++++--
>   libgcc/unwind-dw2.c                    |  19 ++-
>   libgcc/unwind-dw2.h                    |  17 ++-
>   4 files changed, 175 insertions(+), 49 deletions(-)
> 
> diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
> index daf96624b5e..cc225a7e207 100644
> --- a/libgcc/config/aarch64/aarch64-unwind.h
> +++ b/libgcc/config/aarch64/aarch64-unwind.h
> @@ -25,55 +25,155 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>   #if !defined (AARCH64_UNWIND_H) && !defined (__ILP32__)
>   #define AARCH64_UNWIND_H
>   
> -#define DWARF_REGNUM_AARCH64_RA_STATE 34
> +#include "ansidecl.h"
> +#include <stdbool.h>
> +
> +#define AARCH64_DWARF_REGNUM_RA_STATE 34
> +#define AARCH64_DWARF_RA_STATE_MASK   0x1
> +
> +/* The diversifiers used to sign a function's return address. */
> +typedef enum
> +{
> +  AARCH64_RA_no_signing = 0x0,
> +  AARCH64_RA_signing_SP = 0x1,
> +} __attribute__((packed)) aarch64_RA_signing_method_t;
> +
> +/* The key used to sign a function's return address. */
> +typedef enum {
> +  AARCH64_PAUTH_KEY_A,
> +  AARCH64_PAUTH_KEY_B,
> +} __attribute__((packed)) aarch64_pointer_auth_key;
> +
> +#define MD_ARCH_EXTENSION_CIE_AUG_HANDLER(fs, aug) \
> +  aarch64_cie_aug_handler(fs, aug)
> +
> +#define MD_ARCH_EXTENSION_FRAME_INIT(context, fs) \
> +  aarch64_arch_extension_frame_init(context, fs)
>   
>   #define MD_DEMANGLE_RETURN_ADDR(context, fs, addr) \
>     aarch64_demangle_return_addr (context, fs, addr)
>   
> -static inline int
> -aarch64_cie_signed_with_b_key (struct _Unwind_Context *context)
> +static inline aarch64_RA_signing_method_t
> +aarch64_context_RA_state_get(struct _Unwind_Context *context)
> +{
> +  const int index = AARCH64_DWARF_REGNUM_RA_STATE;
> +  return _Unwind_GetGR (context, index) & AARCH64_DWARF_RA_STATE_MASK;
> +}
> +
> +static inline aarch64_RA_signing_method_t
> +aarch64_fs_RA_state_get(_Unwind_FrameState const* fs)
>   {
> -  const struct dwarf_fde *fde = _Unwind_Find_FDE (context->bases.func,
> -						  &context->bases);
> -  if (fde != NULL)
> +  const int index = AARCH64_DWARF_REGNUM_RA_STATE;
> +  return fs->regs.reg[index].loc.offset & AARCH64_DWARF_RA_STATE_MASK;
> +}
> +
> +static inline void
> +aarch64_fs_RA_state_set(_Unwind_FrameState *fs,
> +			aarch64_RA_signing_method_t signing_method)
> +{
> +  fs->regs.reg[AARCH64_DWARF_REGNUM_RA_STATE].loc.offset = signing_method;
> +}
> +
> +static inline void
> +aarch64_fs_RA_state_toggle(_Unwind_FrameState *fs)
> +{
> +  aarch64_RA_signing_method_t signing_method = aarch64_fs_RA_state_get(fs);
> +  gcc_assert (signing_method == AARCH64_RA_no_signing ||
> +	      signing_method == AARCH64_RA_signing_SP);
> +  aarch64_fs_RA_state_set(fs, (signing_method == AARCH64_RA_no_signing)
> +			  ? AARCH64_RA_signing_SP
> +			  : AARCH64_RA_no_signing);
> +}
> +
> +/* CIE handler for custom augmentation string.  */
> +static inline bool
> +aarch64_cie_aug_handler(_Unwind_FrameState *fs, unsigned char aug)
> +{
> +  // AArch64 B-key pointer authentication.
> +  if (aug == 'B')
>       {
> -      const struct dwarf_cie *cie = get_cie (fde);
> -      if (cie != NULL)
> -	{
> -	  const unsigned char *aug_str = cie->augmentation;
> -	  return __builtin_strchr ((const char *) aug_str,
> -				   'B') == NULL ? 0 : 1;
> -	}
> +      fs->regs.signing_key = AARCH64_PAUTH_KEY_B;
> +      return true;
>       }
> -  return 0;
> +  return false;
>   }
>   
> -/* Do AArch64 private extraction on ADDR_WORD based on context info CONTEXT and
> -   unwind frame info FS.  If ADDR_WORD is signed, we do address authentication
> -   on it using CFA of current frame.  */
> +/* At the entrance of a new frame, some cached information from the CIE/FDE,
> + * and registers values related to architectural extensions require a default
> + * initialization.
> + * If any of those values related to architecture extensions had to be saved
> + * for the next frame, it should be done via the architecture extensions handler
> + * MD_FROB_UPDATE_CONTEXT in uw_update_context_1 (libgcc/unwind-dw2.c).  */
> +static inline void
> +aarch64_arch_extension_frame_init (struct _Unwind_Context *context ATTRIBUTE_UNUSED,
> +				   _Unwind_FrameState *fs)
> +{
> +  // Reset previously cached CIE/FDE information.
> +  fs->regs.signing_key = AARCH64_PAUTH_KEY_A;
> +
> +  // Reset some registers.
> +  // Note: the associated fs->how table is automatically reset to REG_UNSAVED in
> +  // uw_frame_state_for (libgcc/unwind-dw2.c). REG_UNSAVED means that whatever
> +  // was in the previous context is the current register value. In other words,
> +  // the next context inherits by default the previous value for a register.
> +  // To keep this logic coherent with some architecture extensions, we need to
> +  // reinitialize fs->how for some registers to REG_ARCHEXT.
> +  const int reg = AARCH64_DWARF_REGNUM_RA_STATE;
> +  fs->regs.how[reg] = REG_ARCHEXT;
> +  aarch64_fs_RA_state_set(fs, AARCH64_RA_no_signing);
> +}
>   
> +/* Do AArch64 private extraction on ADDR_WORD based on context info CONTEXT and
> +   unwind frame info FS. If ADDR_WORD is signed, we do address authentication
> +   on it using CFA of current frame.
> +   Note: this function is called after uw_update_context_1 in uw_update_context.
> +   uw_update_context_1 is the function in which val expression are computed. Thus
> +   if DW_CFA_val_expression is used, the value of the RA state register is stored
> +   in CONTEXT, not FS. (more details about when DW_CFA_val_expression is used in
> +   the next comment below)  */
>   static inline void *
>   aarch64_demangle_return_addr (struct _Unwind_Context *context,
>   			      _Unwind_FrameState *fs,
>   			      _Unwind_Word addr_word)
>   {
>     void *addr = (void *)addr_word;
> -  const int reg = DWARF_REGNUM_AARCH64_RA_STATE;
> -
> -  if (fs->regs.how[reg] == REG_UNSAVED)
> -    return addr;
> +  const int reg = AARCH64_DWARF_REGNUM_RA_STATE;
> +
> +  // In libgcc, REG_ARCHEXT means that the RA state register was set by an AArch64
> +  // DWARF instruction and contains a valid value.
> +  // Return-address signing state is normally toggled by DW_CFA_AARCH64_negate_ra_state
> +  // (also knwon by its alias as DW_CFA_GNU_window_save).
> +  // However, RA state register can be set directly via DW_CFA_val_expression
> +  // too. GCC does not generate such CFI but some other compilers reportedly do.
> +  // (see PR104689 for more details).
> +  // Any other value than REG_ARCHEXT should be interpreted as if the RA state register
> +  // is set by another DWARF instruction, and the value is fetchable via _Unwind_GetGR.
> +  aarch64_RA_signing_method_t signing_method = AARCH64_RA_no_signing;
> +  if (fs->regs.how[reg] == REG_ARCHEXT)
> +    {
> +      signing_method = aarch64_fs_RA_state_get(fs);
> +    }
> +  else if (fs->regs.how[reg] != REG_UNSAVED)
> +    {
> +      signing_method = aarch64_context_RA_state_get(context);
> +
> +      // If the value was set in context, CONTEXT->by_value was set to 1.
> +      // uw_install_context_1 contains an assert on by_value, to check that all
> +      // registers values have been resolved before installing the context.
> +      // This will make the unwinding crash if we don't reset CONTEXT->by_value,
> +      // and CONTEXT->reg[reg].
> +      context->reg[reg] = _Unwind_Get_Unwind_Context_Reg_Val(0x0);
> +      context->by_value[reg] = 0;
> +    }
>   
> -  /* Return-address signing state is toggled by DW_CFA_GNU_window_save (where
> -     REG_UNSAVED/REG_UNSAVED_ARCHEXT means RA signing is disabled/enabled),
> -     or set by a DW_CFA_expression.  */
> -  if (fs->regs.how[reg] == REG_UNSAVED_ARCHEXT
> -      || (_Unwind_GetGR (context, reg) & 0x1) != 0)
> +  if (signing_method == AARCH64_RA_signing_SP)
>       {
>         _Unwind_Word salt = (_Unwind_Word) context->cfa;
> -      if (aarch64_cie_signed_with_b_key (context) != 0)
> +      if (fs->regs.signing_key == AARCH64_PAUTH_KEY_B)
>   	return __builtin_aarch64_autib1716 (addr, salt);
>         return __builtin_aarch64_autia1716 (addr, salt);
>       }
> +  // else {} // signing_method == AARCH64_RA_no_signing, nothing to do.
>   
>     return addr;
>   }
> diff --git a/libgcc/unwind-dw2-execute_cfa.h b/libgcc/unwind-dw2-execute_cfa.h
> index a6b249f9ad6..2ea78c4ef8d 100644
> --- a/libgcc/unwind-dw2-execute_cfa.h
> +++ b/libgcc/unwind-dw2-execute_cfa.h
> @@ -271,23 +271,33 @@
>   	      fs->regs.how[reg] = REG_SAVED_VAL_EXP;
>   	      fs->regs.reg[reg].loc.exp = insn_ptr;
>   	    }
> +	  // Don't execute the expression, but jump over it by adding
> +	  // DW_FORM_block's size to insn_ptr.
>   	  insn_ptr = read_uleb128 (insn_ptr, &utmp);
>   	  insn_ptr += utmp;
>   	  break;
>   
> -	case DW_CFA_GNU_window_save:
>   #if defined (__aarch64__) && !defined (__ILP32__)
> -	  /* This CFA is multiplexed with Sparc.  On AArch64 it's used to toggle
> -	     return address signing status.  REG_UNSAVED/REG_UNSAVED_ARCHEXT
> -	     mean RA signing is disabled/enabled.  */
> -	  reg = DWARF_REGNUM_AARCH64_RA_STATE;
> -	  gcc_assert (fs->regs.how[reg] == REG_UNSAVED
> -		      || fs->regs.how[reg] == REG_UNSAVED_ARCHEXT);
> -	  if (fs->regs.how[reg] == REG_UNSAVED)
> -	    fs->regs.how[reg] = REG_UNSAVED_ARCHEXT;
> -	  else
> -	    fs->regs.how[reg] = REG_UNSAVED;
> +	case DW_CFA_AARCH64_negate_ra_state:
> +	  // This CFA is multiplexed with Sparc.
> +	  // On AArch64 it's used to toggle the status of return address signing
> +	  // with SP as a diversifier.
> +	  // - REG_ARCHEXT means that the RA state register in FS contains a
> +	  // valid value, and that no other DWARF directive has changed the value
> +	  // of this register.
> +	  // - any other value is not compatible with negating the RA state.
> +	  reg = AARCH64_DWARF_REGNUM_RA_STATE;
> +
> +	  // /!\ Mixing DW_CFA_val_expression with DW_CFA_AARCH64_negate_ra_state
> +	  // will result in an undefined behavior (likely an unwinding failure),
> +	  // as the chronology of the DWARF directives will be broken.
> +	  gcc_assert (fs->regs.how[reg] == REG_ARCHEXT);
> +
> +	  // Toggle current state
> +	  aarch64_fs_RA_state_toggle(fs);
> +	  break;
>   #else
> +	case DW_CFA_GNU_window_save:
>   	  /* ??? Hardcoded for SPARC register window configuration.  */
>   	  if (__LIBGCC_DWARF_FRAME_REGISTERS__ >= 32)
>   	    for (reg = 16; reg < 32; ++reg)
> @@ -295,8 +305,8 @@
>   		fs->regs.how[reg] = REG_SAVED_OFFSET;
>   		fs->regs.reg[reg].loc.offset = (reg - 16) * sizeof (void *);
>   	      }
> -#endif
>   	  break;
> +#endif
>   
>   	case DW_CFA_GNU_args_size:
>   	  insn_ptr = read_uleb128 (insn_ptr, &utmp);
> diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
> index 0849e89cd34..fb60853825d 100644
> --- a/libgcc/unwind-dw2.c
> +++ b/libgcc/unwind-dw2.c
> @@ -501,11 +501,11 @@ extract_cie_info (const struct dwarf_cie *cie, struct _Unwind_Context *context,
>   	  fs->signal_frame = 1;
>   	  aug += 1;
>   	}
> -      /* aarch64 B-key pointer authentication.  */
> -      else if (aug[0] == 'B')
> -	{
> -	  aug += 1;
> -      }
> +
> +#if defined(MD_ARCH_EXTENSION_CIE_AUG_HANDLER)
> +      else if (MD_ARCH_EXTENSION_CIE_AUG_HANDLER(fs, aug[0]))
> +	aug += 1;
> +#endif
>   
>         /* Otherwise we have an unknown augmentation string.
>   	 Bail unless we saw a 'z' prefix.  */
> @@ -996,6 +996,9 @@ uw_frame_state_for (struct _Unwind_Context *context, _Unwind_FrameState *fs)
>   
>     memset (&fs->regs.how[0], 0,
>   	  sizeof (*fs) - offsetof (_Unwind_FrameState, regs.how[0]));
> +#if defined(MD_ARCH_EXTENSION_FRAME_INIT)
> +  MD_ARCH_EXTENSION_FRAME_INIT(context, fs);
> +#endif
>     context->args_size = 0;
>     context->lsda = 0;
>   
> @@ -1197,7 +1200,11 @@ uw_update_context_1 (struct _Unwind_Context *context, _Unwind_FrameState *fs)
>         {
>         case REG_UNSAVED:
>         case REG_UNDEFINED:
> -      case REG_UNSAVED_ARCHEXT:
> +      case REG_ARCHEXT: // If the value depends on an augmenter, then there is
> +			// no processing to do here, and the value computation
> +			// should be delayed until the architecture handler
> +			// computes the value correctly based on the augmenter
> +			// information.
>   	break;
>   
>         case REG_SAVED_OFFSET:
> diff --git a/libgcc/unwind-dw2.h b/libgcc/unwind-dw2.h
> index 0dd8611f697..b75f4c65f98 100644
> --- a/libgcc/unwind-dw2.h
> +++ b/libgcc/unwind-dw2.h
> @@ -22,16 +22,17 @@
>      see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>      <http://www.gnu.org/licenses/>.  */
>   
> -enum {
> +enum register_rule
> +{
>     REG_UNSAVED,
>     REG_SAVED_OFFSET,
>     REG_SAVED_REG,
>     REG_SAVED_EXP,
>     REG_SAVED_VAL_OFFSET,
>     REG_SAVED_VAL_EXP,
> -  REG_UNSAVED_ARCHEXT,		/* Target specific extension.  */
> +  REG_ARCHEXT,		/* Target specific extension.  */
>     REG_UNDEFINED
> -};
> +} __attribute__((packed));
>   
>   /* The result of interpreting the frame unwind info for a frame.
>      This is all symbolic at this point, as none of the values can
> @@ -49,7 +50,7 @@ typedef struct
>   	const unsigned char *exp;
>         } loc;
>       } reg[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
> -    unsigned char how[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
> +    enum register_rule how[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
>   
>       enum {
>         CFA_UNSET,
> @@ -65,6 +66,14 @@ typedef struct
>       _Unwind_Sword cfa_offset;
>       _Unwind_Word cfa_reg;
>       const unsigned char *cfa_exp;
> +
> +    /* Architecture extensions information from CIE/FDE.
> +     * Note: those information have to be saved in struct frame_state_reg_info
> +     * instead of _Unwind_FrameState as DW_CFA_restore_state has to be able to
> +     * restore them.  */
> +#if defined(__aarch64__) && !defined (__ILP32__)
> +    unsigned char signing_key;
> +#endif
>     } regs;
>   
>     /* The PC described by the current frame state.  */

Ping.

Re: [PATCH v1 1/3] aarch64: store signing key and signing method in DWARF _Unwind_FrameState

[Richard Sandiford]

Sorry for the slow review.

Matthieu Longo <matthieu.longo@arm.com> writes:
> This patch is only a refactoring of the existing implementation
> of PAuth and returned-address signing. The existing behavior is
> preserved.
>
> _Unwind_FrameState already contains several CIE and FDE information
> (see the attributes below the comment "The information we care
> about from the CIE/FDE" in libgcc/unwind-dw2.h).
> The patch aims at moving the information from DWARF CIE (signing
> key stored in the augmentation string) and FDE (the used signing
> method) into _Unwind_FrameState along the already-stored CIE and
> FDE information.
> Note: those information have to be saved in frame_state_reg_info
> instead of _Unwind_FrameState as they need to be savable by
> DW_CFA_remember_state and restorable by DW_CFA_restore_state, that
> both rely on the attribute "prev".
>
> Those new information in _Unwind_FrameState simplifies the look-up
> of the signing key when the return address is demangled. It also
> allows future signing methods to be easily added.
>
> _Unwind_FrameState is not a part of the public API of libunwind,
> so the change is backward compatible.
>
> A new architecture-specific handler MD_ARCH_EXTENSION_FRAME_INIT
> allows to reset values (if needed) in the frame state and unwind
> context before changing the frame state to the caller context.
>
> A new architecture-specific handler MD_ARCH_EXTENSION_CIE_AUG_HANDLER
> isolates the architecture-specific augmentation strings in AArch64
> backend, and allows others architectures to reuse augmentation
> strings that would have clashed with AArch64 DWARF extensions.
>
> aarch64_demangle_return_addr, DW_CFA_AARCH64_negate_ra_state and
> DW_CFA_val_expression cases in libgcc/unwind-dw2-execute_cfa.h
> were documented to clarify where the value of the RA state register
> is stored (FS and CONTEXT respectively).

The abstraction generally looks good.  My main comment is that,
if we are putting the new behaviour behind target macros (which I
agree is a good idea), could we also have a target macro to abstract:

> +#if defined(__aarch64__) && !defined (__ILP32__)
> +    unsigned char signing_key;
> +#endif

?  E.g. something like:

#ifdef MD_CFI_STATE
    MD_CFI_STATE md;
#endif

with aarch64 defining:

#define MD_CFI_STATE struct { unsigned char signing_key; }

(Names and organisation are just suggestions.)

It might be good to try running the patch through

  contrig/check_GNU_style.py

since the GCC coding standards are quite picky about formatting and
stylistic issues.  The main ones I spotted were:

- In files that already use block comments, all comments should be
  block comments rather than // comments.  The comments should end
  with ".  " (full stop and two spaces).

- Block comments are formatted as:

    /* Line 1
       Line 2.  */

  rather than as:

    /* Line 1
     * Line 2.  */

- Function names are generally all lowrecase (e.g. "ra" rather than "RA").

- In function calls, there should be a space between the function and
  the opening "("

- For pointer types, there should be a space before a "*" (or string of
  "*"s), but no space afterwards.

- "const" qualifiers generally go before the type that they qualify,
  rather than afterwards.

- The line width should be 80 characters or fewer.  (The patch was pretty
  good about this, but there were a couple of long lines).

- In multi-line conditions, the ||s and &&s go at the beginning of lines,
  rather than at the end.  (Same for infix operators in general.)

More detailed comments below.

>
> libgcc/ChangeLog:
>
>   * config/aarch64/aarch64-unwind.h
>   (AARCH64_DWARF_RA_STATE_MASK): The mask for RA state register.
>   (aarch64_RA_signing_method_t): The diversifiers used to sign a
>   function's return address.
>   (aarch64_pointer_auth_key): The key used to sign a function's
>   return address.
>   (aarch64_cie_signed_with_b_key): Deleted as the signing key is
>   available now in _Unwind_FrameState.
>   (MD_ARCH_EXTENSION_CIE_AUG_HANDLER): New CIE augmentation string
>   handler for architecture extensions.
>   (MD_ARCH_EXTENSION_FRAME_INIT): New architecture-extension
>   initialization routine for DWARF frame state and context before
>   execution of DWARF instructions.
>   (aarch64_context_RA_state_get): Read RA state register from CONTEXT.
>   (aarch64_RA_state_get): Read RA state register from FS.
>   (aarch64_RA_state_set): Write RA state register into FS.
>   (aarch64_RA_state_toggle): Toggle RA state register in FS.
>   (aarch64_cie_aug_handler): Handler AArch64 augmentation strings.
>   (aarch64_arch_extension_frame_init): Initialize defaults for the
>   signing key (PAUTH_KEY_A), and RA state register (RA_no_signing).
>   (aarch64_demangle_return_addr): Rely on the frame registers and
>   the signing_key attribute in _Unwind_FrameState.
>   * unwind-dw2-execute_cfa.h:
>   Use the right alias DW_CFA_AARCH64_negate_ra_state for __aarch64__
>   instead of DW_CFA_GNU_window_save.
>   (DW_CFA_AARCH64_negate_ra_state): Save the signing method in RA
>   state register. Toggle RA state register without resetting 'how'
>   to REG_UNSAVED.
>   * unwind-dw2.c:
>   (extract_cie_info): Save the signing key in the current
>   _Unwind_FrameState while parsing the augmentation data.
>   (uw_frame_state_for): Reset some attributes related to architecture
>   extensions in _Unwind_FrameState.
>   (uw_update_context): Move authentication code to AArch64 unwinding.
>   * unwind-dw2.h (enum register_rule): Give a name to the existing
>   enum for the register rules, and replace 'unsigned char' by 'enum
>   register_rule' to facilitate debugging in GDB.
>   (_Unwind_FrameState): Add a new architecture-extension attribute
>   to store the signing key.
> ---
>  libgcc/config/aarch64/aarch64-unwind.h | 154 ++++++++++++++++++++-----
>  libgcc/unwind-dw2-execute_cfa.h        |  34 ++++--
>  libgcc/unwind-dw2.c                    |  19 ++-
>  libgcc/unwind-dw2.h                    |  17 ++-
>  4 files changed, 175 insertions(+), 49 deletions(-)
>
> diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
> index daf96624b5e..cc225a7e207 100644
> --- a/libgcc/config/aarch64/aarch64-unwind.h
> +++ b/libgcc/config/aarch64/aarch64-unwind.h
> @@ -25,55 +25,155 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>  #if !defined (AARCH64_UNWIND_H) && !defined (__ILP32__)
>  #define AARCH64_UNWIND_H
>  
> -#define DWARF_REGNUM_AARCH64_RA_STATE 34
> +#include "ansidecl.h"
> +#include <stdbool.h>
> +
> +#define AARCH64_DWARF_REGNUM_RA_STATE 34
> +#define AARCH64_DWARF_RA_STATE_MASK   0x1
> +
> +/* The diversifiers used to sign a function's return address. */
> +typedef enum
> +{
> +  AARCH64_RA_no_signing = 0x0,
> +  AARCH64_RA_signing_SP = 0x1,
> +} __attribute__((packed)) aarch64_RA_signing_method_t;
> +
> +/* The key used to sign a function's return address. */
> +typedef enum {
> +  AARCH64_PAUTH_KEY_A,
> +  AARCH64_PAUTH_KEY_B,
> +} __attribute__((packed)) aarch64_pointer_auth_key;
> +
> +#define MD_ARCH_EXTENSION_CIE_AUG_HANDLER(fs, aug) \
> +  aarch64_cie_aug_handler(fs, aug)
> +
> +#define MD_ARCH_EXTENSION_FRAME_INIT(context, fs) \
> +  aarch64_arch_extension_frame_init(context, fs)
>  
>  #define MD_DEMANGLE_RETURN_ADDR(context, fs, addr) \
>    aarch64_demangle_return_addr (context, fs, addr)
>  
> -static inline int
> -aarch64_cie_signed_with_b_key (struct _Unwind_Context *context)
> +static inline aarch64_RA_signing_method_t
> +aarch64_context_RA_state_get(struct _Unwind_Context *context)
> +{
> +  const int index = AARCH64_DWARF_REGNUM_RA_STATE;
> +  return _Unwind_GetGR (context, index) & AARCH64_DWARF_RA_STATE_MASK;
> +}
> +
> +static inline aarch64_RA_signing_method_t
> +aarch64_fs_RA_state_get(_Unwind_FrameState const* fs)
>  {
> -  const struct dwarf_fde *fde = _Unwind_Find_FDE (context->bases.func,
> -						  &context->bases);
> -  if (fde != NULL)
> +  const int index = AARCH64_DWARF_REGNUM_RA_STATE;
> +  return fs->regs.reg[index].loc.offset & AARCH64_DWARF_RA_STATE_MASK;
> +}
> +
> +static inline void
> +aarch64_fs_RA_state_set(_Unwind_FrameState *fs,
> +			aarch64_RA_signing_method_t signing_method)
> +{
> +  fs->regs.reg[AARCH64_DWARF_REGNUM_RA_STATE].loc.offset = signing_method;
> +}
> +
> +static inline void
> +aarch64_fs_RA_state_toggle(_Unwind_FrameState *fs)
> +{
> +  aarch64_RA_signing_method_t signing_method = aarch64_fs_RA_state_get(fs);
> +  gcc_assert (signing_method == AARCH64_RA_no_signing ||
> +	      signing_method == AARCH64_RA_signing_SP);
> +  aarch64_fs_RA_state_set(fs, (signing_method == AARCH64_RA_no_signing)
> +			  ? AARCH64_RA_signing_SP
> +			  : AARCH64_RA_no_signing);
> +}
> +
> +/* CIE handler for custom augmentation string.  */
> +static inline bool
> +aarch64_cie_aug_handler(_Unwind_FrameState *fs, unsigned char aug)
> +{
> +  // AArch64 B-key pointer authentication.
> +  if (aug == 'B')
>      {
> -      const struct dwarf_cie *cie = get_cie (fde);
> -      if (cie != NULL)
> -	{
> -	  const unsigned char *aug_str = cie->augmentation;
> -	  return __builtin_strchr ((const char *) aug_str,
> -				   'B') == NULL ? 0 : 1;
> -	}
> +      fs->regs.signing_key = AARCH64_PAUTH_KEY_B;
> +      return true;
>      }
> -  return 0;
> +  return false;
>  }
>  
> -/* Do AArch64 private extraction on ADDR_WORD based on context info CONTEXT and
> -   unwind frame info FS.  If ADDR_WORD is signed, we do address authentication
> -   on it using CFA of current frame.  */
> +/* At the entrance of a new frame, some cached information from the CIE/FDE,
> + * and registers values related to architectural extensions require a default
> + * initialization.
> + * If any of those values related to architecture extensions had to be saved
> + * for the next frame, it should be done via the architecture extensions handler
> + * MD_FROB_UPDATE_CONTEXT in uw_update_context_1 (libgcc/unwind-dw2.c).  */
> +static inline void
> +aarch64_arch_extension_frame_init (struct _Unwind_Context *context ATTRIBUTE_UNUSED,
> +				   _Unwind_FrameState *fs)
> +{
> +  // Reset previously cached CIE/FDE information.
> +  fs->regs.signing_key = AARCH64_PAUTH_KEY_A;

How about making the comment:

  /* By default, DW_CFA_AARCH64_negate_ra_state assumes key A is being used
     for signing.  This can be overridden by adding 'B' to the augmentation
     string.  */

(if you agree that's a reasonable summary).

> +
> +  // Reset some registers.
> +  // Note: the associated fs->how table is automatically reset to REG_UNSAVED in
> +  // uw_frame_state_for (libgcc/unwind-dw2.c). REG_UNSAVED means that whatever
> +  // was in the previous context is the current register value. In other words,
> +  // the next context inherits by default the previous value for a register.
> +  // To keep this logic coherent with some architecture extensions, we need to
> +  // reinitialize fs->how for some registers to REG_ARCHEXT.

How about being a bit more specific about the intent:

  /* All registers are initially in state REG_UNSAVED, which indicates that
     they inherit register values from the previous frame.  However, the
     return address starts every frame in the "unsigned" state.  It also
     starts every frame in a state that supports the original toggle-based
     DW_CFA_AARCH64_negate_ra_state method of controlling RA signing.  */

> +  const int reg = AARCH64_DWARF_REGNUM_RA_STATE;
> +  fs->regs.how[reg] = REG_ARCHEXT;

Very minor, but I think this would be easier to read without the temporary.

> +  aarch64_fs_RA_state_set(fs, AARCH64_RA_no_signing);
> +}
>  
> +/* Do AArch64 private extraction on ADDR_WORD based on context info CONTEXT and
> +   unwind frame info FS. If ADDR_WORD is signed, we do address authentication
> +   on it using CFA of current frame.
> +   Note: this function is called after uw_update_context_1 in uw_update_context.
> +   uw_update_context_1 is the function in which val expression are computed. Thus

expressions

> +   if DW_CFA_val_expression is used, the value of the RA state register is stored
> +   in CONTEXT, not FS. (more details about when DW_CFA_val_expression is used in
> +   the next comment below)  */
>  static inline void *
>  aarch64_demangle_return_addr (struct _Unwind_Context *context,
>  			      _Unwind_FrameState *fs,
>  			      _Unwind_Word addr_word)
>  {
>    void *addr = (void *)addr_word;
> -  const int reg = DWARF_REGNUM_AARCH64_RA_STATE;
> -
> -  if (fs->regs.how[reg] == REG_UNSAVED)
> -    return addr;
> +  const int reg = AARCH64_DWARF_REGNUM_RA_STATE;
> +
> +  // In libgcc, REG_ARCHEXT means that the RA state register was set by an AArch64
> +  // DWARF instruction and contains a valid value.

It's also used to describe the initial state.

> +  // Return-address signing state is normally toggled by DW_CFA_AARCH64_negate_ra_state
> +  // (also knwon by its alias as DW_CFA_GNU_window_save).
> +  // However, RA state register can be set directly via DW_CFA_val_expression
> +  // too. GCC does not generate such CFI but some other compilers reportedly do.

nit: drop the trailing full stop, since the following line continues
the sentence.

> +  // (see PR104689 for more details).
> +  // Any other value than REG_ARCHEXT should be interpreted as if the RA state register
> +  // is set by another DWARF instruction, and the value is fetchable via _Unwind_GetGR.
> +  aarch64_RA_signing_method_t signing_method = AARCH64_RA_no_signing;
> +  if (fs->regs.how[reg] == REG_ARCHEXT)
> +    {
> +      signing_method = aarch64_fs_RA_state_get(fs);
> +    }
> +  else if (fs->regs.how[reg] != REG_UNSAVED)
> +    {
> +      signing_method = aarch64_context_RA_state_get(context);
> +
> +      // If the value was set in context, CONTEXT->by_value was set to 1.
> +      // uw_install_context_1 contains an assert on by_value, to check that all
> +      // registers values have been resolved before installing the context.
> +      // This will make the unwinding crash if we don't reset CONTEXT->by_value,
> +      // and CONTEXT->reg[reg].
> +      context->reg[reg] = _Unwind_Get_Unwind_Context_Reg_Val(0x0);
> +      context->by_value[reg] = 0;

Is this change to the context necessary for the refactor, or is it
instead needed for the follow-on patches?  If it's part of the refactor,
could you explain why it's needed now but wasn't before?

This is the only part that didn't look like a no-op change.

> +    }
>  
> -  /* Return-address signing state is toggled by DW_CFA_GNU_window_save (where
> -     REG_UNSAVED/REG_UNSAVED_ARCHEXT means RA signing is disabled/enabled),
> -     or set by a DW_CFA_expression.  */
> -  if (fs->regs.how[reg] == REG_UNSAVED_ARCHEXT
> -      || (_Unwind_GetGR (context, reg) & 0x1) != 0)
> +  if (signing_method == AARCH64_RA_signing_SP)
>      {
>        _Unwind_Word salt = (_Unwind_Word) context->cfa;
> -      if (aarch64_cie_signed_with_b_key (context) != 0)
> +      if (fs->regs.signing_key == AARCH64_PAUTH_KEY_B)
>  	return __builtin_aarch64_autib1716 (addr, salt);
>        return __builtin_aarch64_autia1716 (addr, salt);
>      }
> +  // else {} // signing_method == AARCH64_RA_no_signing, nothing to do.

IMO it'd be clearer without this line.  Alternatively, we could make it:

  else if (signing_method == AARCH64_RA_no_signing)
    return addr;

  gcc_unreachable ();

>  }
> diff --git a/libgcc/unwind-dw2-execute_cfa.h b/libgcc/unwind-dw2-execute_cfa.h
> index a6b249f9ad6..2ea78c4ef8d 100644
> --- a/libgcc/unwind-dw2-execute_cfa.h
> +++ b/libgcc/unwind-dw2-execute_cfa.h
> @@ -271,23 +271,33 @@
>  	      fs->regs.how[reg] = REG_SAVED_VAL_EXP;
>  	      fs->regs.reg[reg].loc.exp = insn_ptr;
>  	    }
> +	  // Don't execute the expression, but jump over it by adding
> +	  // DW_FORM_block's size to insn_ptr.
>  	  insn_ptr = read_uleb128 (insn_ptr, &utmp);
>  	  insn_ptr += utmp;
>  	  break;
>  
> -	case DW_CFA_GNU_window_save:
>  #if defined (__aarch64__) && !defined (__ILP32__)
> -	  /* This CFA is multiplexed with Sparc.  On AArch64 it's used to toggle
> -	     return address signing status.  REG_UNSAVED/REG_UNSAVED_ARCHEXT
> -	     mean RA signing is disabled/enabled.  */
> -	  reg = DWARF_REGNUM_AARCH64_RA_STATE;
> -	  gcc_assert (fs->regs.how[reg] == REG_UNSAVED
> -		      || fs->regs.how[reg] == REG_UNSAVED_ARCHEXT);
> -	  if (fs->regs.how[reg] == REG_UNSAVED)
> -	    fs->regs.how[reg] = REG_UNSAVED_ARCHEXT;
> -	  else
> -	    fs->regs.how[reg] = REG_UNSAVED;
> +	case DW_CFA_AARCH64_negate_ra_state:
> +	  // This CFA is multiplexed with Sparc.
> +	  // On AArch64 it's used to toggle the status of return address signing
> +	  // with SP as a diversifier.
> +	  // - REG_ARCHEXT means that the RA state register in FS contains a
> +	  // valid value, and that no other DWARF directive has changed the value
> +	  // of this register.
> +	  // - any other value is not compatible with negating the RA state.
> +	  reg = AARCH64_DWARF_REGNUM_RA_STATE;
> +
> +	  // /!\ Mixing DW_CFA_val_expression with DW_CFA_AARCH64_negate_ra_state
> +	  // will result in an undefined behavior (likely an unwinding failure),
> +	  // as the chronology of the DWARF directives will be broken.
> +	  gcc_assert (fs->regs.how[reg] == REG_ARCHEXT);

Could we move the assert into aarch64_fs_RA_state_toggle, or do later
patches not want that?

> +
> +	  // Toggle current state
> +	  aarch64_fs_RA_state_toggle(fs);
> +	  break;
>  #else
> +	case DW_CFA_GNU_window_save:
>  	  /* ??? Hardcoded for SPARC register window configuration.  */
>  	  if (__LIBGCC_DWARF_FRAME_REGISTERS__ >= 32)
>  	    for (reg = 16; reg < 32; ++reg)
> @@ -295,8 +305,8 @@
>  		fs->regs.how[reg] = REG_SAVED_OFFSET;
>  		fs->regs.reg[reg].loc.offset = (reg - 16) * sizeof (void *);
>  	      }
> -#endif
>  	  break;
> +#endif
>  
>  	case DW_CFA_GNU_args_size:
>  	  insn_ptr = read_uleb128 (insn_ptr, &utmp);
> diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
> index 0849e89cd34..fb60853825d 100644
> --- a/libgcc/unwind-dw2.c
> +++ b/libgcc/unwind-dw2.c
> @@ -501,11 +501,11 @@ extract_cie_info (const struct dwarf_cie *cie, struct _Unwind_Context *context,
>  	  fs->signal_frame = 1;
>  	  aug += 1;
>  	}
> -      /* aarch64 B-key pointer authentication.  */
> -      else if (aug[0] == 'B')
> -	{
> -	  aug += 1;
> -      }
> +
> +#if defined(MD_ARCH_EXTENSION_CIE_AUG_HANDLER)
> +      else if (MD_ARCH_EXTENSION_CIE_AUG_HANDLER(fs, aug[0]))
> +	aug += 1;
> +#endif
>  
>        /* Otherwise we have an unknown augmentation string.
>  	 Bail unless we saw a 'z' prefix.  */
> @@ -996,6 +996,9 @@ uw_frame_state_for (struct _Unwind_Context *context, _Unwind_FrameState *fs)
>  
>    memset (&fs->regs.how[0], 0,
>  	  sizeof (*fs) - offsetof (_Unwind_FrameState, regs.how[0]));
> +#if defined(MD_ARCH_EXTENSION_FRAME_INIT)
> +  MD_ARCH_EXTENSION_FRAME_INIT(context, fs);
> +#endif
>    context->args_size = 0;
>    context->lsda = 0;
>  
> @@ -1197,7 +1200,11 @@ uw_update_context_1 (struct _Unwind_Context *context, _Unwind_FrameState *fs)
>        {
>        case REG_UNSAVED:
>        case REG_UNDEFINED:
> -      case REG_UNSAVED_ARCHEXT:
> +      case REG_ARCHEXT: // If the value depends on an augmenter, then there is
> +			// no processing to do here, and the value computation
> +			// should be delayed until the architecture handler
> +			// computes the value correctly based on the augmenter
> +			// information.

This would more usually be written as a comment above the case, rather than
to the right.

>  	break;
>  
>        case REG_SAVED_OFFSET:
> diff --git a/libgcc/unwind-dw2.h b/libgcc/unwind-dw2.h
> index 0dd8611f697..b75f4c65f98 100644
> --- a/libgcc/unwind-dw2.h
> +++ b/libgcc/unwind-dw2.h
> @@ -22,16 +22,17 @@
>     see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -enum {
> +enum register_rule
> +{
>    REG_UNSAVED,
>    REG_SAVED_OFFSET,
>    REG_SAVED_REG,
>    REG_SAVED_EXP,
>    REG_SAVED_VAL_OFFSET,
>    REG_SAVED_VAL_EXP,
> -  REG_UNSAVED_ARCHEXT,		/* Target specific extension.  */
> +  REG_ARCHEXT,		/* Target specific extension.  */
>    REG_UNDEFINED
> -};
> +} __attribute__((packed));
>  
>  /* The result of interpreting the frame unwind info for a frame.
>     This is all symbolic at this point, as none of the values can
> @@ -49,7 +50,7 @@ typedef struct
>  	const unsigned char *exp;
>        } loc;
>      } reg[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
> -    unsigned char how[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
> +    enum register_rule how[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
>  
>      enum {
>        CFA_UNSET,
> @@ -65,6 +66,14 @@ typedef struct
>      _Unwind_Sword cfa_offset;
>      _Unwind_Word cfa_reg;
>      const unsigned char *cfa_exp;
> +
> +    /* Architecture extensions information from CIE/FDE.
> +     * Note: those information have to be saved in struct frame_state_reg_info

this information (singular)

Thanks for doing this.  It's definitely a cleaner structure than what
we had before.

Richard

> +     * instead of _Unwind_FrameState as DW_CFA_restore_state has to be able to
> +     * restore them.  */
> +#if defined(__aarch64__) && !defined (__ILP32__)
> +    unsigned char signing_key;
> +#endif
>    } regs;
>  
>    /* The PC described by the current frame state.  */

Re: [PATCH v1 1/3] aarch64: store signing key and signing method in DWARF _Unwind_FrameState

[Matthieu Longo]

On 2024-08-06 11:06, Richard Sandiford wrote:
> Sorry for the slow review.
> 
> Matthieu Longo <matthieu.longo@arm.com> writes:
>> This patch is only a refactoring of the existing implementation
>> of PAuth and returned-address signing. The existing behavior is
>> preserved.
>>
>> _Unwind_FrameState already contains several CIE and FDE information
>> (see the attributes below the comment "The information we care
>> about from the CIE/FDE" in libgcc/unwind-dw2.h).
>> The patch aims at moving the information from DWARF CIE (signing
>> key stored in the augmentation string) and FDE (the used signing
>> method) into _Unwind_FrameState along the already-stored CIE and
>> FDE information.
>> Note: those information have to be saved in frame_state_reg_info
>> instead of _Unwind_FrameState as they need to be savable by
>> DW_CFA_remember_state and restorable by DW_CFA_restore_state, that
>> both rely on the attribute "prev".
>>
>> Those new information in _Unwind_FrameState simplifies the look-up
>> of the signing key when the return address is demangled. It also
>> allows future signing methods to be easily added.
>>
>> _Unwind_FrameState is not a part of the public API of libunwind,
>> so the change is backward compatible.
>>
>> A new architecture-specific handler MD_ARCH_EXTENSION_FRAME_INIT
>> allows to reset values (if needed) in the frame state and unwind
>> context before changing the frame state to the caller context.
>>
>> A new architecture-specific handler MD_ARCH_EXTENSION_CIE_AUG_HANDLER
>> isolates the architecture-specific augmentation strings in AArch64
>> backend, and allows others architectures to reuse augmentation
>> strings that would have clashed with AArch64 DWARF extensions.
>>
>> aarch64_demangle_return_addr, DW_CFA_AARCH64_negate_ra_state and
>> DW_CFA_val_expression cases in libgcc/unwind-dw2-execute_cfa.h
>> were documented to clarify where the value of the RA state register
>> is stored (FS and CONTEXT respectively).
> 
> The abstraction generally looks good.  My main comment is that,
> if we are putting the new behaviour behind target macros (which I
> agree is a good idea), could we also have a target macro to abstract:
> 
>> +#if defined(__aarch64__) && !defined (__ILP32__)
>> +    unsigned char signing_key;
>> +#endif
> 
> ?  E.g. something like:
> 
> #ifdef MD_CFI_STATE
>      MD_CFI_STATE md;
> #endif
> 
> with aarch64 defining:
> 
> #define MD_CFI_STATE struct { unsigned char signing_key; }
> 
> (Names and organisation are just suggestions.)
> 
> It might be good to try running the patch through
> 
>    contrig/check_GNU_style.py
> 
> since the GCC coding standards are quite picky about formatting and
> stylistic issues.  The main ones I spotted were:
> 
> - In files that already use block comments, all comments should be
>    block comments rather than // comments.  The comments should end
>    with ".  " (full stop and two spaces).
> 
> - Block comments are formatted as:
> 
>      /* Line 1
>         Line 2.  */
> 
>    rather than as:
> 
>      /* Line 1
>       * Line 2.  */
> 
> - Function names are generally all lowrecase (e.g. "ra" rather than "RA").
> 
> - In function calls, there should be a space between the function and
>    the opening "("
> 
> - For pointer types, there should be a space before a "*" (or string of
>    "*"s), but no space afterwards.
> 
> - "const" qualifiers generally go before the type that they qualify,
>    rather than afterwards.
> 
> - The line width should be 80 characters or fewer.  (The patch was pretty
>    good about this, but there were a couple of long lines).
> 
> - In multi-line conditions, the ||s and &&s go at the beginning of lines,
>    rather than at the end.  (Same for infix operators in general.)
> 
> More detailed comments below.
> 
>>
>> libgcc/ChangeLog:
>>
>>    * config/aarch64/aarch64-unwind.h
>>    (AARCH64_DWARF_RA_STATE_MASK): The mask for RA state register.
>>    (aarch64_RA_signing_method_t): The diversifiers used to sign a
>>    function's return address.
>>    (aarch64_pointer_auth_key): The key used to sign a function's
>>    return address.
>>    (aarch64_cie_signed_with_b_key): Deleted as the signing key is
>>    available now in _Unwind_FrameState.
>>    (MD_ARCH_EXTENSION_CIE_AUG_HANDLER): New CIE augmentation string
>>    handler for architecture extensions.
>>    (MD_ARCH_EXTENSION_FRAME_INIT): New architecture-extension
>>    initialization routine for DWARF frame state and context before
>>    execution of DWARF instructions.
>>    (aarch64_context_RA_state_get): Read RA state register from CONTEXT.
>>    (aarch64_RA_state_get): Read RA state register from FS.
>>    (aarch64_RA_state_set): Write RA state register into FS.
>>    (aarch64_RA_state_toggle): Toggle RA state register in FS.
>>    (aarch64_cie_aug_handler): Handler AArch64 augmentation strings.
>>    (aarch64_arch_extension_frame_init): Initialize defaults for the
>>    signing key (PAUTH_KEY_A), and RA state register (RA_no_signing).
>>    (aarch64_demangle_return_addr): Rely on the frame registers and
>>    the signing_key attribute in _Unwind_FrameState.
>>    * unwind-dw2-execute_cfa.h:
>>    Use the right alias DW_CFA_AARCH64_negate_ra_state for __aarch64__
>>    instead of DW_CFA_GNU_window_save.
>>    (DW_CFA_AARCH64_negate_ra_state): Save the signing method in RA
>>    state register. Toggle RA state register without resetting 'how'
>>    to REG_UNSAVED.
>>    * unwind-dw2.c:
>>    (extract_cie_info): Save the signing key in the current
>>    _Unwind_FrameState while parsing the augmentation data.
>>    (uw_frame_state_for): Reset some attributes related to architecture
>>    extensions in _Unwind_FrameState.
>>    (uw_update_context): Move authentication code to AArch64 unwinding.
>>    * unwind-dw2.h (enum register_rule): Give a name to the existing
>>    enum for the register rules, and replace 'unsigned char' by 'enum
>>    register_rule' to facilitate debugging in GDB.
>>    (_Unwind_FrameState): Add a new architecture-extension attribute
>>    to store the signing key.
>> ---
>>   libgcc/config/aarch64/aarch64-unwind.h | 154 ++++++++++++++++++++-----
>>   libgcc/unwind-dw2-execute_cfa.h        |  34 ++++--
>>   libgcc/unwind-dw2.c                    |  19 ++-
>>   libgcc/unwind-dw2.h                    |  17 ++-
>>   4 files changed, 175 insertions(+), 49 deletions(-)
>>
>> diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
>> index daf96624b5e..cc225a7e207 100644
>> --- a/libgcc/config/aarch64/aarch64-unwind.h
>> +++ b/libgcc/config/aarch64/aarch64-unwind.h
>> @@ -25,55 +25,155 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>>   #if !defined (AARCH64_UNWIND_H) && !defined (__ILP32__)
>>   #define AARCH64_UNWIND_H
>>   
>> -#define DWARF_REGNUM_AARCH64_RA_STATE 34
>> +#include "ansidecl.h"
>> +#include <stdbool.h>
>> +
>> +#define AARCH64_DWARF_REGNUM_RA_STATE 34
>> +#define AARCH64_DWARF_RA_STATE_MASK   0x1
>> +
>> +/* The diversifiers used to sign a function's return address. */
>> +typedef enum
>> +{
>> +  AARCH64_RA_no_signing = 0x0,
>> +  AARCH64_RA_signing_SP = 0x1,
>> +} __attribute__((packed)) aarch64_RA_signing_method_t;
>> +
>> +/* The key used to sign a function's return address. */
>> +typedef enum {
>> +  AARCH64_PAUTH_KEY_A,
>> +  AARCH64_PAUTH_KEY_B,
>> +} __attribute__((packed)) aarch64_pointer_auth_key;
>> +
>> +#define MD_ARCH_EXTENSION_CIE_AUG_HANDLER(fs, aug) \
>> +  aarch64_cie_aug_handler(fs, aug)
>> +
>> +#define MD_ARCH_EXTENSION_FRAME_INIT(context, fs) \
>> +  aarch64_arch_extension_frame_init(context, fs)
>>   
>>   #define MD_DEMANGLE_RETURN_ADDR(context, fs, addr) \
>>     aarch64_demangle_return_addr (context, fs, addr)
>>   
>> -static inline int
>> -aarch64_cie_signed_with_b_key (struct _Unwind_Context *context)
>> +static inline aarch64_RA_signing_method_t
>> +aarch64_context_RA_state_get(struct _Unwind_Context *context)
>> +{
>> +  const int index = AARCH64_DWARF_REGNUM_RA_STATE;
>> +  return _Unwind_GetGR (context, index) & AARCH64_DWARF_RA_STATE_MASK;
>> +}
>> +
>> +static inline aarch64_RA_signing_method_t
>> +aarch64_fs_RA_state_get(_Unwind_FrameState const* fs)
>>   {
>> -  const struct dwarf_fde *fde = _Unwind_Find_FDE (context->bases.func,
>> -						  &context->bases);
>> -  if (fde != NULL)
>> +  const int index = AARCH64_DWARF_REGNUM_RA_STATE;
>> +  return fs->regs.reg[index].loc.offset & AARCH64_DWARF_RA_STATE_MASK;
>> +}
>> +
>> +static inline void
>> +aarch64_fs_RA_state_set(_Unwind_FrameState *fs,
>> +			aarch64_RA_signing_method_t signing_method)
>> +{
>> +  fs->regs.reg[AARCH64_DWARF_REGNUM_RA_STATE].loc.offset = signing_method;
>> +}
>> +
>> +static inline void
>> +aarch64_fs_RA_state_toggle(_Unwind_FrameState *fs)
>> +{
>> +  aarch64_RA_signing_method_t signing_method = aarch64_fs_RA_state_get(fs);
>> +  gcc_assert (signing_method == AARCH64_RA_no_signing ||
>> +	      signing_method == AARCH64_RA_signing_SP);
>> +  aarch64_fs_RA_state_set(fs, (signing_method == AARCH64_RA_no_signing)
>> +			  ? AARCH64_RA_signing_SP
>> +			  : AARCH64_RA_no_signing);
>> +}
>> +
>> +/* CIE handler for custom augmentation string.  */
>> +static inline bool
>> +aarch64_cie_aug_handler(_Unwind_FrameState *fs, unsigned char aug)
>> +{
>> +  // AArch64 B-key pointer authentication.
>> +  if (aug == 'B')
>>       {
>> -      const struct dwarf_cie *cie = get_cie (fde);
>> -      if (cie != NULL)
>> -	{
>> -	  const unsigned char *aug_str = cie->augmentation;
>> -	  return __builtin_strchr ((const char *) aug_str,
>> -				   'B') == NULL ? 0 : 1;
>> -	}
>> +      fs->regs.signing_key = AARCH64_PAUTH_KEY_B;
>> +      return true;
>>       }
>> -  return 0;
>> +  return false;
>>   }
>>   
>> -/* Do AArch64 private extraction on ADDR_WORD based on context info CONTEXT and
>> -   unwind frame info FS.  If ADDR_WORD is signed, we do address authentication
>> -   on it using CFA of current frame.  */
>> +/* At the entrance of a new frame, some cached information from the CIE/FDE,
>> + * and registers values related to architectural extensions require a default
>> + * initialization.
>> + * If any of those values related to architecture extensions had to be saved
>> + * for the next frame, it should be done via the architecture extensions handler
>> + * MD_FROB_UPDATE_CONTEXT in uw_update_context_1 (libgcc/unwind-dw2.c).  */
>> +static inline void
>> +aarch64_arch_extension_frame_init (struct _Unwind_Context *context ATTRIBUTE_UNUSED,
>> +				   _Unwind_FrameState *fs)
>> +{
>> +  // Reset previously cached CIE/FDE information.
>> +  fs->regs.signing_key = AARCH64_PAUTH_KEY_A;
> 
> How about making the comment:
> 
>    /* By default, DW_CFA_AARCH64_negate_ra_state assumes key A is being used
>       for signing.  This can be overridden by adding 'B' to the augmentation
>       string.  */
> 
> (if you agree that's a reasonable summary).
> 
>> +
>> +  // Reset some registers.
>> +  // Note: the associated fs->how table is automatically reset to REG_UNSAVED in
>> +  // uw_frame_state_for (libgcc/unwind-dw2.c). REG_UNSAVED means that whatever
>> +  // was in the previous context is the current register value. In other words,
>> +  // the next context inherits by default the previous value for a register.
>> +  // To keep this logic coherent with some architecture extensions, we need to
>> +  // reinitialize fs->how for some registers to REG_ARCHEXT.
> 
> How about being a bit more specific about the intent:
> 
>    /* All registers are initially in state REG_UNSAVED, which indicates that
>       they inherit register values from the previous frame.  However, the
>       return address starts every frame in the "unsigned" state.  It also
>       starts every frame in a state that supports the original toggle-based
>       DW_CFA_AARCH64_negate_ra_state method of controlling RA signing.  */
> 
>> +  const int reg = AARCH64_DWARF_REGNUM_RA_STATE;
>> +  fs->regs.how[reg] = REG_ARCHEXT;
> 
> Very minor, but I think this would be easier to read without the temporary.
> 
>> +  aarch64_fs_RA_state_set(fs, AARCH64_RA_no_signing);
>> +}
>>   
>> +/* Do AArch64 private extraction on ADDR_WORD based on context info CONTEXT and
>> +   unwind frame info FS. If ADDR_WORD is signed, we do address authentication
>> +   on it using CFA of current frame.
>> +   Note: this function is called after uw_update_context_1 in uw_update_context.
>> +   uw_update_context_1 is the function in which val expression are computed. Thus
> 
> expressions
> 
>> +   if DW_CFA_val_expression is used, the value of the RA state register is stored
>> +   in CONTEXT, not FS. (more details about when DW_CFA_val_expression is used in
>> +   the next comment below)  */
>>   static inline void *
>>   aarch64_demangle_return_addr (struct _Unwind_Context *context,
>>   			      _Unwind_FrameState *fs,
>>   			      _Unwind_Word addr_word)
>>   {
>>     void *addr = (void *)addr_word;
>> -  const int reg = DWARF_REGNUM_AARCH64_RA_STATE;
>> -
>> -  if (fs->regs.how[reg] == REG_UNSAVED)
>> -    return addr;
>> +  const int reg = AARCH64_DWARF_REGNUM_RA_STATE;
>> +
>> +  // In libgcc, REG_ARCHEXT means that the RA state register was set by an AArch64
>> +  // DWARF instruction and contains a valid value.
> 
> It's also used to describe the initial state.
> 
>> +  // Return-address signing state is normally toggled by DW_CFA_AARCH64_negate_ra_state
>> +  // (also knwon by its alias as DW_CFA_GNU_window_save).
>> +  // However, RA state register can be set directly via DW_CFA_val_expression
>> +  // too. GCC does not generate such CFI but some other compilers reportedly do.
> 
> nit: drop the trailing full stop, since the following line continues
> the sentence.
> 
>> +  // (see PR104689 for more details).
>> +  // Any other value than REG_ARCHEXT should be interpreted as if the RA state register
>> +  // is set by another DWARF instruction, and the value is fetchable via _Unwind_GetGR.
>> +  aarch64_RA_signing_method_t signing_method = AARCH64_RA_no_signing;
>> +  if (fs->regs.how[reg] == REG_ARCHEXT)
>> +    {
>> +      signing_method = aarch64_fs_RA_state_get(fs);
>> +    }
>> +  else if (fs->regs.how[reg] != REG_UNSAVED)
>> +    {
>> +      signing_method = aarch64_context_RA_state_get(context);
>> +
>> +      // If the value was set in context, CONTEXT->by_value was set to 1.
>> +      // uw_install_context_1 contains an assert on by_value, to check that all
>> +      // registers values have been resolved before installing the context.
>> +      // This will make the unwinding crash if we don't reset CONTEXT->by_value,
>> +      // and CONTEXT->reg[reg].
>> +      context->reg[reg] = _Unwind_Get_Unwind_Context_Reg_Val(0x0);
>> +      context->by_value[reg] = 0;
> 
> Is this change to the context necessary for the refactor, or is it
> instead needed for the follow-on patches?  If it's part of the refactor,
> could you explain why it's needed now but wasn't before?
> 
> This is the only part that didn't look like a no-op change.
>

The Cranelift project decided to set the RA state register with 
DW_CFA_val_expression instead of DW_CFA_AARCH64_negate_ra_state.
Something like:
const RA_SIGN_STATE := 34
DW_CFA_val_expression, RA_SIGN_STATE, 1, DW_OP_lit1

When the value is set in CONTEXT, CONTEXT->by_value is also set to 1.
uw_install_context_1 contains an assert on by_value.
The restoration of the context has apparently never been tested as it 
triggers the assert. There is only one test [1] in the GCC testsuite 
that tests the backtracing.

[1]: gcc/testsuite/gcc.target/aarch64/pr104689.c

The idea of the fix was to reset the RA state register, as it is a local 
frame register, and does not need to be copied into the target frame.

I discussed offline this issue with Richard Sandiford, and here is his 
explanation of why there is this assertion in uw_install_context_1.

** begin quotation **

I think the reason for the assert is that the current frame passed to 
uw_install_context_1 is always the function that calls 
__builtin_eh_return, such as _Unwind_RaiseException. That function is 
supposed to create save slots on the stack for each register whose 
contents need to be tracked and unwound (generally all call-preserved 
registers). So I suppose it was a reasonable assumption that that frame 
would never have registers stored by value, since that would prevent the 
register values from being unwound properly.  And the current code 
wouldn't do something sensible for current->by_value[i] != 0.

Here we're using a register to track a property of a frame, rather than 
to track registers between frames. In that sense it's a new use case.

Personally I think it'd be reasonable for the loop in 
uw_install_context_1 to continue on current->by_value[i] (with a 
comment). That seems preferable to changing the register entry to avoid 
the assert. I don't think the loop in uw_install_context_1 should even 
be trying to copy the RA signing state, although I suppose it doesn't 
matter much either way.

** end quotation **

As recommended by Richard, I added a handler MD_FRAME_LOCAL_REGISTER_P 
(in the next revision) to explicitly skip the RA state register from the 
context copy.

>> +    }
>>   
>> -  /* Return-address signing state is toggled by DW_CFA_GNU_window_save (where
>> -     REG_UNSAVED/REG_UNSAVED_ARCHEXT means RA signing is disabled/enabled),
>> -     or set by a DW_CFA_expression.  */
>> -  if (fs->regs.how[reg] == REG_UNSAVED_ARCHEXT
>> -      || (_Unwind_GetGR (context, reg) & 0x1) != 0)
>> +  if (signing_method == AARCH64_RA_signing_SP)
>>       {
>>         _Unwind_Word salt = (_Unwind_Word) context->cfa;
>> -      if (aarch64_cie_signed_with_b_key (context) != 0)
>> +      if (fs->regs.signing_key == AARCH64_PAUTH_KEY_B)
>>   	return __builtin_aarch64_autib1716 (addr, salt);
>>         return __builtin_aarch64_autia1716 (addr, salt);
>>       }
>> +  // else {} // signing_method == AARCH64_RA_no_signing, nothing to do.
> 
> IMO it'd be clearer without this line.  Alternatively, we could make it:
> 
>    else if (signing_method == AARCH64_RA_no_signing)
>      return addr;
> 
>    gcc_unreachable ();
> 
>>   }
>> diff --git a/libgcc/unwind-dw2-execute_cfa.h b/libgcc/unwind-dw2-execute_cfa.h
>> index a6b249f9ad6..2ea78c4ef8d 100644
>> --- a/libgcc/unwind-dw2-execute_cfa.h
>> +++ b/libgcc/unwind-dw2-execute_cfa.h
>> @@ -271,23 +271,33 @@
>>   	      fs->regs.how[reg] = REG_SAVED_VAL_EXP;
>>   	      fs->regs.reg[reg].loc.exp = insn_ptr;
>>   	    }
>> +	  // Don't execute the expression, but jump over it by adding
>> +	  // DW_FORM_block's size to insn_ptr.
>>   	  insn_ptr = read_uleb128 (insn_ptr, &utmp);
>>   	  insn_ptr += utmp;
>>   	  break;
>>   
>> -	case DW_CFA_GNU_window_save:
>>   #if defined (__aarch64__) && !defined (__ILP32__)
>> -	  /* This CFA is multiplexed with Sparc.  On AArch64 it's used to toggle
>> -	     return address signing status.  REG_UNSAVED/REG_UNSAVED_ARCHEXT
>> -	     mean RA signing is disabled/enabled.  */
>> -	  reg = DWARF_REGNUM_AARCH64_RA_STATE;
>> -	  gcc_assert (fs->regs.how[reg] == REG_UNSAVED
>> -		      || fs->regs.how[reg] == REG_UNSAVED_ARCHEXT);
>> -	  if (fs->regs.how[reg] == REG_UNSAVED)
>> -	    fs->regs.how[reg] = REG_UNSAVED_ARCHEXT;
>> -	  else
>> -	    fs->regs.how[reg] = REG_UNSAVED;
>> +	case DW_CFA_AARCH64_negate_ra_state:
>> +	  // This CFA is multiplexed with Sparc.
>> +	  // On AArch64 it's used to toggle the status of return address signing
>> +	  // with SP as a diversifier.
>> +	  // - REG_ARCHEXT means that the RA state register in FS contains a
>> +	  // valid value, and that no other DWARF directive has changed the value
>> +	  // of this register.
>> +	  // - any other value is not compatible with negating the RA state.
>> +	  reg = AARCH64_DWARF_REGNUM_RA_STATE;
>> +
>> +	  // /!\ Mixing DW_CFA_val_expression with DW_CFA_AARCH64_negate_ra_state
>> +	  // will result in an undefined behavior (likely an unwinding failure),
>> +	  // as the chronology of the DWARF directives will be broken.
>> +	  gcc_assert (fs->regs.how[reg] == REG_ARCHEXT);
> 
> Could we move the assert into aarch64_fs_RA_state_toggle, or do later
> patches not want that?
> 
>> +
>> +	  // Toggle current state
>> +	  aarch64_fs_RA_state_toggle(fs);
>> +	  break;
>>   #else
>> +	case DW_CFA_GNU_window_save:
>>   	  /* ??? Hardcoded for SPARC register window configuration.  */
>>   	  if (__LIBGCC_DWARF_FRAME_REGISTERS__ >= 32)
>>   	    for (reg = 16; reg < 32; ++reg)
>> @@ -295,8 +305,8 @@
>>   		fs->regs.how[reg] = REG_SAVED_OFFSET;
>>   		fs->regs.reg[reg].loc.offset = (reg - 16) * sizeof (void *);
>>   	      }
>> -#endif
>>   	  break;
>> +#endif
>>   
>>   	case DW_CFA_GNU_args_size:
>>   	  insn_ptr = read_uleb128 (insn_ptr, &utmp);
>> diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
>> index 0849e89cd34..fb60853825d 100644
>> --- a/libgcc/unwind-dw2.c
>> +++ b/libgcc/unwind-dw2.c
>> @@ -501,11 +501,11 @@ extract_cie_info (const struct dwarf_cie *cie, struct _Unwind_Context *context,
>>   	  fs->signal_frame = 1;
>>   	  aug += 1;
>>   	}
>> -      /* aarch64 B-key pointer authentication.  */
>> -      else if (aug[0] == 'B')
>> -	{
>> -	  aug += 1;
>> -      }
>> +
>> +#if defined(MD_ARCH_EXTENSION_CIE_AUG_HANDLER)
>> +      else if (MD_ARCH_EXTENSION_CIE_AUG_HANDLER(fs, aug[0]))
>> +	aug += 1;
>> +#endif
>>   
>>         /* Otherwise we have an unknown augmentation string.
>>   	 Bail unless we saw a 'z' prefix.  */
>> @@ -996,6 +996,9 @@ uw_frame_state_for (struct _Unwind_Context *context, _Unwind_FrameState *fs)
>>   
>>     memset (&fs->regs.how[0], 0,
>>   	  sizeof (*fs) - offsetof (_Unwind_FrameState, regs.how[0]));
>> +#if defined(MD_ARCH_EXTENSION_FRAME_INIT)
>> +  MD_ARCH_EXTENSION_FRAME_INIT(context, fs);
>> +#endif
>>     context->args_size = 0;
>>     context->lsda = 0;
>>   
>> @@ -1197,7 +1200,11 @@ uw_update_context_1 (struct _Unwind_Context *context, _Unwind_FrameState *fs)
>>         {
>>         case REG_UNSAVED:
>>         case REG_UNDEFINED:
>> -      case REG_UNSAVED_ARCHEXT:
>> +      case REG_ARCHEXT: // If the value depends on an augmenter, then there is
>> +			// no processing to do here, and the value computation
>> +			// should be delayed until the architecture handler
>> +			// computes the value correctly based on the augmenter
>> +			// information.
> 
> This would more usually be written as a comment above the case, rather than
> to the right.
> 
>>   	break;
>>   
>>         case REG_SAVED_OFFSET:
>> diff --git a/libgcc/unwind-dw2.h b/libgcc/unwind-dw2.h
>> index 0dd8611f697..b75f4c65f98 100644
>> --- a/libgcc/unwind-dw2.h
>> +++ b/libgcc/unwind-dw2.h
>> @@ -22,16 +22,17 @@
>>      see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>>      <http://www.gnu.org/licenses/>.  */
>>   
>> -enum {
>> +enum register_rule
>> +{
>>     REG_UNSAVED,
>>     REG_SAVED_OFFSET,
>>     REG_SAVED_REG,
>>     REG_SAVED_EXP,
>>     REG_SAVED_VAL_OFFSET,
>>     REG_SAVED_VAL_EXP,
>> -  REG_UNSAVED_ARCHEXT,		/* Target specific extension.  */
>> +  REG_ARCHEXT,		/* Target specific extension.  */
>>     REG_UNDEFINED
>> -};
>> +} __attribute__((packed));
>>   
>>   /* The result of interpreting the frame unwind info for a frame.
>>      This is all symbolic at this point, as none of the values can
>> @@ -49,7 +50,7 @@ typedef struct
>>   	const unsigned char *exp;
>>         } loc;
>>       } reg[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
>> -    unsigned char how[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
>> +    enum register_rule how[__LIBGCC_DWARF_FRAME_REGISTERS__+1];
>>   
>>       enum {
>>         CFA_UNSET,
>> @@ -65,6 +66,14 @@ typedef struct
>>       _Unwind_Sword cfa_offset;
>>       _Unwind_Word cfa_reg;
>>       const unsigned char *cfa_exp;
>> +
>> +    /* Architecture extensions information from CIE/FDE.
>> +     * Note: those information have to be saved in struct frame_state_reg_info
> 
> this information (singular)
> 
> Thanks for doing this.  It's definitely a cleaner structure than what
> we had before.
> 
> Richard
> 
>> +     * instead of _Unwind_FrameState as DW_CFA_restore_state has to be able to
>> +     * restore them.  */
>> +#if defined(__aarch64__) && !defined (__ILP32__)
>> +    unsigned char signing_key;
>> +#endif
>>     } regs;
>>   
>>     /* The PC described by the current frame state.  */

All the comments above were addressed in the next revision.

[PATCH v1 2/3] libgcc: hide CIE and FDE data for DWARF architecture extensions behind a handler.

[Matthieu Longo]

This patch provides a new handler MD_ARCH_FRAME_STATE_T to hide an
architecture-specific structure containing CIE and FDE data related
to DWARF architecture extensions.

Hiding the architecture-specific attributes behind a handler has the
following benefits:
1. isolating those data from the generic ones in _Unwind_FrameState
2. avoiding casts to custom types.
3. preserving typing information when debugging with GDB, and so
   facilitating their printing.

This approach required to add a new header md-unwind-def.h included at
the top of libgcc/unwind-dw2.h, and redirecting to the corresponding
architecture header via a symbolic link.

An obvious drawback is the increase in complexity with macros, and
headers. It also caused a split of architecture definitions between
md-unwind-def.h (types definitions used in unwind-dw2.h) and
md-unwind.h (local types definitions and handlers implementations).
The naming of md-unwind.h with .h extension is a bit misleading as
the file is only included in the middle of unwind-dw2.c. Changing
this naming would require modification of others backends, which I
prefered to abstain from. Overall the benefits are worth the added
complexity from my perspective.

libgcc/ChangeLog:

    * Makefile.in: New target for symbolic link to md-unwind-def.h
    * config.host: New parameter md_unwind_def_header. Set it to
    aarch64/aarch64-unwind-def.h for AArch64 targets, or no-unwind.h
    by default.
    * config/aarch64/aarch64-unwind.h
    (aarch64_pointer_auth_key): Move to aarch64-unwind-def.h
    (aarch64_cie_aug_handler): Update.
    (aarch64_arch_extension_frame_init): Update.
    (aarch64_demangle_return_addr): Update.
    * configure.ac: New substitute variable md_unwind_def_header.
    * unwind-dw2.h (defined): MD_ARCH_FRAME_STATE_T.
    * config/aarch64/aarch64-unwind-def.h: New file.
---
 libgcc/Makefile.in                         |  6 +++-
 libgcc/config.host                         | 13 +++++--
 libgcc/config/aarch64/aarch64-unwind-def.h | 41 ++++++++++++++++++++++
 libgcc/config/aarch64/aarch64-unwind.h     | 14 +++-----
 libgcc/configure.ac                        |  1 +
 libgcc/unwind-dw2.h                        |  6 ++--
 6 files changed, 67 insertions(+), 14 deletions(-)
 create mode 100644 libgcc/config/aarch64/aarch64-unwind-def.h

diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in
index 0e46e9ef768..ffc45f21267 100644
--- a/libgcc/Makefile.in
+++ b/libgcc/Makefile.in
@@ -47,6 +47,7 @@ with_aix_soname = @with_aix_soname@
 solaris_ld_v2_maps = @solaris_ld_v2_maps@
 enable_execute_stack = @enable_execute_stack@
 unwind_header = @unwind_header@
+md_unwind_def_header = @md_unwind_def_header@
 md_unwind_header = @md_unwind_header@
 sfp_machine_header = @sfp_machine_header@
 thread_header = @thread_header@
@@ -358,13 +359,16 @@ SHLIBUNWIND_INSTALL =
 
 
 # Create links to files specified in config.host.
-LIBGCC_LINKS = enable-execute-stack.c unwind.h md-unwind-support.h \
+LIBGCC_LINKS = enable-execute-stack.c \
+               unwind.h md-unwind-def.h md-unwind-support.h \
                sfp-machine.h gthr-default.h
 
 enable-execute-stack.c: $(srcdir)/$(enable_execute_stack)
 	-$(LN_S) $< $@
 unwind.h: $(srcdir)/$(unwind_header)
 	-$(LN_S) $< $@
+md-unwind-def.h: $(srcdir)/config/$(md_unwind_def_header)
+	-$(LN_S) $< $@
 md-unwind-support.h: $(srcdir)/config/$(md_unwind_header)
 	-$(LN_S) $< $@
 sfp-machine.h: $(srcdir)/config/$(sfp_machine_header)
diff --git a/libgcc/config.host b/libgcc/config.host
index 9fae51d4ce7..61825e72fe4 100644
--- a/libgcc/config.host
+++ b/libgcc/config.host
@@ -51,8 +51,10 @@
 #			If either is set, EXTRA_PARTS and
 #			EXTRA_MULTILIB_PARTS inherited from the GCC
 #			subdirectory will be ignored.
-#  md_unwind_header	The name of a header file defining
-#			MD_FALLBACK_FRAME_STATE_FOR.
+#  md_unwind_def_header The name of a header file defining architecture-specific
+#			frame information types for unwinding.
+#  md_unwind_header	The name of a header file defining architecture-specific
+#			handlers used in the unwinder.
 #  sfp_machine_header	The name of a sfp-machine.h header file for soft-fp.
 #  			Defaults to "$cpu_type/sfp-machine.h" if it exists,
 #			no-sfp-machine.h otherwise.
@@ -72,6 +74,7 @@ extra_parts=
 tmake_file=
 tm_file=
 tm_define=
+md_unwind_def_header=no-unwind.h
 md_unwind_header=no-unwind.h
 unwind_header=unwind-generic.h
 
@@ -403,6 +406,7 @@ aarch64*-*-elf | aarch64*-*-rtems*)
 	tmake_file="${tmake_file} ${cpu_type}/t-lse t-slibgcc-libgcc"
 	tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp t-crtfm"
 	tmake_file="${tmake_file} t-dfprules"
+	md_unwind_def_header=aarch64/aarch64-unwind-def.h
 	md_unwind_header=aarch64/aarch64-unwind.h
 	;;
 aarch64*-*-freebsd*)
@@ -411,6 +415,7 @@ aarch64*-*-freebsd*)
 	tmake_file="${tmake_file} ${cpu_type}/t-lse t-slibgcc-libgcc"
 	tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp t-crtfm"
 	tmake_file="${tmake_file} t-dfprules"
+	md_unwind_def_header=aarch64/aarch64-unwind-def.h
 	md_unwind_header=aarch64/freebsd-unwind.h
 	;;
 aarch64*-*-netbsd*)
@@ -418,6 +423,7 @@ aarch64*-*-netbsd*)
 	tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
 	tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp t-crtfm"
 	tmake_file="${tmake_file} t-dfprules"
+	md_unwind_def_header=aarch64/aarch64-unwind-def.h
 	md_unwind_header=aarch64/aarch64-unwind.h
 	;;
 aarch64*-*-fuchsia*)
@@ -428,6 +434,7 @@ aarch64*-*-fuchsia*)
 	;;
 aarch64*-*-linux*)
 	extra_parts="$extra_parts crtfastmath.o"
+	md_unwind_def_header=aarch64/aarch64-unwind-def.h
 	md_unwind_header=aarch64/linux-unwind.h
 	tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
 	tmake_file="${tmake_file} ${cpu_type}/t-lse t-slibgcc-libgcc"
@@ -437,6 +444,7 @@ aarch64*-*-linux*)
 	;;
 aarch64*-*-gnu*)
 	extra_parts="$extra_parts crtfastmath.o"
+	md_unwind_def_header=aarch64/aarch64-unwind-def.h
 	md_unwind_header=aarch64/gnu-unwind.h
 	tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
 	tmake_file="${tmake_file} ${cpu_type}/t-lse t-slibgcc-libgcc"
@@ -446,6 +454,7 @@ aarch64*-*-gnu*)
 	;;
 aarch64*-*-vxworks7*)
 	extra_parts="$extra_parts crtfastmath.o"
+	md_unwind_def_header=aarch64/aarch64-unwind-def.h
 	md_unwind_header=aarch64/aarch64-unwind.h
 	tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
 	tmake_file="${tmake_file} ${cpu_type}/t-lse"
diff --git a/libgcc/config/aarch64/aarch64-unwind-def.h b/libgcc/config/aarch64/aarch64-unwind-def.h
new file mode 100644
index 00000000000..d06b3222bed
--- /dev/null
+++ b/libgcc/config/aarch64/aarch64-unwind-def.h
@@ -0,0 +1,41 @@
+/* Copyright (C) 2024 Free Software Foundation, Inc.
+   Contributed by Arm Ltd.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+for more details.
+
+Under Section 7 of GPL version 3, you are granted additional
+permissions described in the GCC Runtime Library Exception, version
+3.1, as published by the Free Software Foundation.
+
+You should have received a copy of the GNU General Public License and
+a copy of the GCC Runtime Library Exception along with this program;
+see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
+<http://www.gnu.org/licenses/>.  */
+
+#if !defined (AARCH64_UNWIND_DEF_H) && !defined (__ILP32__)
+#define AARCH64_UNWIND_DEF_H
+
+/* The key used to sign a function's return address. */
+typedef enum {
+  AARCH64_PAUTH_KEY_A,
+  AARCH64_PAUTH_KEY_B,
+} __attribute__((packed)) aarch64_pointer_auth_key;
+
+typedef struct
+{
+  aarch64_pointer_auth_key signing_key;
+} _AArch64Ext_Unwind_FrameState;
+
+#define MD_ARCH_FRAME_STATE_T _AArch64Ext_Unwind_FrameState
+
+#endif /* defined AARCH64_UNWIND_DEF_H && defined __ILP32__ */
diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
index cc225a7e207..e0da98d5e3b 100644
--- a/libgcc/config/aarch64/aarch64-unwind.h
+++ b/libgcc/config/aarch64/aarch64-unwind.h
@@ -25,6 +25,8 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #if !defined (AARCH64_UNWIND_H) && !defined (__ILP32__)
 #define AARCH64_UNWIND_H
 
+#include "aarch64-unwind-def.h"
+
 #include "ansidecl.h"
 #include <stdbool.h>
 
@@ -38,12 +40,6 @@ typedef enum
   AARCH64_RA_signing_SP = 0x1,
 } __attribute__((packed)) aarch64_RA_signing_method_t;
 
-/* The key used to sign a function's return address. */
-typedef enum {
-  AARCH64_PAUTH_KEY_A,
-  AARCH64_PAUTH_KEY_B,
-} __attribute__((packed)) aarch64_pointer_auth_key;
-
 #define MD_ARCH_EXTENSION_CIE_AUG_HANDLER(fs, aug) \
   aarch64_cie_aug_handler(fs, aug)
 
@@ -92,7 +88,7 @@ aarch64_cie_aug_handler(_Unwind_FrameState *fs, unsigned char aug)
   // AArch64 B-key pointer authentication.
   if (aug == 'B')
     {
-      fs->regs.signing_key = AARCH64_PAUTH_KEY_B;
+      fs->regs.arch_fs.signing_key = AARCH64_PAUTH_KEY_B;
       return true;
     }
   return false;
@@ -109,7 +105,7 @@ aarch64_arch_extension_frame_init (struct _Unwind_Context *context ATTRIBUTE_UNU
 				   _Unwind_FrameState *fs)
 {
   // Reset previously cached CIE/FDE information.
-  fs->regs.signing_key = AARCH64_PAUTH_KEY_A;
+  fs->regs.arch_fs.signing_key = AARCH64_PAUTH_KEY_A;
 
   // Reset some registers.
   // Note: the associated fs->how table is automatically reset to REG_UNSAVED in
@@ -169,7 +165,7 @@ aarch64_demangle_return_addr (struct _Unwind_Context *context,
   if (signing_method == AARCH64_RA_signing_SP)
     {
       _Unwind_Word salt = (_Unwind_Word) context->cfa;
-      if (fs->regs.signing_key == AARCH64_PAUTH_KEY_B)
+      if (fs->regs.arch_fs.signing_key == AARCH64_PAUTH_KEY_B)
 	return __builtin_aarch64_autib1716 (addr, salt);
       return __builtin_aarch64_autia1716 (addr, salt);
     }
diff --git a/libgcc/configure.ac b/libgcc/configure.ac
index c2749fe0958..ca341479177 100644
--- a/libgcc/configure.ac
+++ b/libgcc/configure.ac
@@ -727,6 +727,7 @@ AC_SUBST(extra_parts)
 AC_SUBST(asm_hidden_op)
 AC_SUBST(enable_execute_stack)
 AC_SUBST(unwind_header)
+AC_SUBST(md_unwind_def_header)
 AC_SUBST(md_unwind_header)
 AC_SUBST(sfp_machine_header)
 AC_SUBST(thread_header)
diff --git a/libgcc/unwind-dw2.h b/libgcc/unwind-dw2.h
index b75f4c65f98..789150d3171 100644
--- a/libgcc/unwind-dw2.h
+++ b/libgcc/unwind-dw2.h
@@ -22,6 +22,8 @@
    see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
+#include "md-unwind-def.h"
+
 enum register_rule
 {
   REG_UNSAVED,
@@ -71,8 +73,8 @@ typedef struct
      * Note: those information have to be saved in struct frame_state_reg_info
      * instead of _Unwind_FrameState as DW_CFA_restore_state has to be able to
      * restore them.  */
-#if defined(__aarch64__) && !defined (__ILP32__)
-    unsigned char signing_key;
+#if defined(MD_ARCH_FRAME_STATE_T)
+    MD_ARCH_FRAME_STATE_T arch_fs;
 #endif
   } regs;
 
-- 
2.45.2

Re: [PATCH v1 2/3] libgcc: hide CIE and FDE data for DWARF architecture extensions behind a handler.

[Richard Sandiford]

Matthieu Longo <matthieu.longo@arm.com> writes:
> This patch provides a new handler MD_ARCH_FRAME_STATE_T to hide an
> architecture-specific structure containing CIE and FDE data related
> to DWARF architecture extensions.
>
> Hiding the architecture-specific attributes behind a handler has the
> following benefits:
> 1. isolating those data from the generic ones in _Unwind_FrameState
> 2. avoiding casts to custom types.
> 3. preserving typing information when debugging with GDB, and so
>    facilitating their printing.
>
> This approach required to add a new header md-unwind-def.h included at
> the top of libgcc/unwind-dw2.h, and redirecting to the corresponding
> architecture header via a symbolic link.
>
> An obvious drawback is the increase in complexity with macros, and
> headers. It also caused a split of architecture definitions between
> md-unwind-def.h (types definitions used in unwind-dw2.h) and
> md-unwind.h (local types definitions and handlers implementations).
> The naming of md-unwind.h with .h extension is a bit misleading as
> the file is only included in the middle of unwind-dw2.c. Changing
> this naming would require modification of others backends, which I
> prefered to abstain from. Overall the benefits are worth the added
> complexity from my perspective.

Sorry, I should have read 2/3 before making the suggestion in the
previous review.  I agree that it makes sense to separate this change
out, given that it involves a new header file.

It'd be good to update the comment in no-unwind.h:

/* Dummy header for targets without a definition of
   MD_FALLBACK_FRAME_STATE_FOR.  */

LGTM otherwise, thanks.

On patch 3: IMO it's better to post the regenerated files as part
of the same patch, so that each patch is self-contained.

Richard

> libgcc/ChangeLog:
>
>     * Makefile.in: New target for symbolic link to md-unwind-def.h
>     * config.host: New parameter md_unwind_def_header. Set it to
>     aarch64/aarch64-unwind-def.h for AArch64 targets, or no-unwind.h
>     by default.
>     * config/aarch64/aarch64-unwind.h
>     (aarch64_pointer_auth_key): Move to aarch64-unwind-def.h
>     (aarch64_cie_aug_handler): Update.
>     (aarch64_arch_extension_frame_init): Update.
>     (aarch64_demangle_return_addr): Update.
>     * configure.ac: New substitute variable md_unwind_def_header.
>     * unwind-dw2.h (defined): MD_ARCH_FRAME_STATE_T.
>     * config/aarch64/aarch64-unwind-def.h: New file.
> ---
>  libgcc/Makefile.in                         |  6 +++-
>  libgcc/config.host                         | 13 +++++--
>  libgcc/config/aarch64/aarch64-unwind-def.h | 41 ++++++++++++++++++++++
>  libgcc/config/aarch64/aarch64-unwind.h     | 14 +++-----
>  libgcc/configure.ac                        |  1 +
>  libgcc/unwind-dw2.h                        |  6 ++--
>  6 files changed, 67 insertions(+), 14 deletions(-)
>  create mode 100644 libgcc/config/aarch64/aarch64-unwind-def.h
>
> diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in
> index 0e46e9ef768..ffc45f21267 100644
> --- a/libgcc/Makefile.in
> +++ b/libgcc/Makefile.in
> @@ -47,6 +47,7 @@ with_aix_soname = @with_aix_soname@
>  solaris_ld_v2_maps = @solaris_ld_v2_maps@
>  enable_execute_stack = @enable_execute_stack@
>  unwind_header = @unwind_header@
> +md_unwind_def_header = @md_unwind_def_header@
>  md_unwind_header = @md_unwind_header@
>  sfp_machine_header = @sfp_machine_header@
>  thread_header = @thread_header@
> @@ -358,13 +359,16 @@ SHLIBUNWIND_INSTALL =
>  
>  
>  # Create links to files specified in config.host.
> -LIBGCC_LINKS = enable-execute-stack.c unwind.h md-unwind-support.h \
> +LIBGCC_LINKS = enable-execute-stack.c \
> +               unwind.h md-unwind-def.h md-unwind-support.h \
>                 sfp-machine.h gthr-default.h
>  
>  enable-execute-stack.c: $(srcdir)/$(enable_execute_stack)
>  	-$(LN_S) $< $@
>  unwind.h: $(srcdir)/$(unwind_header)
>  	-$(LN_S) $< $@
> +md-unwind-def.h: $(srcdir)/config/$(md_unwind_def_header)
> +	-$(LN_S) $< $@
>  md-unwind-support.h: $(srcdir)/config/$(md_unwind_header)
>  	-$(LN_S) $< $@
>  sfp-machine.h: $(srcdir)/config/$(sfp_machine_header)
> diff --git a/libgcc/config.host b/libgcc/config.host
> index 9fae51d4ce7..61825e72fe4 100644
> --- a/libgcc/config.host
> +++ b/libgcc/config.host
> @@ -51,8 +51,10 @@
>  #			If either is set, EXTRA_PARTS and
>  #			EXTRA_MULTILIB_PARTS inherited from the GCC
>  #			subdirectory will be ignored.
> -#  md_unwind_header	The name of a header file defining
> -#			MD_FALLBACK_FRAME_STATE_FOR.
> +#  md_unwind_def_header The name of a header file defining architecture-specific
> +#			frame information types for unwinding.
> +#  md_unwind_header	The name of a header file defining architecture-specific
> +#			handlers used in the unwinder.
>  #  sfp_machine_header	The name of a sfp-machine.h header file for soft-fp.
>  #  			Defaults to "$cpu_type/sfp-machine.h" if it exists,
>  #			no-sfp-machine.h otherwise.
> @@ -72,6 +74,7 @@ extra_parts=
>  tmake_file=
>  tm_file=
>  tm_define=
> +md_unwind_def_header=no-unwind.h
>  md_unwind_header=no-unwind.h
>  unwind_header=unwind-generic.h
>  
> @@ -403,6 +406,7 @@ aarch64*-*-elf | aarch64*-*-rtems*)
>  	tmake_file="${tmake_file} ${cpu_type}/t-lse t-slibgcc-libgcc"
>  	tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp t-crtfm"
>  	tmake_file="${tmake_file} t-dfprules"
> +	md_unwind_def_header=aarch64/aarch64-unwind-def.h
>  	md_unwind_header=aarch64/aarch64-unwind.h
>  	;;
>  aarch64*-*-freebsd*)
> @@ -411,6 +415,7 @@ aarch64*-*-freebsd*)
>  	tmake_file="${tmake_file} ${cpu_type}/t-lse t-slibgcc-libgcc"
>  	tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp t-crtfm"
>  	tmake_file="${tmake_file} t-dfprules"
> +	md_unwind_def_header=aarch64/aarch64-unwind-def.h
>  	md_unwind_header=aarch64/freebsd-unwind.h
>  	;;
>  aarch64*-*-netbsd*)
> @@ -418,6 +423,7 @@ aarch64*-*-netbsd*)
>  	tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
>  	tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp t-crtfm"
>  	tmake_file="${tmake_file} t-dfprules"
> +	md_unwind_def_header=aarch64/aarch64-unwind-def.h
>  	md_unwind_header=aarch64/aarch64-unwind.h
>  	;;
>  aarch64*-*-fuchsia*)
> @@ -428,6 +434,7 @@ aarch64*-*-fuchsia*)
>  	;;
>  aarch64*-*-linux*)
>  	extra_parts="$extra_parts crtfastmath.o"
> +	md_unwind_def_header=aarch64/aarch64-unwind-def.h
>  	md_unwind_header=aarch64/linux-unwind.h
>  	tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
>  	tmake_file="${tmake_file} ${cpu_type}/t-lse t-slibgcc-libgcc"
> @@ -437,6 +444,7 @@ aarch64*-*-linux*)
>  	;;
>  aarch64*-*-gnu*)
>  	extra_parts="$extra_parts crtfastmath.o"
> +	md_unwind_def_header=aarch64/aarch64-unwind-def.h
>  	md_unwind_header=aarch64/gnu-unwind.h
>  	tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
>  	tmake_file="${tmake_file} ${cpu_type}/t-lse t-slibgcc-libgcc"
> @@ -446,6 +454,7 @@ aarch64*-*-gnu*)
>  	;;
>  aarch64*-*-vxworks7*)
>  	extra_parts="$extra_parts crtfastmath.o"
> +	md_unwind_def_header=aarch64/aarch64-unwind-def.h
>  	md_unwind_header=aarch64/aarch64-unwind.h
>  	tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
>  	tmake_file="${tmake_file} ${cpu_type}/t-lse"
> diff --git a/libgcc/config/aarch64/aarch64-unwind-def.h b/libgcc/config/aarch64/aarch64-unwind-def.h
> new file mode 100644
> index 00000000000..d06b3222bed
> --- /dev/null
> +++ b/libgcc/config/aarch64/aarch64-unwind-def.h
> @@ -0,0 +1,41 @@
> +/* Copyright (C) 2024 Free Software Foundation, Inc.
> +   Contributed by Arm Ltd.
> +
> +This file is part of GCC.
> +
> +GCC is free software; you can redistribute it and/or modify it under
> +the terms of the GNU General Public License as published by the Free
> +Software Foundation; either version 3, or (at your option) any later
> +version.
> +
> +GCC is distributed in the hope that it will be useful, but WITHOUT ANY
> +WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> +for more details.
> +
> +Under Section 7 of GPL version 3, you are granted additional
> +permissions described in the GCC Runtime Library Exception, version
> +3.1, as published by the Free Software Foundation.
> +
> +You should have received a copy of the GNU General Public License and
> +a copy of the GCC Runtime Library Exception along with this program;
> +see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
> +<http://www.gnu.org/licenses/>.  */
> +
> +#if !defined (AARCH64_UNWIND_DEF_H) && !defined (__ILP32__)
> +#define AARCH64_UNWIND_DEF_H
> +
> +/* The key used to sign a function's return address. */
> +typedef enum {
> +  AARCH64_PAUTH_KEY_A,
> +  AARCH64_PAUTH_KEY_B,
> +} __attribute__((packed)) aarch64_pointer_auth_key;
> +
> +typedef struct
> +{
> +  aarch64_pointer_auth_key signing_key;
> +} _AArch64Ext_Unwind_FrameState;
> +
> +#define MD_ARCH_FRAME_STATE_T _AArch64Ext_Unwind_FrameState
> +
> +#endif /* defined AARCH64_UNWIND_DEF_H && defined __ILP32__ */
> diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
> index cc225a7e207..e0da98d5e3b 100644
> --- a/libgcc/config/aarch64/aarch64-unwind.h
> +++ b/libgcc/config/aarch64/aarch64-unwind.h
> @@ -25,6 +25,8 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>  #if !defined (AARCH64_UNWIND_H) && !defined (__ILP32__)
>  #define AARCH64_UNWIND_H
>  
> +#include "aarch64-unwind-def.h"
> +
>  #include "ansidecl.h"
>  #include <stdbool.h>
>  
> @@ -38,12 +40,6 @@ typedef enum
>    AARCH64_RA_signing_SP = 0x1,
>  } __attribute__((packed)) aarch64_RA_signing_method_t;
>  
> -/* The key used to sign a function's return address. */
> -typedef enum {
> -  AARCH64_PAUTH_KEY_A,
> -  AARCH64_PAUTH_KEY_B,
> -} __attribute__((packed)) aarch64_pointer_auth_key;
> -
>  #define MD_ARCH_EXTENSION_CIE_AUG_HANDLER(fs, aug) \
>    aarch64_cie_aug_handler(fs, aug)
>  
> @@ -92,7 +88,7 @@ aarch64_cie_aug_handler(_Unwind_FrameState *fs, unsigned char aug)
>    // AArch64 B-key pointer authentication.
>    if (aug == 'B')
>      {
> -      fs->regs.signing_key = AARCH64_PAUTH_KEY_B;
> +      fs->regs.arch_fs.signing_key = AARCH64_PAUTH_KEY_B;
>        return true;
>      }
>    return false;
> @@ -109,7 +105,7 @@ aarch64_arch_extension_frame_init (struct _Unwind_Context *context ATTRIBUTE_UNU
>  				   _Unwind_FrameState *fs)
>  {
>    // Reset previously cached CIE/FDE information.
> -  fs->regs.signing_key = AARCH64_PAUTH_KEY_A;
> +  fs->regs.arch_fs.signing_key = AARCH64_PAUTH_KEY_A;
>  
>    // Reset some registers.
>    // Note: the associated fs->how table is automatically reset to REG_UNSAVED in
> @@ -169,7 +165,7 @@ aarch64_demangle_return_addr (struct _Unwind_Context *context,
>    if (signing_method == AARCH64_RA_signing_SP)
>      {
>        _Unwind_Word salt = (_Unwind_Word) context->cfa;
> -      if (fs->regs.signing_key == AARCH64_PAUTH_KEY_B)
> +      if (fs->regs.arch_fs.signing_key == AARCH64_PAUTH_KEY_B)
>  	return __builtin_aarch64_autib1716 (addr, salt);
>        return __builtin_aarch64_autia1716 (addr, salt);
>      }
> diff --git a/libgcc/configure.ac b/libgcc/configure.ac
> index c2749fe0958..ca341479177 100644
> --- a/libgcc/configure.ac
> +++ b/libgcc/configure.ac
> @@ -727,6 +727,7 @@ AC_SUBST(extra_parts)
>  AC_SUBST(asm_hidden_op)
>  AC_SUBST(enable_execute_stack)
>  AC_SUBST(unwind_header)
> +AC_SUBST(md_unwind_def_header)
>  AC_SUBST(md_unwind_header)
>  AC_SUBST(sfp_machine_header)
>  AC_SUBST(thread_header)
> diff --git a/libgcc/unwind-dw2.h b/libgcc/unwind-dw2.h
> index b75f4c65f98..789150d3171 100644
> --- a/libgcc/unwind-dw2.h
> +++ b/libgcc/unwind-dw2.h
> @@ -22,6 +22,8 @@
>     see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> +#include "md-unwind-def.h"
> +
>  enum register_rule
>  {
>    REG_UNSAVED,
> @@ -71,8 +73,8 @@ typedef struct
>       * Note: those information have to be saved in struct frame_state_reg_info
>       * instead of _Unwind_FrameState as DW_CFA_restore_state has to be able to
>       * restore them.  */
> -#if defined(__aarch64__) && !defined (__ILP32__)
> -    unsigned char signing_key;
> +#if defined(MD_ARCH_FRAME_STATE_T)
> +    MD_ARCH_FRAME_STATE_T arch_fs;
>  #endif
>    } regs;

Re: [PATCH v1 2/3] libgcc: hide CIE and FDE data for DWARF architecture extensions behind a handler.

[Matthieu Longo]

On 2024-08-06 11:21, Richard Sandiford wrote:
> Matthieu Longo <matthieu.longo@arm.com> writes:
>> This patch provides a new handler MD_ARCH_FRAME_STATE_T to hide an
>> architecture-specific structure containing CIE and FDE data related
>> to DWARF architecture extensions.
>>
>> Hiding the architecture-specific attributes behind a handler has the
>> following benefits:
>> 1. isolating those data from the generic ones in _Unwind_FrameState
>> 2. avoiding casts to custom types.
>> 3. preserving typing information when debugging with GDB, and so
>>     facilitating their printing.
>>
>> This approach required to add a new header md-unwind-def.h included at
>> the top of libgcc/unwind-dw2.h, and redirecting to the corresponding
>> architecture header via a symbolic link.
>>
>> An obvious drawback is the increase in complexity with macros, and
>> headers. It also caused a split of architecture definitions between
>> md-unwind-def.h (types definitions used in unwind-dw2.h) and
>> md-unwind.h (local types definitions and handlers implementations).
>> The naming of md-unwind.h with .h extension is a bit misleading as
>> the file is only included in the middle of unwind-dw2.c. Changing
>> this naming would require modification of others backends, which I
>> prefered to abstain from. Overall the benefits are worth the added
>> complexity from my perspective.
> 
> Sorry, I should have read 2/3 before making the suggestion in the
> previous review.  I agree that it makes sense to separate this change
> out, given that it involves a new header file.
> 
> It'd be good to update the comment in no-unwind.h:
> 
> /* Dummy header for targets without a definition of
>     MD_FALLBACK_FRAME_STATE_FOR.  */

Done

> 
> LGTM otherwise, thanks.
> 
> On patch 3: IMO it's better to post the regenerated files as part
> of the same patch, so that each patch is self-contained.
 >
 > Richard
 >

I did it this way as it makes things easier to rebase, and regenerate 
the files if there is a conflict.
I agree, this patch can be squashed with the previous when merging those 
changes into master.

Matthieu

[PATCH v1 3/3] libgcc: update configure (regenerated by autoreconf)

[Matthieu Longo]

libgcc/ChangeLog:

        * configure: Regenerate.
---
 libgcc/configure | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libgcc/configure b/libgcc/configure
index a69d314374a..15a0be23644 100755
--- a/libgcc/configure
+++ b/libgcc/configure
@@ -587,6 +587,7 @@ ac_includes_default='/* none */'
 ac_subst_vars='LTLIBOBJS
 LIBOBJS
 md_unwind_header
+md_unwind_def_header
 unwind_header
 enable_execute_stack
 asm_hidden_op
@@ -5786,6 +5787,7 @@ fi
 
 
 
+
 # We need multilib support.
 ac_config_files="$ac_config_files Makefile"
 
-- 
2.45.2

Re: [PATCH v1 0/3][libgcc] store signing key and signing method in DWARF _Unwind_FrameState

[Matthieu Longo]

On 2024-07-19 15:54, Matthieu Longo wrote:
> This patch series is only a refactoring of the existing implementation of PAuth and returned-address signing. The existing behavior is preserved.
> 
> 1. aarch64: store signing key and signing method in DWARF _Unwind_FrameState
> 
> _Unwind_FrameState already contains several CIE and FDE information (see the attributes below the comment "The information we care about from the CIE/FDE" in libgcc/unwind-dw2.h).
> The patch aims at moving the information from DWARF CIE (signing key stored in the augmentation string) and FDE (the used signing method) into _Unwind_FrameState along the already-stored CIE and FDE information.
> Note: those information have to be saved in frame_state_reg_info instead of _Unwind_FrameState as they need to be savable by DW_CFA_remember_state and restorable by DW_CFA_restore_state, that both rely on the attribute "prev".
> Those new information in _Unwind_FrameState simplifies the look-up of the signing key when the return address is demangled. It also allows future signing methods to be easily added.
> _Unwind_FrameState is not a part of the public API of libunwind, so the change is backward compatible.
> 
> A new architecture-specific handler MD_ARCH_EXTENSION_FRAME_INIT allows to reset values in the frame state and unwind context if needed by the architecture extension before changing the frame state to the caller context.
> A new architecture-specific handler MD_ARCH_EXTENSION_CIE_AUG_HANDLER isolates the architecture-specific augmentation strings in AArch64 backend, and allows others architectures to reuse augmentation strings that would have clashed with AArch64 DWARF extensions.
> aarch64_demangle_return_addr, DW_CFA_AARCH64_negate_ra_state and DW_CFA_val_expression cases in libgcc/unwind-dw2-execute_cfa.h were documented to clarify where the value of the RA state register is stored (FS and CONTEXT respectively).
> 
> 2. libgcc: hide CIE and FDE data for DWARF architecture extensions behind a handler.
> 
> This patch provides a new handler MD_ARCH_FRAME_STATE_T to hide an architecture-specific structure containing CIE and FDE data related to DWARF architecture extensions.
> Hiding the architecture-specific attributes behind a handler has the following benefits:
>      1. isolating those data from the generic ones in _Unwind_FrameState
>      2. avoiding casts to custom types.
>      3. preserving typing information when debugging with GDB, and so facilitating their printing.
> 
> This approach required to add a new header md-unwind-def.h included at the top of libgcc/unwind-dw2.h, and redirecting to the corresponding architecture header via a symbolic link.
> An obvious drawback is the increase in complexity with macros, and headers. It also caused a split of architecture definitions between md-unwind-def.h (types definitions used in unwind-dw2.h) and md-unwind.h (local types definitions and handlers implementations).
> The naming of md-unwind.h with .h extension is a bit misleading as the file is only included in the middle of unwind-dw2.c. Changing this naming would require modification of others backends, which I prefered to abstain from.
> Overall the benefits are worth the added complexity from my perspective.
> 
> 3. libgcc: update configure (regenerated by autoreconf)
> 
> Regenerate the build files.
> 
> 
> ## Testing
> 
> Those changes were testing by covering the 3 following cases:
> - backtracing.
> - exception handling in a C++ program.
> - gcc/testsuite/gcc.target/aarch64/pr104689.c: pac-ret with unusual DWARF [1]
> 
> Regression tested on aarch64-unknown-linux-gnu, and no regression found.
> 
> [1]: https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594414.html
> 
> 
> Ok for master? I don't have commit access so I need someone to commit on my behalf.
> 
> Regards,
> Matthieu.
> 
> 
> Matthieu Longo (3):
>    aarch64: store signing key and signing method in DWARF
>      _Unwind_FrameState
>    libgcc: hide CIE and FDE data for DWARF architecture extensions behind
>      a handler.
>    libgcc: update configure (regenerated by autoreconf)
> 
>   libgcc/Makefile.in                         |   6 +-
>   libgcc/config.host                         |  13 +-
>   libgcc/config/aarch64/aarch64-unwind-def.h |  41 ++++++
>   libgcc/config/aarch64/aarch64-unwind.h     | 150 +++++++++++++++++----
>   libgcc/configure                           |   2 +
>   libgcc/configure.ac                        |   1 +
>   libgcc/unwind-dw2-execute_cfa.h            |  34 +++--
>   libgcc/unwind-dw2.c                        |  19 ++-
>   libgcc/unwind-dw2.h                        |  19 ++-
>   9 files changed, 233 insertions(+), 52 deletions(-)
>   create mode 100644 libgcc/config/aarch64/aarch64-unwind-def.h
> 

Adding Ian in CC as he is listed as the maintainer of libgcc in 
MAINTAINERS file.