From 771ac20b11fdd625f092b8f0999f00be32091deb Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Fri, 13 Mar 2026 16:51:15 -0500 Subject: [PATCH] Fix PICO_PANIC_FUNCTION= imelementations - stack should remain properly aligned - risc-v was actually completely broken for user defined handlers because GCC was auto-pushing vaargs for naked function even with no stack frame - new implementation supports full call stacks in gdb for Arm or RISC-V when hitting breakpoint in panic function - new config defines make it explicit that gdb full stgack trace comes at the cost of corrupting stack based vaargs 4th is corrupted on Arm, later on RiSC-V (7 maybe?) --- .../pico_platform_panic/BUILD.bazel | 2 +- .../pico_platform_panic/CMakeLists.txt | 1 + .../custom_panic_function.S | 75 ++++++++++++++++++ .../include/pico/platform/panic.h | 14 +++- src/rp2_common/pico_platform_panic/panic.c | 37 +-------- test/CMakeLists.txt | 1 + test/panic_function_test/CMakeLists.txt | 18 +++++ .../panic_function_test/panic_function_test.c | 78 +++++++++++++++++++ 8 files changed, 189 insertions(+), 37 deletions(-) create mode 100644 src/rp2_common/pico_platform_panic/custom_panic_function.S create mode 100644 test/panic_function_test/CMakeLists.txt create mode 100644 test/panic_function_test/panic_function_test.c diff --git a/src/rp2_common/pico_platform_panic/BUILD.bazel b/src/rp2_common/pico_platform_panic/BUILD.bazel index 1a29b9c1..5e433f52 100644 --- a/src/rp2_common/pico_platform_panic/BUILD.bazel +++ b/src/rp2_common/pico_platform_panic/BUILD.bazel @@ -20,7 +20,7 @@ cc_library( cc_library( name = "pico_platform_panic", - srcs = ["panic.c"], + srcs = ["panic.c", "custom_panic_function.S"], hdrs = ["include/pico/platform/panic.h"], includes = ["include"], target_compatible_with = compatible_with_rp2(), diff --git a/src/rp2_common/pico_platform_panic/CMakeLists.txt b/src/rp2_common/pico_platform_panic/CMakeLists.txt index 0ebdb17c..82195706 100644 --- a/src/rp2_common/pico_platform_panic/CMakeLists.txt +++ b/src/rp2_common/pico_platform_panic/CMakeLists.txt @@ -3,6 +3,7 @@ if (NOT TARGET pico_platform_panic) target_sources(pico_platform_panic INTERFACE ${CMAKE_CURRENT_LIST_DIR}/panic.c + ${CMAKE_CURRENT_LIST_DIR}/custom_panic_function.S ) target_include_directories(pico_platform_panic_headers SYSTEM INTERFACE ${CMAKE_CURRENT_LIST_DIR}/include) diff --git a/src/rp2_common/pico_platform_panic/custom_panic_function.S b/src/rp2_common/pico_platform_panic/custom_panic_function.S new file mode 100644 index 00000000..a6d0f8db --- /dev/null +++ b/src/rp2_common/pico_platform_panic/custom_panic_function.S @@ -0,0 +1,75 @@ +/* + * Copyright (c) 2026 Raspberry Pi (Trading) Ltd. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +#include "pico/asm_helper.S" + +#ifdef PICO_PANIC_FUNCTION +#define __CONCAT1(x,y) x ## y +#define __CONCAT(x,y) __CONCAT1(x,y) + +#define PICO_PANIC_FUNCTION_IS_EMPTY (__CONCAT(PICO_PANIC_FUNCTION, 1)) +.text +// Use a forwarding method here as it is a little simpler than renaming the symbol as it is used from assembler +.weak panic // also allow override +.type panic,%function +panic: +#ifdef __riscv + // we seem to need to help gdb out with call frame on RISC-V + .cfi_sections .debug_frame + .cfi_startproc + #if PICO_PANIC_FUNCTION_IS_EMPTY + 1: ebreak + j 1b + #elif PICO_PANIC_FUNCTION_DOES_NOT_RETURN + // if you get an undefined reference here, you didn't define your PICO_PANIC_FUNCTION! + jal zero, PICO_PANIC_FUNCTION + #else + #if PICO_PANIC_FUNCTION_WITH_CALL_STACK + #if PICO_PANIC_FUNCTION_WITH_ALL_VAARGS + #error PICO_PANIC_FUNCTION_WITH_CALL_STACK & PICO_PANIC_FUNCTION_WITH_ALL_VAARGS are incompatible when PICO_PANIC_FUNCTION_DOES_NOT_RETURN is 0 + #endif + // .insn 0xb842 // cm.push {ra}, -16: Save ultimate return address + // use explicit for better compatibility with gdb stack unwinding + addi sp,sp,-16 + .cfi_def_cfa_offset 16 + sw ra,12(sp) + .cfi_offset ra, -4 + #endif + // if you get an undefined reference here, you didn't define your PICO_PANIC_FUNCTION! + jal PICO_PANIC_FUNCTION + #if !PICO_PANIC_FUNCTION_DOES_NOT_RETURN + 1: ebreak + j 1b + #endif + #endif + .cfi_endproc +#else + #if PICO_PANIC_FUNCTION_IS_EMPTY + 1: bkpt #0 + b 1b + #elif PICO_PANIC_FUNCTION_DOES_NOT_RETURN + // if you get an undefined reference here, you didn't define your PICO_PANIC_FUNCTION! + ldr r4, =PICO_PANIC_FUNCTION + bx r4 + #else + #if PICO_PANIC_FUNCTION_WITH_CALL_STACK + #if PICO_PANIC_FUNCTION_WITH_ALL_VAARGS + #error PICO_PANIC_FUNCTION_WITH_CALL_STACK & PICO_PANIC_FUNCTION_WITH_ALL_VAARGS are incompatible when PICO_PANIC_FUNCTION_DOES_NOT_RETURN is 0 + #endif + push {r4, lr} // must keep 8 byte alignment for call to user function + #endif + // if you get an undefined reference here, you didn't define your PICO_PANIC_FUNCTION! + bl PICO_PANIC_FUNCTION + #if !PICO_PANIC_FUNCTION_DOES_NOT_RETURN + 1: bkpt #0 + b 1b + #endif + #endif +#endif + +#endif // PICO_PANIC_FUNCTION + + diff --git a/src/rp2_common/pico_platform_panic/include/pico/platform/panic.h b/src/rp2_common/pico_platform_panic/include/pico/platform/panic.h index a358e170..749be3bb 100644 --- a/src/rp2_common/pico_platform_panic/include/pico/platform/panic.h +++ b/src/rp2_common/pico_platform_panic/include/pico/platform/panic.h @@ -11,8 +11,20 @@ extern "C" { #endif -#ifndef __ASSEMBLER__ +// PICO_CONFIG: PICO_PANIC_FUNCTION, Name of a function to use in place of the stock panic function or empty string to simply breakpoint on panic, group=pico_runtime +// PICO_CONFIG: PICO_PANIC_FUNCTION_WITH_CALL_STACK, Ensure the call stack is available when using a custom panic function via PICO_PANIC_FUNCTION=. When set to 1 it conflicts with PICO_PANIC_FUNCTION_WITH_ALL_VAARGS=1 as a stack fram is pushed onto the stack corrupting later vaargs. Note this defaults to 1 as most custom panic functions don't actually use the arguments and the call stack seems more useful in general, default=1group=pico_runtime +#ifndef PICO_PANIC_FUNCTION_WITH_CALL_STACK +#define PICO_PANIC_FUNCTION_WITH_CALL_STACK 1 +#endif + +// PICO_CONFIG: PICO_PANIC_FUNCTION_WITH_ALL_VAARGS, Ensures that all vaargs are faithfully passed to the custom panic function via PICO_PANIC_FUNCTION=. When set to 1 it conflicts with PICO_PANIC_FUNCTION_WITH_CALL_STACK=1 because both the later vaargs and the stack frame would be expected at the top of the stack. Note this defaults to 0 as most custom panic functions don't actually use the arguments and the call stack seems more useful in general, default=1group=pico_runtime +#ifndef PICO_PANIC_FUNCTION_WITH_ALL_VAARGS +#define PICO_PANIC_FUNCTION_WITH_ALL_VAARGS 0 +#endif + + +#ifndef __ASSEMBLER__ /*! \brief Panics with the message "Unsupported" * \ingroup pico_platform * \see panic diff --git a/src/rp2_common/pico_platform_panic/panic.c b/src/rp2_common/pico_platform_panic/panic.c index 06c063d2..8c46c5fa 100644 --- a/src/rp2_common/pico_platform_panic/panic.c +++ b/src/rp2_common/pico_platform_panic/panic.c @@ -21,41 +21,8 @@ void __attribute__((noreturn)) panic_unsupported(void) { panic("not supported"); } -// PICO_CONFIG: PICO_PANIC_FUNCTION, Name of a function to use in place of the stock panic function or empty string to simply breakpoint on panic, group=pico_runtime // note the default is not "panic" it is undefined -#ifdef PICO_PANIC_FUNCTION -#define PICO_PANIC_FUNCTION_EMPTY (__CONCAT(PICO_PANIC_FUNCTION, 1) == 1) -#if !PICO_PANIC_FUNCTION_EMPTY -extern void __attribute__((noreturn)) __printflike(1, 0) PICO_PANIC_FUNCTION(__unused const char *fmt, ...); -#endif -// Use a forwarding method here as it is a little simpler than renaming the symbol as it is used from assembler -void __attribute__((naked, noreturn)) __printflike(1, 0) panic(__unused const char *fmt, ...) { - // if you get an undefined reference here, you didn't define your PICO_PANIC_FUNCTION! - pico_default_asm ( -#ifdef __riscv - -#if !PICO_PANIC_FUNCTION_EMPTY - "jal " __XSTRING(PICO_PANIC_FUNCTION) "\n" -#endif - "1: ebreak\n" - "j 1b\n" - -#else - - "push {lr}\n" -#if !PICO_PANIC_FUNCTION_EMPTY - "bl " __XSTRING(PICO_PANIC_FUNCTION) "\n" -#endif - "1: bkpt #0\n" - "b 1b\n" // loop for ever as we are no return - -#endif - : - : - : - ); -} -#else +#ifndef PICO_PANIC_FUNCTION // todo consider making this try harder to output if we panic early // right now, print mutex may be uninitialised (in which case it deadlocks - although after printing "PANIC") // more importantly there may be no stdout/UART initialized yet @@ -75,7 +42,7 @@ void __attribute__((noreturn)) __printflike(1, 0) panic(const char *fmt, ...) { weak_raw_vprintf(fmt, args); #endif va_end(args); - puts("\n"); + puts(""); #endif } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 81647ee9..c772bda6 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -4,6 +4,7 @@ add_subdirectory(pico_stdlib_test) add_subdirectory(pico_stdio_test) add_subdirectory(pico_time_test) add_subdirectory(pico_divider_test) +add_subdirectory(panic_function_test) if (PICO_ON_DEVICE) add_subdirectory(pico_float_test) add_subdirectory(kitchen_sink) diff --git a/test/panic_function_test/CMakeLists.txt b/test/panic_function_test/CMakeLists.txt new file mode 100644 index 00000000..72013f16 --- /dev/null +++ b/test/panic_function_test/CMakeLists.txt @@ -0,0 +1,18 @@ +add_executable(panic_function_test_default ${CMAKE_CURRENT_LIST_DIR}/panic_function_test.c) +target_link_libraries(panic_function_test_default PRIVATE pico_stdlib) +pico_add_extra_outputs(panic_function_test_default) + +add_executable(panic_function_test_bkpt ${CMAKE_CURRENT_LIST_DIR}/panic_function_test.c) +target_compile_definitions(panic_function_test_bkpt PRIVATE PICO_PANIC_FUNCTION=) # should just bkpt +target_link_libraries(panic_function_test_bkpt PRIVATE pico_stdlib) +pico_add_extra_outputs(panic_function_test_bkpt) + +add_executable(panic_function_test_user ${CMAKE_CURRENT_LIST_DIR}/panic_function_test.c) +target_compile_definitions(panic_function_test_user PRIVATE PICO_PANIC_FUNCTION=handle_panic) +target_link_libraries(panic_function_test_user PRIVATE pico_stdlib) +pico_add_extra_outputs(panic_function_test_user) + +add_executable(panic_function_test_user_all_vaargs ${CMAKE_CURRENT_LIST_DIR}/panic_function_test.c) +target_compile_definitions(panic_function_test_user_all_vaargs PRIVATE PICO_PANIC_FUNCTION=handle_panic PICO_PANIC_FUNCTION_WITH_ALL_VAARGS=1 PICO_PANIC_FUNCTION_WITH_CALL_STACK=0) +target_link_libraries(panic_function_test_user_all_vaargs PRIVATE pico_stdlib) +pico_add_extra_outputs(panic_function_test_user_all_vaargs) diff --git a/test/panic_function_test/panic_function_test.c b/test/panic_function_test/panic_function_test.c new file mode 100644 index 00000000..cffdff2f --- /dev/null +++ b/test/panic_function_test/panic_function_test.c @@ -0,0 +1,78 @@ +#include +#include + +#include "pico/stdlib.h" + +#define MAGIC1 ((const char *)0x87654321) +#define MAGIC2 0x97654321 +#define MAGIC3 3.14159265358979323846 +#define MAGIC4 123 +#define MAGIC5 456 +#define MAGIC6 789 +#define MAGIC7 0xabc + +#define PICO_PANIC_FUNCTION_IS_EMPTY (__CONCAT(PICO_PANIC_FUNCTION, 1)) + +void __printflike(1, 0) handle_panic(const char *magic1, ...) +{ + printf("checking first arg...\n"); + if (magic1 != MAGIC1) { + printf("magic1 (%08x) != 0x%08x\n", magic1, MAGIC1); + return; + } + va_list args; + va_start(args, magic1); + printf("checking early vaargs...\n"); + int magic2 = va_arg(args, int); + if (magic2 != MAGIC2) { + printf("magic2 (%08x) != 0x%08x\n", magic2, MAGIC2); + return; + } + double magic3 = va_arg(args, double); + if (magic3 != MAGIC3) { + printf("magic3 (%f) != 0x%f\n", magic3, MAGIC3); + return; + } +#if PICO_PANIC_FUNCTION_WITH_ALL_VAARGS + printf("checking remaining vaargs...\n"); + int magic4 = va_arg(args, int); + if (magic4 != MAGIC4) { + printf("magic4 (%08x) != 0x%08x\n", magic4, MAGIC4); + return; + } + int magic5 = va_arg(args, int); + if (magic5 != MAGIC5) { + printf("magic5 (%08x) != 0x%08x\n", magic5, MAGIC5); + return; + } + int magic6 = va_arg(args, int); + if (magic6 != MAGIC6) { + printf("magic6 (%08x) != 0x%08x\n", magic6, MAGIC6); + return; + } + int magic7 = va_arg(args, int); + if (magic7 != MAGIC7) { + printf("magic7 (%08x) != 0x%08x\n", magic7, MAGIC7); + return; + } +#endif + va_end(args); + puts("PASSED"); +} + +void main() { + stdio_init_all(); +#ifndef PICO_PANIC_FUNCTION + printf("Using default panic function...\n"); + panic("PASSED"); // should be printed +#else + #if PICO_PANIC_FUNCTION_IS_EMPTY + printf("Using empty (bkpt only) panic function...\n"); + printf("PASSED\n"); // we should breakpoint next + panic(MAGIC1, MAGIC2, MAGIC3, MAGIC4, MAGIC5, MAGIC6, MAGIC7); + #endif + printf("Using custom panic function '" __XSTRING(PICO_PANIC_FUNCTION) "'...\n"); + panic(MAGIC1, MAGIC2, MAGIC3, MAGIC4, MAGIC5, MAGIC6, MAGIC7); +#endif + printf("FAILED: expected panic not to return"); +}