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

[Alan Hayward <Alan.Hayward@arm.com>]

Ok, everything looks good to me now.

It’s fairly clear in the code where there is still work to be done.
Do you have a bugzilla account? If so, could you please raise two bugs for the two features.
If not, I can add them.

Doesn’t look like you have write access, so I’ll leave this to early next week and if there
have been no other comments then I’ll commit it.

Thanks for the patch!

Alan.


> On 6 Sep 2020, at 10:27, Fredrik Hederstierna <fredrik.hederstierna@verisure.com> wrote:
> 
> Hi,
> I updated that patch to address your comments, see below and attached patch take2
> 
>> From: Alan Hayward <Alan.Hayward@arm.com>
>> Sent: Wednesday, September 2, 2020 3:24 PM
>> To: Fredrik Hederstierna <fredrik.hederstierna@verisure.com>
> 
>> How easy is it to compile a binary that exhibits this behaviour? If so then a
>> test in testsuite/gdb.arch/ would be nice. For reference, aarch64-sighandler-regs.exp
>> is a similar test but for AArch64.
> 
> I have not had time to further look into this, its probably possible to add such a test case, but I have no possibility to do this currently unfortunately.
> 
>> Have you signed the copyright assignment?
> 
> Yes, to my understanding everything is clear.
> 
>>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>>> index 1ff47c3355..1d80e8cfc8 100644
>>> --- a/gdb/ChangeLog
>>> +++ b/gdb/ChangeLog
>>> @@ -1,3 +1,9 @@
>>> +2020-08-29  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
>>> +         Adam Renquinha <arenquinha@cimeq.qc.ca>
>>> +
>>> +     * arm-tdep.c (arm_m_exception_cache): Try use correct stack
>>> +     pointer and stack frame offset when unwinding.
>>> +
>> 
>> Ideally this part should be left separate from the patch as to prevent
>> merge issues.
> 
> Ok, removed from patch.
> 
>>> +  /* Check if main stack was used.  */
>>> +  main_stack_used = ((lr & 0xf) != 0xd);
>> 
>> This took me a while to confirm. Could you mention that you are checking for
>> SPSEL in the comment. Also, I wonder if it’s worth checking the other bits in lr.
>> Yes they should be all ones in either case. But I’d rather be a little cautious.
>> Only go into the else case if all the bits are correct.
> 
> Ok, added more clear comments and more strict bit checking.
> 
>>> +          /* Thread (process) stack could not be fetched,
>>> +             give warning and exit.  */
>>> +
>>> +          warning (_("no PSP thread stack unwinding supported, exiting."));
>> 
>> I don’t think you mean exit. Maybe just remove “exiting” from the string.
> 
> Ok, removed 'exiting'
> 
>>> +      /* 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."));
>> 
>> This means that we will always get a warning if the extended frame is used.
>> I’d rather that didn’t happen.
>> How easy would be be to check the FPCCR register and then give a warning only if
>> lazy stacking is being used?
> 
> Maybe its possible, but have to time to solve this currently, added memory address of FPCCR,
> its not a register, but probably possible to do memory reading to dig deeper into this.
> Removed warning.
> 
>>> +      /* Basic frame type used.  */
>>> +      cache->prev_sp = unwound_sp + 32;
>> 
>> The mix of hex and decimal in the function is a little glaring.
>> Could you switch this one to 0x20.
> 
> Ok, fixed.
> 
> 
> Here is ChangeLog, separated, new patch variant attached.
> 
> 2020-09-06  Fredrik Hederstierna  <fredrik.hederstierna@verisure.com>
>            Adam Renquinha <arenquinha@cimeq.qc.ca>
> 
>        * arm-tdep.c (arm_m_exception_cache): Try use correct stack
>        pointer and stack frame offset when unwinding.
> 
> 
> BR Fredrik
> <gdb-cortex-m-exception-unwind-fix2.patch>