From d580f6b076eed45dd2f956c65b89a13f1963067a Mon Sep 17 00:00:00 2001 From: Omar Chebib Date: Thu, 27 Oct 2022 18:41:19 +0800 Subject: [PATCH 1/2] RISC-V: Create a wrapper around FreeRTOS Tasks to detect the ones returning --- components/esp_system/panic.c | 16 +++-- .../FreeRTOS-Kernel-SMP/portable/riscv/port.c | 68 +++++++++--------- .../FreeRTOS-Kernel/portable/riscv/port.c | 69 ++++++++++--------- 3 files changed, 80 insertions(+), 73 deletions(-) diff --git a/components/esp_system/panic.c b/components/esp_system/panic.c index 8622a640ce..b263cf120c 100644 --- a/components/esp_system/panic.c +++ b/components/esp_system/panic.c @@ -257,8 +257,8 @@ void esp_panic_handler(panic_info_t *info) * * ---------------------------------------------------------------------------------------- * core - core where exception was triggered - * exception - what kind of exception occured - * description - a short description regarding the exception that occured + * exception - what kind of exception occurred + * description - a short description regarding the exception that occurred * details - more details about the exception * state - processor state like register contents, and backtrace * elf_info - details about the image currently running @@ -323,6 +323,14 @@ void esp_panic_handler(panic_info_t *info) PANIC_INFO_DUMP(info, state); panic_print_str("\r\n"); + /* No matter if we come here from abort or an exception, this variable must be reset. + * Else, any exception/error occurring during the current panic handler would considered + * an abort. Do this after PANIC_INFO_DUMP(info, state) as it also checks this variable. + * For example, if coredump triggers a stack overflow and this variable is not reset, + * the second panic would be still be marked as the result of an abort, even the previous + * message reason would be kept. */ + g_panic_abort = false; + #ifdef WITH_ELF_SHA256 panic_print_str("\r\nELF file SHA256: "); char sha256_buf[65]; @@ -359,10 +367,6 @@ void esp_panic_handler(panic_info_t *info) } else { disable_all_wdts(); s_dumping_core = true; - /* No matter if we come here from abort or an exception, this variable must be reset. - * Else, any exception/error occuring during the current panic handler would considered - * an abort. */ - g_panic_abort = false; #if CONFIG_ESP_COREDUMP_ENABLE_TO_FLASH esp_core_dump_to_flash(info); #endif diff --git a/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c b/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c index 96a7765e76..1a6ada0cc1 100644 --- a/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c +++ b/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c @@ -536,30 +536,6 @@ void vApplicationGetTimerTaskMemory(StaticTask_t **ppxTimerTaskTCBBuffer, // ------------------------ Stack -------------------------- -__attribute__((noreturn)) static void _prvTaskExitError(void) -{ - /* A function that implements a task must not exit or attempt to return to - its caller as there is nothing to return to. If a task wants to exit it - should instead call vTaskDelete( NULL ). - - Artificially force an assert() to be triggered if configASSERT() is - defined, then stop here so application writers can catch the error. */ - portDISABLE_INTERRUPTS(); - abort(); -} - -__attribute__((naked)) static void prvTaskExitError(void) -{ - asm volatile(".option push\n" \ - ".option norvc\n" \ - "nop\n" \ - ".option pop"); - /* Task entry's RA will point here. Shifting RA into prvTaskExitError is necessary - to make GDB backtrace ending inside that function. - Otherwise backtrace will end in the function laying just before prvTaskExitError in address space. */ - _prvTaskExitError(); -} - /** * @brief Align stack pointer in a downward growing stack * @@ -641,6 +617,32 @@ FORCE_INLINE_ATTR UBaseType_t uxInitialiseStackTLS(UBaseType_t uxStackPointer, u return uxStackPointer; } +#if CONFIG_FREERTOS_TASK_FUNCTION_WRAPPER +/** + * Wrapper to allow task functions to return. Force the optimization option -O1 on that function to make sure there + * is no tail-call. Indeed, we need the compiler to keep the return address to this function when calling `panic_abort`. + * + * Thanks to the `naked` attribute, GDB stub backtrace doesn't print: + * #2 0x00000000 in ?? () + * + * However, this also means that when calculating vPortTaskWrapper's call frame in a backtrace, return address + * register `ra`, will NOT contain 0x00000000. + */ +static void __attribute__((optimize("O1"), naked)) vPortTaskWrapper(TaskFunction_t pxCode, void *pvParameters) +{ + extern void __attribute__((noreturn)) panic_abort(const char *details); + static char DRAM_ATTR msg[80] = "FreeRTOS: FreeRTOS Task \"\0"; + pxCode(pvParameters); + //FreeRTOS tasks should not return. Log the task name and abort. + char *pcTaskName = pcTaskGetName(NULL); + /* We cannot use s(n)printf because it is in flash */ + strcat(msg, pcTaskName); + strcat(msg, "\" should not return, Aborting now!"); + panic_abort(msg); + +} +#endif // CONFIG_FREERTOS_TASK_FUNCTION_WRAPPER + /** * @brief Initialize the task's starting interrupt stack frame * @@ -669,16 +671,16 @@ FORCE_INLINE_ATTR UBaseType_t uxInitialiseStackFrame(UBaseType_t uxStackPointer, RvExcFrame *frame = (RvExcFrame *)uxStackPointer; memset(frame, 0, sizeof(RvExcFrame)); - /* - Initialize the stack frame. - - Note: Shifting RA into prvTaskExitError is necessary to make the GDB backtrace terminate inside that function. - Otherwise, the backtrace will end in the function located just before prvTaskExitError in the address space. - */ + /* Initialize the stack frame. */ extern uint32_t __global_pointer$; - frame->ra = (UBaseType_t)prvTaskExitError + 4; // size of the nop instruction at the beginning of prvTaskExitError - frame->mepc = (UBaseType_t)pxCode; - frame->a0 = (UBaseType_t)pvParameters; + #if CONFIG_FREERTOS_TASK_FUNCTION_WRAPPER + frame->mepc = (UBaseType_t)vPortTaskWrapper; + frame->a0 = (UBaseType_t)pxCode; + frame->a1 = (UBaseType_t)pvParameters; + #else + frame->mepc = (UBaseType_t)pxCode; + frame->a0 = (UBaseType_t)pvParameters; + #endif // CONFIG_FREERTOS_TASK_FUNCTION_WRAPPER frame->gp = (UBaseType_t)&__global_pointer$; frame->tp = (UBaseType_t)threadptr_reg_init; diff --git a/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c b/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c index f5b3d08545..d3b1582781 100644 --- a/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c +++ b/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c @@ -114,31 +114,6 @@ void vPortEndScheduler(void) // ------------------------ Stack -------------------------- -__attribute__((noreturn)) static void _prvTaskExitError(void) -{ - /* A function that implements a task must not exit or attempt to return to - its caller as there is nothing to return to. If a task wants to exit it - should instead call vTaskDelete( NULL ). - - Artificially force an assert() to be triggered if configASSERT() is - defined, then stop here so application writers can catch the error. */ - configASSERT(uxCriticalNesting == ~0UL); - portDISABLE_INTERRUPTS(); - abort(); -} - -__attribute__((naked)) static void prvTaskExitError(void) -{ - asm volatile(".option push\n" \ - ".option norvc\n" \ - "nop\n" \ - ".option pop"); - /* Task entry's RA will point here. Shifting RA into prvTaskExitError is necessary - to make GDB backtrace ending inside that function. - Otherwise backtrace will end in the function laying just before prvTaskExitError in address space. */ - _prvTaskExitError(); -} - /** * @brief Align stack pointer in a downward growing stack * @@ -220,6 +195,32 @@ FORCE_INLINE_ATTR UBaseType_t uxInitialiseStackTLS(UBaseType_t uxStackPointer, u return uxStackPointer; } +#if CONFIG_FREERTOS_TASK_FUNCTION_WRAPPER +/** + * Wrapper to allow task functions to return. Force the optimization option -O1 on that function to make sure there + * is no tail-call. Indeed, we need the compiler to keep the return address to this function when calling `panic_abort`. + * + * Thanks to the `naked` attribute, GDB stub backtrace doesn't print: + * #2 0x00000000 in ?? () + * + * However, this also means that when calculating vPortTaskWrapper's call frame in a backtrace, return address + * register `ra`, will NOT contain 0x00000000. + */ +static void __attribute__((optimize("O1"), naked)) vPortTaskWrapper(TaskFunction_t pxCode, void *pvParameters) +{ + extern void __attribute__((noreturn)) panic_abort(const char *details); + static char DRAM_ATTR msg[80] = "FreeRTOS: FreeRTOS Task \"\0"; + pxCode(pvParameters); + //FreeRTOS tasks should not return. Log the task name and abort. + char *pcTaskName = pcTaskGetName(NULL); + /* We cannot use s(n)printf because it is in flash */ + strcat(msg, pcTaskName); + strcat(msg, "\" should not return, Aborting now!"); + panic_abort(msg); + +} +#endif // CONFIG_FREERTOS_TASK_FUNCTION_WRAPPER + /** * @brief Initialize the task's starting interrupt stack frame * @@ -248,16 +249,16 @@ FORCE_INLINE_ATTR UBaseType_t uxInitialiseStackFrame(UBaseType_t uxStackPointer, RvExcFrame *frame = (RvExcFrame *)uxStackPointer; memset(frame, 0, sizeof(RvExcFrame)); - /* - Initialize the stack frame. - - Note: Shifting RA into prvTaskExitError is necessary to make the GDB backtrace terminate inside that function. - Otherwise, the backtrace will end in the function located just before prvTaskExitError in the address space. - */ + /* Initialize the stack frame. */ extern uint32_t __global_pointer$; - frame->ra = (UBaseType_t)prvTaskExitError + 4; // size of the nop instruction at the beginning of prvTaskExitError - frame->mepc = (UBaseType_t)pxCode; - frame->a0 = (UBaseType_t)pvParameters; + #if CONFIG_FREERTOS_TASK_FUNCTION_WRAPPER + frame->mepc = (UBaseType_t)vPortTaskWrapper; + frame->a0 = (UBaseType_t)pxCode; + frame->a1 = (UBaseType_t)pvParameters; + #else + frame->mepc = (UBaseType_t)pxCode; + frame->a0 = (UBaseType_t)pvParameters; + #endif // CONFIG_FREERTOS_TASK_FUNCTION_WRAPPER frame->gp = (UBaseType_t)&__global_pointer$; frame->tp = (UBaseType_t)threadptr_reg_init; From 6fe563163c01125d82581b433d3ac24228e5f7fa Mon Sep 17 00:00:00 2001 From: Omar Chebib Date: Wed, 16 Nov 2022 12:28:40 +0800 Subject: [PATCH 2/2] RISC-V: fix PC not saved when using backtrace --- .../freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c | 9 +++------ .../freertos/FreeRTOS-Kernel/portable/riscv/port.c | 9 +++------ 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c b/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c index 1a6ada0cc1..cb4a8bc8bd 100644 --- a/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c +++ b/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c @@ -622,14 +622,12 @@ FORCE_INLINE_ATTR UBaseType_t uxInitialiseStackTLS(UBaseType_t uxStackPointer, u * Wrapper to allow task functions to return. Force the optimization option -O1 on that function to make sure there * is no tail-call. Indeed, we need the compiler to keep the return address to this function when calling `panic_abort`. * - * Thanks to the `naked` attribute, GDB stub backtrace doesn't print: - * #2 0x00000000 in ?? () - * - * However, this also means that when calculating vPortTaskWrapper's call frame in a backtrace, return address - * register `ra`, will NOT contain 0x00000000. + * Thanks to `naked` attribute, the compiler won't generate a prologue and epilogue for the function, which saves time + * and stack space. */ static void __attribute__((optimize("O1"), naked)) vPortTaskWrapper(TaskFunction_t pxCode, void *pvParameters) { + asm volatile(".cfi_undefined ra\n"); extern void __attribute__((noreturn)) panic_abort(const char *details); static char DRAM_ATTR msg[80] = "FreeRTOS: FreeRTOS Task \"\0"; pxCode(pvParameters); @@ -639,7 +637,6 @@ static void __attribute__((optimize("O1"), naked)) vPortTaskWrapper(TaskFunction strcat(msg, pcTaskName); strcat(msg, "\" should not return, Aborting now!"); panic_abort(msg); - } #endif // CONFIG_FREERTOS_TASK_FUNCTION_WRAPPER diff --git a/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c b/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c index d3b1582781..13b8cc6cce 100644 --- a/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c +++ b/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c @@ -200,14 +200,12 @@ FORCE_INLINE_ATTR UBaseType_t uxInitialiseStackTLS(UBaseType_t uxStackPointer, u * Wrapper to allow task functions to return. Force the optimization option -O1 on that function to make sure there * is no tail-call. Indeed, we need the compiler to keep the return address to this function when calling `panic_abort`. * - * Thanks to the `naked` attribute, GDB stub backtrace doesn't print: - * #2 0x00000000 in ?? () - * - * However, this also means that when calculating vPortTaskWrapper's call frame in a backtrace, return address - * register `ra`, will NOT contain 0x00000000. + * Thanks to `naked` attribute, the compiler won't generate a prologue and epilogue for the function, which saves time + * and stack space. */ static void __attribute__((optimize("O1"), naked)) vPortTaskWrapper(TaskFunction_t pxCode, void *pvParameters) { + asm volatile(".cfi_undefined ra\n"); extern void __attribute__((noreturn)) panic_abort(const char *details); static char DRAM_ATTR msg[80] = "FreeRTOS: FreeRTOS Task \"\0"; pxCode(pvParameters); @@ -217,7 +215,6 @@ static void __attribute__((optimize("O1"), naked)) vPortTaskWrapper(TaskFunction strcat(msg, pcTaskName); strcat(msg, "\" should not return, Aborting now!"); panic_abort(msg); - } #endif // CONFIG_FREERTOS_TASK_FUNCTION_WRAPPER