[NFC][libcxxabi] Moved cxa_guard_acquire/release/abort into GuardBase

[DanielMcIntosh-IBM]

Summary

Since we want GuardByte to focus exclusively on managing reads/writes to
the guard byte, it shouldn't be responsible for co-ordinating with the
InitByte classes. Instead, we let GuardBase co-ordinate between
GuardByte and InitByte.

This design also eliminates the need for the curiously recurring
template pattern, making control flow much easier to follow.

This is the 5th 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 D110097

[DanielMcIntosh-IBM]

Ping

[DanielMcIntosh-IBM]

Ping

[DanielMcIntosh-IBM]

Ping

[DanielMcIntosh-IBM]

Abandoning, since I no longer think this is the best way of grouping the changes. See D115367, D115368 and D115369 for a newer version that groups things a little better, but with the same end result.

libcxxabi/src/cxa_guard_impl.h

 #else
 constexpr uint32_t (*PlatformThreadID)() = nullptr;
 #endif
 
 //===----------------------------------------------------------------------===//
 //                          GuardByte
 //===----------------------------------------------------------------------===//
 
-enum class AcquireResult {
-  INIT_IS_DONE,
-  INIT_IS_PENDING,
-};
-constexpr AcquireResult INIT_IS_DONE = AcquireResult::INIT_IS_DONE;
-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);
 
-/// Manages reads and writes to the guard byte. Until cxa_guard_release is
-/// called this is basically a wrapper around the Derived class
-template <class Derived>
+/// Manages reads and writes to the guard byte.
 struct GuardByte {
   GuardByte() = delete;
   GuardByte(GuardByte const&) = delete;
   GuardByte& operator=(GuardByte const&) = delete;
 
   explicit GuardByte(uint8_t* const guard_byte_address) : guard_byte(guard_byte_address) {}
 
 public:
-  /// Implements __cxa_guard_acquire
-  AcquireResult cxa_guard_acquire() {
-    if (guard_byte.load(std::_AO_Acquire) != UNSET)
-      return INIT_IS_DONE;
-    return derived()->acquire_init_byte();
+  /// The guard_byte portion of cxa_guard_acquire. Returns true if
+  /// initialization has already been completed.
+  /// Note: On completion, we haven't 'acquired' ownership of anything mutex
+  /// related. The name is simply referring to __cxa_guard_acquire.
+  bool acquire_guard_byte() {
+    // if guard_byte is non-zero, we have already completed initialization
+    // (i.e. release_guard_byte has been called)
+    return guard_byte.load(std::_AO_Acquire) != UNSET;
   }
 
-  /// Implements __cxa_guard_release
-  void cxa_guard_release() {
-    // 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();
-  }
+  /// The guard_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_guard_byte() { guard_byte.store(COMPLETE_BIT, std::_AO_Release); }
 
-  /// Implements __cxa_guard_abort
-  void cxa_guard_abort() { derived()->abort_init_byte(); }
+  /// The guard_byte portion of cxa_guard_abort.
+  void abort_guard_byte() {} // Nothing to do
 
 private:
   AtomicInt<uint8_t> guard_byte;
-
-  Derived* derived() { return static_cast<Derived*>(this); }
 };
 
 //===----------------------------------------------------------------------===//
 //                    Single Threaded Implementation
 //===----------------------------------------------------------------------===//
 
 /// 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, uint32_t*) : init_byte_address(_init_byte_address) {}
 
-  /// The init_byte portion of cxa_guard_acquire.
+  /// The init_byte portion of cxa_guard_acquire. Returns true if
+  /// initialization has already been completed.
   /// 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() {
+  bool acquire_init_byte() {
     if (*init_byte_address == COMPLETE_BIT)
-      return INIT_IS_DONE;
+      return true;
     if (*init_byte_address & PENDING_BIT)
       ABORT_WITH_MESSAGE("__cxa_guard_acquire detected recursive initialization");
     *init_byte_address = PENDING_BIT;
-    return INIT_IS_PENDING;
+    return false;
   }
 
   /// 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; }
template <class Mutex, class CondVar, Mutex& global_mutex, CondVar& global_cond,
           uint32_t (*GetThreadID)() = PlatformThreadID>
 struct InitByteGlobalMutex {
 
   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.
+  /// The init_byte portion of cxa_guard_acquire. Returns true if
+  /// initialization has already been completed.
   /// 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() {
+  bool 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;
+      return true;
 
     if (has_thread_id_support)
       *thread_id_address = current_thread_id.get();
 
     *init_byte_address = PENDING_BIT;
-    return INIT_IS_PENDING;
+    return false;
   }
 
   /// 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;
     {
struct InitByteFutex {
 
   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.
+  /// The init_byte portion of cxa_guard_acquire. Returns true if
+  /// initialization has already been completed.
   /// 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() {
+  bool 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;
+        return false;
       }
 
       if (last_val == COMPLETE_BIT)
-        return INIT_IS_DONE;
+        return true;
 
       if (last_val & PENDING_BIT) {
 
         // Check for recursive initialization
         if (has_thread_id_support && thread_id.load(std::_AO_Relaxed) == current_thread_id.get()) {
           ABORT_WITH_MESSAGE("__cxa_guard_acquire detected recursive initialization");
         }
 
         if ((last_val & WAITING_BIT) == 0) {
           // This compare exchange can fail for several reasons
           // (1) another thread finished the whole thing before we got here
           // (2) another thread set the waiting bit we were trying to thread
           // (3) another thread had an exception and failed to finish
           if (!init_byte.compare_exchange(&last_val, PENDING_BIT | WAITING_BIT, std::_AO_Acq_Rel, std::_AO_Release)) {
             // (1) success, via someone else's work!
             if (last_val == COMPLETE_BIT)
-              return INIT_IS_DONE;
+              return true;
 
             // (3) someone else, bailed on doing the work, retry from the start!
             if (last_val == UNSET)
               continue;
 
             // (2) the waiting bit got set, so we are happy to keep waiting
           }
         }
private:
 
   static_assert(Wait != nullptr && Wake != nullptr, "");
 };
 
 //===----------------------------------------------------------------------===//
 //                                   Guards
 //===----------------------------------------------------------------------===//
 
-template <class InitByteT>
-struct GuardBase : GuardByte<GuardBase<InitByteT>>, InitByteT {
-  using GuardByteT = typename GuardBase::GuardByte;
+enum class AcquireResult {
+  INIT_IS_DONE,
+  INIT_IS_PENDING,
+};
+constexpr AcquireResult INIT_IS_DONE = AcquireResult::INIT_IS_DONE;
+constexpr AcquireResult INIT_IS_PENDING = AcquireResult::INIT_IS_PENDING;
 
+/// Co-ordinates between GuardByte and InitByte.
+template <class InitByteT>
+struct GuardBase : GuardByte, InitByteT {
   GuardBase() = delete;
   GuardBase(GuardBase const&) = delete;
   GuardBase& operator=(GuardBase const&) = delete;
 
   /// ARM Constructor
   explicit GuardBase(uint32_t* raw_guard_object)
-      : GuardByteT(reinterpret_cast<uint8_t*>(raw_guard_object)),
+      : GuardByte(reinterpret_cast<uint8_t*>(raw_guard_object)),
         InitByteT(reinterpret_cast<uint8_t*>(raw_guard_object) + 1, nullptr) {}
 
   /// Itanium Constructor
   explicit GuardBase(uint64_t* raw_guard_object)
-      : GuardByteT(reinterpret_cast<uint8_t*>(raw_guard_object)),
+      : GuardByte(reinterpret_cast<uint8_t*>(raw_guard_object)),
         InitByteT(reinterpret_cast<uint8_t*>(raw_guard_object) + 1, reinterpret_cast<uint32_t*>(raw_guard_object) + 1) {
   }
+
+  /// Implements __cxa_guard_acquire.
+  AcquireResult cxa_guard_acquire() {
+    // use short-circuit evalutation to avoid calling acquire_init_byte when
+    // acquire_guard_byte returns true. (i.e. don't call it when we know from
+    // the guard byte that initialization has already been completed)
+    if (acquire_guard_byte() || InitByteT::acquire_init_byte())
+      return INIT_IS_DONE;
+    return INIT_IS_PENDING;
+  }
+
+  /// Implements __cxa_guard_release.
+  void cxa_guard_release() {
+    // Update guard byte first, so if somebody is woken up by release_init_byte
+    // and comes all the way back around to __cxa_guard_acquire again, they see
+    // it as having completed initialization.
+    release_guard_byte();
+    InitByteT::release_init_byte();
+  }
+
+  /// Implements __cxa_guard_abort.
+  void cxa_guard_abort() {
+    abort_guard_byte();
+    InitByteT::abort_init_byte();
+  }
 };
 
 /// NoThreadsGuard - Manages initialization without performing any inter-thread
 /// synchronization.
 struct NoThreadsGuard : GuardBase<InitByteNoThreads> {
   using BaseT = typename NoThreadsGuard::GuardBase;
   using BaseT::BaseT;
 };