public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 1/5] gdb/arm: Fix prologue analysis to support vpush
@ 2022-01-14 16:35 Christophe Lyon
  2022-01-14 16:35 ` [PATCH 2/5] gdb/arm: Define MSP and PSP registers for M-Profile Christophe Lyon
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Christophe Lyon @ 2022-01-14 16:35 UTC (permalink / raw)
  To: gdb-patches; +Cc: torjborn.svensson, Christophe Lyon

While working on adding support for Non-secure/Secure modes unwinding,
I noticed that the prologue analysis lacked support for vpush, which
is used for instance in the CMSE stub routine.

This patch updates thumb_analyze_prologue accordingly.
---
 gdb/arm-tdep.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 7495434484e..14ec0bc8f9e 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -896,6 +896,27 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
 		regs[bits (insn, 0, 3)] = addr;
 	    }
 
+	  else if ((insn & 0xff20) == 0xed20    /* vstmdb Rn{!},
+						   { registers } */
+		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    {
+	      pv_t addr = regs[bits (insn, 0, 3)];
+	      int number = bits (inst2, 0, 7) >> 1;
+
+	      if (stack.store_would_trash (addr))
+		break;
+
+	      /* Calculate offsets of saved registers.  */
+	      for (; number > 0; number--)
+		{
+		  addr = pv_add_constant (addr, -8);
+		  stack.store (addr, 8, pv_register (ARM_D0_REGNUM + number, 0));
+		}
+
+	      if (insn & 0x0020)
+		regs[bits (insn, 0, 3)] = addr;
+	    }
+
 	  else if ((insn & 0xff50) == 0xe940	/* strd Rt, Rt2,
 						   [Rn, #+/-imm]{!} */
 		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/5] gdb/arm: Define MSP and PSP registers for M-Profile
  2022-01-14 16:35 [PATCH 1/5] gdb/arm: Fix prologue analysis to support vpush Christophe Lyon
@ 2022-01-14 16:35 ` Christophe Lyon
  2022-01-17 12:50   ` Luis Machado
  2022-01-14 16:35 ` [PATCH 3/5] gdb/arm: Introduce arm_cache_init Christophe Lyon
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Christophe Lyon @ 2022-01-14 16:35 UTC (permalink / raw)
  To: gdb-patches; +Cc: torjborn.svensson, Christophe Lyon

This patch removes the hardcoded access to PSP in
arm_m_exception_cache() and relies on the definition with the XML
descriptions.
---
 gdb/arch/arm.h                              |  6 ++-
 gdb/arm-tdep.c                              | 45 ++++++++-------------
 gdb/features/arm/arm-m-profile-with-fpa.c   |  2 +
 gdb/features/arm/arm-m-profile-with-fpa.xml |  2 +
 gdb/features/arm/arm-m-profile.c            |  2 +
 gdb/features/arm/arm-m-profile.xml          |  2 +
 6 files changed, 28 insertions(+), 31 deletions(-)

diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
index eabcb434f1f..638780011fc 100644
--- a/gdb/arch/arm.h
+++ b/gdb/arch/arm.h
@@ -49,6 +49,8 @@ enum gdb_regnum {
   ARM_D0_REGNUM,		/* VFP double-precision registers.  */
   ARM_D31_REGNUM = ARM_D0_REGNUM + 31,
   ARM_FPSCR_REGNUM,
+  ARM_MSP_REGNUM,		/* Cortex-M Main Stack Pointer.  */
+  ARM_PSP_REGNUM,		/* Cortex-M Process Stack Pointer.  */
 
   /* Other useful registers.  */
   ARM_FP_REGNUM = 11,		/* Frame register in ARM code, if used.  */
@@ -65,8 +67,8 @@ enum arm_register_counts {
   ARM_NUM_ARG_REGS = 4,
   /* Number of floating point argument registers.  */
   ARM_NUM_FP_ARG_REGS = 4,
-  /* Number of registers (old, defined as ARM_FPSCR_REGNUM + 1.  */
-  ARM_NUM_REGS = ARM_FPSCR_REGNUM + 1
+  /* Number of registers (old, defined as ARM_PSP_REGNUM + 1.  */
+  ARM_NUM_REGS = ARM_PSP_REGNUM + 1
 };
 
 /* Enum describing the different kinds of breakpoints.  */
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 14ec0bc8f9e..b6a1deafad5 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -3012,7 +3012,6 @@ arm_m_exception_cache (struct frame_info *this_frame)
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   struct arm_prologue_cache *cache;
   CORE_ADDR lr;
-  CORE_ADDR sp;
   CORE_ADDR unwound_sp;
   LONGEST xpsr;
   uint32_t exc_return;
@@ -3028,7 +3027,6 @@ arm_m_exception_cache (struct frame_info *this_frame)
      to the exception and if FPU is used (causing extended stack frame).  */
 
   lr = get_frame_register_unsigned (this_frame, ARM_LR_REGNUM);
-  sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
 
   /* Check EXC_RETURN indicator bits.  */
   exc_return = (((lr >> 28) & 0xf) == 0xf);
@@ -3037,37 +3035,13 @@ arm_m_exception_cache (struct frame_info *this_frame)
   process_stack_used = ((lr & (1 << 2)) != 0);
   if (exc_return && process_stack_used)
     {
-      /* Thread (process) stack used.
-	 Potentially this could be other register defined by target, but PSP
-	 can be considered a standard name for the "Process Stack Pointer".
-	 To be fully aware of system registers like MSP and PSP, these could
-	 be added to a separate XML arm-m-system-profile that is valid for
-	 ARMv6-M and ARMv7-M architectures. Also to be able to debug eg a
-	 corefile off-line, then these registers must be defined by GDB,
-	 and also be included in the corefile regsets.  */
-
-      int psp_regnum = user_reg_map_name_to_regnum (gdbarch, "psp", -1);
-      if (psp_regnum == -1)
-	{
-	  /* Thread (process) stack could not be fetched,
-	     give warning and exit.  */
-
-	  warning (_("no PSP thread stack unwinding supported."));
-
-	  /* Terminate any further stack unwinding by refer to self.  */
-	  cache->prev_sp = sp;
-	  return cache;
-	}
-      else
-	{
-	  /* Thread (process) stack used, use PSP as SP.  */
-	  unwound_sp = get_frame_register_unsigned (this_frame, psp_regnum);
-	}
+      /* Thread (process) stack used, use PSP as SP.  */
+      unwound_sp = get_frame_register_unsigned (this_frame, ARM_PSP_REGNUM);
     }
   else
     {
       /* Main stack used, use MSP as SP.  */
-      unwound_sp = sp;
+      unwound_sp = get_frame_register_unsigned (this_frame, ARM_MSP_REGNUM);
     }
 
   /* The hardware saves eight 32-bit words, comprising xPSR,
@@ -9296,6 +9270,19 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       if (!valid_p)
 	return NULL;
 
+      if (is_m)
+	{
+	  feature = tdesc_find_feature (tdesc,
+					"org.gnu.gdb.arm.m-system");
+	  if (feature != NULL)
+	    {
+	      valid_p &= tdesc_numbered_register (feature, tdesc_data.get (),
+						  ARM_MSP_REGNUM, "msp");
+	      valid_p &= tdesc_numbered_register (feature, tdesc_data.get (),
+						  ARM_PSP_REGNUM, "psp");
+	    }
+	}
+
       feature = tdesc_find_feature (tdesc,
 				    "org.gnu.gdb.arm.fpa");
       if (feature != NULL)
diff --git a/gdb/features/arm/arm-m-profile-with-fpa.c b/gdb/features/arm/arm-m-profile-with-fpa.c
index 2b7c78597bb..4afba856875 100644
--- a/gdb/features/arm/arm-m-profile-with-fpa.c
+++ b/gdb/features/arm/arm-m-profile-with-fpa.c
@@ -35,5 +35,7 @@ create_feature_arm_arm_m_profile_with_fpa (struct target_desc *result, long regn
   tdesc_create_reg (feature, "", regnum++, 1, NULL, 96, "arm_fpa_ext");
   tdesc_create_reg (feature, "", regnum++, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "xpsr", regnum++, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "msp", regnum++, 1, NULL, 32, "data_ptr");
+  tdesc_create_reg (feature, "psp", regnum++, 1, NULL, 32, "data_ptr");
   return regnum;
 }
diff --git a/gdb/features/arm/arm-m-profile-with-fpa.xml b/gdb/features/arm/arm-m-profile-with-fpa.xml
index fceeaea7177..ee796b549a2 100644
--- a/gdb/features/arm/arm-m-profile-with-fpa.xml
+++ b/gdb/features/arm/arm-m-profile-with-fpa.xml
@@ -36,4 +36,6 @@
   <reg name="" bitsize="32"/>
 
   <reg name="xpsr" bitsize="32" regnum="25"/>
+  <reg name="msp" bitsize="32" type="data_ptr"/>
+  <reg name="psp" bitsize="32" type="data_ptr"/>
 </feature>
diff --git a/gdb/features/arm/arm-m-profile.c b/gdb/features/arm/arm-m-profile.c
index 027f3c15c56..db6c0b10d15 100644
--- a/gdb/features/arm/arm-m-profile.c
+++ b/gdb/features/arm/arm-m-profile.c
@@ -27,5 +27,7 @@ create_feature_arm_arm_m_profile (struct target_desc *result, long regnum)
   tdesc_create_reg (feature, "pc", regnum++, 1, NULL, 32, "code_ptr");
   regnum = 25;
   tdesc_create_reg (feature, "xpsr", regnum++, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "msp", regnum++, 1, NULL, 32, "data_ptr");
+  tdesc_create_reg (feature, "psp", regnum++, 1, NULL, 32, "data_ptr");
   return regnum;
 }
diff --git a/gdb/features/arm/arm-m-profile.xml b/gdb/features/arm/arm-m-profile.xml
index d56fb246342..3253d1c5339 100644
--- a/gdb/features/arm/arm-m-profile.xml
+++ b/gdb/features/arm/arm-m-profile.xml
@@ -24,4 +24,6 @@
   <reg name="lr" bitsize="32"/>
   <reg name="pc" bitsize="32" type="code_ptr"/>
   <reg name="xpsr" bitsize="32" regnum="25"/>
+  <reg name="msp" bitsize="32" type="data_ptr"/>
+  <reg name="psp" bitsize="32" type="data_ptr"/>
 </feature>
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/5] gdb/arm: Introduce arm_cache_init
  2022-01-14 16:35 [PATCH 1/5] gdb/arm: Fix prologue analysis to support vpush Christophe Lyon
  2022-01-14 16:35 ` [PATCH 2/5] gdb/arm: Define MSP and PSP registers for M-Profile Christophe Lyon
@ 2022-01-14 16:35 ` Christophe Lyon
  2022-01-14 16:35 ` [PATCH 4/5] gdb/arm: Add support for multiple stack pointers on Cortex-M Christophe Lyon
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Christophe Lyon @ 2022-01-14 16:35 UTC (permalink / raw)
  To: gdb-patches; +Cc: torjborn.svensson, Christophe Lyon

This patch is a preparation for the rest of the series and adds two
arm_cache_init helper functions. It updates every place that updates
cache->saved_regs to call the helper instead.
---
 gdb/arm-tdep.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index b6a1deafad5..a345df959d1 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -290,6 +290,23 @@ struct arm_prologue_cache
   trad_frame_saved_reg *saved_regs;
 };
 
+static void
+arm_cache_init (struct arm_prologue_cache *cache, struct gdbarch *gdbarch)
+{
+  cache->framesize = 0;
+  cache->framereg = 0;
+  cache->saved_regs = trad_frame_alloc_saved_regs (gdbarch);
+}
+
+static void
+arm_cache_init (struct arm_prologue_cache *cache, struct frame_info *frame)
+{
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+  cache->prev_sp = get_frame_register_unsigned (frame, ARM_SP_REGNUM);
+
+  arm_cache_init (cache, gdbarch);
+}
+
 namespace {
 
 /* Abstract class to read ARM instructions from memory.  */
@@ -1922,7 +1939,7 @@ arm_make_prologue_cache (struct frame_info *this_frame)
   CORE_ADDR unwound_fp;
 
   cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
-  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+  arm_cache_init (cache, this_frame);
 
   arm_scan_prologue (this_frame, cache);
 
@@ -2388,7 +2405,7 @@ arm_exidx_fill_cache (struct frame_info *this_frame, gdb_byte *entry)
 
   struct arm_prologue_cache *cache;
   cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
-  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+  arm_cache_init (cache, this_frame);
 
   for (;;)
     {
@@ -2782,7 +2799,7 @@ arm_make_epilogue_frame_cache (struct frame_info *this_frame)
   int reg;
 
   cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
-  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+  arm_cache_init (cache, this_frame);
 
   /* Still rely on the offset calculated from prologue.  */
   arm_scan_prologue (this_frame, cache);
@@ -2943,7 +2960,7 @@ arm_make_stub_cache (struct frame_info *this_frame)
   struct arm_prologue_cache *cache;
 
   cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
-  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+  arm_cache_init (cache, this_frame);
 
   cache->prev_sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
 
@@ -3020,7 +3037,7 @@ arm_m_exception_cache (struct frame_info *this_frame)
   uint32_t secure_stack_used;
 
   cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
-  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+  arm_cache_init (cache, this_frame);
 
   /* ARMv7-M Architecture Reference "B1.5.6 Exception entry behavior"
      describes which bits in LR that define which stack was used prior
@@ -13572,7 +13589,7 @@ arm_analyze_prologue_test ()
 
       test_arm_instruction_reader mem_reader (insns);
       arm_prologue_cache cache;
-      cache.saved_regs = trad_frame_alloc_saved_regs (gdbarch);
+      arm_cache_init (&cache, gdbarch);
 
       arm_analyze_prologue (gdbarch, 0, sizeof (insns) - 1, &cache, mem_reader);
     }
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 4/5] gdb/arm: Add support for multiple stack pointers on Cortex-M
  2022-01-14 16:35 [PATCH 1/5] gdb/arm: Fix prologue analysis to support vpush Christophe Lyon
  2022-01-14 16:35 ` [PATCH 2/5] gdb/arm: Define MSP and PSP registers for M-Profile Christophe Lyon
  2022-01-14 16:35 ` [PATCH 3/5] gdb/arm: Introduce arm_cache_init Christophe Lyon
@ 2022-01-14 16:35 ` Christophe Lyon
  2022-01-14 16:35 ` [PATCH 5/5] gdb/arm: Extend arm_m_addr_is_magic to support FNC_RETURN, add unwind-ns-to-s command Christophe Lyon
  2022-01-17 12:49 ` [PATCH 1/5] gdb/arm: Fix prologue analysis to support vpush Luis Machado
  4 siblings, 0 replies; 9+ messages in thread
From: Christophe Lyon @ 2022-01-14 16:35 UTC (permalink / raw)
  To: gdb-patches; +Cc: torjborn.svensson, Christophe Lyon

V8-M architecture with Security extension features four stack pointers
to handle Secure and Non-secure modes.

This patch adds support to switch between them as needed during
unwinding, and replaces all updates of cache->prev_sp with calls to
arm_cache_set_prev_sp.

It makes sure that arm_analyze_prologue updates the right SP as
needed.
---
 gdb/arch/arm.h |   9 +-
 gdb/arm-tdep.c | 229 +++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 217 insertions(+), 21 deletions(-)

diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
index 638780011fc..ea36a0015cc 100644
--- a/gdb/arch/arm.h
+++ b/gdb/arch/arm.h
@@ -52,6 +52,11 @@ enum gdb_regnum {
   ARM_MSP_REGNUM,		/* Cortex-M Main Stack Pointer.  */
   ARM_PSP_REGNUM,		/* Cortex-M Process Stack Pointer.  */
 
+  ARM_MSP_S_REGNUM,		/* Cortex-M Secure Main Stack Pointer.  */
+  ARM_MSP_NS_REGNUM,		/* Cortex-M Non-secure Main Stack Pointer.  */
+  ARM_PSP_S_REGNUM,		/* Cortex-M Secure Process Stack Pointer.  */
+  ARM_PSP_NS_REGNUM,		/* Cortex-M Non-secure Process Stack Pointer.  */
+
   /* Other useful registers.  */
   ARM_FP_REGNUM = 11,		/* Frame register in ARM code, if used.  */
   THUMB_FP_REGNUM = 7,		/* Frame register in Thumb code, if used.  */
@@ -67,8 +72,8 @@ enum arm_register_counts {
   ARM_NUM_ARG_REGS = 4,
   /* Number of floating point argument registers.  */
   ARM_NUM_FP_ARG_REGS = 4,
-  /* Number of registers (old, defined as ARM_PSP_REGNUM + 1.  */
-  ARM_NUM_REGS = ARM_PSP_REGNUM + 1
+  /* Number of registers (old, defined as ARM_PSP_NS_REGNUM + 1.  */
+  ARM_NUM_REGS = ARM_PSP_NS_REGNUM + 1
 };
 
 /* Enum describing the different kinds of breakpoints.  */
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index a345df959d1..8115361d914 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -275,7 +275,19 @@ struct arm_prologue_cache
   /* The stack pointer at the time this frame was created; i.e. the
      caller's stack pointer when this function was called.  It is used
      to identify this frame.  */
-  CORE_ADDR prev_sp;
+  CORE_ADDR sp;
+
+  /* Additional stack pointers used by M-profile with Security extension.  */
+  CORE_ADDR msp_s;
+  CORE_ADDR msp_ns;
+  CORE_ADDR psp_s;
+  CORE_ADDR psp_ns;
+
+  /* Active stack pointer.  */
+  gdb_regnum active_sp_regnum;
+
+  bool have_sec_ext;
+  bool is_m;
 
   /* The frame base for this frame is just prev_sp - frame size.
      FRAMESIZE is the distance from the frame pointer to the
@@ -293,6 +305,10 @@ struct arm_prologue_cache
 static void
 arm_cache_init (struct arm_prologue_cache *cache, struct gdbarch *gdbarch)
 {
+  cache->have_sec_ext = false;
+  cache->is_m = false;
+  cache->active_sp_regnum = ARM_SP_REGNUM;
+
   cache->framesize = 0;
   cache->framereg = 0;
   cache->saved_regs = trad_frame_alloc_saved_regs (gdbarch);
@@ -302,9 +318,181 @@ static void
 arm_cache_init (struct arm_prologue_cache *cache, struct frame_info *frame)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
-  cache->prev_sp = get_frame_register_unsigned (frame, ARM_SP_REGNUM);
+  arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
+  gdb::optional<CORE_ADDR> sp;
+  cache->sp = get_frame_register_unsigned (frame, ARM_SP_REGNUM);
 
   arm_cache_init (cache, gdbarch);
+
+  cache->have_sec_ext = tdep->have_sec_ext;
+  cache->is_m = tdep->is_m;
+
+#define INIT_ARM_CACHE_SP(regnum, member) do {			  \
+    CORE_ADDR val = get_frame_register_unsigned (frame, (regnum)); \
+    if (val == cache->sp || (sp.has_value () && val == *sp))	  \
+      {								  \
+	cache->active_sp_regnum = (regnum);			  \
+      }								  \
+    (member) = val;						  \
+  } while (0)
+
+  if (tdep->have_sec_ext)
+    {
+      INIT_ARM_CACHE_SP (ARM_MSP_S_REGNUM, cache->msp_s);
+      INIT_ARM_CACHE_SP (ARM_MSP_NS_REGNUM, cache->msp_ns);
+      INIT_ARM_CACHE_SP (ARM_PSP_S_REGNUM, cache->psp_s);
+      INIT_ARM_CACHE_SP (ARM_PSP_NS_REGNUM, cache->psp_ns);
+      /* Use MSP_S as default stack pointer.  */
+      if (cache->active_sp_regnum == ARM_SP_REGNUM)
+	  cache->active_sp_regnum = ARM_MSP_S_REGNUM;
+    }
+  else if (tdep->is_m)
+    {
+      INIT_ARM_CACHE_SP (ARM_MSP_REGNUM, cache->msp_s);
+      INIT_ARM_CACHE_SP (ARM_PSP_REGNUM, cache->psp_s);
+    }
+  else
+    {
+      INIT_ARM_CACHE_SP (ARM_SP_REGNUM, cache->msp_s);
+    }
+  #undef INIT_ARM_CACHE_SP
+}
+
+static gdb::optional<CORE_ADDR>
+arm_cache_get_sp_register (struct arm_prologue_cache *cache, int regnum)
+{
+  if (cache->have_sec_ext)
+    {
+      switch (regnum)
+	{
+	case ARM_MSP_S_REGNUM:
+	  return cache->msp_s;
+	case ARM_MSP_NS_REGNUM:
+	  return cache->msp_ns;
+	case ARM_PSP_S_REGNUM:
+	  return cache->psp_s;
+	case ARM_PSP_NS_REGNUM:
+	  return cache->psp_ns;
+	case ARM_SP_REGNUM:
+	  return cache->sp;
+	}
+    }
+  else if (cache->is_m)
+    {
+      switch (regnum)
+	{
+	case ARM_MSP_REGNUM:
+	  return cache->msp_s;
+	case ARM_PSP_REGNUM:
+	  return cache->psp_s;
+	case ARM_SP_REGNUM:
+	  return cache->sp;
+	}
+    }
+  else
+    {
+      switch (regnum)
+	{
+	case ARM_SP_REGNUM:
+	  return cache->sp;
+	}
+    }
+  return {};
+}
+
+static CORE_ADDR
+arm_cache_get_prev_sp (struct arm_prologue_cache *cache)
+{
+  gdb::optional<CORE_ADDR> val
+    = arm_cache_get_sp_register (cache, cache->active_sp_regnum);
+  if (val.has_value ())
+      return *val;
+
+  gdb_assert_not_reached ("Invalid SP selection");
+  return 0;
+}
+
+static void
+arm_cache_set_prev_sp (struct arm_prologue_cache *cache, CORE_ADDR val)
+{
+  if (cache->have_sec_ext)
+    {
+      switch (cache->active_sp_regnum)
+	{
+	case ARM_MSP_S_REGNUM:
+	  cache->msp_s = val;
+	  return;
+	case ARM_MSP_NS_REGNUM:
+	  cache->msp_ns = val;
+	  return;
+	case ARM_PSP_S_REGNUM:
+	  cache->psp_s = val;
+	  return;
+	case ARM_PSP_NS_REGNUM:
+	  cache->psp_ns = val;
+	  return;
+	case ARM_SP_REGNUM:
+	  cache->sp = val;
+	  return;
+	}
+    }
+  else if (cache->is_m)
+    {
+      switch (cache->active_sp_regnum)
+	{
+	case ARM_MSP_REGNUM:
+	  cache->msp_s = val;
+	  return;
+	case ARM_PSP_REGNUM:
+	  cache->psp_s = val;
+	  return;
+	case ARM_SP_REGNUM:
+	  cache->sp = val;
+	  return;
+	}
+    }
+  else
+    {
+      switch (cache->active_sp_regnum)
+	{
+	case ARM_SP_REGNUM:
+	  cache->sp = val;
+	  return;
+	}
+    }
+
+  gdb_assert_not_reached ("Invalid SP selection");
+}
+
+static bool
+arm_cache_is_sp_register (struct arm_prologue_cache *cache, int regnum)
+{
+  switch (regnum)
+    {
+    case ARM_SP_REGNUM:
+    case ARM_MSP_REGNUM:
+    case ARM_MSP_S_REGNUM:
+    case ARM_MSP_NS_REGNUM:
+    case ARM_PSP_REGNUM:
+    case ARM_PSP_S_REGNUM:
+    case ARM_PSP_NS_REGNUM:
+      return true;
+    default:
+      return false;
+    }
+}
+
+static void
+arm_cache_switch_prev_sp (struct arm_prologue_cache *cache, gdb_regnum sp_regnum)
+{
+  gdb_assert (sp_regnum != ARM_SP_REGNUM);
+  if (cache->have_sec_ext)
+    gdb_assert (sp_regnum != ARM_MSP_REGNUM && sp_regnum != ARM_PSP_REGNUM);
+
+  if (!arm_cache_is_sp_register (cache, sp_regnum))
+    gdb_assert_not_reached ("Invalid SP selection");
+
+  cache->active_sp_regnum = sp_regnum;
 }
 
 namespace {
@@ -1826,6 +2014,8 @@ arm_analyze_prologue (struct gdbarch *gdbarch,
       for (regno = 0; regno < ARM_FPS_REGNUM; regno++)
 	if (stack.find_reg (gdbarch, regno, &offset))
 	  cache->saved_regs[regno].set_addr (offset);
+
+      arm_cache_set_prev_sp (cache, regs[ARM_SP_REGNUM].k);
     }
 
   arm_debug_printf ("Prologue scan stopped at %s",
@@ -1947,14 +2137,14 @@ arm_make_prologue_cache (struct frame_info *this_frame)
   if (unwound_fp == 0)
     return cache;
 
-  cache->prev_sp = unwound_fp + cache->framesize;
+  arm_cache_set_prev_sp (cache, unwound_fp + cache->framesize);
 
   /* Calculate actual addresses of saved registers using offsets
      determined by arm_scan_prologue.  */
   for (reg = 0; reg < gdbarch_num_regs (get_frame_arch (this_frame)); reg++)
     if (cache->saved_regs[reg].is_addr ())
       cache->saved_regs[reg].set_addr (cache->saved_regs[reg].addr ()
-				       + cache->prev_sp);
+				       + arm_cache_get_prev_sp (cache));
 
   return cache;
 }
@@ -1980,7 +2170,7 @@ arm_prologue_unwind_stop_reason (struct frame_info *this_frame,
     return UNWIND_OUTERMOST;
 
   /* If we've hit a wall, stop.  */
-  if (cache->prev_sp == 0)
+  if (arm_cache_get_prev_sp (cache) == 0)
     return UNWIND_OUTERMOST;
 
   return UNWIND_NO_REASON;
@@ -2010,7 +2200,7 @@ arm_prologue_this_id (struct frame_info *this_frame,
   if (!func)
     func = pc;
 
-  id = frame_id_build (cache->prev_sp, func);
+  id = frame_id_build (arm_cache_get_prev_sp (cache), func);
   *this_id = id;
 }
 
@@ -2044,7 +2234,8 @@ arm_prologue_prev_register (struct frame_info *this_frame,
      identified by the next frame's stack pointer at the time of the call.
      The value was already reconstructed into PREV_SP.  */
   if (prev_regnum == ARM_SP_REGNUM)
-    return frame_unwind_got_constant (this_frame, prev_regnum, cache->prev_sp);
+    return frame_unwind_got_constant (this_frame, prev_regnum,
+				      arm_cache_get_prev_sp (cache));
 
   /* The CPSR may have been changed by the call instruction and by the
      called function.  The only bit we can reconstruct is the T bit,
@@ -2682,7 +2873,7 @@ arm_exidx_fill_cache (struct frame_info *this_frame, gdb_byte *entry)
     = vsp - get_frame_register_unsigned (this_frame, cache->framereg);
 
   /* We already got the previous SP.  */
-  cache->prev_sp = vsp;
+  arm_cache_set_prev_sp (cache, vsp);
 
   return cache;
 }
@@ -2805,14 +2996,14 @@ arm_make_epilogue_frame_cache (struct frame_info *this_frame)
   arm_scan_prologue (this_frame, cache);
 
   /* Since we are in epilogue, the SP has been restored.  */
-  cache->prev_sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
+  arm_cache_set_prev_sp (cache, get_frame_register_unsigned (this_frame, ARM_SP_REGNUM));
 
   /* Calculate actual addresses of saved registers using offsets
      determined by arm_scan_prologue.  */
   for (reg = 0; reg < gdbarch_num_regs (get_frame_arch (this_frame)); reg++)
     if (cache->saved_regs[reg].is_addr ())
       cache->saved_regs[reg].set_addr (cache->saved_regs[reg].addr ()
-				       + cache->prev_sp);
+				       + arm_cache_get_prev_sp (cache));
 
   return cache;
 }
@@ -2840,7 +3031,7 @@ arm_epilogue_frame_this_id (struct frame_info *this_frame,
   if (func == 0)
     func = pc;
 
-  (*this_id) = frame_id_build (cache->prev_sp, pc);
+  (*this_id) = frame_id_build (arm_cache_get_prev_sp (cache), pc);
 }
 
 /* Implementation of function hook 'prev_register' in
@@ -2962,7 +3153,7 @@ arm_make_stub_cache (struct frame_info *this_frame)
   cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
   arm_cache_init (cache, this_frame);
 
-  cache->prev_sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
+  arm_cache_set_prev_sp (cache, get_frame_register_unsigned (this_frame, ARM_SP_REGNUM));
 
   return cache;
 }
@@ -2980,7 +3171,7 @@ arm_stub_this_id (struct frame_info *this_frame,
     *this_cache = arm_make_stub_cache (this_frame);
   cache = (struct arm_prologue_cache *) *this_cache;
 
-  *this_id = frame_id_build (cache->prev_sp, get_frame_pc (this_frame));
+  *this_id = frame_id_build (arm_cache_get_prev_sp (cache), get_frame_pc (this_frame));
 }
 
 static int
@@ -3100,12 +3291,12 @@ arm_m_exception_cache (struct frame_info *this_frame)
       cache->saved_regs[ARM_FPSCR_REGNUM].set_addr (unwound_sp + 0x60);
 
       /* Offset 0x64 is reserved.  */
-      cache->prev_sp = unwound_sp + 0x68;
+      arm_cache_set_prev_sp (cache, unwound_sp + 0x68);
     }
   else
     {
       /* Standard stack frame type used.  */
-      cache->prev_sp = unwound_sp + 0x20;
+      arm_cache_set_prev_sp (cache, unwound_sp + 0x20);
     }
 
   /* Check EXC_RETURN bit S if Secure or Non-secure stack used.  */
@@ -3127,7 +3318,7 @@ arm_m_exception_cache (struct frame_info *this_frame)
      previous context's stack pointer.  */
   if (safe_read_memory_integer (unwound_sp + 28, 4, byte_order, &xpsr)
       && (xpsr & (1 << 9)) != 0)
-    cache->prev_sp += 4;
+    arm_cache_set_prev_sp (cache, arm_cache_get_prev_sp (cache) + 4);
 
   return cache;
 }
@@ -3147,7 +3338,7 @@ arm_m_exception_this_id (struct frame_info *this_frame,
   cache = (struct arm_prologue_cache *) *this_cache;
 
   /* Our frame ID for a stub frame is the current SP and LR.  */
-  *this_id = frame_id_build (cache->prev_sp,
+  *this_id = frame_id_build (arm_cache_get_prev_sp (cache),
 			     get_frame_pc (this_frame));
 }
 
@@ -3168,7 +3359,7 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
   /* The value was already reconstructed into PREV_SP.  */
   if (prev_regnum == ARM_SP_REGNUM)
     return frame_unwind_got_constant (this_frame, prev_regnum,
-				      cache->prev_sp);
+				      arm_cache_get_prev_sp (cache));
 
   return trad_frame_get_prev_register (this_frame, cache->saved_regs,
 				       prev_regnum);
@@ -3213,7 +3404,7 @@ arm_normal_frame_base (struct frame_info *this_frame, void **this_cache)
     *this_cache = arm_make_prologue_cache (this_frame);
   cache = (struct arm_prologue_cache *) *this_cache;
 
-  return cache->prev_sp - cache->framesize;
+  return arm_cache_get_prev_sp (cache) - cache->framesize;
 }
 
 struct frame_base arm_normal_base = {
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 5/5] gdb/arm: Extend arm_m_addr_is_magic to support FNC_RETURN, add unwind-ns-to-s command
  2022-01-14 16:35 [PATCH 1/5] gdb/arm: Fix prologue analysis to support vpush Christophe Lyon
                   ` (2 preceding siblings ...)
  2022-01-14 16:35 ` [PATCH 4/5] gdb/arm: Add support for multiple stack pointers on Cortex-M Christophe Lyon
@ 2022-01-14 16:35 ` Christophe Lyon
  2022-01-17 12:49 ` [PATCH 1/5] gdb/arm: Fix prologue analysis to support vpush Luis Machado
  4 siblings, 0 replies; 9+ messages in thread
From: Christophe Lyon @ 2022-01-14 16:35 UTC (permalink / raw)
  To: gdb-patches; +Cc: torjborn.svensson, Christophe Lyon

This patch makes use of the support for several stack pointers
introduced by the previous patch to switch between them as needed
during unwinding.

It introduces a new 'unwind-ns-to-s' arm command to enable/disable
mode switching during unwinding. It is enabled by default.

It has been tested using an STM32L5 board (with cortex-m33) and the
sample applications shipped with the STM32Cube development
environment: GTZC_TZSC_MPCBB_TrustZone in
STM32CubeL5/Projects/NUCLEO-L552ZE-Q/Examples/GTZC.

The test consisted in setting breakpoints in various places and check
that the backtrace is correct: SecureFault_Callback (Non-secure mode),
__gnu_cmse_nonsecure_call (before and after the vpush instruction),
SecureFault_Handler (Secure mode).

This implies that we tested only some parts of this patch (only MSP*
were used), but remaining parts seem reasonable.
---
 gdb/NEWS            |   5 +
 gdb/arm-tdep.c      | 295 +++++++++++++++++++++++++++++++++++---------
 gdb/arm-tdep.h      |   1 +
 gdb/doc/gdb.texinfo |  11 ++
 4 files changed, 251 insertions(+), 61 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index eeca1d39b10..0bfa2cfab78 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -4984,6 +4984,11 @@ show arm force-mode
   the current CPSR value for instructions without symbols; previous
   versions of GDB behaved as if "set arm fallback-mode arm".
 
+set arm unwind-ns-to-s
+  Enable unwinding from Non-secure to Secure mode on Cortex-M with
+  Security extension.
+  This can trigger security exception when unwinding exception stack.
+
 set disable-randomization
 show disable-randomization
   Standalone programs run with the virtual address space randomization enabled
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 8115361d914..b35fb3ca08c 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -531,6 +531,7 @@ static CORE_ADDR arm_analyze_prologue
 /* See arm-tdep.h.  */
 
 bool arm_apcs_32 = true;
+bool arm_unwind_ns_to_s = true;
 
 /* Return the bit mask in ARM_PS_REGNUM that indicates Thumb mode.  */
 
@@ -747,28 +748,43 @@ arm_pc_is_thumb (struct gdbarch *gdbarch, CORE_ADDR memaddr)
    0xFFFFFFBC    Return to Thread mode using the process stack.  */
 
 static int
-arm_m_addr_is_magic (CORE_ADDR addr)
-{
-  switch (addr)
-    {
-      /* Values from ARMv8-M Architecture Technical Reference.  */
-      case 0xffffffb0:
-      case 0xffffffb8:
-      case 0xffffffbc:
-      /* Values from Tables in B1.5.8 the EXC_RETURN definitions of
-	 the exception return behavior.  */
-      case 0xffffffe1:
-      case 0xffffffe9:
-      case 0xffffffed:
-      case 0xfffffff1:
-      case 0xfffffff9:
-      case 0xfffffffd:
-	/* Address is magic.  */
-	return 1;
+arm_m_addr_is_magic (struct gdbarch *gdbarch, CORE_ADDR addr)
+{
+  arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
+  if (tdep->have_sec_ext)
+    {
+      switch ((addr & 0xff000000))
+	{
+	case 0xff000000: /* EXC_RETURN pattern.  */
+	case 0xfe000000: /* FNC_RETURN pattern.  */
+	  return 1;
+	default:
+	  return 0;
+	}
+    }
+  else
+    {
+      switch (addr)
+	{
+	  /* Values from ARMv8-M Architecture Technical Reference.  */
+	case 0xffffffb0:
+	case 0xffffffb8:
+	case 0xffffffbc:
+	  /* Values from Tables in B1.5.8 the EXC_RETURN definitions of
+	     the exception return behavior.  */
+	case 0xffffffe1:
+	case 0xffffffe9:
+	case 0xffffffed:
+	case 0xfffffff1:
+	case 0xfffffff9:
+	case 0xfffffffd:
+	  /* Address is magic.  */
+	  return 1;
 
-      default:
-	/* Address is not magic.  */
-	return 0;
+	default:
+	  /* Address is not magic.  */
+	  return 0;
+	}
     }
 }
 
@@ -780,7 +796,7 @@ arm_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR val)
 
   /* On M-profile devices, do not strip the low bit from EXC_RETURN
      (the magic exception return address).  */
-  if (tdep->is_m && arm_m_addr_is_magic (val))
+  if (tdep->is_m && arm_m_addr_is_magic (gdbarch, val))
     return val;
 
   if (arm_apcs_32)
@@ -2211,6 +2227,7 @@ arm_prologue_prev_register (struct frame_info *this_frame,
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   struct arm_prologue_cache *cache;
+  gdb::optional<CORE_ADDR> sp_value;
 
   if (*this_cache == NULL)
     *this_cache = arm_make_prologue_cache (this_frame);
@@ -2237,6 +2254,12 @@ arm_prologue_prev_register (struct frame_info *this_frame,
     return frame_unwind_got_constant (this_frame, prev_regnum,
 				      arm_cache_get_prev_sp (cache));
 
+  /* The value might be one of the alternative SP, if so, use the
+   * value already constructed.  */
+  sp_value = arm_cache_get_sp_register (cache, prev_regnum);
+  if (sp_value.has_value ())
+    return frame_unwind_got_constant (this_frame, prev_regnum, *sp_value);
+
   /* The CPSR may have been changed by the call instruction and by the
      called function.  The only bit we can reconstruct is the T bit,
      by checking the low bit of LR as of the call.  This is a reliable
@@ -3217,15 +3240,20 @@ static struct arm_prologue_cache *
 arm_m_exception_cache (struct frame_info *this_frame)
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   struct arm_prologue_cache *cache;
   CORE_ADDR lr;
+  CORE_ADDR sp;
   CORE_ADDR unwound_sp;
+  uint32_t sp_r0_offset = 0;
   LONGEST xpsr;
   uint32_t exc_return;
-  uint32_t process_stack_used;
+  uint32_t fnc_return;
   uint32_t extended_frame_used;
-  uint32_t secure_stack_used;
+  uint32_t secure_stack_used = 0;
+  uint32_t default_callee_register_stacking = 0;
+  uint32_t exception_domain_is_secure = 0;
 
   cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
   arm_cache_init (cache, this_frame);
@@ -3235,35 +3263,124 @@ arm_m_exception_cache (struct frame_info *this_frame)
      to the exception and if FPU is used (causing extended stack frame).  */
 
   lr = get_frame_register_unsigned (this_frame, ARM_LR_REGNUM);
+  sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
 
-  /* Check EXC_RETURN indicator bits.  */
-  exc_return = (((lr >> 28) & 0xf) == 0xf);
+  fnc_return = ((lr & 0xfffffffe) == 0xfefffffe);
+  if (tdep->have_sec_ext && fnc_return)
+    {
+      gdb_regnum actual_sp;
 
-  /* Check EXC_RETURN bit SPSEL if Main or Thread (process) stack used.  */
-  process_stack_used = ((lr & (1 << 2)) != 0);
-  if (exc_return && process_stack_used)
+      arm_cache_switch_prev_sp (cache, ARM_MSP_NS_REGNUM);
+      arm_cache_set_prev_sp (cache, sp);
+      if (lr & 1)
+	actual_sp = ARM_MSP_S_REGNUM;
+      else
+	actual_sp = ARM_MSP_NS_REGNUM;
+
+      arm_cache_switch_prev_sp (cache, actual_sp);
+      sp = get_frame_register_unsigned (this_frame, actual_sp);
+
+      cache->saved_regs[ARM_LR_REGNUM].set_addr (sp);
+
+      arm_cache_set_prev_sp (cache, sp + 8);
+
+      return cache;
+    }
+
+  /* Check EXC_RETURN indicator bits.  */
+  exc_return = (((lr >> 24) & 0xff) == 0xff);
+  if (exc_return)
     {
-      /* Thread (process) stack used, use PSP as SP.  */
-      unwound_sp = get_frame_register_unsigned (this_frame, ARM_PSP_REGNUM);
+      /* Check EXC_RETURN bit SPSEL if Main or Thread (process) stack used.  */
+      uint32_t process_stack_used = ((lr & (1 << 2)) != 0);
+
+      if (tdep->have_sec_ext)
+	{
+	  secure_stack_used = ((lr & (1 << 6)) != 0);
+	  default_callee_register_stacking = ((lr & (1 << 5)) != 0);
+	  exception_domain_is_secure = ((lr & (1 << 0)) == 0);
+
+	  /* Unwinding from non-secure to secure can trip security
+	   * measures.  In order to avoid the debugger to be
+	   * intrusive, rely on the user to configure the requested
+	   * mode.
+	   */
+	  if (secure_stack_used && !exception_domain_is_secure
+	      && !arm_unwind_ns_to_s)
+	    {
+	      warning (_("Non-secure to secure stack unwinding disabled."));
+
+	      /* Terminate any further stack unwinding by refer to self.  */
+	      arm_cache_set_prev_sp (cache, sp);
+	      return cache;
+	    }
+
+	  if (process_stack_used)
+	    {
+	      if (secure_stack_used)
+		  /* Secure thread (process) stack used, use PSP_S as SP.  */
+		  arm_cache_switch_prev_sp (cache, ARM_PSP_S_REGNUM);
+	      else
+		  /* Non-secure thread (process) stack used, use PSP_NS as SP.  */
+		  arm_cache_switch_prev_sp (cache, ARM_PSP_NS_REGNUM);
+	    }
+	  else
+	    {
+	      if (secure_stack_used)
+		/* Secure main stack used, use MSP_S as SP.  */
+		arm_cache_switch_prev_sp (cache, ARM_MSP_S_REGNUM);
+	      else
+		/* Non-secure main stack used, use MSP_NS as SP.  */
+		arm_cache_switch_prev_sp (cache, ARM_MSP_NS_REGNUM);
+	    }
+	}
+      else
+	{
+	  if (process_stack_used)
+	    /* Thread (process) stack used, use PSP as SP.  */
+	    arm_cache_switch_prev_sp (cache, ARM_PSP_REGNUM);
+	  else
+	    /* Main stack used, use MSP as SP.  */
+	    arm_cache_switch_prev_sp (cache, ARM_MSP_REGNUM);
+	}
     }
   else
     {
       /* Main stack used, use MSP as SP.  */
-      unwound_sp = get_frame_register_unsigned (this_frame, ARM_MSP_REGNUM);
+      arm_cache_switch_prev_sp (cache, ARM_MSP_REGNUM);
+    }
+
+  /* Fetch the SP to use for this frame.  */
+  unwound_sp = arm_cache_get_prev_sp (cache);
+
+  /* With the Security extension, the hardware saves R4..R11 too.  */
+  if (exc_return && tdep->have_sec_ext && secure_stack_used
+      && (!default_callee_register_stacking || exception_domain_is_secure))
+    {
+      /* Read R4..R11 from the integer callee registers.  */
+      cache->saved_regs[4].set_addr (unwound_sp + 0x08);
+      cache->saved_regs[5].set_addr (unwound_sp + 0x0C);
+      cache->saved_regs[6].set_addr (unwound_sp + 0x10);
+      cache->saved_regs[7].set_addr (unwound_sp + 0x14);
+      cache->saved_regs[8].set_addr (unwound_sp + 0x18);
+      cache->saved_regs[9].set_addr (unwound_sp + 0x1C);
+      cache->saved_regs[10].set_addr (unwound_sp + 0x20);
+      cache->saved_regs[11].set_addr (unwound_sp + 0x24);
+      sp_r0_offset = 0x28;
     }
 
   /* The hardware saves eight 32-bit words, comprising xPSR,
      ReturnAddress, LR (R14), R12, R3, R2, R1, R0.  See details in
      "B1.5.6 Exception entry behavior" in
      "ARMv7-M Architecture Reference Manual".  */
-  cache->saved_regs[0].set_addr (unwound_sp);
-  cache->saved_regs[1].set_addr (unwound_sp + 4);
-  cache->saved_regs[2].set_addr (unwound_sp + 8);
-  cache->saved_regs[3].set_addr (unwound_sp + 12);
-  cache->saved_regs[ARM_IP_REGNUM].set_addr (unwound_sp + 16);
-  cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + 20);
-  cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + 24);
-  cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + 28);
+  cache->saved_regs[0].set_addr (unwound_sp + sp_r0_offset);
+  cache->saved_regs[1].set_addr (unwound_sp + sp_r0_offset + 4);
+  cache->saved_regs[2].set_addr (unwound_sp + sp_r0_offset + 8);
+  cache->saved_regs[3].set_addr (unwound_sp + sp_r0_offset + 12);
+  cache->saved_regs[ARM_IP_REGNUM].set_addr (unwound_sp + sp_r0_offset + 16);
+  cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + sp_r0_offset + 20);
+  cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + sp_r0_offset + 24);
+  cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + sp_r0_offset + 28);
 
   /* Check EXC_RETURN bit FTYPE if extended stack frame (FPU regs stored)
      type used.  */
@@ -3282,41 +3399,43 @@ arm_m_exception_cache (struct frame_info *this_frame)
 	 This register is located at address 0xE000EF34.  */
 
       /* Extended stack frame type used.  */
-      fpu_regs_stack_offset = unwound_sp + 0x20;
+      fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x20;
       for (i = 0; i < 16; i++)
 	{
 	  cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);
 	  fpu_regs_stack_offset += 4;
 	}
-      cache->saved_regs[ARM_FPSCR_REGNUM].set_addr (unwound_sp + 0x60);
+      cache->saved_regs[ARM_FPSCR_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x60);
+      fpu_regs_stack_offset += 4;
+
+      if (tdep->have_sec_ext && !default_callee_register_stacking)
+	{
+	  /* Handle floating-point callee saved registers.  */
+	  fpu_regs_stack_offset = 0x90;
+	  for (i = 16; i < 32; i++)
+	    {
+	      cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);
+	      fpu_regs_stack_offset += 4;
+	    }
 
-      /* Offset 0x64 is reserved.  */
-      arm_cache_set_prev_sp (cache, unwound_sp + 0x68);
+	  arm_cache_set_prev_sp (cache, unwound_sp + sp_r0_offset + 0xD0);
+	}
+      else
+	{
+	  /* Offset 0x64 is reserved.  */
+	  arm_cache_set_prev_sp (cache, unwound_sp + sp_r0_offset + 0x68);
+	}
     }
   else
     {
       /* Standard stack frame type used.  */
-      arm_cache_set_prev_sp (cache, unwound_sp + 0x20);
-    }
-
-  /* Check EXC_RETURN bit S if Secure or Non-secure stack used.  */
-  secure_stack_used = ((lr & (1 << 6)) != 0);
-  if (exc_return && secure_stack_used)
-    {
-      /* ARMv8-M Exception and interrupt handling is not considered here.
-	 In the ARMv8-M architecture also EXC_RETURN bit S is controlling if
-	 the Secure or Non-secure stack was used. To separate Secure and
-	 Non-secure stacks, processors that are based on the ARMv8-M
-	 architecture support 4 stack pointers: MSP_S, PSP_S, MSP_NS, PSP_NS.
-	 In addition, a stack limit feature is provided using stack limit
-	 registers (accessible using MSR and MRS instructions) in Privileged
-	 level.  */
+      arm_cache_set_prev_sp (cache, unwound_sp + sp_r0_offset + 0x20);
     }
 
   /* If bit 9 of the saved xPSR is set, then there is a four-byte
      aligner between the top of the 32-byte stack frame and the
      previous context's stack pointer.  */
-  if (safe_read_memory_integer (unwound_sp + 28, 4, byte_order, &xpsr)
+  if (safe_read_memory_integer (unwound_sp + sp_r0_offset + 28, 4, byte_order, &xpsr)
       && (xpsr & (1 << 9)) != 0)
     arm_cache_set_prev_sp (cache, arm_cache_get_prev_sp (cache) + 4);
 
@@ -3351,6 +3470,7 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
 			       int prev_regnum)
 {
   struct arm_prologue_cache *cache;
+  gdb::optional<CORE_ADDR> sp_value;
 
   if (*this_cache == NULL)
     *this_cache = arm_m_exception_cache (this_frame);
@@ -3361,6 +3481,21 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
     return frame_unwind_got_constant (this_frame, prev_regnum,
 				      arm_cache_get_prev_sp (cache));
 
+  /* The value might be one of the alternative SP, if so, use the
+   * value already constructed.  */
+  sp_value = arm_cache_get_sp_register (cache, prev_regnum);
+  if (sp_value.has_value ())
+    return frame_unwind_got_constant (this_frame, prev_regnum, *sp_value);
+
+  if (prev_regnum == ARM_PC_REGNUM)
+    {
+      CORE_ADDR lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM);
+      struct gdbarch *gdbarch = get_frame_arch (this_frame);
+
+      return frame_unwind_got_constant (this_frame, prev_regnum,
+					arm_addr_bits_remove (gdbarch, lr));
+    }
+
   return trad_frame_get_prev_register (this_frame, cache->saved_regs,
 				       prev_regnum);
 }
@@ -3373,13 +3508,14 @@ arm_m_exception_unwind_sniffer (const struct frame_unwind *self,
 				struct frame_info *this_frame,
 				void **this_prologue_cache)
 {
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
   CORE_ADDR this_pc = get_frame_pc (this_frame);
 
   /* No need to check is_m; this sniffer is only registered for
      M-profile architectures.  */
 
   /* Check if exception frame returns to a magic PC value.  */
-  return arm_m_addr_is_magic (this_pc);
+  return arm_m_addr_is_magic (gdbarch, this_pc);
 }
 
 /* Frame unwinder for M-profile exceptions.  */
@@ -8793,6 +8929,15 @@ arm_show_force_mode (struct ui_file *file, int from_tty,
 		    arm_force_mode_string);
 }
 
+static void
+arm_show_unwind_ns_to_s (struct ui_file *file, int from_tty,
+			 struct cmd_list_element *c, const char *value)
+{
+  fprintf_filtered (file,
+		    _("Usage of non-secure to secure exception stack unwinding is %s.\n"),
+		    arm_unwind_ns_to_s ? "on" : "off");
+}
+
 /* If the user changes the register disassembly style used for info
    register and other commands, we have to also switch the style used
    in opcodes for disassembly output.  This function is run in the "set
@@ -9266,6 +9411,7 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   tdesc_arch_data_up tdesc_data;
   int i;
   bool is_m = false;
+  bool have_sec_ext = false;
   int vfp_register_count = 0;
   bool have_s_pseudos = false, have_q_pseudos = false;
   bool have_wmmx_registers = false;
@@ -9633,6 +9779,23 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 		have_q_pseudos = true;
 	    }
 	}
+      feature = tdesc_find_feature (tdesc,
+				    "org.gnu.gdb.arm.secext");
+      if (feature != NULL)
+	{
+	  valid_p &= tdesc_numbered_register (feature, tdesc_data.get (),
+					      ARM_MSP_S_REGNUM, "msp_s");
+	  valid_p &= tdesc_numbered_register (feature, tdesc_data.get (),
+					      ARM_MSP_NS_REGNUM, "msp_ns");
+	  valid_p &= tdesc_numbered_register (feature, tdesc_data.get (),
+					      ARM_PSP_S_REGNUM, "psp_s");
+	  valid_p &= tdesc_numbered_register (feature, tdesc_data.get (),
+					      ARM_PSP_NS_REGNUM, "psp_ns");
+	  if (!valid_p)
+	    return NULL;
+
+	  have_sec_ext = true;
+	}
     }
 
   /* If there is already a candidate, use it.  */
@@ -9673,6 +9836,7 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   tdep->arm_abi = arm_abi;
   tdep->fp_model = fp_model;
   tdep->is_m = is_m;
+  tdep->have_sec_ext = have_sec_ext;
   tdep->have_fpa_registers = have_fpa_registers;
   tdep->have_wmmx_registers = have_wmmx_registers;
   gdb_assert (vfp_register_count == 0
@@ -10083,6 +10247,15 @@ vfp - VFP co-processor."),
 			NULL, NULL, arm_show_force_mode,
 			&setarmcmdlist, &showarmcmdlist);
 
+  /* Add a command to stop triggering security exception when
+   * unwinding exception stack.  */
+  add_setshow_boolean_cmd ("unwind-ns-to-s", no_class, &arm_unwind_ns_to_s,
+			   _("Set usage of non-secure to secure exception stack unwinding."),
+			   _("Show usage of non-secure to secure exception stack unwinding."),
+			   _("When on, the debugger can trigger memory access traps."),
+			   NULL, arm_show_unwind_ns_to_s,
+			   &setarmcmdlist, &showarmcmdlist);
+
   /* Debugging flag.  */
   add_setshow_boolean_cmd ("arm", class_maintenance, &arm_debug,
 			   _("Set ARM debugging."),
diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
index 9012b9da100..12c6ae1a83e 100644
--- a/gdb/arm-tdep.h
+++ b/gdb/arm-tdep.h
@@ -120,6 +120,7 @@ struct arm_gdbarch_tdep : gdbarch_tdep
   int mve_pseudo_count = 0;	/* Total number of MVE pseudo registers.  */
 
   bool is_m = false;		/* Does the target follow the "M" profile.  */
+  bool have_sec_ext = false;	/* Do we have secure extensions?  */
   CORE_ADDR lowest_pc = 0;	/* Lowest address at which instructions
 				   will appear.  */
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9d507795993..66912684379 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -24894,6 +24894,17 @@ of @samp{set arm fallback-mode}.
 
 @item show arm force-mode
 Show the current forced instruction mode.
+Show the currently used ABI.
+
+@item set arm unwind-ns-to-s
+This command enables unwinding from Non-secure to Secure mode on
+Cortex-M with Security extension.
+This can trigger security exceptions when unwinding the exception
+stack.
+It is enabled by default.
+
+@item show arm unwind-ns-to-s
+Show whether unwind from Non-secure to Secure mode is enabled.
 
 @item set debug arm
 Toggle whether to display ARM-specific debugging messages from the ARM
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/5] gdb/arm: Fix prologue analysis to support vpush
  2022-01-14 16:35 [PATCH 1/5] gdb/arm: Fix prologue analysis to support vpush Christophe Lyon
                   ` (3 preceding siblings ...)
  2022-01-14 16:35 ` [PATCH 5/5] gdb/arm: Extend arm_m_addr_is_magic to support FNC_RETURN, add unwind-ns-to-s command Christophe Lyon
@ 2022-01-17 12:49 ` Luis Machado
  2022-01-24 13:55   ` Christophe Lyon
  4 siblings, 1 reply; 9+ messages in thread
From: Luis Machado @ 2022-01-17 12:49 UTC (permalink / raw)
  To: Christophe Lyon, gdb-patches; +Cc: torjborn.svensson

Hi!

On 1/14/22 1:35 PM, Christophe Lyon via Gdb-patches wrote:
> While working on adding support for Non-secure/Secure modes unwinding,
> I noticed that the prologue analysis lacked support for vpush, which
> is used for instance in the CMSE stub routine.

Thanks for the patch.

> 
> This patch updates thumb_analyze_prologue accordingly.
> ---
>   gdb/arm-tdep.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 7495434484e..14ec0bc8f9e 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -896,6 +896,27 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
>   		regs[bits (insn, 0, 3)] = addr;
>   	    }
>   

I wish we'd use constants for instruction masks, but right now the code 
is full of magic numbers, so it is fine to use these. The code doesn't 
change that much anyway.

> +	  else if ((insn & 0xff20) == 0xed20    /* vstmdb Rn{!},
> +						   { registers } */
> +		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
> +	    {
> +	      pv_t addr = regs[bits (insn, 0, 3)];
> +	      int number = bits (inst2, 0, 7) >> 1;

I'd add a comment making it clear what is being checked/extracted here...

> +
> +	      if (stack.store_would_trash (addr))
> +		break;
> +
> +	      /* Calculate offsets of saved registers.  */
> +	      for (; number > 0; number--)
> +		{
> +		  addr = pv_add_constant (addr, -8);
> +		  stack.store (addr, 8, pv_register (ARM_D0_REGNUM + number, 0));
> +		}
> +
> +	      if (insn & 0x0020)
> +		regs[bits (insn, 0, 3)] = addr;

... and here as well.

> +	    }
> +
>   	  else if ((insn & 0xff50) == 0xe940	/* strd Rt, Rt2,
>   						   [Rn, #+/-imm]{!} */
>   		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
> 

Otherwise this look good to me.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/5] gdb/arm: Define MSP and PSP registers for M-Profile
  2022-01-14 16:35 ` [PATCH 2/5] gdb/arm: Define MSP and PSP registers for M-Profile Christophe Lyon
@ 2022-01-17 12:50   ` Luis Machado
  0 siblings, 0 replies; 9+ messages in thread
From: Luis Machado @ 2022-01-17 12:50 UTC (permalink / raw)
  To: Christophe Lyon, gdb-patches; +Cc: torjborn.svensson

On 1/14/22 1:35 PM, Christophe Lyon via Gdb-patches wrote:
> This patch removes the hardcoded access to PSP in
> arm_m_exception_cache() and relies on the definition with the XML
> descriptions.
> ---
>   gdb/arch/arm.h                              |  6 ++-
>   gdb/arm-tdep.c                              | 45 ++++++++-------------
>   gdb/features/arm/arm-m-profile-with-fpa.c   |  2 +
>   gdb/features/arm/arm-m-profile-with-fpa.xml |  2 +
>   gdb/features/arm/arm-m-profile.c            |  2 +
>   gdb/features/arm/arm-m-profile.xml          |  2 +
>   6 files changed, 28 insertions(+), 31 deletions(-)
> 
> diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
> index eabcb434f1f..638780011fc 100644
> --- a/gdb/arch/arm.h
> +++ b/gdb/arch/arm.h
> @@ -49,6 +49,8 @@ enum gdb_regnum {
>     ARM_D0_REGNUM,		/* VFP double-precision registers.  */
>     ARM_D31_REGNUM = ARM_D0_REGNUM + 31,
>     ARM_FPSCR_REGNUM,
> +  ARM_MSP_REGNUM,		/* Cortex-M Main Stack Pointer.  */
> +  ARM_PSP_REGNUM,		/* Cortex-M Process Stack Pointer.  */

Commit ae66a8f19ef6bf2dc7369cf26073f34ddf7c175b started moving things 
away from these hardcoded register numbers. So I'd rather do a runtime 
discovery of register numbers as opposed to having these.

The above commit records some register set data in struct gdbarch_tdep:

   bool have_mve;        /* Do we have a MVE extension?  */
   int mve_vpr_regnum;   /* MVE VPR register number.  */
   int mve_pseudo_base;  /* Number of the first MVE pseudo register.  */
   int mve_pseudo_count; /* Total number of MVE pseudo registers.  */

So, for example, we'd record the numbers for both MSP and PSP.

>   
>     /* Other useful registers.  */
>     ARM_FP_REGNUM = 11,		/* Frame register in ARM code, if used.  */
> @@ -65,8 +67,8 @@ enum arm_register_counts {
>     ARM_NUM_ARG_REGS = 4,
>     /* Number of floating point argument registers.  */
>     ARM_NUM_FP_ARG_REGS = 4,
> -  /* Number of registers (old, defined as ARM_FPSCR_REGNUM + 1.  */
> -  ARM_NUM_REGS = ARM_FPSCR_REGNUM + 1
> +  /* Number of registers (old, defined as ARM_PSP_REGNUM + 1.  */
> +  ARM_NUM_REGS = ARM_PSP_REGNUM + 1
>   };

The above constant (ARM_NUM_REGS) is no longer representative of the 
full set of available registers. It is now used to initialize the number 
of registers before discovery and xml parsing. I'd like to see this 
numbering scheme replaced by a more dynamic mechanism, similar to how 
the MVE register set does.

>   
>   /* Enum describing the different kinds of breakpoints.  */
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 14ec0bc8f9e..b6a1deafad5 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -3012,7 +3012,6 @@ arm_m_exception_cache (struct frame_info *this_frame)
>     enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>     struct arm_prologue_cache *cache;
>     CORE_ADDR lr;
> -  CORE_ADDR sp;
>     CORE_ADDR unwound_sp;
>     LONGEST xpsr;
>     uint32_t exc_return;
> @@ -3028,7 +3027,6 @@ arm_m_exception_cache (struct frame_info *this_frame)
>        to the exception and if FPU is used (causing extended stack frame).  */
>   
>     lr = get_frame_register_unsigned (this_frame, ARM_LR_REGNUM);
> -  sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>   
>     /* Check EXC_RETURN indicator bits.  */
>     exc_return = (((lr >> 28) & 0xf) == 0xf);
> @@ -3037,37 +3035,13 @@ arm_m_exception_cache (struct frame_info *this_frame)
>     process_stack_used = ((lr & (1 << 2)) != 0);
>     if (exc_return && process_stack_used)
>       {
> -      /* Thread (process) stack used.
> -	 Potentially this could be other register defined by target, but PSP
> -	 can be considered a standard name for the "Process Stack Pointer".
> -	 To be fully aware of system registers like MSP and PSP, these could
> -	 be added to a separate XML arm-m-system-profile that is valid for
> -	 ARMv6-M and ARMv7-M architectures. Also to be able to debug eg a
> -	 corefile off-line, then these registers must be defined by GDB,
> -	 and also be included in the corefile regsets.  */
> -
> -      int psp_regnum = user_reg_map_name_to_regnum (gdbarch, "psp", -1);
> -      if (psp_regnum == -1)
> -	{
> -	  /* Thread (process) stack could not be fetched,
> -	     give warning and exit.  */
> -
> -	  warning (_("no PSP thread stack unwinding supported."));
> -
> -	  /* Terminate any further stack unwinding by refer to self.  */
> -	  cache->prev_sp = sp;
> -	  return cache;
> -	}
> -      else
> -	{
> -	  /* Thread (process) stack used, use PSP as SP.  */
> -	  unwound_sp = get_frame_register_unsigned (this_frame, psp_regnum);
> -	}
> +      /* Thread (process) stack used, use PSP as SP.  */
> +      unwound_sp = get_frame_register_unsigned (this_frame, ARM_PSP_REGNUM);
>       }
>     else
>       {
>         /* Main stack used, use MSP as SP.  */
> -      unwound_sp = sp;
> +      unwound_sp = get_frame_register_unsigned (this_frame, ARM_MSP_REGNUM);
>       } >
>     /* The hardware saves eight 32-bit words, comprising xPSR,
> @@ -9296,6 +9270,19 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>         if (!valid_p)
>   	return NULL;
>   
> +      if (is_m)
> +	{
> +	  feature = tdesc_find_feature (tdesc,
> +					"org.gnu.gdb.arm.m-system");

The above xml feature is not present in the xml files that were modified 
below. Is this feature being generated by some debugging stub/probe maybe?

> +	  if (feature != NULL)
> +	    {
> +	      valid_p &= tdesc_numbered_register (feature, tdesc_data.get (),
> +						  ARM_MSP_REGNUM, "msp");
> +	      valid_p &= tdesc_numbered_register (feature, tdesc_data.get (),
> +						  ARM_PSP_REGNUM, "psp");
> +	    }
> +	}
> +
>         feature = tdesc_find_feature (tdesc,
>   				    "org.gnu.gdb.arm.fpa");
>         if (feature != NULL)
> diff --git a/gdb/features/arm/arm-m-profile-with-fpa.c b/gdb/features/arm/arm-m-profile-with-fpa.c
> index 2b7c78597bb..4afba856875 100644
> --- a/gdb/features/arm/arm-m-profile-with-fpa.c
> +++ b/gdb/features/arm/arm-m-profile-with-fpa.c
> @@ -35,5 +35,7 @@ create_feature_arm_arm_m_profile_with_fpa (struct target_desc *result, long regn
>     tdesc_create_reg (feature, "", regnum++, 1, NULL, 96, "arm_fpa_ext");
>     tdesc_create_reg (feature, "", regnum++, 1, NULL, 32, "int");
>     tdesc_create_reg (feature, "xpsr", regnum++, 1, NULL, 32, "int");
> +  tdesc_create_reg (feature, "msp", regnum++, 1, NULL, 32, "data_ptr");
> +  tdesc_create_reg (feature, "psp", regnum++, 1, NULL, 32, "data_ptr");
>     return regnum;
>   }
> diff --git a/gdb/features/arm/arm-m-profile-with-fpa.xml b/gdb/features/arm/arm-m-profile-with-fpa.xml
> index fceeaea7177..ee796b549a2 100644
> --- a/gdb/features/arm/arm-m-profile-with-fpa.xml
> +++ b/gdb/features/arm/arm-m-profile-with-fpa.xml
> @@ -36,4 +36,6 @@
>     <reg name="" bitsize="32"/>
>   
>     <reg name="xpsr" bitsize="32" regnum="25"/>
> +  <reg name="msp" bitsize="32" type="data_ptr"/>
> +  <reg name="psp" bitsize="32" type="data_ptr"/>

In this xml...

>   </feature>
> diff --git a/gdb/features/arm/arm-m-profile.c b/gdb/features/arm/arm-m-profile.c
> index 027f3c15c56..db6c0b10d15 100644
> --- a/gdb/features/arm/arm-m-profile.c
> +++ b/gdb/features/arm/arm-m-profile.c
> @@ -27,5 +27,7 @@ create_feature_arm_arm_m_profile (struct target_desc *result, long regnum)
>     tdesc_create_reg (feature, "pc", regnum++, 1, NULL, 32, "code_ptr");
>     regnum = 25;
>     tdesc_create_reg (feature, "xpsr", regnum++, 1, NULL, 32, "int");
> +  tdesc_create_reg (feature, "msp", regnum++, 1, NULL, 32, "data_ptr");
> +  tdesc_create_reg (feature, "psp", regnum++, 1, NULL, 32, "data_ptr");
>     return regnum;
>   }
> diff --git a/gdb/features/arm/arm-m-profile.xml b/gdb/features/arm/arm-m-profile.xml
> index d56fb246342..3253d1c5339 100644
> --- a/gdb/features/arm/arm-m-profile.xml
> +++ b/gdb/features/arm/arm-m-profile.xml
> @@ -24,4 +24,6 @@
>     <reg name="lr" bitsize="32"/>
>     <reg name="pc" bitsize="32" type="code_ptr"/>
>     <reg name="xpsr" bitsize="32" regnum="25"/>
> +  <reg name="msp" bitsize="32" type="data_ptr"/>
> +  <reg name="psp" bitsize="32" type="data_ptr"/>

... and in this xml the psp and msp registers are present in the 
"org.gnu.gdb.arm.m-profile" feature, not "org.gnu.gdb.arm.m-system".

So the code above that checks for the "org.gnu.gdb.arm.m-system" feature 
won't work as expected.

I like the idea of having these two registers in a separate feature like 
"org.gnu.gdb.arm.m-system". I think it is cleaner that way, and easier 
to manage.

>   </feature>
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/5] gdb/arm: Fix prologue analysis to support vpush
  2022-01-17 12:49 ` [PATCH 1/5] gdb/arm: Fix prologue analysis to support vpush Luis Machado
@ 2022-01-24 13:55   ` Christophe Lyon
  0 siblings, 0 replies; 9+ messages in thread
From: Christophe Lyon @ 2022-01-24 13:55 UTC (permalink / raw)
  To: Luis Machado; +Cc: Christophe Lyon, gdb-patches, torjborn.svensson

[-- Attachment #1: Type: text/plain, Size: 2355 bytes --]

On Mon, Jan 17, 2022 at 1:50 PM Luis Machado via Gdb-patches <
gdb-patches@sourceware.org> wrote:

> Hi!
>
> On 1/14/22 1:35 PM, Christophe Lyon via Gdb-patches wrote:
> > While working on adding support for Non-secure/Secure modes unwinding,
> > I noticed that the prologue analysis lacked support for vpush, which
> > is used for instance in the CMSE stub routine.
>
> Thanks for the patch.
>
> >
> > This patch updates thumb_analyze_prologue accordingly.
> > ---
> >   gdb/arm-tdep.c | 21 +++++++++++++++++++++
> >   1 file changed, 21 insertions(+)
> >
> > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> > index 7495434484e..14ec0bc8f9e 100644
> > --- a/gdb/arm-tdep.c
> > +++ b/gdb/arm-tdep.c
> > @@ -896,6 +896,27 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
> >               regs[bits (insn, 0, 3)] = addr;
> >           }
> >
>
> I wish we'd use constants for instruction masks, but right now the code
> is full of magic numbers, so it is fine to use these. The code doesn't
> change that much anyway.
>

Yeah, I merely cut/pasted the previous code block, just updating the fields.


>
> > +       else if ((insn & 0xff20) == 0xed20    /* vstmdb Rn{!},
> > +                                                { registers } */
> > +                && pv_is_register (regs[bits (insn, 0, 3)],
> ARM_SP_REGNUM))
> > +         {
> > +           pv_t addr = regs[bits (insn, 0, 3)];
> > +           int number = bits (inst2, 0, 7) >> 1;
>
> I'd add a comment making it clear what is being checked/extracted here...
>
> > +
> > +           if (stack.store_would_trash (addr))
> > +             break;
> > +
> > +           /* Calculate offsets of saved registers.  */
> > +           for (; number > 0; number--)
> > +             {
> > +               addr = pv_add_constant (addr, -8);
> > +               stack.store (addr, 8, pv_register (ARM_D0_REGNUM +
> number, 0));
> > +             }
> > +
> > +           if (insn & 0x0020)
> > +             regs[bits (insn, 0, 3)] = addr;
>
> ... and here as well.
>
> > +         }
> > +
> >         else if ((insn & 0xff50) == 0xe940    /* strd Rt, Rt2,
> >                                                  [Rn, #+/-imm]{!} */
> >                  && pv_is_register (regs[bits (insn, 0, 3)],
> ARM_SP_REGNUM))
> >
>
> Otherwise this look good to me.
>

Is the attached patch OK?

Thanks,

Christophe

[-- Attachment #2: 0001-gdb-arm-Fix-prologue-analysis-to-support-vpush.patch --]
[-- Type: text/x-patch, Size: 1756 bytes --]

From 39da58e92d00cb55c3f07d638afc4c617f9c1bb8 Mon Sep 17 00:00:00 2001
From: Christophe Lyon <christophe.lyon@linaro.org>
Date: Fri, 14 Jan 2022 15:24:02 +0100
Subject: [PATCH] gdb/arm: Fix prologue analysis to support vpush

While working on adding support for Non-secure/Secure modes unwinding,
I noticed that the prologue analysis lacked support for vpush, which
is used for instance in the CMSE stub routine.

This patch updates thumb_analyze_prologue accordingly, adding support
for vpush of D-registers.
---
 gdb/arm-tdep.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 7495434484e..32e7b9ceb87 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -896,6 +896,31 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
 		regs[bits (insn, 0, 3)] = addr;
 	    }
 
+	  else if ((insn & 0xff20) == 0xed20    /* vstmdb Rn{!}, { D-registers} */
+		   && (insn2 & 0x0f00) == 0x0b00/* (aka vpush) */
+		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    {
+	      /* Address SP points to.  */
+	      pv_t addr = regs[bits (insn, 0, 3)];
+
+	      /* Number of registers saved.  */
+	      int number = bits (inst2, 0, 7) >> 1;
+
+	      if (stack.store_would_trash (addr))
+		break;
+
+	      /* Calculate offsets of saved registers.  */
+	      for (; number > 0; number--)
+		{
+		  addr = pv_add_constant (addr, -8);
+		  stack.store (addr, 8, pv_register (ARM_D0_REGNUM + number, 0));
+		}
+
+	      /* Writeback SP if needed.  */
+	      if (insn & 0x0020)
+		regs[bits (insn, 0, 3)] = addr;
+	    }
+
 	  else if ((insn & 0xff50) == 0xe940	/* strd Rt, Rt2,
 						   [Rn, #+/-imm]{!} */
 		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/5] gdb/arm: Fix prologue analysis to support vpush
@ 2022-01-17  8:49 Christophe Lyon
  0 siblings, 0 replies; 9+ messages in thread
From: Christophe Lyon @ 2022-01-17  8:49 UTC (permalink / raw)
  To: gdb-patches; +Cc: torbjorn.svensson, Christophe Lyon

[re-sent with the right cc: address]

While working on adding support for Non-secure/Secure modes unwinding,
I noticed that the prologue analysis lacked support for vpush, which
is used for instance in the CMSE stub routine.

This patch updates thumb_analyze_prologue accordingly.
---
 gdb/arm-tdep.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 7495434484e..14ec0bc8f9e 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -896,6 +896,27 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
 		regs[bits (insn, 0, 3)] = addr;
 	    }
 
+	  else if ((insn & 0xff20) == 0xed20    /* vstmdb Rn{!},
+						   { registers } */
+		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    {
+	      pv_t addr = regs[bits (insn, 0, 3)];
+	      int number = bits (inst2, 0, 7) >> 1;
+
+	      if (stack.store_would_trash (addr))
+		break;
+
+	      /* Calculate offsets of saved registers.  */
+	      for (; number > 0; number--)
+		{
+		  addr = pv_add_constant (addr, -8);
+		  stack.store (addr, 8, pv_register (ARM_D0_REGNUM + number, 0));
+		}
+
+	      if (insn & 0x0020)
+		regs[bits (insn, 0, 3)] = addr;
+	    }
+
 	  else if ((insn & 0xff50) == 0xe940	/* strd Rt, Rt2,
 						   [Rn, #+/-imm]{!} */
 		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-01-24 13:55 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-14 16:35 [PATCH 1/5] gdb/arm: Fix prologue analysis to support vpush Christophe Lyon
2022-01-14 16:35 ` [PATCH 2/5] gdb/arm: Define MSP and PSP registers for M-Profile Christophe Lyon
2022-01-17 12:50   ` Luis Machado
2022-01-14 16:35 ` [PATCH 3/5] gdb/arm: Introduce arm_cache_init Christophe Lyon
2022-01-14 16:35 ` [PATCH 4/5] gdb/arm: Add support for multiple stack pointers on Cortex-M Christophe Lyon
2022-01-14 16:35 ` [PATCH 5/5] gdb/arm: Extend arm_m_addr_is_magic to support FNC_RETURN, add unwind-ns-to-s command Christophe Lyon
2022-01-17 12:49 ` [PATCH 1/5] gdb/arm: Fix prologue analysis to support vpush Luis Machado
2022-01-24 13:55   ` Christophe Lyon
2022-01-17  8:49 Christophe Lyon

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