Merge branch 'bugfix/shared_stack_not_switching_correctly' into 'master'

bugfix/shared_stack: Fix task stack not being replaced by shared stack correctly

See merge request espressif/esp-idf!7956

Author: projectgus

components/esp_common/include/esp_expression_with_stack.h

@@ -13,6 +13,7 @@
 // limitations under the License.
 #pragma once
 
+#include <stdbool.h>
 #include "freertos/FreeRTOS.h"
 #include "freertos/semphr.h"
 #include "freertos/task.h"
@@ -23,56 +24,25 @@
 extern "C" {
 #endif
 
+typedef void (*shared_stack_function)(void); 
+
+#define ESP_EXECUTE_EXPRESSION_WITH_STACK(lock, stack, stack_size, expression) \
+    esp_execute_shared_stack_function(lock, stack, stack_size, expression)
+
 /**
- * @brief Executes a 1-line expression with a application alocated stack
+ * @brief Calls user defined shared stack space function
  * @param lock Mutex object to protect in case of shared stack
  * @param stack Pointer to user alocated stack
  * @param stack_size Size of current stack in bytes
- * @param expression Expression or function to be executed using the stack
+ * @param function pointer to the shared stack function to be executed
  * @note  if either lock, stack or stack size is invalid, the expression will
  *          be called using the current stack.
  */
-#define ESP_EXECUTE_EXPRESSION_WITH_STACK(lock, stack, stack_size, expression)      \
-({                                                                                  \
-    assert(lock && stack && (stack_size >= CONFIG_ESP_MINIMAL_SHARED_STACK_SIZE));  \
-    uint32_t backup;                                                                \
-    xSemaphoreTake(lock, portMAX_DELAY);                                            \
-    StackType_t *top_of_stack = esp_switch_stack_setup(stack, stack_size);          \
-    esp_switch_stack_enter(top_of_stack, &backup);                                  \
-    {                                                                               \
-        expression;                                                                 \
-    }                                                                               \
-    esp_switch_stack_exit(&backup);                                                 \
-    StaticTask_t *current = (StaticTask_t *)xTaskGetCurrentTaskHandle();            \
-    /* pxDummy6 is the stack base of current thread defined in TCB_t */             \
-    /* place the watchpoint on current task stack after function execution*/        \
-    vPortSetStackWatchpoint(current->pxDummy6);                                     \
-    xSemaphoreGive(lock);                                                           \
-})
-
-/**
- * @brief Fill stack frame with CPU-specifics value before use
- * @param stack Caller allocated stack pointer
- * @param stack_size Size of stack in bytes
- * @return New pointer to the top of stack
- * @note Application must not call this function directly
- */
-StackType_t * esp_switch_stack_setup(StackType_t *stack, size_t stack_size);
+void esp_execute_shared_stack_function(SemaphoreHandle_t lock, 
+                                      void *stack,
+                                      size_t stack_size,
+                                      shared_stack_function function);
 
-/**
- * @brief Changes CPU sp-register to use another stack space and save the previous one
- * @param stack Caller allocated stack pointer
- * @param backup_stack Pointer to a place to save the current stack
- * @note Application must not call this function directly
- */
-extern void esp_switch_stack_enter(StackType_t *stack, uint32_t *backup_stack);
-
-/**
- * @brief Restores the previous CPU sp-register
- * @param backup_stack Pointer to the place where stack was saved
- * @note Application must not call this function directly
- */
-extern void esp_switch_stack_exit(uint32_t *backup_stack);
 
 #ifdef __cplusplus
 }

components/newlib/test/test_shared_stack_printf.c

@@ -8,25 +8,53 @@
 #include "test_utils.h"
 #include "esp_expression_with_stack.h"
 
-//makes sure this is not the task stack...
+#define SHARED_STACK_SIZE 8192
+
+static StackType_t *shared_stack_sp = NULL; 
+
+void external_stack_function(void)
+{
+    printf("Executing this printf from external stack! sp=%p\n", get_sp());
+    shared_stack_sp = (StackType_t *)get_sp();
+}
+
 void another_external_stack_function(void) 
 {
     //We can even use Freertos resources inside of this context.
-    vTaskDelay(100);
-    printf("Executing this another printf inside a function with external stack");
+    printf("We can even use FreeRTOS resources... yielding, sp=%p\n", get_sp());
+    taskYIELD();
+    shared_stack_sp = (StackType_t *)get_sp();
 }
 
 TEST_CASE("test printf using shared buffer stack", "[newlib]")
 {
-    portSTACK_TYPE *shared_stack = malloc(8192 * sizeof(portSTACK_TYPE));
+    portSTACK_TYPE *shared_stack = malloc(SHARED_STACK_SIZE);
 
-    TEST_ASSERT(shared_stack != NULL);
+    TEST_ASSERT_NOT_NULL(shared_stack);
 
     SemaphoreHandle_t printf_lock = xSemaphoreCreateMutex();
     TEST_ASSERT_NOT_NULL(printf_lock);
+    printf("current task sp: %p\n", get_sp());
+    printf("shared_stack: %p\n", (void *)shared_stack);
+    printf("shared_stack expected top: %p\n", (void *)(shared_stack + SHARED_STACK_SIZE));
+
+
+    esp_execute_shared_stack_function(printf_lock, 
+                                    shared_stack,
+                                    SHARED_STACK_SIZE,
+                                    external_stack_function);
+    
+    TEST_ASSERT(((shared_stack_sp >= shared_stack) && 
+                (shared_stack_sp < (shared_stack + SHARED_STACK_SIZE))));
+    
+    esp_execute_shared_stack_function(printf_lock, 
+                                    shared_stack,
+                                    SHARED_STACK_SIZE,
+                                    another_external_stack_function); 
+    
+    TEST_ASSERT(((shared_stack_sp >= shared_stack) && 
+                (shared_stack_sp < (shared_stack + SHARED_STACK_SIZE))));
 
-    ESP_EXECUTE_EXPRESSION_WITH_STACK(printf_lock, shared_stack,8192,printf("Executing this printf from external stack! \n"));
-    ESP_EXECUTE_EXPRESSION_WITH_STACK(printf_lock, shared_stack,8192,another_external_stack_function()); 
     vSemaphoreDelete(printf_lock);   
     free(shared_stack);
 }

components/xtensa/CMakeLists.txt

@@ -7,8 +7,8 @@ else()
     set(priv_requires soc freertos)
     set(srcs "debug_helpers.c"
              "debug_helpers_asm.S"
-             "expression_with_stack_xtensa_asm.S"
              "expression_with_stack_xtensa.c"
+             "expression_with_stack_xtensa_asm.S"
              "eri.c"
              "trax.c"
              "${target}/trax_init.c"

components/xtensa/expression_with_stack_xtensa.c

@@ -15,29 +15,72 @@
 #include <esp_expression_with_stack.h>
 #include <freertos/xtensa_rtos.h>
 #include <freertos/xtensa_context.h>
+#include <setjmp.h>
+#include <string.h>
 
-StackType_t * esp_switch_stack_setup(StackType_t *stack, size_t stack_size)
+StackType_t *xtensa_shared_stack;  
+shared_stack_function xtensa_shared_stack_callback;
+jmp_buf xtensa_shared_stack_env;
+bool xtensa_shared_stack_function_done = false;
+static portMUX_TYPE xtensa_shared_stack_spinlock = portMUX_INITIALIZER_UNLOCKED;
+static void *current_task_stack = NULL;
+
+extern void esp_shared_stack_invoke_function(void);
+
+static void esp_switch_stack_setup(StackType_t *stack, size_t stack_size)
 {
 #if CONFIG_FREERTOS_WATCHPOINT_END_OF_STACK
     esp_clear_watchpoint(1);
-    uint32_t watchpoint_place = ((uint32_t)stack + 32) & 0x1f ;
+    uint32_t watchpoint_place = ((uint32_t)stack + 32) & ~0x1f ;
 #endif    
-    StackType_t *top_of_stack =  (StackType_t *)&stack[0] +                  
-            ((stack_size * sizeof(StackType_t)) / sizeof(StackType_t));
+    //We need also to tweak current task stackpointer to avoid erroneous
+    //stack overflow indication, so fills the stack with freertos known pattern:
+    memset(stack, 0xa5U, stack_size * sizeof(StackType_t));
 
-    //Align stack to a 16byte boundary, as required by CPU specific:
-    top_of_stack =  (StackType_t *)(((UBaseType_t)(top_of_stack - 31) -
-                                    ALIGNUP(0x10, sizeof(XtSolFrame) )) & 
-                                    ~0xf);
+    StaticTask_t *current = (StaticTask_t *)xTaskGetCurrentTaskHandle();
+    //Then put the fake stack inside of TCB:
+    current_task_stack = current->pxDummy6;
+    current->pxDummy6 = (void *)stack;
 
-    //Fake stack frame to do not break the backtrace
-    XtSolFrame *frame = (XtSolFrame *)top_of_stack;
-    frame->a0 = 0;
-    frame->a1 = (UBaseType_t)top_of_stack;
+    StackType_t *top_of_stack = stack + stack_size;
+
+    //Align stack to a 16byte boundary, as required by CPU specific:
+    top_of_stack =  (StackType_t *)(((UBaseType_t)(top_of_stack - 16) & ~0xf));
 
 #if CONFIG_FREERTOS_WATCHPOINT_END_OF_STACK
     esp_set_watchpoint(1, (uint8_t *)watchpoint_place, 32, ESP_WATCHPOINT_STORE);    
 #endif
 
-    return top_of_stack;           
+    xtensa_shared_stack = top_of_stack;
+}
+
+
+void esp_execute_shared_stack_function(SemaphoreHandle_t lock, void *stack, size_t stack_size, shared_stack_function function)
+{
+    assert(lock);
+    assert(stack);
+    assert(stack_size > 0 && stack_size >= CONFIG_ESP_MINIMAL_SHARED_STACK_SIZE);
+    assert(function);
+
+    xSemaphoreTake(lock, portMAX_DELAY);
+    portENTER_CRITICAL(&xtensa_shared_stack_spinlock);
+    xtensa_shared_stack_function_done = false;
+    esp_switch_stack_setup(stack, stack_size);
+    xtensa_shared_stack_callback = function;
+    portEXIT_CRITICAL(&xtensa_shared_stack_spinlock);
+    
+    setjmp(xtensa_shared_stack_env);    
+    if(!xtensa_shared_stack_function_done) {
+        esp_shared_stack_invoke_function();             
+    }
+
+    portENTER_CRITICAL(&xtensa_shared_stack_spinlock);
+    StaticTask_t *current = (StaticTask_t *)xTaskGetCurrentTaskHandle();
+
+    //Restore current task stack:
+    current->pxDummy6 = (StackType_t *)current_task_stack;
+    vPortSetStackWatchpoint(current->pxDummy6);
+    portEXIT_CRITICAL(&xtensa_shared_stack_spinlock);
+
+    xSemaphoreGive(lock);
 }

components/xtensa/expression_with_stack_xtensa_asm.S

@@ -14,43 +14,38 @@
 
 #include <freertos/xtensa_context.h>
 
-    .extern esp_clear_watchpoint
+    .extern xtensa_shared_stack
+    .extern xtensa_shared_stack_callback
+    .extern xtensa_shared_stack_function_done
+    .extern xtensa_shared_stack_env
+    .extern longjmp
     .text
 
-/**
- * extern void switch_stack_enter(portSTACK_TYPE *stack, portSTACK_TYPE *backup_stack);
- */
-    .globl     esp_switch_stack_enter
-    .type       esp_switch_stack_enter,@function
-    .align      4
-esp_switch_stack_enter:
 
-    #ifndef __XTENSA_CALL0_ABI__
-    entry   sp, 0x10
-    mov   a4, a1
-    s32i   a4, a3, 0 /* on a3 there is a safe place to save the current stack */
-    l32i   a4, a2, 0 /* obtains the user allocated stack buffer */
-    mov   a1, a4     /* sp register now contains caller specified stack */
-    retw
-    #else 
-    #error "this code is written for Window ABI"
-    #endif
+/* extern void esp_shared_stack_invoke_function(void) */
 
-/**
- * extern void switch_stack_exit(portSTACK_TYPE *backup_stack);
- */
-    .globl  esp_switch_stack_exit
-    .type   esp_switch_stack_exit,@function
-    .align  4
-esp_switch_stack_exit:
+    .globl     esp_shared_stack_invoke_function
+    .type       esp_shared_stack_invoke_function,@function
+    .align      4
+esp_shared_stack_invoke_function:
 
     #ifndef __XTENSA_CALL0_ABI__
-    entry   sp, 0x10
-
-    l32i a4, a2, 0  /* recover the original task stack */
-    mov a1, a4      /* put it on sp register again */
-    retw
-
+    movi    a0, 0                   /* must not rotate the window here, */
+                                    /* the state of execution for shared stack */
+                                    /* functions will be completely destroyed at end */
+    movi    a6, xtensa_shared_stack        
+    l32i    sp, a6, 0               /* load shared stack pointer */
+    movi    a12, xtensa_shared_stack_callback
+    l32i    a12, a12, 0
+    callx4  a12                     /* call user function */
+    movi    a6, xtensa_shared_stack_function_done
+    movi    a7, 1       
+    s32i    a7, a6, 0               /* hint the function was finished */
+    movi    a6, xtensa_shared_stack_env    
+    movi    a7, 0
+    movi    a12, longjmp     
+    callx4  a12                 /* jump to last clean state previously saved */
+    ret
     #else 
     #error "this code is written for Window ABI"
     #endif

docs/en/api-reference/system/esp_expression_with_stack.rst renamed to docs/en/api-reference/system/esp_function_with_shared_stack.rst

@@ -8,24 +8,31 @@ A given function can be executed with a user allocated stack space
 which is independent of current task stack, this mechanism can be
 used to save stack space wasted by tasks which call a common function
 with intensive stack usage such as `printf`. The given function can
-be called inside the macro :cpp:func:`ESP_EXECUTE_EXPRESSION_WITH_STACK` 
-it will redirect the target function to be executed using the space
-allocated by the user.
+be called inside the shared stack space which is a callback function
+deferred by calling :cpp:func:`esp_execute_shared_stack_function`, 
+passing that function as parameter
 
 Usage
 -----
 
-:cpp:func:`ESP_EXECUTE_EXPRESSION_WITH_STACK` takes three arguments, 
+:cpp:func:`esp_execute_shared_stack_function` takes four arguments, 
 a mutex object allocated by the caller, which is used to protect if 
 the same function shares its allocated stack, a pointer to the top 
-of stack used to that fuction, and the function itself, note the
-function is passed exactly in the same way as do when you call 
-it on a regular way. 
+of stack used to that fuction, the size in bytes of stack and, a pointer
+to a user function where the shared stack space will reside, after calling
+the function, the user defined function will be deferred as a callback
+where functions can be called using the user allocated space without
+taking space from current task stack. 
 
 The usage may looks like the code below:
 
 .. code-block:: c
 
+    void external_stack_function(void)
+    {
+        printf("Executing this printf from external stack! \n");
+    }
+
     //Let's suppose we wanting to call printf using a separated stack space
     //allowing app to reduce its stack size.
     void app_main()
@@ -39,9 +46,11 @@ The usage may looks like the code below:
         assert(printf_lock != NULL);
      
         //Call the desired function using the macro helper:
-        ESP_EXECUTE_EXPRESSION_WITH_STACK(printf_lock, 
-                                        shared_stack, 
-                                        printf("Executing this from external stack! \n"));
+        esp_execute_shared_stack_function(printf_lock, 
+                                        shared_stack,
+                                        8192,
+                                        external_stack_function);
+        
         vSemaphoreDelete(printf_lock);    
         free(shared_stack); 
     }

docs/en/api-reference/system/index.rst

@@ -19,7 +19,7 @@ System API
     High Resolution Timer <esp_timer>
     :esp32: Himem (large external SPI RAM) API <himem>
     :esp32: Inter-Processor Call <ipc>
-    Call function with external stack <esp_expression_with_stack>
+    Call function with external stack <esp_function_with_shared_stack>
     Interrupt Allocation <intr_alloc>
     Logging <log>
     Miscellaneous System APIs <system>

docs/zh_CN/api-reference/system/esp_expression_with_stack.rst

@@ -0,0 +1 @@
+.. include:: ../../../en/api-reference/system/esp_function_with_shared_stack.rst