* [PATCH V2 0/2] gdb/arm: Sync sp with other *sp registers @ 2022-07-07 9:17 Yvan Roux 2022-07-07 9:19 ` [PATCH V2 1/2] gdb/arm: Rename arm_cache_is_sp_register to Yvan Roux 2022-07-07 9:21 ` [PATCH V2 2/2] gdb/arm: Sync sp with other *sp registers Yvan Roux 0 siblings, 2 replies; 13+ messages in thread From: Yvan Roux @ 2022-07-07 9:17 UTC (permalink / raw) To: gdb-patches, Luis Machado; +Cc: Torbjorn SVENSSON Hi, This the new version of the patch without the unneeded dwarf hook. It also include a cleanup of arm_cache_is_sp_register function. Yvan Roux (2): gdb/arm: Rename arm_cache_is_sp_register to arm_is_alternative_sp_register gdb/arm: Sync sp with other *sp registers gdb/arm-tdep.c | 104 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 95 insertions(+), 9 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V2 1/2] gdb/arm: Rename arm_cache_is_sp_register to 2022-07-07 9:17 [PATCH V2 0/2] gdb/arm: Sync sp with other *sp registers Yvan Roux @ 2022-07-07 9:19 ` Yvan Roux 2022-07-16 2:03 ` Luis Machado 2022-07-07 9:21 ` [PATCH V2 2/2] gdb/arm: Sync sp with other *sp registers Yvan Roux 1 sibling, 1 reply; 13+ messages in thread From: Yvan Roux @ 2022-07-07 9:19 UTC (permalink / raw) To: gdb-patches, Luis Machado; +Cc: Torbjorn SVENSSON All usage of this helper are really made to check if the register is one of the alternative SP (MSP/MSP_S/MSP_NS/PSP/PSP_S/PSP_NS) with ARM_SP_REGNUM case being handled separately. 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 | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c index 3a1b52c2380..89c2779bbb5 100644 --- a/gdb/arm-tdep.c +++ b/gdb/arm-tdep.c @@ -479,14 +479,12 @@ arm_cache_set_active_sp_value (struct arm_prologue_cache *cache, gdb_assert_not_reached ("Invalid SP selection"); } -/* Return true if REGNUM is one of the stack pointers. */ +/* Return true if REGNUM is one of the alternative stack pointers. */ static bool -arm_cache_is_sp_register (struct arm_prologue_cache *cache, - arm_gdbarch_tdep *tdep, int regnum) +arm_is_alternative_sp_register (arm_gdbarch_tdep *tdep, int regnum) { - if ((regnum == ARM_SP_REGNUM) - || (regnum == tdep->m_profile_msp_regnum) + if ((regnum == tdep->m_profile_msp_regnum) || (regnum == tdep->m_profile_msp_s_regnum) || (regnum == tdep->m_profile_msp_ns_regnum) || (regnum == tdep->m_profile_psp_regnum) @@ -503,8 +501,7 @@ static void arm_cache_switch_prev_sp (struct arm_prologue_cache *cache, arm_gdbarch_tdep *tdep, int sp_regnum) { - gdb_assert (sp_regnum != ARM_SP_REGNUM); - gdb_assert (arm_cache_is_sp_register (cache, tdep, sp_regnum)); + gdb_assert (arm_is_alternative_sp_register (tdep, sp_regnum)); if (tdep->have_sec_ext) gdb_assert (sp_regnum != tdep->m_profile_msp_regnum @@ -2347,7 +2344,7 @@ arm_prologue_prev_register (struct frame_info *this_frame, /* The value might be one of the alternative SP, if so, use the value already constructed. */ - if (arm_cache_is_sp_register (cache, tdep, prev_regnum)) + if (arm_is_alternative_sp_register (tdep, prev_regnum)) { sp_value = arm_cache_get_sp_register (cache, tdep, prev_regnum); return frame_unwind_got_constant (this_frame, prev_regnum, sp_value); @@ -3694,7 +3691,7 @@ arm_m_exception_prev_register (struct frame_info *this_frame, /* The value might be one of the alternative SP, if so, use the value already constructed. */ - if (arm_cache_is_sp_register (cache, tdep, prev_regnum)) + if (arm_is_alternative_sp_register (tdep, prev_regnum)) { sp_value = arm_cache_get_sp_register (cache, tdep, prev_regnum); return frame_unwind_got_constant (this_frame, prev_regnum, sp_value); -- 2.17.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 1/2] gdb/arm: Rename arm_cache_is_sp_register to 2022-07-07 9:19 ` [PATCH V2 1/2] gdb/arm: Rename arm_cache_is_sp_register to Yvan Roux @ 2022-07-16 2:03 ` Luis Machado 2022-07-21 13:01 ` Luis Machado 0 siblings, 1 reply; 13+ messages in thread From: Luis Machado @ 2022-07-16 2:03 UTC (permalink / raw) To: Yvan Roux, gdb-patches; +Cc: Torbjorn SVENSSON Hi, On 7/7/22 10:19, Yvan Roux wrote: > All usage of this helper are really made to check if the register is > one of the alternative SP (MSP/MSP_S/MSP_NS/PSP/PSP_S/PSP_NS) with > ARM_SP_REGNUM case being handled separately. > > 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 | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c > index 3a1b52c2380..89c2779bbb5 100644 > --- a/gdb/arm-tdep.c > +++ b/gdb/arm-tdep.c > @@ -479,14 +479,12 @@ arm_cache_set_active_sp_value (struct arm_prologue_cache *cache, > gdb_assert_not_reached ("Invalid SP selection"); > } > > -/* Return true if REGNUM is one of the stack pointers. */ > +/* Return true if REGNUM is one of the alternative stack pointers. */ > > static bool > -arm_cache_is_sp_register (struct arm_prologue_cache *cache, > - arm_gdbarch_tdep *tdep, int regnum) > +arm_is_alternative_sp_register (arm_gdbarch_tdep *tdep, int regnum) > { > - if ((regnum == ARM_SP_REGNUM) > - || (regnum == tdep->m_profile_msp_regnum) > + if ((regnum == tdep->m_profile_msp_regnum) > || (regnum == tdep->m_profile_msp_s_regnum) > || (regnum == tdep->m_profile_msp_ns_regnum) > || (regnum == tdep->m_profile_psp_regnum) > @@ -503,8 +501,7 @@ static void > arm_cache_switch_prev_sp (struct arm_prologue_cache *cache, > arm_gdbarch_tdep *tdep, int sp_regnum) > { > - gdb_assert (sp_regnum != ARM_SP_REGNUM); > - gdb_assert (arm_cache_is_sp_register (cache, tdep, sp_regnum)); > + gdb_assert (arm_is_alternative_sp_register (tdep, sp_regnum)); > > if (tdep->have_sec_ext) > gdb_assert (sp_regnum != tdep->m_profile_msp_regnum > @@ -2347,7 +2344,7 @@ arm_prologue_prev_register (struct frame_info *this_frame, > > /* The value might be one of the alternative SP, if so, use the > value already constructed. */ > - if (arm_cache_is_sp_register (cache, tdep, prev_regnum)) > + if (arm_is_alternative_sp_register (tdep, prev_regnum)) > { > sp_value = arm_cache_get_sp_register (cache, tdep, prev_regnum); > return frame_unwind_got_constant (this_frame, prev_regnum, sp_value); > @@ -3694,7 +3691,7 @@ arm_m_exception_prev_register (struct frame_info *this_frame, > > /* The value might be one of the alternative SP, if so, use the > value already constructed. */ > - if (arm_cache_is_sp_register (cache, tdep, prev_regnum)) > + if (arm_is_alternative_sp_register (tdep, prev_regnum)) > { > sp_value = arm_cache_get_sp_register (cache, tdep, prev_regnum); > return frame_unwind_got_constant (this_frame, prev_regnum, sp_value); Thanks. This LGTM. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 1/2] gdb/arm: Rename arm_cache_is_sp_register to 2022-07-16 2:03 ` Luis Machado @ 2022-07-21 13:01 ` Luis Machado 0 siblings, 0 replies; 13+ messages in thread From: Luis Machado @ 2022-07-21 13:01 UTC (permalink / raw) To: Yvan Roux, gdb-patches; +Cc: Torbjorn SVENSSON On 7/16/22 03:03, Luis Machado wrote: > Hi, > > On 7/7/22 10:19, Yvan Roux wrote: >> All usage of this helper are really made to check if the register is >> one of the alternative SP (MSP/MSP_S/MSP_NS/PSP/PSP_S/PSP_NS) with >> ARM_SP_REGNUM case being handled separately. >> >> 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 | 15 ++++++--------- >> 1 file changed, 6 insertions(+), 9 deletions(-) >> >> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c >> index 3a1b52c2380..89c2779bbb5 100644 >> --- a/gdb/arm-tdep.c >> +++ b/gdb/arm-tdep.c >> @@ -479,14 +479,12 @@ arm_cache_set_active_sp_value (struct arm_prologue_cache *cache, >> gdb_assert_not_reached ("Invalid SP selection"); >> } >> -/* Return true if REGNUM is one of the stack pointers. */ >> +/* Return true if REGNUM is one of the alternative stack pointers. */ >> static bool >> -arm_cache_is_sp_register (struct arm_prologue_cache *cache, >> - arm_gdbarch_tdep *tdep, int regnum) >> +arm_is_alternative_sp_register (arm_gdbarch_tdep *tdep, int regnum) >> { >> - if ((regnum == ARM_SP_REGNUM) >> - || (regnum == tdep->m_profile_msp_regnum) >> + if ((regnum == tdep->m_profile_msp_regnum) >> || (regnum == tdep->m_profile_msp_s_regnum) >> || (regnum == tdep->m_profile_msp_ns_regnum) >> || (regnum == tdep->m_profile_psp_regnum) >> @@ -503,8 +501,7 @@ static void >> arm_cache_switch_prev_sp (struct arm_prologue_cache *cache, >> arm_gdbarch_tdep *tdep, int sp_regnum) >> { >> - gdb_assert (sp_regnum != ARM_SP_REGNUM); >> - gdb_assert (arm_cache_is_sp_register (cache, tdep, sp_regnum)); >> + gdb_assert (arm_is_alternative_sp_register (tdep, sp_regnum)); >> if (tdep->have_sec_ext) >> gdb_assert (sp_regnum != tdep->m_profile_msp_regnum >> @@ -2347,7 +2344,7 @@ arm_prologue_prev_register (struct frame_info *this_frame, >> /* The value might be one of the alternative SP, if so, use the >> value already constructed. */ >> - if (arm_cache_is_sp_register (cache, tdep, prev_regnum)) >> + if (arm_is_alternative_sp_register (tdep, prev_regnum)) >> { >> sp_value = arm_cache_get_sp_register (cache, tdep, prev_regnum); >> return frame_unwind_got_constant (this_frame, prev_regnum, sp_value); >> @@ -3694,7 +3691,7 @@ arm_m_exception_prev_register (struct frame_info *this_frame, >> /* The value might be one of the alternative SP, if so, use the >> value already constructed. */ >> - if (arm_cache_is_sp_register (cache, tdep, prev_regnum)) >> + if (arm_is_alternative_sp_register (tdep, prev_regnum)) >> { >> sp_value = arm_cache_get_sp_register (cache, tdep, prev_regnum); >> return frame_unwind_got_constant (this_frame, prev_regnum, sp_value); > > Thanks. This LGTM. Pushed now on your behalf. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V2 2/2] gdb/arm: Sync sp with other *sp registers 2022-07-07 9:17 [PATCH V2 0/2] gdb/arm: Sync sp with other *sp registers Yvan Roux 2022-07-07 9:19 ` [PATCH V2 1/2] gdb/arm: Rename arm_cache_is_sp_register to Yvan Roux @ 2022-07-07 9:21 ` Yvan Roux 2022-07-16 2:03 ` Luis Machado 1 sibling, 1 reply; 13+ messages in thread From: Yvan Roux @ 2022-07-07 9:21 UTC (permalink / raw) To: gdb-patches, Luis Machado; +Cc: Torbjorn SVENSSON For Arm Cortex-M33 with security extensions, there are 4 different stack pointers (msp_s, msp_ns, psp_s, psp_ns), without security extensions and for other Cortex-M targets, there are 2 different stack pointers (msp and psp). With this path, sp will always be in sync with one of the real stack pointers on Arm targets that contains more than one stack pointer. 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 | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c index 89c2779bbb5..c28543229de 100644 --- a/gdb/arm-tdep.c +++ b/gdb/arm-tdep.c @@ -3812,6 +3812,85 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache, return frame_unwind_got_constant (this_frame, regnum, cpsr); default: + /* Handle the alternative SP on Cortex-M. */ + if (arm_is_alternative_sp_register (tdep, regnum)) + { + bool override_with_sp_value = false; + CORE_ADDR val; + + if (tdep->have_sec_ext) + { + CORE_ADDR sp + = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); + CORE_ADDR msp_s + = get_frame_register_unsigned (this_frame, + tdep->m_profile_msp_s_regnum); + CORE_ADDR msp_ns + = get_frame_register_unsigned (this_frame, + tdep->m_profile_msp_ns_regnum); + CORE_ADDR psp_s + = get_frame_register_unsigned (this_frame, + tdep->m_profile_psp_s_regnum); + CORE_ADDR psp_ns + = get_frame_register_unsigned (this_frame, + tdep->m_profile_psp_ns_regnum); + + if (regnum == tdep->m_profile_msp_regnum + && (msp_s == sp || msp_ns == sp)) + /* Use value of SP for MSP register. */ + override_with_sp_value = true; + else if (regnum == tdep->m_profile_msp_s_regnum && msp_s == sp) + /* Use value of SP for MSP_S register. */ + override_with_sp_value = true; + else if (regnum == tdep->m_profile_msp_ns_regnum && msp_ns == sp) + /* Use value of SP for MSP_NS register. */ + override_with_sp_value = true; + else if (regnum == tdep->m_profile_psp_regnum + && (psp_s == sp || psp_ns == sp)) + /* Use value of SP for PSP register. */ + override_with_sp_value = true; + else if (regnum == tdep->m_profile_psp_s_regnum && psp_s == sp) + /* Use value of SP for PSP_S register. */ + override_with_sp_value = true; + else if (regnum == tdep->m_profile_psp_ns_regnum && psp_ns == sp) + /* Use value of SP for PSP_NS register. */ + override_with_sp_value = true; + } + else if (tdep->is_m) + { + CORE_ADDR sp + = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); + CORE_ADDR msp + = get_frame_register_unsigned (this_frame, + tdep->m_profile_msp_regnum); + CORE_ADDR psp + = get_frame_register_unsigned (this_frame, + tdep->m_profile_psp_regnum); + + if (regnum == tdep->m_profile_msp_regnum && sp == msp) + /* Use value of SP for MSP register. */ + override_with_sp_value = true; + else if (regnum == tdep->m_profile_psp_regnum && sp == psp) + /* Use value of SP for PSP register. */ + override_with_sp_value = true; + } + + if (override_with_sp_value) + { + /* Use value of SP from previous frame. */ + struct frame_info *prev_frame = get_prev_frame(this_frame); + if (prev_frame) + val = get_frame_register_unsigned (prev_frame, ARM_SP_REGNUM); + else + val = get_frame_base(this_frame); + } + else + /* Use value for the register from previous frame. */ + val = get_frame_register_unsigned (this_frame, regnum); + + return frame_unwind_got_constant(this_frame, regnum, val); + } + internal_error (__FILE__, __LINE__, _("Unexpected register %d"), regnum); } @@ -4931,6 +5010,8 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum, struct dwarf2_frame_state_reg *reg, struct frame_info *this_frame) { + arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch); + if (is_pacbti_pseudo (gdbarch, regnum)) { /* Initialize RA_AUTH_CODE to zero. */ @@ -4950,6 +5031,14 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum, case ARM_SP_REGNUM: reg->how = DWARF2_FRAME_REG_CFA; break; + default: + /* Handle the alternative SP on Cortex-M. */ + if (arm_is_alternative_sp_register (tdep, regnum)) + { + reg->how = DWARF2_FRAME_REG_FN; + reg->loc.fn = arm_dwarf2_prev_register; + } + break; } } -- 2.17.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 2/2] gdb/arm: Sync sp with other *sp registers 2022-07-07 9:21 ` [PATCH V2 2/2] gdb/arm: Sync sp with other *sp registers Yvan Roux @ 2022-07-16 2:03 ` Luis Machado 2022-07-21 8:33 ` Torbjorn SVENSSON 0 siblings, 1 reply; 13+ messages in thread From: Luis Machado @ 2022-07-16 2:03 UTC (permalink / raw) To: Yvan Roux, gdb-patches; +Cc: Torbjorn SVENSSON Hi, On 7/7/22 10:21, Yvan Roux wrote: > For Arm Cortex-M33 with security extensions, there are 4 different > stack pointers (msp_s, msp_ns, psp_s, psp_ns), without security > extensions and for other Cortex-M targets, there are 2 different > stack pointers (msp and psp). > > With this path, sp will always be in sync with one of the real stack path -> patch > pointers on Arm targets that contains more than one stack pointer. contains -> contain. > > 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 | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 89 insertions(+) > > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c > index 89c2779bbb5..c28543229de 100644 > --- a/gdb/arm-tdep.c > +++ b/gdb/arm-tdep.c > @@ -3812,6 +3812,85 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache, > return frame_unwind_got_constant (this_frame, regnum, cpsr); > > default: > + /* Handle the alternative SP on Cortex-M. */ > + if (arm_is_alternative_sp_register (tdep, regnum)) > + { > + bool override_with_sp_value = false; > + CORE_ADDR val; > + > + if (tdep->have_sec_ext) > + { > + CORE_ADDR sp > + = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); > + CORE_ADDR msp_s > + = get_frame_register_unsigned (this_frame, > + tdep->m_profile_msp_s_regnum); > + CORE_ADDR msp_ns > + = get_frame_register_unsigned (this_frame, > + tdep->m_profile_msp_ns_regnum); > + CORE_ADDR psp_s > + = get_frame_register_unsigned (this_frame, > + tdep->m_profile_psp_s_regnum); > + CORE_ADDR psp_ns > + = get_frame_register_unsigned (this_frame, > + tdep->m_profile_psp_ns_regnum); > + > + if (regnum == tdep->m_profile_msp_regnum > + && (msp_s == sp || msp_ns == sp)) > + /* Use value of SP for MSP register. */ > + override_with_sp_value = true; > + else if (regnum == tdep->m_profile_msp_s_regnum && msp_s == sp) > + /* Use value of SP for MSP_S register. */ > + override_with_sp_value = true; > + else if (regnum == tdep->m_profile_msp_ns_regnum && msp_ns == sp) > + /* Use value of SP for MSP_NS register. */ > + override_with_sp_value = true; > + else if (regnum == tdep->m_profile_psp_regnum > + && (psp_s == sp || psp_ns == sp)) > + /* Use value of SP for PSP register. */ > + override_with_sp_value = true; > + else if (regnum == tdep->m_profile_psp_s_regnum && psp_s == sp) > + /* Use value of SP for PSP_S register. */ > + override_with_sp_value = true; > + else if (regnum == tdep->m_profile_psp_ns_regnum && psp_ns == sp) > + /* Use value of SP for PSP_NS register. */ > + override_with_sp_value = true; Can the above be simplified in some way? They outcomes are the same, so maybe we could have a single condition. > + } > + else if (tdep->is_m) > + { > + CORE_ADDR sp > + = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); > + CORE_ADDR msp > + = get_frame_register_unsigned (this_frame, > + tdep->m_profile_msp_regnum); > + CORE_ADDR psp > + = get_frame_register_unsigned (this_frame, > + tdep->m_profile_psp_regnum); > + > + if (regnum == tdep->m_profile_msp_regnum && sp == msp) > + /* Use value of SP for MSP register. */ > + override_with_sp_value = true; > + else if (regnum == tdep->m_profile_psp_regnum && sp == psp) > + /* Use value of SP for PSP register. */ > + override_with_sp_value = true; > + } Same here. > + > + if (override_with_sp_value) > + { > + /* Use value of SP from previous frame. */ > + struct frame_info *prev_frame = get_prev_frame(this_frame); Formatting: Space before ( > + if (prev_frame) > + val = get_frame_register_unsigned (prev_frame, ARM_SP_REGNUM); > + else > + val = get_frame_base(this_frame); Formatting: Space before ( > + } > + else > + /* Use value for the register from previous frame. */ > + val = get_frame_register_unsigned (this_frame, regnum); > + > + return frame_unwind_got_constant(this_frame, regnum, val); Formatting: Space before ( Please check for other occurrences. > + } > + > internal_error (__FILE__, __LINE__, > _("Unexpected register %d"), regnum); > } > @@ -4931,6 +5010,8 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum, > struct dwarf2_frame_state_reg *reg, > struct frame_info *this_frame) > { > + arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch); > + > if (is_pacbti_pseudo (gdbarch, regnum)) > { > /* Initialize RA_AUTH_CODE to zero. */ > @@ -4950,6 +5031,14 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum, > case ARM_SP_REGNUM: > reg->how = DWARF2_FRAME_REG_CFA; > break; > + default: > + /* Handle the alternative SP on Cortex-M. */ Maybe "Handle the alternative SP registers on Cortex-M."? > + if (arm_is_alternative_sp_register (tdep, regnum)) > + { > + reg->how = DWARF2_FRAME_REG_FN; > + reg->loc.fn = arm_dwarf2_prev_register; > + } > + break; > } > } > Otherwise this LGTM. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 2/2] gdb/arm: Sync sp with other *sp registers 2022-07-16 2:03 ` Luis Machado @ 2022-07-21 8:33 ` Torbjorn SVENSSON 2022-07-21 8:48 ` Luis Machado 0 siblings, 1 reply; 13+ messages in thread From: Torbjorn SVENSSON @ 2022-07-21 8:33 UTC (permalink / raw) To: Luis Machado, Yvan Roux, gdb-patches Hi, On 2022-07-16 04:03, Luis Machado wrote: > Hi, > > On 7/7/22 10:21, Yvan Roux wrote: >> For Arm Cortex-M33 with security extensions, there are 4 different >> stack pointers (msp_s, msp_ns, psp_s, psp_ns), without security >> extensions and for other Cortex-M targets, there are 2 different >> stack pointers (msp and psp). >> >> With this path, sp will always be in sync with one of the real stack > > path -> patch >> pointers on Arm targets that contains more than one stack pointer. > > contains -> contain. > >> >> 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 | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 89 insertions(+) >> >> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c >> index 89c2779bbb5..c28543229de 100644 >> --- a/gdb/arm-tdep.c >> +++ b/gdb/arm-tdep.c >> @@ -3812,6 +3812,85 @@ arm_dwarf2_prev_register (struct frame_info >> *this_frame, void **this_cache, >> return frame_unwind_got_constant (this_frame, regnum, cpsr); >> default: >> + /* Handle the alternative SP on Cortex-M. */ >> + if (arm_is_alternative_sp_register (tdep, regnum)) >> + { >> + bool override_with_sp_value = false; >> + CORE_ADDR val; >> + >> + if (tdep->have_sec_ext) >> + { >> + CORE_ADDR sp >> + = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); >> + CORE_ADDR msp_s >> + = get_frame_register_unsigned (this_frame, >> + tdep->m_profile_msp_s_regnum); >> + CORE_ADDR msp_ns >> + = get_frame_register_unsigned (this_frame, >> + tdep->m_profile_msp_ns_regnum); >> + CORE_ADDR psp_s >> + = get_frame_register_unsigned (this_frame, >> + tdep->m_profile_psp_s_regnum); >> + CORE_ADDR psp_ns >> + = get_frame_register_unsigned (this_frame, >> + tdep->m_profile_psp_ns_regnum); >> + >> + if (regnum == tdep->m_profile_msp_regnum >> + && (msp_s == sp || msp_ns == sp)) >> + /* Use value of SP for MSP register. */ >> + override_with_sp_value = true; >> + else if (regnum == tdep->m_profile_msp_s_regnum && msp_s == >> sp) >> + /* Use value of SP for MSP_S register. */ >> + override_with_sp_value = true; >> + else if (regnum == tdep->m_profile_msp_ns_regnum && msp_ns >> == sp) >> + /* Use value of SP for MSP_NS register. */ >> + override_with_sp_value = true; >> + else if (regnum == tdep->m_profile_psp_regnum >> + && (psp_s == sp || psp_ns == sp)) >> + /* Use value of SP for PSP register. */ >> + override_with_sp_value = true; >> + else if (regnum == tdep->m_profile_psp_s_regnum && psp_s == >> sp) >> + /* Use value of SP for PSP_S register. */ >> + override_with_sp_value = true; >> + else if (regnum == tdep->m_profile_psp_ns_regnum && psp_ns >> == sp) >> + /* Use value of SP for PSP_NS register. */ >> + override_with_sp_value = true; > > Can the above be simplified in some way? They outcomes are the same, so > maybe we > could have a single condition. Well, it could be combined to a gigantic expression, but I'm not sure it would be as reader friendly. It's 6 distinct cases where the override should happen. Do you think it would be better to have a long expression like this? override_with_sp_value = ((regnum == tdep->m_profile_msp_regnum && (msp_s == sp || msp_ns == sp)) || (regnum == tdep->m_profile_msp_s_regnum && msp_s == sp) || (regnum == tdep->m_profile_msp_ns_regnum && msp_ns == sp) || (regnum == tdep->m_profile_psp_regnum && (psp_s == sp || psp_ns == sp)) || (regnum == tdep->m_profile_psp_s_regnum && psp_s == sp) || (regnum == tdep->m_profile_psp_ns_regnum && psp_ns == sp)); I suppose one could create the bool expressions too, like: bool isMsp = (regnum == tdep->m_profile_msp_regnum && (msp_s == sp || msp_ns == sp); bool isMspS = (regnum == tdep->m_profile_msp_s_regnum && msp_s == sp); bool isMpsNs = (regnum == tdep->m_profile_msp_ns_regnum && msp_ns == sp); bool isPsp = (regnum == tdep->m_profile_psp_regnum && (psp_s == sp || psp_ns == sp)); bool isPspS = (regnum == tdep->m_profile_psp_s_regnum && psp_s == sp); bool isPspNs = (regnum == tdep->m_profile_psp_ns_regnum && psp_ns == sp); override_with_sp_value = (isMsp || isMspS || isMspNs || isPsp || isPspS || isPspNs); Would one of the above be more appropriate? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 2/2] gdb/arm: Sync sp with other *sp registers 2022-07-21 8:33 ` Torbjorn SVENSSON @ 2022-07-21 8:48 ` Luis Machado 2022-07-22 20:59 ` [PATCH V3 0/2] " Torbjörn SVENSSON ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Luis Machado @ 2022-07-21 8:48 UTC (permalink / raw) To: Torbjorn SVENSSON, Yvan Roux, gdb-patches Hi Torbjorn, On 7/21/22 09:33, Torbjorn SVENSSON wrote: > Hi, > > On 2022-07-16 04:03, Luis Machado wrote: >> Hi, >> >> On 7/7/22 10:21, Yvan Roux wrote: >>> For Arm Cortex-M33 with security extensions, there are 4 different >>> stack pointers (msp_s, msp_ns, psp_s, psp_ns), without security >>> extensions and for other Cortex-M targets, there are 2 different >>> stack pointers (msp and psp). >>> >>> With this path, sp will always be in sync with one of the real stack >> >> path -> patch >>> pointers on Arm targets that contains more than one stack pointer. >> >> contains -> contain. >> >>> >>> 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 | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 89 insertions(+) >>> >>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c >>> index 89c2779bbb5..c28543229de 100644 >>> --- a/gdb/arm-tdep.c >>> +++ b/gdb/arm-tdep.c >>> @@ -3812,6 +3812,85 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache, >>> return frame_unwind_got_constant (this_frame, regnum, cpsr); >>> default: >>> + /* Handle the alternative SP on Cortex-M. */ >>> + if (arm_is_alternative_sp_register (tdep, regnum)) >>> + { >>> + bool override_with_sp_value = false; >>> + CORE_ADDR val; >>> + >>> + if (tdep->have_sec_ext) >>> + { >>> + CORE_ADDR sp >>> + = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); >>> + CORE_ADDR msp_s >>> + = get_frame_register_unsigned (this_frame, >>> + tdep->m_profile_msp_s_regnum); >>> + CORE_ADDR msp_ns >>> + = get_frame_register_unsigned (this_frame, >>> + tdep->m_profile_msp_ns_regnum); >>> + CORE_ADDR psp_s >>> + = get_frame_register_unsigned (this_frame, >>> + tdep->m_profile_psp_s_regnum); >>> + CORE_ADDR psp_ns >>> + = get_frame_register_unsigned (this_frame, >>> + tdep->m_profile_psp_ns_regnum); >>> + >>> + if (regnum == tdep->m_profile_msp_regnum >>> + && (msp_s == sp || msp_ns == sp)) >>> + /* Use value of SP for MSP register. */ >>> + override_with_sp_value = true; >>> + else if (regnum == tdep->m_profile_msp_s_regnum && msp_s == sp) >>> + /* Use value of SP for MSP_S register. */ >>> + override_with_sp_value = true; >>> + else if (regnum == tdep->m_profile_msp_ns_regnum && msp_ns == sp) >>> + /* Use value of SP for MSP_NS register. */ >>> + override_with_sp_value = true; >>> + else if (regnum == tdep->m_profile_psp_regnum >>> + && (psp_s == sp || psp_ns == sp)) >>> + /* Use value of SP for PSP register. */ >>> + override_with_sp_value = true; >>> + else if (regnum == tdep->m_profile_psp_s_regnum && psp_s == sp) >>> + /* Use value of SP for PSP_S register. */ >>> + override_with_sp_value = true; >>> + else if (regnum == tdep->m_profile_psp_ns_regnum && psp_ns == sp) >>> + /* Use value of SP for PSP_NS register. */ >>> + override_with_sp_value = true; >> >> Can the above be simplified in some way? They outcomes are the same, so maybe we >> could have a single condition. > > Well, it could be combined to a gigantic expression, but I'm not sure it would be as reader friendly. It's 6 distinct cases where the override should happen. > > Do you think it would be better to have a long expression like this? > > override_with_sp_value = ((regnum == tdep->m_profile_msp_regnum && (msp_s == sp || msp_ns == sp)) || (regnum == tdep->m_profile_msp_s_regnum && msp_s == sp) || (regnum == tdep->m_profile_msp_ns_regnum && msp_ns == sp) || (regnum == tdep->m_profile_psp_regnum && (psp_s == sp || psp_ns == sp)) || (regnum == tdep->m_profile_psp_s_regnum && psp_s == sp) || (regnum == tdep->m_profile_psp_ns_regnum && psp_ns == sp)); > > > I suppose one could create the bool expressions too, like: > > bool isMsp = (regnum == tdep->m_profile_msp_regnum && (msp_s == sp || msp_ns == sp); > bool isMspS = (regnum == tdep->m_profile_msp_s_regnum && msp_s == sp); > bool isMpsNs = (regnum == tdep->m_profile_msp_ns_regnum && msp_ns == sp); > bool isPsp = (regnum == tdep->m_profile_psp_regnum && (psp_s == sp || psp_ns == sp)); > bool isPspS = (regnum == tdep->m_profile_psp_s_regnum && psp_s == sp); > bool isPspNs = (regnum == tdep->m_profile_psp_ns_regnum && psp_ns == sp); > > override_with_sp_value = (isMsp || isMspS || isMspNs || isPsp || isPspS || isPspNs); > > Would one of the above be more appropriate? The latter looks better to me, as we have multiple bools and then set override_with_sp_value based on them. The previous if/else chain makes me expect different outcomes for each individual block. Maybe it's just me. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V3 0/2] gdb/arm: Sync sp with other *sp registers 2022-07-21 8:48 ` Luis Machado @ 2022-07-22 20:59 ` Torbjörn SVENSSON 2022-07-22 20:59 ` [PATCH v3 1/2] gdb/arm: Use if-else if instead of switch Torbjörn SVENSSON 2022-07-22 20:59 ` [PATCH v3 2/2] gdb/arm: Sync sp with other *sp registers Torbjörn SVENSSON 2 siblings, 0 replies; 13+ messages in thread From: Torbjörn SVENSSON @ 2022-07-22 20:59 UTC (permalink / raw) To: gdb-patches Hi, This is a new version of the patch with the eith if-statements replaced with 10 of assignemtns instead. It also include a cleanup that converts the existing swtich statment to if-else statements as the alternative SP register handling on Arm Contex-M does not have known register numbers at compile time. To avoid the confusion; the patch series is a joint effort from me an Yvan and thus, both names exist in the patches. Yvan Roux (2): gdb/arm: Use if-else if instead of switch gdb/arm: Sync sp with other *sp registers gdb/arm-tdep.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 89 insertions(+), 14 deletions(-) ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/2] gdb/arm: Use if-else if instead of switch 2022-07-21 8:48 ` Luis Machado 2022-07-22 20:59 ` [PATCH V3 0/2] " Torbjörn SVENSSON @ 2022-07-22 20:59 ` Torbjörn SVENSSON 2022-07-25 9:48 ` Luis Machado 2022-07-22 20:59 ` [PATCH v3 2/2] gdb/arm: Sync sp with other *sp registers Torbjörn SVENSSON 2 siblings, 1 reply; 13+ messages in thread From: Torbjörn SVENSSON @ 2022-07-22 20:59 UTC (permalink / raw) To: gdb-patches; +Cc: Torbjörn SVENSSON, Yvan Roux As the register number for the alternative Arm SP register are not contstant, it's not possible to use switch statement to define the rules. In order to not have a mix, replace the few existing switch-statement with regular if-else if statments. This patch is a preparation for the next one in the series. Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com> --- gdb/arm-tdep.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c index d3b4fce98a3..7d0944f9e3f 100644 --- a/gdb/arm-tdep.c +++ b/gdb/arm-tdep.c @@ -3785,9 +3785,8 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache, CORE_ADDR lr; ULONGEST cpsr; - switch (regnum) + if (regnum == ARM_PC_REGNUM) { - case ARM_PC_REGNUM: /* The PC is normally copied from the return column, which describes saves of LR. However, that version may have an extra bit set to indicate Thumb state. The bit is not @@ -3807,18 +3806,18 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache, lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM); return frame_unwind_got_constant (this_frame, regnum, arm_addr_bits_remove (gdbarch, lr)); - - case ARM_PS_REGNUM: + } + else if (regnum == ARM_PS_REGNUM) + { /* Reconstruct the T bit; see arm_prologue_prev_register for details. */ cpsr = get_frame_register_unsigned (this_frame, regnum); lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM); cpsr = reconstruct_t_bit (gdbarch, lr, cpsr); return frame_unwind_got_constant (this_frame, regnum, cpsr); - - default: - internal_error (__FILE__, __LINE__, - _("Unexpected register %d"), regnum); } + + internal_error (__FILE__, __LINE__, + _("Unexpected register %d"), regnum); } /* Implement the stack_frame_destroyed_p gdbarch method. */ @@ -4944,17 +4943,13 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum, return; } - switch (regnum) + if (regnum == ARM_PC_REGNUM || regnum == ARM_PS_REGNUM) { - case ARM_PC_REGNUM: - case ARM_PS_REGNUM: reg->how = DWARF2_FRAME_REG_FN; reg->loc.fn = arm_dwarf2_prev_register; - break; - case ARM_SP_REGNUM: - reg->how = DWARF2_FRAME_REG_CFA; - break; } + else if (regnum == ARM_SP_REGNUM) + reg->how = DWARF2_FRAME_REG_CFA; } /* Given BUF, which is OLD_LEN bytes ending at ENDADDR, expand -- 2.25.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] gdb/arm: Use if-else if instead of switch 2022-07-22 20:59 ` [PATCH v3 1/2] gdb/arm: Use if-else if instead of switch Torbjörn SVENSSON @ 2022-07-25 9:48 ` Luis Machado 0 siblings, 0 replies; 13+ messages in thread From: Luis Machado @ 2022-07-25 9:48 UTC (permalink / raw) To: Torbjörn SVENSSON, gdb-patches Hi, On 7/22/22 21:59, Torbjörn SVENSSON via Gdb-patches wrote: > As the register number for the alternative Arm SP register are not "As the register numbers for the alternative Arm SP registers..." > contstant, it's not possible to use switch statement to define the contstant -> constant > rules. In order to not have a mix, replace the few existing > switch-statement with regular if-else if statments. switch-statement -> switch statements statments -> statements > This patch is a preparation for the next one in the series. > > Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com> > --- > gdb/arm-tdep.c | 25 ++++++++++--------------- > 1 file changed, 10 insertions(+), 15 deletions(-) > > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c > index d3b4fce98a3..7d0944f9e3f 100644 > --- a/gdb/arm-tdep.c > +++ b/gdb/arm-tdep.c > @@ -3785,9 +3785,8 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache, > CORE_ADDR lr; > ULONGEST cpsr; > > - switch (regnum) > + if (regnum == ARM_PC_REGNUM) > { > - case ARM_PC_REGNUM: > /* The PC is normally copied from the return column, which > describes saves of LR. However, that version may have an > extra bit set to indicate Thumb state. The bit is not > @@ -3807,18 +3806,18 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache, > lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM); > return frame_unwind_got_constant (this_frame, regnum, > arm_addr_bits_remove (gdbarch, lr)); > - > - case ARM_PS_REGNUM: > + } > + else if (regnum == ARM_PS_REGNUM) > + { > /* Reconstruct the T bit; see arm_prologue_prev_register for details. */ > cpsr = get_frame_register_unsigned (this_frame, regnum); > lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM); > cpsr = reconstruct_t_bit (gdbarch, lr, cpsr); > return frame_unwind_got_constant (this_frame, regnum, cpsr); > - > - default: > - internal_error (__FILE__, __LINE__, > - _("Unexpected register %d"), regnum); > } > + > + internal_error (__FILE__, __LINE__, > + _("Unexpected register %d"), regnum); > } > > /* Implement the stack_frame_destroyed_p gdbarch method. */ > @@ -4944,17 +4943,13 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum, > return; > } > > - switch (regnum) > + if (regnum == ARM_PC_REGNUM || regnum == ARM_PS_REGNUM) > { > - case ARM_PC_REGNUM: > - case ARM_PS_REGNUM: > reg->how = DWARF2_FRAME_REG_FN; > reg->loc.fn = arm_dwarf2_prev_register; > - break; > - case ARM_SP_REGNUM: > - reg->how = DWARF2_FRAME_REG_CFA; > - break; > } > + else if (regnum == ARM_SP_REGNUM) > + reg->how = DWARF2_FRAME_REG_CFA; > } > > /* Given BUF, which is OLD_LEN bytes ending at ENDADDR, expand LGTM with the above fixes. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 2/2] gdb/arm: Sync sp with other *sp registers 2022-07-21 8:48 ` Luis Machado 2022-07-22 20:59 ` [PATCH V3 0/2] " Torbjörn SVENSSON 2022-07-22 20:59 ` [PATCH v3 1/2] gdb/arm: Use if-else if instead of switch Torbjörn SVENSSON @ 2022-07-22 20:59 ` Torbjörn SVENSSON 2022-07-25 9:52 ` Luis Machado 2 siblings, 1 reply; 13+ messages in thread From: Torbjörn SVENSSON @ 2022-07-22 20:59 UTC (permalink / raw) To: gdb-patches; +Cc: Yvan Roux, Torbjörn SVENSSON From: Yvan Roux <yvan.roux@foss.st.com> For Arm Cortex-M33 with security extensions, there are 4 different stack pointers (msp_s, msp_ns, psp_s, psp_ns), without security extensions and for other Cortex-M targets, there are 2 different stack pointers (msp and psp). With this patch, sp will always be in sync with one of the real stack pointers on Arm targets that contain more than one stack pointer. 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 | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c index 7d0944f9e3f..eaec2fd6b14 100644 --- a/gdb/arm-tdep.c +++ b/gdb/arm-tdep.c @@ -3815,6 +3815,78 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache, cpsr = reconstruct_t_bit (gdbarch, lr, cpsr); return frame_unwind_got_constant (this_frame, regnum, cpsr); } + else if (arm_is_alternative_sp_register (tdep, regnum)) + { + /* Handle the alternative SP registers on Cortex-M. */ + bool override_with_sp_value = false; + CORE_ADDR val; + + if (tdep->have_sec_ext) + { + CORE_ADDR sp + = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); + CORE_ADDR msp_s + = get_frame_register_unsigned (this_frame, + tdep->m_profile_msp_s_regnum); + CORE_ADDR msp_ns + = get_frame_register_unsigned (this_frame, + tdep->m_profile_msp_ns_regnum); + CORE_ADDR psp_s + = get_frame_register_unsigned (this_frame, + tdep->m_profile_psp_s_regnum); + CORE_ADDR psp_ns + = get_frame_register_unsigned (this_frame, + tdep->m_profile_psp_ns_regnum); + + bool is_msp = regnum == tdep->m_profile_msp_regnum + && (msp_s == sp || msp_ns == sp); + bool is_msp_s = regnum == tdep->m_profile_msp_s_regnum + && msp_s == sp; + bool is_msp_ns = regnum == tdep->m_profile_msp_ns_regnum + && msp_ns == sp; + bool is_psp = regnum == tdep->m_profile_psp_regnum + && (psp_s == sp || psp_ns == sp); + bool is_psp_s = regnum == tdep->m_profile_psp_s_regnum + && psp_s == sp; + bool is_psp_ns = regnum == tdep->m_profile_psp_ns_regnum + && psp_ns == sp; + + override_with_sp_value = is_msp || is_msp_s || is_msp_ns + || is_psp || is_psp_s || is_psp_ns; + + } + else if (tdep->is_m) + { + CORE_ADDR sp + = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); + CORE_ADDR msp + = get_frame_register_unsigned (this_frame, + tdep->m_profile_msp_regnum); + CORE_ADDR psp + = get_frame_register_unsigned (this_frame, + tdep->m_profile_psp_regnum); + + bool is_msp = regnum == tdep->m_profile_msp_regnum && sp == msp; + bool is_psp = regnum == tdep->m_profile_psp_regnum && sp == psp; + + override_with_sp_value = is_msp || is_psp; + } + + if (override_with_sp_value) + { + /* Use value of SP from previous frame. */ + struct frame_info *prev_frame = get_prev_frame (this_frame); + if (prev_frame) + val = get_frame_register_unsigned (prev_frame, ARM_SP_REGNUM); + else + val = get_frame_base (this_frame); + } + else + /* Use value for the register from previous frame. */ + val = get_frame_register_unsigned (this_frame, regnum); + + return frame_unwind_got_constant (this_frame, regnum, val); + } internal_error (__FILE__, __LINE__, _("Unexpected register %d"), regnum); @@ -4934,6 +5006,8 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum, struct dwarf2_frame_state_reg *reg, struct frame_info *this_frame) { + arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch); + if (is_pacbti_pseudo (gdbarch, regnum)) { /* Initialize RA_AUTH_CODE to zero. */ @@ -4950,6 +5024,12 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum, } else if (regnum == ARM_SP_REGNUM) reg->how = DWARF2_FRAME_REG_CFA; + else if (arm_is_alternative_sp_register (tdep, regnum)) + { + /* Handle the alternative SP registers on Cortex-M. */ + reg->how = DWARF2_FRAME_REG_FN; + reg->loc.fn = arm_dwarf2_prev_register; + } } /* Given BUF, which is OLD_LEN bytes ending at ENDADDR, expand -- 2.25.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] gdb/arm: Sync sp with other *sp registers 2022-07-22 20:59 ` [PATCH v3 2/2] gdb/arm: Sync sp with other *sp registers Torbjörn SVENSSON @ 2022-07-25 9:52 ` Luis Machado 0 siblings, 0 replies; 13+ messages in thread From: Luis Machado @ 2022-07-25 9:52 UTC (permalink / raw) To: Torbjörn SVENSSON, gdb-patches On 7/22/22 21:59, Torbjörn SVENSSON via Gdb-patches wrote: > From: Yvan Roux <yvan.roux@foss.st.com> > > For Arm Cortex-M33 with security extensions, there are 4 different > stack pointers (msp_s, msp_ns, psp_s, psp_ns), without security > extensions and for other Cortex-M targets, there are 2 different > stack pointers (msp and psp). > > With this patch, sp will always be in sync with one of the real stack > pointers on Arm targets that contain more than one stack pointer. > > 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 | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 80 insertions(+) > > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c > index 7d0944f9e3f..eaec2fd6b14 100644 > --- a/gdb/arm-tdep.c > +++ b/gdb/arm-tdep.c > @@ -3815,6 +3815,78 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache, > cpsr = reconstruct_t_bit (gdbarch, lr, cpsr); > return frame_unwind_got_constant (this_frame, regnum, cpsr); > } > + else if (arm_is_alternative_sp_register (tdep, regnum)) > + { > + /* Handle the alternative SP registers on Cortex-M. */ > + bool override_with_sp_value = false; > + CORE_ADDR val; > + > + if (tdep->have_sec_ext) > + { > + CORE_ADDR sp > + = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); > + CORE_ADDR msp_s > + = get_frame_register_unsigned (this_frame, > + tdep->m_profile_msp_s_regnum); > + CORE_ADDR msp_ns > + = get_frame_register_unsigned (this_frame, > + tdep->m_profile_msp_ns_regnum); > + CORE_ADDR psp_s > + = get_frame_register_unsigned (this_frame, > + tdep->m_profile_psp_s_regnum); > + CORE_ADDR psp_ns > + = get_frame_register_unsigned (this_frame, > + tdep->m_profile_psp_ns_regnum); > + > + bool is_msp = regnum == tdep->m_profile_msp_regnum > + && (msp_s == sp || msp_ns == sp); To make it a bit clear, we should have parenthesis guarding the bool expression. bool is_msp = (regnum == tdep->m_profile_msp_regnum) && (msp_s == sp || msp_ns == sp); Same for the other expressions. > + bool is_msp_s = regnum == tdep->m_profile_msp_s_regnum > + && msp_s == sp; > + bool is_msp_ns = regnum == tdep->m_profile_msp_ns_regnum > + && msp_ns == sp; > + bool is_psp = regnum == tdep->m_profile_psp_regnum > + && (psp_s == sp || psp_ns == sp); > + bool is_psp_s = regnum == tdep->m_profile_psp_s_regnum > + && psp_s == sp; > + bool is_psp_ns = regnum == tdep->m_profile_psp_ns_regnum > + && psp_ns == sp; > + > + override_with_sp_value = is_msp || is_msp_s || is_msp_ns > + || is_psp || is_psp_s || is_psp_ns; > + > + } > + else if (tdep->is_m) > + { > + CORE_ADDR sp > + = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); > + CORE_ADDR msp > + = get_frame_register_unsigned (this_frame, > + tdep->m_profile_msp_regnum); > + CORE_ADDR psp > + = get_frame_register_unsigned (this_frame, > + tdep->m_profile_psp_regnum); > + > + bool is_msp = regnum == tdep->m_profile_msp_regnum && sp == msp; > + bool is_psp = regnum == tdep->m_profile_psp_regnum && sp == psp; > + > + override_with_sp_value = is_msp || is_psp; > + } > + > + if (override_with_sp_value) > + { > + /* Use value of SP from previous frame. */ > + struct frame_info *prev_frame = get_prev_frame (this_frame); > + if (prev_frame) > + val = get_frame_register_unsigned (prev_frame, ARM_SP_REGNUM); > + else > + val = get_frame_base (this_frame); > + } > + else > + /* Use value for the register from previous frame. */ > + val = get_frame_register_unsigned (this_frame, regnum); > + > + return frame_unwind_got_constant (this_frame, regnum, val); > + } > > internal_error (__FILE__, __LINE__, > _("Unexpected register %d"), regnum); > @@ -4934,6 +5006,8 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum, > struct dwarf2_frame_state_reg *reg, > struct frame_info *this_frame) > { > + arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch); > + > if (is_pacbti_pseudo (gdbarch, regnum)) > { > /* Initialize RA_AUTH_CODE to zero. */ > @@ -4950,6 +5024,12 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum, > } > else if (regnum == ARM_SP_REGNUM) > reg->how = DWARF2_FRAME_REG_CFA; > + else if (arm_is_alternative_sp_register (tdep, regnum)) > + { > + /* Handle the alternative SP registers on Cortex-M. */ > + reg->how = DWARF2_FRAME_REG_FN; > + reg->loc.fn = arm_dwarf2_prev_register; > + } > } > > /* Given BUF, which is OLD_LEN bytes ending at ENDADDR, expand Otherwise, with the formatting changes, LGTM. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-07-25 9:52 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-07 9:17 [PATCH V2 0/2] gdb/arm: Sync sp with other *sp registers Yvan Roux 2022-07-07 9:19 ` [PATCH V2 1/2] gdb/arm: Rename arm_cache_is_sp_register to Yvan Roux 2022-07-16 2:03 ` Luis Machado 2022-07-21 13:01 ` Luis Machado 2022-07-07 9:21 ` [PATCH V2 2/2] gdb/arm: Sync sp with other *sp registers Yvan Roux 2022-07-16 2:03 ` Luis Machado 2022-07-21 8:33 ` Torbjorn SVENSSON 2022-07-21 8:48 ` Luis Machado 2022-07-22 20:59 ` [PATCH V3 0/2] " Torbjörn SVENSSON 2022-07-22 20:59 ` [PATCH v3 1/2] gdb/arm: Use if-else if instead of switch Torbjörn SVENSSON 2022-07-25 9:48 ` Luis Machado 2022-07-22 20:59 ` [PATCH v3 2/2] gdb/arm: Sync sp with other *sp registers Torbjörn SVENSSON 2022-07-25 9:52 ` Luis Machado
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).