Add pico_thread_local library providing thread local variable support #2910
No reviewers
Labels
No labels
bazel
bluetooth
board settings
boot_stage2
bug
build
c++
can't reproduce
clang
clib-interface
cmsis
code cleanliness
code size
compiler version issues
cyw43_driver
debug
discussion
documentation
duplicate
dynamic_clock
elf2uf2
enhancement
errata
filesystem
flash layout
floating point
FreeRTOS
hardware_adc
hardware_claim
hardware_clocks
hardware_dma
hardware_exception
hardware_flash
hardware_gpio
hardware_i2c
hardware_irq
hardware_pio
hardware_pwm
hardware_rcp
hardware_rtc
hardware_spi
hardware_sync
hardware_uart
hardware_vreg
hardware_watchdog
hardware_xosc
help wanted
host
IAR
installation
invalid
linker scripts
lto
lwip
mbedtls
missing API
needs reporter detail
networking
newlib
newlib-multicore
non-c
non-cmake
non-gcc
openocd
orphan PR
OS integration
pico_async_context
pico_base
pico_binary_info
pico_flash
picolibc
pico_lwip
pico_malloc
pico_multicore
pico_printf
pico_runtime
pico_stdio
pico_stdio_usb
pico_sync
pico_time
pico_util
pioasm
port
question
review
review2
rtos interop
runtime init
static library
svd
test
tinyusb
vscode
wifi integration
wontfix
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-raspberrypi/pico-sdk!2910
Loading…
Reference in a new issue
No description provided.
Delete branch "tls-support"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
PER_THREAD tls mode
GLOBAL tls mode
NONE tls mode
cc @alastairpatrick @keith-packard
@ -0,0 +1,93 @@I don't think
TLS_TYPEneeds to be aCACHEvariable, it can just be a normal oneIt might be better to write this in
CMAKE_CURRENT_BINARY_DIRto avoid polluting the root of the build directory@ -0,0 +76,4 @@message(STATUS "Compiler does not support __thread keyword.")set(TLS_TYPE "unsupported")endif()set(${OUTPUT_VAR} ${TLS_TYPE} PARENT_SCOPE)Maybe delete the files before returning too, to clean up
@ -0,0 +1,122 @@/*It looks like there are 2 names used for this -
PICO_THREAD_LOCAL_THREAD_POINTER_ARM_CONFIRMEDhere which has thePICO_CONFIGbut doesn't affect the build, orPICO_THREAD_LOCAL_THREAD_POINTER_ARM_AEABI_CONFIRMEDbelow which doesn't have aPICO_CONFIGbut does affect the build. I assume these are supposed to be the same name?CMake
setdoesn't use=@ -0,0 +1,122 @@/*i will review - actually i think i'm gonna back out the part of @lurch config checker which requires initializers for defaults of 0 too because these defines just clutter things
@ -151,6 +151,17 @@ string_flag(],@ -0,0 +1,63 @@/* Sections for proper TLS@ -0,0 +42,4 @@}")# Create a temporary file for the sourcePerhaps it's worth doing
and then e.g. line 79 might "simplify" to:
@ -0,0 +7,4 @@#ifndef _PICO_THREAD_LOCAL_PICOTLS_H#define _PICO_THREAD_LOCAL_PICOTLS_H// Note the name of this file is slightly confusing - this is actually overriding <picotls.h> from picolibc (if present)!...and presumably nothing to do with mbedtls? 🤣
@ -0,0 +1,122 @@/*@ -0,0 +1,122 @@/*@ -0,0 +55,4 @@#if PICO_THREAD_LOCAL_EMUTLS_CONFIRMED + PICO_THREAD_LOCAL_THREAD_POINTER_ARM_AEABI_CONFIRMED + PICO_THREAD_LOCAL_THREAD_POINTER_RISCV_REG_CONFIRMED > 1#error expected at most one of PICO_THREAD_LOCAL_EMUTLS_CONFIRMED, PICO_THREAD_LOCAL_THREAD_POINTER_ARM_AEABI_CONFIRMED and PICO_THREAD_LOCAL_THREAD_POINTER_RISCV_REG_CONFIRMED to be set#endifCunning. Maybe it's worth doing something similar to check that only one of
PICO_THREAD_LOCAL_MODE_PER_THREAD,PICO_THREAD_LOCAL_MODE_GLOBALorPICO_THREAD_LOCAL_MODE_NONEis set, rather than the the multiple nested#ifs that are being used currently?@ -0,0 +1,335 @@/*@ -0,0 +1,335 @@/*?
@ -0,0 +1,335 @@/*This seems to missing code-comments for passes 2 and 3? 🤨
@ -0,0 +1,335 @@/*Was this supposed to say
replacement <picotls.h>? 🤷@ -0,0 +1,335 @@/*@ -0,0 +237,4 @@// tls = aligned_alloc(size, _emutls_align);void *tls = malloc(size + _emutls_align - 1);// note we never free the memory, so bumping the pointer is finetls = (void *)((((uintptr_t)tls) + (_emutls_align - 1)) & ~(_emutls_align - 1));Perhaps?
Or is that the kind of optimization that the compiler will do automatically?
@ -0,0 +1,335 @@/*@ -0,0 +314,4 @@// we expect an impl in this case_INIT_TLS_IMPL(tls);}#endifShould this
#errorif_INIT_TLS_IMPLisn't defined?(and obviously same question for
_SET_TLS_IMPLbelow)@ -0,0 +7,4 @@add_executable(pico_thread_local_test_global pico_thread_local_test.c)target_link_libraries(pico_thread_local_test_global PRIVATE pico_stdlib pico_multicore)pico_add_extra_outputs(pico_thread_local_test_global)pico_set_thread_local_implementation(pico_thread_local_test_global global)Should this also be testing the "none" implementation?
EDIT: Doh, forget I said that 😉 "Code using them may not compile or link"
@ -0,0 +1,96 @@/**2026?
@ -0,0 +1,96 @@/**Maybe it's worth doing
and then all the
7s in this file could be replaced withCOUNTER_START?@ -0,0 +1,96 @@/**Huh?
@ -303,8 +303,11 @@ for applicable, all_configs in chips_all_configs.items():first_define_value = get_first_dict_key(defines_obj)I think I'd be happier with this "a missing default #define is allowed if the default is 0" if it was restricted to only
booltypes? i.e.(I've also swapped things around so that the Exception is in the final
else:clause, which is how it was structured before)@ -303,8 +303,11 @@ for applicable, all_configs in chips_all_configs.items():first_define_value = get_first_dict_key(defines_obj)yes, i had already though to restrict it to bools
@ -0,0 +7,4 @@#ifndef _PICO_THREAD_LOCAL_PICOTLS_H#define _PICO_THREAD_LOCAL_PICOTLS_H// Note the name of this file is slightly confusing - this is actually overriding <picotls.h> from picolibc (if present)!correct - hence tls -> thread_local_storage where we have control
@ -0,0 +42,4 @@}")# Create a temporary file for the sourcepossibly, but i dont care enough ;-)
@ -0,0 +1,335 @@/*yes
@ -0,0 +237,4 @@// tls = aligned_alloc(size, _emutls_align);void *tls = malloc(size + _emutls_align - 1);// note we never free the memory, so bumping the pointer is finetls = (void *)((((uintptr_t)tls) + (_emutls_align - 1)) & ~(_emutls_align - 1));yes the compiler is quite capable
@ -0,0 +314,4 @@// we expect an impl in this case_INIT_TLS_IMPL(tls);}#endifit will error anyway, i dont think there is a need for a nicer error
@ -0,0 +1,335 @@/*oops - comment is wrong
@ -0,0 +42,4 @@}")# Create a temporary file for the sourceHaha, fair enough. In that case, do you need to ensure that line 79 is properly quoting the paths?
@ -0,0 +1,96 @@/**@ -0,0 +42,4 @@}")# Create a temporary file for the sourceok, on reflection, i do feel like it!
This has broken the
picotooltest build of pico-examples using Clang (https://github.com/raspberrypi/picotool/actions/runs/25926790767/job/76210082546?pr=314) with the error:when building the
tinyusb_dev_audio_4_channel_micexample. This is using: