[PATCH 0/3] gdb/arm: Cortex-M33 trustzone stack unwinding fixes

[PATCH 0/3] gdb/arm: Cortex-M33 trustzone stack unwinding fixes

[Yvan Roux]

Hi,

This patch series fixes some of the issues introduced by the support for
multiple stack pointer on v8-M architecture and described in:

https://sourceware.org/bugzilla/show_bug.cgi?id=29121

Tested on Cortex-M33 platform.

Yvan Roux (3):
  gdb/arm: Set the correct address to the FPU register on the stack
  gdb/arm: Fetch initial sp value prior to compare
  gdb/arm: Track msp and psp

 gdb/arm-tdep.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

-- 
2.17.1

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

[Yvan Roux]

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;
 	  for (i = 16; i < 32; i++)
 	    {
 	      cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);
-- 
2.17.1

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

[Christophe Lyon]

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?

How did you test this? Does your patch actually work?

Thanks,

Christophe



>   	  for (i = 16; i < 32; i++)
>   	    {
>   	      cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);

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

[Torbjorn SVENSSON]

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

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.

Kind regards,
Torbjörn

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

[Luis Machado]

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.

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

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

[Torbjorn SVENSSON]

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

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

Kind regards,
Torbjörn

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

[Luis Machado]

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

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

[Torbjorn SVENSSON]

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

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

[Yvan Roux]

Thnaks for the reviews, this patch is obsoleted by the coming one.

On Thu, May 19, 2022 at 03:24:46PM +0200, Torbjorn SVENSSON wrote:
> 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
> 

-- 
Y.

[PATCH 2/3] gdb/arm: Fetch initial sp value prior to compare

[Yvan Roux]

For Arm Cortex-M33 with security extensions, there are 4 different
stacks pointers (msp_s, msp_ns, psp_s, psp_ns).  In order to
identify the active one, compare the values of the different
stacks. The value of the initla sp register needs to be fetched to
perform this comparison.

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@st.com>
Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
---
 gdb/arm-tdep.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index c37254c2ce1..4aa277f5bc8 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -341,6 +341,7 @@ arm_cache_init (struct arm_prologue_cache *cache, struct frame_info *frame)
   arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
 
   arm_cache_init (cache, gdbarch);
+  cache->sp = get_frame_register_unsigned (frame, ARM_SP_REGNUM);
 
   if (tdep->have_sec_ext)
     {
-- 
2.17.1

Re: [PATCH 2/3] gdb/arm: Fetch initial sp value prior to compare

[Christophe Lyon]

Hi,

On 5/16/22 15:59, Yvan Roux via Gdb-patches wrote:
> For Arm Cortex-M33 with security extensions, there are 4 different
> stacks pointers (msp_s, msp_ns, psp_s, psp_ns).  In order to
> identify the active one, compare the values of the different
> stacks. The value of the initla sp register needs to be fetched to

Typo: "initial"

> perform this comparison.
> 
> Signed-off-by: Torbj�rn SVENSSON <torbjorn.svensson@st.com>
> Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
> ---
>   gdb/arm-tdep.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index c37254c2ce1..4aa277f5bc8 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -341,6 +341,7 @@ arm_cache_init (struct arm_prologue_cache *cache, struct frame_info *frame)
>     arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
>   
>     arm_cache_init (cache, gdbarch);
> +  cache->sp = get_frame_register_unsigned (frame, ARM_SP_REGNUM);

With such simple fixes, I always wonder "how did that somehow worked so 
far?". Can you describe the broken scenario fixed by this patch?

Thanks,

Christophe

>   
>     if (tdep->have_sec_ext)
>       {

RE: [PATCH 2/3] gdb/arm: Fetch initial sp value prior to compare

[Torbjorn SVENSSON]

Hello,



ST Restricted

> -----Original Message-----
> From: Christophe Lyon <christophe.lyon@arm.com>
> Sent: den 16 maj 2022 16:56
> To: Yvan ROUX - foss <yvan.roux@foss.st.com>; gdb-
> patches@sourceware.org
> Cc: Torbjorn SVENSSON <torbjorn.svensson@st.com>
> Subject: Re: [PATCH 2/3] gdb/arm: Fetch initial sp value prior to compare
> 
> Hi,
> 
> On 5/16/22 15:59, Yvan Roux via Gdb-patches wrote:
> > For Arm Cortex-M33 with security extensions, there are 4 different
> > stacks pointers (msp_s, msp_ns, psp_s, psp_ns).  In order to
> > identify the active one, compare the values of the different
> > stacks. The value of the initla sp register needs to be fetched to
> 
> Typo: "initial"
> 
> > perform this comparison.
> >
> > Signed-off-by: Torbj�rn SVENSSON <torbjorn.svensson@st.com>
> > Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
> > ---
> >   gdb/arm-tdep.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> > index c37254c2ce1..4aa277f5bc8 100644
> > --- a/gdb/arm-tdep.c
> > +++ b/gdb/arm-tdep.c
> > @@ -341,6 +341,7 @@ arm_cache_init (struct arm_prologue_cache
> *cache, struct frame_info *frame)
> >     arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep
> (gdbarch);
> >
> >     arm_cache_init (cache, gdbarch);
> > +  cache->sp = get_frame_register_unsigned (frame, ARM_SP_REGNUM);
> 
> With such simple fixes, I always wonder "how did that somehow worked so
> far?". Can you describe the broken scenario fixed by this patch?

When I  attempted to debug the issues in https://sourceware.org/bugzilla/show_bug.cgi?id=29121, I discovered that the arm_prologue_cache->active_sp_regnum was set to the regnum of psp_ns although psp was not used in the application. After digging a bit further, I noticed that the psp_ns and psp_s registers were both 0x0 and that arm_prologue_cache->sp was also 0x0 even though the sp register was something else.

Kind regards,
Torbjörn

[PATCH 3/3] gdb/arm: Track msp and psp

[Yvan Roux]

For Arm Cortex-M33 with security extensions, there are 4 different
stacks pointers (msp_s, msp_ns, psp_s, psp_ns).  To be compatible
with earlier Cortex-M derivates, the msp and psp register are
aliases for one of the 4 real stack pointer registers.

These are the combinations that exist:
sp -> msp -> msp_s
sp -> msp -> msp_ns
sp -> psp -> psp_s
sp -> psp -> psp_ns

This means that when the GDB client is to show the value of "msp",
the value should always be equal to either "msp_s" or "msp_ns".
Same goes for "psp".

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@st.com>
Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
---
 gdb/arm-tdep.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 4aa277f5bc8..b9c35bcdae6 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -288,6 +288,8 @@ struct arm_prologue_cache
 
   /* Active stack pointer.  */
   int active_sp_regnum;
+  int active_msp_regnum;
+  int active_psp_regnum;
 
   /* The frame base for this frame is just prev_sp - frame size.
      FRAMESIZE is the distance from the frame pointer to the
@@ -345,11 +347,23 @@ arm_cache_init (struct arm_prologue_cache *cache, struct frame_info *frame)
 
   if (tdep->have_sec_ext)
     {
+      CORE_ADDR msp_val = get_frame_register_unsigned (frame, tdep->m_profile_msp_regnum);
+      CORE_ADDR psp_val = get_frame_register_unsigned (frame, tdep->m_profile_psp_regnum);
+
       arm_cache_init_sp (tdep->m_profile_msp_s_regnum, &cache->msp_s, cache, frame);
       arm_cache_init_sp (tdep->m_profile_psp_s_regnum, &cache->psp_s, cache, frame);
       arm_cache_init_sp (tdep->m_profile_msp_ns_regnum, &cache->msp_ns, cache, frame);
       arm_cache_init_sp (tdep->m_profile_psp_ns_regnum, &cache->psp_ns, cache, frame);
 
+      if (msp_val == cache->msp_s)
+        cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
+      else if (msp_val == cache->msp_ns)
+        cache->active_msp_regnum = tdep->m_profile_msp_ns_regnum;
+      if (psp_val == cache->psp_s)
+        cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
+      else if (psp_val == cache->psp_ns)
+        cache->active_psp_regnum = tdep->m_profile_psp_ns_regnum;
+
       /* Use MSP_S as default stack pointer.  */
       if (cache->active_sp_regnum == ARM_SP_REGNUM)
 	  cache->active_sp_regnum = tdep->m_profile_msp_s_regnum;
@@ -384,6 +398,10 @@ arm_cache_get_sp_register (struct arm_prologue_cache *cache,
 	return cache->psp_s;
       if (regnum == tdep->m_profile_psp_ns_regnum)
 	return cache->psp_ns;
+      if (regnum == tdep->m_profile_msp_regnum)
+	return arm_cache_get_sp_register (cache, tdep, cache->active_msp_regnum);
+      if (regnum == tdep->m_profile_psp_regnum)
+	return arm_cache_get_sp_register (cache, tdep, cache->active_psp_regnum);
     }
   else if (tdep->is_m)
     {
-- 
2.17.1

Re: [PATCH 3/3] gdb/arm: Track msp and psp

[Lancelot SIX]

Hi,

I have no knowledge about Arm Cortex-M33 so I cannot comment on the
actual changes.  I however have a purely code-style related remark down
below.

On Mon, May 16, 2022 at 04:00:22PM +0200, Yvan Roux via Gdb-patches wrote:
> For Arm Cortex-M33 with security extensions, there are 4 different
> stacks pointers (msp_s, msp_ns, psp_s, psp_ns).  To be compatible
> with earlier Cortex-M derivates, the msp and psp register are
> aliases for one of the 4 real stack pointer registers.
> 
> These are the combinations that exist:
> sp -> msp -> msp_s
> sp -> msp -> msp_ns
> sp -> psp -> psp_s
> sp -> psp -> psp_ns
> 
> This means that when the GDB client is to show the value of "msp",
> the value should always be equal to either "msp_s" or "msp_ns".
> Same goes for "psp".
> 
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@st.com>
> Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
> ---
>  gdb/arm-tdep.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 4aa277f5bc8..b9c35bcdae6 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -288,6 +288,8 @@ struct arm_prologue_cache
>  
>    /* Active stack pointer.  */
>    int active_sp_regnum;
> +  int active_msp_regnum;
> +  int active_psp_regnum;
>  
>    /* The frame base for this frame is just prev_sp - frame size.
>       FRAMESIZE is the distance from the frame pointer to the
> @@ -345,11 +347,23 @@ arm_cache_init (struct arm_prologue_cache *cache, struct frame_info *frame)
>  
>    if (tdep->have_sec_ext)
>      {
> +      CORE_ADDR msp_val = get_frame_register_unsigned (frame, tdep->m_profile_msp_regnum);
> +      CORE_ADDR psp_val = get_frame_register_unsigned (frame, tdep->m_profile_psp_regnum);
> +
>        arm_cache_init_sp (tdep->m_profile_msp_s_regnum, &cache->msp_s, cache, frame);
>        arm_cache_init_sp (tdep->m_profile_psp_s_regnum, &cache->psp_s, cache, frame);
>        arm_cache_init_sp (tdep->m_profile_msp_ns_regnum, &cache->msp_ns, cache, frame);
>        arm_cache_init_sp (tdep->m_profile_psp_ns_regnum, &cache->psp_ns, cache, frame);
>  
> +      if (msp_val == cache->msp_s)
> +        cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
> +      else if (msp_val == cache->msp_ns)
> +        cache->active_msp_regnum = tdep->m_profile_msp_ns_regnum;
> +      if (psp_val == cache->psp_s)
> +        cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
> +      else if (psp_val == cache->psp_ns)
> +        cache->active_psp_regnum = tdep->m_profile_psp_ns_regnum;
> +

In the above section, you use 8 spaces for indentation instead on a tab
char (in the 4 lines starting with "cache->").

Best,
Lancelot.

>        /* Use MSP_S as default stack pointer.  */
>        if (cache->active_sp_regnum == ARM_SP_REGNUM)
>  	  cache->active_sp_regnum = tdep->m_profile_msp_s_regnum;
> @@ -384,6 +398,10 @@ arm_cache_get_sp_register (struct arm_prologue_cache *cache,
>  	return cache->psp_s;
>        if (regnum == tdep->m_profile_psp_ns_regnum)
>  	return cache->psp_ns;
> +      if (regnum == tdep->m_profile_msp_regnum)
> +	return arm_cache_get_sp_register (cache, tdep, cache->active_msp_regnum);
> +      if (regnum == tdep->m_profile_psp_regnum)
> +	return arm_cache_get_sp_register (cache, tdep, cache->active_psp_regnum);
>      }
>    else if (tdep->is_m)
>      {
> -- 
> 2.17.1
> 

Re: [PATCH 3/3] gdb/arm: Track msp and psp

[Luis Machado]

On 5/16/22 15:00, Yvan Roux via Gdb-patches wrote:
> For Arm Cortex-M33 with security extensions, there are 4 different
> stacks pointers (msp_s, msp_ns, psp_s, psp_ns).  To be compatible

stacks -> stack

> with earlier Cortex-M derivates, the msp and psp register are

register -> registers

> aliases for one of the 4 real stack pointer registers.
> 
> These are the combinations that exist:
> sp -> msp -> msp_s
> sp -> msp -> msp_ns
> sp -> psp -> psp_s
> sp -> psp -> psp_ns
> 
> This means that when the GDB client is to show the value of "msp",
> the value should always be equal to either "msp_s" or "msp_ns".
> Same goes for "psp".
> 

As a general comment, we should include, in the commit message, the bug ticket link related to this problem:

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29121

> Signed-off-by: Torbj�rn SVENSSON <torbjorn.svensson@st.com>
> Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
> ---
>   gdb/arm-tdep.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 4aa277f5bc8..b9c35bcdae6 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -288,6 +288,8 @@ struct arm_prologue_cache
>   
>     /* Active stack pointer.  */
>     int active_sp_regnum;
> +  int active_msp_regnum;
> +  int active_psp_regnum;

Is it possible to have 3 active SP registers at the same time? Are msp and psp really needed here or are we just not doing a good
job of figuring out what SP maps to? (msp_s, msp_ns, psp_s, psp_ns)

>   
>     /* The frame base for this frame is just prev_sp - frame size.
>        FRAMESIZE is the distance from the frame pointer to the
> @@ -345,11 +347,23 @@ arm_cache_init (struct arm_prologue_cache *cache, struct frame_info *frame)
>   
>     if (tdep->have_sec_ext)
>       {
> +      CORE_ADDR msp_val = get_frame_register_unsigned (frame, tdep->m_profile_msp_regnum);
> +      CORE_ADDR psp_val = get_frame_register_unsigned (frame, tdep->m_profile_psp_regnum);
> +
>         arm_cache_init_sp (tdep->m_profile_msp_s_regnum, &cache->msp_s, cache, frame);
>         arm_cache_init_sp (tdep->m_profile_psp_s_regnum, &cache->psp_s, cache, frame);
>         arm_cache_init_sp (tdep->m_profile_msp_ns_regnum, &cache->msp_ns, cache, frame);
>         arm_cache_init_sp (tdep->m_profile_psp_ns_regnum, &cache->psp_ns, cache, frame);
>   
> +      if (msp_val == cache->msp_s)
> +        cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
> +      else if (msp_val == cache->msp_ns)
> +        cache->active_msp_regnum = tdep->m_profile_msp_ns_regnum;
> +      if (psp_val == cache->psp_s)
> +        cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
> +      else if (psp_val == cache->psp_ns)
> +        cache->active_psp_regnum = tdep->m_profile_psp_ns_regnum;
> +
>         /* Use MSP_S as default stack pointer.  */
>         if (cache->active_sp_regnum == ARM_SP_REGNUM)
>   	  cache->active_sp_regnum = tdep->m_profile_msp_s_regnum;
> @@ -384,6 +398,10 @@ arm_cache_get_sp_register (struct arm_prologue_cache *cache,
>   	return cache->psp_s;
>         if (regnum == tdep->m_profile_psp_ns_regnum)
>   	return cache->psp_ns;
> +      if (regnum == tdep->m_profile_msp_regnum)
> +	return arm_cache_get_sp_register (cache, tdep, cache->active_msp_regnum);
> +      if (regnum == tdep->m_profile_psp_regnum)
> +	return arm_cache_get_sp_register (cache, tdep, cache->active_psp_regnum);
>       }
>     else if (tdep->is_m)
>       {

RE: [PATCH 3/3] gdb/arm: Track msp and psp

[Torbjorn SVENSSON]

ST Restricted


> -----Original Message-----
> From: Luis Machado <luis.machado@arm.com>
> Sent: den 17 maj 2022 17:53
> To: Yvan ROUX - foss <yvan.roux@foss.st.com>; gdb-
> patches@sourceware.org
> Cc: Torbjorn SVENSSON <torbjorn.svensson@st.com>
> Subject: Re: [PATCH 3/3] gdb/arm: Track msp and psp
> 
> On 5/16/22 15:00, Yvan Roux via Gdb-patches wrote:
> > For Arm Cortex-M33 with security extensions, there are 4 different
> > stacks pointers (msp_s, msp_ns, psp_s, psp_ns).  To be compatible
> 
> stacks -> stack
> 
> > with earlier Cortex-M derivates, the msp and psp register are
> 
> register -> registers
> 
> > aliases for one of the 4 real stack pointer registers.
> >
> > These are the combinations that exist:
> > sp -> msp -> msp_s
> > sp -> msp -> msp_ns
> > sp -> psp -> psp_s
> > sp -> psp -> psp_ns
> >
> > This means that when the GDB client is to show the value of "msp",
> > the value should always be equal to either "msp_s" or "msp_ns".
> > Same goes for "psp".
> >
> 
> As a general comment, we should include, in the commit message, the bug
> ticket link related to this problem:
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29121
> 
> > Signed-off-by: Torbj�rn SVENSSON <torbjorn.svensson@st.com>
> > Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
> > ---
> >   gdb/arm-tdep.c | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> > index 4aa277f5bc8..b9c35bcdae6 100644
> > --- a/gdb/arm-tdep.c
> > +++ b/gdb/arm-tdep.c
> > @@ -288,6 +288,8 @@ struct arm_prologue_cache
> >
> >     /* Active stack pointer.  */
> >     int active_sp_regnum;
> > +  int active_msp_regnum;
> > +  int active_psp_regnum;
> 
> Is it possible to have 3 active SP registers at the same time? Are msp and psp
> really needed here or are we just not doing a good
> job of figuring out what SP maps to? (msp_s, msp_ns, psp_s, psp_ns)

Only one is "active", but you can view them all at the same time.
The problem without tracking msp and psp (without suffixes) is that a simple "info registers" will not have anything to show for these 2 registers:

The following was executed in secure context of a Cortex-M33.
info r
r0             0x30000000          805306368
r1             0x74                116
r2             0x300000e8          805306600
r3             0xc00021d           201327133
r4             0x0                 0
r5             0x0                 0
r6             0x0                 0
r7             0x30017fe0          805404640
r8             0xffffffff          -1
r9             0xffffffff          -1
r10            0xffffffff          -1
r11            0xffffffff          -1
r12            0xffffffff          -1
sp             0x30017fe0          0x30017fe0
lr             0xc00ef57           201387863
pc             0xc00dc6e           0xc00dc6e <main+6>
xpsr           0x61000000          1627389952
fpscr          0x0                 0
primask        0x0                 0
basepri        0x0                 0
faultmask      0x0                 0
control        0x0                 0
msp            0x30017fe0          0x30017fe0
psp            0x0                 0x0
msp_ns         0x0                 0x0
psp_ns         0xfffffffc          0xfffffffc
msp_s          0x30017fe0          0x30017fe0
psp_s          0x0                 0x0
msplim_s       0x0                 0x0
psplim_s       0x0                 0x0
msplim_ns      0x0                 0x0
psplim_ns      0x0                 0x0
primask_s      0x0                 0
basepri_s      0x0                 0
faultmask_s    0x0                 0
control_s      0x0                 0
primask_ns     0x0                 0
basepri_ns     0x0                 0
faultmask_ns   0x0                 0
control_ns     0x0                 0


As the registers are listed, they need to be tracked, or do you see any other way to give the right value for the register?


Kind regards,
Torbjörn

Re: [PATCH 3/3] gdb/arm: Track msp and psp

[Luis Machado]

On 5/18/22 20:18, Torbjorn SVENSSON wrote:
> 
> 
> ST Restricted
> 
> 
>> -----Original Message-----
>> From: Luis Machado <luis.machado@arm.com>
>> Sent: den 17 maj 2022 17:53
>> To: Yvan ROUX - foss <yvan.roux@foss.st.com>; gdb-
>> patches@sourceware.org
>> Cc: Torbjorn SVENSSON <torbjorn.svensson@st.com>
>> Subject: Re: [PATCH 3/3] gdb/arm: Track msp and psp
>>
>> On 5/16/22 15:00, Yvan Roux via Gdb-patches wrote:
>>> For Arm Cortex-M33 with security extensions, there are 4 different
>>> stacks pointers (msp_s, msp_ns, psp_s, psp_ns).  To be compatible
>>
>> stacks -> stack
>>
>>> with earlier Cortex-M derivates, the msp and psp register are
>>
>> register -> registers
>>
>>> aliases for one of the 4 real stack pointer registers.
>>>
>>> These are the combinations that exist:
>>> sp -> msp -> msp_s
>>> sp -> msp -> msp_ns
>>> sp -> psp -> psp_s
>>> sp -> psp -> psp_ns
>>>
>>> This means that when the GDB client is to show the value of "msp",
>>> the value should always be equal to either "msp_s" or "msp_ns".
>>> Same goes for "psp".
>>>
>>
>> As a general comment, we should include, in the commit message, the bug
>> ticket link related to this problem:
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29121
>>
>>> Signed-off-by: Torbj�rn SVENSSON <torbjorn.svensson@st.com>
>>> Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
>>> ---
>>>    gdb/arm-tdep.c | 18 ++++++++++++++++++
>>>    1 file changed, 18 insertions(+)
>>>
>>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>>> index 4aa277f5bc8..b9c35bcdae6 100644
>>> --- a/gdb/arm-tdep.c
>>> +++ b/gdb/arm-tdep.c
>>> @@ -288,6 +288,8 @@ struct arm_prologue_cache
>>>
>>>      /* Active stack pointer.  */
>>>      int active_sp_regnum;
>>> +  int active_msp_regnum;
>>> +  int active_psp_regnum;
>>
>> Is it possible to have 3 active SP registers at the same time? Are msp and psp
>> really needed here or are we just not doing a good
>> job of figuring out what SP maps to? (msp_s, msp_ns, psp_s, psp_ns)
> 
> Only one is "active", but you can view them all at the same time.
> The problem without tracking msp and psp (without suffixes) is that a simple "info registers" will not have anything to show for these 2 registers:
> 

That's fine, but the naming is incorrect and possibly misleading. My understanding is that there is only SP (the active stack pointer). Yes, we can track the values of
the other registers, but the code should make that clear.

> The following was executed in secure context of a Cortex-M33.
> info r
> r0             0x30000000          805306368
> r1             0x74                116
> r2             0x300000e8          805306600
> r3             0xc00021d           201327133
> r4             0x0                 0
> r5             0x0                 0
> r6             0x0                 0
> r7             0x30017fe0          805404640
> r8             0xffffffff          -1
> r9             0xffffffff          -1
> r10            0xffffffff          -1
> r11            0xffffffff          -1
> r12            0xffffffff          -1
> sp             0x30017fe0          0x30017fe0
> lr             0xc00ef57           201387863
> pc             0xc00dc6e           0xc00dc6e <main+6>
> xpsr           0x61000000          1627389952
> fpscr          0x0                 0
> primask        0x0                 0
> basepri        0x0                 0
> faultmask      0x0                 0
> control        0x0                 0
> msp            0x30017fe0          0x30017fe0
> psp            0x0                 0x0
> msp_ns         0x0                 0x0
> psp_ns         0xfffffffc          0xfffffffc
> msp_s          0x30017fe0          0x30017fe0
> psp_s          0x0                 0x0
> msplim_s       0x0                 0x0
> psplim_s       0x0                 0x0
> msplim_ns      0x0                 0x0
> psplim_ns      0x0                 0x0
> primask_s      0x0                 0
> basepri_s      0x0                 0
> faultmask_s    0x0                 0
> control_s      0x0                 0
> primask_ns     0x0                 0
> basepri_ns     0x0                 0
> faultmask_ns   0x0                 0
> control_ns     0x0                 0
> 
> 
> As the registers are listed, they need to be tracked, or do you see any other way to give the right value for the register?

The most important thing is for GDB to produce a correct stack trace. If it is not doing that properly, then it is failing to calculate the correct SP.

I don't particularly understand what tracking msp/psp has to do with calculating the correct SP here. Maybe I'm missing some context.

> 
> 
> Kind regards,
> Torbjörn

RE: [PATCH 3/3] gdb/arm: Track msp and psp

[Torbjorn SVENSSON]

Hello,


> -----Original Message-----
> From: Luis Machado <luis.machado@arm.com>
> Sent: den 19 maj 2022 10:08
> To: Torbjorn SVENSSON <torbjorn.svensson@st.com>; Yvan ROUX - foss
> <yvan.roux@foss.st.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH 3/3] gdb/arm: Track msp and psp
> 
> On 5/18/22 20:18, Torbjorn SVENSSON wrote:
> >
> >
> > ST Restricted
> >
> >
> >> -----Original Message-----
> >> From: Luis Machado <luis.machado@arm.com>
> >> Sent: den 17 maj 2022 17:53
> >> To: Yvan ROUX - foss <yvan.roux@foss.st.com>; gdb-
> >> patches@sourceware.org
> >> Cc: Torbjorn SVENSSON <torbjorn.svensson@st.com>
> >> Subject: Re: [PATCH 3/3] gdb/arm: Track msp and psp
> >>
> >> On 5/16/22 15:00, Yvan Roux via Gdb-patches wrote:
> >>> For Arm Cortex-M33 with security extensions, there are 4 different
> >>> stacks pointers (msp_s, msp_ns, psp_s, psp_ns).  To be compatible
> >>
> >> stacks -> stack
> >>
> >>> with earlier Cortex-M derivates, the msp and psp register are
> >>
> >> register -> registers
> >>
> >>> aliases for one of the 4 real stack pointer registers.
> >>>
> >>> These are the combinations that exist:
> >>> sp -> msp -> msp_s
> >>> sp -> msp -> msp_ns
> >>> sp -> psp -> psp_s
> >>> sp -> psp -> psp_ns
> >>>
> >>> This means that when the GDB client is to show the value of "msp",
> >>> the value should always be equal to either "msp_s" or "msp_ns".
> >>> Same goes for "psp".
> >>>
> >>
> >> As a general comment, we should include, in the commit message, the
> bug
> >> ticket link related to this problem:
> >>
> >> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29121
> >>
> >>> Signed-off-by: Torbj�rn SVENSSON <torbjorn.svensson@st.com>
> >>> Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
> >>> ---
> >>>    gdb/arm-tdep.c | 18 ++++++++++++++++++
> >>>    1 file changed, 18 insertions(+)
> >>>
> >>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> >>> index 4aa277f5bc8..b9c35bcdae6 100644
> >>> --- a/gdb/arm-tdep.c
> >>> +++ b/gdb/arm-tdep.c
> >>> @@ -288,6 +288,8 @@ struct arm_prologue_cache
> >>>
> >>>      /* Active stack pointer.  */
> >>>      int active_sp_regnum;
> >>> +  int active_msp_regnum;
> >>> +  int active_psp_regnum;
> >>
> >> Is it possible to have 3 active SP registers at the same time? Are msp and
> psp
> >> really needed here or are we just not doing a good
> >> job of figuring out what SP maps to? (msp_s, msp_ns, psp_s, psp_ns)
> >
> > Only one is "active", but you can view them all at the same time.
> > The problem without tracking msp and psp (without suffixes) is that a
> simple "info registers" will not have anything to show for these 2 registers:
> >
> 
> That's fine, but the naming is incorrect and possibly misleading. My
> understanding is that there is only SP (the active stack pointer). Yes, we can
> track the values of
> the other registers, but the code should make that clear.

Depends on how you look at it.

At any given moment, then sp will have the same value as either msp or psp.
At the same time, msp must have the same value as either msp_s or msp_ns. Same goes for psp.

So, the core only "works" on sp, but the user needs to be able to see the state of all the different stack pointers at any time.

Due to the constraint in hardware that msp is either msp_s or msp_ns and that sp is either msp or psp, the value of them can never diverge in the hardware, that's why we need to keep track of what msp and psp "points" to. Do you have a better terminology that "active_msp_regnum" for the "pointer"?


> 
> > The following was executed in secure context of a Cortex-M33.
> > info r
> > r0             0x30000000          805306368
> > r1             0x74                116
> > r2             0x300000e8          805306600
> > r3             0xc00021d           201327133
> > r4             0x0                 0
> > r5             0x0                 0
> > r6             0x0                 0
> > r7             0x30017fe0          805404640
> > r8             0xffffffff          -1
> > r9             0xffffffff          -1
> > r10            0xffffffff          -1
> > r11            0xffffffff          -1
> > r12            0xffffffff          -1
> > sp             0x30017fe0          0x30017fe0
> > lr             0xc00ef57           201387863
> > pc             0xc00dc6e           0xc00dc6e <main+6>
> > xpsr           0x61000000          1627389952
> > fpscr          0x0                 0
> > primask        0x0                 0
> > basepri        0x0                 0
> > faultmask      0x0                 0
> > control        0x0                 0
> > msp            0x30017fe0          0x30017fe0
> > psp            0x0                 0x0
> > msp_ns         0x0                 0x0
> > psp_ns         0xfffffffc          0xfffffffc
> > msp_s          0x30017fe0          0x30017fe0
> > psp_s          0x0                 0x0
> > msplim_s       0x0                 0x0
> > psplim_s       0x0                 0x0
> > msplim_ns      0x0                 0x0
> > psplim_ns      0x0                 0x0
> > primask_s      0x0                 0
> > basepri_s      0x0                 0
> > faultmask_s    0x0                 0
> > control_s      0x0                 0
> > primask_ns     0x0                 0
> > basepri_ns     0x0                 0
> > faultmask_ns   0x0                 0
> > control_ns     0x0                 0
> >
> >
> > As the registers are listed, they need to be tracked, or do you see any
> other way to give the right value for the register?
> 
> The most important thing is for GDB to produce a correct stack trace. If it is
> not doing that properly, then it is failing to calculate the correct SP.

The issue this patch is attempting to correct does not have any implication on the unwinding itself, but as the previous patches by Christophe Lyon added handling of the 4 different stack pointers, the patch did not include the mandatory tracking of the msp and psp "pointers" and as a result, without this patch, an assert will be triggered when doing "info r" in some situations.

> I don't particularly understand what tracking msp/psp has to do with
> calculating the correct SP here. Maybe I'm missing some context.

Well, nothing :)

Kind regards,
Torbjörn