[libc-commits] [PATCH] D75380: [libc] Add linux implementations of thrd_create and thrd_join functions.

[Alex Brachet via Phabricator via libc-commits]

abrachet added inline comments.


================
Comment at: libc/src/threads/linux/thrd_create.cpp:27
+
+static void start_thread(thrd_t *thread, thrd_start_t func, void *arg) {
+  __llvm_libc::syscall(SYS_exit, thread->__retval = func(arg));
----------------
Make this `[[noreturn]]`


================
Comment at: libc/src/threads/linux/thrd_create.cpp:65
+  long clone_result = __llvm_libc::syscall(
+      SYS_clone, clone_flags, (long)stack + DEFAULT_STACK_SIZE - 1,
+      &thread->__tid, clear_tid_address, 0);
----------------
Maybe `(u)intptr_t` is a more descriptive type to use than `long`?


================
Comment at: libc/src/threads/linux/thrd_create.cpp:68
+
+  if (clone_result == 0) {
+    start_thread(thread, func, arg);
----------------
unnecessary brackets


================
Comment at: libc/src/threads/linux/thrd_join.cpp:21
+
+extern FutexData ctid;
+
----------------
I don't think this is used anywhere.


================
Comment at: libc/src/threads/linux/thrd_join.cpp:27
+
+  while (true) {
+    // We cannot do a FUTEX_WAIT_PRIVATE here as the kernel does a
----------------
Is it safe to do `while (atomic_load(clear_tid_address))`?


================
Comment at: libc/src/threads/linux/thread_utils.h:16-18
+#define DEFAULT_STACK_SIZE (1 << 15) // 32 KB
+
+#define CLEAR_TID_VALUE 0xABCD1234
----------------
These can be `constexpr`


================
Comment at: libc/test/src/threads/linux/thrd_test.cpp:17
+static int counter = 0;
+static int thread_func(void *arg) {
+  ++counter;
----------------
You can remove `arg` and just have `thread_func(void *)`to avoid any unused variable warnings.


================
Comment at: libc/test/src/threads/linux/thrd_test.cpp:23
+TEST(ThreadTest, CreateAndJoin) {
+  while (true) {
+    thrd_t thread;
----------------
`for (counter = 0; counter <= 1000; ) {`
Even if you would rather keep the current loop, I think it is important to set counter to 0 in case we ever change the tester to run tests in a different order or run them multiple times whatever the case may be.


================
Comment at: libc/test/src/threads/linux/thrd_test.cpp:46
+    args[i] = i;
+    ASSERT_EQ(__llvm_libc::thrd_create(&thread_list[i], return_arg, args + i),
+              (int)thrd_success);
----------------
Make this `thread_list + i` or make `args + i` `&args[i]`.


================
Comment at: libc/test/src/threads/linux/thrd_test.cpp:51
+  for (int i = 0; i < thread_count; ++i) {
+    int retval = 123; // Start with a retval we dont expect.
+    ASSERT_EQ(__llvm_libc::thrd_join(&thread_list[i], &retval),
----------------
Might as well choose a value greater than `thread_count`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75380