public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Yvan Roux <yvan.roux@foss.st.com>
To: Luis Machado <luis.machado@arm.com>, <gdb-patches@sourceware.org>
Cc: Torbjorn SVENSSON <torbjorn.svensson@foss.st.com>
Subject: Re: [PATCH] gdb/arm: Unwind Non-Secure callbacks from Secure
Date: Fri, 24 Jun 2022 18:05:25 +0200	[thread overview]
Message-ID: <20220624160525.GA30742@gnbcxd0114.gnb.st.com> (raw)
In-Reply-To: <16e5e509-7094-df0d-285e-f8a676ed251b@arm.com>

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

  reply	other threads:[~2022-06-24 16:05 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-17 12:50 Yvan Roux
2022-06-21 11:28 ` Luis Machado
2022-06-24 16:05   ` Yvan Roux [this message]
2022-06-27  8:38     ` Luis Machado

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220624160525.GA30742@gnbcxd0114.gnb.st.com \
    --to=yvan.roux@foss.st.com \
    --cc=gdb-patches@sourceware.org \
    --cc=luis.machado@arm.com \
    --cc=torbjorn.svensson@foss.st.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).