RE: [PATCH 1/3] gdb/arm: Set the correct address to the FPU register on

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

Hello,

> -----Original Message-----
> From: Luis Machado <luis.machado@arm.com>
> Sent: den 19 maj 2022 10:17
> To: Torbjorn SVENSSON <torbjorn.svensson@st.com>; Christophe Lyon
> <christophe.lyon@arm.com>; Yvan ROUX - foss <yvan.roux@foss.st.com>;
> gdb-patches@sourceware.org
> Subject: Re: [PATCH 1/3] gdb/arm: Set the correct address to the FPU register
> on
> 
> On 5/18/22 20:24, Torbjorn SVENSSON wrote:
> > Hello,
> >
> >> -----Original Message-----
> >> From: Luis Machado <luis.machado@arm.com>
> >> Sent: den 17 maj 2022 17:45
> >> To: Torbjorn SVENSSON <torbjorn.svensson@st.com>; Christophe Lyon
> >> <christophe.lyon@arm.com>; Yvan ROUX - foss
> <yvan.roux@foss.st.com>;
> >> gdb-patches@sourceware.org
> >> Subject: Re: [PATCH 1/3] gdb/arm: Set the correct address to the FPU
> >> register on
> >>
> >> On 5/17/22 10:49, Torbjorn SVENSSON via Gdb-patches wrote:
> >>> Hello,
> >>>
> >>>
> >>> ST Restricted
> >>>
> >>>> -----Original Message-----
> >>>> From: Christophe Lyon <christophe.lyon@arm.com>
> >>>> Sent: den 16 maj 2022 16:47
> >>>> To: Yvan ROUX - foss <yvan.roux@foss.st.com>; gdb-
> >>>> patches@sourceware.org
> >>>> Cc: Torbjorn SVENSSON <torbjorn.svensson@st.com>
> >>>> Subject: Re: [PATCH 1/3] gdb/arm: Set the correct address to the FPU
> >>>> register on
> >>>>
> >>>> Hi!
> >>>>
> >>>>
> >>>> On 5/16/22 15:58, Yvan Roux via Gdb-patches wrote:
> >>>>> Registers offsets weren't computed from SP address.
> >>>>>
> >>>>> Signed-off-by: Torbj�rn SVENSSON <torbjorn.svensson@st.com>
> >>>>> Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
> >>>>> ---
> >>>>>     gdb/arm-tdep.c | 2 +-
> >>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> >>>>> index 49664093f00..c37254c2ce1 100644
> >>>>> --- a/gdb/arm-tdep.c
> >>>>> +++ b/gdb/arm-tdep.c
> >>>>> @@ -3475,7 +3475,7 @@ arm_m_exception_cache (struct frame_info
> >>>> *this_frame)
> >>>>>           if (tdep->have_sec_ext && !default_callee_register_stacking)
> >>>>>     	{
> >>>>>     	  /* Handle floating-point callee saved registers.  */
> >>>>> -	  fpu_regs_stack_offset = 0x90;
> >>>>> +	  fpu_regs_stack_offset = unwound_sp + 0x90;
> >>>>
> >>>> Sorry, it looks like this was my mistake when I committed
> ef273377587d.
> >>>>
> >>>> I haven't checked the manual, and I may have forgotten, but shouldn't
> >>>> this be
> >>>> fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x90?
> >>>
> >>> As I see it, it can be either
> >>> fpu_regs_stack_offset = unwound_sp + 0x90;
> >>> or
> >>> fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x68;
> >>> The offset values (0x90 and 0x68) can be seen in the "Stack Frame"
> chapter
> >> at https://developer.arm.com/documentation/100701/0200/Exception-
> >> handlers
> >>
> >> It would be nice to #define these constants and put some comments
> >> pointing at this documentation.
> >
> > While it would be "nice" to have defines, the defines would just have a
> single use and I think it might cause more clobber than it helps.
> 
> The problem here is that 0x90 doesn't translate to anything meaningful. A
> constant would provide an anchor that we can
> document properly.

I agree that this would be a good thing if it were only the 0x90 constant, but the code has ever 4th value between 0x00 and 0xD0, so it's a lot of constants that will only be used in one location of the code.
Would you like this changeset to add constants for every one of these values?

> If you're looking at this for the first time, it is hard to understand where it is
> coming from. But it is clearly incorrect now, so
> we have a chance to fix this and document it properly.
> 
> > Maybe a compromise would be to extend the function comment to point to
> the documentation and mention that the offset values are taken from it?
> 
> What would be the practical difference? GDB is getting more and more C++
> these days, and some of the code tends to be more verbose. We want the
> code to be easy to
> maintain and read.

If you think it will be easier to maintain, then we can obviously add the constants.
Please let me know what you think.

> >>> Based on the rest of the code, maybe "unwound_sp + sp_r0_offset +
> >> 0x68" is the preferred expression here as it removes the "integer callee
> >> saved" part from the equation.
> >>>
> >>>> How did you test this? Does your patch actually work?
> >>>
> >>> I've not tested the code on target (I don't know how to reliably trigger
> the
> >> code), I've just looked at how the rest of the register values are handled in
> >> the same block of code in GDB and they are all using the unwound_sp
> >> variable to identify the appropriate address.
> >>
> >> Was this spotted on visual inspection only? Would it be possible to craft
> some
> >> code that runs into an exception and then forces GDB to go through the
> >> frame restoring the registers?
> >
> > It was spotted using visual inspection yes.
> > I've been trying to create something that would reliably trigger this, but I'm
> not certain of what kind of scenario that triggers the code.
> 
> Ok. So this is not related to the bug you reported, it is a separate fix to which
> we don't have a reproducer. Could we have some sort of validation before
> pursuing it then?

The bug report is about the remaining (known) problems with the unwinding of the stack frames on Cortex-M33, and this particular issue was spotted when looking at the code to try to identify what was the reason the unwind was not correct.
The fix in this patch is really a fix for an issue introduced when the previous patches for Cortex-M33 unwinding were accepted.

Kind regards,
Torbjörn