Re: [PATCH] libgcc: Fix uninitialized RA signing on AArch64 [PR107678]

[Wilco Dijkstra <Wilco.Dijkstra@arm.com>]

Hi Richard,

> Hmm, but the point of the original patch was to support code generators
> that emit DW_CFA_val_expression instead of DW_CFA_AARCH64_negate_ra_state.
> Doesn't this patch undo that?

Well it wasn't clear from the code or comments that was supported. I've
added that back in v2.

> Also, if I understood correctly, the reason we use REG_UNSAVED is to
> ensure that state from one frame isn't carried across to a parent frame,
> in cases where the parent frame lacks any signing.  That is, each frame
> should start out with a zero bit even if a child frame is unwound while
> it has a set bit.

This works fine since all registers are initialized to REG_UNSAVED every frame.

In v2 I've removed some clutter and encode the signing state in REG_UNSAVED/
REG_UNDEFINED.

Cheers,
Wilco

v2: Further cleanup, support DW_CFA_expression.

A recent change only initializes the regs.how[] during Dwarf unwinding
which resulted in an uninitialized offset used in return address signing
and random failures during unwinding.  The fix is to encode the return
address signing state in REG_UNSAVED and REG_UNDEFINED.

Passes bootstrap & regress, OK for commit?

libgcc/
        PR target/107678
        * unwind-dw2.c (execute_cfa_program): Use REG_UNSAVED/UNDEFINED
        to encode return address signing state.
        * config/aarch64/aarch64-unwind.h (aarch64_demangle_return_addr)
        Check current return address signing state.
        (aarch64_frob_update_contex): Remove.

---
diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
index 26db9cbd9e5c526e0c410a4fc6be2bedb7d261cf..1afc3f9d308b95bc787398263e629bab226ff1ba 100644
--- a/libgcc/config/aarch64/aarch64-unwind.h
+++ b/libgcc/config/aarch64/aarch64-unwind.h
@@ -29,8 +29,6 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 
 #define MD_DEMANGLE_RETURN_ADDR(context, fs, addr) \
   aarch64_demangle_return_addr (context, fs, addr)
-#define MD_FROB_UPDATE_CONTEXT(context, fs) \
-  aarch64_frob_update_context (context, fs)
 
 static inline int
 aarch64_cie_signed_with_b_key (struct _Unwind_Context *context)
@@ -55,42 +53,27 @@ aarch64_cie_signed_with_b_key (struct _Unwind_Context *context)
 
 static inline void *
 aarch64_demangle_return_addr (struct _Unwind_Context *context,
-			      _Unwind_FrameState *fs ATTRIBUTE_UNUSED,
+			      _Unwind_FrameState *fs,
 			      _Unwind_Word addr_word)
 {
   void *addr = (void *)addr_word;
-  if (context->flags & RA_SIGNED_BIT)
+  const int reg = DWARF_REGNUM_AARCH64_RA_STATE;
+
+  if (fs->regs.how[reg] == REG_UNSAVED)
+    return addr;
+
+  /* Return-address signing state is toggled by DW_CFA_GNU_window_save (where
+     REG_UNDEFINED means enabled), or set by a DW_CFA_expression.  */
+  if (fs->regs.how[reg] == REG_UNDEFINED
+      || (_Unwind_GetGR (context, reg) & 0x1) != 0)
     {
       _Unwind_Word salt = (_Unwind_Word) context->cfa;
       if (aarch64_cie_signed_with_b_key (context) != 0)
 	return __builtin_aarch64_autib1716 (addr, salt);
       return __builtin_aarch64_autia1716 (addr, salt);
     }
-  else
-    return addr;
-}
-
-/* Do AArch64 private initialization on CONTEXT based on frame info FS.  Mark
-   CONTEXT as return address signed if bit 0 of DWARF_REGNUM_AARCH64_RA_STATE is
-   set.  */
-
-static inline void
-aarch64_frob_update_context (struct _Unwind_Context *context,
-			     _Unwind_FrameState *fs)
-{
-  const int reg = DWARF_REGNUM_AARCH64_RA_STATE;
-  int ra_signed;
-  if (fs->regs.how[reg] == REG_UNSAVED)
-    ra_signed = fs->regs.reg[reg].loc.offset & 0x1;
-  else
-    ra_signed = _Unwind_GetGR (context, reg) & 0x1;
-  if (ra_signed)
-    /* The flag is used for re-authenticating EH handler's address.  */
-    context->flags |= RA_SIGNED_BIT;
-  else
-    context->flags &= ~RA_SIGNED_BIT;
 
-  return;
+  return addr;
 }
 
 #endif /* defined AARCH64_UNWIND_H && defined __ILP32__ */
diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
index eaceace20298b9b13344aff9d1fe9ee5f9c7bd73..7c200cb6e730c5d63cf200ebe8a903f858e79d07 100644
--- a/libgcc/unwind-dw2.c
+++ b/libgcc/unwind-dw2.c
@@ -139,7 +139,6 @@ struct _Unwind_Context
 #define EXTENDED_CONTEXT_BIT ((~(_Unwind_Word) 0 >> 2) + 1)
   /* Bit reserved on AArch64, return address has been signed with A or B
      key.  */
-#define RA_SIGNED_BIT ((~(_Unwind_Word) 0 >> 3) + 1)
   _Unwind_Word flags;
   /* 0 for now, can be increased when further fields are added to
      struct _Unwind_Context.  */
@@ -1206,8 +1205,10 @@ execute_cfa_program (const unsigned char *insn_ptr,
 	  /* This CFA is multiplexed with Sparc.  On AArch64 it's used to toggle
 	     return address signing status.  */
 	  reg = DWARF_REGNUM_AARCH64_RA_STATE;
-	  gcc_assert (fs->regs.how[reg] == REG_UNSAVED);
-	  fs->regs.reg[reg].loc.offset ^= 1;
+	  if (fs->regs.how[reg] == REG_UNSAVED)
+	    fs->regs.how[reg] = REG_UNDEFINED;
+	  else
+	    fs->regs.how[reg] = REG_UNSAVED;
 #else
 	  /* ??? Hardcoded for SPARC register window configuration.  */
 	  if (__LIBGCC_DWARF_FRAME_REGISTERS__ >= 32)