[libc-commits] [libc] [libc] Reset thread-specific key value before invoking destructor (PR #226134)

[via libc-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-libc

Author: Pavel Labath (labath)

<details>
<summary>Changes</summary>

POSIX specifies that before calling a thread-specific key destructor, the value of the key must be set to NULL (so pthread_getspecific returns NULL from inside the destructor), and that the destructor pass must repeat up to PTHREAD_DESTRUCTOR_ITERATIONS times if destructors re-associate non-NULL values with keys.

This patch sets the key payload to nullptr prior to calling the destructor, and loops over the key array up to the destructor iteration limit as long as non-NULL values remain.

Along with that:
- introduce common-threads-macros.h to share the destructor iteration count (4) between C (TSS_DTOR_ITERATIONS) and POSIX (PTHREAD_DESTRUCTOR_ITERATIONS), and use it internally
- expose TSS_DTOR_ITERATIONS in threads.h
- expose and implement _SC_THREAD_DESTRUCTOR_ITERATIONS in sysconf

Note that I am not fixing the issue where a deleted key's destructor can still run on thread exit if it was already cached in thread-local storage. Fixing that properly will require tracking key generations or validity across threads, so I'm leaving that for separate work (#<!-- -->226122).

Assisted-by: Gemini

---
Full diff: https://github.com/llvm/llvm-project/pull/226134.diff


16 Files Affected:

- (modified) libc/include/CMakeLists.txt (+1) 
- (modified) libc/include/llvm-libc-macros/CMakeLists.txt (+16) 
- (added) libc/include/llvm-libc-macros/common-threads-macros.h (+19) 
- (modified) libc/include/llvm-libc-macros/limits-macros.h (+3-1) 
- (modified) libc/include/llvm-libc-macros/linux/unistd-macros.h (+1) 
- (added) libc/include/llvm-libc-macros/threads-macros.h (+22) 
- (modified) libc/include/threads.yaml (+2) 
- (modified) libc/include/unistd.yaml (+2) 
- (modified) libc/src/__support/threads/CMakeLists.txt (+1) 
- (modified) libc/src/__support/threads/thread.cpp (+15-5) 
- (modified) libc/src/unistd/linux/CMakeLists.txt (+1) 
- (modified) libc/src/unistd/linux/sysconf.cpp (+3) 
- (modified) libc/test/integration/src/pthread/pthread_tss_test.cpp (+50) 
- (modified) libc/test/integration/src/threads/tss_test.cpp (+1) 
- (modified) libc/test/src/unistd/CMakeLists.txt (+1) 
- (modified) libc/test/src/unistd/sysconf_test.cpp (+3) 


``````````diff
diff --git a/libc/include/CMakeLists.txt b/libc/include/CMakeLists.txt
index f63e7e3dc6a734..ec258e40a8c9e3 100644
--- a/libc/include/CMakeLists.txt
+++ b/libc/include/CMakeLists.txt
@@ -438,6 +438,7 @@ add_header_macro(
   threads.h
   DEPENDS
     .llvm_libc_common_h
+    .llvm-libc-macros.threads_macros
     .llvm-libc-types.__call_once_func_t
     .llvm-libc-types.once_flag
     .llvm-libc-types.cnd_t
diff --git a/libc/include/llvm-libc-macros/CMakeLists.txt b/libc/include/llvm-libc-macros/CMakeLists.txt
index 9e9559056dae42..10895341f24025 100644
--- a/libc/include/llvm-libc-macros/CMakeLists.txt
+++ b/libc/include/llvm-libc-macros/CMakeLists.txt
@@ -153,10 +153,26 @@ add_macro_header(
     float16-macros.h
 )
 
+add_macro_header(
+  common_threads_macros
+  HDR
+    common-threads-macros.h
+)
+
+add_macro_header(
+  threads_macros
+  HDR
+    threads-macros.h
+  DEPENDS
+    .common_threads_macros
+)
+
 add_macro_header(
   limits_macros
   HDR
     limits-macros.h
+  DEPENDS
+    .common_threads_macros
 )
 
 add_macro_header(
diff --git a/libc/include/llvm-libc-macros/common-threads-macros.h b/libc/include/llvm-libc-macros/common-threads-macros.h
new file mode 100644
index 00000000000000..04d86c1438c4af
--- /dev/null
+++ b/libc/include/llvm-libc-macros/common-threads-macros.h
@@ -0,0 +1,19 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// Definition of common (C and POSIX) thread macros.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_MACROS_COMMON_THREADS_MACROS_H
+#define LLVM_LIBC_MACROS_COMMON_THREADS_MACROS_H
+
+#define __LLVM_LIBC_TSS_DTOR_ITERATIONS 4
+
+#endif // LLVM_LIBC_MACROS_COMMON_THREADS_MACROS_H
diff --git a/libc/include/llvm-libc-macros/limits-macros.h b/libc/include/llvm-libc-macros/limits-macros.h
index 8a341e45e7c3b4..64f1b507ab3861 100644
--- a/libc/include/llvm-libc-macros/limits-macros.h
+++ b/libc/include/llvm-libc-macros/limits-macros.h
@@ -14,6 +14,8 @@
 #ifndef LLVM_LIBC_MACROS_LIMITS_MACROS_H
 #define LLVM_LIBC_MACROS_LIMITS_MACROS_H
 
+#include "common-threads-macros.h"
+
 // Define all C23 macro constants of limits.h
 
 #ifndef CHAR_BIT
@@ -249,7 +251,7 @@
 #endif
 
 #ifndef PTHREAD_DESTRUCTOR_ITERATIONS
-#define PTHREAD_DESTRUCTOR_ITERATIONS _POSIX_THREAD_DESTRUCTOR_ITERATIONS
+#define PTHREAD_DESTRUCTOR_ITERATIONS __LLVM_LIBC_TSS_DTOR_ITERATIONS
 #endif
 
 #ifndef _POSIX_HOST_NAME_MAX
diff --git a/libc/include/llvm-libc-macros/linux/unistd-macros.h b/libc/include/llvm-libc-macros/linux/unistd-macros.h
index 9a0b8209d1705e..0a9facd82fcb22 100644
--- a/libc/include/llvm-libc-macros/linux/unistd-macros.h
+++ b/libc/include/llvm-libc-macros/linux/unistd-macros.h
@@ -30,6 +30,7 @@
 #define _SC_THREAD_SAFE_FUNCTIONS 68
 #define _SC_GETGR_R_SIZE_MAX 69
 #define _SC_GETPW_R_SIZE_MAX 70
+#define _SC_THREAD_DESTRUCTOR_ITERATIONS 73
 #define _SC_THREAD_ATTR_STACKADDR 77
 #define _SC_THREAD_ATTR_STACKSIZE 78
 #define _SC_THREAD_PRIORITY_SCHEDULING 79
diff --git a/libc/include/llvm-libc-macros/threads-macros.h b/libc/include/llvm-libc-macros/threads-macros.h
new file mode 100644
index 00000000000000..edc042a6cfbce7
--- /dev/null
+++ b/libc/include/llvm-libc-macros/threads-macros.h
@@ -0,0 +1,22 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// Definition of macros from threads.h.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_MACROS_THREADS_MACROS_H
+#define LLVM_LIBC_MACROS_THREADS_MACROS_H
+
+#include "common-threads-macros.h"
+
+/// Maximum number of destructor iterations for thread-specific storage.
+#define TSS_DTOR_ITERATIONS __LLVM_LIBC_TSS_DTOR_ITERATIONS
+
+#endif // LLVM_LIBC_MACROS_THREADS_MACROS_H
diff --git a/libc/include/threads.yaml b/libc/include/threads.yaml
index 1e8e42c80587cc..6b41524e587c2e 100644
--- a/libc/include/threads.yaml
+++ b/libc/include/threads.yaml
@@ -4,6 +4,8 @@ standards:
 macros:
   - macro_name: ONCE_FLAG_INIT
     macro_value: '{0}'
+  - macro_name: TSS_DTOR_ITERATIONS
+    macro_header: threads-macros.h
 types:
   - type_name: once_flag
   - type_name: __call_once_func_t
diff --git a/libc/include/unistd.yaml b/libc/include/unistd.yaml
index f8b5bbe12c5828..46d02001dc6a2e 100644
--- a/libc/include/unistd.yaml
+++ b/libc/include/unistd.yaml
@@ -34,6 +34,8 @@ macros:
     macro_header: unistd-macros.h
   - macro_name: _SC_GETPW_R_SIZE_MAX
     macro_header: unistd-macros.h
+  - macro_name: _SC_THREAD_DESTRUCTOR_ITERATIONS
+    macro_header: unistd-macros.h
   - macro_name: _SC_THREAD_ATTR_STACKADDR
     macro_header: unistd-macros.h
   - macro_name: _SC_THREAD_ATTR_STACKSIZE
diff --git a/libc/src/__support/threads/CMakeLists.txt b/libc/src/__support/threads/CMakeLists.txt
index 57f11a849d3c11..4439cb811e7a77 100644
--- a/libc/src/__support/threads/CMakeLists.txt
+++ b/libc/src/__support/threads/CMakeLists.txt
@@ -167,6 +167,7 @@ if(TARGET libc.src.__support.threads.${LIBC_TARGET_OS}.thread)
       .cleanup_stack
       .mutex
       .${LIBC_TARGET_OS}.thread
+      libc.hdr.limits_macros
       libc.hdr.types.struct___pthread_cleanup_frame
       libc.src.__support.fixedvector
       libc.src.__support.CPP.array
diff --git a/libc/src/__support/threads/thread.cpp b/libc/src/__support/threads/thread.cpp
index bd090449dbd292..b03903be47a466 100644
--- a/libc/src/__support/threads/thread.cpp
+++ b/libc/src/__support/threads/thread.cpp
@@ -8,6 +8,7 @@
 
 #include "src/__support/threads/thread.h"
 #include "hdr/types/struct___pthread_cleanup_frame.h"
+#include "hdr/limits_macros.h"
 #include "src/__support/CPP/array.h"
 #include "src/__support/CPP/mutex.h" // lock_guard
 #include "src/__support/CPP/optional.h"
@@ -161,11 +162,20 @@ void call_atexit_callbacks() {
   atexit_callback_mgr.call();
 
   // Thread-specific keys (pthread_key_create).
-  for (size_t i = 0; i < TSS_KEY_COUNT; ++i) {
-    TSSValueUnit &unit = tss_values[i];
-    // Both dtor and value need to nonnull to call dtor
-    if (unit.dtor != nullptr && unit.payload != nullptr)
-      unit.dtor(unit.payload);
+  for (size_t iter = 0; iter < PTHREAD_DESTRUCTOR_ITERATIONS; ++iter) {
+    bool called_dtor = false;
+    for (size_t i = 0; i < TSS_KEY_COUNT; ++i) {
+      TSSValueUnit &unit = tss_values[i];
+      // Both dtor and value need to be nonnull to call dtor
+      if (unit.dtor != nullptr && unit.payload != nullptr) {
+        void *val = unit.payload;
+        unit.payload = nullptr;
+        unit.dtor(val);
+        called_dtor = true;
+      }
+    }
+    if (!called_dtor)
+      break;
   }
 }
 
diff --git a/libc/src/unistd/linux/CMakeLists.txt b/libc/src/unistd/linux/CMakeLists.txt
index 56ede3d837749a..33ce14751979dd 100644
--- a/libc/src/unistd/linux/CMakeLists.txt
+++ b/libc/src/unistd/linux/CMakeLists.txt
@@ -786,6 +786,7 @@ add_entrypoint_object(
   HDRS
     ../sysconf.h
   DEPENDS
+    libc.hdr.limits_macros
     libc.hdr.unistd_macros
     libc.hdr.sys_auxv_macros
     libc.hdr.sys_resource_macros
diff --git a/libc/src/unistd/linux/sysconf.cpp b/libc/src/unistd/linux/sysconf.cpp
index 34c9973da25101..374f4ec481ab02 100644
--- a/libc/src/unistd/linux/sysconf.cpp
+++ b/libc/src/unistd/linux/sysconf.cpp
@@ -15,6 +15,7 @@
 
 #include "src/__support/common.h"
 
+#include "hdr/limits_macros.h"
 #include "hdr/sys_auxv_macros.h"
 #include "hdr/sys_resource_macros.h"
 #include "hdr/types/struct_rlimit.h"
@@ -149,6 +150,8 @@ LLVM_LIBC_FUNCTION(long, sysconf, (int name)) {
     return _POSIX_THREAD_SAFE_FUNCTIONS;
   case _SC_THREAD_SPORADIC_SERVER:
     return _POSIX_THREAD_SPORADIC_SERVER;
+  case _SC_THREAD_DESTRUCTOR_ITERATIONS:
+    return PTHREAD_DESTRUCTOR_ITERATIONS;
   case _SC_GETGR_R_SIZE_MAX:
     // No recommended buffer size for getgrgid_r/getgrnam_r, as they work
     // with any user-supplied buffer.
diff --git a/libc/test/integration/src/pthread/pthread_tss_test.cpp b/libc/test/integration/src/pthread/pthread_tss_test.cpp
index c90525a0000ee1..86b615f626c36d 100644
--- a/libc/test/integration/src/pthread/pthread_tss_test.cpp
+++ b/libc/test/integration/src/pthread/pthread_tss_test.cpp
@@ -78,8 +78,58 @@ static void null_value_test() {
   ASSERT_EQ(LIBC_NAMESPACE::pthread_key_delete(key), 0);
 }
 
+static void *dtor_getspecific_val;
+static void dtor_verify_getspecific_nullptr(void *) {
+  dtor_getspecific_val = LIBC_NAMESPACE::pthread_getspecific(key);
+}
+
+static void *func_getspecific_nullptr(void *) {
+  ASSERT_EQ(LIBC_NAMESPACE::pthread_setspecific(key, &child_thread_data), 0);
+  return nullptr;
+}
+
+static void getspecific_nullptr_in_dtor_test() {
+  pthread_t th;
+  dtor_getspecific_val = &dtor_getspecific_val;
+  ASSERT_EQ(
+      LIBC_NAMESPACE::pthread_key_create(&key, &dtor_verify_getspecific_nullptr),
+      0);
+  ASSERT_EQ(
+      LIBC_NAMESPACE::pthread_create(&th, nullptr, &func_getspecific_nullptr,
+                                     nullptr),
+      0);
+  ASSERT_EQ(LIBC_NAMESPACE::pthread_join(th, nullptr), 0);
+  ASSERT_EQ(dtor_getspecific_val, nullptr);
+  ASSERT_EQ(LIBC_NAMESPACE::pthread_key_delete(key), 0);
+}
+
+static int dtor_call_count = 0;
+static void dtor_multiple(void *data) {
+  ++dtor_call_count;
+  if (dtor_call_count < 2)
+    ASSERT_EQ(LIBC_NAMESPACE::pthread_setspecific(key, data), 0);
+}
+
+static void *func_multiple(void *) {
+  ASSERT_EQ(LIBC_NAMESPACE::pthread_setspecific(key, &child_thread_data), 0);
+  return nullptr;
+}
+
+static void multiple_dtor_test() {
+  pthread_t th;
+  dtor_call_count = 0;
+  ASSERT_EQ(LIBC_NAMESPACE::pthread_key_create(&key, &dtor_multiple), 0);
+  ASSERT_EQ(
+      LIBC_NAMESPACE::pthread_create(&th, nullptr, &func_multiple, nullptr), 0);
+  ASSERT_EQ(LIBC_NAMESPACE::pthread_join(th, nullptr), 0);
+  ASSERT_EQ(dtor_call_count, 2);
+  ASSERT_EQ(LIBC_NAMESPACE::pthread_key_delete(key), 0);
+}
+
 TEST_MAIN() {
   standard_usage_test();
   null_value_test();
+  getspecific_nullptr_in_dtor_test();
+  multiple_dtor_test();
   return 0;
 }
diff --git a/libc/test/integration/src/threads/tss_test.cpp b/libc/test/integration/src/threads/tss_test.cpp
index c1c91c810bdf2d..12d8f479702470 100644
--- a/libc/test/integration/src/threads/tss_test.cpp
+++ b/libc/test/integration/src/threads/tss_test.cpp
@@ -41,6 +41,7 @@ int func(void *obj) {
 }
 
 TEST_MAIN() {
+  ASSERT_EQ(TSS_DTOR_ITERATIONS, 4);
   ASSERT_EQ(LIBC_NAMESPACE::tss_create(&key, &dtor), thrd_success);
   ASSERT_EQ(LIBC_NAMESPACE::tss_set(key, &main_thread_data), thrd_success);
   int *d = reinterpret_cast<int *>(LIBC_NAMESPACE::tss_get(key));
diff --git a/libc/test/src/unistd/CMakeLists.txt b/libc/test/src/unistd/CMakeLists.txt
index a5baa13a18bc67..8f7fbc5607cb26 100644
--- a/libc/test/src/unistd/CMakeLists.txt
+++ b/libc/test/src/unistd/CMakeLists.txt
@@ -852,6 +852,7 @@ add_libc_test(
     sysconf_test.cpp
   DEPENDS
     libc.hdr.errno_macros
+    libc.hdr.limits_macros
     libc.hdr.unistd_macros
     libc.src.unistd.sysconf
     libc.test.UnitTest.ErrnoCheckingTest
diff --git a/libc/test/src/unistd/sysconf_test.cpp b/libc/test/src/unistd/sysconf_test.cpp
index fd596e5884afa9..929a23214a7115 100644
--- a/libc/test/src/unistd/sysconf_test.cpp
+++ b/libc/test/src/unistd/sysconf_test.cpp
@@ -12,6 +12,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "hdr/errno_macros.h"
+#include "hdr/limits_macros.h"
 #include "hdr/unistd_macros.h"
 #include "src/unistd/sysconf.h"
 #include "test/UnitTest/ErrnoCheckingTest.h"
@@ -52,6 +53,8 @@ TEST_F(LlvmLibcSysconfTest, PosixThreadsConstantsTest) {
   EXPECT_EQ(LIBC_NAMESPACE::sysconf(_SC_THREAD_ROBUST_PRIO_PROTECT), -1L);
   EXPECT_EQ(LIBC_NAMESPACE::sysconf(_SC_THREAD_SAFE_FUNCTIONS), 202405L);
   EXPECT_EQ(LIBC_NAMESPACE::sysconf(_SC_THREAD_SPORADIC_SERVER), -1L);
+  EXPECT_EQ(LIBC_NAMESPACE::sysconf(_SC_THREAD_DESTRUCTOR_ITERATIONS),
+            static_cast<long>(PTHREAD_DESTRUCTOR_ITERATIONS));
 }
 
 TEST_F(LlvmLibcSysconfTest, ArgMaxTest) {

``````````

</details>


https://github.com/llvm/llvm-project/pull/226134