address code reviews

Author: Yifan Zhu

libc/config/config.json

@@ -44,11 +44,11 @@
   "pthread": {
     "LIBC_CONF_TIMEOUT_ENSURE_MONOTONICITY": {
       "value": true,
-      "doc": "Automatically adjust timeout to CLOCK_MONOTONIC."
+      "doc": "Automatically adjust timeout to CLOCK_MONOTONIC (default to true). POSIX API may require CLOCK_REALTIME, which can be unstable and leading to unexpected behavior. This option will convert the real-time timestamp to monotonic timestamp relative to the time of call."
     },
     "LIBC_CONF_RAW_MUTEX_DEFAULT_SPIN_COUNT": {
       "value": 100,
-      "doc": "Default number of spins before blocking if a mutex is in contention."
+      "doc": "Default number of spins before blocking if a mutex is in contention (default to 100)."
     }
   }
-}
+}

libc/docs/configure.rst

@@ -35,8 +35,8 @@ to learn about the defaults for your platform and target.
     - ``LIBC_CONF_PRINTF_DISABLE_WRITE_INT``: Disable handling of %n in printf format string.
     - ``LIBC_CONF_PRINTF_FLOAT_TO_STR_USE_MEGA_LONG_DOUBLE_TABLE``: Use large table for better printf long double performance.
 * **"pthread" options**
-    - ``LIBC_CONF_RAW_MUTEX_DEFAULT_SPIN_COUNT``: Default number of spins before blocking if a mutex is in contention.
-    - ``LIBC_CONF_TIMEOUT_ENSURE_MONOTONICITY``: Automatically adjust timeout to CLOCK_MONOTONIC.
+    - ``LIBC_CONF_RAW_MUTEX_DEFAULT_SPIN_COUNT``: Default number of spins before blocking if a mutex is in contention (default to 100).
+    - ``LIBC_CONF_TIMEOUT_ENSURE_MONOTONICITY``: Automatically adjust timeout to CLOCK_MONOTONIC (default to true). POSIX API may require CLOCK_REALTIME, which can be unstable and leading to unexpected behavior. This option will convert the real-time timestamp to monotonic timestamp relative to the time of call.
 * **"string" options**
     - ``LIBC_CONF_MEMSET_X86_USE_SOFTWARE_PREFETCHING``: Inserts prefetch for write instructions (PREFETCHW) for memset on x86 to recover performance when hardware prefetcher is disabled.
     - ``LIBC_CONF_STRING_UNSAFE_WIDE_READ``: Read more than a byte at a time to perform byte-string operations like strlen.

libc/src/__support/File/dir.h

@@ -50,14 +50,16 @@ class Dir {
   // A directory is to be opened by the static method open and closed
   // by the close method. So, all constructors and destructor are declared
   // as private. Inappropriate constructors are declared as deleted.
-  Dir() = delete;
-  Dir(const Dir &) = delete;
+  LIBC_INLINE Dir() = delete;
+  LIBC_INLINE Dir(const Dir &) = delete;
 
-  explicit Dir(int fdesc)
-      : fd(fdesc), readptr(0), fillsize(0), mutex(false, false, false, false) {}
-  ~Dir() = default;
+  LIBC_INLINE explicit Dir(int fdesc)
+      : fd(fdesc), readptr(0), fillsize(0),
+        mutex(/*timed=*/false, /*recursive=*/false, /*robust=*/false,
+              /*pshared=*/false) {}
+  LIBC_INLINE ~Dir() = default;
 
-  Dir &operator=(const Dir &) = delete;
+  LIBC_INLINE Dir &operator=(const Dir &) = delete;
 
 public:
   static ErrorOr<Dir *> open(const char *path);
@@ -69,7 +71,7 @@ class Dir {
   // cleaned up.
   int close();
 
-  int getfd() { return fd; }
+  LIBC_INLINE int getfd() { return fd; }
 };
 
 } // namespace LIBC_NAMESPACE

libc/src/__support/File/file.h

@@ -154,10 +154,11 @@ class File {
                  uint8_t *buffer, size_t buffer_size, int buffer_mode,
                  bool owned, ModeFlags modeflags)
       : platform_write(wf), platform_read(rf), platform_seek(sf),
-        platform_close(cf), mutex(false, false, false, false), ungetc_buf(0),
-        buf(buffer), bufsize(buffer_size), bufmode(buffer_mode), own_buf(owned),
-        mode(modeflags), pos(0), prev_op(FileOp::NONE), read_limit(0),
-        eof(false), err(false) {
+        platform_close(cf), mutex(/*timed=*/false, /*recursive=*/false,
+                                  /*robust=*/false, /*pshared=*/false),
+        ungetc_buf(0), buf(buffer), bufsize(buffer_size), bufmode(buffer_mode),
+        own_buf(owned), mode(modeflags), pos(0), prev_op(FileOp::NONE),
+        read_limit(0), eof(false), err(false) {
     adjust_buf();
   }
 

libc/src/__support/threads/CndVar.h

@@ -17,7 +17,7 @@
 
 namespace LIBC_NAMESPACE {
 
-struct CndVar {
+class CndVar {
   enum CndWaiterStatus : uint32_t {
     WS_Waiting = 0xE,
     WS_Signalled = 0x5,
@@ -30,15 +30,16 @@ struct CndVar {
 
   CndWaiter *waitq_front;
   CndWaiter *waitq_back;
-  internal::RawMutex qmtx;
+  RawMutex qmtx;
 
-  static int init(CndVar *cv) {
+public:
+  LIBC_INLINE static int init(CndVar *cv) {
     cv->waitq_front = cv->waitq_back = nullptr;
-    internal::RawMutex::init(&cv->qmtx);
+    RawMutex::init(&cv->qmtx);
     return 0;
   }
 
-  static void destroy(CndVar *cv) {
+  LIBC_INLINE static void destroy(CndVar *cv) {
     cv->waitq_front = cv->waitq_back = nullptr;
   }
 

libc/src/__support/threads/fork_callbacks.cpp

@@ -34,7 +34,9 @@ class AtForkCallbackManager {
 
 public:
   constexpr AtForkCallbackManager()
-      : mtx(false, false, false, false), next_index(0) {}
+      : mtx(/*timed=*/false, /*recursive=*/false, /*robust=*/false,
+            /*pshared=*/false),
+        next_index(0) {}
 
   bool register_triple(const ForkCallbackTriple &triple) {
     cpp::lock_guard lock(mtx);

libc/src/__support/threads/linux/CMakeLists.txt

@@ -24,7 +24,9 @@ add_header_library(
 
 set(raw_mutex_additional_flags)
 if (LIBC_CONF_TIMEOUT_ENSURE_MONOTONICITY)
-  set(raw_mutex_additional_flags -DLIBC_COPT_TIMEOUT_ENSURE_MONOTONICITY)
+  set(raw_mutex_additional_flags -DLIBC_COPT_TIMEOUT_ENSURE_MONOTONICITY=1)
+else()
+  set(raw_mutex_additional_flags -DLIBC_COPT_TIMEOUT_ENSURE_MONOTONICITY=0)
 endif()
 
 add_header_library(

libc/src/__support/threads/linux/CndVar.cpp

@@ -75,7 +75,7 @@ void CndVar::notify_one() {
   if (waitq_front == nullptr)
     waitq_back = nullptr;
 
-  qmtx.futex = internal::RawMutex::UNLOCKED;
+  qmtx.futex = RawMutex::UNLOCKED;
 
   // this is a special WAKE_OP, so we use syscall directly
   LIBC_NAMESPACE::syscall_impl<long>(

libc/src/__support/threads/linux/mutex.h

@@ -11,14 +11,15 @@
 
 #include "hdr/types/pid_t.h"
 #include "src/__support/CPP/optional.h"
+#include "src/__support/libc_assert.h"
 #include "src/__support/threads/linux/futex_utils.h"
 #include "src/__support/threads/linux/raw_mutex.h"
 #include "src/__support/threads/mutex_common.h"
 
 namespace LIBC_NAMESPACE {
 
 // TODO: support shared/recursive/robust mutexes.
-class Mutex final : private internal::RawMutex {
+class Mutex final : private RawMutex {
   // reserved timed, may be useful when combined with other flags.
   unsigned char timed;
   unsigned char recursive;
@@ -32,12 +33,12 @@ class Mutex final : private internal::RawMutex {
 public:
   LIBC_INLINE constexpr Mutex(bool is_timed, bool is_recursive, bool is_robust,
                               bool is_pshared)
-      : internal::RawMutex(), timed(is_timed), recursive(is_recursive),
-        robust(is_robust), pshared(is_pshared), owner(0), lock_count(0) {}
+      : RawMutex(), timed(is_timed), recursive(is_recursive), robust(is_robust),
+        pshared(is_pshared), owner(0), lock_count(0) {}
 
   LIBC_INLINE static MutexError init(Mutex *mutex, bool is_timed, bool isrecur,
                                      bool isrobust, bool is_pshared) {
-    internal::RawMutex::init(mutex);
+    RawMutex::init(mutex);
     mutex->timed = is_timed;
     mutex->recursive = isrecur;
     mutex->robust = isrobust;
@@ -47,31 +48,37 @@ class Mutex final : private internal::RawMutex {
     return MutexError::NONE;
   }
 
-  LIBC_INLINE static MutexError destroy(Mutex *) { return MutexError::NONE; }
+  LIBC_INLINE static MutexError destroy(Mutex *lock) {
+    LIBC_ASSERT(lock->owner == 0 && lock->lock_count == 0 &&
+                "Mutex destroyed while being locked.");
+    RawMutex::destroy(lock);
+    return MutexError::NONE;
+  }
 
   // TODO: record owner and lock count.
   LIBC_INLINE MutexError lock() {
-    this->internal::RawMutex::lock(
-        /* timeout */ cpp::nullopt, this->pshared);
+    // Since timeout is not specified, we do not need to check the return value.
+    this->RawMutex::lock(
+        /* timeout=*/cpp::nullopt, this->pshared);
     return MutexError::NONE;
   }
 
   // TODO: record owner and lock count.
   LIBC_INLINE MutexError timed_lock(internal::AbsTimeout abs_time) {
-    if (this->internal::RawMutex::lock(abs_time, this->pshared))
+    if (this->RawMutex::lock(abs_time, this->pshared))
       return MutexError::NONE;
     return MutexError::TIMEOUT;
   }
 
   LIBC_INLINE MutexError unlock() {
-    if (this->internal::RawMutex::unlock(this->pshared))
+    if (this->RawMutex::unlock(this->pshared))
       return MutexError::NONE;
     return MutexError::UNLOCK_WITHOUT_LOCK;
   }
 
   // TODO: record owner and lock count.
   LIBC_INLINE MutexError try_lock() {
-    if (this->internal::RawMutex::try_lock())
+    if (this->RawMutex::try_lock())
       return MutexError::NONE;
     return MutexError::BUSY;
   }

libc/src/__support/threads/linux/raw_mutex.h

@@ -8,24 +8,31 @@
 #ifndef LLVM_LIBC_SRC___SUPPORT_THREADS_LINUX_RAW_MUTEX_H
 #define LLVM_LIBC_SRC___SUPPORT_THREADS_LINUX_RAW_MUTEX_H
 
-#include "futex_word.h"
 #include "src/__support/CPP/optional.h"
 #include "src/__support/common.h"
+#include "src/__support/libc_assert.h"
 #include "src/__support/macros/attributes.h"
 #include "src/__support/threads/linux/futex_utils.h"
+#include "src/__support/threads/linux/futex_word.h"
 #include "src/__support/threads/sleep.h"
 #include "src/__support/time/linux/abs_timeout.h"
-#ifdef LIBC_COPT_TIMEOUT_ENSURE_MONOTONICITY
+
+#ifndef LIBC_COPT_TIMEOUT_ENSURE_MONOTONICITY
+#define LIBC_COPT_TIMEOUT_ENSURE_MONOTONICITY 1
+#warning "LIBC_COPT_TIMEOUT_ENSURE_MONOTONICITY is not defined, defaulting to 1"
+#endif
+
+#if LIBC_COPT_TIMEOUT_ENSURE_MONOTONICITY
 #include "src/__support/time/linux/monotonicity.h"
 #endif
 
 #ifndef LIBC_COPT_RAW_MUTEX_DEFAULT_SPIN_COUNT
 #define LIBC_COPT_RAW_MUTEX_DEFAULT_SPIN_COUNT 100
+#warning                                                                       \
+    "LIBC_COPT_RAW_MUTEX_DEFAULT_SPIN_COUNT is not defined, defaulting to 100"
 #endif
 
 namespace LIBC_NAMESPACE {
-struct CndVar;
-namespace internal {
 // Lock is a simple timable lock for internal usage.
 // This is separated from Mutex because this one does not need to consider
 // robustness and reentrancy. Also, this one has spin optimization for shorter
@@ -37,7 +44,8 @@ class RawMutex {
   LIBC_INLINE_VAR static constexpr FutexWordType LOCKED = 0b01;
   LIBC_INLINE_VAR static constexpr FutexWordType IN_CONTENTION = 0b10;
 
-  LIBC_INLINE FutexWordType spin(uint_fast32_t spin_count) {
+private:
+  LIBC_INLINE FutexWordType spin(int spin_count) {
     FutexWordType result;
     for (;;) {
       result = futex.load(cpp::MemoryOrder::RELAXED);
@@ -56,16 +64,16 @@ class RawMutex {
 
   // Return true if the lock is acquired. Return false if timeout happens before
   // the lock is acquired.
-  [[gnu::cold]] LIBC_INLINE bool
-  lock_slow(cpp::optional<Futex::Timeout> timeout, bool is_pshared,
-            uint_fast32_t spin_count) {
+  LIBC_INLINE bool lock_slow(cpp::optional<Futex::Timeout> timeout,
+                             bool is_pshared, int spin_count) {
     FutexWordType state = spin(spin_count);
     // Before go into contention state, try to grab the lock.
     if (state == UNLOCKED &&
         futex.compare_exchange_strong(state, LOCKED, cpp::MemoryOrder::ACQUIRE,
                                       cpp::MemoryOrder::RELAXED))
       return true;
-#ifdef LIBC_COPT_TIMEOUT_ENSURE_MONOTONICITY
+#if LIBC_COPT_TIMEOUT_ENSURE_MONOTONICITY
+    /* ADL should kick in */
     if (timeout)
       ensure_monotonicity(*timeout);
 #endif
@@ -83,14 +91,12 @@ class RawMutex {
     }
   }
 
-  [[gnu::cold]] LIBC_INLINE void wake(bool is_pshared) {
-    futex.notify_one(is_pshared);
-  }
+  LIBC_INLINE void wake(bool is_pshared) { futex.notify_one(is_pshared); }
 
 public:
   LIBC_INLINE static void init(RawMutex *mutex) { mutex->futex = UNLOCKED; }
   LIBC_INLINE constexpr RawMutex() : futex(UNLOCKED) {}
-  LIBC_INLINE bool try_lock() {
+  [[nodiscard]] LIBC_INLINE bool try_lock() {
     FutexWordType expected = UNLOCKED;
     // Use strong version since this is a one-time operation.
     return futex.compare_exchange_strong(
@@ -99,23 +105,25 @@ class RawMutex {
   LIBC_INLINE bool
   lock(cpp::optional<Futex::Timeout> timeout = cpp::nullopt,
        bool is_shared = false,
-       uint_fast32_t spin_count = LIBC_COPT_RAW_MUTEX_DEFAULT_SPIN_COUNT) {
+       int spin_count = LIBC_COPT_RAW_MUTEX_DEFAULT_SPIN_COUNT) {
     // Timeout will not be checked if immediate lock is possible.
-    if (try_lock())
+    if (LIBC_LIKELY(try_lock()))
       return true;
     return lock_slow(timeout, is_shared, spin_count);
   }
   LIBC_INLINE bool unlock(bool is_pshared = false) {
     FutexWordType prev = futex.exchange(UNLOCKED, cpp::MemoryOrder::RELEASE);
     // if there is someone waiting, wake them up
-    if (prev == IN_CONTENTION)
+    if (LIBC_UNLIKELY(prev == IN_CONTENTION))
       wake(is_pshared);
     // Detect invalid unlock operation.
     return prev != UNLOCKED;
   }
-  friend struct ::LIBC_NAMESPACE::CndVar;
+  LIBC_INLINE void static destroy([[maybe_unused]] RawMutex *lock) {
+    LIBC_ASSERT(lock->futex == UNLOCKED && "Mutex destroyed while used.");
+  }
+  friend class CndVar;
 };
-} // namespace internal
 } // namespace LIBC_NAMESPACE
 
 #endif // LLVM_LIBC_SRC___SUPPORT_THREADS_LINUX_RAW_MUTEX_H

libc/src/__support/threads/thread.cpp

@@ -54,7 +54,9 @@ class TSSKeyMgr {
   cpp::array<TSSKeyUnit, TSS_KEY_COUNT> units;
 
 public:
-  constexpr TSSKeyMgr() : mtx(false, false, false, false) {}
+  constexpr TSSKeyMgr()
+      : mtx(/*timed=*/false, /*recursive=*/false, /*robust=*/false,
+            /*pshared=*/false) {}
 
   cpp::optional<unsigned int> new_key(TSSDtor *dtor) {
     cpp::lock_guard lock(mtx);
@@ -111,7 +113,9 @@ class ThreadAtExitCallbackMgr {
   FixedVector<AtExitUnit, 1024> callback_list;
 
 public:
-  constexpr ThreadAtExitCallbackMgr() : mtx(false, false, false, false) {}
+  constexpr ThreadAtExitCallbackMgr()
+      : mtx(/*timed=*/false, /*recursive=*/false, /*robust=*/false,
+            /*pshared=*/false) {}
 
   int add_callback(AtExitCallback *callback, void *obj) {
     cpp::lock_guard lock(mtx);

libc/src/pthread/pthread_mutex_init.cpp

@@ -26,7 +26,7 @@ LLVM_LIBC_FUNCTION(int, pthread_mutex_init,
                     const pthread_mutexattr_t *__restrict attr)) {
   auto mutexattr = attr == nullptr ? DEFAULT_MUTEXATTR : *attr;
   auto err =
-      Mutex::init(reinterpret_cast<Mutex *>(m), /* timeout is supported */ true,
+      Mutex::init(reinterpret_cast<Mutex *>(m), /*is_timed=*/true,
                   get_mutexattr_type(mutexattr) & PTHREAD_MUTEX_RECURSIVE,
                   get_mutexattr_robust(mutexattr) & PTHREAD_MUTEX_ROBUST,
                   get_mutexattr_pshared(mutexattr) & PTHREAD_PROCESS_SHARED);

libc/src/stdlib/atexit.cpp

@@ -17,7 +17,8 @@ namespace LIBC_NAMESPACE {
 
 namespace {
 
-Mutex handler_list_mtx(false, false, false, false);
+Mutex handler_list_mtx(/*timed=*/false, /*recursive=*/false, /*robust=*/false,
+                       /*pshared=*/false);
 
 using AtExitCallback = void(void *);
 using StdCAtExitCallback = void(void);

libc/test/integration/src/__support/threads/thread_detach_test.cpp

@@ -10,7 +10,8 @@
 #include "src/__support/threads/thread.h"
 #include "test/IntegrationTest/test.h"
 
-LIBC_NAMESPACE::Mutex mutex(false, false, false, false);
+LIBC_NAMESPACE::Mutex mutex(/*timed=*/false, /*recursive=*/false,
+                            /*robust=*/false, /*pshared=*/false);
 
 int func(void *) {
   mutex.lock();