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

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

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.

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.

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

> 
> Kind regards,
> Torbjörn