[libc-commits] [PATCH] D74653: [libc] Add simple implementations of mtx_lock and mtx_unlock.

[Alex Brachet via Phabricator via libc-commits]

abrachet added a comment.

In D74653#1877381 <https://reviews.llvm.org/D74653#1877381>, @sivachandra wrote:

> In D74653#1877328 <https://reviews.llvm.org/D74653#1877328>, @MaskRay wrote:
>
> > I think pthread_mutex_timedlock and pthread_mutex_unlock can be implemented first, then rebase mtx_lock and mtx_unlock on pthread. A libc eventually has to implement pthread to be usable on a POSIX platform. It can be a shim, with just the basic functionality. Even if you don't want to think of pthread initially, mtx_lock should be based on mtx_timedlock.
>
>
> In LLVM libc, we want to take the opposite approach of implementing pthread as an extension over standard C thread library.


My intuition (I've never really used <threads.h>) is that it will be more difficult than the opposite because pthread provides many more options than C11's thread library. I think this patch is good and it'll be useful to have mutex's but my guess is when we implement pthread it will become untenable to use what we currently have.



================
Comment at: libc/include/threads.h.def:9-10
+
+#ifndef LLVM_LIBC_STRING_H
+#define LLVM_LIBC_STRING_H
+
----------------
LIBC_THREADS_H


================
Comment at: libc/lib/CMakeLists.txt:15
     munmap
+
+    mtx_init
----------------
You could add a comment here like the rest of them have, but I don't think its needed, its just more cohesive.


================
Comment at: libc/src/threads/linux/mtx_lock.cpp:27
+
+    if (!atomic_compare_exchange_strong(futex_data, &mutex_status, MS_Locked)) {
+      switch (mutex_status) {
----------------
I think it would be more readable if this looked like 

```
if (atomic_compare_exchange_strong(futex_data, &mutex_status, MS_Locked))
  return thrd_success;
switch(mutex_status) {
...
}
```


================
Comment at: libc/src/threads/linux/mtx_unlock.cpp:27
+      // If any thread is waiting to be woken up, then do it.
+      __llvm_libc::syscall(SYS_futex, (long)futex_word, FUTEX_WAKE, 1, 0, 0, 0);
+      return thrd_success;
----------------
The cast should no longer be necessary.


================
Comment at: libc/src/threads/linux/mtx_unlock.cpp:32
+    if (mutex_status == MS_Locked) {
+      // If no body was waiting at this point, just free it.
+      if (atomic_compare_exchange_strong(futex_word, &mutex_status, MS_Free))
----------------
no body -> nobody


================
Comment at: libc/src/threads/linux/mutex.h:17
+// made only if the mutex status is `MS_Waiting`.
+enum MutexStatus : uint32_t { MS_Free, MS_Locked, MS_Waiting };
+
----------------
We could make this `enum class MutexStatus` and drop the MS_ prefix and refer to these as `MutexStatus::Free` perhaps.


================
Comment at: libc/src/threads/linux/mutex.h:19
+
+typedef _Atomic uint32_t FutexData;
+
----------------
Prefer `using`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74653