From e0d3d5ae4d105babcb7584c7aa1f11ae2f0006fb Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Wed, 14 Feb 2024 14:29:37 -0500 Subject: [PATCH 1/2] Fix #1449, POSIX implementation honors stack pointer When the OS_TaskCreate() function is called with a non-NULL stack pointer, the POSIX implementation now should use this pointer as intended. Previously it was always allocating a new stack buffer, ignoring the passed-in pointer. NOTE: When using this feature, the caller must use caution to make sure the buffer being passed in is large enough to use as a task stack. Particularly when running under a desktop Linux/Glibc environment, the infrastructure puts a considerably large object on the stack. For example, in the osal-core-tests, the stack size was too small. --- .../tasking-example/tasking-example.c | 6 +-- src/os/posix/inc/os-impl-tasks.h | 4 +- src/os/posix/src/os-impl-console.c | 5 ++- src/os/posix/src/os-impl-tasks.c | 38 ++++++++++++------- src/os/posix/src/os-impl-timebase.c | 4 +- src/tests/bin-sem-test/bin-sem-test.c | 2 +- .../bin-sem-timeout-test.c | 4 +- src/tests/count-sem-test/count-sem-test.c | 2 +- src/tests/osal-core-test/osal-core-test.c | 34 +++++++++++++++++ src/tests/osal-core-test/osal-core-test.h | 8 ++-- src/tests/queue-test/queue-test.c | 4 +- 11 files changed, 78 insertions(+), 33 deletions(-) diff --git a/src/examples/tasking-example/tasking-example.c b/src/examples/tasking-example/tasking-example.c index 2316e767e..3c6cacd79 100644 --- a/src/examples/tasking-example/tasking-example.c +++ b/src/examples/tasking-example/tasking-example.c @@ -30,7 +30,7 @@ /* Task 1 */ #define TASK_1_ID 1 -#define TASK_1_STACK_SIZE 1024 +#define TASK_1_STACK_SIZE 4096 #define TASK_1_PRIORITY 101 uint32 task_1_stack[TASK_1_STACK_SIZE]; @@ -40,7 +40,7 @@ void task_1(void); /* Task 2 */ #define TASK_2_ID 2 -#define TASK_2_STACK_SIZE 1024 +#define TASK_2_STACK_SIZE 4096 #define TASK_2_PRIORITY 102 uint32 task_2_stack[TASK_2_STACK_SIZE]; @@ -50,7 +50,7 @@ void task_2(void); /* Task 3 */ #define TASK_3_ID 3 -#define TASK_3_STACK_SIZE 1024 +#define TASK_3_STACK_SIZE 4096 #define TASK_3_PRIORITY 103 uint32 task_3_stack[TASK_3_STACK_SIZE]; diff --git a/src/os/posix/inc/os-impl-tasks.h b/src/os/posix/inc/os-impl-tasks.h index 44be73731..25f1ead77 100644 --- a/src/os/posix/inc/os-impl-tasks.h +++ b/src/os/posix/inc/os-impl-tasks.h @@ -40,7 +40,7 @@ typedef struct /* Tables where the OS object information is stored */ extern OS_impl_task_internal_record_t OS_impl_task_table[OS_MAX_TASKS]; -int32 OS_Posix_InternalTaskCreate_Impl(pthread_t *pthr, osal_priority_t priority, size_t stacksz, - PthreadFuncPtr_t entry, void *entry_arg); +int32 OS_Posix_InternalTaskCreate_Impl(pthread_t *pthr, osal_priority_t priority, osal_stackptr_t stackptr, + size_t stacksz, PthreadFuncPtr_t entry, void *entry_arg); #endif /* OS_IMPL_TASKS_H */ diff --git a/src/os/posix/src/os-impl-console.c b/src/os/posix/src/os-impl-console.c index 9d735ff6d..19ba3a90e 100644 --- a/src/os/posix/src/os-impl-console.c +++ b/src/os/posix/src/os-impl-console.c @@ -128,8 +128,9 @@ int32 OS_ConsoleCreate_Impl(const OS_object_token_t *token) { /* cppcheck-suppress unreadVariable // intentional use of other union member */ local_arg.id = OS_ObjectIdFromToken(token); - return_code = OS_Posix_InternalTaskCreate_Impl(&consoletask, OS_CONSOLE_TASK_PRIORITY, 0, - OS_ConsoleTask_Entry, local_arg.opaque_arg); + return_code = + OS_Posix_InternalTaskCreate_Impl(&consoletask, OS_CONSOLE_TASK_PRIORITY, OSAL_TASK_STACK_ALLOCATE, + PTHREAD_STACK_MIN, OS_ConsoleTask_Entry, local_arg.opaque_arg); if (return_code != OS_SUCCESS) { diff --git a/src/os/posix/src/os-impl-tasks.c b/src/os/posix/src/os-impl-tasks.c index a64fec5cc..f1dacf62a 100644 --- a/src/os/posix/src/os-impl-tasks.c +++ b/src/os/posix/src/os-impl-tasks.c @@ -447,8 +447,8 @@ int32 OS_Posix_TaskAPI_Impl_Init(void) * Purpose: Local helper routine, not part of OSAL API. * *-----------------------------------------------------------------*/ -int32 OS_Posix_InternalTaskCreate_Impl(pthread_t *pthr, osal_priority_t priority, size_t stacksz, - PthreadFuncPtr_t entry, void *entry_arg) +int32 OS_Posix_InternalTaskCreate_Impl(pthread_t *pthr, osal_priority_t priority, osal_stackptr_t stackptr, + size_t stacksz, PthreadFuncPtr_t entry, void *entry_arg) { int return_code = 0; pthread_attr_t custom_attr; @@ -467,20 +467,30 @@ int32 OS_Posix_InternalTaskCreate_Impl(pthread_t *pthr, osal_priority_t priority } /* - * Adjust the stack size parameter, add budget for TCB/TLS overhead. - */ - stacksz += OS_IMPL_STACK_EXTRA; + ** Set the Stack Pointer and/or Size + */ + if (stackptr != OSAL_TASK_STACK_ALLOCATE) + { + return_code = pthread_attr_setstack(&custom_attr, stackptr, stacksz); + } + else + { + /* + * Adjust the stack size parameter, add budget for TCB/TLS overhead. + * Note that this budget can only be added when allocating the stack here, + * if the caller passed in a stack, they take responsibility for adding this. + */ + stacksz += OS_IMPL_STACK_EXTRA; - stacksz += POSIX_GlobalVars.PageSize - 1; - stacksz -= stacksz % POSIX_GlobalVars.PageSize; + stacksz += POSIX_GlobalVars.PageSize - 1; + stacksz -= stacksz % POSIX_GlobalVars.PageSize; + + return_code = pthread_attr_setstacksize(&custom_attr, stacksz); + } - /* - ** Set the Stack Size - */ - return_code = pthread_attr_setstacksize(&custom_attr, stacksz); if (return_code != 0) { - OS_DEBUG("pthread_attr_setstacksize error in OS_TaskCreate: %s\n", strerror(return_code)); + OS_DEBUG("Error configuring stack in OS_TaskCreate: %s\n", strerror(return_code)); return OS_ERROR; } @@ -588,8 +598,8 @@ int32 OS_TaskCreate_Impl(const OS_object_token_t *token, uint32 flags) task = OS_OBJECT_TABLE_GET(OS_task_table, *token); impl = OS_OBJECT_TABLE_GET(OS_impl_task_table, *token); - return_code = OS_Posix_InternalTaskCreate_Impl(&impl->id, task->priority, task->stack_size, OS_PthreadTaskEntry, - arg.opaque_arg); + return_code = OS_Posix_InternalTaskCreate_Impl(&impl->id, task->priority, task->stack_pointer, task->stack_size, + OS_PthreadTaskEntry, arg.opaque_arg); return return_code; } diff --git a/src/os/posix/src/os-impl-timebase.c b/src/os/posix/src/os-impl-timebase.c index 0edfd34d0..1fab8e7e1 100644 --- a/src/os/posix/src/os-impl-timebase.c +++ b/src/os/posix/src/os-impl-timebase.c @@ -347,8 +347,8 @@ int32 OS_TimeBaseCreate_Impl(const OS_object_token_t *token) /* cppcheck-suppress unreadVariable // intentional use of other union member */ arg.id = OS_ObjectIdFromToken(token); - return_code = OS_Posix_InternalTaskCreate_Impl(&local->handler_thread, OSAL_PRIORITY_C(0), 0, - OS_TimeBasePthreadEntry, arg.opaque_arg); + return_code = OS_Posix_InternalTaskCreate_Impl(&local->handler_thread, OSAL_PRIORITY_C(0), OSAL_TASK_STACK_ALLOCATE, + PTHREAD_STACK_MIN, OS_TimeBasePthreadEntry, arg.opaque_arg); if (return_code != OS_SUCCESS) { return return_code; diff --git a/src/tests/bin-sem-test/bin-sem-test.c b/src/tests/bin-sem-test/bin-sem-test.c index 1bb9c9f28..e02de461c 100644 --- a/src/tests/bin-sem-test/bin-sem-test.c +++ b/src/tests/bin-sem-test/bin-sem-test.c @@ -23,7 +23,7 @@ #include "utassert.h" #include "uttest.h" -#define TASK_STACK_SIZE 1024 +#define TASK_STACK_SIZE 16384 uint32 task_counter[3]; diff --git a/src/tests/bin-sem-timeout-test/bin-sem-timeout-test.c b/src/tests/bin-sem-timeout-test/bin-sem-timeout-test.c index 00ad53dc7..81410afb1 100644 --- a/src/tests/bin-sem-timeout-test/bin-sem-timeout-test.c +++ b/src/tests/bin-sem-timeout-test/bin-sem-timeout-test.c @@ -32,9 +32,9 @@ void BinSemTimeoutSetup(void); void BinSemTimeoutCheck(void); /* Task 1 */ -#define TASK_1_STACK_SIZE 1024 +#define TASK_1_STACK_SIZE 4096 #define TASK_1_PRIORITY 101 -#define TASK_2_STACK_SIZE 1024 +#define TASK_2_STACK_SIZE 4096 #define TASK_2_PRIORITY 50 uint32 task_1_stack[TASK_1_STACK_SIZE]; diff --git a/src/tests/count-sem-test/count-sem-test.c b/src/tests/count-sem-test/count-sem-test.c index 929ee2a42..23f4ce1e3 100644 --- a/src/tests/count-sem-test/count-sem-test.c +++ b/src/tests/count-sem-test/count-sem-test.c @@ -23,7 +23,7 @@ #include "utassert.h" #include "uttest.h" -#define TASK_STACK_SIZE 1024 +#define TASK_STACK_SIZE 4096 uint32 task_counter[3]; diff --git a/src/tests/osal-core-test/osal-core-test.c b/src/tests/osal-core-test/osal-core-test.c index bd2e37c59..59ec0adec 100644 --- a/src/tests/osal-core-test/osal-core-test.c +++ b/src/tests/osal-core-test/osal-core-test.c @@ -119,11 +119,27 @@ void task_generic_no_exit(void) void task_generic_with_exit(void) {} +void task_test_stackptr_0(void) +{ + int32 LocalVar; + cpuaddr VarAddress; + cpuaddr StackAddress; + + OS_TaskDelay(10); + + VarAddress = (cpuaddr)&LocalVar; + StackAddress = (cpuaddr)OSAL_STACKPTR_C(task_0_stack); + + UtAssert_GT(cpuaddr, VarAddress, StackAddress); + UtAssert_LT(cpuaddr, VarAddress, StackAddress + sizeof(task_0_stack)); +} + typedef struct { osal_id_t task_id; uint32 task_stack[TASK_0_STACK_SIZE]; } TestTaskData; + /* ********************************************** TASKS******************************* */ void TestTasks(void) { @@ -252,6 +268,24 @@ void TestTasks(void) UtAssert_True(OS_TaskDelete(task_1_id) != OS_SUCCESS, "OS_TaskDelete, Task 1"); UtAssert_True(OS_TaskDelete(task_2_id) == OS_SUCCESS, "OS_TaskDelete, Task 2"); UtAssert_True(OS_TaskDelete(task_3_id) == OS_SUCCESS, "OS_TaskDelete, Task 3"); + + /* + * Validate that the user-specified stack pointer parameter is implemented correctly. + * Addresses of local variables within the task should be within the given stack range + */ + UtAssert_INT32_EQ(OS_TaskCreate(&task_0_id, "Task 0", task_test_stackptr_0, OSAL_STACKPTR_C(task_0_stack), + sizeof(task_0_stack), OSAL_PRIORITY_C(TASK_0_PRIORITY), 0), + OS_SUCCESS); + + /* Looping delay in parent task to wait for child task to exit */ + loopcnt = 0; + while ((OS_TaskGetInfo(task_0_id, &taskprop) == OS_SUCCESS) && (loopcnt < UT_EXIT_LOOP_MAX)) + { + OS_TaskDelay(25); + loopcnt++; + } + + UtAssert_INT32_LT(loopcnt, UT_EXIT_LOOP_MAX); } /* ************************************************************************************ */ diff --git a/src/tests/osal-core-test/osal-core-test.h b/src/tests/osal-core-test/osal-core-test.h index 1cdf85284..6fe42f513 100644 --- a/src/tests/osal-core-test/osal-core-test.h +++ b/src/tests/osal-core-test/osal-core-test.h @@ -29,19 +29,19 @@ #define OSAL_CORE_TEST_H /* Task 0 */ -#define TASK_0_STACK_SIZE 1024 +#define TASK_0_STACK_SIZE 4096 #define TASK_0_PRIORITY 230 /* Task 1 */ -#define TASK_1_STACK_SIZE 1024 +#define TASK_1_STACK_SIZE 4096 #define TASK_1_PRIORITY 231 /* Task 2 */ -#define TASK_2_STACK_SIZE 1024 +#define TASK_2_STACK_SIZE 4096 #define TASK_2_PRIORITY 232 /* Task 3 */ -#define TASK_3_STACK_SIZE 1024 +#define TASK_3_STACK_SIZE 4096 #define TASK_3_PRIORITY 233 /* Global Data */ diff --git a/src/tests/queue-test/queue-test.c b/src/tests/queue-test/queue-test.c index 4f8870e1a..a0678fb35 100644 --- a/src/tests/queue-test/queue-test.c +++ b/src/tests/queue-test/queue-test.c @@ -36,9 +36,9 @@ void QueueTimeoutCheck(void); #define MSGQ_BURST 3 /* Task 1 */ -#define TASK_1_STACK_SIZE 1024 +#define TASK_1_STACK_SIZE 4096 #define TASK_1_PRIORITY 101 -#define TASK_2_STACK_SIZE 1024 +#define TASK_2_STACK_SIZE 4096 #define TASK_2_PRIORITY 50 uint32 task_1_stack[TASK_1_STACK_SIZE]; From d9ad6c49725bccc7d64222b795bde0769300a7ce Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Mon, 26 Feb 2024 11:08:13 -0500 Subject: [PATCH 2/2] Fix #1449, skip task stack test on RTEMS --- src/tests/osal-core-test/osal-core-test.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/tests/osal-core-test/osal-core-test.c b/src/tests/osal-core-test/osal-core-test.c index 59ec0adec..846312095 100644 --- a/src/tests/osal-core-test/osal-core-test.c +++ b/src/tests/osal-core-test/osal-core-test.c @@ -39,6 +39,7 @@ typedef struct } TestCallbackState_t; void TestTasks(void); +void TestTaskWithStackPtr(void); void InitializeTaskIds(void); void InitializeQIds(void); void InitializeBinIds(void); @@ -98,6 +99,16 @@ void UtTest_Setup(void) UtTest_AddTeardown(OS_API_Teardown, "Cleanup"); UtTest_Add(TestTasks, NULL, NULL, "TASK"); + + /* + * NOTE: The current RTEMS implementation does not adhere to passed-in stack pointers. + * The facility to create a task with a user-specified stack pointer is not available + * until RTEMS 6.x (development version at the time of this writing). This test will + * fail on currently-released RTEMS versions, so it is skipped. + */ +#ifndef _RTEMS_OS_ + UtTest_Add(TestTaskWithStackPtr, NULL, NULL, "TASKSTACK"); +#endif UtTest_Add(TestQueues, NULL, NULL, "MSGQ"); UtTest_Add(TestBinaries, NULL, NULL, "BSEM"); UtTest_Add(TestMutexes, NULL, NULL, "MSEM"); @@ -268,6 +279,12 @@ void TestTasks(void) UtAssert_True(OS_TaskDelete(task_1_id) != OS_SUCCESS, "OS_TaskDelete, Task 1"); UtAssert_True(OS_TaskDelete(task_2_id) == OS_SUCCESS, "OS_TaskDelete, Task 2"); UtAssert_True(OS_TaskDelete(task_3_id) == OS_SUCCESS, "OS_TaskDelete, Task 3"); +} + +void TestTaskWithStackPtr(void) +{ + OS_task_prop_t taskprop; + int loopcnt; /* * Validate that the user-specified stack pointer parameter is implemented correctly.