Add Thread Local Storage (TLS) support using Picolibc functions

[keith-packard]

Thread Local Storage (TLS) offers a platform-native mechanism for managing per-task variables that doesn't rely on custom support in FreeRTOS for things like errno.

Picolibc uses TLS internally for all variables that are intended to be task-local. Because of the TLS architecture, only variables which are actually used end up getting allocate space in the TLS block. Contrast this with the newlib 'struct _reent' block which holds all possible task-local values, even for APIs not used by the application.

Picolibc also has TLS helper functions that provide the necessary platform-specific code which makes the FreeRTOS changes platform independent. This patch uses those hooks instead of providing the platform specific code in FreeRTOS itself.

The picolibc helper functions rely on elements within the linker script to arrange the TLS data in memory and define some symbols. Applications wanting to use this mechanism will need changes in their linker script when migrating to picolibc.

Signed-off-by: Keith Packard [email protected]

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[codecov[bot]]

Codecov Report

Patch and project coverage have no change.

Comparison is base (e6514fb) 94.34% compared to head (f747433) 94.34%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #343   +/-   ##
=======================================
  Coverage   94.34%   94.34%           
=======================================
  Files           6        6           
  Lines        2368     2368           
  Branches      578      578           
=======================================
  Hits         2234     2234           
  Misses         85       85           
  Partials       49       49           

Flag	Coverage Δ
unittests	94.34% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tasks.c	94.69% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[aggarg]

Thank you for the change and apologies for the delay in getting back on this.

If I understand correctly, you are adding support for additional TLS (Thread Local Storage) when using picolibc because picolibc uses the default one for managing internal per-task variables. Is this understanding correct?

I briefly looked at the picolibc code and it uses linker script to place TLS data in a specific section and uses the variables exported from linker script to initialize the TLS section. Can the application writer not place their TLS data in the same section by using the same section name (.tdata)?

Thanks.

[keith-packard]

If I understand correctly, you are adding support for additional TLS (Thread Local Storage) when using picolibc because picolibc uses the default one for managing internal per-task variables. Is this understanding correct?

I don't understand what you mean by 'the default one'. There's one TLS area allocated for each thread by the OS in prvInitialiseNewTask, any TLS variables will be included in that.

I briefly looked at the picolibc code and it uses linker script to place TLS data in a specific section and uses the variables exported from linker script to initialize the TLS section. Can the application writer not place their TLS data in the same section by using the same section name (.tdata)?

They can (and should) use the .tbss or .tdata section. The compiler automatically places any variables in the __thread storage class in these sections and generates the code necessary to reference them using the architecture ABI for TLS variables.

TLS is a really useful capability to add to the operating system in general. When I submitted picolibc patches to Zephyr, they decided to add general TLS support to the OS and not base it on picolibc's hooks, then they started using TLS variables in the core OS itself, for things like the current task pointer. That's probably because Zephyr supports multi-core targets, so you can't just use regular global variables for some of this stuff.

[aggarg]

Thank you for the clarification. We are discussing internally - will get back to you.

[aggarg]

This PR generalizes the TLS support in FreeRTOS so that it is no longer newlib specific - https://github.com/FreeRTOS/FreeRTOS-Kernel/pull/540.

This should be sufficient to enable TLS support for picolibc?

Thanks.

[keith-packard]

This is now based on @aggarg 's generalized TLS support patch.

[aggarg]

Given that I have merged the other PR, can we close this one?

[keith-packard]

This PR is still valid -- it now sits atop your changes and adds picolibc-specific TLS support using your hooks.

[aggarg]

What do you think about moving this code to the application (i.e. FreeRTOSConfig.h)? This lets the application writer control which memory block is used for TLS - part of task stack, a new dynamically allocated block or a static memory block.

[keith-packard]

What do you think about moving this code to the application (i.e. FreeRTOSConfig.h)? This lets the application writer control which memory block is used for TLS - part of task stack, a new dynamically allocated block or a static memory block.

Applications are free to use their own TLS macros in FreeRTOSConfig.h that allocate the TLS area in some place other than the stack. FreeRTOSConfig.h would then not also have #define configUSE_PICOLIBC_TLS 1. Effectively, this is a recommendation on how to use picolibc's TLS support with FreeRTOS.

This code relies on internal details of where the macros are used -- they have to reference pxTopOfStack, which is a local variable in prvInitialiseNewTask. To me, this means it should be in the OS codebase so that changes to prvInitiliseNewTask can be reflected in these macros.

[aggarg]

This code relies on internal details of where the macros are used -- they have to reference pxTopOfStack, which is a local variable in prvInitialiseNewTask. To me, this means it should be in the OS codebase so that changes to prvInitiliseNewTask can be reflected in these macros.

That is one way to achieve it. The following are the possible scenarios -

1. The application uses xTaskCreate to create a task in which case FreeRTOS uses pvPortMalloc to allocate stack memory. The application writer could call pvPortMalloc to allocate TLS memory block.
2. The application uses xTaskCreate to create a task in which case the application writer supplies the block to be used as stack memory. a. They can allocate a big memory block and use part of it for stack and rest of it for TLS. b. They can use completely separate memory block for TLS.

Is there a specific value that we get by putting TLS memory next to stack in the first case?

[keith-packard]

Is there a specific value that we get by putting TLS memory next to stack in the first case?

• On systems with an MPU, you can save an MPU region by having TLS and stack in the same block of storage.
• TLS blocks are generally quite small; most libc users end up only placing errno there.
• There's no failure branch to handle oom for the TLS macro; how would you suggest handling allocation failures for a separate block?

I think the question here is why you would ever want to place it anywhere else. Of course, sometimes applications do need unusual configurations, and for that they are welcome to use their own TLS macros. But, providing a mechanism for doing the usual thing in the FreeRTOS source code seems like a nice convenience, as well as a good test that the provided TLS macros support another C library.

[RichardBarry]

Looks like an interesting debate. I will have to dive a bit deeper to offer an informed opinion. For now, I don't like creating a coupling between the kernel and any optional libraries if we can help it. I'm uncomfortable with the existing coupling to newlib, so it's great we are making it generic, enabling any lib, let's ensure the final update really is decoupled.

[keith-packard]

Looks like an interesting debate. I will have to dive a bit deeper to offer an informed opinion. For now, I don't like creating a coupling between the kernel and any optional libraries if we can help it. I'm uncomfortable with the existing coupling to newlib, so it's great we are making it generic, enabling any lib, let's ensure the final update really is decoupled.

@aggarg did a good job creating libc-independent TLS hooks in the core code. This patch adds a single chunk of conditional code which contains the link between picolibc and FreeRTOS, which parallels the conditional code block which does the same for newlib. Both chunks use internal details of FreeRTOS so they can't reasonably be hosted outside the FreeRTOS project.

The alternative would be to copy these code chunks into every project needing them, and if changes are ever necessary, patching them everywhere.

[RichardBarry]

The kernel provides a mechanism to enable users to add their own functions and extensions into tasks.c, with access to the private data, without editing the source file. See https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/V10.4.6/tasks.c#L5431 - I wonder if there are benefits to allowing code additions to both the top and bottom of the file. Then we could separate the newlib and pico lib extensions into header files, and have them brought in using a similar mechanism....just a thought.

[keith-packard]

Then we could separate the newlib and pico lib extensions into header files, and have them brought in using a similar mechanism....just a thought.

An easy change would be to move the library-specific TLS settings to separate header files (provided with FreeRTOS) and then have applications select them with something like

#ifdef configTLS_HEADER
#include configTLS_HEADER
#endif

To keep this compatible with applications using configUSE_NEWLIB_REENTRANT, we could have that option perform an appropriate #include as well.

Alternatively, add per-TLS framework config options and have those directly include the relevant header (some developers don't like using #include symbol)

[keith-packard]

I've updated this PR:

1. Pass pxTopOfStack to configINIT_TLS_BLOCK (picolibc needs this)
2. Restructure TLS definitions to place them in separate files (one per C library)
3. Remove use of CONFIG_USE_NEWLIB_REENTRANT. configUSE_C_RUNTIME_TLS_SUPPORT is correct now.
4. Fix a couple of ARC ports which duplicate the TCB struct in their own code.
5. Add Picolibc TLS support in a new file include/picolibc-freertos.h.

[mitch-johnson-ik]

Is there any help I could offer to help push this forward? It would be great to have picolibc support out of the box in FreeRTOS.

[keith-packard]

Is there any help I could offer to help push this forward? It would be great to have picolibc support out of the box in FreeRTOS.

I've rebased past the reformatting patch and it still seems fine. Happy to help get this merged however I can.

[multiplemonomials]

Um, is this a bad time to mention that I've just found a bug in this PR? Actually been using it for several months in our company's project, and this just surfaced today.

The issue is, ARM Cortex-M4 (and maybe other architectures, not sure) require thread stacks to be 8 byte aligned. FreeRTOS already does this here. However, the configINIT_TLS_BLOCK() macro provided by this PR adjusts the stack pointer to include the TLS data without accounting for alignment.

If your TLS block size is not a multiple if 8, it will end up un-aligning the stack pointer and causing all sorts of weird effects. How it manifested for me was that once I added some more TLS variables, suddenly passing int64_t variables to printf() no longer worked.

I think that these lines need to get added into configINIT_TLS_BLOCK():

        /* Align down to the nearest multiple of 8 */                                                  \
        pxTopOfStack = ( StackType_t * ) ( ( ( portPOINTER_SIZE_TYPE ) pxTopOfStack ) & ( ~( ( portPOINTER_SIZE_TYPE ) portBYTE_ALIGNMENT_MASK ) ) ); \

This fixes the issue for me and makes printf work again. This is for STACK_GROWTH < 0, IDK if something is needed for the other direction too.

@keith-packard does this make sense?

[keith-packard]

Yes, I think that would work. Hrm. Actually, it looks like this code is missing the __tls_align() adjustment changes which went into picolibc a couple of months ago, so we need changes here anyways, and we need to adjust both the upward and downward stack growth paths.

For downward growing stacks, we need to align the start of the TLS block (and hence the top of the stack) to the maximum of the stack alignment (8 bytes) and the __tls_align() value.

For upward growing stacks, we first need to align the tls block to the required alignment, then align the stack at least _tls_size() bytes above that to the stack alignment value.

As a simple fix in your case, simply adjusting the value computed for __tls_size in the linker script should suffice; you don't have additional alignment requirements for the TLS block itself.

I've got a patch proposed in https://github.com/FreeRTOS/FreeRTOS-Kernel/pull/637