* [PATCH 0/3] gdb/arm: ARMv8-M Secure/Non-Secure stack support fixes @ 2022-06-14 14:56 Yvan Roux 2022-06-14 14:59 ` [PATCH 1/3] gdb/arm: Fetch initial sp value prior to compare Yvan Roux ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Yvan Roux @ 2022-06-14 14:56 UTC (permalink / raw) To: gdb-patches; +Cc: Luis Machado, Torbjorn SVENSSON 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] gdb/arm: Fetch initial sp value prior to compare 2022-06-14 14:56 [PATCH 0/3] gdb/arm: ARMv8-M Secure/Non-Secure stack support fixes Yvan Roux @ 2022-06-14 14:59 ` Yvan Roux 2022-06-15 8:39 ` Luis Machado 2022-06-14 15:00 ` [PATCH 2/3] gdb/arm: Track msp and psp Yvan Roux 2022-06-14 15:01 ` [PATCH 3/3] gdb/arm: Make sp alias for one of the other stack pointers Yvan Roux 2 siblings, 1 reply; 11+ messages in thread From: Yvan Roux @ 2022-06-14 14:59 UTC (permalink / raw) To: gdb-patches; +Cc: Torbjorn SVENSSON, Luis Machado 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] gdb/arm: Fetch initial sp value prior to compare 2022-06-14 14:59 ` [PATCH 1/3] gdb/arm: Fetch initial sp value prior to compare Yvan Roux @ 2022-06-15 8:39 ` Luis Machado 2022-06-15 9:20 ` Yvan Roux 0 siblings, 1 reply; 11+ messages in thread From: Luis Machado @ 2022-06-15 8:39 UTC (permalink / raw) To: Yvan Roux, gdb-patches; +Cc: Torbjorn SVENSSON 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] gdb/arm: Fetch initial sp value prior to compare 2022-06-15 8:39 ` Luis Machado @ 2022-06-15 9:20 ` Yvan Roux 0 siblings, 0 replies; 11+ messages in thread From: Yvan Roux @ 2022-06-15 9:20 UTC (permalink / raw) To: Luis Machado, gdb-patches; +Cc: Torbjorn SVENSSON 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] gdb/arm: Track msp and psp 2022-06-14 14:56 [PATCH 0/3] gdb/arm: ARMv8-M Secure/Non-Secure stack support fixes Yvan Roux 2022-06-14 14:59 ` [PATCH 1/3] gdb/arm: Fetch initial sp value prior to compare Yvan Roux @ 2022-06-14 15:00 ` Yvan Roux 2022-06-15 8:57 ` Luis Machado 2022-06-14 15:01 ` [PATCH 3/3] gdb/arm: Make sp alias for one of the other stack pointers Yvan Roux 2 siblings, 1 reply; 11+ messages in thread From: Yvan Roux @ 2022-06-14 15:00 UTC (permalink / raw) To: gdb-patches; +Cc: Torbjorn SVENSSON, Luis Machado 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] gdb/arm: Track msp and psp 2022-06-14 15:00 ` [PATCH 2/3] gdb/arm: Track msp and psp Yvan Roux @ 2022-06-15 8:57 ` Luis Machado 2022-06-15 9:28 ` Yvan Roux 0 siblings, 1 reply; 11+ messages in thread From: Luis Machado @ 2022-06-15 8:57 UTC (permalink / raw) To: Yvan Roux, gdb-patches; +Cc: Torbjorn SVENSSON 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] gdb/arm: Track msp and psp 2022-06-15 8:57 ` Luis Machado @ 2022-06-15 9:28 ` Yvan Roux 2022-06-15 9:46 ` Torbjorn SVENSSON 0 siblings, 1 reply; 11+ messages in thread From: Yvan Roux @ 2022-06-15 9:28 UTC (permalink / raw) To: Luis Machado, gdb-patches; +Cc: Torbjorn SVENSSON 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] gdb/arm: Track msp and psp 2022-06-15 9:28 ` Yvan Roux @ 2022-06-15 9:46 ` Torbjorn SVENSSON 2022-06-15 10:15 ` Luis Machado 0 siblings, 1 reply; 11+ messages in thread From: Torbjorn SVENSSON @ 2022-06-15 9:46 UTC (permalink / raw) To: Yvan Roux, Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] gdb/arm: Track msp and psp 2022-06-15 9:46 ` Torbjorn SVENSSON @ 2022-06-15 10:15 ` Luis Machado 2022-06-15 10:46 ` Yvan Roux 0 siblings, 1 reply; 11+ messages in thread From: Luis Machado @ 2022-06-15 10:15 UTC (permalink / raw) To: Torbjorn SVENSSON, Yvan Roux, gdb-patches 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] gdb/arm: Track msp and psp 2022-06-15 10:15 ` Luis Machado @ 2022-06-15 10:46 ` Yvan Roux 0 siblings, 0 replies; 11+ messages in thread From: Yvan Roux @ 2022-06-15 10:46 UTC (permalink / raw) To: Luis Machado, gdb-patches; +Cc: Torbjorn SVENSSON 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] gdb/arm: Make sp alias for one of the other stack pointers 2022-06-14 14:56 [PATCH 0/3] gdb/arm: ARMv8-M Secure/Non-Secure stack support fixes Yvan Roux 2022-06-14 14:59 ` [PATCH 1/3] gdb/arm: Fetch initial sp value prior to compare Yvan Roux 2022-06-14 15:00 ` [PATCH 2/3] gdb/arm: Track msp and psp Yvan Roux @ 2022-06-14 15:01 ` Yvan Roux 2 siblings, 0 replies; 11+ messages in thread From: Yvan Roux @ 2022-06-14 15:01 UTC (permalink / raw) To: gdb-patches; +Cc: Torbjorn SVENSSON, Luis Machado 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-06-15 10:46 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-14 14:56 [PATCH 0/3] gdb/arm: ARMv8-M Secure/Non-Secure stack support fixes Yvan Roux 2022-06-14 14:59 ` [PATCH 1/3] gdb/arm: Fetch initial sp value prior to compare Yvan Roux 2022-06-15 8:39 ` Luis Machado 2022-06-15 9:20 ` Yvan Roux 2022-06-14 15:00 ` [PATCH 2/3] gdb/arm: Track msp and psp Yvan Roux 2022-06-15 8:57 ` Luis Machado 2022-06-15 9:28 ` Yvan Roux 2022-06-15 9:46 ` Torbjorn SVENSSON 2022-06-15 10:15 ` Luis Machado 2022-06-15 10:46 ` Yvan Roux 2022-06-14 15:01 ` [PATCH 3/3] gdb/arm: Make sp alias for one of the other stack pointers Yvan Roux
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).