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

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

> On 10 Sep 2020, at 22:00, Fredrik Hederstierna <fredrik.hederstierna@verisure.com> wrote:
> 
> Hi Alan,
> sounds great! thank for committing the patch,


Committed to head.


> I do not have any GDB Bugzilla account, so please submit bugs for the additional features.

Added:
https://sourceware.org/bugzilla/show_bug.cgi?id=26611
https://sourceware.org/bugzilla/show_bug.cgi?id=26612
https://sourceware.org/bugzilla/show_bug.cgi?id=26613


> It would be great if patch goes in before the GDB 10 branching,


Joel:
Is it ok to pull this patch across to GDB 10? (And is that something you do?)
It’s Arm only, and will only effect programs that are using special stack setups.


Alan.

> thanks! Best Regards, Fredrik
> From: Alan Hayward <Alan.Hayward@arm.com>
> Sent: Wednesday, September 9, 2020 10:12 AM
> To: Fredrik Hederstierna <fredrik.hederstierna@verisure.com>
> Cc: gdb-patches\@sourceware.org <gdb-patches@sourceware.org>; nd <nd@arm.com>; James-Adam Renquinha Henri <arenquinha@cimeq.qc.ca>
> Subject: Re: [PATCH] Fix exception stack unwinding for ARM Cortex-M
>  
> 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>