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

[Torbjorn SVENSSON <torbjorn.svensson@foss.st.com>]

Hi,

On 2022-10-21 11:20, Luis Machado wrote:
> 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:

Just a minor question:
I think the documentation mentioned that the last bit can be either 0 or 
1 and they should both be considered the same instruction as the bit is 
only for thumb mode. Maybe we should also list 0xefffffff?

>> +      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)?

I've tested it a bit on a STM32F4-Discovery board and it works better 
than current HEAD. Based on this board, I'd say LGTM.

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