[NFC][libcxxabi] In cxa_guard, split the InitByte classes into InitByte... and ...Guard

[DanielMcIntosh-IBM]

Summary

We split each of the InitByteNoThreads, InitByteGlobalMutex, and
InitByteFutex classes into 2 different classes.

The three new classes NoThreadsGuard, GlobalMutexGuard, and FutexGuard
replace the old classes, and the old classes now focus only on
providing functions for managing reads/writes to the init byte. That is,
the definitions of acquire/release/abort_init_byte, remain in the old
classes, but nothing else.

The new classes are then responsible for everything else: dividing up
the raw guard object, providing cxa_guard_acquire/release/abort (by
inheriting from GuardObject), etc.

This is the 3rd of 6 changes to overhaul cxa_guard.
See D108343 for what the final result will be and more details on why
cxa_guard is getting overhauled.

Depends on D110088

[DanielMcIntosh-IBM]

Ping

[DanielMcIntosh-IBM]

Ping.

I have a bunch more PRs I'm going to be creating soon (they're getting reviewed downstream rn), and I'd like to get some of these older ones closed off so I don't have too many to keep track of.
@ldionne I know you're probably quite busy. Is there someone else who might also be suitable to review this? I can't see anyone other than you and @EricWF who have made significant modifications to libcxxabi/src/cxa_guard_impl.h (or cxa_guard.cpp).
I've added Nick Lewycky and Jon Roelofs since they seem to have made some smaller modifications, but the file has changed dramatically since they made their changes, and they're not members of the lib++abi project.

[DanielMcIntosh-IBM]

Ping

[ldionne]

libcxxabi/src/cxa_guard_impl.h

174

I was looking at this patch and I found that using CRTP for this was adding a lot of complexity. It it possible that using a free function (template) for implementing cxa_guard_acquire, cxa_guard_release and cxa_guard_abort would greatly simplify things? Imagine:

template <class Guard>
AcquireResult cxa_guard_acquire(Guard& guard) {
  AtomicInt<uint8_t> guard_byte(guard.get_init_byte()); // all types of guards need to answer to that
  if (guard_byte.load(std::_AO_Acquire) != UNSET)
    return INIT_IS_DONE;
  return derived()->acquire_init_byte();
}

// etc..

Then, for example NoThreadsGuard would become:

using NoThreadsGuard = InitByteNoThreads;

At that point we might want to simplify the whole thing and simply call InitByteNoThreads NoThreadsGuard directly.

[DanielMcIntosh-IBM]

libcxxabi/src/cxa_guard_impl.h

174

Yes, using CRTP is one of the reasons I found the current implementation a little difficult to follow, which prompted this whole refactor. I get rid of it in D110101. I'm not entirely sure if I fully understand what it is you're suggesting, but I think that change accomplishes the same goal, except they're part of GuardBase instead of free functions. This works a bit better with the SelectImplementation stuff at the end of the file. Plus, I think having separate classes dedicated to managing the guard byte, to managing the init byte, and to co-coordinating between them is probably a little better for separation of concerns.

[DanielMcIntosh-IBM]

libcxxabi/src/cxa_guard_impl.h

174

Actually, looking at this with fresh eyes, I think there is a better way of grouping these changes. See D115367, D115368 and D115369 for newer versions.

libcxxabi/src/cxa_guard_impl.h

 constexpr AcquireResult INIT_IS_PENDING = AcquireResult::INIT_IS_PENDING;
 
 static constexpr uint8_t UNSET = 0;
 static constexpr uint8_t COMPLETE_BIT = (1 << 0);
 static constexpr uint8_t PENDING_BIT = (1 << 1);
 static constexpr uint8_t WAITING_BIT = (1 << 2);
 
 template <class Derived>
 struct GuardObject {
   GuardObject() = delete;
   GuardObject(GuardObject const&) = delete;
   GuardObject& operator=(GuardObject const&) = delete;
 
-  explicit GuardObject(uint32_t* g)
-      : base_address(g), guard_byte_address(reinterpret_cast<uint8_t*>(g)),
-        init_byte_address(reinterpret_cast<uint8_t*>(g) + 1), thread_id_address(nullptr) {}
-
-  explicit GuardObject(uint64_t* g)
-      : base_address(g), guard_byte_address(reinterpret_cast<uint8_t*>(g)),
-        init_byte_address(reinterpret_cast<uint8_t*>(g) + 1), thread_id_address(reinterpret_cast<uint32_t*>(g) + 1) {}
+  explicit GuardObject(uint8_t* const guard_byte_address) : guard_byte(guard_byte_address) {}
 
 public:
   /// Implements __cxa_guard_acquire
   AcquireResult cxa_guard_acquire() {
-    AtomicInt<uint8_t> guard_byte(guard_byte_address);
     if (guard_byte.load(std::_AO_Acquire) != UNSET)
       return INIT_IS_DONE;
     return derived()->acquire_init_byte();
   }
 
   /// Implements __cxa_guard_release
   void cxa_guard_release() {
-    AtomicInt<uint8_t> guard_byte(guard_byte_address);
     // Store complete first, so that when release wakes other folks, they see
     // it as having been completed.
     guard_byte.store(COMPLETE_BIT, std::_AO_Release);
     derived()->release_init_byte();
   }
 
   /// Implements __cxa_guard_abort
   void cxa_guard_abort() { derived()->abort_init_byte(); }
 
-public:
-  /// base_address - the address of the original guard object.
-  void* const base_address;
-  /// The address of the guard byte at offset 0.
-  uint8_t* const guard_byte_address;
-  /// The address of the byte used by the implementation during initialization.
-  uint8_t* const init_byte_address;
-  /// An optional address storing an identifier for the thread performing initialization.
-  /// It's used to detect recursive initialization.
-  uint32_t* const thread_id_address;
-
 private:
+  AtomicInt<uint8_t> guard_byte;
+
   Derived* derived() { return static_cast<Derived*>(this); }
 };
 
 //===----------------------------------------------------------------------===//
 //                    Single Threaded Implementation
 //===----------------------------------------------------------------------===//
 
-struct InitByteNoThreads : GuardObject<InitByteNoThreads> {
-  using GuardObject::GuardObject;
-
+/// InitByteNoThreads - Doesn't use any inter-thread synchronization when
+/// managing reads and writes to the init byte.
+struct InitByteNoThreads {
+  InitByteNoThreads() = delete;
+  InitByteNoThreads(InitByteNoThreads const&) = delete;
+  InitByteNoThreads& operator=(InitByteNoThreads const&) = delete;
+
+  explicit InitByteNoThreads(uint8_t* _init_byte_address) : init_byte_address(_init_byte_address) {}
+
+  /// The init_byte portion of cxa_guard_acquire.
+  /// Note: On completion, we haven't 'acquired' ownership of anything mutex
+  /// related. The name is simply referring to __cxa_guard_acquire.
   AcquireResult acquire_init_byte() {
     if (*init_byte_address == COMPLETE_BIT)
       return INIT_IS_DONE;
     if (*init_byte_address & PENDING_BIT)
       ABORT_WITH_MESSAGE("__cxa_guard_acquire detected recursive initialization");
     *init_byte_address = PENDING_BIT;
     return INIT_IS_PENDING;
   }
 
+  /// The init_byte portion of cxa_guard_release.
+  /// Note: On completion, we haven't 'released' ownership of anything mutex
+  /// related. The name is simply referring to __cxa_guard_release.
   void release_init_byte() { *init_byte_address = COMPLETE_BIT; }
+  /// The init_byte portion of cxa_guard_abort.
   void abort_init_byte() { *init_byte_address = UNSET; }
+
+private:
+  /// The address of the byte used during initialization.
+  uint8_t* const init_byte_address;
 };
 
 //===----------------------------------------------------------------------===//
 //                     Global Mutex Implementation
 //===----------------------------------------------------------------------===//
 
 struct LibcppMutex;
 struct LibcppCondVar;

 private:
   std::__libcpp_condvar_t cond = _LIBCPP_CONDVAR_INITIALIZER;
 };
 #else
 struct LibcppMutex {};
 struct LibcppCondVar {};
 #endif // !defined(_LIBCXXABI_HAS_NO_THREADS)
 
+/// InitByteGlobalMutex - Uses a global mutex and condition variable (common to
+/// all static local variables) to manage reads and writes to the init byte.
 template <class Mutex, class CondVar, Mutex& global_mutex, CondVar& global_cond,
           uint32_t (*GetThreadID)() = PlatformThreadID>
-struct InitByteGlobalMutex : GuardObject<InitByteGlobalMutex<Mutex, CondVar, global_mutex, global_cond, GetThreadID>> {
-
-  using BaseT = typename InitByteGlobalMutex::GuardObject;
-  using BaseT::BaseT;
+struct InitByteGlobalMutex {
 
-  explicit InitByteGlobalMutex(uint32_t* g) : BaseT(g), has_thread_id_support(false) {}
-  explicit InitByteGlobalMutex(uint64_t* g) : BaseT(g), has_thread_id_support(GetThreadID != nullptr) {}
+  explicit InitByteGlobalMutex(uint8_t* _init_byte_address, uint32_t* _thread_id_address)
+      : init_byte_address(_init_byte_address), thread_id_address(_thread_id_address),
+        has_thread_id_support(_thread_id_address != nullptr && GetThreadID != nullptr) {}
 
 public:
+  /// The init_byte portion of cxa_guard_acquire.
+  /// Note: On completion, we haven't 'acquired' ownership of anything mutex
+  /// related. The name is simply referring to __cxa_guard_acquire.
   AcquireResult acquire_init_byte() {
     LockGuard g("__cxa_guard_acquire");
     // Check for possible recursive initialization.
     if (has_thread_id_support && (*init_byte_address & PENDING_BIT)) {
       if (*thread_id_address == current_thread_id.get())
         ABORT_WITH_MESSAGE("__cxa_guard_acquire detected recursive initialization");
     }
 
     // Wait until the pending bit is not set.
     while (*init_byte_address & PENDING_BIT) {
       *init_byte_address |= WAITING_BIT;
       global_cond.wait(global_mutex);
     }
 
     if (*init_byte_address == COMPLETE_BIT)
       return INIT_IS_DONE;
 
     if (has_thread_id_support)
       *thread_id_address = current_thread_id.get();
 
     *init_byte_address = PENDING_BIT;
     return INIT_IS_PENDING;
   }
 
+  /// The init_byte portion of cxa_guard_release.
+  /// Note: On completion, we haven't 'released' ownership of anything mutex
+  /// related. The name is simply referring to __cxa_guard_release.
   void release_init_byte() {
     bool has_waiting;
     {
       LockGuard g("__cxa_guard_release");
       has_waiting = *init_byte_address & WAITING_BIT;
       *init_byte_address = COMPLETE_BIT;
     }
     if (has_waiting) {
       if (global_cond.broadcast()) {
         ABORT_WITH_MESSAGE("%s failed to broadcast", "__cxa_guard_release");
       }
     }
   }
 
+  /// The init_byte portion of cxa_guard_abort.
   void abort_init_byte() {
     bool has_waiting;
     {
       LockGuard g("__cxa_guard_abort");
       if (has_thread_id_support)
         *thread_id_address = 0;
       has_waiting = *init_byte_address & WAITING_BIT;
       *init_byte_address = UNSET;
     }
     if (has_waiting) {
       if (global_cond.broadcast()) {
         ABORT_WITH_MESSAGE("%s failed to broadcast", "__cxa_guard_abort");
       }
     }
   }
 
 private:
-  using BaseT::init_byte_address;
-  using BaseT::thread_id_address;
+  /// The address of the byte used during initialization.
+  uint8_t* const init_byte_address;
+  /// An optional address storing an identifier for the thread performing initialization.
+  /// It's used to detect recursive initialization.
+  uint32_t* const thread_id_address;
+
   const bool has_thread_id_support;
   LazyValue<uint32_t, GetThreadID> current_thread_id;
 
 private:
   struct LockGuard {
     LockGuard() = delete;
     LockGuard(LockGuard const&) = delete;
     LockGuard& operator=(LockGuard const&) = delete;

 }
 #else
 constexpr void (*PlatformFutexWait)(int*, int) = nullptr;
 constexpr void (*PlatformFutexWake)(int*) = nullptr;
 #endif
 
 constexpr bool PlatformSupportsFutex() { return +PlatformFutexWait != nullptr; }
 
-/// InitByteFutex - Manages initialization using atomics and the futex syscall
-/// for waiting and waking.
+/// InitByteFutex - Uses a futex to manage reads and writes to the init byte.
 template <void (*Wait)(int*, int) = PlatformFutexWait, void (*Wake)(int*) = PlatformFutexWake,
           uint32_t (*GetThreadIDArg)() = PlatformThreadID>
-struct InitByteFutex : GuardObject<InitByteFutex<Wait, Wake, GetThreadIDArg>> {
-  using BaseT = typename InitByteFutex::GuardObject;
+struct InitByteFutex {
 
-  /// ARM Constructor
-  explicit InitByteFutex(uint32_t* g)
-      : BaseT(g), init_byte(this->init_byte_address),
-        has_thread_id_support(this->thread_id_address && GetThreadIDArg != nullptr),
-        thread_id(this->thread_id_address) {}
-
-  /// Itanium Constructor
-  explicit InitByteFutex(uint64_t* g)
-      : BaseT(g), init_byte(this->init_byte_address),
-        has_thread_id_support(this->thread_id_address && GetThreadIDArg != nullptr),
-        thread_id(this->thread_id_address) {}
+  explicit InitByteFutex(uint8_t* _init_byte_address, uint32_t* _thread_id_address)
+      : init_byte(_init_byte_address),
+        has_thread_id_support(_thread_id_address != nullptr && GetThreadIDArg != nullptr),
+        thread_id(_thread_id_address), base_address(reinterpret_cast<int*>(
+                                           /*_init_byte_address & ~0x3*/ _init_byte_address - 1)) {}
 
 public:
+  /// The init_byte portion of cxa_guard_acquire.
+  /// Note: On completion, we haven't 'acquired' ownership of anything mutex
+  /// related. The name is simply referring to __cxa_guard_acquire.
   AcquireResult acquire_init_byte() {
     while (true) {
       uint8_t last_val = UNSET;
       if (init_byte.compare_exchange(&last_val, PENDING_BIT, std::_AO_Acq_Rel, std::_AO_Acquire)) {
         if (has_thread_id_support) {
           thread_id.store(current_thread_id.get(), std::_AO_Relaxed);
         }
         return INIT_IS_PENDING;
while (true) {
             // (2) the waiting bit got set, so we are happy to keep waiting
           }
         }
         wait_on_initialization();
       }
     }
   }
 
+  /// The init_byte portion of cxa_guard_release.
+  /// Note: On completion, we haven't 'released' ownership of anything mutex
+  /// related. The name is simply referring to __cxa_guard_release.
   void release_init_byte() {
     uint8_t old = init_byte.exchange(COMPLETE_BIT, std::_AO_Acq_Rel);
     if (old & WAITING_BIT)
       wake_all();
   }
 
+  /// The init_byte portion of cxa_guard_abort.
   void abort_init_byte() {
     if (has_thread_id_support)
       thread_id.store(0, std::_AO_Relaxed);
 
     uint8_t old = init_byte.exchange(UNSET, std::_AO_Acq_Rel);
     if (old & WAITING_BIT)
       wake_all();
   }
 
 private:
   /// Use the futex to wait on the current guard variable. Futex expects a
-  /// 32-bit 4-byte aligned address as the first argument, so we have to use use
-  /// the base address of the guard variable (not the init byte).
-  void wait_on_initialization() {
-    Wait(static_cast<int*>(this->base_address), expected_value_for_futex(PENDING_BIT | WAITING_BIT));
-  }
-  void wake_all() { Wake(static_cast<int*>(this->base_address)); }
+  /// 32-bit 4-byte aligned address as the first argument, so we use the 4-byte
+  /// aligned address that encompasses the init byte (i.e. the address of the
+  /// raw guard object that was passed to __cxa_guard_acquire/release/abort).
+  void wait_on_initialization() { Wait(base_address, expected_value_for_futex(PENDING_BIT | WAITING_BIT)); }
+  void wake_all() { Wake(base_address); }
 
 private:
   AtomicInt<uint8_t> init_byte;
 
   const bool has_thread_id_support;
   // Unsafe to use unless has_thread_id_support
   AtomicInt<uint32_t> thread_id;
   LazyValue<uint32_t, GetThreadIDArg> current_thread_id;
 
+  /// the 4-byte-aligned address that encompasses the init byte (i.e. the
+  /// address of the raw guard object).
+  int* const base_address;
+
   /// Create the expected integer value for futex `wait(int* addr, int expected)`.
   /// We pass the base address as the first argument, So this function creates
   /// an zero-initialized integer  with `b` copied at the correct offset.
   static int expected_value_for_futex(uint8_t b) {
     int dest_val = 0;
     std::memcpy(reinterpret_cast<char*>(&dest_val) + 1, &b, 1);
     return dest_val;
   }
 
   static_assert(Wait != nullptr && Wake != nullptr, "");
 };
 
 //===----------------------------------------------------------------------===//
+//                                   Guards
+//===----------------------------------------------------------------------===//
+
+/// NoThreadsGuard - Manages initialization without performing any inter-thread
+/// synchronization.
+struct NoThreadsGuard : GuardObject<NoThreadsGuard>, InitByteNoThreads {
+  using BaseT = typename NoThreadsGuard::GuardObject;
+  using InitByteT = typename NoThreadsGuard::InitByteNoThreads;
+
+  explicit NoThreadsGuard(uint32_t* raw_guard_object)
+      : BaseT(reinterpret_cast<uint8_t*>(raw_guard_object)),
+        InitByteT(reinterpret_cast<uint8_t*>(raw_guard_object) + 1) {}
+
+  explicit NoThreadsGuard(uint64_t* raw_guard_object)
+      : BaseT(reinterpret_cast<uint8_t*>(raw_guard_object)),
+        InitByteT(reinterpret_cast<uint8_t*>(raw_guard_object) + 1) {}
+};
+
+/// GlobalMutexGuard - Manages initialization using a global mutex and
+/// condition variable.
+template <class Mutex, class CondVar, Mutex& global_mutex, CondVar& global_cond,
+          uint32_t (*GetThreadID)() = PlatformThreadID>
+struct GlobalMutexGuard : GuardObject<GlobalMutexGuard<Mutex, CondVar, global_mutex, global_cond, GetThreadID>>,
+                          InitByteGlobalMutex<Mutex, CondVar, global_mutex, global_cond, GetThreadID> {
+
+  using BaseT = typename GlobalMutexGuard::GuardObject;
+  using InitByteT = typename GlobalMutexGuard::InitByteGlobalMutex;
+
+  explicit GlobalMutexGuard(uint32_t* raw_guard_object)
+      : BaseT(reinterpret_cast<uint8_t*>(raw_guard_object)),
+        InitByteT(reinterpret_cast<uint8_t*>(raw_guard_object) + 1, nullptr) {}
+  explicit GlobalMutexGuard(uint64_t* raw_guard_object)
+      : BaseT(reinterpret_cast<uint8_t*>(raw_guard_object)),
+        InitByteT(reinterpret_cast<uint8_t*>(raw_guard_object) + 1, reinterpret_cast<uint32_t*>(raw_guard_object) + 1) {
+  }
+};
+
+/// FutexGuard - Manages initialization using atomics and the futex syscall for
+/// waiting and waking.
+template <void (*Wait)(int*, int) = PlatformFutexWait, void (*Wake)(int*) = PlatformFutexWake,
+          uint32_t (*GetThreadIDArg)() = PlatformThreadID>
+struct FutexGuard : GuardObject<FutexGuard<Wait, Wake, GetThreadIDArg>>, InitByteFutex<Wait, Wake, GetThreadIDArg> {
+  using BaseT = typename FutexGuard::GuardObject;
+  using InitByteT = typename FutexGuard::InitByteFutex;
+
+  /// ARM Constructor
+  explicit FutexGuard(uint32_t* raw_guard_object)
+      : BaseT(reinterpret_cast<uint8_t*>(raw_guard_object)),
+        InitByteT(reinterpret_cast<uint8_t*>(raw_guard_object) + 1, nullptr) {}
+
+  /// Itanium Constructor
+  explicit FutexGuard(uint64_t* raw_guard_object)
+      : BaseT(reinterpret_cast<uint8_t*>(raw_guard_object)),
+        InitByteT(reinterpret_cast<uint8_t*>(raw_guard_object) + 1, reinterpret_cast<uint32_t*>(raw_guard_object) + 1) {
+  }
+};
+
+//===----------------------------------------------------------------------===//
 //
 //===----------------------------------------------------------------------===//
 
 template <class T>
 struct GlobalStatic {
   static T instance;
 };
 template <class T>
 _LIBCPP_SAFE_STATIC T GlobalStatic<T>::instance = {};
 
 enum class Implementation { NoThreads, GlobalMutex, Futex };
 
 template <Implementation Impl>
 struct SelectImplementation;
 
 template <>
 struct SelectImplementation<Implementation::NoThreads> {
-  using type = InitByteNoThreads;
+  using type = NoThreadsGuard;
 };
 
 template <>
 struct SelectImplementation<Implementation::GlobalMutex> {
-  using type = InitByteGlobalMutex<LibcppMutex, LibcppCondVar, GlobalStatic<LibcppMutex>::instance,
-                                   GlobalStatic<LibcppCondVar>::instance, PlatformThreadID>;
+  using type = GlobalMutexGuard<LibcppMutex, LibcppCondVar, GlobalStatic<LibcppMutex>::instance,
 };
 
 template <>
 struct SelectImplementation<Implementation::Futex> {
-  using type = InitByteFutex<PlatformFutexWait, PlatformFutexWake, PlatformThreadID>;
+  using type = FutexGuard<PlatformFutexWait, PlatformFutexWake, PlatformThreadID>;
 };
 
 // TODO(EricWF): We should prefer the futex implementation when available. But
 // it should be done in a separate step from adding the implementation.
 constexpr Implementation CurrentImplementation =
 #if defined(_LIBCXXABI_HAS_NO_THREADS)
     Implementation::NoThreads;
 #elif defined(_LIBCXXABI_USE_FUTEX)

libcxxabi/test/guard_test_basic.pass.cpp

 void NopFutexWait(int*, int) { assert(false); }
 void NopFutexWake(int*) { assert(false); }
 uint32_t MockGetThreadID() { return 0; }
 
 int main(int, char**) {
   {
 #if defined(_LIBCXXABI_HAS_NO_THREADS)
     static_assert(CurrentImplementation == Implementation::NoThreads, "");
-    static_assert(
-        std::is_same<SelectedImplementation, InitByteNoThreads>::value, "");
+    static_assert(std::is_same<SelectedImplementation, NoThreadsGuard>::value, "");
 #else
     static_assert(CurrentImplementation == Implementation::GlobalMutex, "");
-    static_assert(
-        std::is_same<
-            SelectedImplementation,
-            InitByteGlobalMutex<LibcppMutex, LibcppCondVar,
-                                GlobalStatic<LibcppMutex>::instance,
+    static_assert(std::is_same<SelectedImplementation,
+                               GlobalMutexGuard<LibcppMutex, LibcppCondVar, GlobalStatic<LibcppMutex>::instance,
                                                 GlobalStatic<LibcppCondVar>::instance>>::value,
                   "");
 #endif
   }
   {
 #if (defined(__APPLE__) || defined(__linux__))  && !defined(_LIBCXXABI_HAS_NO_THREADS)
     assert(PlatformThreadID);
 #endif
     if (PlatformThreadID != nullptr) {
       assert(PlatformThreadID() != 0);
       assert(PlatformThreadID() == PlatformThreadID());
     }
   }
   {
-    Tests<uint32_t, InitByteNoThreads>::test();
-    Tests<uint64_t, InitByteNoThreads>::test();
+    Tests<uint32_t, NoThreadsGuard>::test();
+    Tests<uint64_t, NoThreadsGuard>::test();
   }
   {
-    using MutexImpl =
-        InitByteGlobalMutex<NopMutex, NopCondVar, global_nop_mutex,
-                            global_nop_cond, MockGetThreadID>;
+    using MutexImpl = GlobalMutexGuard<NopMutex, NopCondVar, global_nop_mutex, global_nop_cond, MockGetThreadID>;
     Tests<uint32_t, MutexImpl>::test();
     Tests<uint64_t, MutexImpl>::test();
   }
   {
-    using FutexImpl =
-        InitByteFutex<&NopFutexWait, &NopFutexWake, &MockGetThreadID>;
+    using FutexImpl = FutexGuard<&NopFutexWait, &NopFutexWake, &MockGetThreadID>;
     Tests<uint32_t, FutexImpl>::test();
     Tests<uint64_t, FutexImpl>::test();
   }
 
   return 0;
 }