[libc] Implement barriers for pthreads #148948
Open
+515
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-libc Author: Uzair Nawaz (uzairnawaz) ChangesImplemented barrier synchronization for pthreads
Patch is 21.59 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/148948.diff 19 Files Affected:
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index 59c248871f83a..3a631c9056163 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -1039,6 +1039,9 @@ if(LLVM_LIBC_FULL_BUILD)
libc.src.pthread.pthread_join
libc.src.pthread.pthread_key_create
libc.src.pthread.pthread_key_delete
+ libc.src.pthread.pthread_barrier_init
+ libc.src.pthread.pthread_barrier_wait
+ libc.src.pthread.pthread_barrier_destroy
libc.src.pthread.pthread_mutex_destroy
libc.src.pthread.pthread_mutex_init
libc.src.pthread.pthread_mutex_lock
diff --git a/libc/include/CMakeLists.txt b/libc/include/CMakeLists.txt
index 55268d19529c7..3c16468b8110e 100644
--- a/libc/include/CMakeLists.txt
+++ b/libc/include/CMakeLists.txt
@@ -390,6 +390,8 @@ add_header_macro(
.llvm-libc-types.pthread_attr_t
.llvm-libc-types.pthread_condattr_t
.llvm-libc-types.pthread_key_t
+ .llvm-libc-types.pthread_barrier_t
+ .llvm-libc-types.pthread_barrierattr_t
.llvm-libc-types.pthread_mutex_t
.llvm-libc-types.pthread_mutexattr_t
.llvm-libc-types.pthread_once_t
diff --git a/libc/include/llvm-libc-macros/pthread-macros.h b/libc/include/llvm-libc-macros/pthread-macros.h
index fcc6ef925e3f4..ce467b7cc4d07 100644
--- a/libc/include/llvm-libc-macros/pthread-macros.h
+++ b/libc/include/llvm-libc-macros/pthread-macros.h
@@ -22,6 +22,8 @@
#define PTHREAD_MUTEX_STALLED 0
#define PTHREAD_MUTEX_ROBUST 1
+#define PTHREAD_BARRIER_SERIAL_THREAD -1
+
#define PTHREAD_ONCE_INIT {0}
#define PTHREAD_PROCESS_PRIVATE 0
diff --git a/libc/include/llvm-libc-types/CMakeLists.txt b/libc/include/llvm-libc-types/CMakeLists.txt
index b24c97301668a..405c2cd26b863 100644
--- a/libc/include/llvm-libc-types/CMakeLists.txt
+++ b/libc/include/llvm-libc-types/CMakeLists.txt
@@ -53,6 +53,8 @@ add_header(pthread_condattr_t HDR pthread_condattr_t.h DEPENDS .clockid_t)
add_header(pthread_key_t HDR pthread_key_t.h)
add_header(pthread_mutex_t HDR pthread_mutex_t.h DEPENDS .__futex_word .__mutex_type)
add_header(pthread_mutexattr_t HDR pthread_mutexattr_t.h)
+add_header(pthread_barrier_t HDR pthread_barrier_t.h)
+add_header(pthread_barrierattr_t HDR pthread_barrierattr_t.h)
add_header(pthread_once_t HDR pthread_once_t.h DEPENDS .__futex_word)
add_header(pthread_rwlock_t HDR pthread_rwlock_t.h DEPENDS .__futex_word .pid_t)
add_header(pthread_rwlockattr_t HDR pthread_rwlockattr_t.h)
diff --git a/libc/include/llvm-libc-types/pthread_barrier_t.h b/libc/include/llvm-libc-types/pthread_barrier_t.h
new file mode 100644
index 0000000000000..d95477afb226c
--- /dev/null
+++ b/libc/include/llvm-libc-types/pthread_barrier_t.h
@@ -0,0 +1,16 @@
+//===-- Definition of pthread_barrier_t type ------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_TYPES_PTHREAD_BARRIER_T_H
+#define LLVM_LIBC_TYPES_PTHREAD_BARRIER_T_H
+
+typedef struct {
+ char padding[88];
+} pthread_barrier_t;
+
+#endif // LLVM_LIBC_TYPES_PTHREAD_BARRIER_T_H
diff --git a/libc/include/llvm-libc-types/pthread_barrierattr_t.h b/libc/include/llvm-libc-types/pthread_barrierattr_t.h
new file mode 100644
index 0000000000000..064be5bfb6721
--- /dev/null
+++ b/libc/include/llvm-libc-types/pthread_barrierattr_t.h
@@ -0,0 +1,16 @@
+//===-- Definition of pthread_barrierattr_t type --------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_TYPES_PTHREAD_BARRIERATTR_T_H
+#define LLVM_LIBC_TYPES_PTHREAD_BARRIERATTR_T_H
+
+typedef struct {
+ bool pshared;
+} pthread_barrierattr_t;
+
+#endif // LLVM_LIBC_TYPES_PTHREAD_BARRIERATTR_T_H
diff --git a/libc/include/pthread.yaml b/libc/include/pthread.yaml
index 5b27e68d2f2d8..8afce2098adde 100644
--- a/libc/include/pthread.yaml
+++ b/libc/include/pthread.yaml
@@ -6,6 +6,8 @@ types:
- type_name: pthread_once_t
- type_name: pthread_mutex_t
- type_name: pthread_mutexattr_t
+ - type_name: pthread_barrier_t
+ - type_name: pthread_barrierattr_t
- type_name: pthread_key_t
- type_name: pthread_condattr_t
- type_name: __pthread_tss_dtor_t
@@ -277,6 +279,26 @@ functions:
arguments:
- type: pthread_mutexattr_t *__restrict
- type: int
+ - name: pthread_barrier_init
+ standards:
+ - POSIX
+ return_type: int
+ arguments:
+ - type: pthread_barrier_t *__restrict
+ - type: const pthread_barrierattr_t *__restrict
+ - type: int
+ - name: pthread_barrier_wait
+ standards:
+ - POSIX
+ return_type: int
+ arguments:
+ - type: pthread_barrier_t *
+ - name: pthread_barrier_destroy
+ standards:
+ - POSIX
+ return_type: int
+ arguments:
+ - type: pthread_barrier_t *
- name: pthread_once
standards:
- POSIX
diff --git a/libc/src/__support/threads/CMakeLists.txt b/libc/src/__support/threads/CMakeLists.txt
index bd49bbb5ad2fe..3cf50b6d3f8a9 100644
--- a/libc/src/__support/threads/CMakeLists.txt
+++ b/libc/src/__support/threads/CMakeLists.txt
@@ -90,6 +90,17 @@ if(TARGET libc.src.__support.threads.${LIBC_TARGET_OS}.CndVar)
)
endif()
+add_object_library(
+ barrier
+ HDRS
+ barrier.h
+ SRCS
+ barrier.cpp
+ DEPENDS
+ .CndVar
+ .mutex
+)
+
if (LLVM_LIBC_FULL_BUILD)
set(identifier_dependency_on_thread libc.src.__support.threads.thread)
endif()
diff --git a/libc/src/__support/threads/barrier.cpp b/libc/src/__support/threads/barrier.cpp
new file mode 100644
index 0000000000000..298cb17cb8d00
--- /dev/null
+++ b/libc/src/__support/threads/barrier.cpp
@@ -0,0 +1,82 @@
+//===-- Linux implementation of the callonce function ---------------------===//
+//
+// 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/__support/threads/barrier.h"
+#include "barrier.h"
+#include "hdr/errno_macros.h"
+#include "src/__support/threads/mutex.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+const int BARRIER_FIRST_EXITED = -1;
+
+int Barrier::init(Barrier *b, const pthread_barrierattr_t *attr,
+ unsigned count) {
+ LIBC_ASSERT(attr == nullptr); // TODO implement barrierattr
+ if (count == 0)
+ return EINVAL;
+
+ b->expected = count;
+ b->waiting = 0;
+ b->blocking = true;
+
+ int err;
+ err = CndVar::init(&b->entering);
+ if (err != 0)
+ return err;
+
+ err = CndVar::init(&b->exiting);
+ if (err != 0)
+ return err;
+
+ Mutex::init(&b->m, false, false, false, false);
+ return 0;
+}
+
+int Barrier::wait() {
+ m.lock();
+
+ // if the barrier is emptying out threads, wait until it finishes
+ while (!blocking) {
+ entering.wait(&m);
+ }
+ waiting++;
+
+ if (waiting == expected) {
+ // this is the last thread to call wait(), so lets wake everyone up
+ blocking = false;
+ exiting.broadcast();
+ } else {
+ // block threads until waiting = expected
+ while (blocking) {
+ exiting.wait(&m);
+ }
+ }
+ waiting--;
+
+ // all threads have exited the barrier, lets let the ones waiting to enter
+ // continue
+ if (waiting == 0) {
+ blocking = true;
+ entering.broadcast();
+ m.unlock();
+ return BARRIER_FIRST_EXITED;
+ }
+ m.unlock();
+
+ return 0;
+}
+
+int Barrier::destroy(Barrier *b) {
+ CndVar::destroy(&b->entering);
+ CndVar::destroy(&b->exiting);
+ Mutex::destroy(&b->m);
+ return 0;
+}
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/__support/threads/barrier.h b/libc/src/__support/threads/barrier.h
new file mode 100644
index 0000000000000..b8b410ec1b35e
--- /dev/null
+++ b/libc/src/__support/threads/barrier.h
@@ -0,0 +1,44 @@
+//===-- A platform independent abstraction layer for barriers --*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC___SUPPORT_SRC_THREADS_LINUX_BARRIER_H
+#define LLVM_LIBC___SUPPORT_SRC_THREADS_LINUX_BARRIER_H
+
+#include "include/llvm-libc-types/pthread_barrier_t.h"
+#include "include/llvm-libc-types/pthread_barrierattr_t.h"
+#include "src/__support/macros/config.h"
+#include "src/__support/threads/CndVar.h"
+#include "src/__support/threads/mutex.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+// NOTE: if the size of this class changes, you must ensure that the size of
+// pthread_barrier_t (found in include/llvm-libc/types/pthread_barrier_t.h) is
+// the same size
+
+extern const int BARRIER_FIRST_EXITED;
+
+class Barrier {
+private:
+ unsigned expected;
+ unsigned waiting;
+ bool blocking;
+ CndVar entering;
+ CndVar exiting;
+ Mutex m;
+
+public:
+ static int init(Barrier *b, const pthread_barrierattr_t *attr,
+ unsigned count);
+ static int destroy(Barrier *b);
+ int wait();
+};
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC___SUPPORT_SRC_THREADS_LINUX_BARRIER_H
diff --git a/libc/src/pthread/CMakeLists.txt b/libc/src/pthread/CMakeLists.txt
index c8c66805667fa..e5a0a34cf46a6 100644
--- a/libc/src/pthread/CMakeLists.txt
+++ b/libc/src/pthread/CMakeLists.txt
@@ -271,6 +271,40 @@ add_entrypoint_object(
libc.src.errno.errno
)
+add_entrypoint_object(
+ pthread_barrier_init
+ SRCS
+ pthread_barrier_init.cpp
+ HDRS
+ pthread_barrier_init.h
+ DEPENDS
+ libc.src.errno.errno
+ libc.include.pthread
+ libc.src.__support.threads.barrier
+)
+
+add_entrypoint_object(
+ pthread_barrier_destroy
+ SRCS
+ pthread_barrier_destroy.cpp
+ HDRS
+ pthread_barrier_destroy.h
+ DEPENDS
+ libc.include.pthread
+ libc.src.__support.threads.barrier
+)
+
+add_entrypoint_object(
+ pthread_barrier_wait
+ SRCS
+ pthread_barrier_wait.cpp
+ HDRS
+ pthread_barrier_wait.h
+ DEPENDS
+ libc.include.pthread
+ libc.src.__support.threads.barrier
+)
+
add_entrypoint_object(
pthread_mutex_init
SRCS
diff --git a/libc/src/pthread/pthread_barrier_destroy.cpp b/libc/src/pthread/pthread_barrier_destroy.cpp
new file mode 100644
index 0000000000000..389e1751ed3ee
--- /dev/null
+++ b/libc/src/pthread/pthread_barrier_destroy.cpp
@@ -0,0 +1,23 @@
+//===-- Linux implementation of the pthread_barrier_init function ---------===//
+//
+// 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 "pthread_barrier_destroy.h"
+
+#include "src/__support/common.h"
+#include "src/__support/macros/config.h"
+#include "src/__support/threads/barrier.h"
+
+#include <pthread.h>
+
+namespace LIBC_NAMESPACE_DECL {
+
+LLVM_LIBC_FUNCTION(int, pthread_barrier_destroy, (pthread_barrier_t * b)) {
+ return Barrier::destroy(reinterpret_cast<Barrier *>(b));
+}
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/pthread/pthread_barrier_destroy.h b/libc/src/pthread/pthread_barrier_destroy.h
new file mode 100644
index 0000000000000..0b1d0f090ec26
--- /dev/null
+++ b/libc/src/pthread/pthread_barrier_destroy.h
@@ -0,0 +1,21 @@
+//===-- Implementation header for pthread_barrier_destroy ----------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_PTHREAD_PTHREAD_BARRIER_DESTROY_H
+#define LLVM_LIBC_SRC_PTHREAD_PTHREAD_BARRIER_DESTROY_H
+
+#include "src/__support/macros/config.h"
+#include <pthread.h>
+
+namespace LIBC_NAMESPACE_DECL {
+
+int pthread_barrier_destroy(pthread_barrier_t *b);
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC_PTHREAD_PTHREAD_BARRIER_DESTROY_H
diff --git a/libc/src/pthread/pthread_barrier_init.cpp b/libc/src/pthread/pthread_barrier_init.cpp
new file mode 100644
index 0000000000000..1d0d87342d823
--- /dev/null
+++ b/libc/src/pthread/pthread_barrier_init.cpp
@@ -0,0 +1,31 @@
+//===-- Linux implementation of the pthread_barrier_init function ---------===//
+//
+// 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 "pthread_barrier_init.h"
+
+#include "src/__support/common.h"
+#include "src/__support/macros/config.h"
+#include "src/__support/threads/barrier.h"
+
+#include <pthread.h>
+
+namespace LIBC_NAMESPACE_DECL {
+
+static_assert(
+ sizeof(Barrier) <= sizeof(pthread_barrier_t),
+ "The public pthread_barrier_t type cannot accommodate the internal "
+ "barrier type.");
+
+LLVM_LIBC_FUNCTION(int, pthread_barrier_init,
+ (pthread_barrier_t * b,
+ const pthread_barrierattr_t *__restrict attr,
+ unsigned count)) {
+ return Barrier::init(reinterpret_cast<Barrier *>(b), attr, count);
+}
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/pthread/pthread_barrier_init.h b/libc/src/pthread/pthread_barrier_init.h
new file mode 100644
index 0000000000000..ea79df6da7ea9
--- /dev/null
+++ b/libc/src/pthread/pthread_barrier_init.h
@@ -0,0 +1,23 @@
+//===-- Implementation header for pthread_barrier_init ----------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_PTHREAD_PTHREAD_BARRIER_INIT_H
+#define LLVM_LIBC_SRC_PTHREAD_PTHREAD_BARRIER_INIT_H
+
+#include "src/__support/macros/config.h"
+#include <pthread.h>
+
+namespace LIBC_NAMESPACE_DECL {
+
+int pthread_barrier_init(pthread_barrier_t *b,
+ const pthread_barrierattr_t *__restrict attr,
+ unsigned count);
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC_PTHREAD_PTHREAD_BARRIER_INIT_H
diff --git a/libc/src/pthread/pthread_barrier_wait.cpp b/libc/src/pthread/pthread_barrier_wait.cpp
new file mode 100644
index 0000000000000..617d91dd16f77
--- /dev/null
+++ b/libc/src/pthread/pthread_barrier_wait.cpp
@@ -0,0 +1,27 @@
+//===-- Linux implementation of the pthread_barrier_init function ---------===//
+//
+// 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 "pthread_barrier_wait.h"
+
+#include "src/__support/common.h"
+#include "src/__support/macros/config.h"
+#include "src/__support/threads/barrier.h"
+
+#include <pthread.h>
+
+namespace LIBC_NAMESPACE_DECL {
+
+LLVM_LIBC_FUNCTION(int, pthread_barrier_wait, (pthread_barrier_t * b)) {
+ int out = reinterpret_cast<Barrier *>(b)->wait();
+ if (out == BARRIER_FIRST_EXITED)
+ return PTHREAD_BARRIER_SERIAL_THREAD;
+
+ return out;
+}
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/pthread/pthread_barrier_wait.h b/libc/src/pthread/pthread_barrier_wait.h
new file mode 100644
index 0000000000000..738ca56bd53e4
--- /dev/null
+++ b/libc/src/pthread/pthread_barrier_wait.h
@@ -0,0 +1,21 @@
+//===-- Implementation header for pthread_barrier_wait ----------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_PTHREAD_PTHREAD_BARRIER_WAIT_H
+#define LLVM_LIBC_SRC_PTHREAD_PTHREAD_BARRIER_WAIT_H
+
+#include "src/__support/macros/config.h"
+#include <pthread.h>
+
+namespace LIBC_NAMESPACE_DECL {
+
+int pthread_barrier_wait(pthread_barrier_t *b);
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC_PTHREAD_PTHREAD_BARRIER_WAIT_H
diff --git a/libc/test/integration/src/pthread/CMakeLists.txt b/libc/test/integration/src/pthread/CMakeLists.txt
index 208ba3fd43507..ce3bb9da9d58e 100644
--- a/libc/test/integration/src/pthread/CMakeLists.txt
+++ b/libc/test/integration/src/pthread/CMakeLists.txt
@@ -17,6 +17,23 @@ add_integration_test(
libc.src.pthread.pthread_join
)
+add_integration_test(
+ pthread_barrier_test
+ SUITE
+ libc-pthread-integration-tests
+ SRCS
+ pthread_barrier_test.cpp
+ DEPENDS
+ libc.include.pthread
+ libc.src.errno.errno
+ libc.src.pthread.pthread_barrier_destroy
+ libc.src.pthread.pthread_barrier_wait
+ libc.src.pthread.pthread_barrier_init
+ libc.src.pthread.pthread_create
+ libc.src.pthread.pthread_join
+ libc.src.stdio.printf
+)
+
add_integration_test(
pthread_rwlock_test
SUITE
diff --git a/libc/test/integration/src/pthread/pthread_barrier_test.cpp b/libc/test/integration/src/pthread/pthread_barrier_test.cpp
new file mode 100644
index 0000000000000..d4d696ec82507
--- /dev/null
+++ b/libc/test/integration/src/pthread/pthread_barrier_test.cpp
@@ -0,0 +1,117 @@
+//===-- Tests for pthread_barrier_t ---------------------------------------===//
+//
+// 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_barrier_destroy.h"
+#include "src/pthread/pthread_barrier_init.h"
+#include "src/pthread/pthread_barrier_wait.h"
+
+#include "src/__support/CPP/atomic.h"
+#include "src/pthread/pthread_create.h"
+#include "src/pthread/pthread_join.h"
+#include "src/pthread/pthread_mutex_destroy.h"
+#include "src/pthread/pthread_mutex_init.h"
+#include "src/pthread/pthread_mutex_lock.h"
+#include "src/pthread/pthread_mutex_unlock.h"
+#include "src/stdio/printf.h"
+
+#include "test/IntegrationTest/test.h"
+
+#include <pthread.h>
+
+pthread_barrier_t barrier;
+
+void smoke_test() {
+ ASSERT_EQ(LIBC_NAMESPACE::pthread_barrier_init(&barrier, nullptr, 1), 0);
+ ASSERT_EQ(LIBC_NAMESPACE::pthread_barrier_wait(&barrier),
+ PTHREAD_BARRIER_SERIAL_THREAD);
+ ASSERT_EQ(LIBC_NAMESPACE::pthread_barrier_destroy(&barrier), 0);
+}
+
+LIBC_NAMESPACE::cpp::Atomic<int> counter;
+void *increment_counter_and_wait(void *args) {
+ counter.fetch_add(1);
+ LIBC_NAMESPACE::pthread_barrier_wait(&barrier);
+ return 0;
+}
+
+void single_use_barrier() {
+ counter.set(0);
+ const int NUM_THREADS = 30;
+ pthread_t threads[NUM_THREADS];
+ ASSERT_EQ(
+ LIBC_NAMESPACE::pthread_barrier_init(&barrier, nullptr, NUM_THREADS + 1),
+ 0);
+
+ for (int i = 0; i < NUM_THREADS; ++i)
+ LIBC_NAMESPACE::pthread_create(&threads[i], nullptr,
+ increment_counter_and_wait, nullptr);
+
+ LIBC_NAMESPACE::pthread_barrier_wait(&barrier);
+ ASSERT_EQ(counter.load(), NUM_THREADS);
+
+ for (int i = 0; i < NUM_THREADS; ++i)
+ LIBC_NAMESPACE::pthread_join(threads[i], nullptr);
+
+ LIBC_NAMESPACE::pthread_barrier_destroy(&barrier);
+}
+
+void reusable_barrier() {
+ counter.set(0);
+ const int NUM_THREADS = 30;
+ const int REPEAT = 20;
+ pthread_t threads[NUM_THREADS...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Thanks for the patch. I am reviewing it right now. |
lntue
reviewed
Jul 15, 2025
SchrodingerZhu
requested changes
Jul 16, 2025
| #ifndef LLVM_LIBC_TYPES_PTHREAD_BARRIER_T_H | ||
| #define LLVM_LIBC_TYPES_PTHREAD_BARRIER_T_H | ||
|
|
||
| typedef struct { |
There was a problem hiding this comment.
alignment should not be 1. You can also make it less opaque.
There was a problem hiding this comment.
Thanks for the review! I agree with you that the alignment isn't going to be correct, but I'm not quite sure what the best way to fix this would be. Do you have any suggestions?
My current idea would be something like:
unsigned expected;
unsigned waiting;
bool blocking;
char entering[(sizeof CndVar)];
char exiting[(sizeof CndVar)];
char m[(sizeof Mutex)];
} pthread_barrier_t```
But this still doesn't preserve the alignment of the CndVar/Mutex variables properly.
There was a problem hiding this comment.
I'll let @SchrodingerZhu comment, but I think the only way to get the alignment correct is to add an explicit alignas to the alignof CndVar and alignof Mutex values.
(I'm also not sure of the value of making this less opaque; to me it just seems like a way to let users depend on what should be internal details of the representation.)
There was a problem hiding this comment.
@brooksmoses alignof CndVar does not work here because this is for the public header.
unsigned expected;
unsigned waiting;
bool blocking;
char entering[(sizeof CndVar)];
char exiting[(sizeof CndVar)];
char m[(sizeof Mutex)];
Something like this is good. Feel free to use __prefix as we are inside libc.
However, char m[(sizeof Mutex)] does not provide the alignment info.
Please check https://github.com/llvm/llvm-project/blob/main/libc/include/llvm-libc-types/__mutex_type.h and https://github.com/llvm/llvm-project/blob/main/libc/include/llvm-libc-types/pthread_mutex_t.h
brooksmoses
reviewed
Jul 17, 2025
|
|
||
| namespace LIBC_NAMESPACE_DECL { | ||
|
|
||
| const int BARRIER_FIRST_EXITED = -1; |
There was a problem hiding this comment.
Why not just use PTHREAD_BARRIER_SERIAL_THREAD here, and avoid the special-case logic in pthread_barrier_wait?
Also note that by defining it this way, you're using global storage to store the value (and then loading it from memory every time pthread_barrier_wait is called), rather than just inlining it in the code. That's a bit unnecessary, and introduces the possibility that something could cast away the const-ness and change it.
There was a problem hiding this comment.
My thought process for this was to decouple the threads support implementation from pthreads since the __support/threads folder is used by both C11 threads and pthreads. Although there is not technically a C11 barrier type, I don't like the idea of certain classes in the folder being generic while others are designed with only pthreads in mind. This may not be a real issue; I'd be happy to hear if you disagree (especially since this concept of one thread returning a special value might be unique to pthreads anyway).
As for the second point, isn't it likely that the compiler will inline the value anyway since it's a compile-time constant? Or is there a better way for me to do this? (constexpr probably makes more sense)
There was a problem hiding this comment.
(Repeating offline conversation, for the record.)
I think the concept of one thread returning a special value is fairly pthreads-specific. C11 threads.h doesn't have a barrier functionality, and C++20's std::barrier doesn't return a value. And if this does end up needing to be used by a different threading API, it might be a different return value like returning the decremented thread count. So I would personally use the pthreads constant for it, until we find another thing that needs this return value.
For the second point -- you need to declare the variable as static to keep it from getting global linkage. With global linkage, it's no longer a compile-time constant since some other compilation unit could access it and change it.
| int err; | ||
| err = CndVar::init(&b->entering); | ||
| if (err != 0) | ||
| return err; |
There was a problem hiding this comment.
The answer might be "no", but: Isn't there a macro for this "check error and return it if it's nonzero" construct?
| if (err != 0) | ||
| return err; | ||
|
|
||
| Mutex::init(&b->m, false, false, false, false); |
There was a problem hiding this comment.
Why aren't we checking this for an error return?
(I see that currently Mutex::init always returns MutexError::NONE, but that could change in the future. Alternately, if we don't expect it to change, then the Mutex::init function shouldn't have a return value.)
| LIBC_NAMESPACE::cpp::Atomic<int> counter; | ||
| void *increment_counter_and_wait(void *args) { | ||
| counter.fetch_add(1); | ||
| LIBC_NAMESPACE::pthread_barrier_wait(&barrier); |
There was a problem hiding this comment.
This could check the return value as well, and then you wouldn't need your separate one_nonzero_wait_returnval test.
I'd suggest adding a second counter, which you increment if and only if the return value is PTHREAD_BARRIER_SERIAL_THREAD, and otherwise assert that the return value is 0. Then after the threads are joined you can check that this second counter has a value of 1.
| LIBC_NAMESPACE::pthread_barrier_destroy(&barrier); | ||
| } | ||
|
|
||
| void reusable_barrier() { |
There was a problem hiding this comment.
Nitpick: The barrier is always reusable. Perhaps "reused_barrier"?
|
|
||
| void single_use_barrier() { | ||
| counter.set(0); | ||
| const int NUM_THREADS = 30; |
There was a problem hiding this comment.
There could be edge cases in the case with exactly 1 thread. I think it would be good to make the thread count a parameter of this function (and the other functions), and call it once with 1 and once with a larger number.
| counter.set(0); | ||
| const int NUM_THREADS = 30; | ||
| const int REPEAT = 20; | ||
| pthread_t threads[NUM_THREADS * REPEAT]; |
There was a problem hiding this comment.
Not that it matters, but it seems a little wasteful to create a different set of threads for each repeat, and to create all of them before joining any of them. Why not just join the threads inside the outer loop, and reuse the pthread_t array entries?
Also, if you make REPEAT a parameter of the function, this becomes the same as the previous function when REPEAT is 1, so you can avoid duplication.
| for (int i = 0; i < NUM_THREADS; ++i) { | ||
| void *ret; | ||
| LIBC_NAMESPACE::pthread_join(threads[i], &ret); | ||
| retsum += reinterpret_cast<uintptr_t>(ret); |
There was a problem hiding this comment.
Note that, precisely speaking, checking the sum isn't testing what it claims to. If two thread return -1, one returns 1, and the rest return 0, this will erroneously pass.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implemented barrier synchronization for pthreads