RISC-V: Create a wrapper around FreeRTOS Tasks to detect the ones ret…

…urning
espressif · Nov 23, 2022 · 96346c1 · 96346c1
1 parent 3d1c15c
commit 96346c1
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 59 deletions.
diff --git 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
@@ -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
@@ -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 ).
-
-    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();
-}
+    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);
 
-__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();
 }
+#endif // CONFIG_FREERTOS_TASK_FUNCTION_WRAPPER
+
 
 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