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

[Yao Qi <qiyaoltc at gmail dot com>]

"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 (齐尧)