libgcc: Fix uninitialized RA signing on AArch64 [PR107678]

  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 use REG_SAVED_OFFSET
as the state where the return address signing bit is valid, and if the
state is REG_UNSAVED, initialize it to 0.

Passes bootstrap & regress, OK for commit?

libgcc/
        PR target/107678
        * unwind-dw2.c (execute_cfa_program): Initialize offset of
        DWARF_REGNUM_AARCH64_RA_STATE if in REG_UNSAVED state.
        * config/aarch64/aarch64-unwind.h (aarch64_frob_update_contex):
        Check state is REG_SAVED_OFFSET before using offset for RA state.

---
  

Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes:
> 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 use REG_SAVED_OFFSET
> as the state where the return address signing bit is valid, and if the
> state is REG_UNSAVED, initialize it to 0.
>
> Passes bootstrap & regress, OK for commit?
>
> libgcc/
>         PR target/107678
>         * unwind-dw2.c (execute_cfa_program): Initialize offset of
>         DWARF_REGNUM_AARCH64_RA_STATE if in REG_UNSAVED state.
>         * config/aarch64/aarch64-unwind.h (aarch64_frob_update_contex):
>         Check state is REG_SAVED_OFFSET before using offset for RA state.
>
> ---
>
> diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
> index 26db9cbd9e5c526e0c410a4fc6be2bedb7d261cf..597133b3d708a50a366c8bfeff57475f5522b3f6 100644
> --- a/libgcc/config/aarch64/aarch64-unwind.h
> +++ b/libgcc/config/aarch64/aarch64-unwind.h
> @@ -71,21 +71,15 @@ aarch64_demangle_return_addr (struct _Unwind_Context *context,
>  }
>  
>  /* 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.  */
> +   CONTEXT as having a signed return address if DWARF_REGNUM_AARCH64_RA_STATE
> +   is initialized (REG_SAVED_OFFSET state) and the offset has bit 0 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.  */
> +  if (fs->regs.how[DWARF_REGNUM_AARCH64_RA_STATE] == REG_SAVED_OFFSET
> +      && (fs->regs.reg[DWARF_REGNUM_AARCH64_RA_STATE].loc.offset & 1) != 0)
>      context->flags |= RA_SIGNED_BIT;
>    else
>      context->flags &= ~RA_SIGNED_BIT;

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?

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.

Thanks,
Richard

> diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
> index eaceace20298b9b13344aff9d1fe9ee5f9c7bd73..87f2ae065b67982ce48f74e45523d9c754a7661c 100644
> --- a/libgcc/unwind-dw2.c
> +++ b/libgcc/unwind-dw2.c
> @@ -1203,11 +1203,16 @@ execute_cfa_program (const unsigned char *insn_ptr,
>  
>  	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 = DWARF_REGNUM_AARCH64_RA_STATE;
> -	  gcc_assert (fs->regs.how[reg] == REG_UNSAVED);
> -	  fs->regs.reg[reg].loc.offset ^= 1;
> +	 /* This CFA is multiplexed with Sparc.  On AArch64 it's used to toggle
> +	    the return address signing status.  It is initialized at the first
> +	    use and the state is stored in bit 0 of the offset.  */
> +	 reg = DWARF_REGNUM_AARCH64_RA_STATE;
> +	 if (fs->regs.how[reg] == REG_UNSAVED)
> +	   {
> +	     fs->regs.how[reg] = REG_SAVED_OFFSET;
> +	     fs->regs.reg[reg].loc.offset = 0;
> +	   }
> +	 fs->regs.reg[reg].loc.offset ^= 1;
>  #else
>  	  /* ??? Hardcoded for SPARC register window configuration.  */
>  	  if (__LIBGCC_DWARF_FRAME_REGISTERS__ >= 32)
  

The 12/05/2022 19:04, Richard Sandiford wrote:
> Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes:
> > 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 use REG_SAVED_OFFSET
> > as the state where the return address signing bit is valid, and if the
> > state is REG_UNSAVED, initialize it to 0.
> >
> > Passes bootstrap & regress, OK for commit?
> >
> > libgcc/
> >         PR target/107678
> >         * unwind-dw2.c (execute_cfa_program): Initialize offset of
> >         DWARF_REGNUM_AARCH64_RA_STATE if in REG_UNSAVED state.
> >         * config/aarch64/aarch64-unwind.h (aarch64_frob_update_contex):
> >         Check state is REG_SAVED_OFFSET before using offset for RA state.
> >
> > ---
> >
> > diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
> > index 26db9cbd9e5c526e0c410a4fc6be2bedb7d261cf..597133b3d708a50a366c8bfeff57475f5522b3f6 100644
> > --- a/libgcc/config/aarch64/aarch64-unwind.h
> > +++ b/libgcc/config/aarch64/aarch64-unwind.h
> > @@ -71,21 +71,15 @@ aarch64_demangle_return_addr (struct _Unwind_Context *context,
> >  }
> >  
> >  /* 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.  */
> > +   CONTEXT as having a signed return address if DWARF_REGNUM_AARCH64_RA_STATE
> > +   is initialized (REG_SAVED_OFFSET state) and the offset has bit 0 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.  */
> > +  if (fs->regs.how[DWARF_REGNUM_AARCH64_RA_STATE] == REG_SAVED_OFFSET
> > +      && (fs->regs.reg[DWARF_REGNUM_AARCH64_RA_STATE].loc.offset & 1) != 0)
> >      context->flags |= RA_SIGNED_BIT;
> >    else
> >      context->flags &= ~RA_SIGNED_BIT;
> 
> 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?
> 
> 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.

yes.

i don't think how[*RA_STATE] can ever be set to REG_SAVED_OFFSET,
this pseudo reg is not spilled to the stack, it is reset to 0 in
each frame and then toggled within a frame.

unwind-dw2.c has

	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 = DWARF_REGNUM_AARCH64_RA_STATE;
	  gcc_assert (fs->regs.how[reg] == REG_UNSAVED);
	  fs->regs.reg[reg].loc.offset ^= 1;

for this to work, loc.offset must be reset in uw_frame_state_for.
we may need a new hook for that.


> 
> Thanks,
> Richard
> 
> > diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
> > index eaceace20298b9b13344aff9d1fe9ee5f9c7bd73..87f2ae065b67982ce48f74e45523d9c754a7661c 100644
> > --- a/libgcc/unwind-dw2.c
> > +++ b/libgcc/unwind-dw2.c
> > @@ -1203,11 +1203,16 @@ execute_cfa_program (const unsigned char *insn_ptr,
> >  
> >  	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 = DWARF_REGNUM_AARCH64_RA_STATE;
> > -	  gcc_assert (fs->regs.how[reg] == REG_UNSAVED);
> > -	  fs->regs.reg[reg].loc.offset ^= 1;
> > +	 /* This CFA is multiplexed with Sparc.  On AArch64 it's used to toggle
> > +	    the return address signing status.  It is initialized at the first
> > +	    use and the state is stored in bit 0 of the offset.  */
> > +	 reg = DWARF_REGNUM_AARCH64_RA_STATE;
> > +	 if (fs->regs.how[reg] == REG_UNSAVED)
> > +	   {
> > +	     fs->regs.how[reg] = REG_SAVED_OFFSET;
> > +	     fs->regs.reg[reg].loc.offset = 0;
> > +	   }
> > +	 fs->regs.reg[reg].loc.offset ^= 1;
> >  #else
> >  	  /* ??? Hardcoded for SPARC register window configuration.  */
> >  	  if (__LIBGCC_DWARF_FRAME_REGISTERS__ >= 32)
  

Hi,

> i don't think how[*RA_STATE] can ever be set to REG_SAVED_OFFSET,
> this pseudo reg is not spilled to the stack, it is reset to 0 in
> each frame and then toggled within a frame.

It's is just a state, we can use any state we want since it is a pseudo reg.
These registers are global and shared across all functions in an unwind,
so their state or value isn't reset for each frame. So if we want to reset
it in each frame then using a virtual register to hold per-function data
seems like a bad design. I'm surprised nobody has ever tested it...

Cheers,
Wilco
  

The 12/06/2022 11:58, Wilco Dijkstra wrote:
> > i don't think how[*RA_STATE] can ever be set to REG_SAVED_OFFSET,
> > this pseudo reg is not spilled to the stack, it is reset to 0 in
> > each frame and then toggled within a frame.
> 
> It's is just a state, we can use any state we want since it is a pseudo reg.
> These registers are global and shared across all functions in an unwind,
> so their state or value isn't reset for each frame. So if we want to reset
> it in each frame then using a virtual register to hold per-function data
> seems like a bad design. I'm surprised nobody has ever tested it...

it was tested (and worked when the frame state was initialized).

in principle the CIE can contain instructions to initialize the
register state for a frame. the RA_STATE pseudo reg behaves as if
the CIE always set its value to 0 at the start of the frame.

the design has issues, but this is what we have now.

the toggle instruction for RA_STATE does not really fit the dwarf
model: the CFI instruction sequence is evaluated with a context
that is valid at the end of the sequence so an unwinder only wants
to evaluate a register's state at the end, not intermediate values
(where the context might not even be valid). so we limited the
instructions allowed for RA_STATE: only remember_/restore_state,
toggle and val_expression are supported and the latter two cannot
be mixed. we still have to use the existing struct for keeping
track of this hence reg[RA_STATE].loc.offset.

and of course the RA_STATE pseudo reg is only used for computing
the return address not propagated to the previous frame so it is
special in many ways. so we will need target hooks to fix this
and i think the cleanest is to initialize RA_STATE per frame and
leave the rest as is.
  

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)
  

The 01/03/2023 17:27, Wilco Dijkstra wrote:
> 
> > 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.

this looks good to me.


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

i would keep the assert: how[reg] must be either UNSAVED or UNDEFINED
here, other how[reg] means the toggle cfi instruction is mixed with
incompatible instructions for the pseudo reg.

and i would add a comment about this e.g. saying that UNSAVED/UNDEFINED
how[reg] is used for tracking the return address signing status and
other how[reg] is not allowed here.

otherwise the patch looks good.
  

Hi Szabolcs,

> i would keep the assert: how[reg] must be either UNSAVED or UNDEFINED
> here, other how[reg] means the toggle cfi instruction is mixed with
> incompatible instructions for the pseudo reg.
>
> and i would add a comment about this e.g. saying that UNSAVED/UNDEFINED
> how[reg] is used for tracking the return address signing status and
> other how[reg] is not allowed here.

I've added the assert back and updated the comment.

Cheers,
Wilco

v3: Improve comments, add assert.

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..55fe35520106e848c5d4aea4e7104bf4a0c14891 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.  */
@@ -1204,10 +1203,15 @@ execute_cfa_program (const unsigned char *insn_ptr,
 	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.  */
+	     return address signing status.  The REG_UNDEFINED/UNSAVED states
+	     mean RA signing is enabled/disabled.  */
 	  reg = DWARF_REGNUM_AARCH64_RA_STATE;
-	  gcc_assert (fs->regs.how[reg] == REG_UNSAVED);
-	  fs->regs.reg[reg].loc.offset ^= 1;
+	  gcc_assert (fs->regs.how[reg] == REG_UNSAVED
+		      || fs->regs.how[reg] == REG_UNDEFINED);
+	  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)
  

On Tue, Jan 10, 2023 at 04:33:59PM +0000, Wilco Dijkstra via Gcc-patches wrote:
> @@ -1204,10 +1203,15 @@ execute_cfa_program (const unsigned char *insn_ptr,
>  	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.  */
> +	     return address signing status.  The REG_UNDEFINED/UNSAVED states
> +	     mean RA signing is enabled/disabled.  */
>  	  reg = DWARF_REGNUM_AARCH64_RA_STATE;
> -	  gcc_assert (fs->regs.how[reg] == REG_UNSAVED);
> -	  fs->regs.reg[reg].loc.offset ^= 1;
> +	  gcc_assert (fs->regs.how[reg] == REG_UNSAVED
> +		      || fs->regs.how[reg] == REG_UNDEFINED);
> +	  if (fs->regs.how[reg] == REG_UNSAVED)
> +	    fs->regs.how[reg] = REG_UNDEFINED;
> +	  else
> +	    fs->regs.how[reg] = REG_UNSAVED;

Wouldn't the assert be better written just as:
	  if (fs->regs.how[reg] == REG_UNSAVED)
	    fs->regs.how[reg] = REG_UNDEFINED;
	  else
	    {
	      gcc_assert (fs->regs.how[reg] == REG_UNDEFINED);
	      fs->regs.how[reg] = REG_UNSAVED;
	    }
?

Anyway, the sooner this makes it into gcc trunk, the better, it breaks quite
a lot of stuff.

	Jakub
  

On 1/10/23 19:12, Jakub Jelinek via Gcc-patches wrote:
> Anyway, the sooner this makes it into gcc trunk, the better, it breaks quite
> a lot of stuff.

Yep, please, we're also waiting for this patch for pushing to our gcc13 package.

Cheers,
Martin
  

Hi,

> On 1/10/23 19:12, Jakub Jelinek via Gcc-patches wrote:
>> Anyway, the sooner this makes it into gcc trunk, the better, it breaks quite
>> a lot of stuff.
>
> Yep, please, we're also waiting for this patch for pushing to our gcc13 package.

Well I'm waiting for an OK from a maintainer... I believe Jakub can approve it as well.

Cheers,
Wilco
  

On Wed, Jan 11, 2023 at 11:59:27AM +0000, Wilco Dijkstra wrote:
> Hi,
> 
> > On 1/10/23 19:12, Jakub Jelinek via Gcc-patches wrote:
> >> Anyway, the sooner this makes it into gcc trunk, the better, it breaks quite
> >> a lot of stuff.
> >
> > Yep, please, we're also waiting for this patch for pushing to our gcc13 package.
> 
> Well I'm waiting for an OK from a maintainer... I believe Jakub can approve it as well.

Yes, I can, but am not sure it is appropriate.  While I'm familiar with the
unwinder, I'm not familiar with the pointer signing, and AArch64 has active
maintainers, so I'd prefer to defer the review to them.

	Jakub
  

Jakub Jelinek <jakub@redhat.com> writes:
> On Wed, Jan 11, 2023 at 11:59:27AM +0000, Wilco Dijkstra wrote:
>> Hi,
>> 
>> > On 1/10/23 19:12, Jakub Jelinek via Gcc-patches wrote:
>> >> Anyway, the sooner this makes it into gcc trunk, the better, it breaks quite
>> >> a lot of stuff.
>> >
>> > Yep, please, we're also waiting for this patch for pushing to our gcc13 package.
>> 
>> Well I'm waiting for an OK from a maintainer... I believe Jakub can approve it as well.
>
> Yes, I can, but am not sure it is appropriate.  While I'm familiar with the
> unwinder, I'm not familiar with the pointer signing, and AArch64 has active
> maintainers, so I'd prefer to defer the review to them.

I think my main question is: how clean vs hacky is it to use
REG_UNDEFINED as effectively a toggle bit, rather than using
REG_UNDEFINED for its intended purpose?

In the review for earlier (May) patch, I'd asked whether it would
make sense to add a new enum.  Would that be OK from a target-independent
point of view?  E.g. maybe REG_TOGGLE_ON.

Although we don't AFAIK support using DW_CFA_undefined with RA signing,
the failure mode would be non-obvious: it would effectively toggle the
bit on.

It would be good to remove the definition of RA_SIGNED_BIT as well,
so that people don't accidentally use it in future.

Thanks,
Richard
  

Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> Jakub Jelinek <jakub@redhat.com> writes:
>> On Wed, Jan 11, 2023 at 11:59:27AM +0000, Wilco Dijkstra wrote:
>>> Hi,
>>> 
>>> > On 1/10/23 19:12, Jakub Jelinek via Gcc-patches wrote:
>>> >> Anyway, the sooner this makes it into gcc trunk, the better, it breaks quite
>>> >> a lot of stuff.
>>> >
>>> > Yep, please, we're also waiting for this patch for pushing to our gcc13 package.
>>> 
>>> Well I'm waiting for an OK from a maintainer... I believe Jakub can approve it as well.
>>
>> Yes, I can, but am not sure it is appropriate.  While I'm familiar with the
>> unwinder, I'm not familiar with the pointer signing, and AArch64 has active
>> maintainers, so I'd prefer to defer the review to them.
>
> I think my main question is: how clean vs hacky is it to use
> REG_UNDEFINED as effectively a toggle bit, rather than using
> REG_UNDEFINED for its intended purpose?
>
> In the review for earlier (May) patch, I'd asked whether it would
> make sense to add a new enum.  Would that be OK from a target-independent
> point of view?  E.g. maybe REG_TOGGLE_ON.
>
> Although we don't AFAIK support using DW_CFA_undefined with RA signing,
> the failure mode would be non-obvious: it would effectively toggle the
> bit on.
>
> It would be good to remove the definition of RA_SIGNED_BIT as well,
> so that people don't accidentally use it in future.

Sorry, just realised that the patch does do that.  But please remove
the comment too!

Richard
  

On Thu, Jan 12, 2023 at 12:05:45PM +0000, Richard Sandiford wrote:
> Jakub Jelinek <jakub@redhat.com> writes:
> > On Wed, Jan 11, 2023 at 11:59:27AM +0000, Wilco Dijkstra wrote:
> >> Hi,
> >> 
> >> > On 1/10/23 19:12, Jakub Jelinek via Gcc-patches wrote:
> >> >> Anyway, the sooner this makes it into gcc trunk, the better, it breaks quite
> >> >> a lot of stuff.
> >> >
> >> > Yep, please, we're also waiting for this patch for pushing to our gcc13 package.
> >> 
> >> Well I'm waiting for an OK from a maintainer... I believe Jakub can approve it as well.
> >
> > Yes, I can, but am not sure it is appropriate.  While I'm familiar with the
> > unwinder, I'm not familiar with the pointer signing, and AArch64 has active
> > maintainers, so I'd prefer to defer the review to them.
> 
> I think my main question is: how clean vs hacky is it to use
> REG_UNDEFINED as effectively a toggle bit, rather than using
> REG_UNDEFINED for its intended purpose?
> 
> In the review for earlier (May) patch, I'd asked whether it would
> make sense to add a new enum.  Would that be OK from a target-independent
> point of view?  E.g. maybe REG_TOGGLE_ON.
> 
> Although we don't AFAIK support using DW_CFA_undefined with RA signing,
> the failure mode would be non-obvious: it would effectively toggle the
> bit on.

We don't install unwind-dw2.h nor give user code access to the how array
(and it just lives on the stack of __frame_state_for/uw_init_context_1
functions and address of it is passed to functions called from it),
so I hope all this is private to the libgcc unwinder.  After all, otherwise
e.g. the change how "how" is represented couldn't be done.
That said, if new enum entries are added in the generic code, then
I think uw_update_context_1 will warn about unhandled case in a switch,
unless we e.g. change
      case REG_UNSAVED:
      case REG_UNDEFINED:
        break;
to
      default:
	break;
(and provided that the new enums would want such handling).
Another option is to just define the arch dependent value for how field
in the arch code, right now it is unsigned char type, so using say
(unsigned char) ~0 or (unsigned char) ~0 and (unsigned char) ~1 as arch
specific values might be ok too.

> It would be good to remove the definition of RA_SIGNED_BIT as well,
> so that people don't accidentally use it in future.

	Jakub
  

On Thu, Jan 12, 2023 at 01:28:59PM +0100, Jakub Jelinek wrote:
> > Although we don't AFAIK support using DW_CFA_undefined with RA signing,
> > the failure mode would be non-obvious: it would effectively toggle the
> > bit on.
> 
> We don't install unwind-dw2.h nor give user code access to the how array
> (and it just lives on the stack of __frame_state_for/uw_init_context_1
> functions and address of it is passed to functions called from it),
> so I hope all this is private to the libgcc unwinder.  After all, otherwise
> e.g. the change how "how" is represented couldn't be done.
> That said, if new enum entries are added in the generic code, then
> I think uw_update_context_1 will warn about unhandled case in a switch,
> unless we e.g. change
>       case REG_UNSAVED:
>       case REG_UNDEFINED:
>         break;
> to
>       default:
> 	break;
> (and provided that the new enums would want such handling).
> Another option is to just define the arch dependent value for how field
> in the arch code, right now it is unsigned char type, so using say
> (unsigned char) ~0 or (unsigned char) ~0 and (unsigned char) ~1 as arch
> specific values might be ok too.

Yet another option would be to define 1-2 extra REG_ values in the generic
unwind-dw2.h header, but name them
  REG_ARCH_SPECIFIC_1,
  REG_ARCH_SPECIFIC_2,
or so, and then the machine specific code can
#define REG_AARCH64_TOGGLE_ON REG_ARCH_SPECIFIC_1
Of course, all this depends on whether the arch specific codes can be
handled in uw_update_context_1 by doing break; there and nothing else.

	Jakub
  

Jakub Jelinek <jakub@redhat.com> writes:
> On Thu, Jan 12, 2023 at 01:28:59PM +0100, Jakub Jelinek wrote:
>> > Although we don't AFAIK support using DW_CFA_undefined with RA signing,
>> > the failure mode would be non-obvious: it would effectively toggle the
>> > bit on.
>> 
>> We don't install unwind-dw2.h nor give user code access to the how array
>> (and it just lives on the stack of __frame_state_for/uw_init_context_1
>> functions and address of it is passed to functions called from it),
>> so I hope all this is private to the libgcc unwinder.  After all, otherwise
>> e.g. the change how "how" is represented couldn't be done.
>> That said, if new enum entries are added in the generic code, then
>> I think uw_update_context_1 will warn about unhandled case in a switch,
>> unless we e.g. change
>>       case REG_UNSAVED:
>>       case REG_UNDEFINED:
>>         break;
>> to
>>       default:
>> 	break;
>> (and provided that the new enums would want such handling).

If we have a new enum, I think we should handle it explicitly.  The fact
that the information isn't propagated between frames is a key part of
the semantics.

>> Another option is to just define the arch dependent value for how field
>> in the arch code, right now it is unsigned char type, so using say
>> (unsigned char) ~0 or (unsigned char) ~0 and (unsigned char) ~1 as arch
>> specific values might be ok too.
>
> Yet another option would be to define 1-2 extra REG_ values in the generic
> unwind-dw2.h header, but name them
>   REG_ARCH_SPECIFIC_1,
>   REG_ARCH_SPECIFIC_2,
> or so, and then the machine specific code can
> #define REG_AARCH64_TOGGLE_ON REG_ARCH_SPECIFIC_1
> Of course, all this depends on whether the arch specific codes can be
> handled in uw_update_context_1 by doing break; there and nothing else.

Yeah, personally I'd prefer for target-independent code to provide
the toggle representation, even if it isn't widely used.

Thanks,
Richard
  

On Thu, Jan 12, 2023 at 03:22:56PM +0000, Richard Sandiford wrote:
> If we have a new enum, I think we should handle it explicitly.  The fact
> that the information isn't propagated between frames is a key part of
> the semantics.
> 
> >> Another option is to just define the arch dependent value for how field
> >> in the arch code, right now it is unsigned char type, so using say
> >> (unsigned char) ~0 or (unsigned char) ~0 and (unsigned char) ~1 as arch
> >> specific values might be ok too.
> >
> > Yet another option would be to define 1-2 extra REG_ values in the generic
> > unwind-dw2.h header, but name them
> >   REG_ARCH_SPECIFIC_1,
> >   REG_ARCH_SPECIFIC_2,
> > or so, and then the machine specific code can
> > #define REG_AARCH64_TOGGLE_ON REG_ARCH_SPECIFIC_1
> > Of course, all this depends on whether the arch specific codes can be
> > handled in uw_update_context_1 by doing break; there and nothing else.
> 
> Yeah, personally I'd prefer for target-independent code to provide
> the toggle representation, even if it isn't widely used.

I can live even with that, I just hope it won't make code generation worse
on other targets.
Anyway, I understood aarch64 needs 2 states for the signing, so one would
be REG_TOGGLE_ON and the other anything else?
Users can always create (invalid?) unwind info where they save the magic
register, make it undefined etc.

And
void bar (void), baz (void), boo (void), qux (void), corge (void);
enum {
  REG_UNSAVED,
  REG_SAVED_OFFSET,
  REG_SAVED_REG,
  REG_SAVED_EXP,
  REG_SAVED_VAL_OFFSET,
  REG_SAVED_VAL_EXP,
  REG_UNDEFINED
#ifdef ANOTHER
  , REG_TOGGLE_ON
#endif
};

void
foo (unsigned char c)
{
  switch (c)
    {
      case REG_UNSAVED:
      case REG_UNDEFINED:
#ifdef ANOTHER
      case REG_TOGGLE_ON:
#endif
        break;
          
      case REG_SAVED_OFFSET:
        bar ();
        break;
     
      case REG_SAVED_REG:
        baz ();
        break;
  
      case REG_SAVED_EXP:
        boo ();
        break;
  
      case REG_SAVED_VAL_OFFSET:
        qux ();
        break;

      case REG_SAVED_VAL_EXP:
        corge ();
        break;
    }
}
suggests that it doesn't, already cfg pass turns the implicit default:
into something that covers even the REG_UNSAVED, REG_UNDEFINED and maybe
REG_TOGGLE_ON values into default:

	Jakub
  

Jakub Jelinek <jakub@redhat.com> writes:
> On Thu, Jan 12, 2023 at 03:22:56PM +0000, Richard Sandiford wrote:
>> If we have a new enum, I think we should handle it explicitly.  The fact
>> that the information isn't propagated between frames is a key part of
>> the semantics.
>> 
>> >> Another option is to just define the arch dependent value for how field
>> >> in the arch code, right now it is unsigned char type, so using say
>> >> (unsigned char) ~0 or (unsigned char) ~0 and (unsigned char) ~1 as arch
>> >> specific values might be ok too.
>> >
>> > Yet another option would be to define 1-2 extra REG_ values in the generic
>> > unwind-dw2.h header, but name them
>> >   REG_ARCH_SPECIFIC_1,
>> >   REG_ARCH_SPECIFIC_2,
>> > or so, and then the machine specific code can
>> > #define REG_AARCH64_TOGGLE_ON REG_ARCH_SPECIFIC_1
>> > Of course, all this depends on whether the arch specific codes can be
>> > handled in uw_update_context_1 by doing break; there and nothing else.
>> 
>> Yeah, personally I'd prefer for target-independent code to provide
>> the toggle representation, even if it isn't widely used.
>
> I can live even with that, I just hope it won't make code generation worse
> on other targets.
> Anyway, I understood aarch64 needs 2 states for the signing, so one would
> be REG_TOGGLE_ON and the other anything else?

The other is the default (no signing), so it needs to be REG_UNSAVED.

> Users can always create (invalid?) unwind info where they save the magic
> register, make it undefined etc.
>
> And
> void bar (void), baz (void), boo (void), qux (void), corge (void);
> enum {
>   REG_UNSAVED,
>   REG_SAVED_OFFSET,
>   REG_SAVED_REG,
>   REG_SAVED_EXP,
>   REG_SAVED_VAL_OFFSET,
>   REG_SAVED_VAL_EXP,
>   REG_UNDEFINED
> #ifdef ANOTHER
>   , REG_TOGGLE_ON
> #endif
> };
>
> void
> foo (unsigned char c)
> {
>   switch (c)
>     {
>       case REG_UNSAVED:
>       case REG_UNDEFINED:
> #ifdef ANOTHER
>       case REG_TOGGLE_ON:
> #endif
>         break;
>           
>       case REG_SAVED_OFFSET:
>         bar ();
>         break;
>      
>       case REG_SAVED_REG:
>         baz ();
>         break;
>   
>       case REG_SAVED_EXP:
>         boo ();
>         break;
>   
>       case REG_SAVED_VAL_OFFSET:
>         qux ();
>         break;
>
>       case REG_SAVED_VAL_EXP:
>         corge ();
>         break;
>     }
> }
> suggests that it doesn't, already cfg pass turns the implicit default:
> into something that covers even the REG_UNSAVED, REG_UNDEFINED and maybe
> REG_TOGGLE_ON values into default:

OK, that's good.  Maybe having it behind a macro wouldn't be too bad though,
if it comes to that.

Thanks,
Richard
  

On Tue, Jan 10, 2023 at 04:33:59PM +0000, Wilco Dijkstra via Gcc-patches wrote:
> Hi Szabolcs,
> 
> > i would keep the assert: how[reg] must be either UNSAVED or UNDEFINED
> > here, other how[reg] means the toggle cfi instruction is mixed with
> > incompatible instructions for the pseudo reg.
> >
> > and i would add a comment about this e.g. saying that UNSAVED/UNDEFINED
> > how[reg] is used for tracking the return address signing status and
> > other how[reg] is not allowed here.
> 
> I've added the assert back and updated the comment.

BTW, the patch doesn't apply to trunk cleanly (since the January 2nd
r13-4955-gcb775ecd6e437 commit).

> v3: Improve comments, add assert.
> 
> 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.

	Jakub
  

On 1/12/23 19:57, Jakub Jelinek via Gcc-patches wrote:
> On Tue, Jan 10, 2023 at 04:33:59PM +0000, Wilco Dijkstra via Gcc-patches wrote:
>> Hi Szabolcs,
>>
>>> i would keep the assert: how[reg] must be either UNSAVED or UNDEFINED
>>> here, other how[reg] means the toggle cfi instruction is mixed with
>>> incompatible instructions for the pseudo reg.
>>>
>>> and i would add a comment about this e.g. saying that UNSAVED/UNDEFINED
>>> how[reg] is used for tracking the return address signing status and
>>> other how[reg] is not allowed here.
>>
>> I've added the assert back and updated the comment.
> 
> BTW, the patch doesn't apply to trunk cleanly (since the January 2nd
> r13-4955-gcb775ecd6e437 commit).

@Wilco, can you please send the rebased patch for patch review? We would
need in out openSUSE package soon.

Thank you,
Martin

> 
>> v3: Improve comments, add assert.
>>
>> 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.
> 
> 	Jakub
>
  

Hi,

> @Wilco, can you please send the rebased patch for patch review? We would
> need in out openSUSE package soon.

Here is an updated and rebased version:

Cheers,
Wilco

v4: rebase and add REG_UNSAVED_ARCHEXT.

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 a new state REG_UNSAVED_ARCHEXT.

Passes bootstrap & regress, OK for commit?

libgcc/
	PR target/107678
	* unwind-dw2.h (REG_UNSAVED_ARCHEXT): Add new enum.
	* unwind-dw2.c (uw_update_context_1): Add REG_UNSAVED_ARCHEXT case.
	* unwind-dw2-execute_cfa.h: Use REG_UNSAVED_ARCHEXT/REG_UNSAVED to 
	encode the 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 874cf6c3e77fb72d999f51b636d74cb0b5728bbd..727c27ba5da983958b3134715d9d4d7c0af5c1e2 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_UNSAVED_ARCHEXT
+      || (_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-execute_cfa.h b/libgcc/unwind-dw2-execute_cfa.h
index 264c11c03ec4a09cac2c19a241c5b110b1b6b602..aef377092ceede6bdda8532679f9b081c98fadce 100644
--- a/libgcc/unwind-dw2-execute_cfa.h
+++ b/libgcc/unwind-dw2-execute_cfa.h
@@ -278,10 +278,15 @@
 	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.  */
+	     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.reg[reg].loc.offset ^= 1;
+	  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;
 #else
 	  /* ??? Hardcoded for SPARC register window configuration.  */
 	  if (__LIBGCC_DWARF_FRAME_REGISTERS__ >= 32)
diff --git a/libgcc/unwind-dw2.h b/libgcc/unwind-dw2.h
index e2f81983e1dcf3df6aebde2454630b7bee87d597..53e1b183c7d60112a14411d3356c49cb39cd0de7 100644
--- a/libgcc/unwind-dw2.h
+++ b/libgcc/unwind-dw2.h
@@ -29,6 +29,7 @@ enum {
   REG_SAVED_EXP,
   REG_SAVED_VAL_OFFSET,
   REG_SAVED_VAL_EXP,
+  REG_UNSAVED_ARCHEXT,		/* Target specific extension.  */
   REG_UNDEFINED
 };
 
diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
index 9c5bf7821916d8ac8e10e25a7123cd03f848019a..d0afce7a9ea9f5b12a5a01ef1e940e1452b48cab 100644
--- a/libgcc/unwind-dw2.c
+++ b/libgcc/unwind-dw2.c
@@ -137,9 +137,6 @@ struct _Unwind_Context
 #define SIGNAL_FRAME_BIT ((~(_Unwind_Word) 0 >> 1) + 1)
   /* Context which has version/args_size/by_value fields.  */
 #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.  */
@@ -1200,6 +1197,7 @@ uw_update_context_1 (struct _Unwind_Context *context, _Unwind_FrameState *fs)
       {
       case REG_UNSAVED:
       case REG_UNDEFINED:
+      case REG_UNSAVED_ARCHEXT:
 	break;
 
       case REG_SAVED_OFFSET:
  

Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes:
> Hi,
>
>> @Wilco, can you please send the rebased patch for patch review? We would
>> need in out openSUSE package soon.
>
> Here is an updated and rebased version:
>
> Cheers,
> Wilco
>
> v4: rebase and add REG_UNSAVED_ARCHEXT.
>
> 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 a new state REG_UNSAVED_ARCHEXT.
>
> Passes bootstrap & regress, OK for commit?
>
> libgcc/
>         PR target/107678
>         * unwind-dw2.h (REG_UNSAVED_ARCHEXT): Add new enum.
>         * unwind-dw2.c (uw_update_context_1): Add REG_UNSAVED_ARCHEXT case.
>         * unwind-dw2-execute_cfa.h: Use REG_UNSAVED_ARCHEXT/REG_UNSAVED to
>         encode the 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 874cf6c3e77fb72d999f51b636d74cb0b5728bbd..727c27ba5da983958b3134715d9d4d7c0af5c1e2 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.  */

Needs updating to REG_UNSAVED_ARCHEXT.

OK with that changes, thanks, and sorry for the delays & runaround.

Richard

> +  if (fs->regs.how[reg] == REG_UNSAVED_ARCHEXT
> +      || (_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-execute_cfa.h b/libgcc/unwind-dw2-execute_cfa.h
> index 264c11c03ec4a09cac2c19a241c5b110b1b6b602..aef377092ceede6bdda8532679f9b081c98fadce 100644
> --- a/libgcc/unwind-dw2-execute_cfa.h
> +++ b/libgcc/unwind-dw2-execute_cfa.h
> @@ -278,10 +278,15 @@
>         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.  */
> +            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.reg[reg].loc.offset ^= 1;
> +         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;
>  #else
>           /* ??? Hardcoded for SPARC register window configuration.  */
>           if (__LIBGCC_DWARF_FRAME_REGISTERS__ >= 32)
> diff --git a/libgcc/unwind-dw2.h b/libgcc/unwind-dw2.h
> index e2f81983e1dcf3df6aebde2454630b7bee87d597..53e1b183c7d60112a14411d3356c49cb39cd0de7 100644
> --- a/libgcc/unwind-dw2.h
> +++ b/libgcc/unwind-dw2.h
> @@ -29,6 +29,7 @@ enum {
>    REG_SAVED_EXP,
>    REG_SAVED_VAL_OFFSET,
>    REG_SAVED_VAL_EXP,
> +  REG_UNSAVED_ARCHEXT,         /* Target specific extension.  */
>    REG_UNDEFINED
>  };
>
> diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
> index 9c5bf7821916d8ac8e10e25a7123cd03f848019a..d0afce7a9ea9f5b12a5a01ef1e940e1452b48cab 100644
> --- a/libgcc/unwind-dw2.c
> +++ b/libgcc/unwind-dw2.c
> @@ -137,9 +137,6 @@ struct _Unwind_Context
>  #define SIGNAL_FRAME_BIT ((~(_Unwind_Word) 0 >> 1) + 1)
>    /* Context which has version/args_size/by_value fields.  */
>  #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.  */
> @@ -1200,6 +1197,7 @@ uw_update_context_1 (struct _Unwind_Context *context, _Unwind_FrameState *fs)
>        {
>        case REG_UNSAVED:
>        case REG_UNDEFINED:
> +      case REG_UNSAVED_ARCHEXT:
>         break;
>
>        case REG_SAVED_OFFSET:
  

Hi,

>> +  /* Return-address signing state is toggled by DW_CFA_GNU_window_save (where
>> +     REG_UNDEFINED means enabled), or set by a DW_CFA_expression.  */
>
> Needs updating to REG_UNSAVED_ARCHEXT.
> 
> OK with that changes, thanks, and sorry for the delays & runaround.

Thanks, I've improved the comment and it has been committed to trunk now.

Cheers,
Wilco