From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx07-00178001.pphosted.com (mx07-00178001.pphosted.com [185.132.182.106]) by sourceware.org (Postfix) with ESMTPS id 86C85383F957 for ; Fri, 24 Jun 2022 16:05:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 86C85383F957 Received: from pps.filterd (m0241204.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 25OCISef006881; Fri, 24 Jun 2022 18:05:41 +0200 Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com (PPS) with ESMTPS id 3gua1nq9s1-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 24 Jun 2022 18:05:40 +0200 Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id A82D010002A; Fri, 24 Jun 2022 18:05:29 +0200 (CEST) Received: from Webmail-eu.st.com (shfdag1node2.st.com [10.75.129.70]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id 9DE3422788C; Fri, 24 Jun 2022 18:05:29 +0200 (CEST) Received: from gnbcxd0114.gnb.st.com (10.75.127.49) by SHFDAG1NODE2.st.com (10.75.129.70) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.2308.20; Fri, 24 Jun 2022 18:05:26 +0200 Date: Fri, 24 Jun 2022 18:05:25 +0200 From: Yvan Roux To: Luis Machado , CC: Torbjorn SVENSSON Subject: Re: [PATCH] gdb/arm: Unwind Non-Secure callbacks from Secure Message-ID: <20220624160525.GA30742@gnbcxd0114.gnb.st.com> References: <20220617125000.GA5800@gnbcxd0114.gnb.st.com> <16e5e509-7094-df0d-285e-f8a676ed251b@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <16e5e509-7094-df0d-285e-f8a676ed251b@arm.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-Originating-IP: [10.75.127.49] X-ClientProxiedBy: SFHDAG2NODE3.st.com (10.75.127.6) To SHFDAG1NODE2.st.com (10.75.129.70) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1 definitions=2022-06-24_08,2022-06-23_01,2022-06-22_01 X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Jun 2022 16:05:46 -0000 Hi, Thanks for the review Luis, here is a new version of the patch. On Tue, Jun 21, 2022 at 12:28:07PM +0100, Luis Machado wrote: > Hi, > > On 6/17/22 13:50, Yvan Roux wrote: > > Hi, > > > > Without this changeset, the unwinding doesn't take into account > > Non-Secure to Secure stack unwinding enablement status and > > doesn't choose on the proper SP to do the unwinding. > > choose on -> choose? > > > > > This patch only unwind the stack when Non-Secure to Secure > > unwind -> unwinds > > unwinding is enabled, previous SP is set w/r to the current mode > > (Handler -> msp_s, Thread -> psp_s) and then the Secure stack is > > unwound. Ensure thumb bit is set in PSR when needed. Also, drop > > thumb bit from PC if set. > > Also, two spaces after `.` according to the GNU Coding Standards. > > > > > Signed-off-by: Torbjörn SVENSSON > > Signed-off-by: Yvan ROUX > > --- > > gdb/arm-tdep.c | 121 +++++++++++++++++++++++++++++++++++++------------ > > 1 file changed, 91 insertions(+), 30 deletions(-) > > > > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c > > index 0c907482036..8a84754cfa6 100644 > > --- a/gdb/arm-tdep.c > > +++ b/gdb/arm-tdep.c > > @@ -309,6 +309,21 @@ struct arm_prologue_cache > > arm_prologue_cache() = default; > > }; > > + > > +/* Reconstruct T bit in program status register from LR value. */ > > + > > +static inline ULONGEST > > +reconstruct_t_bit(struct gdbarch *gdbarch, CORE_ADDR lr, ULONGEST psr) > > +{ > > + ULONGEST t_bit = arm_psr_thumb_bit (gdbarch); > > + if (IS_THUMB_ADDR (lr)) > > + psr |= t_bit; > > + else > > + psr &= ~t_bit; > > + > > + return psr; > > +} > > + > > /* Initialize stack pointers, and flag the active one. */ > > static inline void > > @@ -2342,15 +2357,10 @@ arm_prologue_prev_register (struct frame_info *this_frame, > > but the processor status is likely valid. */ > > if (prev_regnum == ARM_PS_REGNUM) > > { > > - CORE_ADDR lr, cpsr; > > - ULONGEST t_bit = arm_psr_thumb_bit (gdbarch); > > + ULONGEST cpsr = get_frame_register_unsigned (this_frame, prev_regnum); > > + CORE_ADDR lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM); > > - cpsr = get_frame_register_unsigned (this_frame, prev_regnum); > > - lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM); > > - if (IS_THUMB_ADDR (lr)) > > - cpsr |= t_bit; > > - else > > - cpsr &= ~t_bit; > > + cpsr = reconstruct_t_bit (gdbarch, lr, cpsr); > > return frame_unwind_got_constant (this_frame, prev_regnum, cpsr); > > } > > @@ -3363,24 +3373,46 @@ arm_m_exception_cache (struct frame_info *this_frame) > > return cache; > > } > > - fnc_return = ((lr & 0xfffffffe) == 0xfefffffe); > > + fnc_return = (((lr >> 24) & 0xff) == 0xfe); > > Is the above throwing away the comparison with the lower byte 0xfe? Yes, it only checks the FNC_RETURN prefix for coherency with how EXC_RETURN is handled and how it is also done in arm_m_addr_is_magic function (line 777), but according to the reference manual bits [23:1] are all 1, so we can keep the current comparison if you prefer. > > if (tdep->have_sec_ext && fnc_return) > > { > > - int actual_sp; > > + if (!arm_unwind_secure_frames) > > + { > > + warning (_("Non-secure to secure stack unwinding disabled.")); > > - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_ns_regnum); > > - arm_cache_set_active_sp_value (cache, tdep, sp); > > - if (lr & 1) > > - actual_sp = tdep->m_profile_msp_s_regnum; > > + /* Terminate any further stack unwinding by referring to self. */ > > + arm_cache_set_active_sp_value (cache, tdep, sp); > > + return cache; > > + } > > + > > + xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM); > > + if ((xpsr & 0xff) != 0) > > + /* Handler mode */ > > What is the handler mode? Can we expand on that to make it clear? This is the mode in which exceptions are handled and Threadmode is the normal or main mode where a program runs in, the comment are more explicits in this new version of the patch. > > + arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum); > > else > > - actual_sp = tdep->m_profile_msp_ns_regnum; > > + /* Thread mode */ > > Similarly, what is the thread mode? > > > + arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_s_regnum); > > + > > + unwound_sp = arm_cache_get_prev_sp_value (cache, tdep); > > - arm_cache_switch_prev_sp (cache, tdep, actual_sp); > > - sp = get_frame_register_unsigned (this_frame, actual_sp); > > + /* Stack layout for a function call from Secure to Non-Secure state > > + (ARMv8-M section B3.16): > > - cache->saved_regs[ARM_LR_REGNUM].set_addr (sp); > > + SP Offset > > - arm_cache_set_active_sp_value (cache, tdep, sp + 8); > > + +-------------------+ > > + 0x08 | | > > + +-------------------+ <-- Original SP > > + 0x04 | Partial xPSR | > > + +-------------------+ > > + 0x00 | Return Address | > > + +===================+ <-- New SP */ > > + > > + cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + 0x00); > > + cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + 0x00); > > + cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + 0x04); > > + > > + arm_cache_set_active_sp_value (cache, tdep, unwound_sp + 0x08); > > return cache; > > } > > @@ -3441,11 +3473,6 @@ arm_m_exception_cache (struct frame_info *this_frame) > > arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum); > > } > > } > > - else > > - { > > - /* Main stack used, use MSP as SP. */ > > - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum); > > - } > > /* Fetch the SP to use for this frame. */ > > unwound_sp = arm_cache_get_prev_sp_value (cache, tdep); > > @@ -3641,6 +3668,20 @@ arm_m_exception_prev_register (struct frame_info *this_frame, > > return frame_unwind_got_constant (this_frame, prev_regnum, > > arm_cache_get_prev_sp_value (cache, tdep)); > > + /* If we are asked to unwind the PC, strip the saved T bit. */ > > + if (prev_regnum == ARM_PC_REGNUM) > > + { > > + CORE_ADDR pc; > > + struct value *value; > > + > > + value = trad_frame_get_prev_register (this_frame, cache->saved_regs, > > + prev_regnum); > > + > > + pc = value_as_address (value); > > + return frame_unwind_got_constant (this_frame, prev_regnum, > > + UNMAKE_THUMB_ADDR (pc)); > > + } > > + > > /* 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)) > > @@ -3649,6 +3690,29 @@ arm_m_exception_prev_register (struct frame_info *this_frame, > > return frame_unwind_got_constant (this_frame, prev_regnum, sp_value); > > } > > + /* If we are asked to unwind the xPSR, set T bit if PC is in thumb mode. > > + * LR register is unreliable as it contains FNC_RETURN or EXC_RETURN pattern. > > + */ > > I think this style of comments doesn't match GDB's coding standards. > > > + if (prev_regnum == ARM_PS_REGNUM) > > + { > > + CORE_ADDR pc; > > + ULONGEST xpsr; > > > You could declare both pc and xpsr at their assignment locations. > > > + struct value *value; > > Same thing as above. Declare it in its assignment. > > > + struct gdbarch *gdbarch = get_frame_arch (this_frame); > > + > > + value = trad_frame_get_prev_register (this_frame, cache->saved_regs, > > + ARM_PC_REGNUM); > > here... > > > + pc = value_as_address (value); > > here... > > + > > + value = trad_frame_get_prev_register (this_frame, cache->saved_regs, > > + ARM_PS_REGNUM); > > + xpsr = value_as_long (value); > > and here. > > + > > + /* Reconstruct the T bit; see arm_prologue_prev_register for details. */ > > /* Reconstruct the T bit. See arm_prologue_prev_register for details. */ > > > + xpsr = reconstruct_t_bit (gdbarch, pc, xpsr); > > + return frame_unwind_got_constant (this_frame, ARM_PS_REGNUM, xpsr); > > + } > > + > > return trad_frame_get_prev_register (this_frame, cache->saved_regs, > > prev_regnum); > > } > > @@ -3711,8 +3775,8 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache, > > { > > struct gdbarch * gdbarch = get_frame_arch (this_frame); > > arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch); > > - CORE_ADDR lr, cpsr; > > - ULONGEST t_bit = arm_psr_thumb_bit (gdbarch); > > + CORE_ADDR lr; > > + ULONGEST cpsr; > > switch (regnum) > > { > > @@ -3741,10 +3805,7 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache, > > /* 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); > > - if (IS_THUMB_ADDR (lr)) > > - cpsr |= t_bit; > > - else > > - cpsr &= ~t_bit; > > + cpsr = reconstruct_t_bit (gdbarch, lr, cpsr); > > return frame_unwind_got_constant (this_frame, regnum, cpsr); > > default Updated version: gdb/arm: Unwind Non-Secure callbacks from Secure Without this changeset, the unwinding doesn't take into account Non-Secure to Secure stack unwinding enablement status and doesn't choose the proper SP to do the unwinding. This patch only unwinds the stack when Non-Secure to Secure unwinding is enabled, previous SP is set w/r to the current mode (Handler -> msp_s, Thread -> psp_s) and then the Secure stack is unwound. Ensure thumb bit is set in PSR when needed. Also, drop thumb bit from PC if set. Signed-off-by: Torbjörn SVENSSON Signed-off-by: Yvan ROUX --- gdb/arm-tdep.c | 117 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 87 insertions(+), 30 deletions(-) diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c index 96d70d40b28..8ae0c9fecd7 100644 --- a/gdb/arm-tdep.c +++ b/gdb/arm-tdep.c @@ -309,6 +309,21 @@ struct arm_prologue_cache arm_prologue_cache() = default; }; + +/* Reconstruct T bit in program status register from LR value. */ + +static inline ULONGEST +reconstruct_t_bit(struct gdbarch *gdbarch, CORE_ADDR lr, ULONGEST psr) +{ + ULONGEST t_bit = arm_psr_thumb_bit (gdbarch); + if (IS_THUMB_ADDR (lr)) + psr |= t_bit; + else + psr &= ~t_bit; + + return psr; +} + /* Initialize stack pointers, and flag the active one. */ static inline void @@ -2348,15 +2363,10 @@ arm_prologue_prev_register (struct frame_info *this_frame, but the processor status is likely valid. */ if (prev_regnum == ARM_PS_REGNUM) { - CORE_ADDR lr, cpsr; - ULONGEST t_bit = arm_psr_thumb_bit (gdbarch); + ULONGEST cpsr = get_frame_register_unsigned (this_frame, prev_regnum); + CORE_ADDR lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM); - cpsr = get_frame_register_unsigned (this_frame, prev_regnum); - lr = frame_unwind_register_unsigned (this_frame, ARM_LR_REGNUM); - if (IS_THUMB_ADDR (lr)) - cpsr |= t_bit; - else - cpsr &= ~t_bit; + cpsr = reconstruct_t_bit (gdbarch, lr, cpsr); return frame_unwind_got_constant (this_frame, prev_regnum, cpsr); } @@ -3369,24 +3379,46 @@ arm_m_exception_cache (struct frame_info *this_frame) return cache; } - fnc_return = ((lr & 0xfffffffe) == 0xfefffffe); + fnc_return = (((lr >> 24) & 0xff) == 0xfe); if (tdep->have_sec_ext && fnc_return) { - int actual_sp; + if (!arm_unwind_secure_frames) + { + warning (_("Non-secure to secure stack unwinding disabled.")); - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_ns_regnum); - arm_cache_set_active_sp_value (cache, tdep, sp); - if (lr & 1) - actual_sp = tdep->m_profile_msp_s_regnum; + /* Terminate any further stack unwinding by referring to self. */ + arm_cache_set_active_sp_value (cache, tdep, sp); + return cache; + } + + xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM); + if ((xpsr & 0xff) != 0) + /* Handler mode: This is mode that exceptions are handled in. */ + arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum); else - actual_sp = tdep->m_profile_msp_ns_regnum; + /* Thread mode: This is the normal mode that programs run in. */ + arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_s_regnum); + + unwound_sp = arm_cache_get_prev_sp_value (cache, tdep); - arm_cache_switch_prev_sp (cache, tdep, actual_sp); - sp = get_frame_register_unsigned (this_frame, actual_sp); + /* Stack layout for a function call from Secure to Non-Secure state + (ARMv8-M section B3.16): - cache->saved_regs[ARM_LR_REGNUM].set_addr (sp); + SP Offset - arm_cache_set_active_sp_value (cache, tdep, sp + 8); + +-------------------+ + 0x08 | | + +-------------------+ <-- Original SP + 0x04 | Partial xPSR | + +-------------------+ + 0x00 | Return Address | + +===================+ <-- New SP */ + + cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + 0x00); + cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + 0x00); + cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + 0x04); + + arm_cache_set_active_sp_value (cache, tdep, unwound_sp + 0x08); return cache; } @@ -3447,11 +3479,6 @@ arm_m_exception_cache (struct frame_info *this_frame) arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum); } } - else - { - /* Main stack used, use MSP as SP. */ - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_regnum); - } /* Fetch the SP to use for this frame. */ unwound_sp = arm_cache_get_prev_sp_value (cache, tdep); @@ -3647,6 +3674,20 @@ arm_m_exception_prev_register (struct frame_info *this_frame, return frame_unwind_got_constant (this_frame, prev_regnum, arm_cache_get_prev_sp_value (cache, tdep)); + /* If we are asked to unwind the PC, strip the saved T bit. */ + if (prev_regnum == ARM_PC_REGNUM) + { + CORE_ADDR pc; + struct value *value; + + value = trad_frame_get_prev_register (this_frame, cache->saved_regs, + prev_regnum); + + pc = value_as_address (value); + return frame_unwind_got_constant (this_frame, prev_regnum, + UNMAKE_THUMB_ADDR (pc)); + } + /* 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)) @@ -3655,6 +3696,25 @@ arm_m_exception_prev_register (struct frame_info *this_frame, return frame_unwind_got_constant (this_frame, prev_regnum, sp_value); } + /* If we are asked to unwind the xPSR, set T bit if PC is in thumb mode. + LR register is unreliable as it contains FNC_RETURN or EXC_RETURN + pattern. */ + if (prev_regnum == ARM_PS_REGNUM) + { + struct gdbarch *gdbarch = get_frame_arch (this_frame); + struct value *value = trad_frame_get_prev_register (this_frame, + cache->saved_regs, + ARM_PC_REGNUM); + CORE_ADDR pc = value_as_address (value); + value = trad_frame_get_prev_register (this_frame, cache->saved_regs, + ARM_PS_REGNUM); + ULONGEST xpsr = value_as_long (value); + + /* Reconstruct the T bit; see arm_prologue_prev_register for details. */ + xpsr = reconstruct_t_bit (gdbarch, pc, xpsr); + return frame_unwind_got_constant (this_frame, ARM_PS_REGNUM, xpsr); + } + return trad_frame_get_prev_register (this_frame, cache->saved_regs, prev_regnum); } @@ -3717,8 +3777,8 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache, { struct gdbarch * gdbarch = get_frame_arch (this_frame); arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch); - CORE_ADDR lr, cpsr; - ULONGEST t_bit = arm_psr_thumb_bit (gdbarch); + CORE_ADDR lr; + ULONGEST cpsr; switch (regnum) { @@ -3747,10 +3807,7 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache, /* 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); - if (IS_THUMB_ADDR (lr)) - cpsr |= t_bit; - else - cpsr &= ~t_bit; + cpsr = reconstruct_t_bit (gdbarch, lr, cpsr); return frame_unwind_got_constant (this_frame, regnum, cpsr); default: -- 2.17.1