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

Yao Qi
2016-07-29T11:47:02Z

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