[LIBC] Strengthen `pthread_join` tests; NFC

[goldstein.w.n]

Rebase

[goldstein.w.n]

Rebase

[goldstein.w.n]

Rebase

[goldstein.w.n]

Fix style

[goldstein.w.n]

Fix rand_val for iters

[goldstein.w.n]

Fix join check

[michaelrj]

I like the general idea of having a more thorough test for pthreads as a whole, but it should probably be in a separate file instead of replacing the pthread_join test. I think it would fit better in the pthread_test file.

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

2

nit: fix header

[sivachandra]

Thanks @michaelrj for responding on this review. @goldstein.w.n - the tests you have added are great but couple of reasons for why I was putting this review off and eventually forgot about it: 1. The logic was not straightforward enough that without proper comments, I have to read through line by line to understand what exactly is going on. 2. Use of random values and I wanted to spend time to see what would be good alternates. Thanks to @michaelrj, I have actually read this patch in detail and have couple of comments:

1. The non-null join test is best suited as a fuzz test. See examples here: https://github.com/llvm/llvm-project/tree/main/libc/fuzzing/stdlib
2. For this patch, make an equivalent of the non-null join test which is deterministic (as in it does not use random numbers).

We do not run any of libc's fuzz tests on public infrastructure but we do run them downstream. If you can convert the test into a fuzz test, it might actually become complicated enough that we can make an attempt at running it on OSS-fuzz: https://github.com/google/oss-fuzz

[goldstein.w.n]

In D148837#4397630, @sivachandra wrote:

Thanks @michaelrj for responding on this review. @goldstein.w.n - the tests you have added are great but couple of reasons for why I was putting this review off and eventually forgot about it: 1. The logic was not straightforward enough that without proper comments, I have to read through line by line to understand what exactly is going on. 2. Use of random values and I wanted to spend time to see what would be good alternates. Thanks to @michaelrj, I have actually read this patch in detail and have couple of comments:

Fair enough regarding the comments, can properly document.

1. The non-null join test is best suited as a fuzz test. See examples here: https://github.com/llvm/llvm-project/tree/main/libc/fuzzing/stdlib

Why is that? Its really just checking that the runtime 1) returns the correct value and 2) doesn't return early.
I can drop the detached aspect if thats getting in the way.

I don't really see the fuzz aspect of it. There are no invalid joins (or at least I hope not) and its not really meant
to stress the system in some way to see if breaks down.

2. For this patch, make an equivalent of the non-null join test which is deterministic (as in it does not use random numbers).

The random values are seeded with with srand(123) which should make the test entirely deterministic.

We do not run any of libc's fuzz tests on public infrastructure but we do run them downstream. If you can convert the test into a fuzz test, it might actually become complicated enough that we can make an attempt at running it on OSS-fuzz: https://github.com/google/oss-fuzz

[sivachandra]

In D148837#4397873, @goldstein.w.n wrote:

1. The non-null join test is best suited as a fuzz test. See examples here: https://github.com/llvm/llvm-project/tree/main/libc/fuzzing/stdlib

Why is that? Its really just checking that the runtime 1) returns the correct value and 2) doesn't return early.
I can drop the detached aspect if thats getting in the way.

The random values are seeded with with srand(123) which should make the test entirely deterministic.

I agree it is entirely deterministic. In which case, I would prefer a hand-crafted series without reliance on [s]rand. I am suggesting a fuzz test because you can do two things better: a) The test actually get a real random input. b) The test will get stress tested. But, if you eliminate the use of [s]rand, I will leave it up to you if you want to add a fuzz test at all.

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

add_integration_test(
   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.stdlib.srand
+    libc.src.stdlib.rand
+    libc.src.__support.CPP.atomic
+    libc.src.__support.CPP.new
 )
 
 add_integration_test(
   pthread_create_test
   SUITE
     libc-pthread-integration-tests
   SRCS
     pthread_create_test.cpp

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

-//===-- Tests for pthread_join-- ------------------------------------------===//
+//===-- Tests for pthread_join
+//--------------------------------------------===//
 //
 // 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/stdlib/rand.h"
+#include "src/stdlib/srand.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>
 
-static void *simpleFunc(void *) { return nullptr; }
-static void nullJoinTest() {
-  pthread_t Tid;
-  ASSERT_EQ(__llvm_libc::pthread_create(&Tid, nullptr, simpleFunc, nullptr), 0);
+struct TestThreadArgs {
+  pthread_t tid;
+  int detached;
+  volatile int done;
+  size_t spin_cnt;
+  void *ret;
+};
+
+static __llvm_libc::AllocChecker global_ac;
+
+static void *func(void *arg) {
+  TestThreadArgs *th_arg = reinterpret_cast<TestThreadArgs *>(arg);
+  volatile size_t spin_cnt = th_arg->spin_cnt;
+
+  while (spin_cnt) {
+    --spin_cnt;
+  }
+
+  th_arg->done = 1;
+  return th_arg->ret;
+}
+
+static void *simple_func(void *) { return nullptr; }
+
+static TestThreadArgs *initialize_test_thread_args(size_t num_threads,
+                                                   unsigned detached_percent) {
+  TestThreadArgs *th_args = new (global_ac) TestThreadArgs[num_threads];
+  for (size_t i = 0; i < num_threads; ++i) {
+    unsigned rand_val = __llvm_libc::rand();
+    uintptr_t rand_ret = __llvm_libc::rand();
+    rand_ret *= __llvm_libc::rand();
+    th_args[i].detached = detached_percent > (rand_val % 100);
+    th_args[i].done = 0;
+    th_args[i].spin_cnt = rand_val % (1000 * 1000);
+    th_args[i].ret = reinterpret_cast<void *>(rand_ret);
+  }
+
+  pthread_attr_t attr;
+  ASSERT_EQ(__llvm_libc::pthread_attr_init(&attr), 0);
+  for (size_t i = 0; i < num_threads; ++i) {
+    ASSERT_EQ(__llvm_libc::pthread_attr_setdetachstate(
+                  &attr, th_args[i].detached ? PTHREAD_CREATE_DETACHED
+                                             : PTHREAD_CREATE_JOINABLE),
+              0);
+    ASSERT_EQ(libc_errno, 0);
+
+    ASSERT_EQ(
+        __llvm_libc::pthread_create(&th_args[i].tid, &attr, func,
+                                    reinterpret_cast<void *>(th_args + i)),
+        0);
+    ASSERT_EQ(libc_errno, 0);
+  }
+  return th_args;
+}
+
+static void run_join_tests(size_t num_threads, bool join_as_completed,
+                           unsigned detached_percent) {
+  TestThreadArgs *th_args =
+      initialize_test_thread_args(num_threads, detached_percent);
+  for (size_t join_cnt = 0; join_cnt < num_threads;) {
+    for (size_t i = 0; i < num_threads; ++i) {
+      if (th_args[i].detached) {
+        if (th_args[i].done == 1) {
+          th_args[i].done = 2;
+          ++join_cnt;
+        }
+      } else {
+        if (th_args[i].done == 1 ||
+            (!join_as_completed && th_args[i].done != 2)) {
+          void *th_ret;
+          ASSERT_EQ(__llvm_libc::pthread_join(th_args[i].tid, &th_ret), 0);
           ASSERT_EQ(libc_errno, 0);
-  ASSERT_EQ(__llvm_libc::pthread_join(Tid, nullptr), 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;
-  nullJoinTest();
+  __llvm_libc::srand(123);
+
+  null_join_test();
+
+  for (unsigned detached_percent = 0; detached_percent <= 100;
+       detached_percent += 25) {
+    for (size_t num_threads = 1; num_threads <= 8; ++num_threads) {
+      run_join_tests(num_threads, /*join_as_completed*/ true, detached_percent);
+      run_join_tests(num_threads, /*join_as_completed*/ false,
+                     detached_percent);
+    }
+  }
+
   return 0;
 }