From 96346c1eed739f7a995b4d87b13ef22ee1372878 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 | 57 ++++++++++-------- .../FreeRTOS-Kernel/portable/riscv/port.c | 58 ++++++++++--------- 3 files changed, 73 insertions(+), 58 deletions(-) diff --git a/components/esp_system/panic.c b/components/esp_system/panic.c index d060f48bfb..559bd6c68e 100644 --- a/components/esp_system/panic.c +++ b/components/esp_system/panic.c @@ -246,8 +246,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 @@ -312,6 +312,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]; @@ -348,10 +356,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 e9e10b1972..070cc3cf68 100644 --- a/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c +++ b/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c @@ -504,30 +504,32 @@ void vApplicationGetTimerTaskMemory(StaticTask_t **ppxTimerTaskTCBBuffer, #endif //( configSUPPORT_STATIC_ALLOCATION == 1 ) // ------------------------ Stack -------------------------- - -__attribute__((noreturn)) static void _prvTaskExitError(void) +#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) { - /* 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 ). + 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); - Artificially force an assert() to be triggered if configASSERT() is - defined, then stop here so application writers can catch the error. */ - portDISABLE_INTERRUPTS(); - abort(); } +#endif // CONFIG_FREERTOS_TASK_FUNCTION_WRAPPER -__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(); -} StackType_t *pxPortInitialiseStack(StackType_t *pxTopOfStack, TaskFunction_t pxCode, void *pvParameters) { @@ -591,11 +593,16 @@ StackType_t *pxPortInitialiseStack(StackType_t *pxTopOfStack, TaskFunction_t pxC sp -= RV_STK_FRMSZ; RvExcFrame *frame = (RvExcFrame *)sp; memset(frame, 0, sizeof(*frame)); - /* 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. */ - frame->ra = (UBaseType_t)prvTaskExitError + 4/*size of the nop insruction at the beginning of prvTaskExitError*/; - frame->mepc = (UBaseType_t)pxCode; - frame->a0 = (UBaseType_t)pvParameters; + + /* Initialize the stack frame. */ + #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; diff --git a/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c b/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c index f001259166..ae36e950fa 100644 --- a/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c +++ b/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c @@ -113,31 +113,29 @@ void vPortEndScheduler(void) } // ------------------------ Stack -------------------------- - -__attribute__((noreturn)) static void _prvTaskExitError(void) +#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 ?? () + */ +static void __attribute__((optimize("O1"), naked)) vPortTaskWrapper(TaskFunction_t pxCode, void *pvParameters) { - /* 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 ). + 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); - 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(); } +#endif // CONFIG_FREERTOS_TASK_FUNCTION_WRAPPER -__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(); -} StackType_t *pxPortInitialiseStack(StackType_t *pxTopOfStack, TaskFunction_t pxCode, void *pvParameters) { @@ -201,12 +199,18 @@ StackType_t *pxPortInitialiseStack(StackType_t *pxTopOfStack, TaskFunction_t pxC sp -= RV_STK_FRMSZ; RvExcFrame *frame = (RvExcFrame *)sp; memset(frame, 0, sizeof(*frame)); - /* 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. */ - frame->ra = (UBaseType_t)prvTaskExitError + 4/*size of the nop insruction at the beginning of prvTaskExitError*/; - frame->mepc = (UBaseType_t)pxCode; - frame->a0 = (UBaseType_t)pvParameters; - frame->gp = (UBaseType_t)&__global_pointer$; + + /* Initialize the stack frame. */ + extern uint32_t __global_pointer$; + #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; //TODO: IDF-2393 From a4b2aa8a579143641634c6813644cc528c86ea25 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 | 6 +++--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c b/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c index 070cc3cf68..42c2cc0f83 100644 --- a/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c +++ b/components/freertos/FreeRTOS-Kernel-SMP/portable/riscv/port.c @@ -509,14 +509,12 @@ void vApplicationGetTimerTaskMemory(StaticTask_t **ppxTimerTaskTCBBuffer, * 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); @@ -526,7 +524,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 ae36e950fa..183b05ca1b 100644 --- a/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c +++ b/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c @@ -118,11 +118,12 @@ void vPortEndScheduler(void) * 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 ?? () + * 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); @@ -132,7 +133,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