Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [PATCH/arm] Backtrace through exception frames on arm/cortex-m target
@ 2012-11-13 14:16 Yao Qi
  2012-11-13 14:27 ` Mark Kettenis
  2012-11-13 15:50 ` Joel Brobecker
  0 siblings, 2 replies; 8+ messages in thread
From: Yao Qi @ 2012-11-13 14:16 UTC (permalink / raw)
  To: gdb-patches

Hi,
This patch adds support for backtracing through exceptions on
M-profile targets.  Dan wrote this patch two years ago, tested
by hand that set a breakpoint on an interrupt handler and GDB will
show the complete stack frame including the exception handler
and the source location that caused the fault.  I update the patch a
little for some API changes in GDB trunk.

I considered to write a test case for this, but it hard to write
single case for different corex-m boards with different system
libraries.  So no test case is included.

Regression tested for arm-none-eabi (-march=armv6-m and -march=armv7-m
respectively) and arm-none-linux-gnueabi (-marm/-mthumb
-march=armv4/-mthumb -march=armv7-a).  OK to apply?

gdb:

2012-11-13  Daniel Jacobowitz  <dan@codesourcery.com>
	    Yao Qi  <yao@codesourcery.com>

	* arm-tdep.c (arm_addr_bits_remove): Do not adjust the low
	bit of EXC_RETURN.
	(arm_m_exception_cache, arm_m_exception_this_id)
	(arm_m_exception_prev_register, arm_m_exception_unwind_sniffer)
	(arm_m_exception_unwind): New.
	(arm_gdbarch_init): Register arm_m_exception_unwind.
---
 gdb/arm-tdep.c |  104 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 104 insertions(+), 0 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 1a67366..5f22649 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -448,6 +448,11 @@ arm_pc_is_thumb (struct gdbarch *gdbarch, CORE_ADDR memaddr)
 static CORE_ADDR
 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 (gdbarch_tdep (gdbarch)->is_m && (val & 0xfffffff0) == 0xfffffff0)
+    return val;
+
   if (arm_apcs_32)
     return UNMAKE_THUMB_ADDR (val);
   else
@@ -2926,6 +2931,103 @@ struct frame_unwind arm_stub_unwind = {
   arm_stub_unwind_sniffer
 };
 
+static struct arm_prologue_cache *
+arm_m_exception_cache (struct frame_info *this_frame)
+{
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+  struct arm_prologue_cache *cache;
+  CORE_ADDR unwound_sp;
+  LONGEST xpsr;
+
+  cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
+  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+
+  unwound_sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
+
+  cache->saved_regs[0].addr = unwound_sp;
+  cache->saved_regs[1].addr = unwound_sp + 4;
+  cache->saved_regs[2].addr = unwound_sp + 8;
+  cache->saved_regs[3].addr = unwound_sp + 12;
+  cache->saved_regs[12].addr = unwound_sp + 16;
+  cache->saved_regs[14].addr = unwound_sp + 20;
+  cache->saved_regs[15].addr = unwound_sp + 24;
+  cache->saved_regs[ARM_PS_REGNUM].addr = unwound_sp + 28;
+
+  /* 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.  */
+  cache->prev_sp = unwound_sp + 32;
+  if (safe_read_memory_integer (unwound_sp + 28, 4, byte_order, &xpsr)
+      && (xpsr & (1 << 9)) != 0)
+    cache->prev_sp += 4;
+
+  return cache;
+}
+
+/* Our frame ID for a stub frame is the current SP and LR.  */
+
+static void
+arm_m_exception_this_id (struct frame_info *this_frame,
+			 void **this_cache,
+			 struct frame_id *this_id)
+{
+  struct arm_prologue_cache *cache;
+
+  if (*this_cache == NULL)
+    *this_cache = arm_m_exception_cache (this_frame);
+  cache = *this_cache;
+
+  *this_id = frame_id_build (cache->prev_sp, get_frame_pc (this_frame));
+}
+
+static struct value *
+arm_m_exception_prev_register (struct frame_info *this_frame,
+			       void **this_cache,
+			       int prev_regnum)
+{
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  struct arm_prologue_cache *cache;
+
+  if (*this_cache == NULL)
+    *this_cache = arm_m_exception_cache (this_frame);
+  cache = *this_cache;
+
+  /* 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 trad_frame_get_prev_register (this_frame, cache->saved_regs,
+				       prev_regnum);
+}
+
+static int
+arm_m_exception_unwind_sniffer (const struct frame_unwind *self,
+				struct frame_info *this_frame,
+				void **this_prologue_cache)
+{
+  CORE_ADDR this_pc = get_frame_pc (this_frame);
+
+  /* No need to check is_m; this sniffer is only registered for
+     M-profile architectures.  */
+
+  /* Exception frames return to one of these magic PCs.  Other values
+     are not defined as of v7-M.  */
+  if (this_pc == 0xfffffff1 || this_pc == 0xfffffff9 || this_pc == 0xfffffffd)
+    return 1;
+
+  return 0;
+}
+
+struct frame_unwind arm_m_exception_unwind = {
+  SIGTRAMP_FRAME,
+  default_frame_unwind_stop_reason,
+  arm_m_exception_this_id,
+  arm_m_exception_prev_register,
+  NULL,
+  arm_m_exception_unwind_sniffer
+};
+
 static CORE_ADDR
 arm_normal_frame_base (struct frame_info *this_frame, void **this_cache)
 {
@@ -10218,6 +10320,8 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   dwarf2_frame_set_init_reg (gdbarch, arm_dwarf2_frame_init_reg);
 
   /* Add some default predicates.  */
+  if (is_m)
+    frame_unwind_append_unwinder (gdbarch, &arm_m_exception_unwind);
   frame_unwind_append_unwinder (gdbarch, &arm_stub_unwind);
   dwarf2_append_unwinders (gdbarch);
   frame_unwind_append_unwinder (gdbarch, &arm_exidx_unwind);
-- 
1.7.7.6


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

* Re: [PATCH/arm] Backtrace through exception frames on arm/cortex-m target
  2012-11-13 14:16 [PATCH/arm] Backtrace through exception frames on arm/cortex-m target Yao Qi
@ 2012-11-13 14:27 ` Mark Kettenis
  2012-11-13 14:44   ` Yao Qi
  2012-11-13 15:50 ` Joel Brobecker
  1 sibling, 1 reply; 8+ messages in thread
From: Mark Kettenis @ 2012-11-13 14:27 UTC (permalink / raw)
  To: yao; +Cc: gdb-patches

> From: Yao Qi <yao@codesourcery.com>
> Date: Tue, 13 Nov 2012 22:15:40 +0800
> 
> Hi,
> This patch adds support for backtracing through exceptions on
> M-profile targets.  Dan wrote this patch two years ago, tested
> by hand that set a breakpoint on an interrupt handler and GDB will
> show the complete stack frame including the exception handler
> and the source location that caused the fault.  I update the patch a
> little for some API changes in GDB trunk.
> 
> I considered to write a test case for this, but it hard to write
> single case for different corex-m boards with different system
> libraries.  So no test case is included.
> 
> Regression tested for arm-none-eabi (-march=armv6-m and -march=armv7-m
> respectively) and arm-none-linux-gnueabi (-marm/-mthumb
> -march=armv4/-mthumb -march=armv7-a).  OK to apply?

No.  Because...

> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 1a67366..5f22649 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -448,6 +448,11 @@ arm_pc_is_thumb (struct gdbarch *gdbarch, CORE_ADDR memaddr)
>  static CORE_ADDR
>  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 (gdbarch_tdep (gdbarch)->is_m && (val & 0xfffffff0) == 0xfffffff0)
> +    return val;
> +
>    if (arm_apcs_32)
>      return UNMAKE_THUMB_ADDR (val);
>    else
> @@ -2926,6 +2931,103 @@ struct frame_unwind arm_stub_unwind = {
>    arm_stub_unwind_sniffer
>  };
>  
> +static struct arm_prologue_cache *
> +arm_m_exception_cache (struct frame_info *this_frame)
> +{
> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  struct arm_prologue_cache *cache;
> +  CORE_ADDR unwound_sp;
> +  LONGEST xpsr;
> +
> +  cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
> +  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
> +
> +  unwound_sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);

Calling get_frame_register_unsign(this_frame, ...) in the unwinder
itself is verboten!.


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

* Re: [PATCH/arm] Backtrace through exception frames on arm/cortex-m target
  2012-11-13 14:27 ` Mark Kettenis
@ 2012-11-13 14:44   ` Yao Qi
  2012-11-13 15:12     ` Mark Kettenis
  0 siblings, 1 reply; 8+ messages in thread
From: Yao Qi @ 2012-11-13 14:44 UTC (permalink / raw)
  To: Mark Kettenis; +Cc: gdb-patches

On 11/13/2012 10:27 PM, Mark Kettenis wrote:
>> +  unwound_sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
> Calling get_frame_register_unsign(this_frame, ...) in the unwinder
> itself is verboten!.

Really?  It is a surprise to me.  I see get_frame_register_unsigned is 
called in many other unwinders, for example,

*** alpha-mdebug-tdep.c:
alpha_mdebug_frame_unwind_cache[217] vfp = get_frame_register_unsigned 
(this_frame, PROC_FRAME_REG (proc_desc));

*** amd64obsd-tdep.c:
amd64obsd_trapframe_cache[373] sp = get_frame_register_unsigned 
(this_frame, AMD64_RSP_REGNUM);

*** arm-linux-tdep.c:
arm_linux_sigtramp_cache[258]  CORE_ADDR sp = 
get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);

*** avr-tdep.c:
avr_frame_unwind_cache[987]    this_base = get_frame_register_unsigned 
(this_frame, AVR_FP_REGNUM);

Am I missing something?

-- 
Yao


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

* Re: [PATCH/arm] Backtrace through exception frames on arm/cortex-m target
  2012-11-13 14:44   ` Yao Qi
@ 2012-11-13 15:12     ` Mark Kettenis
  0 siblings, 0 replies; 8+ messages in thread
From: Mark Kettenis @ 2012-11-13 15:12 UTC (permalink / raw)
  To: yao; +Cc: gdb-patches

> Date: Tue, 13 Nov 2012 22:44:07 +0800
> From: Yao Qi <yao@codesourcery.com>
> 
> On 11/13/2012 10:27 PM, Mark Kettenis wrote:
> >> +  unwound_sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
> > Calling get_frame_register_unsign(this_frame, ...) in the unwinder
> > itself is verboten!.
> 
> Really?  It is a surprise to me.  I see get_frame_register_unsigned is 
> called in many other unwinders, for example,
> 
> *** alpha-mdebug-tdep.c:
> alpha_mdebug_frame_unwind_cache[217] vfp = get_frame_register_unsigned 
> (this_frame, PROC_FRAME_REG (proc_desc));
> 
> *** amd64obsd-tdep.c:
> amd64obsd_trapframe_cache[373] sp = get_frame_register_unsigned 
> (this_frame, AMD64_RSP_REGNUM);
> 
> *** arm-linux-tdep.c:
> arm_linux_sigtramp_cache[258]  CORE_ADDR sp = 
> get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
> 
> *** avr-tdep.c:
> avr_frame_unwind_cache[987]    this_base = get_frame_register_unsigned 
> (this_frame, AVR_FP_REGNUM);
> 
> Am I missing something?

Ugh, no, you're right.  It's unwinding THIS_FRAME in the unwinder that
is verboten.  Objection withdrawn.


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

* Re: [PATCH/arm] Backtrace through exception frames on arm/cortex-m target
  2012-11-13 14:16 [PATCH/arm] Backtrace through exception frames on arm/cortex-m target Yao Qi
  2012-11-13 14:27 ` Mark Kettenis
@ 2012-11-13 15:50 ` Joel Brobecker
  2012-11-14  2:23   ` Yao Qi
  1 sibling, 1 reply; 8+ messages in thread
From: Joel Brobecker @ 2012-11-13 15:50 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

> 2012-11-13  Daniel Jacobowitz  <dan@codesourcery.com>
> 	    Yao Qi  <yao@codesourcery.com>
> 
> 	* arm-tdep.c (arm_addr_bits_remove): Do not adjust the low
> 	bit of EXC_RETURN.
> 	(arm_m_exception_cache, arm_m_exception_this_id)
> 	(arm_m_exception_prev_register, arm_m_exception_unwind_sniffer)
> 	(arm_m_exception_unwind): New.
> 	(arm_gdbarch_init): Register arm_m_exception_unwind.

Yao, can you add a comment/description for each new function that
you introduce?

-- 
Joel


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

* Re: [PATCH/arm] Backtrace through exception frames on arm/cortex-m target
  2012-11-13 15:50 ` Joel Brobecker
@ 2012-11-14  2:23   ` Yao Qi
  2012-11-14  2:57     ` Joel Brobecker
  0 siblings, 1 reply; 8+ messages in thread
From: Yao Qi @ 2012-11-14  2:23 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches

On 11/13/2012 11:49 PM, Joel Brobecker wrote:
>> 2012-11-13  Daniel Jacobowitz<dan@codesourcery.com>
>> >	    Yao Qi<yao@codesourcery.com>
>> >
>> >	* arm-tdep.c (arm_addr_bits_remove): Do not adjust the low
>> >	bit of EXC_RETURN.
>> >	(arm_m_exception_cache, arm_m_exception_this_id)
>> >	(arm_m_exception_prev_register, arm_m_exception_unwind_sniffer)
>> >	(arm_m_exception_unwind): New.
>> >	(arm_gdbarch_init): Register arm_m_exception_unwind.
> Yao, can you add a comment/description for each new function that
> you introduce?

Sure.  We didn't add comments to these functions because they are
installed to 'struct frame_unwind' to compose a unwinder for a specific
type of frames.  The situation is similar to gdbarch hook functions, so
I add comment in the similar way, for example,

+/* Implementation of function hook 'this_id' in
+   'struct frame_uwnind'.  */
+
+static void
+arm_m_exception_this_id (struct frame_info *this_frame,

besides them, I also add some comments on the code, point readers to
the right section of the right manual, which is more useful, IMO.

Below is the updated patch.

-- 
Yao

gdb:

2012-11-14  Daniel Jacobowitz  <dan@codesourcery.com>
	    Yao Qi  <yao@codesourcery.com>

	* arm-tdep.c (arm_addr_bits_remove): Do not adjust the low
	bit of EXC_RETURN.
	(arm_m_exception_cache, arm_m_exception_this_id)
	(arm_m_exception_prev_register, arm_m_exception_unwind_sniffer)
	(arm_m_exception_unwind): New.
	(arm_gdbarch_init): Register arm_m_exception_unwind.
---
 gdb/arm-tdep.c |  122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 122 insertions(+), 0 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 1a67366..01af187 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -448,6 +448,11 @@ arm_pc_is_thumb (struct gdbarch *gdbarch, CORE_ADDR memaddr)
 static CORE_ADDR
 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 (gdbarch_tdep (gdbarch)->is_m && (val & 0xfffffff0) == 0xfffffff0)
+    return val;
+
   if (arm_apcs_32)
     return UNMAKE_THUMB_ADDR (val);
   else
@@ -2926,6 +2931,121 @@ struct frame_unwind arm_stub_unwind = {
   arm_stub_unwind_sniffer
 };
 
+/* Put here the code to store, into CACHE->saved_regs, the addresses of
+   the saved registers of frame described by THIS_FRAME.  CACHE is
+   returned.  */
+
+static struct arm_prologue_cache *
+arm_m_exception_cache (struct frame_info *this_frame)
+{
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+  struct arm_prologue_cache *cache;
+  CORE_ADDR unwound_sp;
+  LONGEST xpsr;
+
+  cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
+  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+
+  unwound_sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
+
+  /* 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].addr = unwound_sp;
+  cache->saved_regs[1].addr = unwound_sp + 4;
+  cache->saved_regs[2].addr = unwound_sp + 8;
+  cache->saved_regs[3].addr = unwound_sp + 12;
+  cache->saved_regs[12].addr = unwound_sp + 16;
+  cache->saved_regs[14].addr = unwound_sp + 20;
+  cache->saved_regs[15].addr = unwound_sp + 24;
+  cache->saved_regs[ARM_PS_REGNUM].addr = unwound_sp + 28;
+
+  /* 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.  */
+  cache->prev_sp = unwound_sp + 32;
+  if (safe_read_memory_integer (unwound_sp + 28, 4, byte_order, &xpsr)
+      && (xpsr & (1 << 9)) != 0)
+    cache->prev_sp += 4;
+
+  return cache;
+}
+
+/* Implementation of function hook 'this_id' in
+   'struct frame_uwnind'.  */
+
+static void
+arm_m_exception_this_id (struct frame_info *this_frame,
+			 void **this_cache,
+			 struct frame_id *this_id)
+{
+  struct arm_prologue_cache *cache;
+
+  if (*this_cache == NULL)
+    *this_cache = arm_m_exception_cache (this_frame);
+  cache = *this_cache;
+
+  /* Our frame ID for a stub frame is the current SP and LR.  */
+  *this_id = frame_id_build (cache->prev_sp, get_frame_pc (this_frame));
+}
+
+/* Implementation of function hook 'prev_register' in
+   'struct frame_uwnind'.  */
+
+static struct value *
+arm_m_exception_prev_register (struct frame_info *this_frame,
+			       void **this_cache,
+			       int prev_regnum)
+{
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  struct arm_prologue_cache *cache;
+
+  if (*this_cache == NULL)
+    *this_cache = arm_m_exception_cache (this_frame);
+  cache = *this_cache;
+
+  /* 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 trad_frame_get_prev_register (this_frame, cache->saved_regs,
+				       prev_regnum);
+}
+
+/* Implementation of function hook 'sniffer' in
+   'struct frame_uwnind'.  */
+
+static int
+arm_m_exception_unwind_sniffer (const struct frame_unwind *self,
+				struct frame_info *this_frame,
+				void **this_prologue_cache)
+{
+  CORE_ADDR this_pc = get_frame_pc (this_frame);
+
+  /* No need to check is_m; this sniffer is only registered for
+     M-profile architectures.  */
+
+  /* Exception frames return to one of these magic PCs.  Other values
+     are not defined as of v7-M.  See details in "B1.5.8 Exception return
+     behavior" in "ARMv7-M Architecture Reference Manual".  */
+  if (this_pc == 0xfffffff1 || this_pc == 0xfffffff9 || this_pc == 0xfffffffd)
+    return 1;
+
+  return 0;
+}
+
+/* Frame unwinder for M-profile exceptions.  */
+
+struct frame_unwind arm_m_exception_unwind = {
+  SIGTRAMP_FRAME,
+  default_frame_unwind_stop_reason,
+  arm_m_exception_this_id,
+  arm_m_exception_prev_register,
+  NULL,
+  arm_m_exception_unwind_sniffer
+};
+
 static CORE_ADDR
 arm_normal_frame_base (struct frame_info *this_frame, void **this_cache)
 {
@@ -10218,6 +10338,8 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   dwarf2_frame_set_init_reg (gdbarch, arm_dwarf2_frame_init_reg);
 
   /* Add some default predicates.  */
+  if (is_m)
+    frame_unwind_append_unwinder (gdbarch, &arm_m_exception_unwind);
   frame_unwind_append_unwinder (gdbarch, &arm_stub_unwind);
   dwarf2_append_unwinders (gdbarch);
   frame_unwind_append_unwinder (gdbarch, &arm_exidx_unwind);
-- 
1.7.7.6


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

* Re: [PATCH/arm] Backtrace through exception frames on arm/cortex-m target
  2012-11-14  2:23   ` Yao Qi
@ 2012-11-14  2:57     ` Joel Brobecker
  2012-11-14  3:48       ` Yao Qi
  0 siblings, 1 reply; 8+ messages in thread
From: Joel Brobecker @ 2012-11-14  2:57 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

> Sure.  We didn't add comments to these functions because they are
> installed to 'struct frame_unwind' to compose a unwinder for a specific
> type of frames.  The situation is similar to gdbarch hook functions, so
> I add comment in the similar way, for example,

Thank you. I know the comment is slightly superfluous, but a comment
like the one you added is good to confirm what it's for, and where
to go looking for the function's general documentation. And one of
the reasons why I usually insist on documenting every function is
because it is just simpler to say "document everything" than to say
"document everything except ...".

> 2012-11-14  Daniel Jacobowitz  <dan@codesourcery.com>
> 	    Yao Qi  <yao@codesourcery.com>
> 
> 	* arm-tdep.c (arm_addr_bits_remove): Do not adjust the low
> 	bit of EXC_RETURN.
> 	(arm_m_exception_cache, arm_m_exception_this_id)
> 	(arm_m_exception_prev_register, arm_m_exception_unwind_sniffer)
> 	(arm_m_exception_unwind): New.
> 	(arm_gdbarch_init): Register arm_m_exception_unwind.

The patch looks fine to me, except for one minor formatting nit.
Pre-appoved with that change.

Also, a small reminder that the soft limit for line length is 70
chars, with a hard limit of 80 per an earlier discussion on this list.
Some of the comments are a little wide, in that respect, but nothing
significant enough to worry about. Just something to keep in mind
for the future, if you don't mind.

> +  /* On M-profile devices, do not strip the low bit from EXC_RETURN
> +     (the magic exception return address).  */

Question from an arm novice: Why? (and would it be something useful
to add to the comment as well?

> +struct frame_unwind arm_m_exception_unwind = {

Can you put the opening curly brace on the next line? We're a little
inconsistent regarding this, but I believe that this is the style
that we should be using.

Thanks,
-- 
Joel


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

* Re: [PATCH/arm] Backtrace through exception frames on arm/cortex-m target
  2012-11-14  2:57     ` Joel Brobecker
@ 2012-11-14  3:48       ` Yao Qi
  0 siblings, 0 replies; 8+ messages in thread
From: Yao Qi @ 2012-11-14  3:48 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches

On 11/14/2012 10:57 AM, Joel Brobecker wrote:
> Thank you. I know the comment is slightly superfluous, but a comment
> like the one you added is good to confirm what it's for, and where
> to go looking for the function's general documentation. And one of
> the reasons why I usually insist on documenting every function is
> because it is just simpler to say "document everything" than to say
> "document everything except ...".
>

OK, that is reasonable to me.

>> >2012-11-14  Daniel Jacobowitz<dan@codesourcery.com>
>> >	    Yao Qi<yao@codesourcery.com>
>> >
>> >	* arm-tdep.c (arm_addr_bits_remove): Do not adjust the low
>> >	bit of EXC_RETURN.
>> >	(arm_m_exception_cache, arm_m_exception_this_id)
>> >	(arm_m_exception_prev_register, arm_m_exception_unwind_sniffer)
>> >	(arm_m_exception_unwind): New.
>> >	(arm_gdbarch_init): Register arm_m_exception_unwind.
> The patch looks fine to me, except for one minor formatting nit.
> Pre-appoved with that change.
>

Thanks.

> Also, a small reminder that the soft limit for line length is 70
> chars, with a hard limit of 80 per an earlier discussion on this list.
> Some of the comments are a little wide, in that respect, but nothing
> significant enough to worry about. Just something to keep in mind
> for the future, if you don't mind.
>

OK, no problem.  I've updated my ~/.emacs to set the max length per line 
is 70, and emacs will complain if it exceeds 70 next time.

>> >+  /* On M-profile devices, do not strip the low bit from EXC_RETURN
>> >+     (the magic exception return address).  */
> Question from an arm novice: Why? (and would it be something useful
> to add to the comment as well?
>

The last two bits or one bit are set and cleared for some purpose (e.g., 
they are used for mode switch, arm <->thumb).  Function 
arm_addr_bits_remove is to clear the last two or one bits of address, 
and this patch is avoid doing so to keep these magic address unchanged.

>> >+struct frame_unwind arm_m_exception_unwind = {
> Can you put the opening curly brace on the next line? We're a little
> inconsistent regarding this, but I believe that this is the style
> that we should be using.

OK, no problem.  I am seeing both styles (opening curly brace in 
new-line vs. same-line), and I'll find somewhere to document this rule.

Below is what I committed.

-- 
Yao (齐尧)

gdb:

2012-11-14  Daniel Jacobowitz  <dan@codesourcery.com>
	    Yao Qi  <yao@codesourcery.com>

	* arm-tdep.c (arm_addr_bits_remove): Do not adjust the low
	bit of EXC_RETURN.
	(arm_m_exception_cache, arm_m_exception_this_id)
	(arm_m_exception_prev_register, arm_m_exception_unwind_sniffer)
	(arm_m_exception_unwind): New.
	(arm_gdbarch_init): Register arm_m_exception_unwind.
---
  gdb/arm-tdep.c |  129 
++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 files changed, 129 insertions(+), 0 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 1a67366..a221fd6 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -448,6 +448,12 @@ arm_pc_is_thumb (struct gdbarch *gdbarch, CORE_ADDR 
memaddr)
  static CORE_ADDR
  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 (gdbarch_tdep (gdbarch)->is_m
+      && (val & 0xfffffff0) == 0xfffffff0)
+    return val;
+
    if (arm_apcs_32)
      return UNMAKE_THUMB_ADDR (val);
    else
@@ -2926,6 +2932,127 @@ struct frame_unwind arm_stub_unwind = {
    arm_stub_unwind_sniffer
  };

+/* Put here the code to store, into CACHE->saved_regs, the addresses
+   of the saved registers of frame described by THIS_FRAME.  CACHE is
+   returned.  */
+
+static struct arm_prologue_cache *
+arm_m_exception_cache (struct frame_info *this_frame)
+{
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+  struct arm_prologue_cache *cache;
+  CORE_ADDR unwound_sp;
+  LONGEST xpsr;
+
+  cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
+  cache->saved_regs = trad_frame_alloc_saved_regs (this_frame);
+
+  unwound_sp = get_frame_register_unsigned (this_frame,
+					    ARM_SP_REGNUM);
+
+  /* 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].addr = unwound_sp;
+  cache->saved_regs[1].addr = unwound_sp + 4;
+  cache->saved_regs[2].addr = unwound_sp + 8;
+  cache->saved_regs[3].addr = unwound_sp + 12;
+  cache->saved_regs[12].addr = unwound_sp + 16;
+  cache->saved_regs[14].addr = unwound_sp + 20;
+  cache->saved_regs[15].addr = unwound_sp + 24;
+  cache->saved_regs[ARM_PS_REGNUM].addr = unwound_sp + 28;
+
+  /* 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.  */
+  cache->prev_sp = unwound_sp + 32;
+  if (safe_read_memory_integer (unwound_sp + 28, 4, byte_order, &xpsr)
+      && (xpsr & (1 << 9)) != 0)
+    cache->prev_sp += 4;
+
+  return cache;
+}
+
+/* Implementation of function hook 'this_id' in
+   'struct frame_uwnind'.  */
+
+static void
+arm_m_exception_this_id (struct frame_info *this_frame,
+			 void **this_cache,
+			 struct frame_id *this_id)
+{
+  struct arm_prologue_cache *cache;
+
+  if (*this_cache == NULL)
+    *this_cache = arm_m_exception_cache (this_frame);
+  cache = *this_cache;
+
+  /* Our frame ID for a stub frame is the current SP and LR.  */
+  *this_id = frame_id_build (cache->prev_sp,
+			     get_frame_pc (this_frame));
+}
+
+/* Implementation of function hook 'prev_register' in
+   'struct frame_uwnind'.  */
+
+static struct value *
+arm_m_exception_prev_register (struct frame_info *this_frame,
+			       void **this_cache,
+			       int prev_regnum)
+{
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  struct arm_prologue_cache *cache;
+
+  if (*this_cache == NULL)
+    *this_cache = arm_m_exception_cache (this_frame);
+  cache = *this_cache;
+
+  /* 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 trad_frame_get_prev_register (this_frame, cache->saved_regs,
+				       prev_regnum);
+}
+
+/* Implementation of function hook 'sniffer' in
+   'struct frame_uwnind'.  */
+
+static int
+arm_m_exception_unwind_sniffer (const struct frame_unwind *self,
+				struct frame_info *this_frame,
+				void **this_prologue_cache)
+{
+  CORE_ADDR this_pc = get_frame_pc (this_frame);
+
+  /* No need to check is_m; this sniffer is only registered for
+     M-profile architectures.  */
+
+  /* Exception frames return to one of these magic PCs.  Other values
+     are not defined as of v7-M.  See details in "B1.5.8 Exception
+     return behavior" in "ARMv7-M Architecture Reference Manual".  */
+  if (this_pc == 0xfffffff1 || this_pc == 0xfffffff9
+      || this_pc == 0xfffffffd)
+    return 1;
+
+  return 0;
+}
+
+/* Frame unwinder for M-profile exceptions.  */
+
+struct frame_unwind arm_m_exception_unwind =
+{
+  SIGTRAMP_FRAME,
+  default_frame_unwind_stop_reason,
+  arm_m_exception_this_id,
+  arm_m_exception_prev_register,
+  NULL,
+  arm_m_exception_unwind_sniffer
+};
+
  static CORE_ADDR
  arm_normal_frame_base (struct frame_info *this_frame, void **this_cache)
  {
@@ -10218,6 +10345,8 @@ arm_gdbarch_init (struct gdbarch_info info, 
struct gdbarch_list *arches)
    dwarf2_frame_set_init_reg (gdbarch, arm_dwarf2_frame_init_reg);

    /* Add some default predicates.  */
+  if (is_m)
+    frame_unwind_append_unwinder (gdbarch, &arm_m_exception_unwind);
    frame_unwind_append_unwinder (gdbarch, &arm_stub_unwind);
    dwarf2_append_unwinders (gdbarch);
    frame_unwind_append_unwinder (gdbarch, &arm_exidx_unwind);
-- 
1.7.7.6


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

end of thread, other threads:[~2012-11-14  3:48 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-13 14:16 [PATCH/arm] Backtrace through exception frames on arm/cortex-m target Yao Qi
2012-11-13 14:27 ` Mark Kettenis
2012-11-13 14:44   ` Yao Qi
2012-11-13 15:12     ` Mark Kettenis
2012-11-13 15:50 ` Joel Brobecker
2012-11-14  2:23   ` Yao Qi
2012-11-14  2:57     ` Joel Brobecker
2012-11-14  3:48       ` Yao Qi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox