[PATCH 0/3] gdb/arm: ARMv8-M Secure/Non-Secure stack support fixes

[PATCH 0/3] gdb/arm: ARMv8-M Secure/Non-Secure stack support fixes

[Yvan Roux]

Hi,

This patch series is a re-work and enhanced version of the one submitted to
fixe the issues with multiple stack pointers described in:

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

Yvan Roux (3):
  gdb/arm: Fetch initial sp value prior to compare
  gdb/arm: Track msp and psp
  gdb/arm: Make sp alias for one of the other stack pointers

 gdb/arm-tdep.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

-- 
2.17.1

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

[Yvan Roux]

For Arm Cortex-M33 with security extensions, there are 4 different
stack 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 initial sp register needs to be fetched to
perform this comparison.

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.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 456649afdaa..1df57f5b5c8 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

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

[Yvan Roux]

For Arm Cortex-M33 with security extensions, there are 4 different
stack pointers (msp_s, msp_ns, psp_s, psp_ns).  To be compatible
with earlier Cortex-M derivates, the msp and psp registers 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".

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

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.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 1df57f5b5c8..e497dd1b3b8 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

[PATCH 3/3] gdb/arm: Make sp alias for one of the other stack pointers

[Yvan Roux]

For Cortex-M targets, the sp register is never detached from msp or
psp, it always has the same value as one of them. Let GDB treat
ARM_SP_REGNUM as an alias similar to what is done in hardware.

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

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index e497dd1b3b8..694fe8a8859 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -385,9 +385,6 @@ static CORE_ADDR
 arm_cache_get_sp_register (struct arm_prologue_cache *cache,
 			   arm_gdbarch_tdep *tdep, int regnum)
 {
-  if (regnum == ARM_SP_REGNUM)
-    return cache->sp;
-
   if (tdep->have_sec_ext)
     {
       if (regnum == tdep->m_profile_msp_s_regnum)
@@ -402,6 +399,8 @@ arm_cache_get_sp_register (struct arm_prologue_cache *cache,
 	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);
+      if (regnum == ARM_SP_REGNUM)
+	return arm_cache_get_sp_register (cache, tdep, cache->active_sp_regnum);
     }
   else if (tdep->is_m)
     {
@@ -409,7 +408,11 @@ arm_cache_get_sp_register (struct arm_prologue_cache *cache,
 	return cache->msp_s;
       if (regnum == tdep->m_profile_psp_regnum)
 	return cache->psp_s;
+      if (regnum == ARM_SP_REGNUM)
+	return arm_cache_get_sp_register (cache, tdep, cache->active_sp_regnum);
     }
+  else if (regnum == ARM_SP_REGNUM)
+    return cache->sp;
 
   gdb_assert_not_reached ("Invalid SP selection");
 }
@@ -430,12 +433,6 @@ static void
 arm_cache_set_active_sp_value (struct arm_prologue_cache *cache,
 			       arm_gdbarch_tdep *tdep, CORE_ADDR val)
 {
-  if (cache->active_sp_regnum == ARM_SP_REGNUM)
-    {
-      cache->sp = val;
-      return;
-    }
-
   if (tdep->have_sec_ext)
     {
       if (cache->active_sp_regnum == tdep->m_profile_msp_s_regnum)
@@ -458,6 +455,11 @@ arm_cache_set_active_sp_value (struct arm_prologue_cache *cache,
 
       return;
     }
+  else if (cache->active_sp_regnum == ARM_SP_REGNUM)
+    {
+      cache->sp = val;
+      return;
+    }
 
   gdb_assert_not_reached ("Invalid SP selection");
 }
-- 
2.17.1

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

[Luis Machado]

On 6/14/22 15:59, Yvan Roux wrote:
> For Arm Cortex-M33 with security extensions, there are 4 different
> stack 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 initial sp register needs to be fetched to
> perform this comparison.
> 
> Signed-off-by: Torbj�rn SVENSSON <torbjorn.svensson@foss.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 456649afdaa..1df57f5b5c8 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)
>       {

Does that mean that before this patch the value of cache->sp was uninitialized?

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

[Luis Machado]

On 6/14/22 16:00, Yvan Roux wrote:
> For Arm Cortex-M33 with security extensions, there are 4 different
> stack pointers (msp_s, msp_ns, psp_s, psp_ns).  To be compatible
> with earlier Cortex-M derivates, the msp and psp registers 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".
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29121
> 
> Signed-off-by: Torbj�rn SVENSSON <torbjorn.svensson@foss.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 1df57f5b5c8..e497dd1b3b8 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)
>       {

Just so I understand this correctly, the problem is that we're not keeping the aliases in sync
all the time, so sp/msp/psp and the 4 other registers are going out-of-sync.

Also, although SP will be either msp or psp, we can have distinct and valid msp/psp values, right?

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

[Yvan Roux]

On Wed, Jun 15, 2022 at 09:39:21AM +0100, Luis Machado wrote:
> On 6/14/22 15:59, Yvan Roux wrote:
> > For Arm Cortex-M33 with security extensions, there are 4 different
> > stack 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 initial sp register needs to be fetched to
> > perform this comparison.
> > 
> > Signed-off-by: Torbj�rn SVENSSON <torbjorn.svensson@foss.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 456649afdaa..1df57f5b5c8 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)
> >       {
> 
> Does that mean that before this patch the value of cache->sp was uninitialized?

Yes, what Torbjorn found is that arm_prologue_cache->sp value was 0x0 due to
lack of initialization.

-- 
Y.

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

[Yvan Roux]

On Wed, Jun 15, 2022 at 09:57:22AM +0100, Luis Machado wrote:
> On 6/14/22 16:00, Yvan Roux wrote:
> > For Arm Cortex-M33 with security extensions, there are 4 different
> > stack pointers (msp_s, msp_ns, psp_s, psp_ns).  To be compatible
> > with earlier Cortex-M derivates, the msp and psp registers 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".
> > 
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29121
> > 
> > Signed-off-by: Torbj�rn SVENSSON <torbjorn.svensson@foss.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 1df57f5b5c8..e497dd1b3b8 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)
> >       {
> 
> Just so I understand this correctly, the problem is that we're not keeping the aliases in sync
> all the time, so sp/msp/psp and the 4 other registers are going out-of-sync.
> 
> Also, although SP will be either msp or psp, we can have distinct and valid msp/psp values, right?

Yes that's it, it the info provided by Torbjorn in the BZ, we can see:

#2  <signal handler called>
info r
...
sp             0x30017f30          0x30017f30
...
msp            0x30017f18          0x30017f18
psp            0x0                 0x0
msp_ns         0x2002ffd8          0x2002ffd8
psp_ns         0xfffffffc          0xfffffffc
msp_s          0x30017f18          0x30017f18
psp_s          0x0                 0x0

where msp is in sync with msp_s and psp with psp_s but SP value is different

-- 
Y.

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

[Torbjorn SVENSSON]

On 2022-06-15 11:28, Yvan Roux wrote:
> On Wed, Jun 15, 2022 at 09:57:22AM +0100, Luis Machado wrote:
>> On 6/14/22 16:00, Yvan Roux wrote:
>>> For Arm Cortex-M33 with security extensions, there are 4 different
>>> stack pointers (msp_s, msp_ns, psp_s, psp_ns).  To be compatible
>>> with earlier Cortex-M derivates, the msp and psp registers 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".
>>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29121
>>>
>>> Signed-off-by: Torbj�rn SVENSSON <torbjorn.svensson@foss.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 1df57f5b5c8..e497dd1b3b8 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)
>>>        {
>>
>> Just so I understand this correctly, the problem is that we're not keeping the aliases in sync
>> all the time, so sp/msp/psp and the 4 other registers are going out-of-sync.
>>
>> Also, although SP will be either msp or psp, we can have distinct and valid msp/psp values, right?
> 
> Yes that's it, it the info provided by Torbjorn in the BZ, we can see:
> 
> #2  <signal handler called>
> info r
> ...
> sp             0x30017f30          0x30017f30
> ...
> msp            0x30017f18          0x30017f18
> psp            0x0                 0x0
> msp_ns         0x2002ffd8          0x2002ffd8
> psp_ns         0xfffffffc          0xfffffffc
> msp_s          0x30017f18          0x30017f18
> psp_s          0x0                 0x0
> 
> where msp is in sync with msp_s and psp with psp_s but SP value is different
> 

To add a bit more context; GDB does not really use the register msp (or 
psp) internally, but they are part of the set of registers that are 
provided by the target.xml file. As a result, they will be part of the 
set of registers that is printed using the "info r" command.
Without this particular patch, GDB will hit the assert in the bottom of 
the arm_cache_get_sp_register function.

Regards,
Torbjörn

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

[Luis Machado]

On 6/15/22 10:46, Torbjorn SVENSSON wrote:
> 
> 
> On 2022-06-15 11:28, Yvan Roux wrote:
>> On Wed, Jun 15, 2022 at 09:57:22AM +0100, Luis Machado wrote:
>>> On 6/14/22 16:00, Yvan Roux wrote:
>>>> For Arm Cortex-M33 with security extensions, there are 4 different
>>>> stack pointers (msp_s, msp_ns, psp_s, psp_ns).  To be compatible
>>>> with earlier Cortex-M derivates, the msp and psp registers 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".
>>>>
>>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29121
>>>>
>>>> Signed-off-by: Torbj�rn SVENSSON <torbjorn.svensson@foss.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 1df57f5b5c8..e497dd1b3b8 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)
>>>>        {
>>>
>>> Just so I understand this correctly, the problem is that we're not keeping the aliases in sync
>>> all the time, so sp/msp/psp and the 4 other registers are going out-of-sync.
>>>
>>> Also, although SP will be either msp or psp, we can have distinct and valid msp/psp values, right?
>>
>> Yes that's it, it the info provided by Torbjorn in the BZ, we can see:
>>
>> #2  <signal handler called>
>> info r
>> ...
>> sp             0x30017f30          0x30017f30
>> ...
>> msp            0x30017f18          0x30017f18
>> psp            0x0                 0x0
>> msp_ns         0x2002ffd8          0x2002ffd8
>> psp_ns         0xfffffffc          0xfffffffc
>> msp_s          0x30017f18          0x30017f18
>> psp_s          0x0                 0x0
>>
>> where msp is in sync with msp_s and psp with psp_s but SP value is different
>>
> 
> To add a bit more context; GDB does not really use the register msp (or psp) internally, but they are part of the set of registers that are provided by the target.xml file. As a result, they will be part of the set of registers that is printed using the "info r" command.
> Without this particular patch, GDB will hit the assert in the bottom of the arm_cache_get_sp_register function.

Thanks for the explanation. Yvan, would you please add this to the commit message as well?

Otherwise, this series is OK.

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

[Yvan Roux]

On Wed, Jun 15, 2022 at 11:15:25AM +0100, Luis Machado wrote:
> On 6/15/22 10:46, Torbjorn SVENSSON wrote:
> > 
> > 
> > On 2022-06-15 11:28, Yvan Roux wrote:
> > > On Wed, Jun 15, 2022 at 09:57:22AM +0100, Luis Machado wrote:
> > > > On 6/14/22 16:00, Yvan Roux wrote:
> > > > > For Arm Cortex-M33 with security extensions, there are 4 different
> > > > > stack pointers (msp_s, msp_ns, psp_s, psp_ns).  To be compatible
> > > > > with earlier Cortex-M derivates, the msp and psp registers 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".
> > > > > 
> > > > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29121
> > > > > 
> > > > > Signed-off-by: Torbj�rn SVENSSON <torbjorn.svensson@foss.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 1df57f5b5c8..e497dd1b3b8 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)
> > > > >        {
> > > > 
> > > > Just so I understand this correctly, the problem is that we're not keeping the aliases in sync
> > > > all the time, so sp/msp/psp and the 4 other registers are going out-of-sync.
> > > > 
> > > > Also, although SP will be either msp or psp, we can have distinct and valid msp/psp values, right?
> > > 
> > > Yes that's it, it the info provided by Torbjorn in the BZ, we can see:
> > > 
> > > #2  <signal handler called>
> > > info r
> > > ...
> > > sp             0x30017f30          0x30017f30
> > > ...
> > > msp            0x30017f18          0x30017f18
> > > psp            0x0                 0x0
> > > msp_ns         0x2002ffd8          0x2002ffd8
> > > psp_ns         0xfffffffc          0xfffffffc
> > > msp_s          0x30017f18          0x30017f18
> > > psp_s          0x0                 0x0
> > > 
> > > where msp is in sync with msp_s and psp with psp_s but SP value is different
> > > 
> > 
> > To add a bit more context; GDB does not really use the register msp (or psp) internally, but they are part of the set of registers that are provided by the target.xml file. As a result, they will be part of the set of registers that is printed using the "info r" command.
> > Without this particular patch, GDB will hit the assert in the bottom of the arm_cache_get_sp_register function.
> 
> Thanks for the explanation. Yvan, would you please add this to the commit message as well?
> 
> Otherwise, this series is OK.

Sure, Thanks Luis.

-- 
Y.