[LIBC] Fix incorrect handling of `pthread_join(tid, nullptr)`

[goldstein.w.n]

Summary

Previously unconditionally stored to the return value. This is
incorrect, we should only return if user value is non-null.

[sivachandra]

Can you please separate this patch out into two so that the null return value can be go in separately? I can then also add comments for the other changes in their own focused review.

[goldstein.w.n]

Remove unrelated changes

[goldstein.w.n]

In D148293#4267017, @sivachandra wrote:

Can you please separate this patch out into two so that the null return value can be go in separately? I can then also add comments for the other changes in their own focused review.

Done.

[sivachandra]

Sorry for more work, but this change is required for proper functioning of https://reviews.llvm.org/D148291, which has already landed. So, can you just prepare patch for the nullptr handling and not make it depend on the attributes patch? Or, make the tests in the patch not use the detach functionality. Either way, we should land this patch with some urgency.

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

58

We should not use randomized inputs/data in unit tests because we want them to be reproducible.

[goldstein.w.n]

Remove dep on pthread_create patch
Use reproducible rand

[goldstein.w.n]

In D148293#4282901, @sivachandra wrote:

Sorry for more work, but this change is required for proper functioning of https://reviews.llvm.org/D148291, which has already landed. So, can you just prepare patch for the nullptr handling and not make it depend on the attributes patch? Or, make the tests in the patch not use the detach functionality. Either way, we should land this patch with some urgency.

Oh shoot, forgot about the dependency when I pushed!

Updated v2 so no deps. If any issue I can just revert D148291

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

58

Switched with fixed seed rand. That should be reproducible.

[sivachandra]

I am accepting but have also requested changes to reduce the scope of testing even further. I will review your other patch before the end of this week and we can include all kinds of testing in that patch.

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

30

Type names not specified by the C/C++ standard should be in UpperCase style: https://libc.llvm.org/dev/code_style.html#naming-style

72

Since we do not currently honor attrs, can you move this part to your other patch?

112

Just keep this in this patch.

[goldstein.w.n]

Pair down tests

[goldstein.w.n]

Use llvm style

[goldstein.w.n]

@sivachandra this appears to be causing: https://lab.llvm.org/buildbot/#/builders/257/builds/630
want me to revert this and the key commit? Unable to reproduce locally so not really sure how to debug.

[sivachandra]

It is not because of your change. Accidentally, we have not been running tests on the builders for some time. I fixed it today so it started showing up regressions. I have already fixed that particular failure you have pointed out: https://reviews.llvm.org/D148844.

libc/src/__support/threads/linux/thread.cpp

#endif
     cleanup_thread_resources(attrib);
     return -clone_result;
   }
 
   return 0;
 }
 
 int Thread::join(ThreadReturnValue &retval) {
+  // Its UB either way, but we might as well make it fast :)
+  if (attrib->detach_state.load() == uint32_t(DetachState::DETACHED))
+    return EINVAL;
+
   wait();
 
   if (attrib->style == ThreadStyle::POSIX)
     retval.posix_retval = attrib->retval.posix_retval;
   else
     retval.stdc_retval = attrib->retval.stdc_retval;
 
   cleanup_thread_resources(attrib);
 
   return 0;
 }
 
 int Thread::detach() {
   uint32_t joinable_state = uint32_t(DetachState::JOINABLE);
   if (attrib->detach_state.compare_exchange_strong(
           joinable_state, uint32_t(DetachState::DETACHED))) {
     return int(DetachType::SIMPLE);
   }
 
   // If the thread was already detached, then the detach method should not
   // be called at all. If the thread is exiting, then we wait for it to exit
   // and free up resources.
+  // TODO: Should we handle this less destructively? Maybe just return EINVAL?
   wait();
 
   cleanup_thread_resources(attrib);
 
   return int(DetachType::CLEANUP);
 }
 
 void Thread::wait() {

libc/src/__support/threads/thread.h

return run(ThreadStyle::STDC, runner, arg, stack, stacksize, guardsize,
                detached);
   }
 
   int join(int *val) {
     ThreadReturnValue retval;
     int status = join(retval);
     if (status != 0)
       return status;
+    if (val != nullptr)
       *val = retval.stdc_retval;
     return 0;
   }
 
   int join(void **val) {
     ThreadReturnValue retval;
     int status = join(retval);
     if (status != 0)
       return status;
+    if (val != nullptr)
       *val = retval.posix_retval;
     return 0;
   }
 
   // Platform should implement the functions below.
 
   // Return 0 on success or an error value on failure.
   int run(ThreadStyle style, ThreadRunner runner, void *arg, void *stack,
           size_t stacksize, size_t guardsize, bool detached);

libc/test/integration/src/pthread/CMakeLists.txt

DEPENDS
     libc.src.sys.mman.mmap
     libc.src.sys.mman.munmap
     libc.src.sys.random.getrandom
     libc.src.__support.threads.thread
     libc.src.__support.CPP.atomic
     libc.src.__support.CPP.array
     libc.src.__support.CPP.new
 )
+
+add_integration_test(
+  pthread_join_test
+  SUITE
+    libc-pthread-integration-tests
+  SRCS
+    pthread_join_test.cpp
+  DEPENDS
+    libc.include.pthread
+    libc.include.errno
+    libc.include.stdio
+    libc.src.pthread.pthread_create
+    libc.src.pthread.pthread_join
+    libc.src.pthread.pthread_attr_setdetachstate
+    libc.src.pthread.pthread_attr_init
+    libc.src.pthread.pthread_attr_destroy
+    libc.src.sys.random.getrandom
+    libc.src.__support.CPP.atomic
+    libc.src.__support.CPP.new
+)

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

+//===-- Tests for pthread_create ------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/pthread/pthread_attr_destroy.h"
+#include "src/pthread/pthread_attr_init.h"
+#include "src/pthread/pthread_attr_setdetachstate.h"
+#include "src/pthread/pthread_create.h"
+#include "src/pthread/pthread_join.h"
+
+#include "src/sys/random/getrandom.h"
+
+#include "src/__support/CPP/atomic.h"
+#include "src/__support/CPP/new.h"
+
+#include "src/__support/threads/thread.h"
+#include "src/errno/libc_errno.h"
+
+#include "test/IntegrationTest/test.h"
+
+#include <linux/param.h> // For EXEC_PAGESIZE.
+#include <pthread.h>
+
+struct test_thread_args {
+  pthread_t tid;
+  int detached;
+  volatile int done;
+  size_t spin_cnt;
+  void *ret;
+};
+static __llvm_libc::AllocChecker ac;
+
+static void *func(void *arg) {
+  test_thread_args *th_arg = reinterpret_cast<test_thread_args *>(arg);
+  volatile size_t spin_cnt = th_arg->spin_cnt;
+  void *ret = th_arg->ret;
+
+  while (spin_cnt) {
+    --spin_cnt;
+  }
+
+  th_arg->done = 1;
+  return ret;
+}
+
+static void *simple_func(void *) { return nullptr; }
+
+static test_thread_args *
+initialize_test_thread_args(size_t nthreads, unsigned detached_percent) {
+  test_thread_args *th_args = new (ac) test_thread_args[nthreads];
+  for (size_t i = 0; i < nthreads; ++i) {
+    unsigned rval;
+    ASSERT_EQ(__llvm_libc::getrandom(&rval, sizeof(rval), 0),
+              static_cast<ssize_t>(sizeof(rval)));
+    rval %= 100;
+    th_args[i].detached = detached_percent > rval;
+    th_args[i].done = 0;
+    th_args[i].spin_cnt = rval % (1000 * 1000);
+
+    ASSERT_EQ(__llvm_libc::getrandom(&th_args[i].ret, sizeof(void *), 0),
+              static_cast<ssize_t>(sizeof(void *)));
+  }
+
+  pthread_attr_t attr;
+  ASSERT_EQ(__llvm_libc::pthread_attr_init(&attr), 0);
+  for (size_t i = 0; i < nthreads; ++i) {
+    ASSERT_EQ(__llvm_libc::pthread_attr_setdetachstate(
+                  &attr, th_args[i].detached ? PTHREAD_CREATE_DETACHED
+                                             : PTHREAD_CREATE_JOINABLE),
+              0);
+
+    ASSERT_EQ(
+        __llvm_libc::pthread_create(&th_args[i].tid, &attr, func,
+                                    reinterpret_cast<void *>(th_args + i)),
+        0);
+  }
+  return th_args;
+}
+
+static void run_join_tests(size_t nthreads, bool join_as_completed) {
+  test_thread_args *th_args = initialize_test_thread_args(nthreads, 0);
+  for (size_t join_cnt = 0; join_cnt < nthreads;) {
+    for (size_t i = 0; i < nthreads; ++i) {
+      if (th_args[i].detached) {
+        if (th_args[i].done == 1) {
+          th_args[i].done = 2;
+          ++join_cnt;
+        }
+      } else {
+        if (!join_as_completed || th_args[i].done == 1) {
+          void *th_ret;
+          ASSERT_EQ(__llvm_libc::pthread_join(th_args[i].tid, &th_ret), 0);
+          ASSERT_EQ(libc_errno, 0);
+
+          ASSERT_EQ(th_args[i].done, 1);
+          ASSERT_EQ(th_ret, th_args[i].ret);
+
+          th_args[i].done = 2;
+          ++join_cnt;
+        }
+      }
+    }
+  }
+  delete th_args;
+}
+
+static void null_join_test() {
+  pthread_t tid;
+  ASSERT_EQ(__llvm_libc::pthread_create(&tid, nullptr, simple_func, nullptr),
+            0);
+  ASSERT_EQ(libc_errno, 0);
+  ASSERT_EQ(__llvm_libc::pthread_join(tid, nullptr), 0);
+  ASSERT_EQ(libc_errno, 0);
+}
+
+TEST_MAIN() {
+  libc_errno = 0;
+  null_join_test();
+
+  for (size_t nthreads = 1; nthreads < 8; ++nthreads) {
+    run_join_tests(nthreads, true);
+    run_join_tests(nthreads, false);
+  }
+
+  return 0;
+}