[LIBC] Fix incorrect behavior with pthread_key_t when value was nullptr

[goldstein.w.n]

Summary

We should not call destructor if value is nullptr.

[sivachandra]

OK with inline comments addressed.

libc/src/__support/threads/thread.cpp

90

This is already in an anonymous namespace so static is not required.

libc/test/integration/src/pthread/pthread_tss_test.cpp

33–36

Add a comment explaining why this always failing assert is used here.

[goldstein.w.n]

Remove 'static' qualifier. Add comment.

libc/src/__support/threads/thread.cpp

public:
   }
 
   bool is_valid_key(unsigned int key) {
     MutexLock lock(&mtx);
     return units[key].active;
   }
 };
 
 TSSKeyMgr tss_key_mgr;
 
 struct TSSValueUnit {
   bool active = false;
   void *payload = nullptr;
   TSSDtor *dtor = nullptr;
 
   constexpr TSSValueUnit() = default;
   constexpr TSSValueUnit(void *p, TSSDtor *d)

 ThreadAtExitCallbackMgr *get_thread_atexit_callback_mgr() {
   return &atexit_callback_mgr;
 }
 
 void call_atexit_callbacks(ThreadAttributes *attrib) {
   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);
   }
 }
 
 } // namespace internal
 
 cpp::optional<unsigned int> new_tss_key(TSSDtor *dtor) {
   return tss_key_mgr.new_key(dtor);

libc/test/integration/src/pthread/pthread_tss_test.cpp

 #include "test/IntegrationTest/test.h"
 
 #include <pthread.h>
 
 static constexpr int THREAD_DATA_INITVAL = 0x1234;
 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);
   ASSERT_EQ(&child_thread_data, d);
   ASSERT_EQ(*d, THREAD_DATA_INITVAL);
   *reinterpret_cast<int *>(obj) = THREAD_RUN_VAL;
   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));
   ASSERT_TRUE(d != nullptr);
   ASSERT_EQ(&main_thread_data, d);
   ASSERT_EQ(*d, THREAD_DATA_INITVAL);
 
   pthread_t th;
   int arg = 0xBAD;
   ASSERT_EQ(__llvm_libc::pthread_create(&th, nullptr, &func, &arg), 0);
   void *retval = &child_thread_data; // Init to some non-nullptr val.
   ASSERT_EQ(__llvm_libc::pthread_join(th, &retval), 0);
   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;
 }