* [PATCH] gdb/arm: Cleanup of arm_m_exception_cache
@ 2022-08-08 7:56 Torbjörn SVENSSON
2022-08-09 13:02 ` Luis Machado
0 siblings, 1 reply; 4+ messages in thread
From: Torbjörn SVENSSON @ 2022-08-08 7:56 UTC (permalink / raw)
To: gdb-patches; +Cc: 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 | 343 ++++++++++++++++++++++++++-----------------------
1 file changed, 183 insertions(+), 160 deletions(-)
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 7d8d040f8f1..ad5ada39aea 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -3345,19 +3345,13 @@ 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;
+ ULONGEST xpsr;
+ bool 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);
@@ -3380,9 +3374,13 @@ arm_m_exception_cache (struct frame_info *this_frame)
return cache;
}
+ /* Check FNC_RETURN indicator bits (24-31). */
fnc_return = (((lr >> 24) & 0xff) == 0xfe);
- if (tdep->have_sec_ext && fnc_return)
+ if (fnc_return)
{
+ /* FNC_RETURN is only valid for targets with Security Extension. */
+ gdb_assert (tdep->have_sec_ext);
+
if (!arm_unwind_secure_frames)
{
warning (_("Non-secure to secure stack unwinding disabled."));
@@ -3428,6 +3426,14 @@ arm_m_exception_cache (struct frame_info *this_frame)
exc_return = (((lr >> 24) & 0xff) == 0xff);
if (exc_return)
{
+ int sp_regnum;
+ uint32_t sp_r0_offset = 0;
+ bool extended_frame_used;
+ 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);
@@ -3455,188 +3461,205 @@ 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);
+ sp_regnum = tdep->m_profile_msp_regnum;
+ }
- /* 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. */
+ /* Set the active SP regnum. */
+ arm_cache_switch_prev_sp (cache, tdep, sp_regnum);
- /* 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;
+ /* Fetch the SP to use for this frame. */
+ unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
- if (tdep->have_sec_ext && !default_callee_register_stacking && fpccr_ts)
+ /* 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 */
+
+ /* 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. */
+ extended_frame_used = ((lr & (1 << 4)) == 0);
+ if (extended_frame_used)
{
- /* Handle floating-point callee saved registers. */
- fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x68;
- for (i = 8; i < 16; i++)
- {
- cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);
+ 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;
- arm_cache_set_active_sp_value (cache, tdep,
- unwound_sp + sp_r0_offset + 0xA8);
+ 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++)
+ {
+ cache->saved_regs[ARM_D0_REGNUM + i]
+ .set_addr (fpu_regs_stack_offset);
+ fpu_regs_stack_offset += 8;
+ }
+
+ arm_cache_set_active_sp_value (cache, tdep,
+ unwound_sp + sp_r0_offset + 0xA8);
+ }
+ else
+ {
+ /* Offset 0x64 is reserved. */
+ arm_cache_set_active_sp_value (cache, tdep,
+ unwound_sp + sp_r0_offset + 0x68);
+ }
}
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. */
- 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);
+ /* 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. */
+ gdb_assert (safe_read_memory_unsigned_integer (cache->saved_regs[
+ ARM_PS_REGNUM].addr (), 4,
+ byte_order, &xpsr));
+ if ((xpsr & (1 << 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;
+ return cache;
+ }
+
+ gdb_assert_not_reached ("Invalid LR contet");
}
/* Implementation of function hook 'this_id' in
--
2.25.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gdb/arm: Cleanup of arm_m_exception_cache
2022-08-08 7:56 [PATCH] gdb/arm: Cleanup of arm_m_exception_cache Torbjörn SVENSSON
@ 2022-08-09 13:02 ` Luis Machado
2022-08-09 13:43 ` Torbjorn SVENSSON
0 siblings, 1 reply; 4+ messages in thread
From: Luis Machado @ 2022-08-09 13:02 UTC (permalink / raw)
To: Torbjörn SVENSSON, gdb-patches
Hi,
On 8/8/22 08:56, Torbjörn SVENSSON via Gdb-patches 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.
If we have corrupt data, do we risk running into this assertion?
>
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> ---
> gdb/arm-tdep.c | 343 ++++++++++++++++++++++++++-----------------------
> 1 file changed, 183 insertions(+), 160 deletions(-)
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 7d8d040f8f1..ad5ada39aea 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -3345,19 +3345,13 @@ 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;
> + ULONGEST xpsr;
> + bool 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);
> @@ -3380,9 +3374,13 @@ arm_m_exception_cache (struct frame_info *this_frame)
> return cache;
> }
>
> + /* Check FNC_RETURN indicator bits (24-31). */
> fnc_return = (((lr >> 24) & 0xff) == 0xfe);
> - if (tdep->have_sec_ext && fnc_return)
> + if (fnc_return)
> {
> + /* FNC_RETURN is only valid for targets with Security Extension. */
> + gdb_assert (tdep->have_sec_ext);
> +
An assertion is a bit of a strong hand here. It will crash GDB, essentially. Should we go for an
error instead? That will stop the unwinder dead in its tracks and return.
Unwinders may get corrupt data, so it is hard to rule out issues, even if GDB is doing the right thing.
> if (!arm_unwind_secure_frames)
> {
> warning (_("Non-secure to secure stack unwinding disabled."));
> @@ -3428,6 +3426,14 @@ arm_m_exception_cache (struct frame_info *this_frame)
> exc_return = (((lr >> 24) & 0xff) == 0xff);
> if (exc_return)
> {
> + int sp_regnum;
> + uint32_t sp_r0_offset = 0;
> + bool extended_frame_used;
extended_frame_used could be defined further down or removed.
Since we're touching this code, it would be nice to get the variable declarations closer to
where they're used. This makes things cleaner now that we can do it.
> + 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);
>
> @@ -3455,188 +3461,205 @@ 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);
> + sp_regnum = tdep->m_profile_msp_regnum;
> + }
>
> - /* 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. */
> + /* Set the active SP regnum. */
> + arm_cache_switch_prev_sp (cache, tdep, sp_regnum);
>
> - /* 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;
> + /* Fetch the SP to use for this frame. */
> + unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
>
> - if (tdep->have_sec_ext && !default_callee_register_stacking && fpccr_ts)
> + /* 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 */
> +
> + /* 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. */
> + extended_frame_used = ((lr & (1 << 4)) == 0);
Declaring extended_frame_used here would be nice. That's the only place where it is
used.
Alternatively, you could check for bit 4 in the conditional expression below and add a
comment explaining this is checking if the extended frame is being used.
> + if (extended_frame_used)
> {
> - /* Handle floating-point callee saved registers. */
> - fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x68;
> - for (i = 8; i < 16; i++)
> - {
> - cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);
> + 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;
>
> - arm_cache_set_active_sp_value (cache, tdep,
> - unwound_sp + sp_r0_offset + 0xA8);
> + 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++)
> + {
> + cache->saved_regs[ARM_D0_REGNUM + i]
> + .set_addr (fpu_regs_stack_offset);
> + fpu_regs_stack_offset += 8;
> + }
> +
> + arm_cache_set_active_sp_value (cache, tdep,
> + unwound_sp + sp_r0_offset + 0xA8);
> + }
> + else
> + {
> + /* Offset 0x64 is reserved. */
> + arm_cache_set_active_sp_value (cache, tdep,
> + unwound_sp + sp_r0_offset + 0x68);
> + }
> }
> 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. */
> - 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);
> + /* 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. */
> + gdb_assert (safe_read_memory_unsigned_integer (cache->saved_regs[
> + ARM_PS_REGNUM].addr (), 4,
> + byte_order, &xpsr));
> + if ((xpsr & (1 << 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;
> + return cache;
> + }
> +
> + gdb_assert_not_reached ("Invalid LR contet");
"content". Again, this will crash GDB. Should we error out instead?
> }
>
> /* Implementation of function hook 'this_id' in
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gdb/arm: Cleanup of arm_m_exception_cache
2022-08-09 13:02 ` Luis Machado
@ 2022-08-09 13:43 ` Torbjorn SVENSSON
2022-08-09 14:09 ` Luis Machado
0 siblings, 1 reply; 4+ messages in thread
From: Torbjorn SVENSSON @ 2022-08-09 13:43 UTC (permalink / raw)
To: Luis Machado, gdb-patches
Hello,
Thanks for the review. Please see my comments below.
On 8/9/22 15:02, Luis Machado wrote:
> Hi,
>
> On 8/8/22 08:56, Torbjörn SVENSSON via Gdb-patches 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.
>
>
> If we have corrupt data, do we risk running into this assertion?
See my comments below on individual cases.
>
>>
>> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
>> ---
>> gdb/arm-tdep.c | 343 ++++++++++++++++++++++++++-----------------------
>> 1 file changed, 183 insertions(+), 160 deletions(-)
>>
>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>> index 7d8d040f8f1..ad5ada39aea 100644
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -3345,19 +3345,13 @@ 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;
>> + ULONGEST xpsr;
>> + bool 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);
>> @@ -3380,9 +3374,13 @@ arm_m_exception_cache (struct frame_info
>> *this_frame)
>> return cache;
>> }
>> + /* Check FNC_RETURN indicator bits (24-31). */
>> fnc_return = (((lr >> 24) & 0xff) == 0xfe);
>> - if (tdep->have_sec_ext && fnc_return)
>> + if (fnc_return)
>> {
>> + /* FNC_RETURN is only valid for targets with Security
>> Extension. */
>> + gdb_assert (tdep->have_sec_ext);
>> +
>
> An assertion is a bit of a strong hand here. It will crash GDB,
> essentially. Should we go for an
> error instead? That will stop the unwinder dead in its tracks and return.
>
> Unwinders may get corrupt data, so it is hard to rule out issues, even
> if GDB is doing the right thing.
Okay, lets do an error message instead and abort the unwind.
What do you think the error message should say?
"FNC_RETURN pattern is only valid for targets with Security Extension."
- is that okay or do you have any other phase in mind?
>
>> if (!arm_unwind_secure_frames)
>> {
>> warning (_("Non-secure to secure stack unwinding disabled."));
>> @@ -3428,6 +3426,14 @@ arm_m_exception_cache (struct frame_info
>> *this_frame)
>> exc_return = (((lr >> 24) & 0xff) == 0xff);
>> if (exc_return)
>> {
>> + int sp_regnum;
>> + uint32_t sp_r0_offset = 0;
>> + bool extended_frame_used;
>
> extended_frame_used could be defined further down or removed.
>
> Since we're touching this code, it would be nice to get the variable
> declarations closer to
> where they're used. This makes things cleaner now that we can do it.
>
>> + 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);
>> @@ -3455,188 +3461,205 @@ 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);
>> + sp_regnum = tdep->m_profile_msp_regnum;
>> + }
>> - /* 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. */
>> + /* Set the active SP regnum. */
>> + arm_cache_switch_prev_sp (cache, tdep, sp_regnum);
>> - /* 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;
>> + /* Fetch the SP to use for this frame. */
>> + unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
>> - if (tdep->have_sec_ext && !default_callee_register_stacking
>> && fpccr_ts)
>> + /* 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 */
>> +
>> + /* 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. */
>> + extended_frame_used = ((lr & (1 << 4)) == 0);
>
> Declaring extended_frame_used here would be nice. That's the only
> place where it is
> used.
>
> Alternatively, you could check for bit 4 in the conditional expression
> below and add a
> comment explaining this is checking if the extended frame is being used.
I'll move the declaration down here as I think the variable is helpful
if every debugging this code.
>
>> + if (extended_frame_used)
>> {
>> - /* Handle floating-point callee saved registers. */
>> - fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x68;
>> - for (i = 8; i < 16; i++)
>> - {
>> - cache->saved_regs[ARM_D0_REGNUM + i].set_addr
>> (fpu_regs_stack_offset);
>> + 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;
>> - arm_cache_set_active_sp_value (cache, tdep,
>> - unwound_sp + sp_r0_offset + 0xA8);
>> + 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++)
>> + {
>> + cache->saved_regs[ARM_D0_REGNUM + i]
>> + .set_addr (fpu_regs_stack_offset);
>> + fpu_regs_stack_offset += 8;
>> + }
>> +
>> + arm_cache_set_active_sp_value (cache, tdep,
>> + unwound_sp + sp_r0_offset + 0xA8);
>> + }
>> + else
>> + {
>> + /* Offset 0x64 is reserved. */
>> + arm_cache_set_active_sp_value (cache, tdep,
>> + unwound_sp + sp_r0_offset + 0x68);
>> + }
>> }
>> 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. */
>> - 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);
>> + /* 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. */
>> + gdb_assert (safe_read_memory_unsigned_integer (cache->saved_regs[
>> + ARM_PS_REGNUM].addr (), 4,
>> + byte_order, &xpsr));
>> + if ((xpsr & (1 << 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;
>> + return cache;
>> + }
>> +
>> + gdb_assert_not_reached ("Invalid LR contet");
>
> "content". Again, this will crash GDB. Should we error out instead?
This place is different from the security extension check above.
When this line is reached, there is an internal error in the GDB source
code as the sniffer has decided that $lr contains an EXC_PATTERN or
FNC_PATTERN, but if you reach this line, it would mean that the sniffer
and this function are out of sync.
Do you think it's sane to assert or do you still want to have an error
message instead?
>
>> }
>> /* Implementation of function hook 'this_id' in
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gdb/arm: Cleanup of arm_m_exception_cache
2022-08-09 13:43 ` Torbjorn SVENSSON
@ 2022-08-09 14:09 ` Luis Machado
0 siblings, 0 replies; 4+ messages in thread
From: Luis Machado @ 2022-08-09 14:09 UTC (permalink / raw)
To: Torbjorn SVENSSON, gdb-patches
On 8/9/22 14:43, Torbjorn SVENSSON wrote:
> Hello,
>
> Thanks for the review. Please see my comments below.
>
> On 8/9/22 15:02, Luis Machado wrote:
>> Hi,
>>
>> On 8/8/22 08:56, Torbjörn SVENSSON via Gdb-patches 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.
>>
>>
>> If we have corrupt data, do we risk running into this assertion?
>
> See my comments below on individual cases.
>
>>
>>>
>>> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
>>> ---
>>> gdb/arm-tdep.c | 343 ++++++++++++++++++++++++++-----------------------
>>> 1 file changed, 183 insertions(+), 160 deletions(-)
>>>
>>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>>> index 7d8d040f8f1..ad5ada39aea 100644
>>> --- a/gdb/arm-tdep.c
>>> +++ b/gdb/arm-tdep.c
>>> @@ -3345,19 +3345,13 @@ 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;
>>> + ULONGEST xpsr;
>>> + bool 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);
>>> @@ -3380,9 +3374,13 @@ arm_m_exception_cache (struct frame_info *this_frame)
>>> return cache;
>>> }
>>> + /* Check FNC_RETURN indicator bits (24-31). */
>>> fnc_return = (((lr >> 24) & 0xff) == 0xfe);
>>> - if (tdep->have_sec_ext && fnc_return)
>>> + if (fnc_return)
>>> {
>>> + /* FNC_RETURN is only valid for targets with Security Extension. */
>>> + gdb_assert (tdep->have_sec_ext);
>>> +
>>
>> An assertion is a bit of a strong hand here. It will crash GDB, essentially. Should we go for an
>> error instead? That will stop the unwinder dead in its tracks and return.
>>
>> Unwinders may get corrupt data, so it is hard to rule out issues, even if GDB is doing the right thing.
>
> Okay, lets do an error message instead and abort the unwind.
>
> What do you think the error message should say?
>
> "FNC_RETURN pattern is only valid for targets with Security Extension." - is that okay or do you have any other phase in mind?
>
For the sake of the user, I think something less low level.
"While unwinding an exception frame, found unexpected Link Register value <hex value of LR>"?
Given this is a showstopper, might as well add more to it.
"This should not happen and may be caused by corrupt data or a bug in GDB".
What do you think?
>>
>>> if (!arm_unwind_secure_frames)
>>> {
>>> warning (_("Non-secure to secure stack unwinding disabled."));
>>> @@ -3428,6 +3426,14 @@ arm_m_exception_cache (struct frame_info *this_frame)
>>> exc_return = (((lr >> 24) & 0xff) == 0xff);
>>> if (exc_return)
>>> {
>>> + int sp_regnum;
>>> + uint32_t sp_r0_offset = 0;
>>> + bool extended_frame_used;
>>
>> extended_frame_used could be defined further down or removed.
>>
>> Since we're touching this code, it would be nice to get the variable declarations closer to
>> where they're used. This makes things cleaner now that we can do it.
>>
>>> + 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);
>>> @@ -3455,188 +3461,205 @@ 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);
>>> + sp_regnum = tdep->m_profile_msp_regnum;
>>> + }
>>> - /* 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. */
>>> + /* Set the active SP regnum. */
>>> + arm_cache_switch_prev_sp (cache, tdep, sp_regnum);
>>> - /* 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;
>>> + /* Fetch the SP to use for this frame. */
>>> + unwound_sp = arm_cache_get_prev_sp_value (cache, tdep);
>>> - if (tdep->have_sec_ext && !default_callee_register_stacking && fpccr_ts)
>>> + /* 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 */
>>> +
>>> + /* 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. */
>>> + extended_frame_used = ((lr & (1 << 4)) == 0);
>>
>> Declaring extended_frame_used here would be nice. That's the only place where it is
>> used.
>>
>> Alternatively, you could check for bit 4 in the conditional expression below and add a
>> comment explaining this is checking if the extended frame is being used.
> I'll move the declaration down here as I think the variable is helpful if every debugging this code.
>>
>>> + if (extended_frame_used)
>>> {
>>> - /* Handle floating-point callee saved registers. */
>>> - fpu_regs_stack_offset = unwound_sp + sp_r0_offset + 0x68;
>>> - for (i = 8; i < 16; i++)
>>> - {
>>> - cache->saved_regs[ARM_D0_REGNUM + i].set_addr (fpu_regs_stack_offset);
>>> + 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;
>>> - arm_cache_set_active_sp_value (cache, tdep,
>>> - unwound_sp + sp_r0_offset + 0xA8);
>>> + 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++)
>>> + {
>>> + cache->saved_regs[ARM_D0_REGNUM + i]
>>> + .set_addr (fpu_regs_stack_offset);
>>> + fpu_regs_stack_offset += 8;
>>> + }
>>> +
>>> + arm_cache_set_active_sp_value (cache, tdep,
>>> + unwound_sp + sp_r0_offset + 0xA8);
>>> + }
>>> + else
>>> + {
>>> + /* Offset 0x64 is reserved. */
>>> + arm_cache_set_active_sp_value (cache, tdep,
>>> + unwound_sp + sp_r0_offset + 0x68);
>>> + }
>>> }
>>> 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. */
>>> - 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);
>>> + /* 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. */
>>> + gdb_assert (safe_read_memory_unsigned_integer (cache->saved_regs[
>>> + ARM_PS_REGNUM].addr (), 4,
>>> + byte_order, &xpsr));
>>> + if ((xpsr & (1 << 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;
>>> + return cache;
>>> + }
>>> +
>>> + gdb_assert_not_reached ("Invalid LR contet");
>>
>> "content". Again, this will crash GDB. Should we error out instead?
>
> This place is different from the security extension check above.
>
> When this line is reached, there is an internal error in the GDB source code as the sniffer has decided that $lr contains an EXC_PATTERN or FNC_PATTERN, but if you reach this line, it would mean that the sniffer and this function are out of sync.
>
> Do you think it's sane to assert or do you still want to have an error message instead?
>
Given unwinders deal with potentially corrupt data, an error message here would allow a user to still interact with GDB in a limited way. They would be
able to see register values, memory etc.
If we assert, that will kill GDB and prevent any form of interaction. Unless we are sure this is a situation that only happens if GDB is faulty, I'd say
we should error out with a descriptive message.
>>
>>> }
>>> /* Implementation of function hook 'this_id' in
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-09 14:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-08 7:56 [PATCH] gdb/arm: Cleanup of arm_m_exception_cache Torbjörn SVENSSON
2022-08-09 13:02 ` Luis Machado
2022-08-09 13:43 ` Torbjorn SVENSSON
2022-08-09 14:09 ` 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).