[libc-commits] [PATCH] D148291: [LIBC] Fix incorrect behavior with pthread_key_t when value was nullptr

[Noah Goldstein via Phabricator via libc-commits]

goldstein.w.n updated this revision to Diff 514063.
goldstein.w.n added a comment.

Remove 'static' qualifier. Add comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148291/new/

https://reviews.llvm.org/D148291

Files:
  libc/src/__support/threads/thread.cpp
  libc/test/integration/src/pthread/pthread_tss_test.cpp


Index: libc/test/integration/src/pthread/pthread_tss_test.cpp
===================================================================
--- libc/test/integration/src/pthread/pthread_tss_test.cpp
+++ libc/test/integration/src/pthread/pthread_tss_test.cpp
@@ -21,16 +21,19 @@
 static constexpr int THREAD_DATA_FINIVAL = 0x4321;
 static constexpr int THREAD_RUN_VAL = 0x600D;
 
-int child_thread_data = THREAD_DATA_INITVAL;
-int main_thread_data = THREAD_DATA_INITVAL;
+static int child_thread_data = THREAD_DATA_INITVAL;
+static int main_thread_data = THREAD_DATA_INITVAL;
 
-pthread_key_t key;
-void dtor(void *data) {
+static pthread_key_t key;
+static void dtor(void *data) {
   auto *v = reinterpret_cast<int *>(data);
   *v = THREAD_DATA_FINIVAL;
 }
 
-void *func(void *obj) {
+// Used to test that we don't call the destructor when the mapped value in NULL.
+static void dtor_failure(void *) { ASSERT_TRUE(false); }
+
+static void *func(void *obj) {
   ASSERT_EQ(__llvm_libc::pthread_setspecific(key, &child_thread_data), 0);
   int *d = reinterpret_cast<int *>(__llvm_libc::pthread_getspecific(key));
   ASSERT_TRUE(d != nullptr);
@@ -40,7 +43,14 @@
   return nullptr;
 }
 
-TEST_MAIN() {
+static void *func_null_val(void *) {
+  // null value, we should not call dtor
+  ASSERT_EQ(__llvm_libc::pthread_setspecific(key, nullptr), 0);
+  ASSERT_EQ(__llvm_libc::pthread_getspecific(key), nullptr);
+  return nullptr;
+}
+
+static void standard_usage_test() {
   ASSERT_EQ(__llvm_libc::pthread_key_create(&key, &dtor), 0);
   ASSERT_EQ(__llvm_libc::pthread_setspecific(key, &main_thread_data), 0);
   int *d = reinterpret_cast<int *>(__llvm_libc::pthread_getspecific(key));
@@ -56,7 +66,20 @@
   ASSERT_EQ(retval, nullptr);
   ASSERT_EQ(arg, THREAD_RUN_VAL);
   ASSERT_EQ(child_thread_data, THREAD_DATA_FINIVAL);
+  ASSERT_EQ(__llvm_libc::pthread_key_delete(key), 0);
+}
 
+static void null_value_test() {
+  pthread_t th;
+  ASSERT_EQ(__llvm_libc::pthread_key_create(&key, &dtor_failure), 0);
+  ASSERT_EQ(__llvm_libc::pthread_create(&th, nullptr, &func_null_val, nullptr),
+            0);
+  ASSERT_EQ(__llvm_libc::pthread_join(th, nullptr), 0);
   ASSERT_EQ(__llvm_libc::pthread_key_delete(key), 0);
+}
+
+TEST_MAIN() {
+  standard_usage_test();
+  null_value_test();
   return 0;
 }
Index: libc/src/__support/threads/thread.cpp
===================================================================
--- libc/src/__support/threads/thread.cpp
+++ libc/src/__support/threads/thread.cpp
@@ -151,7 +151,8 @@
   attrib->atexit_callback_mgr->call();
   for (size_t i = 0; i < TSS_KEY_COUNT; ++i) {
     TSSValueUnit &unit = tss_values[i];
-    if (unit.dtor != nullptr)
+    // Both dtor and value need to nonnull to call dtor
+    if (unit.dtor != nullptr && unit.payload != nullptr)
       unit.dtor(unit.payload);
   }
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D148291.514063.patch
Type: text/x-patch
Size: 2816 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libc-commits/attachments/20230417/cd53add3/attachment.bin>