[PATCH] gdb/arm: Unwind Non-Secure callbacks from Secure

[PATCH] gdb/arm: Unwind Non-Secure callbacks from Secure

[Yvan Roux]

Hi,

Without this changeset, the unwinding doesn't take into account
Non-Secure to Secure stack unwinding enablement status and
doesn't choose on the proper SP to do the unwinding.

This patch only unwind the stack when Non-Secure to Secure
unwinding is enabled, previous SP is set w/r to the current mode
(Handler -> msp_s, Thread -> psp_s) and then the Secure stack is
unwound. Ensure thumb bit is set in PSR when needed. Also, drop
thumb bit from PC if set.

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Signed-off-by: Yvan ROUX <yvan.roux@foss.st.com>
---
 gdb/arm-tdep.c | 121 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 91 insertions(+), 30 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 0c907482036..8a84754cfa6 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -309,6 +309,21 @@ struct arm_prologue_cache
   arm_prologue_cache() = default;
 };
 
+
+/* Reconstruct T bit in program status register from LR value.  */
+
+static inline ULONGEST
+reconstruct_t_bit(struct gdbarch *gdbarch, CORE_ADDR lr, ULONGEST psr)
+{
+  ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
+  if (IS_THUMB_ADDR (lr))
+    psr |= t_bit;
+  else
+    psr &= ~t_bit;
+
+  return psr;
+}
+
 /* Initialize stack pointers, and flag the active one.  */
 
 static inline void
@@ -2342,15 +2357,10 @@ arm_prologue_prev_register (struct frame_info *this_frame,
      but the processor status is likely valid.  */
   if (prev_regnum == ARM_PS_REGNUM)
     {
-      CORE_ADDR lr, cpsr;
-      ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
+      ULONGEST cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
+      CORE_ADDR lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
 
-      cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
-      lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
-      if (IS_THUMB_ADDR (lr))
-	cpsr |= t_bit;
-      else
-	cpsr &= ~t_bit;
+      cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
       return frame_unwind_got_constant (this_frame, prev_regnum, cpsr);
     }
 
@@ -3363,24 +3373,46 @@ arm_m_exception_cache (struct frame_info *this_frame)
       return cache;
     }
 
-  fnc_return = ((lr & 0xfffffffe) == 0xfefffffe);
+  fnc_return = (((lr >> 24) & 0xff) == 0xfe);
   if (tdep->have_sec_ext && fnc_return)
     {
-      int actual_sp;
+      if (!arm_unwind_secure_frames)
+	{
+	  warning (_("Non-secure to secure stack unwinding disabled."));
 
-      arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_ns_regnum);
-      arm_cache_set_active_sp_value (cache, tdep, sp);
-      if (lr & 1)
-	actual_sp = tdep->m_profile_msp_s_regnum;
+	  /* Terminate any further stack unwinding by referring to self.  */
+	  arm_cache_set_active_sp_value (cache, tdep, sp);
+	  return cache;
+	}
+
+      xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
+      if ((xpsr & 0xff) != 0)
+	/* Handler mode */
+	arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum);
       else
-	actual_sp = tdep->m_profile_msp_ns_regnum;
+	/* Thread mode */
+	arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_s_regnum);
+
+      unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
 
-      arm_cache_switch_prev_sp (cache, tdep, actual_sp);
-      sp = get_frame_register_unsigned (this_frame, actual_sp);
+      /* Stack layout for a function call from Secure to Non-Secure state
+	 (ARMv8-M section B3.16):
 
-      cache->saved_regs[ARM_LR_REGNUM].set_addr (sp);
+	    SP Offset
 
-      arm_cache_set_active_sp_value (cache, tdep, sp + 8);
+		    +-------------------+
+	     0x08   |                   |
+		    +-------------------+   <-- Original SP
+	     0x04   |    Partial xPSR   |
+		    +-------------------+
+	     0x00   |   Return Address  |
+		    +===================+   <-- New SP  */
+
+      cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + 0x00);
+      cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + 0x00);
+      cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + 0x04);
+
+      arm_cache_set_active_sp_value (cache, tdep, unwound_sp + 0x08);
 
       return cache;
     }
@@ -3441,11 +3473,6 @@ arm_m_exception_cache (struct frame_info *this_frame)
 	    arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum);
 	}
     }
-  else
-    {
-      /* Main stack used, use MSP as SP.  */
-      arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum);
-    }
 
   /* Fetch the SP to use for this frame.  */
   unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
@@ -3641,6 +3668,20 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
     return frame_unwind_got_constant (this_frame, prev_regnum,
 				      arm_cache_get_prev_sp_value (cache, tdep));
 
+  /* If we are asked to unwind the PC, strip the saved T bit.  */
+  if (prev_regnum == ARM_PC_REGNUM)
+    {
+      CORE_ADDR pc;
+      struct value *value;
+
+      value = trad_frame_get_prev_register (this_frame, cache->saved_regs,
+					    prev_regnum);
+
+      pc = value_as_address (value);
+      return frame_unwind_got_constant (this_frame, prev_regnum,
+				        UNMAKE_THUMB_ADDR (pc));
+    }
+
   /* The value might be one of the alternative SP, if so, use the
      value already constructed.  */
   if (arm_cache_is_sp_register (cache, tdep, prev_regnum))
@@ -3649,6 +3690,29 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
       return frame_unwind_got_constant (this_frame, prev_regnum, sp_value);
     }
 
+  /* If we are asked to unwind the xPSR, set T bit if PC is in thumb mode.
+   * LR register is unreliable as it contains FNC_RETURN or EXC_RETURN pattern.
+   */
+  if (prev_regnum == ARM_PS_REGNUM)
+    {
+      CORE_ADDR pc;
+      ULONGEST xpsr;
+      struct value *value;
+      struct gdbarch *gdbarch = get_frame_arch (this_frame);
+
+      value = trad_frame_get_prev_register (this_frame, cache->saved_regs,
+					    ARM_PC_REGNUM);
+      pc = value_as_address (value);
+
+      value = trad_frame_get_prev_register (this_frame, cache->saved_regs,
+					    ARM_PS_REGNUM);
+      xpsr = value_as_long (value);
+
+      /* Reconstruct the T bit; see arm_prologue_prev_register for details.  */
+      xpsr = reconstruct_t_bit (gdbarch, pc, xpsr);
+      return frame_unwind_got_constant (this_frame, ARM_PS_REGNUM, xpsr);
+    }
+
   return trad_frame_get_prev_register (this_frame, cache->saved_regs,
 				       prev_regnum);
 }
@@ -3711,8 +3775,8 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache,
 {
   struct gdbarch * gdbarch = get_frame_arch (this_frame);
   arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
-  CORE_ADDR lr, cpsr;
-  ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
+  CORE_ADDR lr;
+  ULONGEST cpsr;
 
   switch (regnum)
     {
@@ -3741,10 +3805,7 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache,
       /* Reconstruct the T bit; see arm_prologue_prev_register for details.  */
       cpsr = get_frame_register_unsigned (this_frame, regnum);
       lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
-      if (IS_THUMB_ADDR (lr))
-	cpsr |= t_bit;
-      else
-	cpsr &= ~t_bit;
+      cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
       return frame_unwind_got_constant (this_frame, regnum, cpsr);
 
     default:
-- 
2.17.1

Re: [PATCH] gdb/arm: Unwind Non-Secure callbacks from Secure

[Luis Machado]

Hi,

On 6/17/22 13:50, Yvan Roux wrote:
> Hi,
> 
> Without this changeset, the unwinding doesn't take into account
> Non-Secure to Secure stack unwinding enablement status and
> doesn't choose on the proper SP to do the unwinding.

choose on -> choose?

> 
> This patch only unwind the stack when Non-Secure to Secure

unwind -> unwinds
> unwinding is enabled, previous SP is set w/r to the current mode
> (Handler -> msp_s, Thread -> psp_s) and then the Secure stack is
> unwound. Ensure thumb bit is set in PSR when needed. Also, drop
> thumb bit from PC if set.

Also, two spaces after `.` according to the GNU Coding Standards.

> 
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> Signed-off-by: Yvan ROUX <yvan.roux@foss.st.com>
> ---
>   gdb/arm-tdep.c | 121 +++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 91 insertions(+), 30 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 0c907482036..8a84754cfa6 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -309,6 +309,21 @@ struct arm_prologue_cache
>     arm_prologue_cache() = default;
>   };
>   
> +
> +/* Reconstruct T bit in program status register from LR value.  */
> +
> +static inline ULONGEST
> +reconstruct_t_bit(struct gdbarch *gdbarch, CORE_ADDR lr, ULONGEST psr)
> +{
> +  ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
> +  if (IS_THUMB_ADDR (lr))
> +    psr |= t_bit;
> +  else
> +    psr &= ~t_bit;
> +
> +  return psr;
> +}
> +
>   /* Initialize stack pointers, and flag the active one.  */
>   
>   static inline void
> @@ -2342,15 +2357,10 @@ arm_prologue_prev_register (struct frame_info *this_frame,
>        but the processor status is likely valid.  */
>     if (prev_regnum == ARM_PS_REGNUM)
>       {
> -      CORE_ADDR lr, cpsr;
> -      ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
> +      ULONGEST cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
> +      CORE_ADDR lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
>   
> -      cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
> -      lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
> -      if (IS_THUMB_ADDR (lr))
> -	cpsr |= t_bit;
> -      else
> -	cpsr &= ~t_bit;
> +      cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
>         return frame_unwind_got_constant (this_frame, prev_regnum, cpsr);
>       }
>   
> @@ -3363,24 +3373,46 @@ arm_m_exception_cache (struct frame_info *this_frame)
>         return cache;
>       }
>   
> -  fnc_return = ((lr & 0xfffffffe) == 0xfefffffe);
> +  fnc_return = (((lr >> 24) & 0xff) == 0xfe);

Is the above throwing away the comparison with the lower byte 0xfe?

>     if (tdep->have_sec_ext && fnc_return)
>       {
> -      int actual_sp;
> +      if (!arm_unwind_secure_frames)
> +	{
> +	  warning (_("Non-secure to secure stack unwinding disabled."));
>   
> -      arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_ns_regnum);
> -      arm_cache_set_active_sp_value (cache, tdep, sp);
> -      if (lr & 1)
> -	actual_sp = tdep->m_profile_msp_s_regnum;
> +	  /* Terminate any further stack unwinding by referring to self.  */
> +	  arm_cache_set_active_sp_value (cache, tdep, sp);
> +	  return cache;
> +	}
> +
> +      xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
> +      if ((xpsr & 0xff) != 0)
> +	/* Handler mode */

What is the handler mode? Can we expand on that to make it clear?

> +	arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum);
>         else
> -	actual_sp = tdep->m_profile_msp_ns_regnum;
> +	/* Thread mode */

Similarly, what is the thread mode?

> +	arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_s_regnum);
> +
> +      unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
>   
> -      arm_cache_switch_prev_sp (cache, tdep, actual_sp);
> -      sp = get_frame_register_unsigned (this_frame, actual_sp);
> +      /* Stack layout for a function call from Secure to Non-Secure state
> +	 (ARMv8-M section B3.16):
>   
> -      cache->saved_regs[ARM_LR_REGNUM].set_addr (sp);
> +	    SP Offset
>   
> -      arm_cache_set_active_sp_value (cache, tdep, sp + 8);
> +		    +-------------------+
> +	     0x08   |                   |
> +		    +-------------------+   <-- Original SP
> +	     0x04   |    Partial xPSR   |
> +		    +-------------------+
> +	     0x00   |   Return Address  |
> +		    +===================+   <-- New SP  */
> +
> +      cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + 0x00);
> +      cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + 0x00);
> +      cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + 0x04);
> +
> +      arm_cache_set_active_sp_value (cache, tdep, unwound_sp + 0x08);
>   
>         return cache;
>       }
> @@ -3441,11 +3473,6 @@ arm_m_exception_cache (struct frame_info *this_frame)
>   	    arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum);
>   	}
>       }
> -  else
> -    {
> -      /* Main stack used, use MSP as SP.  */
> -      arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum);
> -    }
>   
>     /* Fetch the SP to use for this frame.  */
>     unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
> @@ -3641,6 +3668,20 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
>       return frame_unwind_got_constant (this_frame, prev_regnum,
>   				      arm_cache_get_prev_sp_value (cache, tdep));
>   
> +  /* If we are asked to unwind the PC, strip the saved T bit.  */
> +  if (prev_regnum == ARM_PC_REGNUM)
> +    {
> +      CORE_ADDR pc;
> +      struct value *value;
> +
> +      value = trad_frame_get_prev_register (this_frame, cache->saved_regs,
> +					    prev_regnum);
> +
> +      pc = value_as_address (value);
> +      return frame_unwind_got_constant (this_frame, prev_regnum,
> +				        UNMAKE_THUMB_ADDR (pc));
> +    }
> +
>     /* The value might be one of the alternative SP, if so, use the
>        value already constructed.  */
>     if (arm_cache_is_sp_register (cache, tdep, prev_regnum))
> @@ -3649,6 +3690,29 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
>         return frame_unwind_got_constant (this_frame, prev_regnum, sp_value);
>       }
>   
> +  /* If we are asked to unwind the xPSR, set T bit if PC is in thumb mode.
> +   * LR register is unreliable as it contains FNC_RETURN or EXC_RETURN pattern.
> +   */

I think this style of comments doesn't match GDB's coding standards.

> +  if (prev_regnum == ARM_PS_REGNUM)
> +    {
> +      CORE_ADDR pc;
> +      ULONGEST xpsr;


You could declare both pc and xpsr at their assignment locations.

> +      struct value *value;

Same thing as above. Declare it in its assignment.

> +      struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +
> +      value = trad_frame_get_prev_register (this_frame, cache->saved_regs,
> +					    ARM_PC_REGNUM);

here...

> +      pc = value_as_address (value);

here...
> +
> +      value = trad_frame_get_prev_register (this_frame, cache->saved_regs,
> +					    ARM_PS_REGNUM);
> +      xpsr = value_as_long (value);

and here.
> +
> +      /* Reconstruct the T bit; see arm_prologue_prev_register for details.  */

/* Reconstruct the T bit.  See arm_prologue_prev_register for details.  */

> +      xpsr = reconstruct_t_bit (gdbarch, pc, xpsr);
> +      return frame_unwind_got_constant (this_frame, ARM_PS_REGNUM, xpsr);
> +    }
> +
>     return trad_frame_get_prev_register (this_frame, cache->saved_regs,
>   				       prev_regnum);
>   }
> @@ -3711,8 +3775,8 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache,
>   {
>     struct gdbarch * gdbarch = get_frame_arch (this_frame);
>     arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
> -  CORE_ADDR lr, cpsr;
> -  ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
> +  CORE_ADDR lr;
> +  ULONGEST cpsr;
>   
>     switch (regnum)
>       {
> @@ -3741,10 +3805,7 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache,
>         /* Reconstruct the T bit; see arm_prologue_prev_register for details.  */
>         cpsr = get_frame_register_unsigned (this_frame, regnum);
>         lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
> -      if (IS_THUMB_ADDR (lr))
> -	cpsr |= t_bit;
> -      else
> -	cpsr &= ~t_bit;
> +      cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
>         return frame_unwind_got_constant (this_frame, regnum, cpsr);
>   
>       default

Re: [PATCH] gdb/arm: Unwind Non-Secure callbacks from Secure

[Yvan Roux]

Hi,

Thanks for the review Luis, here is a new version of the patch.

On Tue, Jun 21, 2022 at 12:28:07PM +0100, Luis Machado wrote:
> Hi,
> 
> On 6/17/22 13:50, Yvan Roux wrote:
> > Hi,
> > 
> > Without this changeset, the unwinding doesn't take into account
> > Non-Secure to Secure stack unwinding enablement status and
> > doesn't choose on the proper SP to do the unwinding.
> 
> choose on -> choose?
> 
> > 
> > This patch only unwind the stack when Non-Secure to Secure
> 
> unwind -> unwinds
> > unwinding is enabled, previous SP is set w/r to the current mode
> > (Handler -> msp_s, Thread -> psp_s) and then the Secure stack is
> > unwound. Ensure thumb bit is set in PSR when needed. Also, drop
> > thumb bit from PC if set.
> 
> Also, two spaces after `.` according to the GNU Coding Standards.
> 
> > 
> > Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> > Signed-off-by: Yvan ROUX <yvan.roux@foss.st.com>
> > ---
> >   gdb/arm-tdep.c | 121 +++++++++++++++++++++++++++++++++++++------------
> >   1 file changed, 91 insertions(+), 30 deletions(-)
> > 
> > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> > index 0c907482036..8a84754cfa6 100644
> > --- a/gdb/arm-tdep.c
> > +++ b/gdb/arm-tdep.c
> > @@ -309,6 +309,21 @@ struct arm_prologue_cache
> >     arm_prologue_cache() = default;
> >   };
> > +
> > +/* Reconstruct T bit in program status register from LR value.  */
> > +
> > +static inline ULONGEST
> > +reconstruct_t_bit(struct gdbarch *gdbarch, CORE_ADDR lr, ULONGEST psr)
> > +{
> > +  ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
> > +  if (IS_THUMB_ADDR (lr))
> > +    psr |= t_bit;
> > +  else
> > +    psr &= ~t_bit;
> > +
> > +  return psr;
> > +}
> > +
> >   /* Initialize stack pointers, and flag the active one.  */
> >   static inline void
> > @@ -2342,15 +2357,10 @@ arm_prologue_prev_register (struct frame_info *this_frame,
> >        but the processor status is likely valid.  */
> >     if (prev_regnum == ARM_PS_REGNUM)
> >       {
> > -      CORE_ADDR lr, cpsr;
> > -      ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
> > +      ULONGEST cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
> > +      CORE_ADDR lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
> > -      cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
> > -      lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
> > -      if (IS_THUMB_ADDR (lr))
> > -	cpsr |= t_bit;
> > -      else
> > -	cpsr &= ~t_bit;
> > +      cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
> >         return frame_unwind_got_constant (this_frame, prev_regnum, cpsr);
> >       }
> > @@ -3363,24 +3373,46 @@ arm_m_exception_cache (struct frame_info *this_frame)
> >         return cache;
> >       }
> > -  fnc_return = ((lr & 0xfffffffe) == 0xfefffffe);
> > +  fnc_return = (((lr >> 24) & 0xff) == 0xfe);
> 
> Is the above throwing away the comparison with the lower byte 0xfe?

Yes, it only checks the FNC_RETURN prefix for coherency with how EXC_RETURN is
handled and how it is also done in arm_m_addr_is_magic function (line 777), but
according to the reference manual bits [23:1] are all 1, so we can keep the
current comparison if you prefer.
 
> >     if (tdep->have_sec_ext && fnc_return)
> >       {
> > -      int actual_sp;
> > +      if (!arm_unwind_secure_frames)
> > +	{
> > +	  warning (_("Non-secure to secure stack unwinding disabled."));
> > -      arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_ns_regnum);
> > -      arm_cache_set_active_sp_value (cache, tdep, sp);
> > -      if (lr & 1)
> > -	actual_sp = tdep->m_profile_msp_s_regnum;
> > +	  /* Terminate any further stack unwinding by referring to self.  */
> > +	  arm_cache_set_active_sp_value (cache, tdep, sp);
> > +	  return cache;
> > +	}
> > +
> > +      xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
> > +      if ((xpsr & 0xff) != 0)
> > +	/* Handler mode */
> 
> What is the handler mode? Can we expand on that to make it clear?

This is the mode in which exceptions are handled and Threadmode is the normal
or main mode where a program runs in, the comment are more explicits in this
new version of the patch.

> > +	arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum);
> >         else
> > -	actual_sp = tdep->m_profile_msp_ns_regnum;
> > +	/* Thread mode */
> 
> Similarly, what is the thread mode?
> 
> > +	arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_s_regnum);
> > +
> > +      unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
> > -      arm_cache_switch_prev_sp (cache, tdep, actual_sp);
> > -      sp = get_frame_register_unsigned (this_frame, actual_sp);
> > +      /* Stack layout for a function call from Secure to Non-Secure state
> > +	 (ARMv8-M section B3.16):
> > -      cache->saved_regs[ARM_LR_REGNUM].set_addr (sp);
> > +	    SP Offset
> > -      arm_cache_set_active_sp_value (cache, tdep, sp + 8);
> > +		    +-------------------+
> > +	     0x08   |                   |
> > +		    +-------------------+   <-- Original SP
> > +	     0x04   |    Partial xPSR   |
> > +		    +-------------------+
> > +	     0x00   |   Return Address  |
> > +		    +===================+   <-- New SP  */
> > +
> > +      cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + 0x00);
> > +      cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + 0x00);
> > +      cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + 0x04);
> > +
> > +      arm_cache_set_active_sp_value (cache, tdep, unwound_sp + 0x08);
> >         return cache;
> >       }
> > @@ -3441,11 +3473,6 @@ arm_m_exception_cache (struct frame_info *this_frame)
> >   	    arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum);
> >   	}
> >       }
> > -  else
> > -    {
> > -      /* Main stack used, use MSP as SP.  */
> > -      arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum);
> > -    }
> >     /* Fetch the SP to use for this frame.  */
> >     unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
> > @@ -3641,6 +3668,20 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
> >       return frame_unwind_got_constant (this_frame, prev_regnum,
> >   				      arm_cache_get_prev_sp_value (cache, tdep));
> > +  /* If we are asked to unwind the PC, strip the saved T bit.  */
> > +  if (prev_regnum == ARM_PC_REGNUM)
> > +    {
> > +      CORE_ADDR pc;
> > +      struct value *value;
> > +
> > +      value = trad_frame_get_prev_register (this_frame, cache->saved_regs,
> > +					    prev_regnum);
> > +
> > +      pc = value_as_address (value);
> > +      return frame_unwind_got_constant (this_frame, prev_regnum,
> > +				        UNMAKE_THUMB_ADDR (pc));
> > +    }
> > +
> >     /* The value might be one of the alternative SP, if so, use the
> >        value already constructed.  */
> >     if (arm_cache_is_sp_register (cache, tdep, prev_regnum))
> > @@ -3649,6 +3690,29 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
> >         return frame_unwind_got_constant (this_frame, prev_regnum, sp_value);
> >       }
> > +  /* If we are asked to unwind the xPSR, set T bit if PC is in thumb mode.
> > +   * LR register is unreliable as it contains FNC_RETURN or EXC_RETURN pattern.
> > +   */
> 
> I think this style of comments doesn't match GDB's coding standards.
> 
> > +  if (prev_regnum == ARM_PS_REGNUM)
> > +    {
> > +      CORE_ADDR pc;
> > +      ULONGEST xpsr;
> 
> 
> You could declare both pc and xpsr at their assignment locations.
> 
> > +      struct value *value;
> 
> Same thing as above. Declare it in its assignment.
> 
> > +      struct gdbarch *gdbarch = get_frame_arch (this_frame);
> > +
> > +      value = trad_frame_get_prev_register (this_frame, cache->saved_regs,
> > +					    ARM_PC_REGNUM);
> 
> here...
> 
> > +      pc = value_as_address (value);
> 
> here...
> > +
> > +      value = trad_frame_get_prev_register (this_frame, cache->saved_regs,
> > +					    ARM_PS_REGNUM);
> > +      xpsr = value_as_long (value);
> 
> and here.
> > +
> > +      /* Reconstruct the T bit; see arm_prologue_prev_register for details.  */
> 
> /* Reconstruct the T bit.  See arm_prologue_prev_register for details.  */
> 
> > +      xpsr = reconstruct_t_bit (gdbarch, pc, xpsr);
> > +      return frame_unwind_got_constant (this_frame, ARM_PS_REGNUM, xpsr);
> > +    }
> > +
> >     return trad_frame_get_prev_register (this_frame, cache->saved_regs,
> >   				       prev_regnum);
> >   }
> > @@ -3711,8 +3775,8 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache,
> >   {
> >     struct gdbarch * gdbarch = get_frame_arch (this_frame);
> >     arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
> > -  CORE_ADDR lr, cpsr;
> > -  ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
> > +  CORE_ADDR lr;
> > +  ULONGEST cpsr;
> >     switch (regnum)
> >       {
> > @@ -3741,10 +3805,7 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache,
> >         /* Reconstruct the T bit; see arm_prologue_prev_register for details.  */
> >         cpsr = get_frame_register_unsigned (this_frame, regnum);
> >         lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
> > -      if (IS_THUMB_ADDR (lr))
> > -	cpsr |= t_bit;
> > -      else
> > -	cpsr &= ~t_bit;
> > +      cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
> >         return frame_unwind_got_constant (this_frame, regnum, cpsr);
> >       default

Updated version:

gdb/arm: Unwind Non-Secure callbacks from Secure

Without this changeset, the unwinding doesn't take into account
Non-Secure to Secure stack unwinding enablement status and
doesn't choose the proper SP to do the unwinding.

This patch only unwinds the stack when Non-Secure to Secure
unwinding is enabled, previous SP is set w/r to the current mode
(Handler -> msp_s, Thread -> psp_s) and then the Secure stack is
unwound.  Ensure thumb bit is set in PSR when needed. Also, drop
thumb bit from PC if set.

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Signed-off-by: Yvan ROUX <yvan.roux@foss.st.com>
---
 gdb/arm-tdep.c | 117 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 87 insertions(+), 30 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 96d70d40b28..8ae0c9fecd7 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -309,6 +309,21 @@ struct arm_prologue_cache
   arm_prologue_cache() = default;
 };
 
+
+/* Reconstruct T bit in program status register from LR value.  */
+
+static inline ULONGEST
+reconstruct_t_bit(struct gdbarch *gdbarch, CORE_ADDR lr, ULONGEST psr)
+{
+  ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
+  if (IS_THUMB_ADDR (lr))
+    psr |= t_bit;
+  else
+    psr &= ~t_bit;
+
+  return psr;
+}
+
 /* Initialize stack pointers, and flag the active one.  */
 
 static inline void
@@ -2348,15 +2363,10 @@ arm_prologue_prev_register (struct frame_info *this_frame,
      but the processor status is likely valid.  */
   if (prev_regnum == ARM_PS_REGNUM)
     {
-      CORE_ADDR lr, cpsr;
-      ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
+      ULONGEST cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
+      CORE_ADDR lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
 
-      cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
-      lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
-      if (IS_THUMB_ADDR (lr))
-	cpsr |= t_bit;
-      else
-	cpsr &= ~t_bit;
+      cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
       return frame_unwind_got_constant (this_frame, prev_regnum, cpsr);
     }
 
@@ -3369,24 +3379,46 @@ arm_m_exception_cache (struct frame_info *this_frame)
       return cache;
     }
 
-  fnc_return = ((lr & 0xfffffffe) == 0xfefffffe);
+  fnc_return = (((lr >> 24) & 0xff) == 0xfe);
   if (tdep->have_sec_ext && fnc_return)
     {
-      int actual_sp;
+      if (!arm_unwind_secure_frames)
+	{
+	  warning (_("Non-secure to secure stack unwinding disabled."));
 
-      arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_ns_regnum);
-      arm_cache_set_active_sp_value (cache, tdep, sp);
-      if (lr & 1)
-	actual_sp = tdep->m_profile_msp_s_regnum;
+	  /* Terminate any further stack unwinding by referring to self.  */
+	  arm_cache_set_active_sp_value (cache, tdep, sp);
+	  return cache;
+	}
+
+      xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
+      if ((xpsr & 0xff) != 0)
+	/* Handler mode: This is mode that exceptions are handled in.  */
+	arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum);
       else
-	actual_sp = tdep->m_profile_msp_ns_regnum;
+	/* Thread mode: This is the normal mode that programs run in.  */
+	arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_s_regnum);
+
+      unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
 
-      arm_cache_switch_prev_sp (cache, tdep, actual_sp);
-      sp = get_frame_register_unsigned (this_frame, actual_sp);
+      /* Stack layout for a function call from Secure to Non-Secure state
+	 (ARMv8-M section B3.16):
 
-      cache->saved_regs[ARM_LR_REGNUM].set_addr (sp);
+	    SP Offset
 
-      arm_cache_set_active_sp_value (cache, tdep, sp + 8);
+		    +-------------------+
+	     0x08   |                   |
+		    +-------------------+   <-- Original SP
+	     0x04   |    Partial xPSR   |
+		    +-------------------+
+	     0x00   |   Return Address  |
+		    +===================+   <-- New SP  */
+
+      cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + 0x00);
+      cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + 0x00);
+      cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + 0x04);
+
+      arm_cache_set_active_sp_value (cache, tdep, unwound_sp + 0x08);
 
       return cache;
     }
@@ -3447,11 +3479,6 @@ arm_m_exception_cache (struct frame_info *this_frame)
 	    arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum);
 	}
     }
-  else
-    {
-      /* Main stack used, use MSP as SP.  */
-      arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum);
-    }
 
   /* Fetch the SP to use for this frame.  */
   unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
@@ -3647,6 +3674,20 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
     return frame_unwind_got_constant (this_frame, prev_regnum,
 				      arm_cache_get_prev_sp_value (cache, tdep));
 
+  /* If we are asked to unwind the PC, strip the saved T bit.  */
+  if (prev_regnum == ARM_PC_REGNUM)
+    {
+      CORE_ADDR pc;
+      struct value *value;
+
+      value = trad_frame_get_prev_register (this_frame, cache->saved_regs,
+					    prev_regnum);
+
+      pc = value_as_address (value);
+      return frame_unwind_got_constant (this_frame, prev_regnum,
+				        UNMAKE_THUMB_ADDR (pc));
+    }
+
   /* The value might be one of the alternative SP, if so, use the
      value already constructed.  */
   if (arm_cache_is_sp_register (cache, tdep, prev_regnum))
@@ -3655,6 +3696,25 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
       return frame_unwind_got_constant (this_frame, prev_regnum, sp_value);
     }
 
+  /* If we are asked to unwind the xPSR, set T bit if PC is in thumb mode.
+     LR register is unreliable as it contains FNC_RETURN or EXC_RETURN
+     pattern.  */
+  if (prev_regnum == ARM_PS_REGNUM)
+    {
+      struct gdbarch *gdbarch = get_frame_arch (this_frame);
+      struct value *value = trad_frame_get_prev_register (this_frame,
+							  cache->saved_regs,
+							  ARM_PC_REGNUM);
+      CORE_ADDR pc = value_as_address (value);
+      value = trad_frame_get_prev_register (this_frame, cache->saved_regs,
+					    ARM_PS_REGNUM);
+      ULONGEST xpsr = value_as_long (value);
+
+      /* Reconstruct the T bit; see arm_prologue_prev_register for details.  */
+      xpsr = reconstruct_t_bit (gdbarch, pc, xpsr);
+      return frame_unwind_got_constant (this_frame, ARM_PS_REGNUM, xpsr);
+    }
+
   return trad_frame_get_prev_register (this_frame, cache->saved_regs,
 				       prev_regnum);
 }
@@ -3717,8 +3777,8 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache,
 {
   struct gdbarch * gdbarch = get_frame_arch (this_frame);
   arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
-  CORE_ADDR lr, cpsr;
-  ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
+  CORE_ADDR lr;
+  ULONGEST cpsr;
 
   switch (regnum)
     {
@@ -3747,10 +3807,7 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache,
       /* Reconstruct the T bit; see arm_prologue_prev_register for details.  */
       cpsr = get_frame_register_unsigned (this_frame, regnum);
       lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
-      if (IS_THUMB_ADDR (lr))
-	cpsr |= t_bit;
-      else
-	cpsr &= ~t_bit;
+      cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
       return frame_unwind_got_constant (this_frame, regnum, cpsr);
 
     default:
-- 
2.17.1

Re: [PATCH] gdb/arm: Unwind Non-Secure callbacks from Secure

[Luis Machado]

On 6/24/22 17:05, Yvan Roux wrote:
> Hi,
> 
> Thanks for the review Luis, here is a new version of the patch.
> 
> On Tue, Jun 21, 2022 at 12:28:07PM +0100, Luis Machado wrote:
>> Hi,
>>
>> On 6/17/22 13:50, Yvan Roux wrote:
>>> Hi,
>>>
>>> Without this changeset, the unwinding doesn't take into account
>>> Non-Secure to Secure stack unwinding enablement status and
>>> doesn't choose on the proper SP to do the unwinding.
>>
>> choose on -> choose?
>>
>>>
>>> This patch only unwind the stack when Non-Secure to Secure
>>
>> unwind -> unwinds
>>> unwinding is enabled, previous SP is set w/r to the current mode
>>> (Handler -> msp_s, Thread -> psp_s) and then the Secure stack is
>>> unwound. Ensure thumb bit is set in PSR when needed. Also, drop
>>> thumb bit from PC if set.
>>
>> Also, two spaces after `.` according to the GNU Coding Standards.
>>
>>>
>>> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
>>> Signed-off-by: Yvan ROUX <yvan.roux@foss.st.com>
>>> ---
>>>    gdb/arm-tdep.c | 121 +++++++++++++++++++++++++++++++++++++------------
>>>    1 file changed, 91 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>>> index 0c907482036..8a84754cfa6 100644
>>> --- a/gdb/arm-tdep.c
>>> +++ b/gdb/arm-tdep.c
>>> @@ -309,6 +309,21 @@ struct arm_prologue_cache
>>>      arm_prologue_cache() = default;
>>>    };
>>> +
>>> +/* Reconstruct T bit in program status register from LR value.  */
>>> +
>>> +static inline ULONGEST
>>> +reconstruct_t_bit(struct gdbarch *gdbarch, CORE_ADDR lr, ULONGEST psr)
>>> +{
>>> +  ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
>>> +  if (IS_THUMB_ADDR (lr))
>>> +    psr |= t_bit;
>>> +  else
>>> +    psr &= ~t_bit;
>>> +
>>> +  return psr;
>>> +}
>>> +
>>>    /* Initialize stack pointers, and flag the active one.  */
>>>    static inline void
>>> @@ -2342,15 +2357,10 @@ arm_prologue_prev_register (struct frame_info *this_frame,
>>>         but the processor status is likely valid.  */
>>>      if (prev_regnum == ARM_PS_REGNUM)
>>>        {
>>> -      CORE_ADDR lr, cpsr;
>>> -      ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
>>> +      ULONGEST cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
>>> +      CORE_ADDR lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
>>> -      cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
>>> -      lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
>>> -      if (IS_THUMB_ADDR (lr))
>>> -	cpsr |= t_bit;
>>> -      else
>>> -	cpsr &= ~t_bit;
>>> +      cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
>>>          return frame_unwind_got_constant (this_frame, prev_regnum, cpsr);
>>>        }
>>> @@ -3363,24 +3373,46 @@ arm_m_exception_cache (struct frame_info *this_frame)
>>>          return cache;
>>>        }
>>> -  fnc_return = ((lr & 0xfffffffe) == 0xfefffffe);
>>> +  fnc_return = (((lr >> 24) & 0xff) == 0xfe);
>>
>> Is the above throwing away the comparison with the lower byte 0xfe?
> 
> Yes, it only checks the FNC_RETURN prefix for coherency with how EXC_RETURN is
> handled and how it is also done in arm_m_addr_is_magic function (line 777), but
> according to the reference manual bits [23:1] are all 1, so we can keep the
> current comparison if you prefer.
>   

Thanks for the explanation. No need to keep it if it doesn't make sense.

I just wanted to make sure we're not dropping something that may be used by other targets.

>>>      if (tdep->have_sec_ext && fnc_return)
>>>        {
>>> -      int actual_sp;
>>> +      if (!arm_unwind_secure_frames)
>>> +	{
>>> +	  warning (_("Non-secure to secure stack unwinding disabled."));
>>> -      arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_ns_regnum);
>>> -      arm_cache_set_active_sp_value (cache, tdep, sp);
>>> -      if (lr & 1)
>>> -	actual_sp = tdep->m_profile_msp_s_regnum;
>>> +	  /* Terminate any further stack unwinding by referring to self.  */
>>> +	  arm_cache_set_active_sp_value (cache, tdep, sp);
>>> +	  return cache;
>>> +	}
>>> +
>>> +      xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
>>> +      if ((xpsr & 0xff) != 0)
>>> +	/* Handler mode */
>>
>> What is the handler mode? Can we expand on that to make it clear?
> 
> This is the mode in which exceptions are handled and Threadmode is the normal
> or main mode where a program runs in, the comment are more explicits in this
> new version of the patch.
> 

Thanks.

>>> +	arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum);
>>>          else
>>> -	actual_sp = tdep->m_profile_msp_ns_regnum;
>>> +	/* Thread mode */
>>
>> Similarly, what is the thread mode?
>>
>>> +	arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_s_regnum);
>>> +
>>> +      unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
>>> -      arm_cache_switch_prev_sp (cache, tdep, actual_sp);
>>> -      sp = get_frame_register_unsigned (this_frame, actual_sp);
>>> +      /* Stack layout for a function call from Secure to Non-Secure state
>>> +	 (ARMv8-M section B3.16):
>>> -      cache->saved_regs[ARM_LR_REGNUM].set_addr (sp);
>>> +	    SP Offset
>>> -      arm_cache_set_active_sp_value (cache, tdep, sp + 8);
>>> +		    +-------------------+
>>> +	     0x08   |                   |
>>> +		    +-------------------+   <-- Original SP
>>> +	     0x04   |    Partial xPSR   |
>>> +		    +-------------------+
>>> +	     0x00   |   Return Address  |
>>> +		    +===================+   <-- New SP  */
>>> +
>>> +      cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + 0x00);
>>> +      cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + 0x00);
>>> +      cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + 0x04);
>>> +
>>> +      arm_cache_set_active_sp_value (cache, tdep, unwound_sp + 0x08);
>>>          return cache;
>>>        }
>>> @@ -3441,11 +3473,6 @@ arm_m_exception_cache (struct frame_info *this_frame)
>>>    	    arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum);
>>>    	}
>>>        }
>>> -  else
>>> -    {
>>> -      /* Main stack used, use MSP as SP.  */
>>> -      arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum);
>>> -    }
>>>      /* Fetch the SP to use for this frame.  */
>>>      unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
>>> @@ -3641,6 +3668,20 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
>>>        return frame_unwind_got_constant (this_frame, prev_regnum,
>>>    				      arm_cache_get_prev_sp_value (cache, tdep));
>>> +  /* If we are asked to unwind the PC, strip the saved T bit.  */
>>> +  if (prev_regnum == ARM_PC_REGNUM)
>>> +    {
>>> +      CORE_ADDR pc;
>>> +      struct value *value;
>>> +
>>> +      value = trad_frame_get_prev_register (this_frame, cache->saved_regs,
>>> +					    prev_regnum);
>>> +
>>> +      pc = value_as_address (value);
>>> +      return frame_unwind_got_constant (this_frame, prev_regnum,
>>> +				        UNMAKE_THUMB_ADDR (pc));
>>> +    }
>>> +
>>>      /* The value might be one of the alternative SP, if so, use the
>>>         value already constructed.  */
>>>      if (arm_cache_is_sp_register (cache, tdep, prev_regnum))
>>> @@ -3649,6 +3690,29 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
>>>          return frame_unwind_got_constant (this_frame, prev_regnum, sp_value);
>>>        }
>>> +  /* If we are asked to unwind the xPSR, set T bit if PC is in thumb mode.
>>> +   * LR register is unreliable as it contains FNC_RETURN or EXC_RETURN pattern.
>>> +   */
>>
>> I think this style of comments doesn't match GDB's coding standards.
>>
>>> +  if (prev_regnum == ARM_PS_REGNUM)
>>> +    {
>>> +      CORE_ADDR pc;
>>> +      ULONGEST xpsr;
>>
>>
>> You could declare both pc and xpsr at their assignment locations.
>>
>>> +      struct value *value;
>>
>> Same thing as above. Declare it in its assignment.
>>
>>> +      struct gdbarch *gdbarch = get_frame_arch (this_frame);
>>> +
>>> +      value = trad_frame_get_prev_register (this_frame, cache->saved_regs,
>>> +					    ARM_PC_REGNUM);
>>
>> here...
>>
>>> +      pc = value_as_address (value);
>>
>> here...
>>> +
>>> +      value = trad_frame_get_prev_register (this_frame, cache->saved_regs,
>>> +					    ARM_PS_REGNUM);
>>> +      xpsr = value_as_long (value);
>>
>> and here.
>>> +
>>> +      /* Reconstruct the T bit; see arm_prologue_prev_register for details.  */
>>
>> /* Reconstruct the T bit.  See arm_prologue_prev_register for details.  */
>>
>>> +      xpsr = reconstruct_t_bit (gdbarch, pc, xpsr);
>>> +      return frame_unwind_got_constant (this_frame, ARM_PS_REGNUM, xpsr);
>>> +    }
>>> +
>>>      return trad_frame_get_prev_register (this_frame, cache->saved_regs,
>>>    				       prev_regnum);
>>>    }
>>> @@ -3711,8 +3775,8 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache,
>>>    {
>>>      struct gdbarch * gdbarch = get_frame_arch (this_frame);
>>>      arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
>>> -  CORE_ADDR lr, cpsr;
>>> -  ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
>>> +  CORE_ADDR lr;
>>> +  ULONGEST cpsr;
>>>      switch (regnum)
>>>        {
>>> @@ -3741,10 +3805,7 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache,
>>>          /* Reconstruct the T bit; see arm_prologue_prev_register for details.  */
>>>          cpsr = get_frame_register_unsigned (this_frame, regnum);
>>>          lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
>>> -      if (IS_THUMB_ADDR (lr))
>>> -	cpsr |= t_bit;
>>> -      else
>>> -	cpsr &= ~t_bit;
>>> +      cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
>>>          return frame_unwind_got_constant (this_frame, regnum, cpsr);
>>>        default
> 
> Updated version:
> 
> gdb/arm: Unwind Non-Secure callbacks from Secure
> 
> Without this changeset, the unwinding doesn't take into account
> Non-Secure to Secure stack unwinding enablement status and
> doesn't choose the proper SP to do the unwinding.
> 
> This patch only unwinds the stack when Non-Secure to Secure
> unwinding is enabled, previous SP is set w/r to the current mode
> (Handler -> msp_s, Thread -> psp_s) and then the Secure stack is
> unwound.  Ensure thumb bit is set in PSR when needed. Also, drop
> thumb bit from PC if set.
> 
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> Signed-off-by: Yvan ROUX <yvan.roux@foss.st.com>
> ---
>   gdb/arm-tdep.c | 117 ++++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 87 insertions(+), 30 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 96d70d40b28..8ae0c9fecd7 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -309,6 +309,21 @@ struct arm_prologue_cache
>     arm_prologue_cache() = default;
>   };
>   
> +
> +/* Reconstruct T bit in program status register from LR value.  */
> +
> +static inline ULONGEST
> +reconstruct_t_bit(struct gdbarch *gdbarch, CORE_ADDR lr, ULONGEST psr)
> +{
> +  ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
> +  if (IS_THUMB_ADDR (lr))
> +    psr |= t_bit;
> +  else
> +    psr &= ~t_bit;
> +
> +  return psr;
> +}
> +
>   /* Initialize stack pointers, and flag the active one.  */
>   
>   static inline void
> @@ -2348,15 +2363,10 @@ arm_prologue_prev_register (struct frame_info *this_frame,
>        but the processor status is likely valid.  */
>     if (prev_regnum == ARM_PS_REGNUM)
>       {
> -      CORE_ADDR lr, cpsr;
> -      ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
> +      ULONGEST cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
> +      CORE_ADDR lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
>   
> -      cpsr = get_frame_register_unsigned (this_frame, prev_regnum);
> -      lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
> -      if (IS_THUMB_ADDR (lr))
> -	cpsr |= t_bit;
> -      else
> -	cpsr &= ~t_bit;
> +      cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
>         return frame_unwind_got_constant (this_frame, prev_regnum, cpsr);
>       }
>   
> @@ -3369,24 +3379,46 @@ arm_m_exception_cache (struct frame_info *this_frame)
>         return cache;
>       }
>   
> -  fnc_return = ((lr & 0xfffffffe) == 0xfefffffe);
> +  fnc_return = (((lr >> 24) & 0xff) == 0xfe);
>     if (tdep->have_sec_ext && fnc_return)
>       {
> -      int actual_sp;
> +      if (!arm_unwind_secure_frames)
> +	{
> +	  warning (_("Non-secure to secure stack unwinding disabled."));
>   
> -      arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_ns_regnum);
> -      arm_cache_set_active_sp_value (cache, tdep, sp);
> -      if (lr & 1)
> -	actual_sp = tdep->m_profile_msp_s_regnum;
> +	  /* Terminate any further stack unwinding by referring to self.  */
> +	  arm_cache_set_active_sp_value (cache, tdep, sp);
> +	  return cache;
> +	}
> +
> +      xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
> +      if ((xpsr & 0xff) != 0)
> +	/* Handler mode: This is mode that exceptions are handled in.  */


This is mode -> This is the mode.

Otherwise LGTM.

Thanks!