pthread: Remove pthread TLS cleanup dependency on FreeRTOS Static Task Cleanup Hook

This commit removes the need to define the vTaskCleanupTCB hook in
pthread to cleanup the thread-specific data before a thread is deleted.

components/freertos/Kconfig

@@ -293,7 +293,7 @@ menu "FreeRTOS"
 
         config FREERTOS_TLSP_DELETION_CALLBACKS
             bool "Enable thread local storage pointers deletion callbacks"
-            depends on FREERTOS_SMP && (FREERTOS_THREAD_LOCAL_STORAGE_POINTERS > 0)
+            depends on (FREERTOS_THREAD_LOCAL_STORAGE_POINTERS > 0)
             default y
             help
                 ESP-IDF provides users with the ability to free TLSP memory by registering TLSP deletion callbacks.

components/freertos/esp_additions/include/freertos/FreeRTOSConfig.h

@@ -266,7 +266,7 @@ Note: Include trace macros here and not above as trace macros are dependent on s
 
 // ---------------------- Features -------------------------
 
-#define configTHREAD_LOCAL_STORAGE_DELETE_CALLBACKS     1
+#define configTHREAD_LOCAL_STORAGE_DELETE_CALLBACKS     CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS
 #ifndef configIDLE_TASK_STACK_SIZE
 #define configIDLE_TASK_STACK_SIZE                      CONFIG_FREERTOS_IDLE_TASK_STACKSIZE
 #endif

components/pthread/CMakeLists.txt

@@ -13,10 +13,6 @@ list(APPEND extra_link_flags "-u pthread_include_pthread_cond_impl")
 list(APPEND extra_link_flags "-u pthread_include_pthread_local_storage_impl")
 list(APPEND extra_link_flags "-u pthread_include_pthread_rwlock_impl")
 
-if(CONFIG_FREERTOS_ENABLE_STATIC_TASK_CLEAN_UP AND NOT CONFIG_FREERTOS_SMP) # See IDF-4955
-    target_link_libraries(${COMPONENT_LIB} INTERFACE "-Wl,--wrap=vPortCleanUpTCB")
-endif()
-
 if(extra_link_flags)
     target_link_libraries(${COMPONENT_LIB} INTERFACE "${extra_link_flags}")
 endif()

components/pthread/pthread.c

@@ -365,6 +365,8 @@ int pthread_join(pthread_t thread, void **retval)
             pthread_delete(pthread);
             xSemaphoreGive(s_threads_mux);
         }
+        /* clean up thread local storage before task deletion */
+        pthread_internal_local_storage_destructor_callback(handle);
         vTaskDelete(handle);
     }
 
@@ -399,6 +401,8 @@ int pthread_detach(pthread_t thread)
     } else {
         // pthread already stopped
         pthread_delete(pthread);
+        /* clean up thread local storage before task deletion */
+        pthread_internal_local_storage_destructor_callback(handle);
         vTaskDelete(handle);
     }
     xSemaphoreGive(s_threads_mux);
@@ -409,9 +413,8 @@ int pthread_detach(pthread_t thread)
 void pthread_exit(void *value_ptr)
 {
     bool detached = false;
-    /* preemptively clean up thread local storage, rather than
+    /* clean up thread local storage before task deletion */
-       waiting for the idle task to clean up the thread */
+    pthread_internal_local_storage_destructor_callback(NULL);
-    pthread_internal_local_storage_destructor_callback();
 
     if (xSemaphoreTake(s_threads_mux, portMAX_DELAY) != pdTRUE) {
         assert(false && "Failed to lock threads list!");

components/pthread/pthread_internal.h

@@ -1,18 +1,10 @@
-// Copyright 2017 Espressif Systems (Shanghai) PTE LTD
+/*
-//
+ * SPDX-FileCopyrightText: 2017-2022 Espressif Systems (Shanghai) CO LTD
-// Licensed under the Apache License, Version 2.0 (the "License");
+ *
-// you may not use this file except in compliance with the License.
+ * SPDX-License-Identifier: Apache-2.0
-// You may obtain a copy of the License at
+ */
-
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
 #pragma once
 
-void pthread_internal_local_storage_destructor_callback(void);
+void pthread_internal_local_storage_destructor_callback(TaskHandle_t handle);
 
 extern portMUX_TYPE pthread_lazy_init_lock;

components/pthread/pthread_local_storage.c

@@ -15,6 +15,11 @@
 
 #include "pthread_internal.h"
 
+/* Sanity check to ensure that the number of FreeRTOS TLSPs is at least 1 */
+#if (CONFIG_FREERTOS_THREAD_LOCAL_STORAGE_POINTERS < 1)
+    #error "CONFIG_FREERTOS_THREAD_LOCAL_STORAGE_POINTERS cannot be 0 for pthread TLS"
+#endif
+
 #define PTHREAD_TLS_INDEX 0
 
 typedef void (*pthread_destructor_t)(void*);
@@ -117,7 +122,7 @@ int pthread_key_delete(pthread_key_t key)
    - The destructor is always called in the context of the thread itself - which is important if the task then calls
      pthread_getspecific() or pthread_setspecific() to update the state further, as allowed for in the spec.
 */
-static void pthread_local_storage_thread_deleted_callback(int index, void *v_tls)
+static void pthread_cleanup_thread_specific_data_callback(int index, void *v_tls)
 {
     values_list_t *tls = (values_list_t *)v_tls;
     assert(tls != NULL);
@@ -142,46 +147,23 @@ static void pthread_local_storage_thread_deleted_callback(int index, void *v_tls
     free(tls);
 }
 
-#if CONFIG_FREERTOS_ENABLE_STATIC_TASK_CLEAN_UP && !CONFIG_FREERTOS_SMP // IDF-4955
-/* Called from FreeRTOS task delete hook */
-void pthread_local_storage_cleanup(TaskHandle_t task)
-{
-    void *tls = pvTaskGetThreadLocalStoragePointer(task, PTHREAD_TLS_INDEX);
-    if (tls != NULL) {
-        pthread_local_storage_thread_deleted_callback(PTHREAD_TLS_INDEX, tls);
-        vTaskSetThreadLocalStoragePointer(task, PTHREAD_TLS_INDEX, NULL);
-    }
-}
-
-void __real_vPortCleanUpTCB(void *tcb);
-
-/* If static task cleanup hook is defined then its applications responsibility to define `vPortCleanUpTCB`.
-   Here we are wrapping it, so that we can do pthread specific TLS cleanup and then invoke application
-   real specific `vPortCleanUpTCB` */
-void __wrap_vPortCleanUpTCB(void *tcb)
-{
-    pthread_local_storage_cleanup(tcb);
-    __real_vPortCleanUpTCB(tcb);
-}
-#endif  // CONFIG_FREERTOS_ENABLE_STATIC_TASK_CLEAN_UP && !CONFIG_FREERTOS_SMP
-
 /* this function called from pthread_task_func for "early" cleanup of TLS in a pthread */
-void pthread_internal_local_storage_destructor_callback(void)
+void pthread_internal_local_storage_destructor_callback(TaskHandle_t handle)
 {
-    void *tls = pvTaskGetThreadLocalStoragePointer(NULL, PTHREAD_TLS_INDEX);
+    void *tls = pvTaskGetThreadLocalStoragePointer(handle, PTHREAD_TLS_INDEX);
     if (tls != NULL) {
-        pthread_local_storage_thread_deleted_callback(PTHREAD_TLS_INDEX, tls);
+        pthread_cleanup_thread_specific_data_callback(PTHREAD_TLS_INDEX, tls);
         /* remove the thread-local-storage pointer to avoid the idle task cleanup
            calling it again...
         */
-#if CONFIG_FREERTOS_ENABLE_STATIC_TASK_CLEAN_UP && !CONFIG_FREERTOS_SMP // IDF-4955
+#if !defined(CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS)
         vTaskSetThreadLocalStoragePointer(NULL, PTHREAD_TLS_INDEX, NULL);
 #else
         vTaskSetThreadLocalStoragePointerAndDelCallback(NULL,
                                                         PTHREAD_TLS_INDEX,
                                                         NULL,
                                                         NULL);
-#endif // CONFIG_FREERTOS_ENABLE_STATIC_TASK_CLEAN_UP && !CONFIG_FREERTOS_SMP
+#endif /* CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS */
     }
 }
 
@@ -223,14 +205,14 @@ int pthread_setspecific(pthread_key_t key, const void *value)
         if (tls == NULL) {
             return ENOMEM;
         }
-#if defined(CONFIG_FREERTOS_ENABLE_STATIC_TASK_CLEAN_UP)
+#if !defined(CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS)
         vTaskSetThreadLocalStoragePointer(NULL, PTHREAD_TLS_INDEX, tls);
 #else
         vTaskSetThreadLocalStoragePointerAndDelCallback(NULL,
                                                         PTHREAD_TLS_INDEX,
                                                         tls,
-                                                        pthread_local_storage_thread_deleted_callback);
+                                                        pthread_cleanup_thread_specific_data_callback);
-#endif
+#endif /* CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS */
     }
 
     value_entry_t *entry = find_value(tls, key);
@@ -255,7 +237,7 @@ int pthread_setspecific(pthread_key_t key, const void *value)
         // a destructor may call pthread_setspecific() to add a new non-NULL value
         // to the list, and this should be processed after all other entries.
         //
-        // See pthread_local_storage_thread_deleted_callback()
+        // See pthread_cleanup_thread_specific_data_callback()
         value_entry_t *last_entry = NULL;
         value_entry_t *it;
         SLIST_FOREACH(it, tls, next) {

components/pthread/test_apps/pthread_unity_tests/main/test_pthread_local_storage.c

@@ -12,7 +12,7 @@
 #include "test_utils.h"
 #include "esp_random.h"
 
-TEST_CASE("pthread local storage basics", "[pthread]")
+TEST_CASE("pthread local storage basics", "[thread-specific]")
 {
     pthread_key_t key;
     TEST_ASSERT_EQUAL(0, pthread_key_create(&key, NULL));
@@ -35,7 +35,7 @@ TEST_CASE("pthread local storage basics", "[pthread]")
     TEST_ASSERT_EQUAL(0, pthread_key_delete(key));
 }
 
-TEST_CASE("pthread local storage unique keys", "[pthread]")
+TEST_CASE("pthread local storage unique keys", "[thread-specific]")
 {
     const int NUM_KEYS = 10;
     pthread_key_t keys[NUM_KEYS];
@@ -63,7 +63,7 @@ static void *expected_destructor_ptr;
 static void *actual_destructor_ptr;
 static void *thread_test_pthread_destructor(void *);
 
-TEST_CASE("pthread local storage destructor", "[pthread]")
+TEST_CASE("pthread local storage destructor", "[thread-specific]")
 {
     pthread_t thread;
     pthread_key_t key = -1;
@@ -84,9 +84,25 @@ TEST_CASE("pthread local storage destructor", "[pthread]")
     TEST_ASSERT_EQUAL(0, pthread_key_delete(key));
 }
 
+static void *thread_test_pthread_destructor(void *v_key)
+{
+    printf("Local storage thread running...\n");
+    pthread_key_t key = (pthread_key_t) v_key;
+    expected_destructor_ptr = &key; // address of stack variable in the task...
+    pthread_setspecific(key, expected_destructor_ptr);
+    printf("Local storage thread done.\n");
+    return NULL;
+}
+
+static void test_pthread_destructor(void *value)
+{
+    actual_destructor_ptr = value;
+}
+
+#if defined(CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS)
 static void task_test_pthread_destructor(void *v_key);
 
-TEST_CASE("pthread local storage destructor in FreeRTOS task", "[pthread]")
+TEST_CASE("pthread local storage destructor in FreeRTOS task", "[thread-specific]")
 {
     // Same as previous test case, but doesn't use pthread APIs therefore must wait
     // for the idle task to call the destructor
@@ -112,29 +128,13 @@ TEST_CASE("pthread local storage destructor in FreeRTOS task", "[pthread]")
     TEST_ASSERT_EQUAL(0, pthread_key_delete(key));
 }
 
-
-
-static void *thread_test_pthread_destructor(void *v_key)
-{
-    printf("Local storage thread running...\n");
-    pthread_key_t key = (pthread_key_t) v_key;
-    expected_destructor_ptr = &key; // address of stack variable in the task...
-    pthread_setspecific(key, expected_destructor_ptr);
-    printf("Local storage thread done.\n");
-    return NULL;
-}
-
-static void test_pthread_destructor(void *value)
-{
-    actual_destructor_ptr = value;
-}
-
 static void task_test_pthread_destructor(void *v_key)
 {
     /* call the pthread main routine, then delete ourselves... */
     thread_test_pthread_destructor(v_key);
     vTaskDelete(NULL);
 }
+#endif /* CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS */
 
 #define STRESS_NUMITER 2000000
 #define STRESS_NUMTASKS 16
@@ -155,7 +155,7 @@ static void *thread_stress_test(void *v_key)
 
 
 // This test case added to reproduce issues with unpinned tasks and TLS
-TEST_CASE("pthread local storage stress test", "[pthread]")
+TEST_CASE("pthread local storage stress test", "[thread-specific]")
 {
     pthread_key_t key = -1;
     pthread_t threads[STRESS_NUMTASKS] = { 0 };
@@ -187,7 +187,7 @@ static void *s_test_repeat_destructor_thread(void *vp_state);
 // pthread_setspecific() to set another value when it runs, and also
 //
 // As described in https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_key_create.html
-TEST_CASE("pthread local storage 'repeat' destructor test", "[pthread]")
+TEST_CASE("pthread local storage 'repeat' destructor test", "[thread-specific]")
 {
     int r;
     destr_test_state_t state = { .last_idx = -1 };

components/pthread/test_apps/pthread_unity_tests/pytest_pthread_unity_tests.py

@@ -16,7 +16,7 @@ from pytest_embedded import Dut
 )
 def test_pthread(dut: Dut) -> None:
     dut.expect_exact('Press ENTER to see the list of tests')
-    dut.write('*')
+    dut.write('![thread-specific]')
     dut.expect_unity_test_output(timeout=300)
 
 
@@ -31,5 +31,34 @@ def test_pthread(dut: Dut) -> None:
 )
 def test_pthread_single_core(dut: Dut) -> None:
     dut.expect_exact('Press ENTER to see the list of tests')
-    dut.write('*')
+    dut.write('![thread-specific]')
+    dut.expect_unity_test_output(timeout=300)
+
+
+@pytest.mark.generic
+@pytest.mark.supported_targets
+@pytest.mark.parametrize(
+    'config',
+    [
+        'tls',
+    ],
+    indirect=True,
+)
+def test_pthread_tls(dut: Dut) -> None:
+    dut.expect_exact('Press ENTER to see the list of tests')
+    dut.write('[thread-specific]')
+    dut.expect_unity_test_output(timeout=300)
+
+
+@pytest.mark.generic
+@pytest.mark.parametrize(
+    'config',
+    [
+        pytest.param('single_core_esp32_tls', marks=[pytest.mark.esp32]),
+    ],
+    indirect=True,
+)
+def test_pthread_single_core_tls(dut: Dut) -> None:
+    dut.expect_exact('Press ENTER to see the list of tests')
+    dut.write('[thread-specific]')
     dut.expect_unity_test_output(timeout=300)

components/pthread/test_apps/pthread_unity_tests/sdkconfig.ci.single_core_esp32_tls

@@ -0,0 +1,4 @@
+CONFIG_IDF_TARGET="esp32"
+CONFIG_FREERTOS_UNICORE=y
+CONFIG_FREERTOS_THREAD_LOCAL_STORAGE_POINTERS=1
+CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS=y

components/pthread/test_apps/pthread_unity_tests/sdkconfig.ci.tls

@@ -0,0 +1,2 @@
+CONFIG_FREERTOS_THREAD_LOCAL_STORAGE_POINTERS=1
+CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS=y

docs/en/api-reference/system/pthread.rst

@@ -120,7 +120,7 @@ Thread-Specific Data
 * ``pthread_key_delete()``
 * ``pthread_setspecific()`` / ``pthread_getspecific()``
 
-.. note:: These functions can be called from tasks created using either pthread or FreeRTOS APIs
+.. note:: These functions can be called from tasks created using either pthread or FreeRTOS APIs. When calling these functions from tasks created using FreeRTOS APIs, :ref:`CONFIG_FREERTOS_TLSP_DELETION_CALLBACKS` config option must be enabled to ensure the thread-specific data is cleaned up before the task is deleted.
 
 .. note:: There are other options for thread local storage in ESP-IDF, including options with higher performance. See :doc:`/api-guides/thread-local-storage`.
 

tools/ci/check_copyright_ignore.txt

@@ -884,7 +884,6 @@ components/openthread/include/esp_openthread_lock.h
 components/protocomm/include/transports/protocomm_console.h
 components/protocomm/include/transports/protocomm_httpd.h
 components/pthread/pthread_cond_var.c
-components/pthread/pthread_internal.h
 components/pthread/test/test_cxx_cond_var.cpp
 components/pthread/test/test_cxx_std_future.cpp
 components/pthread/test/test_pthread.c