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

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

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