Re: [PATCH v3] gdb/arm: Terminate frame unwinding in M-profile lockup state

[Luis Machado <luis.machado@arm.com>]

On 10/17/22 20:30, Tomas Vanek wrote:
> In the lockup state the PC value of the the outer frame is irreversibly
> lost. The other registers are intact so LR likely contains
> PC of some frame next to the outer one, but we cannot analyze
> the nearest outer frame without knowing its PC
> therefore we do not know SP fixup for this frame.
> 
> The frame unwinder possibly gets mad due to the wrong SP value.
> To prevent problems terminate unwinding if PC contains the magic
> value of the lockup state.
> 
> Example session wihtout this change,
> Cortex-M33 CPU in lockup, gdb 13.0.50.20221016-git:
> ----------------
>    (gdb) c
>    Continuing.
> 
>    Program received signal SIGINT, Interrupt.
>    0xeffffffe in ?? ()
>    (gdb) bt
>    #0  0xeffffffe in ?? ()
>    #1  0x0c000a9c in HardFault_Handler ()
>        at C:/dvl/stm32l5trustzone/GPIO_IOToggle_TrustZone/Secure/Src/stm32l5xx_it.c:99
>    #2  0x2002ffd8 in ?? ()
>    Backtrace stopped: previous frame identical to this frame (corrupt stack?)
>    (gdb)
> ----------------
> The frame #1 is at correct PC taken from LR, #2 is a total nonsense.
> 
> With the change:
> ----------------
>    (gdb) c
>    Continuing.
> 
>    Program received signal SIGINT, Interrupt.
>    warning: ARM M in lockup state, stack unwinding terminated.
>    <signal handler called>
>    (gdb) bt
>    #0  <signal handler called>
>    (gdb)
> ----------------
> 
> There is a visible drawback of emitting a warning in a cache buildnig routine
> as introduced in Torbjörn SVENSSON's
> [PATCH v4] gdb/arm: Stop unwinding on error, but do not assert
> The warning is printed just once and not repeated on each backtrace command.
> 
> v2 update: warning supressed for other frames than the innermost one.
> v3 update: boolean values and comment fixes
> 
> Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
> ---
>   gdb/arm-tdep.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index b5facae..aefd241 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -724,9 +724,30 @@ class target_arm_instruction_reader : public arm_instruction_reader
>     return 0;
>   }
>   
> +static inline bool
> +arm_m_addr_is_lockup (CORE_ADDR addr)
> +{
> +  switch (addr)
> +    {
> +      /* Values for lockup state.
> +	 For more details see "B1.5.15 Unrecoverable exception cases" in
> +	 both ARMv6-M and ARMv7-M Architecture Reference Manuals, or
> +	 see "B4.32 Lockup" in ARMv8-M Architecture Reference Manual.  */
> +      case 0xeffffffe:
> +      case 0xfffffffe:
> +      case 0xffffffff:
> +	return true;
> +
> +      default:
> +	/* Address is not lockup.  */
> +	return false;
> +    }
> +}
> +
>   /* Determine if the address specified equals any of these magic return
>      values, called EXC_RETURN, defined by the ARM v6-M, v7-M and v8-M
> -   architectures.
> +   architectures. Also include lockup magic PC value.

Formatting: Two spaces after '.'

> +   Check also for FNC_RETURN if we have v8-M security extension.

we have -> we have the

>   
>      From ARMv6-M Reference Manual B1.5.8
>      Table B1-5 Exception return behavior
> @@ -769,6 +790,9 @@ class target_arm_instruction_reader : public arm_instruction_reader
>   static int
>   arm_m_addr_is_magic (struct gdbarch *gdbarch, CORE_ADDR addr)
>   {
> +  if (arm_m_addr_is_lockup (addr))
> +    return 1;
> +
>     arm_gdbarch_tdep *tdep = gdbarch_tdep<arm_gdbarch_tdep> (gdbarch);
>     if (tdep->have_sec_ext)
>       {
> @@ -3355,6 +3379,30 @@ struct frame_unwind arm_stub_unwind = {
>        describes which bits in LR that define which stack was used prior
>        to the exception and if FPU is used (causing extended stack frame).  */
>   
> +  /* In the lockup state PC contains a lockup magic value.
> +     The PC value of the the next outer frame is irreversibly
> +     lost. The other registers are intact so LR likely contains

Formatting: Two spaces after '.'.

> +     PC of some frame next to the outer one, but we cannot analyze
> +     the next outer frame without knowing its PC
> +     therefore we do not know SP fixup for this frame.
> +     Some heuristics to resynchronize SP might be possible.
> +     For simplicity just terminate unwinding to prevent the unwinder
> +     going mad.  */

How about...

For simplicity, just terminate the unwinding to prevent it going astray and
attempting to read data/addresses it shouldn't, which may cause further issues due
to side-effects.

Does that make sense?

> +  CORE_ADDR pc = get_frame_pc (this_frame);
> +  if (arm_m_addr_is_lockup (pc))
> +    {
> +      /* The lockup can be real just in the innermost frame
> +	 as the CPU is stopped and cannot create more frames.
> +	 If we hit lockup magic PC in the other frame, it is
> +	 just a sentinel at the top of stack: do not warn then.  */
> +      if (frame_relative_level (this_frame) == 0)
> +	warning (_("ARM M in lockup state, stack unwinding terminated."));
> +
> +      /* Terminate any further stack unwinding.  */
> +      arm_cache_set_active_sp_value (cache, tdep, 0);
> +      return cache;
> +    }
> +
>     CORE_ADDR lr = get_frame_register_unsigned (this_frame, ARM_LR_REGNUM);
>   
>     /* ARMv7-M Architecture Reference "A2.3.1 Arm core registers"
> @@ -3824,11 +3872,12 @@ struct frame_unwind arm_stub_unwind = {
>     return arm_m_addr_is_magic (gdbarch, this_pc);
>   }
>   
> -/* Frame unwinder for M-profile exceptions.  */
> +/* Frame unwinder for M-profile exceptions (EXC_RETURN on stack),
> +   lockup and secure/nonsecure interstate function calls (FNC_RETURN).  */
>   
>   struct frame_unwind arm_m_exception_unwind =
>   {
> -  "arm m exception",
> +  "arm m exception lockup sec_fnc",
>     SIGTRAMP_FRAME,
>     arm_m_exception_frame_unwind_stop_reason,
>     arm_m_exception_this_id,

Torbjörn, have you managed to exercise this on your end? Does it behave as expected (stops unwinding gracefully)?

 From looking at the code, this looks OK to me, but I don't have the proper setup to exercise it.