Merge branch 'staging/riscv_wrapper_freertos_tasks' into 'master'

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

Closes IDF-4662

See merge request espressif/esp-idf!20845

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

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,29 @@ 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 `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);
+    //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 +668,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;
 

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,29 @@ 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 `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);
+    //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 +246,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;