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

[Alan Hayward <Alan.Hayward@arm.com>]

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