Re: [PATCH] (ARM Cortex-M) FPU and PSP aware exception frame unwinder

[Luis Machado <lgustavo@codesourcery.com>]

On 04/07/2016 05:07 PM, James-Adam Renquinha Henri wrote:
> I submitted it as a bug to the GNU ARM Embedded initially, see here for
> details: https://bugs.launchpad.net/gcc-arm-embedded/+bug/1566054
>
> Basically, this patch allow gdb to unwind properly an extended stack
> frame, that is an exception frame with FPU state stacked. Additionally,
> because all Cortex-M variants have 2 stack pointers, the Main Stack
> Pointer (MSP) and the Process Stack Pointer (PSP), the code in the patch
> also check which stack was used prior to the exception. That way,
> backtraces work beautifully.
>
> In my original submission, I mentioned a known issue that I didn't try
> to fix *yet*, because that would involve a lot more work, and the impact
> is relatively minor: for a given outer frame, some FPU registers may not
> be reported correctly. I hope you don't mind too much. I consider the
> current patch still useful, because at least backtraces work, and it's
> an annoyance not to be able to get them.

I have feeling people will mind. Ideally it should keep the old behavior 
intact if possible. So if you can fallback to the old code, it should be ok.

What that said, i'm not going through the arch-specific details, just 
the more general patch.

>
> --
> James-Adam Renquinha Henri, Ing. jr
> Ingenieur d'application
> CIMEQ INC.
>
>
> patch
>
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 0412f71..8f342c1 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -470,8 +470,10 @@ 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).  */
> + /* NOTE: 0xf0000000 is the EXC_RETURN pattern, according to B1.5.8 of the
> +    ARMv7-M Reference Manual.  */
>     if (gdbarch_tdep (gdbarch)->is_m
> -      && (val & 0xfffffff0) == 0xfffffff0)
> +      && (val & 0xf0000000) == 0xf0000000)
>       return val;
>
>     if (arm_apcs_32)
> @@ -2907,13 +2909,31 @@ arm_m_exception_cache (struct frame_info *this_frame)
>     enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>     struct arm_prologue_cache *cache;
>     CORE_ADDR unwound_sp;
> +  CORE_ADDR this_lr;
>     LONGEST xpsr;
> +  int main_stack_used;
> +  int extended_frame_type;
> +  int stack_regnum;
>
>     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);
> +  /* We need LR to know: 1- if the FPU was used, 2- which stack was used.
> +     "B1.5.6 Exception entry behavior" in ARMv7-M Architecture Reference
> +     Manual Issue D (or the last one) gives the various bits in LR
> +     involved in this. NOTE: this LR is different of the stacked one.  */
> +  this_lr
> +    = get_frame_register_unsigned (this_frame,
> +				   user_reg_map_name_to_regnum (gdbarch,
> +								"lr",
> +								-1));
> +  main_stack_used = (this_lr & 0xf) != 0xd;
> +  extended_frame_type = (this_lr & (1 << 4)) == 0;
> +  stack_regnum = user_reg_map_name_to_regnum (gdbarch,
> +					      main_stack_used ? "sp" : "psp",
> +					      -1);
> +
> +  unwound_sp = get_frame_register_unsigned (this_frame, stack_regnum);
>
>     /* The hardware saves eight 32-bit words, comprising xPSR,
>        ReturnAddress, LR (R14), R12, R3, R2, R1, R0.  See details in
> @@ -2928,10 +2948,47 @@ arm_m_exception_cache (struct frame_info *this_frame)
>     cache->saved_regs[15].addr = unwound_sp + 24;
>     cache->saved_regs[ARM_PS_REGNUM].addr = unwound_sp + 28;
>
> +  if (extended_frame_type)
> +    {
> +      int s0_offset;
> +      int fpscr_offset;
> +
> +      s0_offset = user_reg_map_name_to_regnum (gdbarch, "s0", -1);
> +      fpscr_offset = user_reg_map_name_to_regnum (gdbarch, "fpscr", -1);
> +
> +      if (s0_offset == -1 || fpscr_offset == -1)
> +	{
> +	  /* Ooops. */
> +	  warning (_("can't get register offsets in cache; "
> +		     "fpu info may be wrong"));
> +	}
> +      else
> +	{
> +	  int i;
> +	  int fpu_reg_offset;
> +
> +	  fpu_reg_offset = unwound_sp + 0x20;
> +
> +	  /* XXX: This doesn't take into account the lazy stacking, see "Lazy
> +	     context save of FP state", in B1.5.7.  */

I'm almost sure we don't want to introduce FIXME's in the code. A 
fallback to the correct behavior would be ideal.

> +	  for (i = 0; i < 16; ++i, fpu_reg_offset += 4)
> +	    {
> +	      cache->saved_regs[s0_offset + i].addr = fpu_reg_offset;
> +	    }
> +	  cache->saved_regs[fpscr_offset].addr = unwound_sp + 0x60;
> +	}
> +
> +	/* Offset 0x64 is reserved */
> +	cache->prev_sp = unwound_sp + 0x68;
> +    }
> +  else
> +    {
> +      cache->prev_sp = unwound_sp + 32;
> +    }
> +

No need to have the { }'s for single line conditionals.

>     /* 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;
> @@ -2997,11 +3054,19 @@ arm_m_exception_unwind_sniffer (const struct frame_unwind *self,
>     /* 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;
> +  switch (this_pc)
> +    {
> +      case 0xffffffe1:
> +      case 0xffffffe9:
> +      case 0xffffffed:
> +      case 0xfffffff1:
> +      case 0xfffffff9:
> +      case 0xfffffffd:
> +        return 1;
>
> -  return 0;
> +      default:
> +        return 0;
> +    }

The above should probably go to a function that checks for the correct 
patterns. The code should be cleaner that way.

>   }
>
>   /* Frame unwinder for M-profile exceptions.  */