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

[Fredrik Hederstierna <fredrik.hederstierna@verisure.com>]

[-- 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;