Re: [PATCH] Fix exception unwinding for ARM Cortex-M

Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[James-Adam Renquinha Henri]

Hi everyone in this thread, (not many as I see)

We could take another shot at integrating some changes to GDB so it 
works correctly, but that may only happen if there are the least amount 
of useless bike-shedding and that the right people in the mailing list 
contribute.

Basically, we must 1) make *very* clear the fact that Cortex-M devices 
are used typically in a bare-metal system with no OS (read: Linux) 
whatsoever, only possibly a tiny scheduler and that's all. That means we 
don't have to be concerned about "user visible" registers and whatnot, 
they are all visible anyway, and if the spec says that a register 
exists, *it is visible*, period. Other thing though, 2) my patch 
achieved its goal in a rather weaselly way, and the correct way is 
indeed to have it integrated into target features. But I need more info 
on the matter, and I'm a bit clueless about what to do with those 
"target features", how they are handled by GDB and how I can test the 
new configuration in isolation, without being "altered" by e.g. OpenOCD, 
which handily provides a target description that I can use as I did before.

I sure can figure it out all by myself, but that might be only after 
pouring an rather big amount of time into the matter that I cannot 
afford to do now. I need active collaboration and answers.

Here, let that message be in the mailing list for everyone to see.

James-Adam Renquinha Henri, Ing. jr
Ingénieur d'application
CIMEQ INC.


On 19-06-02 03 h 31, Fredrik Hederstierna wrote:
> Hi Yao, Adam,
> 
> 
> Some years back there was done some work done on unwinding on Cortex-M.
> 
> Only the very first start of the chain of patches was completed (new 
> function arm_m_pc_is_magic()).
> 
> 
> The actual real patch work with code work by me/Adam for cortex-M4F 
> MSP/PSP and unwinding did never reached the repo.
> 
> 
> What do you think of making another attempt to fix the stuff, I guess 
> all paper-work etc is in place, so we can try do a retake on this?
> 
> 
> Thanks, Best Regards, Fredrik
> 
> ------------------------------------------------------------------------
> *From:* Yao Qi <qiyaoltc@gmail.com>
> *Sent:* Tuesday, September 27, 2016 3:38 AM
> *To:* Adam Renquinha
> *Cc:* Fredrik Hederstierna; gdb-patches@sourceware.org
> *Subject:* Re: [PATCH] Fix exception unwinding for ARM Cortex-M
> On Mon, Sep 26, 2016 at 3:26 PM, Adam Renquinha <arenquinha@cimeq.qc.ca> 
> wrote:
>> That looks correct.
>>
> 
> Patch is pushed into master and 7.12.  The rest of your original
> patch is still welcome!
> 
> -- 
> Yao (齐尧)

Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[Alan Hayward]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3369 bytes --]

Not much development happens on the Arm port of GDB these days, so getting unwinding support in would be useful and much appreciated.  I have on my backlog a plan to switch the Arm port to the newer way of doing target descriptions - I suspect that might help your patches.

I’m happy to take a look at what you had. To save time and to make things easier, for the remaining patches that are still outstanding could you please either repost them or provide some links to sourceware.org/ml/gdb-patches/ where they were posted. Also, so as not to just repeat the original discussions, what were the outstanding objections with them?

Alan.

> On 10 Jun 2019, at 22:24, James-Adam Renquinha Henri <arenquinha@cimeq.qc.ca> wrote:
> 
> Hi everyone in this thread, (not many as I see)
> 
> We could take another shot at integrating some changes to GDB so it works correctly, but that may only happen if there are the least amount of useless bike-shedding and that the right people in the mailing list contribute.
> 
> Basically, we must 1) make *very* clear the fact that Cortex-M devices are used typically in a bare-metal system with no OS (read: Linux) whatsoever, only possibly a tiny scheduler and that's all. That means we don't have to be concerned about "user visible" registers and whatnot, they are all visible anyway, and if the spec says that a register exists, *it is visible*, period. Other thing though, 2) my patch achieved its goal in a rather weaselly way, and the correct way is indeed to have it integrated into target features. But I need more info on the matter, and I'm a bit clueless about what to do with those "target features", how they are handled by GDB and how I can test the new configuration in isolation, without being "altered" by e.g. OpenOCD, which handily provides a target description that I can use as I did before.
> 
> I sure can figure it out all by myself, but that might be only after pouring an rather big amount of time into the matter that I cannot afford to do now. I need active collaboration and answers.
> 
> Here, let that message be in the mailing list for everyone to see.
> 
> James-Adam Renquinha Henri, Ing. jr
> Ingénieur d'application
> CIMEQ INC.
> 
> 
> On 19-06-02 03 h 31, Fredrik Hederstierna wrote:
>> Hi Yao, Adam,
>> Some years back there was done some work done on unwinding on Cortex-M.
>> Only the very first start of the chain of patches was completed (new function arm_m_pc_is_magic()).
>> The actual real patch work with code work by me/Adam for cortex-M4F MSP/PSP and unwinding did never reached the repo.
>> What do you think of making another attempt to fix the stuff, I guess all paper-work etc is in place, so we can try do a retake on this?
>> Thanks, Best Regards, Fredrik
>> ------------------------------------------------------------------------
>> *From:* Yao Qi <qiyaoltc@gmail.com>
>> *Sent:* Tuesday, September 27, 2016 3:38 AM
>> *To:* Adam Renquinha
>> *Cc:* Fredrik Hederstierna; gdb-patches@sourceware.org
>> *Subject:* Re: [PATCH] Fix exception unwinding for ARM Cortex-M
>> On Mon, Sep 26, 2016 at 3:26 PM, Adam Renquinha <arenquinha@cimeq.qc.ca> wrote:
>>> That looks correct.
>>> 
>> Patch is pushed into master and 7.12.  The rest of your original
>> patch is still welcome!
>> -- 
>> Yao (齐尧)

\x16º&Öéj×!zÊÞ¶êçמ»çIb²Ö«r\x18\x1dn–­r\x17¬

[PATCH] Fix exception stack unwinding for ARM Cortex-M

[Fredrik Hederstierna]

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

For Cortex-M targets using floating-point, eg the Cortex-M4F, its not possible to get any call-stack backtrace if setting a breakpoint in ISR.

The exception stack unwinder for Cortex-M does not consider if floating-point registers was stacked or not,
further the Cortex-M has two stack pointers: MSP (Main Stack Pointer) and PSP (Process Stack Pointer).
This is not handled when GDB tries to backtrace in the exception stack unwinder.

This patch fixes this, and gives a correct call-stack backtrace from breakpoints set in a handler or ISR.

Best Regards,
Fredrik Hederstierna

Senior Software Developer
Verisure Innovation Centre
Malmoe Sweden

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gdb-cortex-m-exception-unwind-fix.patch --]
[-- Type: text/x-patch; name="gdb-cortex-m-exception-unwind-fix.patch", Size: 5233 bytes --]

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1ff47c3355..1d80e8cfc8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2020-08-29  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
+	    Adam Renquinha <arenquinha@cimeq.qc.ca>
+
+	* arm-tdep.c (arm_m_exception_cache): Try use correct stack
+	pointer and stack frame offset when unwinding.
+
 2020-08-29  Pedro Alves  <pedro@palves.net>
 
 	* progspace.c (print_program_space): Use all_inferiors.  Switch to
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 074eedb480..ed7d4b1d37 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -2923,14 +2923,59 @@ 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 lr;
+  CORE_ADDR sp;
   CORE_ADDR unwound_sp;
   LONGEST xpsr;
+  uint32_t main_stack_used;
+  uint32_t extended_frame_used;
 
   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);
+  /* ARMv7-M Architecture Reference "B1.5.6 Exception entry behavior"
+     describes which bits in LR that define which stack was used prior
+     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 if main stack was used.  */
+  main_stack_used = ((lr & 0xf) != 0xd);
+  if (main_stack_used)
+    {
+      /* Main stack used, use MSP as SP.  */
+      unwound_sp = sp;
+    }
+  else
+    {
+      /* 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, exiting."));
+
+          /* 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);
+        }
+    }
 
   /* The hardware saves eight 32-bit words, comprising xPSR,
      ReturnAddress, LR (R14), R12, R3, R2, R1, R0.  See details in
@@ -2940,15 +2985,47 @@ arm_m_exception_cache (struct frame_info *this_frame)
   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_IP_REGNUM].addr = unwound_sp + 16;
+  cache->saved_regs[ARM_LR_REGNUM].addr = unwound_sp + 20;
+  cache->saved_regs[ARM_PC_REGNUM].addr = unwound_sp + 24;
   cache->saved_regs[ARM_PS_REGNUM].addr = unwound_sp + 28;
 
+  /* Check if extended stack frame (FPU regs stored) was used.  */
+  extended_frame_used = ((lr & (1 << 4)) == 0);
+  if (extended_frame_used)
+    {
+      int i;
+      int fpu_regs_stack_offset;
+
+      /* This code does not take into account the lazy stacking, see "Lazy
+         context save of FP state", in B1.5.7, also ARM AN298, supported
+         by Cortex-M4F architecture. Give a warning and try do best effort.
+         To fully handle this the FPCCR register (Floating-point Context
+         Control Register) needs to be read out and the bits ASPEN and LSPEN
+         could be checked to setup correct lazy stacked FP registers.  */
+
+      warning (_("no FPU lazy stack unwinding supported, check FPCCR."));
+
+      fpu_regs_stack_offset = unwound_sp + 0x20;
+      for (i = 0; i < 16; i++)
+        {
+          cache->saved_regs[ARM_D0_REGNUM + i].addr = fpu_regs_stack_offset;
+          fpu_regs_stack_offset += 4;
+        }
+      cache->saved_regs[ARM_FPSCR_REGNUM].addr = unwound_sp + 0x60;
+
+      /* Offset 0x64 is reserved.  */
+      cache->prev_sp = unwound_sp + 0x68;
+    }
+  else
+    {
+      /* Basic frame type used.  */
+      cache->prev_sp = unwound_sp + 32;
+    }
+
   /* 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;

Re: [PATCH] Fix exception stack unwinding for ARM Cortex-M

[Alan Hayward]

> On 29 Aug 2020, at 09:35, Fredrik Hederstierna <fredrik.hederstierna@verisure.com> wrote:
> 
> For Cortex-M targets using floating-point, eg the Cortex-M4F, its not possible to get any call-stack backtrace if setting a breakpoint in ISR.
> 
> The exception stack unwinder for Cortex-M does not consider if floating-point registers was stacked or not,
> further the Cortex-M has two stack pointers: MSP (Main Stack Pointer) and PSP (Process Stack Pointer).
> This is not handled when GDB tries to backtrace in the exception stack unwinder.
> 
> This patch fixes this, and gives a correct call-stack backtrace from breakpoints set in a handler or ISR.

Thanks for doing this fix. This mostly looks fine, but comments inlined below.

How easy is it to compile a binary that exhibits this behaviour? If so then a
test in testsuite/gdb.arch/ would be nice. For reference, aarch64-sighandler-regs.exp
is a similar test but for AArch64.

> 
> Best Regards,
> Fredrik Hederstierna

Have you signed the copyright assignment?
https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment

> 
> Senior Software Developer
> Verisure Innovation Centre
> Malmoe Sweden
> <gdb-cortex-m-exception-unwind-fix.patch>


> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 1ff47c3355..1d80e8cfc8 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2020-08-29  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
> +	    Adam Renquinha <arenquinha@cimeq.qc.ca>
> +
> +	* arm-tdep.c (arm_m_exception_cache): Try use correct stack
> +	pointer and stack frame offset when unwinding.
> +

Ideally this part should be left separate from the patch as to prevent
merge issues.

>  2020-08-29  Pedro Alves  <pedro@palves.net>
>
>  	* progspace.c (print_program_space): Use all_inferiors.  Switch to
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 074eedb480..ed7d4b1d37 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -2923,14 +2923,59 @@ 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 lr;
> +  CORE_ADDR sp;
>    CORE_ADDR unwound_sp;
>    LONGEST xpsr;
> +  uint32_t main_stack_used;
> +  uint32_t extended_frame_used;
>
>    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);
> +  /* ARMv7-M Architecture Reference "B1.5.6 Exception entry behavior"
> +     describes which bits in LR that define which stack was used prior
> +     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 if main stack was used.  */
> +  main_stack_used = ((lr & 0xf) != 0xd);

This took me a while to confirm. Could you mention that you are checking for
SPSEL in the comment. Also, I wonder if it’s worth checking the other bits in lr.
Yes they should be all ones in either case. But I’d rather be a little cautious.
Only go into the else case if all the bits are correct.

> +  if (main_stack_used)
> +    {
> +      /* Main stack used, use MSP as SP.  */
> +      unwound_sp = sp;
> +    }
> +  else
> +    {
> +      /* 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, exiting."));

I don’t think you mean exit. Maybe just remove “exiting” from the string.

> +
> +          /* 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);
> +        }
> +    }
>
>    /* The hardware saves eight 32-bit words, comprising xPSR,
>       ReturnAddress, LR (R14), R12, R3, R2, R1, R0.  See details in
> @@ -2940,15 +2985,47 @@ arm_m_exception_cache (struct frame_info *this_frame)
>    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_IP_REGNUM].addr = unwound_sp + 16;
> +  cache->saved_regs[ARM_LR_REGNUM].addr = unwound_sp + 20;
> +  cache->saved_regs[ARM_PC_REGNUM].addr = unwound_sp + 24;
>    cache->saved_regs[ARM_PS_REGNUM].addr = unwound_sp + 28;

Thanks for switching this to use the enums.

>
> +  /* Check if extended stack frame (FPU regs stored) was used.  */
> +  extended_frame_used = ((lr & (1 << 4)) == 0);
> +  if (extended_frame_used)
> +    {
> +      int i;
> +      int fpu_regs_stack_offset;
> +
> +      /* This code does not take into account the lazy stacking, see "Lazy
> +         context save of FP state", in B1.5.7, also ARM AN298, supported
> +         by Cortex-M4F architecture. Give a warning and try do best effort.
> +         To fully handle this the FPCCR register (Floating-point Context
> +         Control Register) needs to be read out and the bits ASPEN and LSPEN
> +         could be checked to setup correct lazy stacked FP registers.  */
> +
> +      warning (_("no FPU lazy stack unwinding supported, check FPCCR."));

This means that we will always get a warning if the extended frame is used.
I’d rather that didn’t happen.
How easy would be be to check the FPCCR register and then give a warning only if
lazy stacking is being used?

> +
> +      fpu_regs_stack_offset = unwound_sp + 0x20;
> +      for (i = 0; i < 16; i++)
> +        {
> +          cache->saved_regs[ARM_D0_REGNUM + i].addr = fpu_regs_stack_offset;
> +          fpu_regs_stack_offset += 4;
> +        }
> +      cache->saved_regs[ARM_FPSCR_REGNUM].addr = unwound_sp + 0x60;
> +
> +      /* Offset 0x64 is reserved.  */
> +      cache->prev_sp = unwound_sp + 0x68;
> +    }
> +  else
> +    {
> +      /* Basic frame type used.  */
> +      cache->prev_sp = unwound_sp + 32;

The mix of hex and decimal in the function is a little glaring.
Could you switch this one to 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.  */
> -  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;



Thanks,
Alan.

Re: [PATCH] Fix exception stack unwinding for ARM Cortex-M

[Fredrik Hederstierna]

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

Hi,
I updated that patch to address your comments, see below and attached patch take2

> From: Alan Hayward <Alan.Hayward@arm.com>
> Sent: Wednesday, September 2, 2020 3:24 PM
> To: Fredrik Hederstierna <fredrik.hederstierna@verisure.com>

> How easy is it to compile a binary that exhibits this behaviour? If so then a
> test in testsuite/gdb.arch/ would be nice. For reference, aarch64-sighandler-regs.exp
> is a similar test but for AArch64.

I have not had time to further look into this, its probably possible to add such a test case, but I have no possibility to do this currently unfortunately.

> Have you signed the copyright assignment?

Yes, to my understanding everything is clear.

>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 1ff47c3355..1d80e8cfc8 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,9 @@
>> +2020-08-29  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
>> +         Adam Renquinha <arenquinha@cimeq.qc.ca>
>> +
>> +     * arm-tdep.c (arm_m_exception_cache): Try use correct stack
>> +     pointer and stack frame offset when unwinding.
>> +
>
> Ideally this part should be left separate from the patch as to prevent
> merge issues.

Ok, removed from patch.

>> +  /* Check if main stack was used.  */
>> +  main_stack_used = ((lr & 0xf) != 0xd);
>
> This took me a while to confirm. Could you mention that you are checking for
> SPSEL in the comment. Also, I wonder if it’s worth checking the other bits in lr.
> Yes they should be all ones in either case. But I’d rather be a little cautious.
> Only go into the else case if all the bits are correct.

Ok, added more clear comments and more strict bit checking.

>> +          /* Thread (process) stack could not be fetched,
>> +             give warning and exit.  */
>> +
>> +          warning (_("no PSP thread stack unwinding supported, exiting."));
>
> I don’t think you mean exit. Maybe just remove “exiting” from the string.

Ok, removed 'exiting'

>> +      /* This code does not take into account the lazy stacking, see "Lazy
>> +         context save of FP state", in B1.5.7, also ARM AN298, supported
>> +         by Cortex-M4F architecture. Give a warning and try do best effort.
>> +         To fully handle this the FPCCR register (Floating-point Context
>> +         Control Register) needs to be read out and the bits ASPEN and LSPEN
>> +         could be checked to setup correct lazy stacked FP registers.  */
>> +
>> +      warning (_("no FPU lazy stack unwinding supported, check FPCCR."));
>
> This means that we will always get a warning if the extended frame is used.
> I’d rather that didn’t happen.
> How easy would be be to check the FPCCR register and then give a warning only if
> lazy stacking is being used?

Maybe its possible, but have to time to solve this currently, added memory address of FPCCR,
its not a register, but probably possible to do memory reading to dig deeper into this.
Removed warning.

>> +      /* Basic frame type used.  */
>> +      cache->prev_sp = unwound_sp + 32;
>
> The mix of hex and decimal in the function is a little glaring.
> Could you switch this one to 0x20.

Ok, fixed.


Here is ChangeLog, separated, new patch variant attached.

2020-09-06  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
            Adam Renquinha <arenquinha@cimeq.qc.ca>

        * arm-tdep.c (arm_m_exception_cache): Try use correct stack
        pointer and stack frame offset when unwinding.


BR Fredrik

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gdb-cortex-m-exception-unwind-fix2.patch --]
[-- Type: text/x-patch; name="gdb-cortex-m-exception-unwind-fix2.patch", Size: 7024 bytes --]

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 074eedb480..1a5017495a 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -469,7 +469,7 @@ arm_pc_is_thumb (struct gdbarch *gdbarch, CORE_ADDR memaddr)
 }
 
 /* Determine if the address specified equals any of these magic return
-   values, called EXC_RETURN, defined by the ARM v6-M and v7-M
+   values, called EXC_RETURN, defined by the ARM v6-M, v7-M and v8-M
    architectures.
 
    From ARMv6-M Reference Manual B1.5.8
@@ -500,13 +500,25 @@ arm_pc_is_thumb (struct gdbarch *gdbarch, CORE_ADDR memaddr)
    0xFFFFFFFD    Thread mode      Process         Basic
 
    For more details see "B1.5.8 Exception return behavior"
-   in both ARMv6-M and ARMv7-M Architecture Reference Manuals.  */
+   in both ARMv6-M and ARMv7-M Architecture Reference Manuals.
+
+   In the ARMv8-M Architecture Technical Reference also adds
+   for implementations without the Security Extension:
+
+   EXC_RETURN    Condition
+   0xFFFFFFB0    Return to Handler mode.
+   0xFFFFFFB8    Return to Thread mode using the main stack.
+   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:
@@ -2923,14 +2935,64 @@ 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 lr;
+  CORE_ADDR sp;
   CORE_ADDR unwound_sp;
   LONGEST xpsr;
+  uint32_t exc_return;
+  uint32_t process_stack_used;
+  uint32_t extended_frame_used;
+  uint32_t secure_stack_used;
 
   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);
+  /* ARMv7-M Architecture Reference "B1.5.6 Exception entry behavior"
+     describes which bits in LR that define which stack was used prior
+     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);
+
+  /* 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)
+    {
+      /* 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);
+        }
+    }
+  else
+    {
+      /* Main stack used, use MSP as SP.  */
+      unwound_sp = sp;
+    }
 
   /* The hardware saves eight 32-bit words, comprising xPSR,
      ReturnAddress, LR (R14), R12, R3, R2, R1, R0.  See details in
@@ -2940,15 +3002,62 @@ arm_m_exception_cache (struct frame_info *this_frame)
   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_IP_REGNUM].addr = unwound_sp + 16;
+  cache->saved_regs[ARM_LR_REGNUM].addr = unwound_sp + 20;
+  cache->saved_regs[ARM_PC_REGNUM].addr = unwound_sp + 24;
   cache->saved_regs[ARM_PS_REGNUM].addr = unwound_sp + 28;
 
+  /* Check EXC_RETURN bit FTYPE if extended stack frame (FPU regs stored)
+     type used.  */
+  extended_frame_used = ((lr & (1 << 4)) == 0);
+  if (exc_return && extended_frame_used)
+    {
+      int i;
+      int fpu_regs_stack_offset;
+
+      /* This code does not take into account the lazy stacking, see "Lazy
+         context save of FP state", in B1.5.7, also ARM AN298, supported
+         by Cortex-M4F architecture.
+         To fully handle this the FPCCR register (Floating-point Context
+         Control Register) needs to be read out and the bits ASPEN and LSPEN
+         could be checked to setup correct lazy stacked FP registers.
+         This register is located at address 0xE000EF34.  */
+
+      /* Extended stack frame type used.  */
+      fpu_regs_stack_offset = unwound_sp + 0x20;
+      for (i = 0; i < 16; i++)
+        {
+          cache->saved_regs[ARM_D0_REGNUM + i].addr = fpu_regs_stack_offset;
+          fpu_regs_stack_offset += 4;
+        }
+      cache->saved_regs[ARM_FPSCR_REGNUM].addr = unwound_sp + 0x60;
+
+      /* Offset 0x64 is reserved.  */
+      cache->prev_sp = unwound_sp + 0x68;
+    }
+  else
+    {
+      /* Standard stack frame type used.  */
+      cache->prev_sp = 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.  */
+    }
+
   /* 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;

Re: [PATCH] Fix exception stack unwinding for ARM Cortex-M

[Alan Hayward]

Ok, everything looks good to me now.

It’s fairly clear in the code where there is still work to be done.
Do you have a bugzilla account? If so, could you please raise two bugs for the two features.
If not, I can add them.

Doesn’t look like you have write access, so I’ll leave this to early next week and if there
have been no other comments then I’ll commit it.

Thanks for the patch!

Alan.


> On 6 Sep 2020, at 10:27, Fredrik Hederstierna <fredrik.hederstierna@verisure.com> wrote:
> 
> Hi,
> I updated that patch to address your comments, see below and attached patch take2
> 
>> From: Alan Hayward <Alan.Hayward@arm.com>
>> Sent: Wednesday, September 2, 2020 3:24 PM
>> To: Fredrik Hederstierna <fredrik.hederstierna@verisure.com>
> 
>> How easy is it to compile a binary that exhibits this behaviour? If so then a
>> test in testsuite/gdb.arch/ would be nice. For reference, aarch64-sighandler-regs.exp
>> is a similar test but for AArch64.
> 
> I have not had time to further look into this, its probably possible to add such a test case, but I have no possibility to do this currently unfortunately.
> 
>> Have you signed the copyright assignment?
> 
> Yes, to my understanding everything is clear.
> 
>>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>>> index 1ff47c3355..1d80e8cfc8 100644
>>> --- a/gdb/ChangeLog
>>> +++ b/gdb/ChangeLog
>>> @@ -1,3 +1,9 @@
>>> +2020-08-29  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
>>> +         Adam Renquinha <arenquinha@cimeq.qc.ca>
>>> +
>>> +     * arm-tdep.c (arm_m_exception_cache): Try use correct stack
>>> +     pointer and stack frame offset when unwinding.
>>> +
>> 
>> Ideally this part should be left separate from the patch as to prevent
>> merge issues.
> 
> Ok, removed from patch.
> 
>>> +  /* Check if main stack was used.  */
>>> +  main_stack_used = ((lr & 0xf) != 0xd);
>> 
>> This took me a while to confirm. Could you mention that you are checking for
>> SPSEL in the comment. Also, I wonder if it’s worth checking the other bits in lr.
>> Yes they should be all ones in either case. But I’d rather be a little cautious.
>> Only go into the else case if all the bits are correct.
> 
> Ok, added more clear comments and more strict bit checking.
> 
>>> +          /* Thread (process) stack could not be fetched,
>>> +             give warning and exit.  */
>>> +
>>> +          warning (_("no PSP thread stack unwinding supported, exiting."));
>> 
>> I don’t think you mean exit. Maybe just remove “exiting” from the string.
> 
> Ok, removed 'exiting'
> 
>>> +      /* This code does not take into account the lazy stacking, see "Lazy
>>> +         context save of FP state", in B1.5.7, also ARM AN298, supported
>>> +         by Cortex-M4F architecture. Give a warning and try do best effort.
>>> +         To fully handle this the FPCCR register (Floating-point Context
>>> +         Control Register) needs to be read out and the bits ASPEN and LSPEN
>>> +         could be checked to setup correct lazy stacked FP registers.  */
>>> +
>>> +      warning (_("no FPU lazy stack unwinding supported, check FPCCR."));
>> 
>> This means that we will always get a warning if the extended frame is used.
>> I’d rather that didn’t happen.
>> How easy would be be to check the FPCCR register and then give a warning only if
>> lazy stacking is being used?
> 
> Maybe its possible, but have to time to solve this currently, added memory address of FPCCR,
> its not a register, but probably possible to do memory reading to dig deeper into this.
> Removed warning.
> 
>>> +      /* Basic frame type used.  */
>>> +      cache->prev_sp = unwound_sp + 32;
>> 
>> The mix of hex and decimal in the function is a little glaring.
>> Could you switch this one to 0x20.
> 
> Ok, fixed.
> 
> 
> Here is ChangeLog, separated, new patch variant attached.
> 
> 2020-09-06  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
>            Adam Renquinha <arenquinha@cimeq.qc.ca>
> 
>        * arm-tdep.c (arm_m_exception_cache): Try use correct stack
>        pointer and stack frame offset when unwinding.
> 
> 
> BR Fredrik
> <gdb-cortex-m-exception-unwind-fix2.patch>

Re: [PATCH] Fix exception stack unwinding for ARM Cortex-M

[Fredrik Hederstierna]

Hi Alan,
sounds great! thank for committing the patch,
I do not have any GDB Bugzilla account, so please submit bugs for the additional features.
It would be great if patch goes in before the GDB 10 branching,
thanks! Best Regards, Fredrik
________________________________
From: Alan Hayward <Alan.Hayward@arm.com>
Sent: Wednesday, September 9, 2020 10:12 AM
To: Fredrik Hederstierna <fredrik.hederstierna@verisure.com>
Cc: gdb-patches\@sourceware.org <gdb-patches@sourceware.org>; nd <nd@arm.com>; James-Adam Renquinha Henri <arenquinha@cimeq.qc.ca>
Subject: Re: [PATCH] Fix exception stack unwinding for ARM Cortex-M

Ok, everything looks good to me now.

It’s fairly clear in the code where there is still work to be done.
Do you have a bugzilla account? If so, could you please raise two bugs for the two features.
If not, I can add them.

Doesn’t look like you have write access, so I’ll leave this to early next week and if there
have been no other comments then I’ll commit it.

Thanks for the patch!

Alan.


> On 6 Sep 2020, at 10:27, Fredrik Hederstierna <fredrik.hederstierna@verisure.com> wrote:
>
> Hi,
> I updated that patch to address your comments, see below and attached patch take2
>
>> From: Alan Hayward <Alan.Hayward@arm.com>
>> Sent: Wednesday, September 2, 2020 3:24 PM
>> To: Fredrik Hederstierna <fredrik.hederstierna@verisure.com>
>
>> How easy is it to compile a binary that exhibits this behaviour? If so then a
>> test in testsuite/gdb.arch/ would be nice. For reference, aarch64-sighandler-regs.exp
>> is a similar test but for AArch64.
>
> I have not had time to further look into this, its probably possible to add such a test case, but I have no possibility to do this currently unfortunately.
>
>> Have you signed the copyright assignment?
>
> Yes, to my understanding everything is clear.
>
>>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>>> index 1ff47c3355..1d80e8cfc8 100644
>>> --- a/gdb/ChangeLog
>>> +++ b/gdb/ChangeLog
>>> @@ -1,3 +1,9 @@
>>> +2020-08-29  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
>>> +         Adam Renquinha <arenquinha@cimeq.qc.ca>
>>> +
>>> +     * arm-tdep.c (arm_m_exception_cache): Try use correct stack
>>> +     pointer and stack frame offset when unwinding.
>>> +
>>
>> Ideally this part should be left separate from the patch as to prevent
>> merge issues.
>
> Ok, removed from patch.
>
>>> +  /* Check if main stack was used.  */
>>> +  main_stack_used = ((lr & 0xf) != 0xd);
>>
>> This took me a while to confirm. Could you mention that you are checking for
>> SPSEL in the comment. Also, I wonder if it’s worth checking the other bits in lr.
>> Yes they should be all ones in either case. But I’d rather be a little cautious.
>> Only go into the else case if all the bits are correct.
>
> Ok, added more clear comments and more strict bit checking.
>
>>> +          /* Thread (process) stack could not be fetched,
>>> +             give warning and exit.  */
>>> +
>>> +          warning (_("no PSP thread stack unwinding supported, exiting."));
>>
>> I don’t think you mean exit. Maybe just remove “exiting” from the string.
>
> Ok, removed 'exiting'
>
>>> +      /* This code does not take into account the lazy stacking, see "Lazy
>>> +         context save of FP state", in B1.5.7, also ARM AN298, supported
>>> +         by Cortex-M4F architecture. Give a warning and try do best effort.
>>> +         To fully handle this the FPCCR register (Floating-point Context
>>> +         Control Register) needs to be read out and the bits ASPEN and LSPEN
>>> +         could be checked to setup correct lazy stacked FP registers.  */
>>> +
>>> +      warning (_("no FPU lazy stack unwinding supported, check FPCCR."));
>>
>> This means that we will always get a warning if the extended frame is used.
>> I’d rather that didn’t happen.
>> How easy would be be to check the FPCCR register and then give a warning only if
>> lazy stacking is being used?
>
> Maybe its possible, but have to time to solve this currently, added memory address of FPCCR,
> its not a register, but probably possible to do memory reading to dig deeper into this.
> Removed warning.
>
>>> +      /* Basic frame type used.  */
>>> +      cache->prev_sp = unwound_sp + 32;
>>
>> The mix of hex and decimal in the function is a little glaring.
>> Could you switch this one to 0x20.
>
> Ok, fixed.
>
>
> Here is ChangeLog, separated, new patch variant attached.
>
> 2020-09-06  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
>            Adam Renquinha <arenquinha@cimeq.qc.ca>
>
>        * arm-tdep.c (arm_m_exception_cache): Try use correct stack
>        pointer and stack frame offset when unwinding.
>
>
> BR Fredrik
> <gdb-cortex-m-exception-unwind-fix2.patch>

Re: [PATCH] Fix exception stack unwinding for ARM Cortex-M

[Alan Hayward]

> On 10 Sep 2020, at 22:00, Fredrik Hederstierna <fredrik.hederstierna@verisure.com> wrote:
> 
> Hi Alan,
> sounds great! thank for committing the patch,


Committed to head.


> I do not have any GDB Bugzilla account, so please submit bugs for the additional features.

Added:
https://sourceware.org/bugzilla/show_bug.cgi?id=26611
https://sourceware.org/bugzilla/show_bug.cgi?id=26612
https://sourceware.org/bugzilla/show_bug.cgi?id=26613


> It would be great if patch goes in before the GDB 10 branching,


Joel:
Is it ok to pull this patch across to GDB 10? (And is that something you do?)
It’s Arm only, and will only effect programs that are using special stack setups.


Alan.

> thanks! Best Regards, Fredrik
> From: Alan Hayward <Alan.Hayward@arm.com>
> Sent: Wednesday, September 9, 2020 10:12 AM
> To: Fredrik Hederstierna <fredrik.hederstierna@verisure.com>
> Cc: gdb-patches\@sourceware.org <gdb-patches@sourceware.org>; nd <nd@arm.com>; James-Adam Renquinha Henri <arenquinha@cimeq.qc.ca>
> Subject: Re: [PATCH] Fix exception stack unwinding for ARM Cortex-M
>  
> Ok, everything looks good to me now.
> 
> It’s fairly clear in the code where there is still work to be done.
> Do you have a bugzilla account? If so, could you please raise two bugs for the two features.
> If not, I can add them.
> 
> Doesn’t look like you have write access, so I’ll leave this to early next week and if there
> have been no other comments then I’ll commit it.
> 
> Thanks for the patch!
> 
> Alan.
> 
> 
> > On 6 Sep 2020, at 10:27, Fredrik Hederstierna <fredrik.hederstierna@verisure.com> wrote:
> > 
> > Hi,
> > I updated that patch to address your comments, see below and attached patch take2
> > 
> >> From: Alan Hayward <Alan.Hayward@arm.com>
> >> Sent: Wednesday, September 2, 2020 3:24 PM
> >> To: Fredrik Hederstierna <fredrik.hederstierna@verisure.com>
> > 
> >> How easy is it to compile a binary that exhibits this behaviour? If so then a
> >> test in testsuite/gdb.arch/ would be nice. For reference, aarch64-sighandler-regs.exp
> >> is a similar test but for AArch64.
> > 
> > I have not had time to further look into this, its probably possible to add such a test case, but I have no possibility to do this currently unfortunately.
> > 
> >> Have you signed the copyright assignment?
> > 
> > Yes, to my understanding everything is clear.
> > 
> >>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> >>> index 1ff47c3355..1d80e8cfc8 100644
> >>> --- a/gdb/ChangeLog
> >>> +++ b/gdb/ChangeLog
> >>> @@ -1,3 +1,9 @@
> >>> +2020-08-29  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
> >>> +         Adam Renquinha <arenquinha@cimeq.qc.ca>
> >>> +
> >>> +     * arm-tdep.c (arm_m_exception_cache): Try use correct stack
> >>> +     pointer and stack frame offset when unwinding.
> >>> +
> >> 
> >> Ideally this part should be left separate from the patch as to prevent
> >> merge issues.
> > 
> > Ok, removed from patch.
> > 
> >>> +  /* Check if main stack was used.  */
> >>> +  main_stack_used = ((lr & 0xf) != 0xd);
> >> 
> >> This took me a while to confirm. Could you mention that you are checking for
> >> SPSEL in the comment. Also, I wonder if it’s worth checking the other bits in lr.
> >> Yes they should be all ones in either case. But I’d rather be a little cautious.
> >> Only go into the else case if all the bits are correct.
> > 
> > Ok, added more clear comments and more strict bit checking.
> > 
> >>> +          /* Thread (process) stack could not be fetched,
> >>> +             give warning and exit.  */
> >>> +
> >>> +          warning (_("no PSP thread stack unwinding supported, exiting."));
> >> 
> >> I don’t think you mean exit. Maybe just remove “exiting” from the string.
> > 
> > Ok, removed 'exiting'
> > 
> >>> +      /* This code does not take into account the lazy stacking, see "Lazy
> >>> +         context save of FP state", in B1.5.7, also ARM AN298, supported
> >>> +         by Cortex-M4F architecture. Give a warning and try do best effort.
> >>> +         To fully handle this the FPCCR register (Floating-point Context
> >>> +         Control Register) needs to be read out and the bits ASPEN and LSPEN
> >>> +         could be checked to setup correct lazy stacked FP registers.  */
> >>> +
> >>> +      warning (_("no FPU lazy stack unwinding supported, check FPCCR."));
> >> 
> >> This means that we will always get a warning if the extended frame is used.
> >> I’d rather that didn’t happen.
> >> How easy would be be to check the FPCCR register and then give a warning only if
> >> lazy stacking is being used?
> > 
> > Maybe its possible, but have to time to solve this currently, added memory address of FPCCR,
> > its not a register, but probably possible to do memory reading to dig deeper into this.
> > Removed warning.
> > 
> >>> +      /* Basic frame type used.  */
> >>> +      cache->prev_sp = unwound_sp + 32;
> >> 
> >> The mix of hex and decimal in the function is a little glaring.
> >> Could you switch this one to 0x20.
> > 
> > Ok, fixed.
> > 
> > 
> > Here is ChangeLog, separated, new patch variant attached.
> > 
> > 2020-09-06  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
> >            Adam Renquinha <arenquinha@cimeq.qc.ca>
> > 
> >        * arm-tdep.c (arm_m_exception_cache): Try use correct stack
> >        pointer and stack frame offset when unwinding.
> > 
> > 
> > BR Fredrik
> > <gdb-cortex-m-exception-unwind-fix2.patch>

Re: [PATCH] Fix exception stack unwinding for ARM Cortex-M

[Joel Brobecker]

Hi Alan,

> > I do not have any GDB Bugzilla account, so please submit bugs for the additional features.
> 
> Added:
> https://sourceware.org/bugzilla/show_bug.cgi?id=26611
> https://sourceware.org/bugzilla/show_bug.cgi?id=26612
> https://sourceware.org/bugzilla/show_bug.cgi?id=26613
> 
> 
> > It would be great if patch goes in before the GDB 10 branching,
> 
> 
> Joel:
> Is it ok to pull this patch across to GDB 10? (And is that something you do?)
> It’s Arm only, and will only effect programs that are using special stack setups.

As the Area Maintainer for ARM, if the patch looks sufficiently safe
to you, you can approve the backport to a release branch. I'm always
happy to provide a second pair of eyes and an opinion when asked,
but that's not a requirement, simply because I'm not as well versed
in most areas of the code.

Looking at how the patch is written, it's not entirely obvious to me how
this can only affect programs using that special stack setup, but that's
probably because I don't know the ARM architecture as well as you do.
If you're confident about the patch, go ahead.

-- 
Joel

Re: [PATCH] Fix exception stack unwinding for ARM Cortex-M

[Alan Hayward]

> On 14 Sep 2020, at 19:31, Joel Brobecker <brobecker@adacore.com> wrote:
> 
> Hi Alan,
> 
>>> I do not have any GDB Bugzilla account, so please submit bugs for the additional features.
>> 
>> Added:
>> https://sourceware.org/bugzilla/show_bug.cgi?id=26611
>> https://sourceware.org/bugzilla/show_bug.cgi?id=26612
>> https://sourceware.org/bugzilla/show_bug.cgi?id=26613
>> 
>> 
>>> It would be great if patch goes in before the GDB 10 branching,
>> 
>> 
>> Joel:
>> Is it ok to pull this patch across to GDB 10? (And is that something you do?)
>> It’s Arm only, and will only effect programs that are using special stack setups.
> 
> As the Area Maintainer for ARM, if the patch looks sufficiently safe
> to you, you can approve the backport to a release branch. I'm always
> happy to provide a second pair of eyes and an opinion when asked,
> but that's not a requirement, simply because I'm not as well versed
> in most areas of the code.
> 
> Looking at how the patch is written, it's not entirely obvious to me how
> this can only affect programs using that special stack setup, but that's
> probably because I don't know the ARM architecture as well as you do.
> If you're confident about the patch, go ahead.

Ok, thanks for clearing that up :)

Agreed it’s not immediately obvious, but the new code is all inside if blocks,
with the standard case behaving identical to previously.

I’ve pushed the patch to gdb-10-branch now.


Thanks!
Alan.



> 
> -- 
> Joel

Re: Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[James-Adam Renquinha Henri]

Hi, I'm back from the dead.

Seriously though, I was a bit too busy at work to pursue on this and I 
didn't have a clear idea of where to start after the comments. Moreover, 
as the patch did "just work" at my workplace, it was a bit difficult to 
justify spending a lot more time digging information about target 
descriptions, how to determine what features are present on target, etc.

So thanks to push toward the progress of this issue, apparently with 
your mail subject you successfully brought people that gives more 
specific guidance to get this done.

Indeed, the patch looks really close to what I've done, to the point 
that I can see that, aside from a small refactoring, the changes between 
our patches are mainly variable name changes and added comments. You 
really wrote your patch from scratch? You've got rid of the various 
calls to `user_reg_map_name_to_regnum', though, which is nice.

 > Cortex-M has two stack pointers: MSP (Main Stack Pointer) and PSP 
(Process Stack Pointer).
 > This is not handled when GDB tries to backtrace in the exception 
stack unwinder.
 > Meaning for eg. Cortex-M4F its not possible to get any call-stack 
backtrace if setting a breakpoint in ISR, and floating-point variable 
was used.

Actually, the info here is inaccurate: FPU context stacking and the 
MSP/PSP switch are two unrelated concepts. Meaning, you can fix the code 
to deal with the PSP, but it still won't handle FPU register unstacking.

 > This patch was inspired by the great work done by James-Adam 
Renquinha Henri, submitted April this year.
Thanks :)

 > The next thing would then be to also add FPU context control reg 
FPCCR, which is needed for retrieving info on the FPU lazy stacking.
 > Though its complicated I think and I will try to investigate an 
'arm-m-fpu.xml' profile further, if this is solution perhaps.

It indeed is just a bit more complicated. Let me summarize what needs to 
be done. Have the ARMv7-M Architecture Reference Manual handy, see B1.5.7.

- Check if lazy stacking is enabled (FPCCR.LSPEN == 1). If it's not, the 
case is uncomplicated, registers are stacked as usual
- If lazy stacking is active (FPCCR.LSACT == 1), the extended stack has 
space reserved for the FPU registers (S0-S15, FPSCR), but there are not 
stacked, they are still in FPU registers unmodified.

So that adds more random fetches from "memory" of the target, because 
these are memory-mapped.

ARM gives some example scenarios with chronograms of some important bits 
with stacking information [1].

 > ChangeLog entry should cover what do you change on the function level.
 > Please read https://sourceware.org/gdb/wiki/ContributionChecklist

If only I knew that checklist! I searched for something like this, but 
maybe it didn't use the correct search terms, I didn't find it.

[1] 
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0298a/DAFGGBJD.html

James-Adam Renquinha Henri, Ing. jr
Ingénieur d'application
CIMEQ INC.


Le 2016-08-03 à 06:56, Fredrik Hederstierna a écrit :
> Hi Yao,
>
> thanks for reviewing, I attach here updated patch.
>
> I have FSF assignment papers for GCC from December 2005, maybe needs separate assignment for GDB,
> so I have now sent in application to FSF for GDB assignment as well.
>
> gdb/ChangeLog:
>
> 2016-08-03  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
>
>        * arm-tdep.c (arm_m_addr_is_magic): New function.
>        (arm_addr_bits_remove)
>        (arm_m_exception_unwind_sniffer):  Check correct EXC_RETURN values.
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index d2661cb..1d154cc 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -464,6 +464,61 @@ arm_pc_is_thumb (struct gdbarch *gdbarch, CORE_ADDR memaddr)
>    return 0;
>  }
>
> +/* Determine if the address specified equals any of
> +  these magic return values, called EXC_RETURN, defined
> +  by the ARM v6-M and v7-M architectures.
> +
> +  From ARMv6-M Reference Manual B1.5.8
> +  Table B1-5 Exception return behavior
> +
> +  EXC_RETURN    Return To        Return Stack
> +  0xFFFFFFF1    Handler mode     Main
> +  0xFFFFFFF9    Thread mode      Main
> +  0xFFFFFFFD    Thread mode      Process
> +
> +  From ARMv7-M Reference Manual B1.5.8
> +  Table B1-8 EXC_RETURN definition of exception return behavior, no FP
> +
> +  EXC_RETURN    Return To        Return Stack
> +  0xFFFFFFF1    Handler mode     Main
> +  0xFFFFFFF9    Thread mode      Main
> +  0xFFFFFFFD    Thread mode      Process
> +
> +  Table B1-9 EXC_RETURN definition of exception return behavior, with FP
> +
> +  EXC_RETURN    Return To        Return Stack    Frame Type
> +  0xFFFFFFE1    Handler mode     Main            Extended
> +  0xFFFFFFE9    Thread mode      Main            Extended
> +  0xFFFFFFED    Thread mode      Process         Extended
> +  0xFFFFFFF1    Handler mode     Main            Basic
> +  0xFFFFFFF9    Thread mode      Main            Basic
> +  0xFFFFFFFD    Thread mode      Process         Basic
> +
> +  For more details see "B1.5.8 Exception return behavior"
> +  in both ARMv6-M and ARMv7-M Architecture Reference Manuals.  */
> +
> +static int
> +arm_m_addr_is_magic (CORE_ADDR addr)
> +{
> +  switch (addr)
> +    {
> +      /* 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;
> +    }
> +}
> +
>  /* Remove useless bits from addresses in a running program.  */
>  static CORE_ADDR
>  arm_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR val)
> @@ -471,7 +526,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 (gdbarch_tdep (gdbarch)->is_m
> -      && (val & 0xfffffff0) =0xfffffff0)
> +      && arm_m_addr_is_magic (val))
>      return val;
>
>    if (arm_apcs_32)
> @@ -2990,14 +3045,8 @@ arm_m_exception_unwind_sniffer (const struct frame_unwind *self,
>    /* 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;
> +  /* Check if exception frame returns to a magic PC value.  */
> +  return arm_m_addr_is_magic (this_pc);
>  }
>
>  /* Frame unwinder for M-profile exceptions.  */
> --
>

Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[Yao Qi]

"Fredrik Hederstierna" <fredrik.hederstierna@verisure.com> writes:

Fredrik,
A general comment to this patch is that you need to split it.  Each
patch only addresses one issue or adds one support.

> @@ -2919,15 +2966,47 @@ arm_m_exception_cache (struct frame_info *this_frame)
>    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_IP_REGNUM].addr = unwound_sp + 16;
> +  cache->saved_regs[ARM_LR_REGNUM].addr = unwound_sp + 20;
> +  cache->saved_regs[ARM_PC_REGNUM].addr = unwound_sp + 24;
>    cache->saved_regs[ARM_PS_REGNUM].addr = unwound_sp + 28;

This change can be in a separate patch.  Could you move it to separate
patch, write changelog entry, and post it again?  It is OK to commit.

>  
> +  /* Check if extended stack frame (FPU regs stored) was used.  */
> +  extended_frame_used = ((lr & (1 << 4)) == 0);
> +  if (extended_frame_used)
> +    {
> +	  int i;
> +	  int fpu_regs_stack_offset;
> +
> +	  /* This code does not take into account the lazy stacking, see "Lazy
> +	     context save of FP state", in B1.5.7, also ARM AN298, supported
> +	     by Cortex-M4F architecture. Give a warning and try do best effort.
> +	     To fully handle this the FPCCR register (Floating-point Context
> +	     Control Register) needs to be read out and the bits ASPEN and LSPEN
> +	     could be checked to setup correct lazy stacked FP registers.  */
> +
> +	  warning (_("no FPU lazy stack unwinding supported, check FPCCR."));
> +
> +	  fpu_regs_stack_offset = unwound_sp + 0x20;
> +	  for (i = 0; i < 16; i++)
> +	    {
> +	      cache->saved_regs[ARM_D0_REGNUM + i].addr = fpu_regs_stack_offset;
> +	      fpu_regs_stack_offset += 4;
> +	    }
> +	  cache->saved_regs[ARM_FPSCR_REGNUM].addr = unwound_sp + 0x60;
> +
> +	  /* Offset 0x64 is reserved.  */
> +	  cache->prev_sp = unwound_sp + 0x68;
> +    }
> +  else
> +    {
> +      /* Basic frame type used.  */
> +      cache->prev_sp = unwound_sp + 32;
> +    }
> +

I don't know much about lazy stacking, but it should be in a separate
patch for lazy stacking.

>    /* 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;
> @@ -2977,6 +3056,41 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
>  				       prev_regnum);
>  }
>  
> +/* Determine if the program counter specified equals any of
> +  these magic return values defined by v7-M architecture. */
> +
> +static int
> +arm_m_pc_is_magic (CORE_ADDR pc)
> +{
> +  /* Exception frames return to one of these magic PCs defined in v7-M.
> +     For more details see "B1.5.8 Exception return behavior"
> +     in "ARMv7-M Architecture Reference Manual".  */

We need to consider ARMv6-M as well,

> +  switch (pc)
> +    {
> +      /* From Table B1-8 and B1-9 the EXC_RETURN definition of
> +         the exception return behavior.  */
> +
> +      /* Return to Handler mode. Return stack Main. Frame type Extended.  */

I don't see anything useful the comment has.  We can remove it.
Instead, we need to document, on ARMv6-M and ARMv7-M without FP
extension, the exc_return is 0xfffffff{1,9,d}.  On ARMv7-M with FP
extension, exc_return can be 0xffffff{e,f}{1,9,d}.

> +      case 0xffffffe1:
> +      /* Return to Thread mode. Return stack Main. Frame type Extended.  */
> +      case 0xffffffe9:
> +      /* Return to Thread mode. Return stack Process. Frame type Extended.  */
> +      case 0xffffffed:
> +      /* Return to Handler mode. Return stack Main. Frame type Basic.  */
> +      case 0xfffffff1:
> +      /* Return to Thread mode. Return stack Main. Frame type Basic.  */
> +      case 0xfffffff9:
> +      /* Return to Thread mode. Return stack Process. Frame type Basic.  */
> +      case 0xfffffffd:
> +        /* PC is magic.  */
> +        return 1;
> +
> +      default:
> +        /* PC is not magic.  */
> +        return 0;
> +    }
> +}
> +
>  /* Implementation of function hook 'sniffer' in
>     'struct frame_uwnind'.  */
>  
> @@ -2990,14 +3104,8 @@ arm_m_exception_unwind_sniffer (const struct frame_unwind *self,
>    /* 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;
> +  /* Check if exception frame returns to a magic PC value.  */
> +  return arm_m_pc_is_magic (this_pc);

Please post the patch only for magic pc handling, then I can review and
approve it.

-- 
Yao (齐尧)

Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[Fredrik Hederstierna]

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


-----Yao Qi <qiyaoltc@gmail.com> wrote: -----
>Fredrik,
>A general comment to this patch is that you need to split it. Each
>patch only addresses one issue or adds one support.

Ok, I send you first patch, this one fix EXC_RETURN values.

>We need to consider ARMv6-M as well,
>> + switch (pc)
>> + {
>> + /* From Table B1-8 and B1-9 the EXC_RETURN definition of
>> + the exception return behavior. */
>> +
>> + /* Return to Handler mode. Return stack Main. Frame type
>Extended. */
>
>I don't see anything useful the comment has. We can remove it.
>Instead, we need to document, on ARMv6-M and ARMv7-M without FP
>extension, the exc_return is 0xfffffff{1,9,d}. On ARMv7-M with FP
>extension, exc_return can be 0xffffff{e,f}{1,9,d}.

Ok, I documented also ARMv6-M in new patch.

>Please post the patch only for magic pc handling, then I can review
>and approve it.

Ok, done. Attached first patch.
I will submit next when it is ready.

Thanks & Kind Regards,
Fredrik

[-- Attachment #2: gdb-cortex-m-exc-return-values.patch --]
[-- Type: application/octet-stream, Size: 3697 bytes --]

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7820302..2c8573c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2016-08-02  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
+
+	* arm-tdep.c: Fix EXC_RETURN values for ARMv6-M and ARMv7-M.
+
 2016-08-01  Joel Brobecker  <brobecker@adacore.com>
 
 	* NEWS: Create a new section for the next release branch.
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index d2661cb..43436df 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -469,9 +469,11 @@ 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).  */
+     (the magic exception return address).
+     According to B1.5.8 of both the ARMv6-M and ARMv7-M Reference Manuals
+     the EXC_RETURN value is 0xF in Bits[31:28].  */
   if (gdbarch_tdep (gdbarch)->is_m
-      && (val & 0xfffffff0) == 0xfffffff0)
+      && (val & 0xf0000000) == 0xf0000000)
     return val;
 
   if (arm_apcs_32)
@@ -2977,6 +2979,61 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
 				       prev_regnum);
 }
 
+/* Determine if the program counter specified equals any of
+  these magic return values, called EXC_RETURN, defined by the
+  ARM v6-M and v7-M architectures.
+
+  From ARMv6-M Reference Manual B1.5.8
+  Table B1-5 Exception return behavior
+
+  EXC_RETURN    Return To        Return Stack
+  0xFFFFFFF1    Handler mode     Main
+  0xFFFFFFF9    Thread mode      Main
+  0xFFFFFFFD    Thread mode      Process
+
+  From ARMv7-M Reference Manual B1.5.8
+  Table B1-8 EXC_RETURN definition of exception return behavior, no FP
+
+  EXC_RETURN    Return To        Return Stack
+  0xFFFFFFF1    Handler mode     Main
+  0xFFFFFFF9    Thread mode      Main
+  0xFFFFFFFD    Thread mode      Process
+
+  Table B1-9 EXC_RETURN definition of exception return behavior, with FP
+
+  EXC_RETURN    Return To        Return Stack    Frame Type
+  0xFFFFFFE1    Handler mode     Main            Extended
+  0xFFFFFFE9    Thread mode      Main            Extended
+  0xFFFFFFED    Thread mode      Process         Extended
+  0xFFFFFFF1    Handler mode     Main            Basic
+  0xFFFFFFF9    Thread mode      Main            Basic
+  0xFFFFFFFD    Thread mode      Process         Basic
+
+  For more details see "B1.5.8 Exception return behavior"
+  in both ARMv6-M and ARMv7-M Architecture Reference Manuals.  */
+
+static int
+arm_m_pc_is_magic (CORE_ADDR pc)
+{
+  switch (pc)
+    {
+      /* 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:
+        /* PC is magic.  */
+        return 1;
+
+      default:
+        /* PC is not magic.  */
+        return 0;
+    }
+}
+
 /* Implementation of function hook 'sniffer' in
    'struct frame_uwnind'.  */
 
@@ -2990,14 +3047,8 @@ arm_m_exception_unwind_sniffer (const struct frame_unwind *self,
   /* 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;
+  /* Check if exception frame returns to a magic PC value.  */
+  return arm_m_pc_is_magic (this_pc);
 }
 
 /* Frame unwinder for M-profile exceptions.  */

Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[Yao Qi]

"Fredrik Hederstierna" <fredrik.hederstierna@verisure.com> writes:

Hi Fredrik,
Thanks for splitting the patch.

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 7820302..2c8573c 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,7 @@
> +2016-08-02  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
> +
> +	* arm-tdep.c: Fix EXC_RETURN values for ARMv6-M and ARMv7-M.
> +

ChangeLog entry should cover what do you change on the function level.
Please read https://sourceware.org/gdb/wiki/ContributionChecklist

>  2016-08-01  Joel Brobecker  <brobecker@adacore.com>
>  
>  	* NEWS: Create a new section for the next release branch.
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index d2661cb..43436df 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -469,9 +469,11 @@ 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).  */
> +     (the magic exception return address).
> +     According to B1.5.8 of both the ARMv6-M and ARMv7-M Reference Manuals
> +     the EXC_RETURN value is 0xF in Bits[31:28].  */
>    if (gdbarch_tdep (gdbarch)->is_m
> -      && (val & 0xfffffff0) == 0xfffffff0)
> +      && (val & 0xf0000000) == 0xf0000000)


Let us do the strict checking.  In ARMv7-M manual, the pseudo code of 
ExceptionReturn(bits(28) EXC_RETURN) has

if HaveFPExt() then
if !IsOnes(EXC_RETURN<27:5>) then UNPREDICTABLE;
else
if !IsOnes(EXC_RETURN<27:4>) then UNPREDICTABLE;

so let's check "val" should be 0xFFFFFF{E,F}{1,9,D}, or use
arm_m_pc_is_magic too, like,

       if (gdbarch_tdep (gdbarch)->is_m
           && arm_m_pc_is_magic (val))
          return val;

Do you have FSF copyright assignment?  I don't find your record.  You
can fill in the form request-assign.future in
https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment
then, we can pick up your change.

-- 
Yao (齐尧)

Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[Fredrik Hederstierna]

Hi Yao,

thanks for reviewing, I attach here updated patch.

I have FSF assignment papers for GCC from December 2005, maybe needs separate assignment for GDB,
so I have now sent in application to FSF for GDB assignment as well.

gdb/ChangeLog:

2016-08-03  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>

       * arm-tdep.c (arm_m_addr_is_magic): New function.
       (arm_addr_bits_remove)
       (arm_m_exception_unwind_sniffer):  Check correct EXC_RETURN values.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index d2661cb..1d154cc 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -464,6 +464,61 @@ arm_pc_is_thumb (struct gdbarch *gdbarch, CORE_ADDR memaddr)
   return 0;
 }
 
+/* Determine if the address specified equals any of
+  these magic return values, called EXC_RETURN, defined
+  by the ARM v6-M and v7-M architectures.
+
+  From ARMv6-M Reference Manual B1.5.8
+  Table B1-5 Exception return behavior
+
+  EXC_RETURN    Return To        Return Stack
+  0xFFFFFFF1    Handler mode     Main
+  0xFFFFFFF9    Thread mode      Main
+  0xFFFFFFFD    Thread mode      Process
+
+  From ARMv7-M Reference Manual B1.5.8
+  Table B1-8 EXC_RETURN definition of exception return behavior, no FP
+
+  EXC_RETURN    Return To        Return Stack
+  0xFFFFFFF1    Handler mode     Main
+  0xFFFFFFF9    Thread mode      Main
+  0xFFFFFFFD    Thread mode      Process
+
+  Table B1-9 EXC_RETURN definition of exception return behavior, with FP
+
+  EXC_RETURN    Return To        Return Stack    Frame Type
+  0xFFFFFFE1    Handler mode     Main            Extended
+  0xFFFFFFE9    Thread mode      Main            Extended
+  0xFFFFFFED    Thread mode      Process         Extended
+  0xFFFFFFF1    Handler mode     Main            Basic
+  0xFFFFFFF9    Thread mode      Main            Basic
+  0xFFFFFFFD    Thread mode      Process         Basic
+
+  For more details see "B1.5.8 Exception return behavior"
+  in both ARMv6-M and ARMv7-M Architecture Reference Manuals.  */
+
+static int
+arm_m_addr_is_magic (CORE_ADDR addr)
+{
+  switch (addr)
+    {
+      /* 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;
+    }
+}
+
 /* Remove useless bits from addresses in a running program.  */
 static CORE_ADDR
 arm_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR val)
@@ -471,7 +526,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 (gdbarch_tdep (gdbarch)->is_m
-      && (val & 0xfffffff0) == 0xfffffff0)
+      && arm_m_addr_is_magic (val))
     return val;
 
   if (arm_apcs_32)
@@ -2990,14 +3045,8 @@ arm_m_exception_unwind_sniffer (const struct frame_unwind *self,
   /* 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;
+  /* Check if exception frame returns to a magic PC value.  */
+  return arm_m_addr_is_magic (this_pc);
 }
 
 /* Frame unwinder for M-profile exceptions.  */
--

Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[Yao Qi]

On Wed, Aug 3, 2016 at 11:56 AM, Fredrik Hederstierna
<fredrik.hederstierna@verisure.com> wrote:
>
> I have FSF assignment papers for GCC from December 2005, maybe needs separate assignment for GDB,
> so I have now sent in application to FSF for GDB assignment as well.
>

Yes, you need a separate assignment for GDB.  Let me know once your
assignment is ready.  In the meantime, you can continue posting other
patches.

-- 
Yao (齐尧)

Re: Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[Fredrik Hederstierna]

Hi James-Adam,

-----James-Adam Renquinha Henri <arenquinha@cimeq.qc.ca> wrote: -----
>Hi, I'm back from the dead.

Nice to see you back :)


>Seriously though, I was a bit too busy at work to pursue on this and
>I 
>didn't have a clear idea of where to start after the comments.
>Moreover, 
>as the patch did "just work" at my workplace, it was a bit difficult
>to 
>justify spending a lot more time digging information about target 
>descriptions, how to determine what features are present on target,
>etc.
>So thanks to push toward the progress of this issue, apparently with 
>your mail subject you successfully brought people that gives more 
>specific guidance to get this done.

I tried to check the status on your submission mid-July (https://sourceware.org/ml/gdb/2016-07/msg00011.html),
but since I got no directions from anyone in the community at that time, I decided to try continue myself to get this fixed.
We work alot with Cortex-M4F on a daily basis, and do really need this to be fixed.


>Indeed, the patch looks really close to what I've done, to the point 
>that I can see that, aside from a small refactoring, the changes
>between 
>our patches are mainly variable name changes and added comments. You 
>really wrote your patch from scratch? You've got rid of the various 
>calls to `user_reg_map_name_to_regnum', though, which is nice.

Yes I wrote it from scratch, but I agree my patch looks similar to yours, as its implementing according to ARM Cortex-M spec,
I guess it needs to be. But it absolutely helped me to have your ideas as starting point to look at, to see where modifications needed to be done,
so I do not mind if you would like to stand as co-author in the ChangeLog?
If you have FSF GDB Assignment, or would like to apply for assignment,
I can add your name to ChangeLog, its totally fine by me.

The important thing for me is to get this fixed, and who gets the actual and final credits I do not mind.
And I think it would be great if we could try to continue this work together, to get the remaining parts for Cortex-M support in GDB.
The more people who are working on this, the better.
I am not so experienced in GDB internals, and also seek guidance how to continue go get the last parts in place.
I think both to get GDB aware of MSP and PSP, and also FPCCR needs to be a bigger change and are more complicated.
My idea is to submit a bug entry in bugzilla on this, one issue for the MSP/PSP support, another one for FPCCR, so we can get track of this, and someone could be assigned.


> > Cortex-M has two stack pointers: MSP (Main Stack Pointer) and PSP 
>(Process Stack Pointer).
> > This is not handled when GDB tries to backtrace in the exception 
>stack unwinder.
> > Meaning for eg. Cortex-M4F its not possible to get any call-stack 
>backtrace if setting a breakpoint in ISR, and floating-point variable
>was used.
>
>Actually, the info here is inaccurate: FPU context stacking and the 
>MSP/PSP switch are two unrelated concepts. Meaning, you can fix the
>code 
>to deal with the PSP, but it still won't handle FPU register
>unstacking.

Correct, my plan now was to split the patch as Yao proposed.
The the MSP/PSP-fix and FPU-fix would be two different patches.


> > This patch was inspired by the great work done by James-Adam 
>Renquinha Henri, submitted April this year.
>Thanks :)
>
> > The next thing would then be to also add FPU context control reg 
>FPCCR, which is needed for retrieving info on the FPU lazy stacking.
> > Though its complicated I think and I will try to investigate an 
>'arm-m-fpu.xml' profile further, if this is solution perhaps.
>
>It indeed is just a bit more complicated. Let me summarize what needs
>to 
>be done. Have the ARMv7-M Architecture Reference Manual handy, see
>B1.5.7.
>
>- Check if lazy stacking is enabled (FPCCR.LSPEN == 1). If it's not,
>the 
>case is uncomplicated, registers are stacked as usual
>- If lazy stacking is active (FPCCR.LSACT == 1), the extended stack
>has 
>space reserved for the FPU registers (S0-S15, FPSCR), but there are
>not 
>stacked, they are still in FPU registers unmodified.
>
>So that adds more random fetches from "memory" of the target, because
>
>these are memory-mapped.
>
>ARM gives some example scenarios with chronograms of some important
>bits 
>with stacking information [1].

Yes, my idea was that GDB needs to be aware of FPCCR, so we need probably an new XML profile feature for this, just as for PSP/MSP.
But you are correct, its way more complicated. See also ARM Application Note I'm referring to in my patch comments.


> > ChangeLog entry should cover what do you change on the function
>level.
> > Please read https://sourceware.org/gdb/wiki/ContributionChecklist
>
>If only I knew that checklist! I searched for something like this,
>but 
>maybe it didn't use the correct search terms, I didn't find it.

If you want to stand as co-author in ChangeLog, its totally fine by me,
just give me a hint if you have the FSF assignment done,
and I also would be more than happy if you have time to continue your great work, and get the last parts of unwinding support for Cortex-M in place.


Thanks, and Kind Regards,
Fredrik

Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[Adam Renquinha]

Hi Fredrik,

 >> Seriously though, I was a bit too busy at work to pursue on this and
 >> I
 >> didn't have a clear idea of where to start after the comments.
 >> Moreover,
 >> as the patch did "just work" at my workplace, it was a bit difficult
 >> to
 >> justify spending a lot more time digging information about target
 >> descriptions, how to determine what features are present on target,
 >> etc.
 >> So thanks to push toward the progress of this issue, apparently with
 >> your mail subject you successfully brought people that gives more
 >> specific guidance to get this done.
 >
 > I tried to check the status on your submission mid-July 
(https://sourceware.org/ml/gdb/2016-07/msg00011.html),
 > but since I got no directions from anyone in the community at that 
time, I decided to try continue myself to get this fixed.
 > We work alot with Cortex-M4F on a daily basis, and do really need 
this to be fixed.

We do, too. It isn't fun when you try to debug a firmware using FreeRTOS 
and the FPU on a Cortex-M4, only to see that you cannot backtrace into 
suspended tasks, is it? It took quite a bit of time for us to understand 
where the problem lied... I worked on this bug because I could build 
about anything really easily in my Arch Linux VM, and I know very well 
how to use plain gdb.

 >> Indeed, the patch looks really close to what I've done, to the point
 >> that I can see that, aside from a small refactoring, the changes
 >> between
 >> our patches are mainly variable name changes and added comments. You
 >> really wrote your patch from scratch? You've got rid of the various
 >> calls to `user_reg_map_name_to_regnum', though, which is nice.
 >
 > Yes I wrote it from scratch, but I agree my patch looks similar to 
yours, as its implementing according to ARM Cortex-M spec,
 > I guess it needs to be. But it absolutely helped me to have your 
ideas as starting point to look at, to see where modifications needed to 
be done,
 > so I do not mind if you would like to stand as co-author in the 
ChangeLog?
 > If you have FSF GDB Assignment, or would like to apply for assignment,
 > I can add your name to ChangeLog, its totally fine by me.

That is fine for me too, I don't mind at all. I never had submitted work 
that required an FSF submission, but I plan to do that.

 > And I think it would be great if we could try to continue this work 
together, to get the remaining parts for Cortex-M support in GDB.
 > The more people who are working on this, the better.

Yes! :)

 > I am not so experienced in GDB internals, and also seek guidance how 
to continue go get the last parts in place.

I'm not experienced at all with GDB internals either. I ran a test 
program under `arm-none-eabi-gdb' under plain gdb (a bit of a headache, 
a debugger inside a debugger), and I traced what happened upon 
unwinding, and fixed rather directly what I saw was wrong. I knew that 
what would result from that process would be rather crude, due to my 
inexperience. For instance, target descriptions were news for me.

Guidance on that specific part (handling target description) is needed. 
Anyone?

 > My idea is to submit a bug entry in bugzilla on this, one issue for 
the MSP/PSP support, another one for FPCCR, so we can get track of this, 
and someone could be assigned.

Good idea.

 >>> Cortex-M has two stack pointers: MSP (Main Stack Pointer) and PSP
 >> (Process Stack Pointer).
 >>> This is not handled when GDB tries to backtrace in the exception
 >> stack unwinder.
 >>> Meaning for eg. Cortex-M4F its not possible to get any call-stack
 >> backtrace if setting a breakpoint in ISR, and floating-point variable
 >> was used.
 >>
 >> Actually, the info here is inaccurate: FPU context stacking and the
 >> MSP/PSP switch are two unrelated concepts. Meaning, you can fix the
 >> code
 >> to deal with the PSP, but it still won't handle FPU register
 >> unstacking.
 >
 > Correct, my plan now was to split the patch as Yao proposed.
 > The the MSP/PSP-fix and FPU-fix would be two different patches.

Yes. Smaller patches will help things moving forward. I wasn't sure at 
the time if beginning with a small patch just covering e.g. magic PC 
values handling would be considered useful, because in itself it fixes 
nothing, but it really is important to handle the MSP/PSP and the FPU 
cases. So I opted for a big comprehensive patch. But indeed, it's easier 
to review, and easier to integrate.

 >>> ChangeLog entry should cover what do you change on the function
 >> level.
 >>> Please read https://sourceware.org/gdb/wiki/ContributionChecklist
 >>
 >> If only I knew that checklist! I searched for something like this,
 >> but
 >> maybe it didn't use the correct search terms, I didn't find it.
 >
 > If you want to stand as co-author in ChangeLog, its totally fine by me,
 > just give me a hint if you have the FSF assignment done,
 > and I also would be more than happy if you have time to continue your 
great work, and get the last parts of unwinding support for Cortex-M in 
place.

Hmm, if I understand correctly, I should e-mail one of the gdb 
maintainers to get the assignment to fill? You did that before; how it went?

Thanks,


James-Adam Renquinha Henri, Ing. jr
Ingénieur d'application
CIMEQ INC.

Le 2016-08-04 à 03:03, Fredrik Hederstierna a écrit :
> Hi James-Adam,
>
> -----James-Adam Renquinha Henri <arenquinha@cimeq.qc.ca> wrote: -----
>> Hi, I'm back from the dead.
>
> Nice to see you back :)
>
>
>> Seriously though, I was a bit too busy at work to pursue on this and
>> I
>> didn't have a clear idea of where to start after the comments.
>> Moreover,
>> as the patch did "just work" at my workplace, it was a bit difficult
>> to
>> justify spending a lot more time digging information about target
>> descriptions, how to determine what features are present on target,
>> etc.
>> So thanks to push toward the progress of this issue, apparently with
>> your mail subject you successfully brought people that gives more
>> specific guidance to get this done.
>
> I tried to check the status on your submission mid-July (https://sourceware.org/ml/gdb/2016-07/msg00011.html),
> but since I got no directions from anyone in the community at that time, I decided to try continue myself to get this fixed.
> We work alot with Cortex-M4F on a daily basis, and do really need this to be fixed.
>
>
>> Indeed, the patch looks really close to what I've done, to the point
>> that I can see that, aside from a small refactoring, the changes
>> between
>> our patches are mainly variable name changes and added comments. You
>> really wrote your patch from scratch? You've got rid of the various
>> calls to `user_reg_map_name_to_regnum', though, which is nice.
>
> Yes I wrote it from scratch, but I agree my patch looks similar to yours, as its implementing according to ARM Cortex-M spec,
> I guess it needs to be. But it absolutely helped me to have your ideas as starting point to look at, to see where modifications needed to be done,
> so I do not mind if you would like to stand as co-author in the ChangeLog?
> If you have FSF GDB Assignment, or would like to apply for assignment,
> I can add your name to ChangeLog, its totally fine by me.
>
> The important thing for me is to get this fixed, and who gets the actual and final credits I do not mind.
> And I think it would be great if we could try to continue this work together, to get the remaining parts for Cortex-M support in GDB.
> The more people who are working on this, the better.
> I am not so experienced in GDB internals, and also seek guidance how to continue go get the last parts in place.
> I think both to get GDB aware of MSP and PSP, and also FPCCR needs to be a bigger change and are more complicated.
> My idea is to submit a bug entry in bugzilla on this, one issue for the MSP/PSP support, another one for FPCCR, so we can get track of this, and someone could be assigned.
>
>
>>> Cortex-M has two stack pointers: MSP (Main Stack Pointer) and PSP
>> (Process Stack Pointer).
>>> This is not handled when GDB tries to backtrace in the exception
>> stack unwinder.
>>> Meaning for eg. Cortex-M4F its not possible to get any call-stack
>> backtrace if setting a breakpoint in ISR, and floating-point variable
>> was used.
>>
>> Actually, the info here is inaccurate: FPU context stacking and the
>> MSP/PSP switch are two unrelated concepts. Meaning, you can fix the
>> code
>> to deal with the PSP, but it still won't handle FPU register
>> unstacking.
>
> Correct, my plan now was to split the patch as Yao proposed.
> The the MSP/PSP-fix and FPU-fix would be two different patches.
>
>
>>> This patch was inspired by the great work done by James-Adam
>> Renquinha Henri, submitted April this year.
>> Thanks :)
>>
>>> The next thing would then be to also add FPU context control reg
>> FPCCR, which is needed for retrieving info on the FPU lazy stacking.
>>> Though its complicated I think and I will try to investigate an
>> 'arm-m-fpu.xml' profile further, if this is solution perhaps.
>>
>> It indeed is just a bit more complicated. Let me summarize what needs
>> to
>> be done. Have the ARMv7-M Architecture Reference Manual handy, see
>> B1.5.7.
>>
>> - Check if lazy stacking is enabled (FPCCR.LSPEN == 1). If it's not,
>> the
>> case is uncomplicated, registers are stacked as usual
>> - If lazy stacking is active (FPCCR.LSACT == 1), the extended stack
>> has
>> space reserved for the FPU registers (S0-S15, FPSCR), but there are
>> not
>> stacked, they are still in FPU registers unmodified.
>>
>> So that adds more random fetches from "memory" of the target, because
>>
>> these are memory-mapped.
>>
>> ARM gives some example scenarios with chronograms of some important
>> bits
>> with stacking information [1].
>
> Yes, my idea was that GDB needs to be aware of FPCCR, so we need probably an new XML profile feature for this, just as for PSP/MSP.
> But you are correct, its way more complicated. See also ARM Application Note I'm referring to in my patch comments.
>
>
>>> ChangeLog entry should cover what do you change on the function
>> level.
>>> Please read https://sourceware.org/gdb/wiki/ContributionChecklist
>>
>> If only I knew that checklist! I searched for something like this,
>> but
>> maybe it didn't use the correct search terms, I didn't find it.
>
> If you want to stand as co-author in ChangeLog, its totally fine by me,
> just give me a hint if you have the FSF assignment done,
> and I also would be more than happy if you have time to continue your great work, and get the last parts of unwinding support for Cortex-M in place.
>
>
> Thanks, and Kind Regards,
> Fredrik
>

Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[Yao Qi]

On Fri, Aug 5, 2016 at 8:01 PM, Adam Renquinha <arenquinha@cimeq.qc.ca> wrote:
>
> Hmm, if I understand correctly, I should e-mail one of the gdb maintainers
> to get the assignment to fill? You did that before; how it went?

You can follow this, and use the third form (request-assign.future)
https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment

-- 
Yao (齐尧)

Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[Adam Renquinha]

FYI, the assignment process is done for me as well as for Fredrik. The 
patch can be submitted.

James-Adam Renquinha Henri, Ing. jr
Ingénieur d'application
CIMEQ INC.


Le 2016-08-09 à 05:16, Yao Qi a écrit :
> On Fri, Aug 5, 2016 at 8:01 PM, Adam Renquinha <arenquinha@cimeq.qc.ca> wrote:
>>
>> Hmm, if I understand correctly, I should e-mail one of the gdb maintainers
>> to get the assignment to fill? You did that before; how it went?
>
> You can follow this, and use the third form (request-assign.future)
> https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment
>

Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[Yao Qi]

Adam Renquinha <arenquinha@cimeq.qc.ca> writes:

> FYI, the assignment process is done for me as well as for Fredrik. The
> patch can be submitted.

OK, I rebased Fredrik's patch
https://sourceware.org/ml/gdb-patches/2016-08/msg00050.html, add some
commit log, and adjust the comment format a little bit.  What do you
think of the patch below?  If this is OK, I'll push it in to master and
7.12 branch.

-- 
Yao (齐尧)
From 7b10ca5f8650e9050c7211702c0a5b9cb0937722 Mon Sep 17 00:00:00 2001
From: Fredrik Hederstierna <fredrik.hederstierna@verisure.com>
Date: Mon, 26 Sep 2016 03:32:21 +0100
Subject: [PATCH] Detect the magic address of EXC_RETURN in ARM coretx-m
 profile

On ARMv6-M and ARMv7-M, the exception return address is sort of magic
address defined by the manual.  This patch is to let GDB well handle
these magic addresses.

2016-09-25  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>

	* arm-tdep.c (arm_m_addr_is_magic): New function.
	(arm_addr_bits_remove): Call arm_m_addr_is_magic.
	(arm_m_exception_unwind_sniffer): Likewise.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 4dfd76b..a07d93b 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -465,6 +465,62 @@ arm_pc_is_thumb (struct gdbarch *gdbarch, CORE_ADDR memaddr)
   return 0;
 }
 
+/* Determine if the address specified equals any of these magic return
+   values, called EXC_RETURN, defined by the ARM v6-M and v7-M
+   architectures.
+
+   From ARMv6-M Reference Manual B1.5.8
+   Table B1-5 Exception return behavior
+
+   EXC_RETURN    Return To        Return Stack
+   0xFFFFFFF1    Handler mode     Main
+   0xFFFFFFF9    Thread mode      Main
+   0xFFFFFFFD    Thread mode      Process
+
+   From ARMv7-M Reference Manual B1.5.8
+   Table B1-8 EXC_RETURN definition of exception return behavior, no FP
+
+   EXC_RETURN    Return To        Return Stack
+   0xFFFFFFF1    Handler mode     Main
+   0xFFFFFFF9    Thread mode      Main
+   0xFFFFFFFD    Thread mode      Process
+
+   Table B1-9 EXC_RETURN definition of exception return behavior, with
+   FP
+
+   EXC_RETURN    Return To        Return Stack    Frame Type
+   0xFFFFFFE1    Handler mode     Main            Extended
+   0xFFFFFFE9    Thread mode      Main            Extended
+   0xFFFFFFED    Thread mode      Process         Extended
+   0xFFFFFFF1    Handler mode     Main            Basic
+   0xFFFFFFF9    Thread mode      Main            Basic
+   0xFFFFFFFD    Thread mode      Process         Basic
+
+   For more details see "B1.5.8 Exception return behavior"
+   in both ARMv6-M and ARMv7-M Architecture Reference Manuals.  */
+
+static int
+arm_m_addr_is_magic (CORE_ADDR addr)
+{
+  switch (addr)
+    {
+      /* 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;
+    }
+}
+
 /* Remove useless bits from addresses in a running program.  */
 static CORE_ADDR
 arm_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR val)
@@ -472,7 +528,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 (gdbarch_tdep (gdbarch)->is_m
-      && (val & 0xfffffff0) == 0xfffffff0)
+      && arm_m_addr_is_magic (val))
     return val;
 
   if (arm_apcs_32)
@@ -2991,14 +3047,8 @@ arm_m_exception_unwind_sniffer (const struct frame_unwind *self,
   /* 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;
+  /* Check if exception frame returns to a magic PC value.  */
+  return arm_m_addr_is_magic (this_pc);
 }
 
 /* Frame unwinder for M-profile exceptions.  */

Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[Adam Renquinha]

That looks correct.

James-Adam Renquinha Henri, Ing. jr
Ingénieur d'application
CIMEQ INC.


Le 2016-09-25 à 22:47, Yao Qi a écrit :
> Adam Renquinha <arenquinha@cimeq.qc.ca> writes:
>
>> FYI, the assignment process is done for me as well as for Fredrik. The
>> patch can be submitted.
>
> OK, I rebased Fredrik's patch
> https://sourceware.org/ml/gdb-patches/2016-08/msg00050.html, add some
> commit log, and adjust the comment format a little bit.  What do you
> think of the patch below?  If this is OK, I'll push it in to master and
> 7.12 branch.
>

Re: [PATCH] Fix exception unwinding for ARM Cortex-M

[Yao Qi]

On Mon, Sep 26, 2016 at 3:26 PM, Adam Renquinha <arenquinha@cimeq.qc.ca> wrote:
> That looks correct.
>

Patch is pushed into master and 7.12.  The rest of your original
patch is still welcome!

-- 
Yao (齐尧)