[RFC] arm-tdep.c (arm_m_exception_cache): Handle stack switching to PSP during exception unwind.

  The current GDB code sometimes unwinds the exception stack incorrectly
on a Cortex-M3 based CPU (Atmel ATSAM3X8E). The problem is that the
current stack unwinding code does not take into account that the stack
pointer may switch from the main stack pointer (MSP) to the process
stack pointer (PSP) when the exception returns. 

The result is that the code displayed as executing at the time of the
exception is incorrect:

(gdb) bt
#0  delayMS (millis=300) at FreeRTOS_ARM/FreeRTOS_ARM.c:17
#1  errorBlink (n=n@entry=1) at FreeRTOS_ARM/FreeRTOS_ARM.c:32
#2  0x00085cd4 in HardFault_Handler () at FreeRTOS_ARM/FreeRTOS_ARM.c:44
#3  <signal handler called>
#4  0x0008684e in prvPortStartFirstTask () at FreeRTOS_ARM/utility/port.c:277
#5  0x00086a38 in xPortStartScheduler () at FreeRTOS_ARM/utility/port.c:356
#6  0xa5a5a5a4 in ?? ()

With the patch applied the code which accessed the bad pointer to
trigger the exception is displayed instead:

(gdb) bt
#0  delayMS (millis=300) at FreeRTOS_ARM/FreeRTOS_ARM.c:17
#1  errorBlink (n=n@entry=1) at FreeRTOS_ARM/FreeRTOS_ARM.c:32
#2  0x00085cd4 in HardFault_Handler () at FreeRTOS_ARM/FreeRTOS_ARM.c:44
#3  <signal handler called>
#4  xQueueGenericReceive (xQueue=0xa5a5a5a5, pvBuffer=0x20082856, xTicksToWait=4294967295, xJustPeeking=0)
    at FreeRTOS_ARM/utility/queue.c:1193
#5  0x00085214 in BuzzerTask::task (this=0x2007f5c0) at BuzzerTask.cpp:68
#6  0x000818ec in Task::taskCaller (this=0x2007f5c0) at Task.cpp:78
#7  0x0008193e in Task::_task (self=<optimized out>) at Task.cpp:87
#8  0x000868bc in ulPortSetInterruptMask () at FreeRTOS_ARM/utility/port.c:423


I do not know the best way to access the PSP register to find the
process stack pointer. It looks like that this has to go via the target
description because it is not included in the default register list. In
the patch I follow the mechanism that GDB uses to lookup the register
number during "p $psp". 

This patch has only been tested on this single target running FreeRTOS.
I used an Olimex ARM-USB-TINY-H with OpenOCD to attach to the device.

This my first GDB patch submission and I have not been through this FSF
copyright assignment process.



2014-09-20  Jon Burgess  <jburgess777@gmail.com>

        * arm-tdep.c (arm_m_exception_cache): Handle stack switching
       to PSP during exception unwind.


 
@@ -3019,6 +3020,24 @@ arm_m_exception_cache (struct frame_info
*this_frame)
   unwound_sp = get_frame_register_unsigned (this_frame,
                                            ARM_SP_REGNUM);
 
+  /* The EXC_RETURN address indicates what type of transition
+     the CPU makes when returning from the exception. A value
+     of 0xfffffffd causes the stack pointer to switch from
+     MSP to PSP. */
+  if (this_pc == 0xfffffffd) {
+    int pspreg;
+    struct regcache *regcache;
+    struct value *pspval;
+
+    pspreg = user_reg_map_name_to_regnum (gdbarch, "psp", 3);
+    gdb_assert (pspreg != -1);
+
+    regcache = get_current_regcache ();
+    pspval = regcache_cooked_read_value (regcache, pspreg);
+    if (pspval && !value_lazy (pspval))
+      unwound_sp = value_as_address (pspval);
+  }
+
   /* The hardware saves eight 32-bit words, comprising xPSR,
      ReturnAddress, LR (R14), R12, R3, R2, R1, R0.  See details in
      "B1.5.6 Exception entry behavior" in
  

On Saturday, September 20 2014, Jon Burgess wrote:

> This my first GDB patch submission and I have not been through this FSF
> copyright assignment process.

Hi Jon,

Thanks for the patch.  I can submit the necessary form offlist so that
you can get started in the process of obtaining the copyright assignment
process.

Meanwhile, here goes a non-technical review, which is meant to fix
formatting issues.

> 2014-09-20  Jon Burgess  <jburgess777@gmail.com>
>
>         * arm-tdep.c (arm_m_exception_cache): Handle stack switching
>        to PSP during exception unwind.

This is almost perfect; the only thing missing is that the entry need to
be indented with TAB instead of spaces.  Take a look at the
gdb/ChangeLog file for examples.

> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 5cdfc5b..66a0ae8 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -3010,6 +3010,7 @@ 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 this_pc = get_frame_pc (this_frame);
>    CORE_ADDR unwound_sp;
>    LONGEST xpsr;
>  
> @@ -3019,6 +3020,24 @@ arm_m_exception_cache (struct frame_info
> *this_frame)
>    unwound_sp = get_frame_register_unsigned (this_frame,
>                                             ARM_SP_REGNUM);
>  
> +  /* The EXC_RETURN address indicates what type of transition
> +     the CPU makes when returning from the exception. A value
> +     of 0xfffffffd causes the stack pointer to switch from
> +     MSP to PSP. */

Two spaces after dot.

  /* The EXC_RETURN address indicates what type of transition
     the CPU makes when returning from the exception.  A value
     of 0xfffffffd causes the stack pointer to switch from
     MSP to PSP.  */

> +  if (this_pc == 0xfffffffd) {
> +    int pspreg;
> +    struct regcache *regcache;
> +    struct value *pspval;
> +
> +    pspreg = user_reg_map_name_to_regnum (gdbarch, "psp", 3);
> +    gdb_assert (pspreg != -1);
> +
> +    regcache = get_current_regcache ();
> +    pspval = regcache_cooked_read_value (regcache, pspreg);
> +    if (pspval && !value_lazy (pspval))
> +      unwound_sp = value_as_address (pspval);
> +  }

The GNU Coding Style says that the open/close brackets should be
indented one line below and two whitespaces after the "if" column, so it
would be:

  if (this_pc == 0xfffffffd)
    {
      int pspreg;
      struct regcache *regcache;
      struct value *pspval;

      pspreg = user_reg_map_name_to_regnum (gdbarch, "psp", 3);
      gdb_assert (pspreg != -1);

      regcache = get_current_regcache ();
      pspval = regcache_cooked_read_value (regcache, pspreg);
      if (pspval && !value_lazy (pspval))
        unwound_sp = value_as_address (pspval);
    }

Also, if a line has 8 whitespaces, they have to be converted to a TAB
char (but this is not the case here).

> +
>    /* The hardware saves eight 32-bit words, comprising xPSR,
>       ReturnAddress, LR (R14), R12, R3, R2, R1, R0.  See details in
>       "B1.5.6 Exception entry behavior" in

Cheers,
  

Jon Burgess <jburgess777@gmail.com> writes:

> +  /* The EXC_RETURN address indicates what type of transition

Nitpick: I'd like to say "EXC_RETURN (exception return address)" rather
than "EXC_RETURN address", because its value is the magic address.

> +     the CPU makes when returning from the exception. A value
> +     of 0xfffffffd causes the stack pointer to switch from
> +     MSP to PSP. */

I'd like to replace MSP and PSP with SP_main and SP_process, which are
used in the ARMv7-M ARM.

These details in your comments are described in ARMv7-M ARM.  IWBN to
add something like:

See details in "B1.5.8 Exception return behavior" in "ARMv7-M
Architecture Reference Manual".

> +  if (this_pc == 0xfffffffd) {
> +    int pspreg;
> +    struct regcache *regcache;
> +    struct value *pspval;
> +
> +    pspreg = user_reg_map_name_to_regnum (gdbarch, "psp", 3);

The main sp and process sp aren't in the gdb's target description.
Different gdb stubs (jtag probes) may name them differently.  OpenOCD
names it as "psp", but our codesourcery sprite names it as "sp_main".
We may need an array for the names of process sp, and iterate the array
to find the number by different name variants.

> +    gdb_assert (pspreg != -1);

This is too strong to me.  If the process sp isn't found, it means GDB
stub doesn't provide process sp in the target description or the name of
process sp isn't recognized by GDB.  It is not GDB's fault.  We can emit
an error here, IMO.

> +
> +    regcache = get_current_regcache ();
> +    pspval = regcache_cooked_read_value (regcache, pspreg);

In general, getting the register value in unwinding from current
regcache is wrong, because register value should be got from the
previous frame.  However, in the case that getting process sp in
exception on cortex-m, it is correct.  At this point, the program is
still in exception, and main sp is used (process sp isn't used nor changed).
When GDB is unwinding frames from the exception to the application, the
process sp register is still valid.
  

On 09/23/2014 06:26 AM, Yao Qi wrote:
> Jon Burgess <jburgess777@gmail.com> writes:
> 
>> +  /* The EXC_RETURN address indicates what type of transition
> 
> Nitpick: I'd like to say "EXC_RETURN (exception return address)" rather
> than "EXC_RETURN address", because its value is the magic address.
> 
>> +     the CPU makes when returning from the exception. A value
>> +     of 0xfffffffd causes the stack pointer to switch from
>> +     MSP to PSP. */
> 
> I'd like to replace MSP and PSP with SP_main and SP_process, which are
> used in the ARMv7-M ARM.
> 
> These details in your comments are described in ARMv7-M ARM.  IWBN to
> add something like:
> 
> See details in "B1.5.8 Exception return behavior" in "ARMv7-M
> Architecture Reference Manual".
> 
>> +  if (this_pc == 0xfffffffd) {
>> +    int pspreg;
>> +    struct regcache *regcache;
>> +    struct value *pspval;
>> +
>> +    pspreg = user_reg_map_name_to_regnum (gdbarch, "psp", 3);
> 
> The main sp and process sp aren't in the gdb's target description.
> Different gdb stubs (jtag probes) may name them differently.  OpenOCD
> names it as "psp", but our codesourcery sprite names it as "sp_main".
> We may need an array for the names of process sp, and iterate the array
> to find the number by different name variants.

Let's start by fixing this properly, please.  If GDB needs to know
about these registers, then they should be part of a known feature
in the target description.  According to B1.4 in the same document,
we see:

"The ARMv7-M profile has the following registers closely coupled to the core:

- general purpose registers R0-R12
- 2 Stack Pointer registers, SP_main and SP_process (banked versions of R13)
- the Link Register, LR (R14)
- the Program Counter, PC
- status registers for flags, exception/interrupt level, and execution state bits
- mask registers associated with managing the prioritization scheme for exceptions and interrupts
- a control register (CONTROL) to identify the current stack and thread mode privilege level.
"

Seems like even more core things other than SP_main/SP_process are
missing from org.gnu.gdb.arm.m-profile, when debugging at this level.

> 
>> +    gdb_assert (pspreg != -1);
> 
> This is too strong to me.  If the process sp isn't found, it means GDB
> stub doesn't provide process sp in the target description or the name of
> process sp isn't recognized by GDB.  It is not GDB's fault.  We can emit
> an error here, IMO.
> 
>> +
>> +    regcache = get_current_regcache ();
>> +    pspval = regcache_cooked_read_value (regcache, pspreg);
> 
> In general, getting the register value in unwinding from current
> regcache is wrong, because register value should be got from the
> previous frame.  However, in the case that getting process sp in
> exception on cortex-m, it is correct.  At this point, the program is
> still in exception, and main sp is used (process sp isn't used nor changed).
> When GDB is unwinding frames from the exception to the application, the
> process sp register is still valid.

I suppose this would still break if we have nested exceptions.

Would it not work to get the register from the frame instead of
the regcache directly?  If nothing saves/clobbers it up the frame
chain, the sentinel frame will end up unwinding/reading it from
the current regcache just the same.

Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

> Let's start by fixing this properly, please.  If GDB needs to know
> about these registers, then they should be part of a known feature
> in the target description.  According to B1.4 in the same document,
> we see:
>
> "The ARMv7-M profile has the following registers closely coupled to the core:
>
> - general purpose registers R0-R12
> - 2 Stack Pointer registers, SP_main and SP_process (banked versions of R13)
> - the Link Register, LR (R14)
> - the Program Counter, PC
> - status registers for flags, exception/interrupt level, and execution state bits
> - mask registers associated with managing the prioritization scheme
> for exceptions and interrupts
> - a control register (CONTROL) to identify the current stack and
> thread mode privilege level.
> "
>
> Seems like even more core things other than SP_main/SP_process are
> missing from org.gnu.gdb.arm.m-profile, when debugging at this level.
>

If we add more registers to feature org.gnu.gdb.arm.m-profile, xml files
provided by gdb stubs (such as openocd) have to be updated too,
otherwise GDB will reject the target description, right?  Does this
cause any compatibility issue? new gdb rejects the target description
from old openocd.

>> 
>>> +    gdb_assert (pspreg != -1);
>> 
>> This is too strong to me.  If the process sp isn't found, it means GDB
>> stub doesn't provide process sp in the target description or the name of
>> process sp isn't recognized by GDB.  It is not GDB's fault.  We can emit
>> an error here, IMO.
>> 
>>> +
>>> +    regcache = get_current_regcache ();
>>> +    pspval = regcache_cooked_read_value (regcache, pspreg);
>> 
>> In general, getting the register value in unwinding from current
>> regcache is wrong, because register value should be got from the
>> previous frame.  However, in the case that getting process sp in
>> exception on cortex-m, it is correct.  At this point, the program is
>> still in exception, and main sp is used (process sp isn't used nor changed).
>> When GDB is unwinding frames from the exception to the application, the
>> process sp register is still valid.
>
> I suppose this would still break if we have nested exceptions.
>

For nested exceptions, it returns to handler mode, so the return address
(EXC_RETURN) is 0xfffffff1.  In this case, EXC_RETURN is 0xfffffffd, it
returns to thread mode, so can't be in nested exceptions.

> Would it not work to get the register from the frame instead of
> the regcache directly?  If nothing saves/clobbers it up the frame
> chain, the sentinel frame will end up unwinding/reading it from
> the current regcache just the same.

Agreed.  It is better to get register from frame.  However, we need to
extend standard feature org.gnu.gdb.arm.m-profile first, as you said.
  

On 09/23/2014 01:22 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Let's start by fixing this properly, please.  If GDB needs to know
>> about these registers, then they should be part of a known feature
>> in the target description.  According to B1.4 in the same document,
>> we see:
>>
>> "The ARMv7-M profile has the following registers closely coupled to the core:
>>
>> - general purpose registers R0-R12
>> - 2 Stack Pointer registers, SP_main and SP_process (banked versions of R13)
>> - the Link Register, LR (R14)
>> - the Program Counter, PC
>> - status registers for flags, exception/interrupt level, and execution state bits
>> - mask registers associated with managing the prioritization scheme
>> for exceptions and interrupts
>> - a control register (CONTROL) to identify the current stack and
>> thread mode privilege level.
>> "
>>
>> Seems like even more core things other than SP_main/SP_process are
>> missing from org.gnu.gdb.arm.m-profile, when debugging at this level.
>>
> 
> If we add more registers to feature org.gnu.gdb.arm.m-profile, xml files
> provided by gdb stubs (such as openocd) have to be updated too,
> otherwise GDB will reject the target description, right?  Does this
> cause any compatibility issue? new gdb rejects the target description
> from old openocd.

Right, just making GDB expect more registers in the existing feature
would be a problem for compatibility.  So we don't add new registers to
existing features.  Instead we either:

 - create a replacement feature for "org.gnu.gdb.arm.m-profile" with
   the full set of core registers.
 - or create a separate feature for the missing registers, and have
   the reported target description include both
   the "org.gnu.gdb.arm.m-profile" feature and the new feature.

I'm guessing org.gnu.gdb.arm.m-profile doesn't include those
exception/interrupt related registers as these aren't available to
userspace programs (*) ?  Do non-M-profile ARMs have them too?

(*) BTW, if so, what does ARM call the mode that allows access
to these registers?  "kernel mode", or something else?

Thanks,
Pedro Alves
  

Pedro Alves <palves <at> redhat.com> writes:
> On 09/23/2014 01:22 PM, Yao Qi wrote:
> > Pedro Alves <palves <at> redhat.com> writes:
> > 
> >> Let's start by fixing this properly, please.

Was this ever addressed properly or any improper way?  Could somebody propose 
and implement a new feature for the target description?  It seems we're real 
close.

cheers
  simon