[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/thread.h

int run(ThreadRunnerStdc *func, void *arg, void *stack, size_t size,
     return run(ThreadStyle::STDC, runner, arg, stack, size, 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 stack_size, bool detached);

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

DEPENDS
     libc.src.pthread.pthread_mutex_destroy
     libc.src.pthread.pthread_mutex_init
     libc.src.pthread.pthread_mutex_lock
     libc.src.pthread.pthread_mutex_unlock
     libc.src.pthread.pthread_create
     libc.src.pthread.pthread_join
     libc.src.__support.CPP.atomic
 )
+
+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/test/integration/src/pthread/pthread_join_test.cpp

+//===-- 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_create.h"
+#include "src/pthread/pthread_join.h"
+
+#include "src/errno/libc_errno.h"
+
+#include "test/IntegrationTest/test.h"
+#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);
+  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();
+  return 0;
+}