Possible non-restricting feature on vTask SetThreadLocalStoragePointer()

Kernel
,
1

I wanted to discuss what i think might be a flaw in vTaskSetThreadLocalStaragePointer.
I was working on a project and by mistake, gave, as the first parameter (TaskHandle_t) of vTaskSetThreadLocalStaragePointer, pvParameters (which in my case was NULL) instead of pxCreatedTask. This done before starting the kernel.
The function vTaskSetThreadLocalStaragePointer gets the TCB from the macro prvGetTCBFromHandle, which gives out the handle of the current executing task if its parameter is NULL.
In my case it all seemed to run well until i changed priorities and found out that in one of my tasks i was overwriting another tasks thread local storage at creation, thus getting a runtime unexpected behaviour.

I understand it was a disattention from my side but i think vTaskSetThreadLocalStaragePointer should be more restrictive on input parameter, making sure its not NULL, and also returning pass or fail. In my opinion it should be the user to decide if he wants the current running tasks local storage pointer to be written (e.g by using xTaskGetCurrentTaskHandle), and it shall not be the default if NULL is given.

I’d like to hear feedback on this.
Thank you

2

The use of NULL pointers to mean the current task is a very convenient part of the FreeRTOS API. It would be natural for a given task to want to set its own pointer, as it allocates its memory.

I suppose one thing that could be added (perhaps as an option) would be that the use of NULL in the prvGetTCBFromHandle would assert if the scheduler wasn’t running, but that would add run-time cost to all calls that pass the null value.

3

I think it would be a good assert as NULL for task handle means “currently running task” which is meaningless till the scheduler is started.

4

Thank you for the responses. Considering your feedback a modification on the macro prvGetTCBFromHandle might be proposed. I was thinking of something on the lines of:

#define prvGetTCBFromHandle(pxHandle)                    \
    (                                                    \
        ((pxHandle) == NULL) ?                           \
        ({                                               \
            configASSERT((xSchedulerRunning) != pdFALSE);\
            pxCurrentTCB;                                \
        })                                               \
        : (pxHandle)                                     \
    )

even tough i’m not sure about the parameter of the assert, are there better ways to assert if the scheduler is running?

Thank you in advance for your feedback.

5

One issue is that is using a GCCism that may not work on other implementations. (The use of a block as an expression.

Standard C would do:

#define prvGetTCBFromHandle(pxHandle)                    \
    (                                                    \
        ((pxHandle) == NULL) ?                           \
        (   configASSERT((xSchedulerRunning) != pdFALSE),\
            pxCurrentTCB )                               \
        : (pxHandle)                                     \
    )

(using a comma expression to add the assert)

6

xSchedulerRunning denotes whether the scheduler was started or not. So you are right about the parameter of the assert.

1 Like
7

This does not compile for me. I think the issue is that configASSERT expands into an if statement, which cannot be used as part of a comma expression in the ternary operator.
Please correct me if i’m wrong

8

Right, you need to convert configAssert to use the same sort of trinary operation instead of an if to do that, but that wouldn’t be backwards compatible with most existing definitions.

Likely the full fix would be to make prvGetTCBFromHandle into a full function and not just a macro that expands to an expression.