coredump: ARM: Ensure sp in dump is set as gdb expects #73189

mrkhldn · 2024-05-23T01:23:46Z

Gdb is typically able to reconstruct the first two frames of the failing stack using the "pc" and "lr" registers. After that, (if the frame pointer is omitted) it appears to need the stack pointer (sp register) to point to the top of the stack before a fatal error occurred.

The ARM Cortex-M7 processor pushes registers r0-r3, r12, LR, {possibly FPU registers}, PC, SPSR onto the stack before entering the exception handler. We adjust the stack pointer back to the point before these registers were pushed for preservation in the dump.

During k_oops/k_panic, the sp wasn't stored in the core dump at all. Apply similar logic to store it when failures occur in that path.

kartben · 2024-06-12T02:11:50Z

@mrkhldn can you rebase to solve conflict?

arch/arm/core/cortex_m/fault.c

arch/arm/core/fatal.c

mrkhldn · 2024-06-17T21:35:27Z

For reference, using the coredump_gdbserver.py script and connection from gdb to get the backtrace, before this change when using the output of the test at tests/subsys/debug/coredump_backends/debug.coredump.backends.logging:

(Wrong stack)

(gdb) bt
#0  dump_entry (p1=<optimized out>, p2=0x0 <cbvprintf_package>, p3=0x0 <cbvprintf_package>) at ~/zephyrproject/zephyr/tests/subsys/debug/coredump_backends/src/main.c:34
#1  0x00000e84 in z_arm_usage_fault ()
#2  0x00000e70 in z_arm_fault_init () at ~/zephyrproject/zephyr/arch/arm/core/cortex_m/fault.c:1196
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

And after this change:
(Expected stack)

(gdb) bt
#0  dump_entry (p1=<optimized out>, p2=0x0 <cbvprintf_package>, p3=0x0 <cbvprintf_package>) at ~/zephyrproject/zephyr/tests/subsys/debug/coredump_backends/src/main.c:34
#1  0x000008b0 in z_thread_entry (entry=0x3ad <dump_entry>, p1=0x0 <cbvprintf_package>, p2=0x0 <cbvprintf_package>, p3=0x0 <cbvprintf_package>)
    at ~/zephyrproject/zephyr/lib/os/thread_entry.c:48
#2  0x00000000 in ?? ()

arbrauns · 2024-08-07T08:24:04Z

arch/arm/core/cortex_m/fault.c

 #ifdef CONFIG_DEBUG_COREDUMP
 	z_arm_coredump_fault_sp = POINTER_TO_UINT(esf);
-#endif
+#if defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) || defined(CONFIG_ARMV6_M_ARMV8_M_BASELINE)
+	/* Gdb expects a stack pointer that does not include the exception stack frame in order to
+	 * unwind. So adjust the stack pointer accordingly.
+	 */
+	z_arm_coredump_fault_sp += sizeof(esf->basic);
+
+#if defined(CONFIG_FPU) && defined(CONFIG_FPU_SHARING)
+	/* Assess whether thread had been using the FP registers and add size of additional
+	 * registers if necessary
+	 */
+	if ((exc_return & EXC_RETURN_STACK_FRAME_TYPE_STANDARD) ==
+			EXC_RETURN_STACK_FRAME_TYPE_EXTENDED) {
+		z_arm_coredump_fault_sp += sizeof(esf->fpu);
+	}
+#endif /* CONFIG_FPU && CONFIG_FPU_SHARING */
+#endif /* CONFIG_ARMV7_M_ARMV8_M_MAINLINE || CONFIG_ARMV6_M_ARMV8_M_BASELINE */
+#endif /* CONFIG_DEBUG_COREDUMP */


style: how about extracting all this into a static inline in exception.h so the whole ifdef mess doesn't have to be implemented in two places?

arbrauns · 2024-08-07T08:25:44Z

arch/arm/core/fatal.c

 */
-void z_do_kernel_oops(const struct arch_esf *esf, _callee_saved_t *callee_regs)
+void z_do_kernel_oops(const struct arch_esf *esf, _callee_saved_t *callee_regs, uint32_t exc_return)


In #76221 I made sure Cortex-A/Cortex-R don't pass garbage here, but I don't know if that's really necessary.

Makes sense to me

arbrauns · 2024-08-07T08:27:17Z

arch/arm/include/cortex_m/exception.h

+/* Exception Return (EXC_RETURN) is provided in LR upon exception entry.
+ * It is used to perform an exception return and to detect possible state
+ * transition upon exception.
+ */
+
+/* Prefix. Indicates that this is an EXC_RETURN value.
+ * This field reads as 0b11111111.
+ */
+#define EXC_RETURN_INDICATOR_PREFIX     (0xFF << 24)
+/* bit[0]: Exception Secure. The security domain the exception was taken to. */
+#define EXC_RETURN_EXCEPTION_SECURE_Pos 0
+#define EXC_RETURN_EXCEPTION_SECURE_Msk \
+		BIT(EXC_RETURN_EXCEPTION_SECURE_Pos)
+#define EXC_RETURN_EXCEPTION_SECURE_Non_Secure 0
+#define EXC_RETURN_EXCEPTION_SECURE_Secure EXC_RETURN_EXCEPTION_SECURE_Msk
+/* bit[2]: Stack Pointer selection. */
+#define EXC_RETURN_SPSEL_Pos 2
+#define EXC_RETURN_SPSEL_Msk BIT(EXC_RETURN_SPSEL_Pos)
+#define EXC_RETURN_SPSEL_MAIN 0
+#define EXC_RETURN_SPSEL_PROCESS EXC_RETURN_SPSEL_Msk
+/* bit[3]: Mode. Indicates the Mode that was stacked from. */
+#define EXC_RETURN_MODE_Pos 3
+#define EXC_RETURN_MODE_Msk BIT(EXC_RETURN_MODE_Pos)
+#define EXC_RETURN_MODE_HANDLER 0
+#define EXC_RETURN_MODE_THREAD EXC_RETURN_MODE_Msk
+/* bit[4]: Stack frame type. Indicates whether the stack frame is a standard
+ * integer only stack frame or an extended floating-point stack frame.
+ */
+#define EXC_RETURN_STACK_FRAME_TYPE_Pos 4
+#define EXC_RETURN_STACK_FRAME_TYPE_Msk BIT(EXC_RETURN_STACK_FRAME_TYPE_Pos)
+#define EXC_RETURN_STACK_FRAME_TYPE_EXTENDED 0
+#define EXC_RETURN_STACK_FRAME_TYPE_STANDARD EXC_RETURN_STACK_FRAME_TYPE_Msk
+/* bit[5]: Default callee register stacking. Indicates whether the default
+ * stacking rules apply, or whether the callee registers are already on the
+ * stack.
+ */
+#define EXC_RETURN_CALLEE_STACK_Pos 5
+#define EXC_RETURN_CALLEE_STACK_Msk BIT(EXC_RETURN_CALLEE_STACK_Pos)
+#define EXC_RETURN_CALLEE_STACK_SKIPPED 0
+#define EXC_RETURN_CALLEE_STACK_DEFAULT EXC_RETURN_CALLEE_STACK_Msk
+/* bit[6]: Secure or Non-secure stack. Indicates whether a Secure or
+ * Non-secure stack is used to restore stack frame on exception return.
+ */
+#define EXC_RETURN_RETURN_STACK_Pos 6
+#define EXC_RETURN_RETURN_STACK_Msk BIT(EXC_RETURN_RETURN_STACK_Pos)
+#define EXC_RETURN_RETURN_STACK_Non_Secure 0
+#define EXC_RETURN_RETURN_STACK_Secure EXC_RETURN_RETURN_STACK_Msk


Since this is only relevant for Cortex-M, maybe it should be put behind an #if defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) || defined(CONFIG_ARMV6_M_ARMV8_M_BASELINE) to avoid accidental misuse in Cortex-A/Cortex-R code.

The path to this include file is arch/arm/include/cortex_m/exception.h, so hopefully that's enough to make it clear that it's intended for Cortex-M

Ah, I missed that, that's fine then.

I think you also need to check xPSR bit 9 to see if the stack is padded an additional 4 bytes.

I think you also need to check xPSR bit 9 to see if the stack is padded an additional 4 bytes.

Added, thanks

Gdb is typically able to reconstruct the first two frames of the failing stack using the "pc" and "lr" registers. After that, (if the frame pointer is omitted) it appears to need the stack pointer (sp register) to point to the top of the stack before a fatal error occurred. The ARM Cortex-M processors push registers r0-r3, r12, LR, {possibly FPU registers}, PC, SPSR onto the stack before entering the exception handler. We adjust the stack pointer back to the point before these registers were pushed for preservation in the dump. During k_oops/k_panic, the sp wasn't stored in the core dump at all. Apply similar logic to store it when failures occur in that path. Signed-off-by: Mark Holden <mholden@meta.com>

ithinuel · 2024-09-24T16:35:32Z

@vbrzeski would you mind checking if this resolved your issue and if so approve this PR please ?

vbrzeski · 2024-10-01T18:29:40Z

Thanks for adding the xPSR check! LGTM after clang-formatting the source code.

dawidwojtas-mudita · 2024-10-08T12:32:05Z

@mrkhldn I checked your changes in my test code and it works for nRF52840.
Before:

Reading symbols from ../build/zephyr/zephyr.elf...
someFunc () at /home/dawid/Documents/test/src/main.cpp:55
55	    __asm__ volatile("udf #0" : : :);
#0  someFunc () at /home/dawid/Documents/test/src/main.cpp:55
Backtrace stopped: Cannot access memory at address 0x200022c0

After:

Reading symbols from ../build/zephyr/zephyr.elf...
someFunc () at /home/dawid/Documents/test/src/main.cpp:55
55	    __asm__ volatile("udf #0" : : :);
#0  someFunc () at /home/dawid/Documents/test/src/main.cpp:55
#1  0x0001dcac in main () at /home/dawid/Documents/test/src/main.cpp:69

Great job!

aescolar · 2024-10-09T09:21:27Z

@mrkhldn this commit introduced a regression in main
https://github.com/zephyrproject-rtos/zephyr/actions/runs/11250552333/job/31279885750#step:11:1033

aescolar · 2024-10-09T12:01:21Z

@mrkhldn unfortunately this PR was reverted due to the introduced regression :( .
You are more than welcome to resubmit the PR with a fix so it also builds for M33 cores.

mrkhldn · 2024-10-09T17:06:47Z

Fixed the regression and re-submitted here #79622

zephyrbot added the area: ARM ARM (32-bit) Architecture label May 23, 2024

zephyrbot requested review from bbolen, carlocaione, galak, ithinuel, MaureenHelm, microbuilder and stephanosio May 23, 2024 01:24

zephyrbot assigned ithinuel May 23, 2024

mrkhldn mentioned this pull request Jun 11, 2024

Core dump issue while enabling CONFIG_ASSERT=y #74053

Closed

mrkhldn force-pushed the coredump_arm_sp branch from a1c4e3b to 5346635 Compare June 12, 2024 15:50

ithinuel reviewed Jun 17, 2024

View reviewed changes

arch/arm/core/cortex_m/fault.c Outdated Show resolved Hide resolved

arch/arm/core/fatal.c Outdated Show resolved Hide resolved

mrkhldn force-pushed the coredump_arm_sp branch from 5346635 to a470b11 Compare June 17, 2024 21:29

ithinuel previously approved these changes Jun 17, 2024

View reviewed changes

mrkhldn mentioned this pull request Aug 6, 2024

Fix stack pointer in core dumps #76221

Closed

arbrauns reviewed Aug 7, 2024

View reviewed changes

mrkhldn dismissed ithinuel’s stale review via 9e58123 August 7, 2024 22:16

mrkhldn force-pushed the coredump_arm_sp branch from a470b11 to 9e58123 Compare August 7, 2024 22:16

ithinuel previously approved these changes Aug 12, 2024

View reviewed changes

mrkhldn dismissed ithinuel’s stale review via f4c019e September 20, 2024 23:17

mrkhldn force-pushed the coredump_arm_sp branch from 9e58123 to f4c019e Compare September 20, 2024 23:17

vbrzeski mentioned this pull request Sep 20, 2024

Exception Stack Frames are not correct in coredumps #78788

Closed

mrkhldn linked an issue Sep 21, 2024 that may be closed by this pull request

Exception Stack Frames are not correct in coredumps #78788

Closed

ithinuel approved these changes Sep 24, 2024

View reviewed changes

ycsin approved these changes Oct 9, 2024

View reviewed changes

aescolar merged commit ec7943b into zephyrproject-rtos:main Oct 9, 2024
22 checks passed

aescolar mentioned this pull request Oct 9, 2024

tests/subsys/debug/coredump* fail to build for for ARM m33 processors #79594

Closed

kasjer mentioned this pull request Oct 9, 2024

drivers: usb: udc: add Smartbond UDC driver #79184

Merged

mrkhldn mentioned this pull request Oct 9, 2024

coredump: ARM: Ensure sp in dump is set as gdb expects #79622

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coredump: ARM: Ensure sp in dump is set as gdb expects #73189

coredump: ARM: Ensure sp in dump is set as gdb expects #73189

mrkhldn commented May 23, 2024

kartben commented Jun 12, 2024

mrkhldn commented Jun 17, 2024

arbrauns Aug 7, 2024

arbrauns Aug 7, 2024

mrkhldn Aug 7, 2024

arbrauns Aug 7, 2024

mrkhldn Aug 7, 2024

arbrauns Aug 8, 2024

vbrzeski Sep 16, 2024

mrkhldn Sep 20, 2024

ithinuel commented Sep 24, 2024

vbrzeski commented Oct 1, 2024

dawidwojtas-mudita commented Oct 8, 2024 •

edited

Loading

aescolar commented Oct 9, 2024

aescolar commented Oct 9, 2024

mrkhldn commented Oct 9, 2024

coredump: ARM: Ensure sp in dump is set as gdb expects #73189

coredump: ARM: Ensure sp in dump is set as gdb expects #73189

Conversation

mrkhldn commented May 23, 2024

kartben commented Jun 12, 2024

mrkhldn commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ithinuel commented Sep 24, 2024

vbrzeski commented Oct 1, 2024

dawidwojtas-mudita commented Oct 8, 2024 • edited Loading

aescolar commented Oct 9, 2024

aescolar commented Oct 9, 2024

mrkhldn commented Oct 9, 2024

dawidwojtas-mudita commented Oct 8, 2024 •

edited

Loading