* [PATCH v2] gdb/arm: Cleanup of arm_m_exception_cache
@ 2022-08-09 15:30 Torbjörn SVENSSON
2022-08-09 16:15 ` Luis Machado
0 siblings, 1 reply; 4+ messages in thread
From: Torbjörn SVENSSON @ 2022-08-09 15:30 UTC (permalink / raw)
To: gdb-patches; +Cc: luis.machado, Torbjörn SVENSSON
With this change, only valid content of LR is accepted for the current
target. If the content for LR is anything but EXC_RETURN or FNC_RETURN
will cause GDB to assert since it's an invalid state for the unwinder.
FNC_RETURN pattern requires Security Extensions to be enabled or GDB
will assert due to the bad state of the unwinder.
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
---
gdb/arm-tdep.c | 380 ++++++++++++++++++++++++++-----------------------
1 file changed, 204 insertions(+), 176 deletions(-)
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index cf8b610a381..299c416fe52 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -3346,19 +3346,7 @@ arm_m_exception_cache (struct frame_info *this_frame)
{
struct gdbarch *gdbarch = get_frame_arch (this_frame);
arm_gdbarch_tdep *tdep = gdbarch_tdep<arm_gdbarch_tdep> (gdbarch);
- enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
struct arm_prologue_cache *cache;
- CORE_ADDR lr;
- CORE_ADDR sp;
- CORE_ADDR unwound_sp;
- uint32_t sp_r0_offset = 0;
- LONGEST xpsr;
- uint32_t exc_return;
- bool fnc_return;
- uint32_t extended_frame_used;
- bool secure_stack_used = false;
- bool default_callee_register_stacking = false;
- bool exception_domain_is_secure = false;
cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
arm_cache_init (cache, this_frame);
@@ -3367,8 +3355,8 @@ arm_m_exception_cache (struct frame_info *this_frame)
describes which bits in LR that define which stack was used prior
to the exception and if FPU is used (causing extended stack frame). */
- lr = get_frame_register_unsigned (this_frame, ARM_LR_REGNUM);
- sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
+ CORE_ADDR lr = get_frame_register_unsigned (this_frame, ARM_LR_REGNUM);
+ CORE_ADDR sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
/* ARMv7-M Architecture Reference "A2.3.1 Arm core registers"
states that LR is set to 0xffffffff on reset. ARMv8-M Architecture
@@ -3381,9 +3369,22 @@ arm_m_exception_cache (struct frame_info *this_frame)
return cache;
}
- fnc_return = (((lr >> 24) & 0xff) == 0xfe);
- if (tdep->have_sec_ext && fnc_return)
+ /* Check FNC_RETURN indicator bits (24-31). */
+ bool fnc_return = (((lr >> 24) & 0xff) == 0xfe);
+ if (fnc_return)
{
+ /* FNC_RETURN is only valid for targets with Security Extension. */
+ if (!tdep->have_sec_ext)
+ {
+ error (_("While unwinding an exception frame, found unexpected Link "
+ "Register value 0x%lx. This should not happen and may be "
+ "caused by corrupt data or a bug in GDB."), lr);
+
+ /* Terminate any further stack unwinding by referring to self. */
+ arm_cache_set_active_sp_value (cache, tdep, sp);
+ return cache;
+ }
+
if (!arm_unwind_secure_frames)
{
warning (_("Non-secure to secure stack unwinding disabled."));
@@ -3393,7 +3394,7 @@ arm_m_exception_cache (struct frame_info *this_frame)
return cache;
}
- xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
+ ULONGEST xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
if ((xpsr & 0xff) != 0)
/* Handler mode: This is the mode that exceptions are handled in. */
arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum);
@@ -3401,7 +3402,7 @@ arm_m_exception_cache (struct frame_info *this_frame)
/* 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);
+ CORE_ADDR unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
/* Stack layout for a function call from Secure to Non-Secure state
(ARMv8-M section B3.16):
@@ -3426,17 +3427,23 @@ arm_m_exception_cache (struct frame_info *this_frame)
}
/* Check EXC_RETURN indicator bits (24-31). */
- exc_return = (((lr >> 24) & 0xff) == 0xff);
+ bool exc_return = (((lr >> 24) & 0xff) == 0xff);
if (exc_return)
{
+ int sp_regnum;
+ bool secure_stack_used = false;
+ bool default_callee_register_stacking = false;
+ bool exception_domain_is_secure = false;
+ enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+
/* Check EXC_RETURN bit SPSEL if Main or Thread (process) stack used. */
- bool process_stack_used = ((lr & (1 << 2)) != 0);
+ bool process_stack_used = (bit (lr,2) != 0);
if (tdep->have_sec_ext)
{
- secure_stack_used = ((lr & (1 << 6)) != 0);
- default_callee_register_stacking = ((lr & (1 << 5)) != 0);
- exception_domain_is_secure = ((lr & (1 << 0)) == 0);
+ secure_stack_used = (bit (lr,6) != 0);
+ default_callee_register_stacking = (bit (lr,5) != 0);
+ exception_domain_is_secure = (bit (lr,0) == 0);
/* Unwinding from non-secure to secure can trip security
measures. In order to avoid the debugger being
@@ -3456,187 +3463,208 @@ arm_m_exception_cache (struct frame_info *this_frame)
{
if (secure_stack_used)
/* Secure thread (process) stack used, use PSP_S as SP. */
- arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_s_regnum);
+ sp_regnum = tdep->m_profile_psp_s_regnum;
else
/* Non-secure thread (process) stack used, use PSP_NS as SP. */
- arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_ns_regnum);
+ sp_regnum = tdep->m_profile_psp_ns_regnum;
}
else
{
if (secure_stack_used)
/* Secure main stack used, use MSP_S as SP. */
- arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum);
+ sp_regnum = tdep->m_profile_msp_s_regnum;
else
/* Non-secure main stack used, use MSP_NS as SP. */
- arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_ns_regnum);
+ sp_regnum = tdep->m_profile_msp_ns_regnum;
}
}
else
{
if (process_stack_used)
/* Thread (process) stack used, use PSP as SP. */
- arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_regnum);
+ sp_regnum = tdep->m_profile_psp_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);
-
- /* Exception entry context stacking are described in ARMv8-M (section B3.19)
- and ARMv7-M (sections B1.5.6 and B1.5.7) Architecture Reference Manuals.
-
- The following figure shows the structure of the stack frame when Security
- and Floating-point extensions are present.
-
- SP Offsets
- Without With
- Callee Regs Callee Regs
- (Secure -> Non-Secure)
- +-------------------+
- 0xA8 | | 0xD0
- +===================+ --+ <-- Original SP
- 0xA4 | S31 | 0xCC |
- +-------------------+ |
- ... | Additional FP context
- +-------------------+ |
- 0x68 | S16 | 0x90 |
- +===================+ --+
- 0x64 | Reserved | 0x8C |
- +-------------------+ |
- 0x60 | FPSCR | 0x88 |
- +-------------------+ |
- 0x5C | S15 | 0x84 | FP context
- +-------------------+ |
- ... |
- +-------------------+ |
- 0x20 | S0 | 0x48 |
- +===================+ --+
- 0x1C | xPSR | 0x44 |
- +-------------------+ |
- 0x18 | Return address | 0x40 |
- +-------------------+ |
- 0x14 | LR(R14) | 0x3C |
- +-------------------+ |
- 0x10 | R12 | 0x38 | State context
- +-------------------+ |
- 0x0C | R3 | 0x34 |
- +-------------------+ |
- ... |
- +-------------------+ |
- 0x00 | R0 | 0x28 |
- +===================+ --+
- | R11 | 0x24 |
- +-------------------+ |
- ... |
- +-------------------+ | Additional state context
- | R4 | 0x08 | when transitioning from
- +-------------------+ | Secure to Non-Secure
- | Reserved | 0x04 |
- +-------------------+ |
- | Magic signature | 0x00 |
- +===================+ --+ <-- New SP */
-
- /* With the Security extension, the hardware saves R4..R11 too. */
- if (exc_return && tdep->have_sec_ext && secure_stack_used
- && (!default_callee_register_stacking || exception_domain_is_secure))
- {
- /* Read R4..R11 from the integer callee registers. */
- cache->saved_regs[4].set_addr (unwound_sp + 0x08);
- cache->saved_regs[5].set_addr (unwound_sp + 0x0C);
- cache->saved_regs[6].set_addr (unwound_sp + 0x10);
- cache->saved_regs[7].set_addr (unwound_sp + 0x14);
- cache->saved_regs[8].set_addr (unwound_sp + 0x18);
- cache->saved_regs[9].set_addr (unwound_sp + 0x1C);
- cache->saved_regs[10].set_addr (unwound_sp + 0x20);
- cache->saved_regs[11].set_addr (unwound_sp + 0x24);
- sp_r0_offset = 0x28;
- }
-
- /* The hardware saves eight 32-bit words, comprising xPSR,
- ReturnAddress, LR (R14), R12, R3, R2, R1, R0. See details in
- "B1.5.6 Exception entry behavior" in
- "ARMv7-M Architecture Reference Manual". */
- cache->saved_regs[0].set_addr (unwound_sp + sp_r0_offset);
- cache->saved_regs[1].set_addr (unwound_sp + sp_r0_offset + 0x04);
- cache->saved_regs[2].set_addr (unwound_sp + sp_r0_offset + 0x08);
- cache->saved_regs[3].set_addr (unwound_sp + sp_r0_offset + 0x0C);
- cache->saved_regs[ARM_IP_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x10);
- cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x14);
- cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x18);
- cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x1C);
-
- /* Check EXC_RETURN bit FTYPE if extended stack frame (FPU regs stored)
- type used. */
- extended_frame_used = ((lr & (1 << 4)) == 0);
- if (exc_return && extended_frame_used)
- {
- int i;
- int fpu_regs_stack_offset;
- ULONGEST fpccr;
-
- /* Read FPCCR register. */
- gdb_assert (safe_read_memory_unsigned_integer (FPCCR,
- ARM_INT_REGISTER_SIZE,
- byte_order, &fpccr));
- bool fpccr_ts = bit (fpccr,26);
-
- /* This code does not take into account the lazy stacking, see "Lazy
- context save of FP state", in B1.5.7, also ARM AN298, supported
- by Cortex-M4F architecture.
- To fully handle this the FPCCR register (Floating-point Context
- Control Register) needs to be read out and the bits ASPEN and LSPEN
- could be checked to setup correct lazy stacked FP registers.
- This register is located at address 0xE000EF34. */
-
- /* Extended stack frame type used. */
- fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x20;
- for (i = 0; i < 8; i++)
- {
- cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);
- fpu_regs_stack_offset += 8;
- }
- cache->saved_regs[ARM_FPSCR_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x60);
- fpu_regs_stack_offset += 4;
+ sp_regnum = tdep->m_profile_msp_regnum;
+ }
+
+ /* Set the active SP regnum. */
+ arm_cache_switch_prev_sp (cache, tdep, sp_regnum);
+
+ /* Fetch the SP to use for this frame. */
+ CORE_ADDR unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
+
+ /* Exception entry context stacking are described in ARMv8-M (section
+ B3.19) and ARMv7-M (sections B1.5.6 and B1.5.7) Architecture Reference
+ Manuals.
+
+ The following figure shows the structure of the stack frame when
+ Security and Floating-point extensions are present.
+
+ SP Offsets
+ Without With
+ Callee Regs Callee Regs
+ (Secure -> Non-Secure)
+ +-------------------+
+ 0xA8 | | 0xD0
+ +===================+ --+ <-- Original SP
+ 0xA4 | S31 | 0xCC |
+ +-------------------+ |
+ ... | Additional FP context
+ +-------------------+ |
+ 0x68 | S16 | 0x90 |
+ +===================+ --+
+ 0x64 | Reserved | 0x8C |
+ +-------------------+ |
+ 0x60 | FPSCR | 0x88 |
+ +-------------------+ |
+ 0x5C | S15 | 0x84 | FP context
+ +-------------------+ |
+ ... |
+ +-------------------+ |
+ 0x20 | S0 | 0x48 |
+ +===================+ --+
+ 0x1C | xPSR | 0x44 |
+ +-------------------+ |
+ 0x18 | Return address | 0x40 |
+ +-------------------+ |
+ 0x14 | LR(R14) | 0x3C |
+ +-------------------+ |
+ 0x10 | R12 | 0x38 | State context
+ +-------------------+ |
+ 0x0C | R3 | 0x34 |
+ +-------------------+ |
+ ... |
+ +-------------------+ |
+ 0x00 | R0 | 0x28 |
+ +===================+ --+
+ | R11 | 0x24 |
+ +-------------------+ |
+ ... |
+ +-------------------+ | Additional state
+ | R4 | 0x08 | context when
+ +-------------------+ | transitioning from
+ | Reserved | 0x04 | Secure to Non-Secure
+ +-------------------+ |
+ | Magic signature | 0x00 |
+ +===================+ --+ <-- New SP */
+
+ uint32_t sp_r0_offset = 0;
+
+ /* With the Security extension, the hardware saves R4..R11 too. */
+ if (tdep->have_sec_ext && secure_stack_used
+ && (!default_callee_register_stacking || exception_domain_is_secure))
+ {
+ /* Read R4..R11 from the integer callee registers. */
+ cache->saved_regs[4].set_addr (unwound_sp + 0x08);
+ cache->saved_regs[5].set_addr (unwound_sp + 0x0C);
+ cache->saved_regs[6].set_addr (unwound_sp + 0x10);
+ cache->saved_regs[7].set_addr (unwound_sp + 0x14);
+ cache->saved_regs[8].set_addr (unwound_sp + 0x18);
+ cache->saved_regs[9].set_addr (unwound_sp + 0x1C);
+ cache->saved_regs[10].set_addr (unwound_sp + 0x20);
+ cache->saved_regs[11].set_addr (unwound_sp + 0x24);
+ sp_r0_offset = 0x28;
+ }
+
+ /* The hardware saves eight 32-bit words, comprising xPSR,
+ ReturnAddress, LR (R14), R12, R3, R2, R1, R0. See details in
+ "B1.5.6 Exception entry behavior" in
+ "ARMv7-M Architecture Reference Manual". */
+ cache->saved_regs[0].set_addr (unwound_sp + sp_r0_offset);
+ cache->saved_regs[1].set_addr (unwound_sp + sp_r0_offset + 0x04);
+ cache->saved_regs[2].set_addr (unwound_sp + sp_r0_offset + 0x08);
+ cache->saved_regs[3].set_addr (unwound_sp + sp_r0_offset + 0x0C);
+ cache->saved_regs[ARM_IP_REGNUM].set_addr (unwound_sp + sp_r0_offset
+ + 0x10);
+ cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + sp_r0_offset
+ + 0x14);
+ cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + sp_r0_offset
+ + 0x18);
+ cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + sp_r0_offset
+ + 0x1C);
+
+ /* Check EXC_RETURN bit FTYPE if extended stack frame (FPU regs stored)
+ type used. */
+ bool extended_frame_used = (bit (lr,4) == 0);
+ if (extended_frame_used)
+ {
+ ULONGEST fpccr;
+
+ /* Read FPCCR register. */
+ gdb_assert (safe_read_memory_unsigned_integer (FPCCR,
+ ARM_INT_REGISTER_SIZE,
+ byte_order, &fpccr));
+ bool fpccr_ts = bit (fpccr,26);
+
+ /* This code does not take into account the lazy stacking, see "Lazy
+ context save of FP state", in B1.5.7, also ARM AN298, supported
+ by Cortex-M4F architecture.
+ To fully handle this the FPCCR register (Floating-point Context
+ Control Register) needs to be read out and the bits ASPEN and
+ LSPEN could be checked to setup correct lazy stacked FP registers.
+ This register is located at address 0xE000EF34. */
+
+ /* Extended stack frame type used. */
+ CORE_ADDR addr = unwound_sp + sp_r0_offset + 0x20;
+ for (int i = 0; i < 8; i++)
+ {
+ cache->saved_regs[ARM_D0_REGNUM + i].set_addr (addr);
+ addr += 8;
+ }
+ cache->saved_regs[ARM_FPSCR_REGNUM].set_addr (unwound_sp
+ + sp_r0_offset + 0x60);
+
+ if (tdep->have_sec_ext && !default_callee_register_stacking
+ && fpccr_ts)
+ {
+ /* Handle floating-point callee saved registers. */
+ addr = unwound_sp + sp_r0_offset + 0x68;
+ for (int i = 8; i < 16; i++)
+ {
+ cache->saved_regs[ARM_D0_REGNUM + i].set_addr (addr);
+ addr += 8;
+ }
- if (tdep->have_sec_ext && !default_callee_register_stacking && fpccr_ts)
- {
- /* Handle floating-point callee saved registers. */
- fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x68;
- for (i = 8; i < 16; i++)
+ arm_cache_set_active_sp_value (cache, tdep,
+ unwound_sp + sp_r0_offset + 0xA8);
+ }
+ else
{
- cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);
- fpu_regs_stack_offset += 8;
+ /* Offset 0x64 is reserved. */
+ arm_cache_set_active_sp_value (cache, tdep,
+ unwound_sp + sp_r0_offset + 0x68);
}
-
- arm_cache_set_active_sp_value (cache, tdep,
- unwound_sp + sp_r0_offset + 0xA8);
}
else
{
- /* Offset 0x64 is reserved. */
+ /* Standard stack frame type used. */
arm_cache_set_active_sp_value (cache, tdep,
- unwound_sp + sp_r0_offset + 0x68);
+ unwound_sp + sp_r0_offset + 0x20);
}
- }
- else
- {
- /* Standard stack frame type used. */
- arm_cache_set_active_sp_value (cache, tdep,
- unwound_sp + sp_r0_offset + 0x20);
+
+ /* If bit 9 of the saved xPSR is set, then there is a four-byte
+ aligner between the top of the 32-byte stack frame and the
+ previous context's stack pointer. */
+ ULONGEST xpsr;
+ gdb_assert (safe_read_memory_unsigned_integer (cache->saved_regs[
+ ARM_PS_REGNUM].addr (), 4,
+ byte_order, &xpsr));
+ if (bit (xpsr,9) != 0)
+ {
+ CORE_ADDR new_sp = arm_cache_get_prev_sp_value (cache, tdep) + 4;
+ arm_cache_set_active_sp_value (cache, tdep, new_sp);
+ }
+
+ return cache;
}
- /* If bit 9 of the saved xPSR is set, then there is a four-byte
- aligner between the top of the 32-byte stack frame and the
- previous context's stack pointer. */
- if (safe_read_memory_integer (unwound_sp + sp_r0_offset + 0x1C, 4,
- byte_order, &xpsr)
- && (xpsr & (1 << 9)) != 0)
- arm_cache_set_active_sp_value (cache, tdep,
- arm_cache_get_prev_sp_value (cache, tdep) + 4);
+ error (_("While unwinding an exception frame, found unexpected Link Register "
+ "value 0x%lx. This should not happen and may be caused by corrupt "
+ "data or a bug in GDB."), lr);
+ /* Terminate any further stack unwinding by referring to self. */
+ arm_cache_set_active_sp_value (cache, tdep, sp);
return cache;
}
--
2.25.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] gdb/arm: Cleanup of arm_m_exception_cache
2022-08-09 15:30 [PATCH v2] gdb/arm: Cleanup of arm_m_exception_cache Torbjörn SVENSSON
@ 2022-08-09 16:15 ` Luis Machado
2022-08-09 17:13 ` Torbjorn SVENSSON
0 siblings, 1 reply; 4+ messages in thread
From: Luis Machado @ 2022-08-09 16:15 UTC (permalink / raw)
To: Torbjörn SVENSSON, gdb-patches
On 8/9/22 16:30, Torbjörn SVENSSON wrote:
> With this change, only valid content of LR is accepted for the current
> target. If the content for LR is anything but EXC_RETURN or FNC_RETURN
> will cause GDB to assert since it's an invalid state for the unwinder.
> FNC_RETURN pattern requires Security Extensions to be enabled or GDB
> will assert due to the bad state of the unwinder.
>
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> ---
> gdb/arm-tdep.c | 380 ++++++++++++++++++++++++++-----------------------
> 1 file changed, 204 insertions(+), 176 deletions(-)
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index cf8b610a381..299c416fe52 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -3346,19 +3346,7 @@ arm_m_exception_cache (struct frame_info *this_frame)
> {
> struct gdbarch *gdbarch = get_frame_arch (this_frame);
> arm_gdbarch_tdep *tdep = gdbarch_tdep<arm_gdbarch_tdep> (gdbarch);
> - enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> struct arm_prologue_cache *cache;
> - CORE_ADDR lr;
> - CORE_ADDR sp;
> - CORE_ADDR unwound_sp;
> - uint32_t sp_r0_offset = 0;
> - LONGEST xpsr;
> - uint32_t exc_return;
> - bool fnc_return;
> - uint32_t extended_frame_used;
> - bool secure_stack_used = false;
> - bool default_callee_register_stacking = false;
> - bool exception_domain_is_secure = false;
>
> cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
> arm_cache_init (cache, this_frame);
> @@ -3367,8 +3355,8 @@ arm_m_exception_cache (struct frame_info *this_frame)
> describes which bits in LR that define which stack was used prior
> to the exception and if FPU is used (causing extended stack frame). */
>
> - lr = get_frame_register_unsigned (this_frame, ARM_LR_REGNUM);
> - sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
> + CORE_ADDR lr = get_frame_register_unsigned (this_frame, ARM_LR_REGNUM);
> + CORE_ADDR sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>
> /* ARMv7-M Architecture Reference "A2.3.1 Arm core registers"
> states that LR is set to 0xffffffff on reset. ARMv8-M Architecture
> @@ -3381,9 +3369,22 @@ arm_m_exception_cache (struct frame_info *this_frame)
> return cache;
> }
>
> - fnc_return = (((lr >> 24) & 0xff) == 0xfe);
> - if (tdep->have_sec_ext && fnc_return)
> + /* Check FNC_RETURN indicator bits (24-31). */
> + bool fnc_return = (((lr >> 24) & 0xff) == 0xfe);
> + if (fnc_return)
> {
> + /* FNC_RETURN is only valid for targets with Security Extension. */
> + if (!tdep->have_sec_ext)
> + {
> + error (_("While unwinding an exception frame, found unexpected Link "
> + "Register value 0x%lx. This should not happen and may be "
You should use the %s format here and use phex to turn the 32-bit value to hex.
Also, since this is checking explicitly for a value and the Security Extension, we should
add that to the error message to make it more obvious what is failing.
"While unwinding an exception frame, found unexpected Link Register value %s that requires the security extension, but the extension was not found or is disabled. This should not happen and may be caused by corrupt data or a bug in GDB."
> + "caused by corrupt data or a bug in GDB."), lr);
> +
> + /* Terminate any further stack unwinding by referring to self. */
> + arm_cache_set_active_sp_value (cache, tdep, sp);
> + return cache;
Since you errored out, there's no use returning or executing any other statements after error.
> + }
> +
> if (!arm_unwind_secure_frames)
> {
> warning (_("Non-secure to secure stack unwinding disabled."));
> @@ -3393,7 +3394,7 @@ arm_m_exception_cache (struct frame_info *this_frame)
> return cache;
> }
>
> - xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
> + ULONGEST xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
> if ((xpsr & 0xff) != 0)
> /* Handler mode: This is the mode that exceptions are handled in. */
> arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum);
> @@ -3401,7 +3402,7 @@ arm_m_exception_cache (struct frame_info *this_frame)
> /* 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);
> + CORE_ADDR unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
>
> /* Stack layout for a function call from Secure to Non-Secure state
> (ARMv8-M section B3.16):
> @@ -3426,17 +3427,23 @@ arm_m_exception_cache (struct frame_info *this_frame)
> }
>
> /* Check EXC_RETURN indicator bits (24-31). */
> - exc_return = (((lr >> 24) & 0xff) == 0xff);
> + bool exc_return = (((lr >> 24) & 0xff) == 0xff);
> if (exc_return)
> {
> + int sp_regnum;
> + bool secure_stack_used = false;
> + bool default_callee_register_stacking = false;
> + bool exception_domain_is_secure = false;
> + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +
> /* Check EXC_RETURN bit SPSEL if Main or Thread (process) stack used. */
> - bool process_stack_used = ((lr & (1 << 2)) != 0);
> + bool process_stack_used = (bit (lr,2) != 0);
>
> if (tdep->have_sec_ext)
> {
> - secure_stack_used = ((lr & (1 << 6)) != 0);
> - default_callee_register_stacking = ((lr & (1 << 5)) != 0);
> - exception_domain_is_secure = ((lr & (1 << 0)) == 0);
> + secure_stack_used = (bit (lr,6) != 0);
Could you please address the formatting issues? space before `(`, space after `,`
> + default_callee_register_stacking = (bit (lr,5) != 0);> + exception_domain_is_secure = (bit (lr,0) == 0);
>
> /* Unwinding from non-secure to secure can trip security
> measures. In order to avoid the debugger being
> @@ -3456,187 +3463,208 @@ arm_m_exception_cache (struct frame_info *this_frame)
> {
> if (secure_stack_used)
> /* Secure thread (process) stack used, use PSP_S as SP. */
> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_s_regnum);
> + sp_regnum = tdep->m_profile_psp_s_regnum;
> else
> /* Non-secure thread (process) stack used, use PSP_NS as SP. */
> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_ns_regnum);
> + sp_regnum = tdep->m_profile_psp_ns_regnum;
> }
> else
> {
> if (secure_stack_used)
> /* Secure main stack used, use MSP_S as SP. */
> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum);
> + sp_regnum = tdep->m_profile_msp_s_regnum;
> else
> /* Non-secure main stack used, use MSP_NS as SP. */
> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_ns_regnum);
> + sp_regnum = tdep->m_profile_msp_ns_regnum;
> }
> }
> else
> {
> if (process_stack_used)
> /* Thread (process) stack used, use PSP as SP. */
> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_regnum);
> + sp_regnum = tdep->m_profile_psp_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);
> -
> - /* Exception entry context stacking are described in ARMv8-M (section B3.19)
> - and ARMv7-M (sections B1.5.6 and B1.5.7) Architecture Reference Manuals.
> -
> - The following figure shows the structure of the stack frame when Security
> - and Floating-point extensions are present.
> -
> - SP Offsets
> - Without With
> - Callee Regs Callee Regs
> - (Secure -> Non-Secure)
> - +-------------------+
> - 0xA8 | | 0xD0
> - +===================+ --+ <-- Original SP
> - 0xA4 | S31 | 0xCC |
> - +-------------------+ |
> - ... | Additional FP context
> - +-------------------+ |
> - 0x68 | S16 | 0x90 |
> - +===================+ --+
> - 0x64 | Reserved | 0x8C |
> - +-------------------+ |
> - 0x60 | FPSCR | 0x88 |
> - +-------------------+ |
> - 0x5C | S15 | 0x84 | FP context
> - +-------------------+ |
> - ... |
> - +-------------------+ |
> - 0x20 | S0 | 0x48 |
> - +===================+ --+
> - 0x1C | xPSR | 0x44 |
> - +-------------------+ |
> - 0x18 | Return address | 0x40 |
> - +-------------------+ |
> - 0x14 | LR(R14) | 0x3C |
> - +-------------------+ |
> - 0x10 | R12 | 0x38 | State context
> - +-------------------+ |
> - 0x0C | R3 | 0x34 |
> - +-------------------+ |
> - ... |
> - +-------------------+ |
> - 0x00 | R0 | 0x28 |
> - +===================+ --+
> - | R11 | 0x24 |
> - +-------------------+ |
> - ... |
> - +-------------------+ | Additional state context
> - | R4 | 0x08 | when transitioning from
> - +-------------------+ | Secure to Non-Secure
> - | Reserved | 0x04 |
> - +-------------------+ |
> - | Magic signature | 0x00 |
> - +===================+ --+ <-- New SP */
> -
> - /* With the Security extension, the hardware saves R4..R11 too. */
> - if (exc_return && tdep->have_sec_ext && secure_stack_used
> - && (!default_callee_register_stacking || exception_domain_is_secure))
> - {
> - /* Read R4..R11 from the integer callee registers. */
> - cache->saved_regs[4].set_addr (unwound_sp + 0x08);
> - cache->saved_regs[5].set_addr (unwound_sp + 0x0C);
> - cache->saved_regs[6].set_addr (unwound_sp + 0x10);
> - cache->saved_regs[7].set_addr (unwound_sp + 0x14);
> - cache->saved_regs[8].set_addr (unwound_sp + 0x18);
> - cache->saved_regs[9].set_addr (unwound_sp + 0x1C);
> - cache->saved_regs[10].set_addr (unwound_sp + 0x20);
> - cache->saved_regs[11].set_addr (unwound_sp + 0x24);
> - sp_r0_offset = 0x28;
> - }
> -
> - /* The hardware saves eight 32-bit words, comprising xPSR,
> - ReturnAddress, LR (R14), R12, R3, R2, R1, R0. See details in
> - "B1.5.6 Exception entry behavior" in
> - "ARMv7-M Architecture Reference Manual". */
> - cache->saved_regs[0].set_addr (unwound_sp + sp_r0_offset);
> - cache->saved_regs[1].set_addr (unwound_sp + sp_r0_offset + 0x04);
> - cache->saved_regs[2].set_addr (unwound_sp + sp_r0_offset + 0x08);
> - cache->saved_regs[3].set_addr (unwound_sp + sp_r0_offset + 0x0C);
> - cache->saved_regs[ARM_IP_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x10);
> - cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x14);
> - cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x18);
> - cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x1C);
> -
> - /* Check EXC_RETURN bit FTYPE if extended stack frame (FPU regs stored)
> - type used. */
> - extended_frame_used = ((lr & (1 << 4)) == 0);
> - if (exc_return && extended_frame_used)
> - {
> - int i;
> - int fpu_regs_stack_offset;
> - ULONGEST fpccr;
> -
> - /* Read FPCCR register. */
> - gdb_assert (safe_read_memory_unsigned_integer (FPCCR,
> - ARM_INT_REGISTER_SIZE,
> - byte_order, &fpccr));
> - bool fpccr_ts = bit (fpccr,26);
> -
> - /* This code does not take into account the lazy stacking, see "Lazy
> - context save of FP state", in B1.5.7, also ARM AN298, supported
> - by Cortex-M4F architecture.
> - To fully handle this the FPCCR register (Floating-point Context
> - Control Register) needs to be read out and the bits ASPEN and LSPEN
> - could be checked to setup correct lazy stacked FP registers.
> - This register is located at address 0xE000EF34. */
> -
> - /* Extended stack frame type used. */
> - fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x20;
> - for (i = 0; i < 8; i++)
> - {
> - cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);
> - fpu_regs_stack_offset += 8;
> - }
> - cache->saved_regs[ARM_FPSCR_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x60);
> - fpu_regs_stack_offset += 4;
> + sp_regnum = tdep->m_profile_msp_regnum;
> + }
> +
> + /* Set the active SP regnum. */
> + arm_cache_switch_prev_sp (cache, tdep, sp_regnum);
> +
> + /* Fetch the SP to use for this frame. */
> + CORE_ADDR unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
> +
> + /* Exception entry context stacking are described in ARMv8-M (section
> + B3.19) and ARMv7-M (sections B1.5.6 and B1.5.7) Architecture Reference
> + Manuals.
> +
> + The following figure shows the structure of the stack frame when
> + Security and Floating-point extensions are present.
> +
> + SP Offsets
> + Without With
> + Callee Regs Callee Regs
> + (Secure -> Non-Secure)
> + +-------------------+
> + 0xA8 | | 0xD0
> + +===================+ --+ <-- Original SP
> + 0xA4 | S31 | 0xCC |
> + +-------------------+ |
> + ... | Additional FP context
> + +-------------------+ |
> + 0x68 | S16 | 0x90 |
> + +===================+ --+
> + 0x64 | Reserved | 0x8C |
> + +-------------------+ |
> + 0x60 | FPSCR | 0x88 |
> + +-------------------+ |
> + 0x5C | S15 | 0x84 | FP context
> + +-------------------+ |
> + ... |
> + +-------------------+ |
> + 0x20 | S0 | 0x48 |
> + +===================+ --+
> + 0x1C | xPSR | 0x44 |
> + +-------------------+ |
> + 0x18 | Return address | 0x40 |
> + +-------------------+ |
> + 0x14 | LR(R14) | 0x3C |
> + +-------------------+ |
> + 0x10 | R12 | 0x38 | State context
> + +-------------------+ |
> + 0x0C | R3 | 0x34 |
> + +-------------------+ |
> + ... |
> + +-------------------+ |
> + 0x00 | R0 | 0x28 |
> + +===================+ --+
> + | R11 | 0x24 |
> + +-------------------+ |
> + ... |
> + +-------------------+ | Additional state
> + | R4 | 0x08 | context when
> + +-------------------+ | transitioning from
> + | Reserved | 0x04 | Secure to Non-Secure
> + +-------------------+ |
> + | Magic signature | 0x00 |
> + +===================+ --+ <-- New SP */
> +
> + uint32_t sp_r0_offset = 0;
> +
> + /* With the Security extension, the hardware saves R4..R11 too. */
> + if (tdep->have_sec_ext && secure_stack_used
> + && (!default_callee_register_stacking || exception_domain_is_secure))
> + {
> + /* Read R4..R11 from the integer callee registers. */
> + cache->saved_regs[4].set_addr (unwound_sp + 0x08);
> + cache->saved_regs[5].set_addr (unwound_sp + 0x0C);
> + cache->saved_regs[6].set_addr (unwound_sp + 0x10);
> + cache->saved_regs[7].set_addr (unwound_sp + 0x14);
> + cache->saved_regs[8].set_addr (unwound_sp + 0x18);
> + cache->saved_regs[9].set_addr (unwound_sp + 0x1C);
> + cache->saved_regs[10].set_addr (unwound_sp + 0x20);
> + cache->saved_regs[11].set_addr (unwound_sp + 0x24);
> + sp_r0_offset = 0x28;
> + }
> +
> + /* The hardware saves eight 32-bit words, comprising xPSR,
> + ReturnAddress, LR (R14), R12, R3, R2, R1, R0. See details in
> + "B1.5.6 Exception entry behavior" in
> + "ARMv7-M Architecture Reference Manual". */
> + cache->saved_regs[0].set_addr (unwound_sp + sp_r0_offset);
> + cache->saved_regs[1].set_addr (unwound_sp + sp_r0_offset + 0x04);
> + cache->saved_regs[2].set_addr (unwound_sp + sp_r0_offset + 0x08);
> + cache->saved_regs[3].set_addr (unwound_sp + sp_r0_offset + 0x0C);
> + cache->saved_regs[ARM_IP_REGNUM].set_addr (unwound_sp + sp_r0_offset
> + + 0x10);
> + cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + sp_r0_offset
> + + 0x14);
> + cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + sp_r0_offset
> + + 0x18);
> + cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + sp_r0_offset
> + + 0x1C);
> +
> + /* Check EXC_RETURN bit FTYPE if extended stack frame (FPU regs stored)
> + type used. */
> + bool extended_frame_used = (bit (lr,4) == 0);
> + if (extended_frame_used)
> + {
> + ULONGEST fpccr;
> +
> + /* Read FPCCR register. */
> + gdb_assert (safe_read_memory_unsigned_integer (FPCCR,
> + ARM_INT_REGISTER_SIZE,
> + byte_order, &fpccr));
> + bool fpccr_ts = bit (fpccr,26);
Space after `,`
> +
> + /* This code does not take into account the lazy stacking, see "Lazy
> + context save of FP state", in B1.5.7, also ARM AN298, supported
> + by Cortex-M4F architecture.
> + To fully handle this the FPCCR register (Floating-point Context
> + Control Register) needs to be read out and the bits ASPEN and
> + LSPEN could be checked to setup correct lazy stacked FP registers.
> + This register is located at address 0xE000EF34. */
> +
> + /* Extended stack frame type used. */
> + CORE_ADDR addr = unwound_sp + sp_r0_offset + 0x20;
> + for (int i = 0; i < 8; i++)
> + {
> + cache->saved_regs[ARM_D0_REGNUM + i].set_addr (addr);
> + addr += 8;
> + }
> + cache->saved_regs[ARM_FPSCR_REGNUM].set_addr (unwound_sp
> + + sp_r0_offset + 0x60);
> +
> + if (tdep->have_sec_ext && !default_callee_register_stacking
> + && fpccr_ts)
> + {
> + /* Handle floating-point callee saved registers. */
> + addr = unwound_sp + sp_r0_offset + 0x68;
> + for (int i = 8; i < 16; i++)
> + {
> + cache->saved_regs[ARM_D0_REGNUM + i].set_addr (addr);
> + addr += 8;
> + }
>
> - if (tdep->have_sec_ext && !default_callee_register_stacking && fpccr_ts)
> - {
> - /* Handle floating-point callee saved registers. */
> - fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x68;
> - for (i = 8; i < 16; i++)
> + arm_cache_set_active_sp_value (cache, tdep,
> + unwound_sp + sp_r0_offset + 0xA8);
> + }
> + else
> {
> - cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);
> - fpu_regs_stack_offset += 8;
> + /* Offset 0x64 is reserved. */
> + arm_cache_set_active_sp_value (cache, tdep,
> + unwound_sp + sp_r0_offset + 0x68);
> }
> -
> - arm_cache_set_active_sp_value (cache, tdep,
> - unwound_sp + sp_r0_offset + 0xA8);
> }
> else
> {
> - /* Offset 0x64 is reserved. */
> + /* Standard stack frame type used. */
> arm_cache_set_active_sp_value (cache, tdep,
> - unwound_sp + sp_r0_offset + 0x68);
> + unwound_sp + sp_r0_offset + 0x20);
> }
> - }
> - else
> - {
> - /* Standard stack frame type used. */
> - arm_cache_set_active_sp_value (cache, tdep,
> - unwound_sp + sp_r0_offset + 0x20);
> +
> + /* If bit 9 of the saved xPSR is set, then there is a four-byte
> + aligner between the top of the 32-byte stack frame and the
> + previous context's stack pointer. */
> + ULONGEST xpsr;
> + gdb_assert (safe_read_memory_unsigned_integer (cache->saved_regs[
> + ARM_PS_REGNUM].addr (), 4,
> + byte_order, &xpsr));
> + if (bit (xpsr,9) != 0)
> + {
> + CORE_ADDR new_sp = arm_cache_get_prev_sp_value (cache, tdep) + 4;
> + arm_cache_set_active_sp_value (cache, tdep, new_sp);
> + }
> +
> + return cache;
> }
>
> - /* If bit 9 of the saved xPSR is set, then there is a four-byte
> - aligner between the top of the 32-byte stack frame and the
> - previous context's stack pointer. */
> - if (safe_read_memory_integer (unwound_sp + sp_r0_offset + 0x1C, 4,
> - byte_order, &xpsr)
> - && (xpsr & (1 << 9)) != 0)
> - arm_cache_set_active_sp_value (cache, tdep,
> - arm_cache_get_prev_sp_value (cache, tdep) + 4);
> + error (_("While unwinding an exception frame, found unexpected Link Register "
> + "value 0x%lx. This should not happen and may be caused by corrupt "
> + "data or a bug in GDB."), lr);
Same comment about using %s and phex as opposed to %lx.
What does this case have that is different from the previous error? Does it contain an
unrecognized LR value? If so, we should mention that explicitly to make it as helpful to
the user as possible.
>
> + /* Terminate any further stack unwinding by referring to self. */
> + arm_cache_set_active_sp_value (cache, tdep, sp);
> return cache;
This is dead code now. Nothing gets executed after error ().
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] gdb/arm: Cleanup of arm_m_exception_cache
2022-08-09 16:15 ` Luis Machado
@ 2022-08-09 17:13 ` Torbjorn SVENSSON
2022-08-10 10:53 ` Luis Machado
0 siblings, 1 reply; 4+ messages in thread
From: Torbjorn SVENSSON @ 2022-08-09 17:13 UTC (permalink / raw)
To: Luis Machado, gdb-patches
In addition to comments below, v3 will also contain a rephrased commit
message to make it clear that GDB does not call assert, but instead
errors out.
On 8/9/22 18:15, Luis Machado wrote:
> On 8/9/22 16:30, Torbjörn SVENSSON wrote:
>> With this change, only valid content of LR is accepted for the current
>> target. If the content for LR is anything but EXC_RETURN or FNC_RETURN
>> will cause GDB to assert since it's an invalid state for the unwinder.
>> FNC_RETURN pattern requires Security Extensions to be enabled or GDB
>> will assert due to the bad state of the unwinder.
>>
>> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
>> ---
>> gdb/arm-tdep.c | 380 ++++++++++++++++++++++++++-----------------------
>> 1 file changed, 204 insertions(+), 176 deletions(-)
>>
>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>> index cf8b610a381..299c416fe52 100644
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -3346,19 +3346,7 @@ arm_m_exception_cache (struct frame_info
>> *this_frame)
>> {
>> struct gdbarch *gdbarch = get_frame_arch (this_frame);
>> arm_gdbarch_tdep *tdep = gdbarch_tdep<arm_gdbarch_tdep> (gdbarch);
>> - enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> struct arm_prologue_cache *cache;
>> - CORE_ADDR lr;
>> - CORE_ADDR sp;
>> - CORE_ADDR unwound_sp;
>> - uint32_t sp_r0_offset = 0;
>> - LONGEST xpsr;
>> - uint32_t exc_return;
>> - bool fnc_return;
>> - uint32_t extended_frame_used;
>> - bool secure_stack_used = false;
>> - bool default_callee_register_stacking = false;
>> - bool exception_domain_is_secure = false;
>> cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
>> arm_cache_init (cache, this_frame);
>> @@ -3367,8 +3355,8 @@ arm_m_exception_cache (struct frame_info
>> *this_frame)
>> describes which bits in LR that define which stack was used prior
>> to the exception and if FPU is used (causing extended stack
>> frame). */
>> - lr = get_frame_register_unsigned (this_frame, ARM_LR_REGNUM);
>> - sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>> + CORE_ADDR lr = get_frame_register_unsigned (this_frame,
>> ARM_LR_REGNUM);
>> + CORE_ADDR sp = get_frame_register_unsigned (this_frame,
>> ARM_SP_REGNUM);
>> /* ARMv7-M Architecture Reference "A2.3.1 Arm core registers"
>> states that LR is set to 0xffffffff on reset. ARMv8-M
>> Architecture
>> @@ -3381,9 +3369,22 @@ arm_m_exception_cache (struct frame_info
>> *this_frame)
>> return cache;
>> }
>> - fnc_return = (((lr >> 24) & 0xff) == 0xfe);
>> - if (tdep->have_sec_ext && fnc_return)
>> + /* Check FNC_RETURN indicator bits (24-31). */
>> + bool fnc_return = (((lr >> 24) & 0xff) == 0xfe);
>> + if (fnc_return)
>> {
>> + /* FNC_RETURN is only valid for targets with Security
>> Extension. */
>> + if (!tdep->have_sec_ext)
>> + {
>> + error (_("While unwinding an exception frame, found unexpected
>> Link "
>> + "Register value 0x%lx. This should not happen and may be "
>
> You should use the %s format here and use phex to turn the 32-bit
> value to hex.
>
> Also, since this is checking explicitly for a value and the Security
> Extension, we should
> add that to the error message to make it more obvious what is failing.
>
> "While unwinding an exception frame, found unexpected Link Register
> value %s that requires the security extension, but the extension was
> not found or is disabled. This should not happen and may be caused by
> corrupt data or a bug in GDB."
Ok, string updated in v3.
>
>> + "caused by corrupt data or a bug in GDB."), lr);
>> +
>> + /* Terminate any further stack unwinding by referring to
>> self. */
>> + arm_cache_set_active_sp_value (cache, tdep, sp);
>> + return cache;
>
> Since you errored out, there's no use returning or executing any other
> statements after error.
Was uncertain if error() would bail or if it would just print the
message and continue. Thanks for letting me know!
>
>> + }
>> +
>> if (!arm_unwind_secure_frames)
>> {
>> warning (_("Non-secure to secure stack unwinding disabled."));
>> @@ -3393,7 +3394,7 @@ arm_m_exception_cache (struct frame_info
>> *this_frame)
>> return cache;
>> }
>> - xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
>> + ULONGEST xpsr = get_frame_register_unsigned (this_frame,
>> ARM_PS_REGNUM);
>> if ((xpsr & 0xff) != 0)
>> /* Handler mode: This is the mode that exceptions are handled
>> in. */
>> arm_cache_switch_prev_sp (cache, tdep,
>> tdep->m_profile_msp_s_regnum);
>> @@ -3401,7 +3402,7 @@ arm_m_exception_cache (struct frame_info
>> *this_frame)
>> /* 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);
>> + CORE_ADDR unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
>> /* Stack layout for a function call from Secure to
>> Non-Secure state
>> (ARMv8-M section B3.16):
>> @@ -3426,17 +3427,23 @@ arm_m_exception_cache (struct frame_info
>> *this_frame)
>> }
>> /* Check EXC_RETURN indicator bits (24-31). */
>> - exc_return = (((lr >> 24) & 0xff) == 0xff);
>> + bool exc_return = (((lr >> 24) & 0xff) == 0xff);
>> if (exc_return)
>> {
>> + int sp_regnum;
>> + bool secure_stack_used = false;
>> + bool default_callee_register_stacking = false;
>> + bool exception_domain_is_secure = false;
>> + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> +
>> /* Check EXC_RETURN bit SPSEL if Main or Thread (process)
>> stack used. */
>> - bool process_stack_used = ((lr & (1 << 2)) != 0);
>> + bool process_stack_used = (bit (lr,2) != 0);
>> if (tdep->have_sec_ext)
>> {
>> - secure_stack_used = ((lr & (1 << 6)) != 0);
>> - default_callee_register_stacking = ((lr & (1 << 5)) != 0);
>> - exception_domain_is_secure = ((lr & (1 << 0)) == 0);
>> + secure_stack_used = (bit (lr,6) != 0);
>
> Could you please address the formatting issues? space before `(`,
> space after `,`
I copied an existing usage of bit(), but sure. Will be corrected in v3.
>
>> + default_callee_register_stacking = (bit (lr,5) != 0);> +
>> exception_domain_is_secure = (bit (lr,0) == 0);
>> /* Unwinding from non-secure to secure can trip security
>> measures. In order to avoid the debugger being
>> @@ -3456,187 +3463,208 @@ arm_m_exception_cache (struct frame_info
>> *this_frame)
>> {
>> if (secure_stack_used)
>> /* Secure thread (process) stack used, use PSP_S as SP. */
>> - arm_cache_switch_prev_sp (cache, tdep,
>> tdep->m_profile_psp_s_regnum);
>> + sp_regnum = tdep->m_profile_psp_s_regnum;
>> else
>> /* Non-secure thread (process) stack used, use PSP_NS as
>> SP. */
>> - arm_cache_switch_prev_sp (cache, tdep,
>> tdep->m_profile_psp_ns_regnum);
>> + sp_regnum = tdep->m_profile_psp_ns_regnum;
>> }
>> else
>> {
>> if (secure_stack_used)
>> /* Secure main stack used, use MSP_S as SP. */
>> - arm_cache_switch_prev_sp (cache, tdep,
>> tdep->m_profile_msp_s_regnum);
>> + sp_regnum = tdep->m_profile_msp_s_regnum;
>> else
>> /* Non-secure main stack used, use MSP_NS as SP. */
>> - arm_cache_switch_prev_sp (cache, tdep,
>> tdep->m_profile_msp_ns_regnum);
>> + sp_regnum = tdep->m_profile_msp_ns_regnum;
>> }
>> }
>> else
>> {
>> if (process_stack_used)
>> /* Thread (process) stack used, use PSP as SP. */
>> - arm_cache_switch_prev_sp (cache, tdep,
>> tdep->m_profile_psp_regnum);
>> + sp_regnum = tdep->m_profile_psp_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);
>> -
>> - /* Exception entry context stacking are described in ARMv8-M
>> (section B3.19)
>> - and ARMv7-M (sections B1.5.6 and B1.5.7) Architecture Reference
>> Manuals.
>> -
>> - The following figure shows the structure of the stack frame
>> when Security
>> - and Floating-point extensions are present.
>> -
>> - SP Offsets
>> - Without With
>> - Callee Regs Callee Regs
>> - (Secure -> Non-Secure)
>> - +-------------------+
>> - 0xA8 | | 0xD0
>> - +===================+ --+ <-- Original SP
>> - 0xA4 | S31 | 0xCC |
>> - +-------------------+ |
>> - ... | Additional FP
>> context
>> - +-------------------+ |
>> - 0x68 | S16 | 0x90 |
>> - +===================+ --+
>> - 0x64 | Reserved | 0x8C |
>> - +-------------------+ |
>> - 0x60 | FPSCR | 0x88 |
>> - +-------------------+ |
>> - 0x5C | S15 | 0x84 | FP context
>> - +-------------------+ |
>> - ... |
>> - +-------------------+ |
>> - 0x20 | S0 | 0x48 |
>> - +===================+ --+
>> - 0x1C | xPSR | 0x44 |
>> - +-------------------+ |
>> - 0x18 | Return address | 0x40 |
>> - +-------------------+ |
>> - 0x14 | LR(R14) | 0x3C |
>> - +-------------------+ |
>> - 0x10 | R12 | 0x38 | State context
>> - +-------------------+ |
>> - 0x0C | R3 | 0x34 |
>> - +-------------------+ |
>> - ... |
>> - +-------------------+ |
>> - 0x00 | R0 | 0x28 |
>> - +===================+ --+
>> - | R11 | 0x24 |
>> - +-------------------+ |
>> - ... |
>> - +-------------------+ | Additional
>> state context
>> - | R4 | 0x08 | when
>> transitioning from
>> - +-------------------+ | Secure to
>> Non-Secure
>> - | Reserved | 0x04 |
>> - +-------------------+ |
>> - | Magic signature | 0x00 |
>> - +===================+ --+ <-- New SP */
>> -
>> - /* With the Security extension, the hardware saves R4..R11 too. */
>> - if (exc_return && tdep->have_sec_ext && secure_stack_used
>> - && (!default_callee_register_stacking ||
>> exception_domain_is_secure))
>> - {
>> - /* Read R4..R11 from the integer callee registers. */
>> - cache->saved_regs[4].set_addr (unwound_sp + 0x08);
>> - cache->saved_regs[5].set_addr (unwound_sp + 0x0C);
>> - cache->saved_regs[6].set_addr (unwound_sp + 0x10);
>> - cache->saved_regs[7].set_addr (unwound_sp + 0x14);
>> - cache->saved_regs[8].set_addr (unwound_sp + 0x18);
>> - cache->saved_regs[9].set_addr (unwound_sp + 0x1C);
>> - cache->saved_regs[10].set_addr (unwound_sp + 0x20);
>> - cache->saved_regs[11].set_addr (unwound_sp + 0x24);
>> - sp_r0_offset = 0x28;
>> - }
>> -
>> - /* The hardware saves eight 32-bit words, comprising xPSR,
>> - ReturnAddress, LR (R14), R12, R3, R2, R1, R0. See details in
>> - "B1.5.6 Exception entry behavior" in
>> - "ARMv7-M Architecture Reference Manual". */
>> - cache->saved_regs[0].set_addr (unwound_sp + sp_r0_offset);
>> - cache->saved_regs[1].set_addr (unwound_sp + sp_r0_offset + 0x04);
>> - cache->saved_regs[2].set_addr (unwound_sp + sp_r0_offset + 0x08);
>> - cache->saved_regs[3].set_addr (unwound_sp + sp_r0_offset + 0x0C);
>> - cache->saved_regs[ARM_IP_REGNUM].set_addr (unwound_sp +
>> sp_r0_offset + 0x10);
>> - cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp +
>> sp_r0_offset + 0x14);
>> - cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp +
>> sp_r0_offset + 0x18);
>> - cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp +
>> sp_r0_offset + 0x1C);
>> -
>> - /* Check EXC_RETURN bit FTYPE if extended stack frame (FPU regs
>> stored)
>> - type used. */
>> - extended_frame_used = ((lr & (1 << 4)) == 0);
>> - if (exc_return && extended_frame_used)
>> - {
>> - int i;
>> - int fpu_regs_stack_offset;
>> - ULONGEST fpccr;
>> -
>> - /* Read FPCCR register. */
>> - gdb_assert (safe_read_memory_unsigned_integer (FPCCR,
>> - ARM_INT_REGISTER_SIZE,
>> - byte_order, &fpccr));
>> - bool fpccr_ts = bit (fpccr,26);
>> -
>> - /* This code does not take into account the lazy stacking, see
>> "Lazy
>> - context save of FP state", in B1.5.7, also ARM AN298, supported
>> - by Cortex-M4F architecture.
>> - To fully handle this the FPCCR register (Floating-point Context
>> - Control Register) needs to be read out and the bits ASPEN and
>> LSPEN
>> - could be checked to setup correct lazy stacked FP registers.
>> - This register is located at address 0xE000EF34. */
>> -
>> - /* Extended stack frame type used. */
>> - fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x20;
>> - for (i = 0; i < 8; i++)
>> - {
>> - cache->saved_regs[ARM_D0_REGNUM + i].set_addr
>> (fpu_regs_stack_offset);
>> - fpu_regs_stack_offset += 8;
>> - }
>> - cache->saved_regs[ARM_FPSCR_REGNUM].set_addr (unwound_sp +
>> sp_r0_offset + 0x60);
>> - fpu_regs_stack_offset += 4;
>> + sp_regnum = tdep->m_profile_msp_regnum;
>> + }
>> +
>> + /* Set the active SP regnum. */
>> + arm_cache_switch_prev_sp (cache, tdep, sp_regnum);
>> +
>> + /* Fetch the SP to use for this frame. */
>> + CORE_ADDR unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
>> +
>> + /* Exception entry context stacking are described in ARMv8-M
>> (section
>> + B3.19) and ARMv7-M (sections B1.5.6 and B1.5.7) Architecture
>> Reference
>> + Manuals.
>> +
>> + The following figure shows the structure of the stack frame when
>> + Security and Floating-point extensions are present.
>> +
>> + SP Offsets
>> + Without With
>> + Callee Regs Callee Regs
>> + (Secure -> Non-Secure)
>> + +-------------------+
>> + 0xA8 | | 0xD0
>> + +===================+ --+ <-- Original SP
>> + 0xA4 | S31 | 0xCC |
>> + +-------------------+ |
>> + ... | Additional FP context
>> + +-------------------+ |
>> + 0x68 | S16 | 0x90 |
>> + +===================+ --+
>> + 0x64 | Reserved | 0x8C |
>> + +-------------------+ |
>> + 0x60 | FPSCR | 0x88 |
>> + +-------------------+ |
>> + 0x5C | S15 | 0x84 | FP context
>> + +-------------------+ |
>> + ... |
>> + +-------------------+ |
>> + 0x20 | S0 | 0x48 |
>> + +===================+ --+
>> + 0x1C | xPSR | 0x44 |
>> + +-------------------+ |
>> + 0x18 | Return address | 0x40 |
>> + +-------------------+ |
>> + 0x14 | LR(R14) | 0x3C |
>> + +-------------------+ |
>> + 0x10 | R12 | 0x38 | State context
>> + +-------------------+ |
>> + 0x0C | R3 | 0x34 |
>> + +-------------------+ |
>> + ... |
>> + +-------------------+ |
>> + 0x00 | R0 | 0x28 |
>> + +===================+ --+
>> + | R11 | 0x24 |
>> + +-------------------+ |
>> + ... |
>> + +-------------------+ | Additional state
>> + | R4 | 0x08 | context when
>> + +-------------------+ | transitioning from
>> + | Reserved | 0x04 | Secure to Non-Secure
>> + +-------------------+ |
>> + | Magic signature | 0x00 |
>> + +===================+ --+ <-- New SP */
>> +
>> + uint32_t sp_r0_offset = 0;
>> +
>> + /* With the Security extension, the hardware saves R4..R11
>> too. */
>> + if (tdep->have_sec_ext && secure_stack_used
>> + && (!default_callee_register_stacking ||
>> exception_domain_is_secure))
>> + {
>> + /* Read R4..R11 from the integer callee registers. */
>> + cache->saved_regs[4].set_addr (unwound_sp + 0x08);
>> + cache->saved_regs[5].set_addr (unwound_sp + 0x0C);
>> + cache->saved_regs[6].set_addr (unwound_sp + 0x10);
>> + cache->saved_regs[7].set_addr (unwound_sp + 0x14);
>> + cache->saved_regs[8].set_addr (unwound_sp + 0x18);
>> + cache->saved_regs[9].set_addr (unwound_sp + 0x1C);
>> + cache->saved_regs[10].set_addr (unwound_sp + 0x20);
>> + cache->saved_regs[11].set_addr (unwound_sp + 0x24);
>> + sp_r0_offset = 0x28;
>> + }
>> +
>> + /* The hardware saves eight 32-bit words, comprising xPSR,
>> + ReturnAddress, LR (R14), R12, R3, R2, R1, R0. See details in
>> + "B1.5.6 Exception entry behavior" in
>> + "ARMv7-M Architecture Reference Manual". */
>> + cache->saved_regs[0].set_addr (unwound_sp + sp_r0_offset);
>> + cache->saved_regs[1].set_addr (unwound_sp + sp_r0_offset + 0x04);
>> + cache->saved_regs[2].set_addr (unwound_sp + sp_r0_offset + 0x08);
>> + cache->saved_regs[3].set_addr (unwound_sp + sp_r0_offset + 0x0C);
>> + cache->saved_regs[ARM_IP_REGNUM].set_addr (unwound_sp +
>> sp_r0_offset
>> + + 0x10);
>> + cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp +
>> sp_r0_offset
>> + + 0x14);
>> + cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp +
>> sp_r0_offset
>> + + 0x18);
>> + cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp +
>> sp_r0_offset
>> + + 0x1C);
>> +
>> + /* Check EXC_RETURN bit FTYPE if extended stack frame (FPU
>> regs stored)
>> + type used. */
>> + bool extended_frame_used = (bit (lr,4) == 0);
>> + if (extended_frame_used)
>> + {
>> + ULONGEST fpccr;
>> +
>> + /* Read FPCCR register. */
>> + gdb_assert (safe_read_memory_unsigned_integer (FPCCR,
>> + ARM_INT_REGISTER_SIZE,
>> + byte_order, &fpccr));
>> + bool fpccr_ts = bit (fpccr,26);
>
> Space after `,`
>
>> +
>> + /* This code does not take into account the lazy stacking, see
>> "Lazy
>> + context save of FP state", in B1.5.7, also ARM AN298,
>> supported
>> + by Cortex-M4F architecture.
>> + To fully handle this the FPCCR register (Floating-point
>> Context
>> + Control Register) needs to be read out and the bits ASPEN and
>> + LSPEN could be checked to setup correct lazy stacked FP
>> registers.
>> + This register is located at address 0xE000EF34. */
>> +
>> + /* Extended stack frame type used. */
>> + CORE_ADDR addr = unwound_sp + sp_r0_offset + 0x20;
>> + for (int i = 0; i < 8; i++)
>> + {
>> + cache->saved_regs[ARM_D0_REGNUM + i].set_addr (addr);
>> + addr += 8;
>> + }
>> + cache->saved_regs[ARM_FPSCR_REGNUM].set_addr (unwound_sp
>> + + sp_r0_offset + 0x60);
>> +
>> + if (tdep->have_sec_ext && !default_callee_register_stacking
>> + && fpccr_ts)
>> + {
>> + /* Handle floating-point callee saved registers. */
>> + addr = unwound_sp + sp_r0_offset + 0x68;
>> + for (int i = 8; i < 16; i++)
>> + {
>> + cache->saved_regs[ARM_D0_REGNUM + i].set_addr (addr);
>> + addr += 8;
>> + }
>> - if (tdep->have_sec_ext && !default_callee_register_stacking
>> && fpccr_ts)
>> - {
>> - /* Handle floating-point callee saved registers. */
>> - fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x68;
>> - for (i = 8; i < 16; i++)
>> + arm_cache_set_active_sp_value (cache, tdep,
>> + unwound_sp + sp_r0_offset + 0xA8);
>> + }
>> + else
>> {
>> - cache->saved_regs[ARM_D0_REGNUM + i].set_addr
>> (fpu_regs_stack_offset);
>> - fpu_regs_stack_offset += 8;
>> + /* Offset 0x64 is reserved. */
>> + arm_cache_set_active_sp_value (cache, tdep,
>> + unwound_sp + sp_r0_offset + 0x68);
>> }
>> -
>> - arm_cache_set_active_sp_value (cache, tdep,
>> - unwound_sp + sp_r0_offset + 0xA8);
>> }
>> else
>> {
>> - /* Offset 0x64 is reserved. */
>> + /* Standard stack frame type used. */
>> arm_cache_set_active_sp_value (cache, tdep,
>> - unwound_sp + sp_r0_offset + 0x68);
>> + unwound_sp + sp_r0_offset + 0x20);
>> }
>> - }
>> - else
>> - {
>> - /* Standard stack frame type used. */
>> - arm_cache_set_active_sp_value (cache, tdep,
>> - unwound_sp + sp_r0_offset + 0x20);
>> +
>> + /* If bit 9 of the saved xPSR is set, then there is a four-byte
>> + aligner between the top of the 32-byte stack frame and the
>> + previous context's stack pointer. */
>> + ULONGEST xpsr;
>> + gdb_assert (safe_read_memory_unsigned_integer (cache->saved_regs[
>> + ARM_PS_REGNUM].addr (), 4,
>> + byte_order, &xpsr));
>> + if (bit (xpsr,9) != 0)
>> + {
>> + CORE_ADDR new_sp = arm_cache_get_prev_sp_value (cache, tdep) + 4;
>> + arm_cache_set_active_sp_value (cache, tdep, new_sp);
>> + }
>> +
>> + return cache;
>> }
>> - /* If bit 9 of the saved xPSR is set, then there is a four-byte
>> - aligner between the top of the 32-byte stack frame and the
>> - previous context's stack pointer. */
>> - if (safe_read_memory_integer (unwound_sp + sp_r0_offset + 0x1C, 4,
>> - byte_order, &xpsr)
>> - && (xpsr & (1 << 9)) != 0)
>> - arm_cache_set_active_sp_value (cache, tdep,
>> - arm_cache_get_prev_sp_value (cache, tdep) + 4);
>> + error (_("While unwinding an exception frame, found unexpected
>> Link Register "
>> + "value 0x%lx. This should not happen and may be caused by
>> corrupt "
>> + "data or a bug in GDB."), lr);
>
> Same comment about using %s and phex as opposed to %lx.
>
> What does this case have that is different from the previous error?
> Does it contain an
> unrecognized LR value? If so, we should mention that explicitly to
> make it as helpful to
> the user as possible.
The difference is that for the security extension check above, that is a
wrongful state of the hardware. This case down here is when LR does not
match an EXC_RETURN or FNC_RETURN pattern, thus is an inconsistency
between the sniffer and the function that creates the cache object.
The error case in the bottom should never happen as the sniffer has
checked that LR matches either EXC_RETURN or FNC_RETURN, so it's more of
a safe guard in case something is broken in GDB itself. The case with
the security extension check should not happen neither, but I suppose it
could happen if the data that is fetched from the target is corrupt in a
certain way...
Maybe the last one should be internal_error() instead of error()?
>
>
>> + /* Terminate any further stack unwinding by referring to self. */
>> + arm_cache_set_active_sp_value (cache, tdep, sp);
>> return cache;
>
> This is dead code now. Nothing gets executed after error ().
>
>> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] gdb/arm: Cleanup of arm_m_exception_cache
2022-08-09 17:13 ` Torbjorn SVENSSON
@ 2022-08-10 10:53 ` Luis Machado
0 siblings, 0 replies; 4+ messages in thread
From: Luis Machado @ 2022-08-10 10:53 UTC (permalink / raw)
To: Torbjorn SVENSSON, gdb-patches
On 8/9/22 18:13, Torbjorn SVENSSON wrote:
> In addition to comments below, v3 will also contain a rephrased commit message to make it clear that GDB does not call assert, but instead errors out.
>
> On 8/9/22 18:15, Luis Machado wrote:
>> On 8/9/22 16:30, Torbjörn SVENSSON wrote:
>>> With this change, only valid content of LR is accepted for the current
>>> target. If the content for LR is anything but EXC_RETURN or FNC_RETURN
>>> will cause GDB to assert since it's an invalid state for the unwinder.
>>> FNC_RETURN pattern requires Security Extensions to be enabled or GDB
>>> will assert due to the bad state of the unwinder.
>>>
>>> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
>>> ---
>>> gdb/arm-tdep.c | 380 ++++++++++++++++++++++++++-----------------------
>>> 1 file changed, 204 insertions(+), 176 deletions(-)
>>>
>>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>>> index cf8b610a381..299c416fe52 100644
>>> --- a/gdb/arm-tdep.c
>>> +++ b/gdb/arm-tdep.c
>>> @@ -3346,19 +3346,7 @@ arm_m_exception_cache (struct frame_info *this_frame)
>>> {
>>> struct gdbarch *gdbarch = get_frame_arch (this_frame);
>>> arm_gdbarch_tdep *tdep = gdbarch_tdep<arm_gdbarch_tdep> (gdbarch);
>>> - enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>> struct arm_prologue_cache *cache;
>>> - CORE_ADDR lr;
>>> - CORE_ADDR sp;
>>> - CORE_ADDR unwound_sp;
>>> - uint32_t sp_r0_offset = 0;
>>> - LONGEST xpsr;
>>> - uint32_t exc_return;
>>> - bool fnc_return;
>>> - uint32_t extended_frame_used;
>>> - bool secure_stack_used = false;
>>> - bool default_callee_register_stacking = false;
>>> - bool exception_domain_is_secure = false;
>>> cache = FRAME_OBSTACK_ZALLOC (struct arm_prologue_cache);
>>> arm_cache_init (cache, this_frame);
>>> @@ -3367,8 +3355,8 @@ arm_m_exception_cache (struct frame_info *this_frame)
>>> describes which bits in LR that define which stack was used prior
>>> to the exception and if FPU is used (causing extended stack frame). */
>>> - lr = get_frame_register_unsigned (this_frame, ARM_LR_REGNUM);
>>> - sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>>> + CORE_ADDR lr = get_frame_register_unsigned (this_frame, ARM_LR_REGNUM);
>>> + CORE_ADDR sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>>> /* ARMv7-M Architecture Reference "A2.3.1 Arm core registers"
>>> states that LR is set to 0xffffffff on reset. ARMv8-M Architecture
>>> @@ -3381,9 +3369,22 @@ arm_m_exception_cache (struct frame_info *this_frame)
>>> return cache;
>>> }
>>> - fnc_return = (((lr >> 24) & 0xff) == 0xfe);
>>> - if (tdep->have_sec_ext && fnc_return)
>>> + /* Check FNC_RETURN indicator bits (24-31). */
>>> + bool fnc_return = (((lr >> 24) & 0xff) == 0xfe);
>>> + if (fnc_return)
>>> {
>>> + /* FNC_RETURN is only valid for targets with Security Extension. */
>>> + if (!tdep->have_sec_ext)
>>> + {
>>> + error (_("While unwinding an exception frame, found unexpected Link "
>>> + "Register value 0x%lx. This should not happen and may be "
>>
>> You should use the %s format here and use phex to turn the 32-bit value to hex.
>>
>> Also, since this is checking explicitly for a value and the Security Extension, we should
>> add that to the error message to make it more obvious what is failing.
>>
>> "While unwinding an exception frame, found unexpected Link Register value %s that requires the security extension, but the extension was not found or is disabled. This should not happen and may be caused by corrupt data or a bug in GDB."
> Ok, string updated in v3.
>>
>>> + "caused by corrupt data or a bug in GDB."), lr);
>>> +
>>> + /* Terminate any further stack unwinding by referring to self. */
>>> + arm_cache_set_active_sp_value (cache, tdep, sp);
>>> + return cache;
>>
>> Since you errored out, there's no use returning or executing any other statements after error.
> Was uncertain if error() would bail or if it would just print the message and continue. Thanks for letting me know!
>>
>>> + }
>>> +
>>> if (!arm_unwind_secure_frames)
>>> {
>>> warning (_("Non-secure to secure stack unwinding disabled."));
>>> @@ -3393,7 +3394,7 @@ arm_m_exception_cache (struct frame_info *this_frame)
>>> return cache;
>>> }
>>> - xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
>>> + ULONGEST xpsr = get_frame_register_unsigned (this_frame, ARM_PS_REGNUM);
>>> if ((xpsr & 0xff) != 0)
>>> /* Handler mode: This is the mode that exceptions are handled in. */
>>> arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum);
>>> @@ -3401,7 +3402,7 @@ arm_m_exception_cache (struct frame_info *this_frame)
>>> /* 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);
>>> + CORE_ADDR unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
>>> /* Stack layout for a function call from Secure to Non-Secure state
>>> (ARMv8-M section B3.16):
>>> @@ -3426,17 +3427,23 @@ arm_m_exception_cache (struct frame_info *this_frame)
>>> }
>>> /* Check EXC_RETURN indicator bits (24-31). */
>>> - exc_return = (((lr >> 24) & 0xff) == 0xff);
>>> + bool exc_return = (((lr >> 24) & 0xff) == 0xff);
>>> if (exc_return)
>>> {
>>> + int sp_regnum;
>>> + bool secure_stack_used = false;
>>> + bool default_callee_register_stacking = false;
>>> + bool exception_domain_is_secure = false;
>>> + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>> +
>>> /* Check EXC_RETURN bit SPSEL if Main or Thread (process) stack used. */
>>> - bool process_stack_used = ((lr & (1 << 2)) != 0);
>>> + bool process_stack_used = (bit (lr,2) != 0);
>>> if (tdep->have_sec_ext)
>>> {
>>> - secure_stack_used = ((lr & (1 << 6)) != 0);
>>> - default_callee_register_stacking = ((lr & (1 << 5)) != 0);
>>> - exception_domain_is_secure = ((lr & (1 << 0)) == 0);
>>> + secure_stack_used = (bit (lr,6) != 0);
>>
>> Could you please address the formatting issues? space before `(`, space after `,`
> I copied an existing usage of bit(), but sure. Will be corrected in v3.
>>
>>> + default_callee_register_stacking = (bit (lr,5) != 0);> + exception_domain_is_secure = (bit (lr,0) == 0);
>>> /* Unwinding from non-secure to secure can trip security
>>> measures. In order to avoid the debugger being
>>> @@ -3456,187 +3463,208 @@ arm_m_exception_cache (struct frame_info *this_frame)
>>> {
>>> if (secure_stack_used)
>>> /* Secure thread (process) stack used, use PSP_S as SP. */
>>> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_s_regnum);
>>> + sp_regnum = tdep->m_profile_psp_s_regnum;
>>> else
>>> /* Non-secure thread (process) stack used, use PSP_NS as SP. */
>>> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_ns_regnum);
>>> + sp_regnum = tdep->m_profile_psp_ns_regnum;
>>> }
>>> else
>>> {
>>> if (secure_stack_used)
>>> /* Secure main stack used, use MSP_S as SP. */
>>> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_s_regnum);
>>> + sp_regnum = tdep->m_profile_msp_s_regnum;
>>> else
>>> /* Non-secure main stack used, use MSP_NS as SP. */
>>> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_msp_ns_regnum);
>>> + sp_regnum = tdep->m_profile_msp_ns_regnum;
>>> }
>>> }
>>> else
>>> {
>>> if (process_stack_used)
>>> /* Thread (process) stack used, use PSP as SP. */
>>> - arm_cache_switch_prev_sp (cache, tdep, tdep->m_profile_psp_regnum);
>>> + sp_regnum = tdep->m_profile_psp_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);
>>> -
>>> - /* Exception entry context stacking are described in ARMv8-M (section B3.19)
>>> - and ARMv7-M (sections B1.5.6 and B1.5.7) Architecture Reference Manuals.
>>> -
>>> - The following figure shows the structure of the stack frame when Security
>>> - and Floating-point extensions are present.
>>> -
>>> - SP Offsets
>>> - Without With
>>> - Callee Regs Callee Regs
>>> - (Secure -> Non-Secure)
>>> - +-------------------+
>>> - 0xA8 | | 0xD0
>>> - +===================+ --+ <-- Original SP
>>> - 0xA4 | S31 | 0xCC |
>>> - +-------------------+ |
>>> - ... | Additional FP context
>>> - +-------------------+ |
>>> - 0x68 | S16 | 0x90 |
>>> - +===================+ --+
>>> - 0x64 | Reserved | 0x8C |
>>> - +-------------------+ |
>>> - 0x60 | FPSCR | 0x88 |
>>> - +-------------------+ |
>>> - 0x5C | S15 | 0x84 | FP context
>>> - +-------------------+ |
>>> - ... |
>>> - +-------------------+ |
>>> - 0x20 | S0 | 0x48 |
>>> - +===================+ --+
>>> - 0x1C | xPSR | 0x44 |
>>> - +-------------------+ |
>>> - 0x18 | Return address | 0x40 |
>>> - +-------------------+ |
>>> - 0x14 | LR(R14) | 0x3C |
>>> - +-------------------+ |
>>> - 0x10 | R12 | 0x38 | State context
>>> - +-------------------+ |
>>> - 0x0C | R3 | 0x34 |
>>> - +-------------------+ |
>>> - ... |
>>> - +-------------------+ |
>>> - 0x00 | R0 | 0x28 |
>>> - +===================+ --+
>>> - | R11 | 0x24 |
>>> - +-------------------+ |
>>> - ... |
>>> - +-------------------+ | Additional state context
>>> - | R4 | 0x08 | when transitioning from
>>> - +-------------------+ | Secure to Non-Secure
>>> - | Reserved | 0x04 |
>>> - +-------------------+ |
>>> - | Magic signature | 0x00 |
>>> - +===================+ --+ <-- New SP */
>>> -
>>> - /* With the Security extension, the hardware saves R4..R11 too. */
>>> - if (exc_return && tdep->have_sec_ext && secure_stack_used
>>> - && (!default_callee_register_stacking || exception_domain_is_secure))
>>> - {
>>> - /* Read R4..R11 from the integer callee registers. */
>>> - cache->saved_regs[4].set_addr (unwound_sp + 0x08);
>>> - cache->saved_regs[5].set_addr (unwound_sp + 0x0C);
>>> - cache->saved_regs[6].set_addr (unwound_sp + 0x10);
>>> - cache->saved_regs[7].set_addr (unwound_sp + 0x14);
>>> - cache->saved_regs[8].set_addr (unwound_sp + 0x18);
>>> - cache->saved_regs[9].set_addr (unwound_sp + 0x1C);
>>> - cache->saved_regs[10].set_addr (unwound_sp + 0x20);
>>> - cache->saved_regs[11].set_addr (unwound_sp + 0x24);
>>> - sp_r0_offset = 0x28;
>>> - }
>>> -
>>> - /* The hardware saves eight 32-bit words, comprising xPSR,
>>> - ReturnAddress, LR (R14), R12, R3, R2, R1, R0. See details in
>>> - "B1.5.6 Exception entry behavior" in
>>> - "ARMv7-M Architecture Reference Manual". */
>>> - cache->saved_regs[0].set_addr (unwound_sp + sp_r0_offset);
>>> - cache->saved_regs[1].set_addr (unwound_sp + sp_r0_offset + 0x04);
>>> - cache->saved_regs[2].set_addr (unwound_sp + sp_r0_offset + 0x08);
>>> - cache->saved_regs[3].set_addr (unwound_sp + sp_r0_offset + 0x0C);
>>> - cache->saved_regs[ARM_IP_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x10);
>>> - cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x14);
>>> - cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x18);
>>> - cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x1C);
>>> -
>>> - /* Check EXC_RETURN bit FTYPE if extended stack frame (FPU regs stored)
>>> - type used. */
>>> - extended_frame_used = ((lr & (1 << 4)) == 0);
>>> - if (exc_return && extended_frame_used)
>>> - {
>>> - int i;
>>> - int fpu_regs_stack_offset;
>>> - ULONGEST fpccr;
>>> -
>>> - /* Read FPCCR register. */
>>> - gdb_assert (safe_read_memory_unsigned_integer (FPCCR,
>>> - ARM_INT_REGISTER_SIZE,
>>> - byte_order, &fpccr));
>>> - bool fpccr_ts = bit (fpccr,26);
>>> -
>>> - /* This code does not take into account the lazy stacking, see "Lazy
>>> - context save of FP state", in B1.5.7, also ARM AN298, supported
>>> - by Cortex-M4F architecture.
>>> - To fully handle this the FPCCR register (Floating-point Context
>>> - Control Register) needs to be read out and the bits ASPEN and LSPEN
>>> - could be checked to setup correct lazy stacked FP registers.
>>> - This register is located at address 0xE000EF34. */
>>> -
>>> - /* Extended stack frame type used. */
>>> - fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x20;
>>> - for (i = 0; i < 8; i++)
>>> - {
>>> - cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);
>>> - fpu_regs_stack_offset += 8;
>>> - }
>>> - cache->saved_regs[ARM_FPSCR_REGNUM].set_addr (unwound_sp + sp_r0_offset + 0x60);
>>> - fpu_regs_stack_offset += 4;
>>> + sp_regnum = tdep->m_profile_msp_regnum;
>>> + }
>>> +
>>> + /* Set the active SP regnum. */
>>> + arm_cache_switch_prev_sp (cache, tdep, sp_regnum);
>>> +
>>> + /* Fetch the SP to use for this frame. */
>>> + CORE_ADDR unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
>>> +
>>> + /* Exception entry context stacking are described in ARMv8-M (section
>>> + B3.19) and ARMv7-M (sections B1.5.6 and B1.5.7) Architecture Reference
>>> + Manuals.
>>> +
>>> + The following figure shows the structure of the stack frame when
>>> + Security and Floating-point extensions are present.
>>> +
>>> + SP Offsets
>>> + Without With
>>> + Callee Regs Callee Regs
>>> + (Secure -> Non-Secure)
>>> + +-------------------+
>>> + 0xA8 | | 0xD0
>>> + +===================+ --+ <-- Original SP
>>> + 0xA4 | S31 | 0xCC |
>>> + +-------------------+ |
>>> + ... | Additional FP context
>>> + +-------------------+ |
>>> + 0x68 | S16 | 0x90 |
>>> + +===================+ --+
>>> + 0x64 | Reserved | 0x8C |
>>> + +-------------------+ |
>>> + 0x60 | FPSCR | 0x88 |
>>> + +-------------------+ |
>>> + 0x5C | S15 | 0x84 | FP context
>>> + +-------------------+ |
>>> + ... |
>>> + +-------------------+ |
>>> + 0x20 | S0 | 0x48 |
>>> + +===================+ --+
>>> + 0x1C | xPSR | 0x44 |
>>> + +-------------------+ |
>>> + 0x18 | Return address | 0x40 |
>>> + +-------------------+ |
>>> + 0x14 | LR(R14) | 0x3C |
>>> + +-------------------+ |
>>> + 0x10 | R12 | 0x38 | State context
>>> + +-------------------+ |
>>> + 0x0C | R3 | 0x34 |
>>> + +-------------------+ |
>>> + ... |
>>> + +-------------------+ |
>>> + 0x00 | R0 | 0x28 |
>>> + +===================+ --+
>>> + | R11 | 0x24 |
>>> + +-------------------+ |
>>> + ... |
>>> + +-------------------+ | Additional state
>>> + | R4 | 0x08 | context when
>>> + +-------------------+ | transitioning from
>>> + | Reserved | 0x04 | Secure to Non-Secure
>>> + +-------------------+ |
>>> + | Magic signature | 0x00 |
>>> + +===================+ --+ <-- New SP */
>>> +
>>> + uint32_t sp_r0_offset = 0;
>>> +
>>> + /* With the Security extension, the hardware saves R4..R11 too. */
>>> + if (tdep->have_sec_ext && secure_stack_used
>>> + && (!default_callee_register_stacking || exception_domain_is_secure))
>>> + {
>>> + /* Read R4..R11 from the integer callee registers. */
>>> + cache->saved_regs[4].set_addr (unwound_sp + 0x08);
>>> + cache->saved_regs[5].set_addr (unwound_sp + 0x0C);
>>> + cache->saved_regs[6].set_addr (unwound_sp + 0x10);
>>> + cache->saved_regs[7].set_addr (unwound_sp + 0x14);
>>> + cache->saved_regs[8].set_addr (unwound_sp + 0x18);
>>> + cache->saved_regs[9].set_addr (unwound_sp + 0x1C);
>>> + cache->saved_regs[10].set_addr (unwound_sp + 0x20);
>>> + cache->saved_regs[11].set_addr (unwound_sp + 0x24);
>>> + sp_r0_offset = 0x28;
>>> + }
>>> +
>>> + /* The hardware saves eight 32-bit words, comprising xPSR,
>>> + ReturnAddress, LR (R14), R12, R3, R2, R1, R0. See details in
>>> + "B1.5.6 Exception entry behavior" in
>>> + "ARMv7-M Architecture Reference Manual". */
>>> + cache->saved_regs[0].set_addr (unwound_sp + sp_r0_offset);
>>> + cache->saved_regs[1].set_addr (unwound_sp + sp_r0_offset + 0x04);
>>> + cache->saved_regs[2].set_addr (unwound_sp + sp_r0_offset + 0x08);
>>> + cache->saved_regs[3].set_addr (unwound_sp + sp_r0_offset + 0x0C);
>>> + cache->saved_regs[ARM_IP_REGNUM].set_addr (unwound_sp + sp_r0_offset
>>> + + 0x10);
>>> + cache->saved_regs[ARM_LR_REGNUM].set_addr (unwound_sp + sp_r0_offset
>>> + + 0x14);
>>> + cache->saved_regs[ARM_PC_REGNUM].set_addr (unwound_sp + sp_r0_offset
>>> + + 0x18);
>>> + cache->saved_regs[ARM_PS_REGNUM].set_addr (unwound_sp + sp_r0_offset
>>> + + 0x1C);
>>> +
>>> + /* Check EXC_RETURN bit FTYPE if extended stack frame (FPU regs stored)
>>> + type used. */
>>> + bool extended_frame_used = (bit (lr,4) == 0);
>>> + if (extended_frame_used)
>>> + {
>>> + ULONGEST fpccr;
>>> +
>>> + /* Read FPCCR register. */
>>> + gdb_assert (safe_read_memory_unsigned_integer (FPCCR,
>>> + ARM_INT_REGISTER_SIZE,
>>> + byte_order, &fpccr));
>>> + bool fpccr_ts = bit (fpccr,26);
>>
>> Space after `,`
>>
>>> +
>>> + /* This code does not take into account the lazy stacking, see "Lazy
>>> + context save of FP state", in B1.5.7, also ARM AN298, supported
>>> + by Cortex-M4F architecture.
>>> + To fully handle this the FPCCR register (Floating-point Context
>>> + Control Register) needs to be read out and the bits ASPEN and
>>> + LSPEN could be checked to setup correct lazy stacked FP registers.
>>> + This register is located at address 0xE000EF34. */
>>> +
>>> + /* Extended stack frame type used. */
>>> + CORE_ADDR addr = unwound_sp + sp_r0_offset + 0x20;
>>> + for (int i = 0; i < 8; i++)
>>> + {
>>> + cache->saved_regs[ARM_D0_REGNUM + i].set_addr (addr);
>>> + addr += 8;
>>> + }
>>> + cache->saved_regs[ARM_FPSCR_REGNUM].set_addr (unwound_sp
>>> + + sp_r0_offset + 0x60);
>>> +
>>> + if (tdep->have_sec_ext && !default_callee_register_stacking
>>> + && fpccr_ts)
>>> + {
>>> + /* Handle floating-point callee saved registers. */
>>> + addr = unwound_sp + sp_r0_offset + 0x68;
>>> + for (int i = 8; i < 16; i++)
>>> + {
>>> + cache->saved_regs[ARM_D0_REGNUM + i].set_addr (addr);
>>> + addr += 8;
>>> + }
>>> - if (tdep->have_sec_ext && !default_callee_register_stacking && fpccr_ts)
>>> - {
>>> - /* Handle floating-point callee saved registers. */
>>> - fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x68;
>>> - for (i = 8; i < 16; i++)
>>> + arm_cache_set_active_sp_value (cache, tdep,
>>> + unwound_sp + sp_r0_offset + 0xA8);
>>> + }
>>> + else
>>> {
>>> - cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);
>>> - fpu_regs_stack_offset += 8;
>>> + /* Offset 0x64 is reserved. */
>>> + arm_cache_set_active_sp_value (cache, tdep,
>>> + unwound_sp + sp_r0_offset + 0x68);
>>> }
>>> -
>>> - arm_cache_set_active_sp_value (cache, tdep,
>>> - unwound_sp + sp_r0_offset + 0xA8);
>>> }
>>> else
>>> {
>>> - /* Offset 0x64 is reserved. */
>>> + /* Standard stack frame type used. */
>>> arm_cache_set_active_sp_value (cache, tdep,
>>> - unwound_sp + sp_r0_offset + 0x68);
>>> + unwound_sp + sp_r0_offset + 0x20);
>>> }
>>> - }
>>> - else
>>> - {
>>> - /* Standard stack frame type used. */
>>> - arm_cache_set_active_sp_value (cache, tdep,
>>> - unwound_sp + sp_r0_offset + 0x20);
>>> +
>>> + /* If bit 9 of the saved xPSR is set, then there is a four-byte
>>> + aligner between the top of the 32-byte stack frame and the
>>> + previous context's stack pointer. */
>>> + ULONGEST xpsr;
>>> + gdb_assert (safe_read_memory_unsigned_integer (cache->saved_regs[
>>> + ARM_PS_REGNUM].addr (), 4,
>>> + byte_order, &xpsr));
>>> + if (bit (xpsr,9) != 0)
>>> + {
>>> + CORE_ADDR new_sp = arm_cache_get_prev_sp_value (cache, tdep) + 4;
>>> + arm_cache_set_active_sp_value (cache, tdep, new_sp);
>>> + }
>>> +
>>> + return cache;
>>> }
>>> - /* If bit 9 of the saved xPSR is set, then there is a four-byte
>>> - aligner between the top of the 32-byte stack frame and the
>>> - previous context's stack pointer. */
>>> - if (safe_read_memory_integer (unwound_sp + sp_r0_offset + 0x1C, 4,
>>> - byte_order, &xpsr)
>>> - && (xpsr & (1 << 9)) != 0)
>>> - arm_cache_set_active_sp_value (cache, tdep,
>>> - arm_cache_get_prev_sp_value (cache, tdep) + 4);
>>> + error (_("While unwinding an exception frame, found unexpected Link Register "
>>> + "value 0x%lx. This should not happen and may be caused by corrupt "
>>> + "data or a bug in GDB."), lr);
>>
>> Same comment about using %s and phex as opposed to %lx.
>>
>> What does this case have that is different from the previous error? Does it contain an
>> unrecognized LR value? If so, we should mention that explicitly to make it as helpful to
>> the user as possible.
>
> The difference is that for the security extension check above, that is a wrongful state of the hardware. This case down here is when LR does not match an EXC_RETURN or FNC_RETURN pattern, thus is an inconsistency between the sniffer and the function that creates the cache object.
>
> The error case in the bottom should never happen as the sniffer has checked that LR matches either EXC_RETURN or FNC_RETURN, so it's more of a safe guard in case something is broken in GDB itself. The case with the security extension check should not happen neither, but I suppose it could happen if the data that is fetched from the target is corrupt in a certain way...
>
> Maybe the last one should be internal_error() instead of error()>
From your description, it sounds like it.
>>
>>
>>> + /* Terminate any further stack unwinding by referring to self. */
>>> + arm_cache_set_active_sp_value (cache, tdep, sp);
>>> return cache;
>>
>> This is dead code now. Nothing gets executed after error ().
>>
>>> }
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-10 10:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-09 15:30 [PATCH v2] gdb/arm: Cleanup of arm_m_exception_cache Torbjörn SVENSSON
2022-08-09 16:15 ` Luis Machado
2022-08-09 17:13 ` Torbjorn SVENSSON
2022-08-10 10:53 ` 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).