[PATCH 0/3] gdb/arm: Fixes for Cortex-M stack unwinding

[PATCH 0/3] gdb/arm: Fixes for Cortex-M stack unwinding

[Torbjörn SVENSSON]

Hi,

This patchset attempts to attack the issues reported in pr/29716 and
pr/29738.

Based on my testing on a STM32L552 with TrustZone, it appears to work as
expected, but I'd love to have another set of eyes on this!

Most of the items reported can be seen using the GTZC example from ST:
https://github.com/STMicroelectronics/STM32CubeL5/tree/master/Projects/NUCLEO-L552ZE-Q/Examples/GTZC/GTZC_TZSC_MPCBB_TrustZone

I've manually verfied the register content by setting break points in
* main
* Error_Handler
* SecureFault_Handler


For the dwarf2 problem in pr/29738, I added 7-chained functions like
below and placed a breakpoint at myfunc7:

  void myfunc7()
    {
      HAL_Delay(1);
    }
  ...
  void myfunc1()
    {
      HAL_Delay(1);
      myfunc2();
    }

For each breakpoint, I then used the below macros to print the
registers. For each of the printed frames, I then manually checked if
the values maked sense.

define show_reg
  shell printf "%-8s " '$arg0'
  p/x $arg0
end
define show_stacks
  shell echo
  shell echo -e "\x1b[35mContent at $arg0\x1b[0m"
  bt
  set $i = 0
  while $i <= $arg1
    f $i
    show_reg $lr
    show_reg $pc
    show_reg $sp
    show_reg $msp
    show_reg $msp_s
    show_reg $msp_ns
    show_reg $psp
    show_reg $psp_s
    show_reg $psp_ns

    shell echo
    set $i = $i + 1
  end
  shell echo
  shell echo
end


The show_stacks macro takes 2 arguments, a description and the number of
frames to print registers for.
To show the registers for all the frames when at the Error_Handler
breakpoint, I used:
show_stacks "Error_Handler" 7


With this series applied, I've not been able to reproduce the stack
trace shown in comment 0 in pr/29716. I'm not sure if the series fixes
the issue, or if it still there but I'm doing something wrong.


Kind regards,
Torbjörn

[PATCH 1/3] gdb/arm: Update active msp/psp when switching stack

[Torbjörn SVENSSON]

For targets with secext, msp and psp can be seen as an alias for one
of msp_s, msp_ns, psp_s or psp_ns. When switching active sp, the
coresponding msp/psp needs to be switched too.

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
---
 gdb/arm-tdep.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 7cb3f5f3050..124a94dc87d 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -504,8 +504,23 @@ arm_cache_switch_prev_sp (struct arm_prologue_cache *cache,
   gdb_assert (arm_is_alternative_sp_register (tdep, sp_regnum));
 
   if (tdep->have_sec_ext)
-    gdb_assert (sp_regnum != tdep->m_profile_msp_regnum
-		&& sp_regnum != tdep->m_profile_psp_regnum);
+    {
+      gdb_assert (sp_regnum != tdep->m_profile_msp_regnum
+		  && sp_regnum != tdep->m_profile_psp_regnum);
+
+      if (sp_regnum == tdep->m_profile_msp_s_regnum
+	  || sp_regnum == tdep->m_profile_psp_s_regnum)
+	{
+	  cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
+	  cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
+	}
+      else if (sp_regnum == tdep->m_profile_msp_ns_regnum
+	       || sp_regnum == tdep->m_profile_psp_ns_regnum)
+	{
+	  cache->active_msp_regnum = tdep->m_profile_msp_ns_regnum;
+	  cache->active_psp_regnum = tdep->m_profile_psp_ns_regnum;
+	}
+    }
 
   cache->active_sp_regnum = sp_regnum;
 }
-- 
2.25.1

[PATCH 2/3] gdb/arm: Ensure that stack pointers are in sync

[Torbjörn SVENSSON]

Without this patch, sp might be secure, but msp or psp is non-secure
(this state can not happen in the hardware).

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
---
 gdb/arm-tdep.c | 86 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 58 insertions(+), 28 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 124a94dc87d..c011b2aa973 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -324,20 +324,6 @@ reconstruct_t_bit(struct gdbarch *gdbarch, CORE_ADDR lr, ULONGEST psr)
   return psr;
 }
 
-/* Initialize stack pointers, and flag the active one.  */
-
-static inline void
-arm_cache_init_sp (int regnum, CORE_ADDR* member,
-				      struct arm_prologue_cache *cache,
-				      frame_info_ptr frame)
-{
-  CORE_ADDR val = get_frame_register_unsigned (frame, regnum);
-  if (val == cache->sp)
-    cache->active_sp_regnum = regnum;
-
-  *member = val;
-}
-
 /* Initialize CACHE fields for which zero is not adequate (CACHE is
    expected to have been ZALLOC'ed before calling this function).  */
 
@@ -362,34 +348,78 @@ arm_cache_init (struct arm_prologue_cache *cache, frame_info_ptr frame)
 
   if (tdep->have_sec_ext)
     {
-      CORE_ADDR msp_val = get_frame_register_unsigned (frame, tdep->m_profile_msp_regnum);
-      CORE_ADDR psp_val = get_frame_register_unsigned (frame, tdep->m_profile_psp_regnum);
-
-      arm_cache_init_sp (tdep->m_profile_msp_s_regnum, &cache->msp_s, cache, frame);
-      arm_cache_init_sp (tdep->m_profile_psp_s_regnum, &cache->psp_s, cache, frame);
-      arm_cache_init_sp (tdep->m_profile_msp_ns_regnum, &cache->msp_ns, cache, frame);
-      arm_cache_init_sp (tdep->m_profile_psp_ns_regnum, &cache->psp_ns, cache, frame);
-
+      const CORE_ADDR msp_val
+	= get_frame_register_unsigned (frame, tdep->m_profile_msp_regnum);
+      const CORE_ADDR psp_val
+	= get_frame_register_unsigned (frame, tdep->m_profile_psp_regnum);
+
+      cache->msp_s
+	= get_frame_register_unsigned (frame, tdep->m_profile_msp_s_regnum);
+      cache->msp_ns
+	= get_frame_register_unsigned (frame, tdep->m_profile_msp_ns_regnum);
+      cache->psp_s
+	= get_frame_register_unsigned (frame, tdep->m_profile_psp_s_regnum);
+      cache->psp_ns
+	= get_frame_register_unsigned (frame, tdep->m_profile_psp_ns_regnum);
+
+      /* Identify what msp is alias for (msp_s or msp_ns).  */
       if (msp_val == cache->msp_s)
 	cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
       else if (msp_val == cache->msp_ns)
 	cache->active_msp_regnum = tdep->m_profile_msp_ns_regnum;
+      else
+	{
+	  warning (_("Invalid state, unable to determine msp alias."));
+	  cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
+	}
+
+      /* Identify what psp is alias for (psp_s or psp_ns).  */
       if (psp_val == cache->psp_s)
 	cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
       else if (psp_val == cache->psp_ns)
 	cache->active_psp_regnum = tdep->m_profile_psp_ns_regnum;
+      else
+	{
+	  warning (_("Invalid state, unable to determine psp alias."));
+	  cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
+	}
 
-      /* Use MSP_S as default stack pointer.  */
-      if (cache->active_sp_regnum == ARM_SP_REGNUM)
-	  cache->active_sp_regnum = tdep->m_profile_msp_s_regnum;
+      /* Identify what sp is alias for (msp_s, msp_ns, psp_s or psp_ns).  */
+      if (msp_val == cache->sp)
+	cache->active_sp_regnum = cache->active_msp_regnum;
+      else if (psp_val == cache->sp)
+	cache->active_sp_regnum = cache->active_psp_regnum;
+      else
+	{
+	  warning (_("Invalid state, unable to determine sp alias."));
+	  cache->active_sp_regnum = cache->active_msp_regnum;
+	}
     }
   else if (tdep->is_m)
     {
-      arm_cache_init_sp (tdep->m_profile_msp_regnum, &cache->msp_s, cache, frame);
-      arm_cache_init_sp (tdep->m_profile_psp_regnum, &cache->psp_s, cache, frame);
+      cache->msp_s
+	= get_frame_register_unsigned (frame, tdep->m_profile_msp_s_regnum);
+      cache->psp_s
+	= get_frame_register_unsigned (frame, tdep->m_profile_psp_s_regnum);
+
+      /* Identify what sp is alias for (msp or psp).  */
+      if (cache->msp_s == cache->sp)
+	cache->active_sp_regnum = tdep->m_profile_msp_regnum;
+      else if (cache->psp_s == cache->sp)
+	cache->active_sp_regnum = tdep->m_profile_psp_regnum;
+      else
+	{
+	  warning (_("Invalid state, unable to determine sp alias."));
+	  cache->active_sp_regnum = tdep->m_profile_msp_regnum;
+	}
     }
   else
-    arm_cache_init_sp (ARM_SP_REGNUM, &cache->msp_s, cache, frame);
+    {
+      cache->msp_s
+	= get_frame_register_unsigned (frame, ARM_SP_REGNUM);
+
+      cache->active_sp_regnum = ARM_SP_REGNUM;
+    }
 }
 
 /* Return the requested stack pointer value (in REGNUM), taking into
-- 
2.25.1

[PATCH 3/3] gdb/arm: PR 29738 Cache value for stack pointers for dwarf2 frames

[Torbjörn SVENSSON]

Without this patch, the number of calls to arm_dwarf2_prev_register
would grow in a too rapid way when the number of frames increase.

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
---
 gdb/arm-tdep.c | 141 +++++++++++++++++++++++--------------------------
 1 file changed, 66 insertions(+), 75 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index c011b2aa973..a6fb660bcbc 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -3992,78 +3992,6 @@ arm_dwarf2_prev_register (frame_info_ptr this_frame, void **this_cache,
       cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
       return frame_unwind_got_constant (this_frame, regnum, cpsr);
     }
-  else if (arm_is_alternative_sp_register (tdep, regnum))
-    {
-      /* Handle the alternative SP registers on Cortex-M.  */
-      bool override_with_sp_value = false;
-      CORE_ADDR val;
-
-      if (tdep->have_sec_ext)
-	{
-	  CORE_ADDR sp
-	    = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
-	  CORE_ADDR msp_s
-	    = get_frame_register_unsigned (this_frame,
-					   tdep->m_profile_msp_s_regnum);
-	  CORE_ADDR msp_ns
-	    = get_frame_register_unsigned (this_frame,
-					   tdep->m_profile_msp_ns_regnum);
-	  CORE_ADDR psp_s
-	    = get_frame_register_unsigned (this_frame,
-					   tdep->m_profile_psp_s_regnum);
-	  CORE_ADDR psp_ns
-	    = get_frame_register_unsigned (this_frame,
-					   tdep->m_profile_psp_ns_regnum);
-
-	  bool is_msp = (regnum == tdep->m_profile_msp_regnum)
-	    && (msp_s == sp || msp_ns == sp);
-	  bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
-	    && (msp_s == sp);
-	  bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
-	    && (msp_ns == sp);
-	  bool is_psp = (regnum == tdep->m_profile_psp_regnum)
-	    && (psp_s == sp || psp_ns == sp);
-	  bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
-	    && (psp_s == sp);
-	  bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
-	    && (psp_ns == sp);
-
-	  override_with_sp_value = is_msp || is_msp_s || is_msp_ns
-	    || is_psp || is_psp_s || is_psp_ns;
-
-	}
-      else if (tdep->is_m)
-	{
-	  CORE_ADDR sp
-	    = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
-	  CORE_ADDR msp
-	    = get_frame_register_unsigned (this_frame,
-					   tdep->m_profile_msp_regnum);
-	  CORE_ADDR psp
-	    = get_frame_register_unsigned (this_frame,
-					   tdep->m_profile_psp_regnum);
-
-	  bool is_msp = (regnum == tdep->m_profile_msp_regnum) && (sp == msp);
-	  bool is_psp = (regnum == tdep->m_profile_psp_regnum) && (sp == psp);
-
-	  override_with_sp_value = is_msp || is_psp;
-	}
-
-      if (override_with_sp_value)
-	{
-	  /* Use value of SP from previous frame.  */
-	  frame_info_ptr prev_frame = get_prev_frame (this_frame);
-	  if (prev_frame)
-	    val = get_frame_register_unsigned (prev_frame, ARM_SP_REGNUM);
-	  else
-	    val = get_frame_base (this_frame);
-	}
-      else
-	/* Use value for the register from previous frame.  */
-	val = get_frame_register_unsigned (this_frame, regnum);
-
-      return frame_unwind_got_constant (this_frame, regnum, val);
-    }
 
   internal_error (_("Unexpected register %d"), regnum);
 }
@@ -5202,9 +5130,72 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum,
     reg->how = DWARF2_FRAME_REG_CFA;
   else if (arm_is_alternative_sp_register (tdep, regnum))
     {
-      /* Handle the alternative SP registers on Cortex-M.  */
-      reg->how = DWARF2_FRAME_REG_FN;
-      reg->loc.fn = arm_dwarf2_prev_register;
+      /* Identify what stack pointers that are synced with sp.  */
+      bool override_with_sp_value = false;
+
+      if (tdep->have_sec_ext)
+	{
+	  CORE_ADDR sp
+	    = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
+
+	  CORE_ADDR msp_s
+	    = get_frame_register_unsigned (this_frame,
+					   tdep->m_profile_msp_s_regnum);
+	  CORE_ADDR msp_ns
+	    = get_frame_register_unsigned (this_frame,
+					   tdep->m_profile_msp_ns_regnum);
+	  CORE_ADDR psp_s
+	    = get_frame_register_unsigned (this_frame,
+					   tdep->m_profile_psp_s_regnum);
+	  CORE_ADDR psp_ns
+	    = get_frame_register_unsigned (this_frame,
+					   tdep->m_profile_psp_ns_regnum);
+
+	  bool is_msp = (regnum == tdep->m_profile_msp_regnum)
+	    && (msp_s == sp || msp_ns == sp);
+	  bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
+	    && (msp_s == sp);
+	  bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
+	    && (msp_ns == sp);
+	  bool is_psp = (regnum == tdep->m_profile_psp_regnum)
+	    && (psp_s == sp || psp_ns == sp);
+	  bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
+	    && (psp_s == sp);
+	  bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
+	    && (psp_ns == sp);
+
+	  override_with_sp_value = is_msp || is_msp_s || is_msp_ns
+	    || is_psp || is_psp_s || is_psp_ns;
+
+	}
+      else if (tdep->is_m)
+	{
+	  CORE_ADDR sp
+	    = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
+
+	  CORE_ADDR msp
+	    = get_frame_register_unsigned (this_frame,
+					   tdep->m_profile_msp_regnum);
+	  CORE_ADDR psp
+	    = get_frame_register_unsigned (this_frame,
+					   tdep->m_profile_psp_regnum);
+
+	  bool is_msp = (regnum == tdep->m_profile_msp_regnum) && (sp == msp);
+	  bool is_psp = (regnum == tdep->m_profile_psp_regnum) && (sp == psp);
+
+	  override_with_sp_value = is_msp || is_psp;
+	}
+
+      if (override_with_sp_value)
+	{
+	  /* Use the CFA value for this stack pointer register.  */
+	  reg->how = DWARF2_FRAME_REG_CFA;
+	}
+      else
+	{
+	  /* This frame does not have any update for this stack pointer.  */
+	  reg->how = DWARF2_FRAME_REG_SAME_VALUE;
+	}
     }
 }
 
-- 
2.25.1

Re: [PATCH 2/3] gdb/arm: Ensure that stack pointers are in sync

[Tomas Vanek]

Torbjorn,

thanks for addressing the issue so fast!
With two fixes commented inline the patch series resolves [Bug tdep/29738]

Tomas

On 04/11/2022 15:44, Torbjörn SVENSSON wrote:
> Without this patch, sp might be secure, but msp or psp is non-secure
> (this state can not happen in the hardware).
>
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> ---
>   gdb/arm-tdep.c | 86 ++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 58 insertions(+), 28 deletions(-)
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 124a94dc87d..c011b2aa973 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -324,20 +324,6 @@ reconstruct_t_bit(struct gdbarch *gdbarch, CORE_ADDR lr, ULONGEST psr)
>     return psr;
>   }
>   
> -/* Initialize stack pointers, and flag the active one.  */
> -
> -static inline void
> -arm_cache_init_sp (int regnum, CORE_ADDR* member,
> -				      struct arm_prologue_cache *cache,
> -				      frame_info_ptr frame)
> -{
> -  CORE_ADDR val = get_frame_register_unsigned (frame, regnum);
> -  if (val == cache->sp)
> -    cache->active_sp_regnum = regnum;
> -
> -  *member = val;
> -}
> -
>   /* Initialize CACHE fields for which zero is not adequate (CACHE is
>      expected to have been ZALLOC'ed before calling this function).  */
>   
> @@ -362,34 +348,78 @@ arm_cache_init (struct arm_prologue_cache *cache, frame_info_ptr frame)
>   
>     if (tdep->have_sec_ext)
>       {
> -      CORE_ADDR msp_val = get_frame_register_unsigned (frame, tdep->m_profile_msp_regnum);
> -      CORE_ADDR psp_val = get_frame_register_unsigned (frame, tdep->m_profile_psp_regnum);
> -
> -      arm_cache_init_sp (tdep->m_profile_msp_s_regnum, &cache->msp_s, cache, frame);
> -      arm_cache_init_sp (tdep->m_profile_psp_s_regnum, &cache->psp_s, cache, frame);
> -      arm_cache_init_sp (tdep->m_profile_msp_ns_regnum, &cache->msp_ns, cache, frame);
> -      arm_cache_init_sp (tdep->m_profile_psp_ns_regnum, &cache->psp_ns, cache, frame);
> -
> +      const CORE_ADDR msp_val
> +	= get_frame_register_unsigned (frame, tdep->m_profile_msp_regnum);
> +      const CORE_ADDR psp_val
> +	= get_frame_register_unsigned (frame, tdep->m_profile_psp_regnum);
> +
> +      cache->msp_s
> +	= get_frame_register_unsigned (frame, tdep->m_profile_msp_s_regnum);
> +      cache->msp_ns
> +	= get_frame_register_unsigned (frame, tdep->m_profile_msp_ns_regnum);
> +      cache->psp_s
> +	= get_frame_register_unsigned (frame, tdep->m_profile_psp_s_regnum);
> +      cache->psp_ns
> +	= get_frame_register_unsigned (frame, tdep->m_profile_psp_ns_regnum);
> +
> +      /* Identify what msp is alias for (msp_s or msp_ns).  */
>         if (msp_val == cache->msp_s)
>   	cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
>         else if (msp_val == cache->msp_ns)
>   	cache->active_msp_regnum = tdep->m_profile_msp_ns_regnum;
> +      else
> +	{
> +	  warning (_("Invalid state, unable to determine msp alias."));
> +	  cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
> +	}
> +
> +      /* Identify what psp is alias for (psp_s or psp_ns).  */
>         if (psp_val == cache->psp_s)
>   	cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
>         else if (psp_val == cache->psp_ns)
>   	cache->active_psp_regnum = tdep->m_profile_psp_ns_regnum;
> +      else
> +	{
> +	  warning (_("Invalid state, unable to determine psp alias."));
> +	  cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
> +	}
>   
> -      /* Use MSP_S as default stack pointer.  */
> -      if (cache->active_sp_regnum == ARM_SP_REGNUM)
> -	  cache->active_sp_regnum = tdep->m_profile_msp_s_regnum;
> +      /* Identify what sp is alias for (msp_s, msp_ns, psp_s or psp_ns).  */
> +      if (msp_val == cache->sp)
> +	cache->active_sp_regnum = cache->active_msp_regnum;
> +      else if (psp_val == cache->sp)
> +	cache->active_sp_regnum = cache->active_psp_regnum;
> +      else
> +	{
> +	  warning (_("Invalid state, unable to determine sp alias."));
> +	  cache->active_sp_regnum = cache->active_msp_regnum;
> +	}
>       }
>     else if (tdep->is_m)
>       {
> -      arm_cache_init_sp (tdep->m_profile_msp_regnum, &cache->msp_s, cache, frame);
> -      arm_cache_init_sp (tdep->m_profile_psp_regnum, &cache->psp_s, cache, frame);
> +      cache->msp_s
> +	= get_frame_register_unsigned (frame, tdep->m_profile_msp_s_regnum);

Should use tdep->m_profile_msp_regnum,
tdep->m_profile_msp_s_regnum is not initialized on M-profile without sec ext
> +      cache->psp_s
> +	= get_frame_register_unsigned (frame, tdep->m_profile_psp_s_regnum);

And here similarly tdep->m_profile_psp_regnum
> +
> +      /* Identify what sp is alias for (msp or psp).  */
> +      if (cache->msp_s == cache->sp)
> +	cache->active_sp_regnum = tdep->m_profile_msp_regnum;
> +      else if (cache->psp_s == cache->sp)
> +	cache->active_sp_regnum = tdep->m_profile_psp_regnum;
> +      else
> +	{
> +	  warning (_("Invalid state, unable to determine sp alias."));
> +	  cache->active_sp_regnum = tdep->m_profile_msp_regnum;
> +	}
>       }
>     else
> -    arm_cache_init_sp (ARM_SP_REGNUM, &cache->msp_s, cache, frame);
> +    {
> +      cache->msp_s
> +	= get_frame_register_unsigned (frame, ARM_SP_REGNUM);
> +
> +      cache->active_sp_regnum = ARM_SP_REGNUM;
> +    }
>   }
>   
>   /* Return the requested stack pointer value (in REGNUM), taking into

Re: [PATCH 2/3] gdb/arm: Ensure that stack pointers are in sync

[Torbjorn SVENSSON]

Hi Tomas,

On 2022-11-04 18:31, Tomas Vanek wrote:
> Torbjorn,
> 
> thanks for addressing the issue so fast!
> With two fixes commented inline the patch series resolves [Bug tdep/29738]

Thanks for reviewing and testing the patch series!

> 
> Tomas
> 
> On 04/11/2022 15:44, Torbjörn SVENSSON wrote:
>> Without this patch, sp might be secure, but msp or psp is non-secure
>> (this state can not happen in the hardware).
>>
>> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
>> ---
>>   gdb/arm-tdep.c | 86 ++++++++++++++++++++++++++++++++++----------------
>>   1 file changed, 58 insertions(+), 28 deletions(-)
>>
>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>> index 124a94dc87d..c011b2aa973 100644
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -324,20 +324,6 @@ reconstruct_t_bit(struct gdbarch *gdbarch, 
>> CORE_ADDR lr, ULONGEST psr)
>>     return psr;
>>   }
>> -/* Initialize stack pointers, and flag the active one.  */
>> -
>> -static inline void
>> -arm_cache_init_sp (int regnum, CORE_ADDR* member,
>> -                      struct arm_prologue_cache *cache,
>> -                      frame_info_ptr frame)
>> -{
>> -  CORE_ADDR val = get_frame_register_unsigned (frame, regnum);
>> -  if (val == cache->sp)
>> -    cache->active_sp_regnum = regnum;
>> -
>> -  *member = val;
>> -}
>> -
>>   /* Initialize CACHE fields for which zero is not adequate (CACHE is
>>      expected to have been ZALLOC'ed before calling this function).  */
>> @@ -362,34 +348,78 @@ arm_cache_init (struct arm_prologue_cache 
>> *cache, frame_info_ptr frame)
>>     if (tdep->have_sec_ext)
>>       {
>> -      CORE_ADDR msp_val = get_frame_register_unsigned (frame, 
>> tdep->m_profile_msp_regnum);
>> -      CORE_ADDR psp_val = get_frame_register_unsigned (frame, 
>> tdep->m_profile_psp_regnum);
>> -
>> -      arm_cache_init_sp (tdep->m_profile_msp_s_regnum, &cache->msp_s, 
>> cache, frame);
>> -      arm_cache_init_sp (tdep->m_profile_psp_s_regnum, &cache->psp_s, 
>> cache, frame);
>> -      arm_cache_init_sp (tdep->m_profile_msp_ns_regnum, 
>> &cache->msp_ns, cache, frame);
>> -      arm_cache_init_sp (tdep->m_profile_psp_ns_regnum, 
>> &cache->psp_ns, cache, frame);
>> -
>> +      const CORE_ADDR msp_val
>> +    = get_frame_register_unsigned (frame, tdep->m_profile_msp_regnum);
>> +      const CORE_ADDR psp_val
>> +    = get_frame_register_unsigned (frame, tdep->m_profile_psp_regnum);
>> +
>> +      cache->msp_s
>> +    = get_frame_register_unsigned (frame, tdep->m_profile_msp_s_regnum);
>> +      cache->msp_ns
>> +    = get_frame_register_unsigned (frame, 
>> tdep->m_profile_msp_ns_regnum);
>> +      cache->psp_s
>> +    = get_frame_register_unsigned (frame, tdep->m_profile_psp_s_regnum);
>> +      cache->psp_ns
>> +    = get_frame_register_unsigned (frame, 
>> tdep->m_profile_psp_ns_regnum);
>> +
>> +      /* Identify what msp is alias for (msp_s or msp_ns).  */
>>         if (msp_val == cache->msp_s)
>>       cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
>>         else if (msp_val == cache->msp_ns)
>>       cache->active_msp_regnum = tdep->m_profile_msp_ns_regnum;
>> +      else
>> +    {
>> +      warning (_("Invalid state, unable to determine msp alias."));
>> +      cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
>> +    }
>> +
>> +      /* Identify what psp is alias for (psp_s or psp_ns).  */
>>         if (psp_val == cache->psp_s)
>>       cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
>>         else if (psp_val == cache->psp_ns)
>>       cache->active_psp_regnum = tdep->m_profile_psp_ns_regnum;
>> +      else
>> +    {
>> +      warning (_("Invalid state, unable to determine psp alias."));
>> +      cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
>> +    }
>> -      /* Use MSP_S as default stack pointer.  */
>> -      if (cache->active_sp_regnum == ARM_SP_REGNUM)
>> -      cache->active_sp_regnum = tdep->m_profile_msp_s_regnum;
>> +      /* Identify what sp is alias for (msp_s, msp_ns, psp_s or 
>> psp_ns).  */
>> +      if (msp_val == cache->sp)
>> +    cache->active_sp_regnum = cache->active_msp_regnum;
>> +      else if (psp_val == cache->sp)
>> +    cache->active_sp_regnum = cache->active_psp_regnum;
>> +      else
>> +    {
>> +      warning (_("Invalid state, unable to determine sp alias."));
>> +      cache->active_sp_regnum = cache->active_msp_regnum;
>> +    }
>>       }
>>     else if (tdep->is_m)
>>       {
>> -      arm_cache_init_sp (tdep->m_profile_msp_regnum, &cache->msp_s, 
>> cache, frame);
>> -      arm_cache_init_sp (tdep->m_profile_psp_regnum, &cache->psp_s, 
>> cache, frame);
>> +      cache->msp_s
>> +    = get_frame_register_unsigned (frame, tdep->m_profile_msp_s_regnum);
> 
> Should use tdep->m_profile_msp_regnum,
> tdep->m_profile_msp_s_regnum is not initialized on M-profile without sec 
> ext
>> +      cache->psp_s
>> +    = get_frame_register_unsigned (frame, tdep->m_profile_psp_s_regnum);
> 
> And here similarly tdep->m_profile_psp_regnum

Opps, obviously, you are correct. I only had a Cortex-M33 board 
available to test, so thanks for spotting this part.

Kind regards,
Torbjörn

>> +
>> +      /* Identify what sp is alias for (msp or psp).  */
>> +      if (cache->msp_s == cache->sp)
>> +    cache->active_sp_regnum = tdep->m_profile_msp_regnum;
>> +      else if (cache->psp_s == cache->sp)
>> +    cache->active_sp_regnum = tdep->m_profile_psp_regnum;
>> +      else
>> +    {
>> +      warning (_("Invalid state, unable to determine sp alias."));
>> +      cache->active_sp_regnum = tdep->m_profile_msp_regnum;
>> +    }
>>       }
>>     else
>> -    arm_cache_init_sp (ARM_SP_REGNUM, &cache->msp_s, cache, frame);
>> +    {
>> +      cache->msp_s
>> +    = get_frame_register_unsigned (frame, ARM_SP_REGNUM);
>> +
>> +      cache->active_sp_regnum = ARM_SP_REGNUM;
>> +    }
>>   }
>>   /* Return the requested stack pointer value (in REGNUM), taking into
> 

Re: [PATCH 3/3] gdb/arm: PR 29738 Cache value for stack pointers for dwarf2 frames

[Luis Machado]

Hi Torbjörn,

On 11/4/22 14:44, Torbjörn SVENSSON wrote:
> Without this patch, the number of calls to arm_dwarf2_prev_register
> would grow in a too rapid way when the number of frames increase.
> 
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> ---
>   gdb/arm-tdep.c | 141 +++++++++++++++++++++++--------------------------
>   1 file changed, 66 insertions(+), 75 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index c011b2aa973..a6fb660bcbc 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -3992,78 +3992,6 @@ arm_dwarf2_prev_register (frame_info_ptr this_frame, void **this_cache,
>         cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
>         return frame_unwind_got_constant (this_frame, regnum, cpsr);
>       }
> -  else if (arm_is_alternative_sp_register (tdep, regnum))
> -    {
> -      /* Handle the alternative SP registers on Cortex-M.  */
> -      bool override_with_sp_value = false;
> -      CORE_ADDR val;
> -
> -      if (tdep->have_sec_ext)
> -	{
> -	  CORE_ADDR sp
> -	    = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
> -	  CORE_ADDR msp_s
> -	    = get_frame_register_unsigned (this_frame,
> -					   tdep->m_profile_msp_s_regnum);
> -	  CORE_ADDR msp_ns
> -	    = get_frame_register_unsigned (this_frame,
> -					   tdep->m_profile_msp_ns_regnum);
> -	  CORE_ADDR psp_s
> -	    = get_frame_register_unsigned (this_frame,
> -					   tdep->m_profile_psp_s_regnum);
> -	  CORE_ADDR psp_ns
> -	    = get_frame_register_unsigned (this_frame,
> -					   tdep->m_profile_psp_ns_regnum);
> -
> -	  bool is_msp = (regnum == tdep->m_profile_msp_regnum)
> -	    && (msp_s == sp || msp_ns == sp);
> -	  bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
> -	    && (msp_s == sp);
> -	  bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
> -	    && (msp_ns == sp);
> -	  bool is_psp = (regnum == tdep->m_profile_psp_regnum)
> -	    && (psp_s == sp || psp_ns == sp);
> -	  bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
> -	    && (psp_s == sp);
> -	  bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
> -	    && (psp_ns == sp);
> -
> -	  override_with_sp_value = is_msp || is_msp_s || is_msp_ns
> -	    || is_psp || is_psp_s || is_psp_ns;
> -
> -	}
> -      else if (tdep->is_m)
> -	{
> -	  CORE_ADDR sp
> -	    = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
> -	  CORE_ADDR msp
> -	    = get_frame_register_unsigned (this_frame,
> -					   tdep->m_profile_msp_regnum);
> -	  CORE_ADDR psp
> -	    = get_frame_register_unsigned (this_frame,
> -					   tdep->m_profile_psp_regnum);
> -
> -	  bool is_msp = (regnum == tdep->m_profile_msp_regnum) && (sp == msp);
> -	  bool is_psp = (regnum == tdep->m_profile_psp_regnum) && (sp == psp);
> -
> -	  override_with_sp_value = is_msp || is_psp;
> -	}
> -
> -      if (override_with_sp_value)
> -	{
> -	  /* Use value of SP from previous frame.  */
> -	  frame_info_ptr prev_frame = get_prev_frame (this_frame);
> -	  if (prev_frame)
> -	    val = get_frame_register_unsigned (prev_frame, ARM_SP_REGNUM);
> -	  else
> -	    val = get_frame_base (this_frame);
> -	}
> -      else
> -	/* Use value for the register from previous frame.  */
> -	val = get_frame_register_unsigned (this_frame, regnum);
> -
> -      return frame_unwind_got_constant (this_frame, regnum, val);
> -    }
>   
>     internal_error (_("Unexpected register %d"), regnum);
>   }
> @@ -5202,9 +5130,72 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum,
>       reg->how = DWARF2_FRAME_REG_CFA;
>     else if (arm_is_alternative_sp_register (tdep, regnum))
>       {
> -      /* Handle the alternative SP registers on Cortex-M.  */
> -      reg->how = DWARF2_FRAME_REG_FN;
> -      reg->loc.fn = arm_dwarf2_prev_register;
> +      /* Identify what stack pointers that are synced with sp.  */
> +      bool override_with_sp_value = false;
> +
> +      if (tdep->have_sec_ext)
> +	{
> +	  CORE_ADDR sp
> +	    = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
> +
> +	  CORE_ADDR msp_s
> +	    = get_frame_register_unsigned (this_frame,
> +					   tdep->m_profile_msp_s_regnum);
> +	  CORE_ADDR msp_ns
> +	    = get_frame_register_unsigned (this_frame,
> +					   tdep->m_profile_msp_ns_regnum);
> +	  CORE_ADDR psp_s
> +	    = get_frame_register_unsigned (this_frame,
> +					   tdep->m_profile_psp_s_regnum);
> +	  CORE_ADDR psp_ns
> +	    = get_frame_register_unsigned (this_frame,
> +					   tdep->m_profile_psp_ns_regnum);
> +
> +	  bool is_msp = (regnum == tdep->m_profile_msp_regnum)
> +	    && (msp_s == sp || msp_ns == sp);
> +	  bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
> +	    && (msp_s == sp);
> +	  bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
> +	    && (msp_ns == sp);
> +	  bool is_psp = (regnum == tdep->m_profile_psp_regnum)
> +	    && (psp_s == sp || psp_ns == sp);
> +	  bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
> +	    && (psp_s == sp);
> +	  bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
> +	    && (psp_ns == sp);
> +
> +	  override_with_sp_value = is_msp || is_msp_s || is_msp_ns
> +	    || is_psp || is_psp_s || is_psp_ns;
> +
> +	}
> +      else if (tdep->is_m)
> +	{
> +	  CORE_ADDR sp
> +	    = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
> +
> +	  CORE_ADDR msp
> +	    = get_frame_register_unsigned (this_frame,
> +					   tdep->m_profile_msp_regnum);
> +	  CORE_ADDR psp
> +	    = get_frame_register_unsigned (this_frame,
> +					   tdep->m_profile_psp_regnum);
> +
> +	  bool is_msp = (regnum == tdep->m_profile_msp_regnum) && (sp == msp);
> +	  bool is_psp = (regnum == tdep->m_profile_psp_regnum) && (sp == psp);
> +
> +	  override_with_sp_value = is_msp || is_psp;
> +	}
> +
> +      if (override_with_sp_value)
> +	{
> +	  /* Use the CFA value for this stack pointer register.  */
> +	  reg->how = DWARF2_FRAME_REG_CFA;
> +	}
> +      else
> +	{
> +	  /* This frame does not have any update for this stack pointer.  */
> +	  reg->how = DWARF2_FRAME_REG_SAME_VALUE;
> +	}
>       }
>   }
>   

Although it does make sense to cache the values to address this issue, I don't think this approach is appropriate.

The init_reg () hook is supposed to initialize the rules. Even if unwinding works from this particular function, I don't think it is safe to do so.

If we attempt to point at a register whose rule hasn't been initialize yet, we may run into issues. I'm not sure if that is technically possible, I haven't exercised this in practice.

I think we need to take a step back and re-assess how we're determining the active sp register. From what I can tell, it is just a value check. Since we need to check at least 6 values, it gets fairly expensive the more frames you have in the debugging session.

One idea that comes to mind is to introduce a pseudo-register that stores the active sp register number (say active_sp). It may not need a name if it is only going to be used for unwinding purposes.

In the sentinel frame, we can easily determine this value by doing quick checks with all the alternate sp registers. The other frames would query this pseudo-register to know what value they must use for the alternate sp registers.

I think this will reduce the number of alternate sp value fetches considerably, and thus will address this performance issue.

For the alternate sp registers, we can still use the arm_dwarf2_prev_register function, but now we'll fetch the active_sp pseudo_register to help us decide what value to return.

On other unwinders of different frame types (prologue, exception etc), we can adjust the value of active_sp accordingly if the active stack pointer changed.

Does that make sense?

Re: [PATCH 3/3] gdb/arm: PR 29738 Cache value for stack pointers for dwarf2 frames

[Tomas Vanek]

On 11/11/2022 10:15, Luis Machado wrote:
> Hi Torbjörn,
>
> On 11/4/22 14:44, Torbjörn SVENSSON wrote:
>> Without this patch, the number of calls to arm_dwarf2_prev_register
>> would grow in a too rapid way when the number of frames increase.
>>
>> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
>> ---
>>   gdb/arm-tdep.c | 141 +++++++++++++++++++++++--------------------------
>>   1 file changed, 66 insertions(+), 75 deletions(-)
>>
>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>> index c011b2aa973..a6fb660bcbc 100644
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -3992,78 +3992,6 @@ arm_dwarf2_prev_register (frame_info_ptr 
>> this_frame, void **this_cache,
>>         cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
>>         return frame_unwind_got_constant (this_frame, regnum, cpsr);
>>       }
>> -  else if (arm_is_alternative_sp_register (tdep, regnum))
>> -    {
>> -      /* Handle the alternative SP registers on Cortex-M.  */
>> -      bool override_with_sp_value = false;
>> -      CORE_ADDR val;
>> -
>> -      if (tdep->have_sec_ext)
>> -    {
>> -      CORE_ADDR sp
>> -        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>> -      CORE_ADDR msp_s
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_msp_s_regnum);
>> -      CORE_ADDR msp_ns
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_msp_ns_regnum);
>> -      CORE_ADDR psp_s
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_psp_s_regnum);
>> -      CORE_ADDR psp_ns
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_psp_ns_regnum);
>> -
>> -      bool is_msp = (regnum == tdep->m_profile_msp_regnum)
>> -        && (msp_s == sp || msp_ns == sp);
>> -      bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
>> -        && (msp_s == sp);
>> -      bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
>> -        && (msp_ns == sp);
>> -      bool is_psp = (regnum == tdep->m_profile_psp_regnum)
>> -        && (psp_s == sp || psp_ns == sp);
>> -      bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
>> -        && (psp_s == sp);
>> -      bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
>> -        && (psp_ns == sp);
>> -
>> -      override_with_sp_value = is_msp || is_msp_s || is_msp_ns
>> -        || is_psp || is_psp_s || is_psp_ns;
>> -
>> -    }
>> -      else if (tdep->is_m)
>> -    {
>> -      CORE_ADDR sp
>> -        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>> -      CORE_ADDR msp
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_msp_regnum);
>> -      CORE_ADDR psp
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_psp_regnum);
>> -
>> -      bool is_msp = (regnum == tdep->m_profile_msp_regnum) && (sp == 
>> msp);
>> -      bool is_psp = (regnum == tdep->m_profile_psp_regnum) && (sp == 
>> psp);
>> -
>> -      override_with_sp_value = is_msp || is_psp;
>> -    }
>> -
>> -      if (override_with_sp_value)
>> -    {
>> -      /* Use value of SP from previous frame.  */
>> -      frame_info_ptr prev_frame = get_prev_frame (this_frame);
>> -      if (prev_frame)
>> -        val = get_frame_register_unsigned (prev_frame, ARM_SP_REGNUM);
>> -      else
>> -        val = get_frame_base (this_frame);
>> -    }
>> -      else
>> -    /* Use value for the register from previous frame.  */
>> -    val = get_frame_register_unsigned (this_frame, regnum);
>> -
>> -      return frame_unwind_got_constant (this_frame, regnum, val);
>> -    }
>>       internal_error (_("Unexpected register %d"), regnum);
>>   }
>> @@ -5202,9 +5130,72 @@ arm_dwarf2_frame_init_reg (struct gdbarch 
>> *gdbarch, int regnum,
>>       reg->how = DWARF2_FRAME_REG_CFA;
>>     else if (arm_is_alternative_sp_register (tdep, regnum))
>>       {
>> -      /* Handle the alternative SP registers on Cortex-M.  */
>> -      reg->how = DWARF2_FRAME_REG_FN;
>> -      reg->loc.fn = arm_dwarf2_prev_register;
>> +      /* Identify what stack pointers that are synced with sp. */
>> +      bool override_with_sp_value = false;
>> +
>> +      if (tdep->have_sec_ext)
>> +    {
>> +      CORE_ADDR sp
>> +        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>> +
>> +      CORE_ADDR msp_s
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_msp_s_regnum);
>> +      CORE_ADDR msp_ns
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_msp_ns_regnum);
>> +      CORE_ADDR psp_s
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_psp_s_regnum);
>> +      CORE_ADDR psp_ns
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_psp_ns_regnum);
>> +
>> +      bool is_msp = (regnum == tdep->m_profile_msp_regnum)
>> +        && (msp_s == sp || msp_ns == sp);
>> +      bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
>> +        && (msp_s == sp);
>> +      bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
>> +        && (msp_ns == sp);
>> +      bool is_psp = (regnum == tdep->m_profile_psp_regnum)
>> +        && (psp_s == sp || psp_ns == sp);
>> +      bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
>> +        && (psp_s == sp);
>> +      bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
>> +        && (psp_ns == sp);
>> +
>> +      override_with_sp_value = is_msp || is_msp_s || is_msp_ns
>> +        || is_psp || is_psp_s || is_psp_ns;
>> +
>> +    }
>> +      else if (tdep->is_m)
>> +    {
>> +      CORE_ADDR sp
>> +        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>> +
>> +      CORE_ADDR msp
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_msp_regnum);
>> +      CORE_ADDR psp
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_psp_regnum);
>> +
>> +      bool is_msp = (regnum == tdep->m_profile_msp_regnum) && (sp == 
>> msp);
>> +      bool is_psp = (regnum == tdep->m_profile_psp_regnum) && (sp == 
>> psp);
>> +
>> +      override_with_sp_value = is_msp || is_psp;
>> +    }
>> +
>> +      if (override_with_sp_value)
>> +    {
>> +      /* Use the CFA value for this stack pointer register.  */
>> +      reg->how = DWARF2_FRAME_REG_CFA;
>> +    }
>> +      else
>> +    {
>> +      /* This frame does not have any update for this stack 
>> pointer.  */
>> +      reg->how = DWARF2_FRAME_REG_SAME_VALUE;
>> +    }
>>       }
>>   }
>
> Although it does make sense to cache the values to address this issue, 
> I don't think this approach is appropriate.
>
> The init_reg () hook is supposed to initialize the rules. Even if 
> unwinding works from this particular function, I don't think it is 
> safe to do so.
>
> If we attempt to point at a register whose rule hasn't been initialize 
> yet, we may run into issues. I'm not sure if that is technically 
> possible, I haven't exercised this in practice.
>
> I think we need to take a step back and re-assess how we're 
> determining the active sp register. From what I can tell, it is just a 
> value check. Since we need to check at least 6 values, it gets fairly 
> expensive the more frames you have in the debugging session.
>
> One idea that comes to mind is to introduce a pseudo-register that 
> stores the active sp register number (say active_sp). It may not need 
> a name if it is only going to be used for unwinding purposes.
>
> In the sentinel frame, we can easily determine this value by doing 
> quick checks with all the alternate sp registers. The other frames 
> would query this pseudo-register to know what value they must use for 
> the alternate sp registers.
>
> I think this will reduce the number of alternate sp value fetches 
> considerably, and thus will address this performance issue.
>
> For the alternate sp registers, we can still use the 
> arm_dwarf2_prev_register function, but now we'll fetch the active_sp 
> pseudo_register to help us decide what value to return.
>
> On other unwinders of different frame types (prologue, exception etc), 
> we can adjust the value of active_sp accordingly if the active stack 
> pointer changed.
>
> Does that make sense?

It makes a lot of sense to me.

Although I doubt if adding 'active_sp' is enough to prevent the rapid 
growth of the number of fetches.
To resolve prev SP value the unwinder fetches 'active_sp' and the 
specific xsp_x pointed by active_sp.
The prev frame needs active_sp again to find if the specific xsp_x is 
updated to CFA and if not it fetches xsp_x.
And if we consider the evaluation of active_sp may be as expensive as 
looking up secure gateway veneer name
and checking if prefixed by '__acle_se_' as used in
https://sourceware.org/pipermail/gdb-patches/2022-November/193433.html
I would incline to use better caching in the dwarf2 frames regardless of 
using 'active_sp'.

My RFC patch series is sub-optimal from the performance point of view
and having handy 'active_sp' pseudo register available could make the 
code simpler (and hopefully faster).
The bad news for me is that the change series would need almost a 
complete rework (for the second time).

Please keep me informed.

Tomas

Re: [PATCH 3/3] gdb/arm: PR 29738 Cache value for stack pointers for dwarf2 frames

[Torbjorn SVENSSON]

Hi Luis,

Thanks for taking another look at this.
My comments below will be slightly overlaping what Tomas already 
replied, but I think it makes more sense to reply to this email than to 
Tomas reply as my comments is on your mail.

On 2022-11-11 10:15, Luis Machado wrote:
> Hi Torbjörn,
> 
> On 11/4/22 14:44, Torbjörn SVENSSON wrote:
>> Without this patch, the number of calls to arm_dwarf2_prev_register
>> would grow in a too rapid way when the number of frames increase.
>>
>> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
>> ---
>>   gdb/arm-tdep.c | 141 +++++++++++++++++++++++--------------------------
>>   1 file changed, 66 insertions(+), 75 deletions(-)
>>
>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>> index c011b2aa973..a6fb660bcbc 100644
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -3992,78 +3992,6 @@ arm_dwarf2_prev_register (frame_info_ptr 
>> this_frame, void **this_cache,
>>         cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
>>         return frame_unwind_got_constant (this_frame, regnum, cpsr);
>>       }
>> -  else if (arm_is_alternative_sp_register (tdep, regnum))
>> -    {
>> -      /* Handle the alternative SP registers on Cortex-M.  */
>> -      bool override_with_sp_value = false;
>> -      CORE_ADDR val;
>> -
>> -      if (tdep->have_sec_ext)
>> -    {
>> -      CORE_ADDR sp
>> -        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>> -      CORE_ADDR msp_s
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_msp_s_regnum);
>> -      CORE_ADDR msp_ns
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_msp_ns_regnum);
>> -      CORE_ADDR psp_s
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_psp_s_regnum);
>> -      CORE_ADDR psp_ns
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_psp_ns_regnum);
>> -
>> -      bool is_msp = (regnum == tdep->m_profile_msp_regnum)
>> -        && (msp_s == sp || msp_ns == sp);
>> -      bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
>> -        && (msp_s == sp);
>> -      bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
>> -        && (msp_ns == sp);
>> -      bool is_psp = (regnum == tdep->m_profile_psp_regnum)
>> -        && (psp_s == sp || psp_ns == sp);
>> -      bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
>> -        && (psp_s == sp);
>> -      bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
>> -        && (psp_ns == sp);
>> -
>> -      override_with_sp_value = is_msp || is_msp_s || is_msp_ns
>> -        || is_psp || is_psp_s || is_psp_ns;
>> -
>> -    }
>> -      else if (tdep->is_m)
>> -    {
>> -      CORE_ADDR sp
>> -        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>> -      CORE_ADDR msp
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_msp_regnum);
>> -      CORE_ADDR psp
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_psp_regnum);
>> -
>> -      bool is_msp = (regnum == tdep->m_profile_msp_regnum) && (sp == 
>> msp);
>> -      bool is_psp = (regnum == tdep->m_profile_psp_regnum) && (sp == 
>> psp);
>> -
>> -      override_with_sp_value = is_msp || is_psp;
>> -    }
>> -
>> -      if (override_with_sp_value)
>> -    {
>> -      /* Use value of SP from previous frame.  */
>> -      frame_info_ptr prev_frame = get_prev_frame (this_frame);
>> -      if (prev_frame)
>> -        val = get_frame_register_unsigned (prev_frame, ARM_SP_REGNUM);
>> -      else
>> -        val = get_frame_base (this_frame);
>> -    }
>> -      else
>> -    /* Use value for the register from previous frame.  */
>> -    val = get_frame_register_unsigned (this_frame, regnum);
>> -
>> -      return frame_unwind_got_constant (this_frame, regnum, val);
>> -    }
>>     internal_error (_("Unexpected register %d"), regnum);
>>   }
>> @@ -5202,9 +5130,72 @@ arm_dwarf2_frame_init_reg (struct gdbarch 
>> *gdbarch, int regnum,
>>       reg->how = DWARF2_FRAME_REG_CFA;
>>     else if (arm_is_alternative_sp_register (tdep, regnum))
>>       {
>> -      /* Handle the alternative SP registers on Cortex-M.  */
>> -      reg->how = DWARF2_FRAME_REG_FN;
>> -      reg->loc.fn = arm_dwarf2_prev_register;
>> +      /* Identify what stack pointers that are synced with sp.  */
>> +      bool override_with_sp_value = false;
>> +
>> +      if (tdep->have_sec_ext)
>> +    {
>> +      CORE_ADDR sp
>> +        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>> +
>> +      CORE_ADDR msp_s
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_msp_s_regnum);
>> +      CORE_ADDR msp_ns
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_msp_ns_regnum);
>> +      CORE_ADDR psp_s
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_psp_s_regnum);
>> +      CORE_ADDR psp_ns
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_psp_ns_regnum);
>> +
>> +      bool is_msp = (regnum == tdep->m_profile_msp_regnum)
>> +        && (msp_s == sp || msp_ns == sp);
>> +      bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
>> +        && (msp_s == sp);
>> +      bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
>> +        && (msp_ns == sp);
>> +      bool is_psp = (regnum == tdep->m_profile_psp_regnum)
>> +        && (psp_s == sp || psp_ns == sp);
>> +      bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
>> +        && (psp_s == sp);
>> +      bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
>> +        && (psp_ns == sp);
>> +
>> +      override_with_sp_value = is_msp || is_msp_s || is_msp_ns
>> +        || is_psp || is_psp_s || is_psp_ns;
>> +
>> +    }
>> +      else if (tdep->is_m)
>> +    {
>> +      CORE_ADDR sp
>> +        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>> +
>> +      CORE_ADDR msp
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_msp_regnum);
>> +      CORE_ADDR psp
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_psp_regnum);
>> +
>> +      bool is_msp = (regnum == tdep->m_profile_msp_regnum) && (sp == 
>> msp);
>> +      bool is_psp = (regnum == tdep->m_profile_psp_regnum) && (sp == 
>> psp);
>> +
>> +      override_with_sp_value = is_msp || is_psp;
>> +    }
>> +
>> +      if (override_with_sp_value)
>> +    {
>> +      /* Use the CFA value for this stack pointer register.  */
>> +      reg->how = DWARF2_FRAME_REG_CFA;
>> +    }
>> +      else
>> +    {
>> +      /* This frame does not have any update for this stack pointer.  */
>> +      reg->how = DWARF2_FRAME_REG_SAME_VALUE;
>> +    }
>>       }
>>   }
> 
> Although it does make sense to cache the values to address this issue, I 
> don't think this approach is appropriate.
> 
> The init_reg () hook is supposed to initialize the rules. Even if 
> unwinding works from this particular function, I don't think it is safe 
> to do so. >
> If we attempt to point at a register whose rule hasn't been initialize 
> yet, we may run into issues. I'm not sure if that is technically 
> possible, I haven't exercised this in practice.

As I said on IRC; where are currently only fetching values for the inner 
frame and not the current frame, so I think the risk that some rule has 
not yet been initialized is slim to none. :)
It can still be a bad way to go though...

> I think we need to take a step back and re-assess how we're determining 
> the active sp register. From what I can tell, it is just a value check. 
> Since we need to check at least 6 values, it gets fairly expensive the 
> more frames you have in the debugging session.

So, to lets give everyone some background here to have some context.
For Arm Cortex-M33 with security extension (TrustZone), there are 4 
stack pointer registers. msp_s, mps_ns, psp_s and psp_ns. In addition to 
these, there is the 'sp' register that is always in sync with one of the 
previous mentioned 4 registers. Internally in GDB, there is only one 
stack pointer and its register is sp. When sp is updated, for some 
reason, in GDB during the unwind, then the matching of the 4 other 
registers should have the same modification as they are in sync in the core.
To further complicate things, there are 2 other registers, msp and psp, 
that may (or may not) exist on the target. If they exist, then they 
should be kept in sync with their _ns or _s registers depending on 
securty context. When the core is in secure context, msp, psp and sp are 
always alias for one of msp_s or psp_s. When the core is in non-secure 
context, msp, psp and sp are always alias for one of msp_ns or psp_ns.
So, all in all; On Cortex-M33 with TrustZone, there are 7 stack pointers 
where 3 of them are "aliases" for one of the basic 4 registers.
For all Cortex-M without TrustZone (as far as I know anyway), the stack 
pointers are reduced to sp, msp and psp. In this case, sp is to be 
considered as an "alias" for either msp or psp.

Regardless of the Cortex-M type or existence of TrustZone, the core 
always works on what is called 'sp', so it's a matter of updating what 
this 'sp' is alias for when doing interrupts or similar.

This description might be simplified or not 100% acurate from a core 
perspective, but it's my view of how the hardware works. If anyone has a 
better picture to draw, please help me get a more clear view of how to 
look at all these registers from a core perspective. :)

> 
> One idea that comes to mind is to introduce a pseudo-register that 
> stores the active sp register number (say active_sp). It may not need a 
> name if it is only going to be used for unwinding purposes.

Yes, it would reduce the number of registers to fetch, but it would 
still be more than what I think is acceptable.

For a non-TrustZone core, I suppose it would be:
* sp: just the ARM_SP_REGNUM register.
* msp: active_sp and then either the register that active_sp points to 
or the msp register number.
* psp: active_sp and then either the register that active_sp points to 
or the psp register number.

For a TrustZone core, it would be worse:
* sp: just the ARM_SP_REGNUM register.
* msp_s: active_sp and then the register that active_sp points to or the 
msp_s register number.
* msp_ns: active_sp and then the register that active_sp points to or 
the msp_ns register number.
* psp_s: active_sp and then the register that active_sp points to or the 
psp_s register number.
* psp_ns: active_sp and then the register that active_sp points to or 
the psp_ns register number.
* msp: active_sp and then the register that active_sp points to or the 
msp register number.
* psp: active_sp and then the register that active_sp points to or the 
psp register number.


So, in best case, it's 1 register lookup, but in worse case it's 2 
register lookups, and this is per fetch of a stack pointer.
To fetch the values once for a frame, it would be 5 register fetches for 
non-TrustZone and 13 register fetches for TrustZone. This is still lower 
than the original implementation, but is it acceptable with that large 
number of fetches of register content per frame?

> 
> In the sentinel frame, we can easily determine this value by doing quick 
> checks with all the alternate sp registers. The other frames would query 
> this pseudo-register to know what value they must use for the alternate 
> sp registers.
> 
> I think this will reduce the number of alternate sp value fetches 
> considerably, and thus will address this performance issue.

Yes, it would lower the number, but it would still be rather high as 
fetching a single register from the inner frame would result in 1 or 2 
fetches from the more inner frame and so on, so it's stil growing fast.

> 
> For the alternate sp registers, we can still use the 
> arm_dwarf2_prev_register function, but now we'll fetch the active_sp 
> pseudo_register to help us decide what value to return.
> 
> On other unwinders of different frame types (prologue, exception etc), 
> we can adjust the value of active_sp accordingly if the active stack 
> pointer changed.

Sure, it would mean that the identification of what sp is alias for is 
already available by the inner frame, but I don't see that much 
difference compare to the code in this patch, except that this porposed 
algorithm would be fetching a lot more registers from the inner frame 
each time a stack pointer is to be returned.
Performance wise, I'd say that your solution is somehwere in between 
what is currently merged in master and this patch.

> 
> Does that make sense?

I think there are only 2 viable solutions for dwarf2 frames:
* eager cache (that's what I did in this patch)
* lazy cache (this is what Tomas suggested in his RFC series)
Both have their pros and cons.

I don't know if I'm still stuck in the loop of what I've provided or if 
there is a better alternative that I can't see.

Kind regards,
Torbjörn

Re: [PATCH 3/3] gdb/arm: PR 29738 Cache value for stack pointers for dwarf2 frames

[Luis Machado]

On 11/12/22 19:54, Torbjorn SVENSSON wrote:
> Hi Luis,
> 
> Thanks for taking another look at this.
> My comments below will be slightly overlaping what Tomas already replied, but I think it makes more sense to reply to this email than to Tomas reply as my comments is on your mail.
> 
> On 2022-11-11 10:15, Luis Machado wrote:
>> Hi Torbjörn,
>>
>> On 11/4/22 14:44, Torbjörn SVENSSON wrote:
>>> Without this patch, the number of calls to arm_dwarf2_prev_register
>>> would grow in a too rapid way when the number of frames increase.
>>>
>>> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
>>> ---
>>>   gdb/arm-tdep.c | 141 +++++++++++++++++++++++--------------------------
>>>   1 file changed, 66 insertions(+), 75 deletions(-)
>>>
>>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>>> index c011b2aa973..a6fb660bcbc 100644
>>> --- a/gdb/arm-tdep.c
>>> +++ b/gdb/arm-tdep.c
>>> @@ -3992,78 +3992,6 @@ arm_dwarf2_prev_register (frame_info_ptr this_frame, void **this_cache,
>>>         cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
>>>         return frame_unwind_got_constant (this_frame, regnum, cpsr);
>>>       }
>>> -  else if (arm_is_alternative_sp_register (tdep, regnum))
>>> -    {
>>> -      /* Handle the alternative SP registers on Cortex-M.  */
>>> -      bool override_with_sp_value = false;
>>> -      CORE_ADDR val;
>>> -
>>> -      if (tdep->have_sec_ext)
>>> -    {
>>> -      CORE_ADDR sp
>>> -        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>>> -      CORE_ADDR msp_s
>>> -        = get_frame_register_unsigned (this_frame,
>>> -                       tdep->m_profile_msp_s_regnum);
>>> -      CORE_ADDR msp_ns
>>> -        = get_frame_register_unsigned (this_frame,
>>> -                       tdep->m_profile_msp_ns_regnum);
>>> -      CORE_ADDR psp_s
>>> -        = get_frame_register_unsigned (this_frame,
>>> -                       tdep->m_profile_psp_s_regnum);
>>> -      CORE_ADDR psp_ns
>>> -        = get_frame_register_unsigned (this_frame,
>>> -                       tdep->m_profile_psp_ns_regnum);
>>> -
>>> -      bool is_msp = (regnum == tdep->m_profile_msp_regnum)
>>> -        && (msp_s == sp || msp_ns == sp);
>>> -      bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
>>> -        && (msp_s == sp);
>>> -      bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
>>> -        && (msp_ns == sp);
>>> -      bool is_psp = (regnum == tdep->m_profile_psp_regnum)
>>> -        && (psp_s == sp || psp_ns == sp);
>>> -      bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
>>> -        && (psp_s == sp);
>>> -      bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
>>> -        && (psp_ns == sp);
>>> -
>>> -      override_with_sp_value = is_msp || is_msp_s || is_msp_ns
>>> -        || is_psp || is_psp_s || is_psp_ns;
>>> -
>>> -    }
>>> -      else if (tdep->is_m)
>>> -    {
>>> -      CORE_ADDR sp
>>> -        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>>> -      CORE_ADDR msp
>>> -        = get_frame_register_unsigned (this_frame,
>>> -                       tdep->m_profile_msp_regnum);
>>> -      CORE_ADDR psp
>>> -        = get_frame_register_unsigned (this_frame,
>>> -                       tdep->m_profile_psp_regnum);
>>> -
>>> -      bool is_msp = (regnum == tdep->m_profile_msp_regnum) && (sp == msp);
>>> -      bool is_psp = (regnum == tdep->m_profile_psp_regnum) && (sp == psp);
>>> -
>>> -      override_with_sp_value = is_msp || is_psp;
>>> -    }
>>> -
>>> -      if (override_with_sp_value)
>>> -    {
>>> -      /* Use value of SP from previous frame.  */
>>> -      frame_info_ptr prev_frame = get_prev_frame (this_frame);
>>> -      if (prev_frame)
>>> -        val = get_frame_register_unsigned (prev_frame, ARM_SP_REGNUM);
>>> -      else
>>> -        val = get_frame_base (this_frame);
>>> -    }
>>> -      else
>>> -    /* Use value for the register from previous frame.  */
>>> -    val = get_frame_register_unsigned (this_frame, regnum);
>>> -
>>> -      return frame_unwind_got_constant (this_frame, regnum, val);
>>> -    }
>>>     internal_error (_("Unexpected register %d"), regnum);
>>>   }
>>> @@ -5202,9 +5130,72 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum,
>>>       reg->how = DWARF2_FRAME_REG_CFA;
>>>     else if (arm_is_alternative_sp_register (tdep, regnum))
>>>       {
>>> -      /* Handle the alternative SP registers on Cortex-M.  */
>>> -      reg->how = DWARF2_FRAME_REG_FN;
>>> -      reg->loc.fn = arm_dwarf2_prev_register;
>>> +      /* Identify what stack pointers that are synced with sp.  */
>>> +      bool override_with_sp_value = false;
>>> +
>>> +      if (tdep->have_sec_ext)
>>> +    {
>>> +      CORE_ADDR sp
>>> +        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>>> +
>>> +      CORE_ADDR msp_s
>>> +        = get_frame_register_unsigned (this_frame,
>>> +                       tdep->m_profile_msp_s_regnum);
>>> +      CORE_ADDR msp_ns
>>> +        = get_frame_register_unsigned (this_frame,
>>> +                       tdep->m_profile_msp_ns_regnum);
>>> +      CORE_ADDR psp_s
>>> +        = get_frame_register_unsigned (this_frame,
>>> +                       tdep->m_profile_psp_s_regnum);
>>> +      CORE_ADDR psp_ns
>>> +        = get_frame_register_unsigned (this_frame,
>>> +                       tdep->m_profile_psp_ns_regnum);
>>> +
>>> +      bool is_msp = (regnum == tdep->m_profile_msp_regnum)
>>> +        && (msp_s == sp || msp_ns == sp);
>>> +      bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
>>> +        && (msp_s == sp);
>>> +      bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
>>> +        && (msp_ns == sp);
>>> +      bool is_psp = (regnum == tdep->m_profile_psp_regnum)
>>> +        && (psp_s == sp || psp_ns == sp);
>>> +      bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
>>> +        && (psp_s == sp);
>>> +      bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
>>> +        && (psp_ns == sp);
>>> +
>>> +      override_with_sp_value = is_msp || is_msp_s || is_msp_ns
>>> +        || is_psp || is_psp_s || is_psp_ns;
>>> +
>>> +    }
>>> +      else if (tdep->is_m)
>>> +    {
>>> +      CORE_ADDR sp
>>> +        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>>> +
>>> +      CORE_ADDR msp
>>> +        = get_frame_register_unsigned (this_frame,
>>> +                       tdep->m_profile_msp_regnum);
>>> +      CORE_ADDR psp
>>> +        = get_frame_register_unsigned (this_frame,
>>> +                       tdep->m_profile_psp_regnum);
>>> +
>>> +      bool is_msp = (regnum == tdep->m_profile_msp_regnum) && (sp == msp);
>>> +      bool is_psp = (regnum == tdep->m_profile_psp_regnum) && (sp == psp);
>>> +
>>> +      override_with_sp_value = is_msp || is_psp;
>>> +    }
>>> +
>>> +      if (override_with_sp_value)
>>> +    {
>>> +      /* Use the CFA value for this stack pointer register.  */
>>> +      reg->how = DWARF2_FRAME_REG_CFA;
>>> +    }
>>> +      else
>>> +    {
>>> +      /* This frame does not have any update for this stack pointer.  */
>>> +      reg->how = DWARF2_FRAME_REG_SAME_VALUE;
>>> +    }
>>>       }
>>>   }
>>
>> Although it does make sense to cache the values to address this issue, I don't think this approach is appropriate.
>>
>> The init_reg () hook is supposed to initialize the rules. Even if unwinding works from this particular function, I don't think it is safe to do so. >
>> If we attempt to point at a register whose rule hasn't been initialize yet, we may run into issues. I'm not sure if that is technically possible, I haven't exercised this in practice.
> 
> As I said on IRC; where are currently only fetching values for the inner frame and not the current frame, so I think the risk that some rule has not yet been initialized is slim to none. :)
> It can still be a bad way to go though...
> 
>> I think we need to take a step back and re-assess how we're determining the active sp register. From what I can tell, it is just a value check. Since we need to check at least 6 values, it gets fairly expensive the more frames you have in the debugging session.
> 
> So, to lets give everyone some background here to have some context.
> For Arm Cortex-M33 with security extension (TrustZone), there are 4 stack pointer registers. msp_s, mps_ns, psp_s and psp_ns. In addition to these, there is the 'sp' register that is always in sync with one of the previous mentioned 4 registers. Internally in GDB, there is only one stack pointer and its register is sp. When sp is updated, for some reason, in GDB during the unwind, then the matching of the 4 other registers should have the same modification as they are in sync in the core.
> To further complicate things, there are 2 other registers, msp and psp, that may (or may not) exist on the target. If they exist, then they should be kept in sync with their _ns or _s registers depending on securty context. When the core is in secure context, msp, psp and sp are always alias for one of msp_s or psp_s. When the core is in non-secure context, msp, psp and sp are always alias for one of msp_ns or psp_ns.
> So, all in all; On Cortex-M33 with TrustZone, there are 7 stack pointers where 3 of them are "aliases" for one of the basic 4 registers.
> For all Cortex-M without TrustZone (as far as I know anyway), the stack pointers are reduced to sp, msp and psp. In this case, sp is to be considered as an "alias" for either msp or psp.
> 
> Regardless of the Cortex-M type or existence of TrustZone, the core always works on what is called 'sp', so it's a matter of updating what this 'sp' is alias for when doing interrupts or similar.
> 
> This description might be simplified or not 100% acurate from a core perspective, but it's my view of how the hardware works. If anyone has a better picture to draw, please help me get a more clear view of how to look at all these registers from a core perspective. :)

I think we're dealing with a couple issues here, and we need to handle them separately for now. One of the issues if being able to backtrace through secure/non-secure frame transitions. The other issue is trying to keep the current sp and the sp aliases in sync.

Considering only sp, the actual current stack pointer (no matter what it is an alias to), can we actually handle secure/non-secure frame transitions without dealing/having to expose all the other aliases? Given sp is always the current stack pointer, I suppose we
could restore it to a valid state and produce a valid backtrace across secure/non-secure frame transitions, maybe with the help of saved state from the exception frame. Is that possible?

If we can solve that problem, I think the matter of keeping msp/psp/msp_s/msp_ns/psp_s/psp_ns in sync with sp is secondary and mostly cosmetic. The important functionality is being able to produce a useful backtrace to the user, right?

> 
>>
>> One idea that comes to mind is to introduce a pseudo-register that stores the active sp register number (say active_sp). It may not need a name if it is only going to be used for unwinding purposes.
> 
> Yes, it would reduce the number of registers to fetch, but it would still be more than what I think is acceptable.
> 

The dwarf register cache doesn't cache values like we do for, say, exception or prologue caches. The cache is basically dwarf rules that we execute to find the value we want. It is the same case for the custom FN hook, which return a value that doesn't get cached either.

One particular value is cached though, and that is the CFA. That's why it is resolved quickly. Given the other stack pointers don't have any DWARF definitions, we have to calculate those by hand unfortunately.

If we really want to cache something, we may want to consider extending the dwarf frame cache to include multiple CFA's, and a rule that we can use to index that particular CFA entry. I think this is the cleanest solution for this problem.

> For a non-TrustZone core, I suppose it would be:
> * sp: just the ARM_SP_REGNUM register.
> * msp: active_sp and then either the register that active_sp points to or the msp register number.
> * psp: active_sp and then either the register that active_sp points to or the psp register number.
> 
> For a TrustZone core, it would be worse:
> * sp: just the ARM_SP_REGNUM register.
> * msp_s: active_sp and then the register that active_sp points to or the msp_s register number.
> * msp_ns: active_sp and then the register that active_sp points to or the msp_ns register number.
> * psp_s: active_sp and then the register that active_sp points to or the psp_s register number.
> * psp_ns: active_sp and then the register that active_sp points to or the psp_ns register number.
> * msp: active_sp and then the register that active_sp points to or the msp register number.
> * psp: active_sp and then the register that active_sp points to or the psp register number.
> 
> 
> So, in best case, it's 1 register lookup, but in worse case it's 2 register lookups, and this is per fetch of a stack pointer.
> To fetch the values once for a frame, it would be 5 register fetches for non-TrustZone and 13 register fetches for TrustZone. This is still lower than the original implementation, but is it acceptable with that large number of fetches of register content per frame?

I don't think so. And that's why I'm suggesting leaving the synchronization of alternate sp's aside for now, as that's the step that is costing us the most.

> 
>>
>> In the sentinel frame, we can easily determine this value by doing quick checks with all the alternate sp registers. The other frames would query this pseudo-register to know what value they must use for the alternate sp registers.
>>
>> I think this will reduce the number of alternate sp value fetches considerably, and thus will address this performance issue.
> 
> Yes, it would lower the number, but it would still be rather high as fetching a single register from the inner frame would result in 1 or 2 fetches from the more inner frame and so on, so it's stil growing fast.
> 
>>
>> For the alternate sp registers, we can still use the arm_dwarf2_prev_register function, but now we'll fetch the active_sp pseudo_register to help us decide what value to return.
>>
>> On other unwinders of different frame types (prologue, exception etc), we can adjust the value of active_sp accordingly if the active stack pointer changed.
> 
> Sure, it would mean that the identification of what sp is alias for is already available by the inner frame, but I don't see that much difference compare to the code in this patch, except that this porposed algorithm would be fetching a lot more registers from the inner frame each time a stack pointer is to be returned.
> Performance wise, I'd say that your solution is somehwere in between what is currently merged in master and this patch.

Sure, but the problem is that the approach of patch 3/3 has no precedent and seems to be abusing a function with a distinct purpose.

We have had cases in the past where unwinding from some random function works, but it doesn't mean it is correct. It has complexities and we don't want to risk running into infinite loops and other issues.

> 
>>
>> Does that make sense?
> 
> I think there are only 2 viable solutions for dwarf2 frames:
> * eager cache (that's what I did in this patch)

I think we should explore the option of having 2 cfa's. Or, at the least, make things work with only sp (which is cached already) and think about a solution to the synchronization problem at a later stage (post GDB 13).

> * lazy cache (this is what Tomas suggested in his RFC series)

That would be an alternative approach. I can see how it would be useful to be able to dynamically change the HOW rule (as real dwarf unwinding does, based on PC). But that would be a bigger change in my opinion.

Right now it seems the only complication we're dealing with is a second CFA, for which DWARF and gdb have not support for.

It might also be the case that we need to take a couple steps back and discuss proper DWARF id's for msp/psp/msp_s/msp_ns/psp_s/psp_ns. Then the compiler would generate proper dwarf for these stack pointers.

> Both have their pros and cons.
> 
> I don't know if I'm still stuck in the loop of what I've provided or if there is a better alternative that I can't see.
> 
> Kind regards,
> Torbjörn