From 588a65759c8199e31051b04e53cd702d984ce13b Mon Sep 17 00:00:00 2001 From: Mahavir Jain Date: Mon, 9 May 2022 15:45:38 +0530 Subject: [PATCH 1/4] freertos: fix allocation order for stack and TCB per portSTACK_GROWTH This is as per FreeRTOS recommendation and allows to protect task TCB in case task stack has overflowed. --- .../FreeRTOS-Kernel/portable/port_common.c | 42 ++++++++++++++++--- 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/components/freertos/FreeRTOS-Kernel/portable/port_common.c b/components/freertos/FreeRTOS-Kernel/portable/port_common.c index f132e09efd..ac0e8f48ad 100644 --- a/components/freertos/FreeRTOS-Kernel/portable/port_common.c +++ b/components/freertos/FreeRTOS-Kernel/portable/port_common.c @@ -159,9 +159,24 @@ void vApplicationGetIdleTaskMemory(StaticTask_t **ppxIdleTaskTCBBuffer, { StaticTask_t *pxTCBBufferTemp; StackType_t *pxStackBufferTemp; - //Allocate TCB and stack buffer in internal memory - pxTCBBufferTemp = pvPortMallocTcbMem(sizeof(StaticTask_t)); - pxStackBufferTemp = pvPortMallocStackMem(configIDLE_TASK_STACK_SIZE); + + /* If the stack grows down then allocate the stack then the TCB so the stack + * does not grow into the TCB. Likewise if the stack grows up then allocate + * the TCB then the stack. */ + #if (portSTACK_GROWTH > 0) + { + //Allocate TCB and stack buffer in internal memory + pxTCBBufferTemp = pvPortMallocTcbMem(sizeof(StaticTask_t)); + pxStackBufferTemp = pvPortMallocStackMem(configIDLE_TASK_STACK_SIZE); + } + #else /* portSTACK_GROWTH */ + { + //Allocate TCB and stack buffer in internal memory + pxStackBufferTemp = pvPortMallocStackMem(configIDLE_TASK_STACK_SIZE); + pxTCBBufferTemp = pvPortMallocTcbMem(sizeof(StaticTask_t)); + } + #endif /* portSTACK_GROWTH */ + assert(pxTCBBufferTemp != NULL); assert(pxStackBufferTemp != NULL); //Write back pointers @@ -184,9 +199,24 @@ void vApplicationGetTimerTaskMemory(StaticTask_t **ppxTimerTaskTCBBuffer, { StaticTask_t *pxTCBBufferTemp; StackType_t *pxStackBufferTemp; - //Allocate TCB and stack buffer in internal memory - pxTCBBufferTemp = pvPortMallocTcbMem(sizeof(StaticTask_t)); - pxStackBufferTemp = pvPortMallocStackMem(configTIMER_TASK_STACK_DEPTH); + + /* If the stack grows down then allocate the stack then the TCB so the stack + * does not grow into the TCB. Likewise if the stack grows up then allocate + * the TCB then the stack. */ + #if (portSTACK_GROWTH > 0) + { + //Allocate TCB and stack buffer in internal memory + pxTCBBufferTemp = pvPortMallocTcbMem(sizeof(StaticTask_t)); + pxStackBufferTemp = pvPortMallocStackMem(configTIMER_TASK_STACK_DEPTH); + } + #else /* portSTACK_GROWTH */ + { + //Allocate TCB and stack buffer in internal memory + pxStackBufferTemp = pvPortMallocStackMem(configTIMER_TASK_STACK_DEPTH); + pxTCBBufferTemp = pvPortMallocTcbMem(sizeof(StaticTask_t)); + } + #endif /* portSTACK_GROWTH */ + assert(pxTCBBufferTemp != NULL); assert(pxStackBufferTemp != NULL); //Write back pointers From 5b817038a00289f5b50520cd6455a0e772bd9987 Mon Sep 17 00:00:00 2001 From: Mahavir Jain Date: Mon, 9 May 2022 15:47:13 +0530 Subject: [PATCH 2/4] freertos: add return value to API `vTaskGetSnapshot` `vTaskGetSnapshot` is being used in coredump module to collect diagnostic information. It is possible that input arguments are invalid and `assert` in this situation is not correct. This commit modifies API signature to return pdTRUE in case of success, and pdFALSE otherwise. Caller can verify return value and then take appropriate decision. --- .../esp_additions/include/freertos/task_snapshot.h | 3 ++- .../private_include/freertos_tasks_c_additions.h | 9 ++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/components/freertos/esp_additions/include/freertos/task_snapshot.h b/components/freertos/esp_additions/include/freertos/task_snapshot.h index bd8fe82a1b..07849a8cd1 100644 --- a/components/freertos/esp_additions/include/freertos/task_snapshot.h +++ b/components/freertos/esp_additions/include/freertos/task_snapshot.h @@ -54,8 +54,9 @@ TaskHandle_t pxTaskGetNext( TaskHandle_t pxTask ); * does not acquire any locks. * @param[in] pxTask Task's handle * @param[out] pxTaskSnapshot Snapshot of the task + * @return pdTRUE if operation was successful else pdFALSE */ -void vTaskGetSnapshot( TaskHandle_t pxTask, TaskSnapshot_t *pxTaskSnapshot ); +BaseType_t vTaskGetSnapshot( TaskHandle_t pxTask, TaskSnapshot_t *pxTaskSnapshot ); /** * @brief Fill an array of TaskSnapshot_t structures for every task in the system diff --git a/components/freertos/esp_additions/private_include/freertos_tasks_c_additions.h b/components/freertos/esp_additions/private_include/freertos_tasks_c_additions.h index faa0415813..1703e3c5c9 100644 --- a/components/freertos/esp_additions/private_include/freertos_tasks_c_additions.h +++ b/components/freertos/esp_additions/private_include/freertos_tasks_c_additions.h @@ -162,14 +162,17 @@ TaskHandle_t pxTaskGetNext( TaskHandle_t pxTask ) return pxNextTCB; } -void vTaskGetSnapshot( TaskHandle_t pxTask, TaskSnapshot_t *pxTaskSnapshot ) +BaseType_t vTaskGetSnapshot( TaskHandle_t pxTask, TaskSnapshot_t *pxTaskSnapshot ) { - configASSERT( portVALID_TCB_MEM(pxTask) ); - configASSERT( pxTaskSnapshot != NULL ); + if (portVALID_TCB_MEM(pxTask) == false || pxTaskSnapshot == NULL) { + return pdFALSE; + } + TCB_t *pxTCB = (TCB_t *)pxTask; pxTaskSnapshot->pxTCB = pxTCB; pxTaskSnapshot->pxTopOfStack = (StackType_t *)pxTCB->pxTopOfStack; pxTaskSnapshot->pxEndOfStack = (StackType_t *)pxTCB->pxEndOfStack; + return pdTRUE; } UBaseType_t uxTaskGetSnapshotAll( TaskSnapshot_t * const pxTaskSnapshotArray, const UBaseType_t uxArrayLength, UBaseType_t * const pxTCBSize ) From 2a30a1012d3a8f85a9b88410bf3d979c7ea8a8d5 Mon Sep 17 00:00:00 2001 From: Mahavir Jain Date: Tue, 10 May 2022 11:40:47 +0530 Subject: [PATCH 3/4] docs: add migration guide note about `vTaskGetSnapshot` signature --- docs/en/migration-guides/freertos.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/en/migration-guides/freertos.rst b/docs/en/migration-guides/freertos.rst index 5aab155c47..76829e9ff2 100644 --- a/docs/en/migration-guides/freertos.rst +++ b/docs/en/migration-guides/freertos.rst @@ -14,6 +14,7 @@ Tasks Snapshot The header ``task_snapshot.h`` has been removed from ``freertos/task.h``. ESP-IDF developers should include ``"freertos/task_snapshot.h``` in case they need tasks snapshot API. +The function :cpp:func:`vTaskGetSnapshot` now returns ``BaseType_t``. Return value shall be ``pdTRUE`` on success and ``pdFALSE`` otherwise. FreeRTOS Asserts ---------------- From 318f7230421a60bc77d90f653e29107404361088 Mon Sep 17 00:00:00 2001 From: Mahavir Jain Date: Tue, 10 May 2022 14:19:17 +0530 Subject: [PATCH 4/4] freertos: extend snapshot test to check return status of `vTaskGetSnapshot` --- .../freertos/test/integration/tasks/test_tasks_snapshot.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/components/freertos/test/integration/tasks/test_tasks_snapshot.c b/components/freertos/test/integration/tasks/test_tasks_snapshot.c index 8551005d13..5b8ff6a0ce 100644 --- a/components/freertos/test/integration/tasks/test_tasks_snapshot.c +++ b/components/freertos/test/integration/tasks/test_tasks_snapshot.c @@ -138,7 +138,8 @@ TEST_CASE("Task snapshot: Iterate", "[freertos]") TaskHandle_t cur_task_handle = pxTaskGetNext(NULL); while (cur_task_handle != NULL) { // Get the task's snapshot - vTaskGetSnapshot(cur_task_handle, &task_snapshots[num_snapshots]); + BaseType_t Result = vTaskGetSnapshot(cur_task_handle, &task_snapshots[num_snapshots]); + TEST_ASSERT_EQUAL(pdTRUE, Result); num_snapshots++; cur_task_handle = pxTaskGetNext(cur_task_handle); }