[libcxxabi] Pulled guard byte code out of GuardObject

[DanielMcIntosh-IBM]

Summary

Right now, GuardObject is in charge of both reading and writing to the guard
byte, and co-ordinating with the InitByte... classes. In order to improve
separation of concerns, create a separate class responsible for managing the
guard byte and use that inside GuardObject.

This is the 3rd of 5 changes to overhaul cxa_guard.
See D108343 for what the final result will be.

Depends on D110088

[DanielMcIntosh-IBM]

Fix copy-paste error from re-organization of reviews

[ldionne]

libcxxabi/src/cxa_guard_impl.h

15

This is confusing me -- does the compiler really check the guard byte? Isn't it what we do in the library when we do guard_byte.load(std::_AO_Acquire) != UNSET?

232

[DanielMcIntosh-IBM]

Update comments

[DanielMcIntosh-IBM]

libcxxabi/src/cxa_guard_impl.h

15

Yes, for performance reasons we check it inline before calling __cxa_guard_acquire: https://godbolt.org/z/hY1555Wcx. I believe in clang this is generated around clang/lib/CodeGen/ItaniumCXXABI.cpp:2470. As you point out, we also check it inside __cxa_guard_acquire, I assume this is in case the library is used with a compiler that doesn't generate the inline check (or something along those lines).

[ldionne]

Please rebase onto main and make sure you have a green CI run.

libcxxabi/src/cxa_guard_impl.h

15

Thanks for the information!

[DanielMcIntosh-IBM]

No change, just rebase and re-trigger CI to update base revision

libcxxabi/src/cxa_guard_impl.h

 //===----------------------------------------------------------------------===//
 //
 // 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 LIBCXXABI_SRC_INCLUDE_CXA_GUARD_IMPL_H
 #define LIBCXXABI_SRC_INCLUDE_CXA_GUARD_IMPL_H
 
 /* cxa_guard_impl.h - Implements the C++ runtime support for function local
  * static guards.
  * The layout of the guard object is the same across ARM and Itanium.
  *
  * The first "guard byte" (which is checked by the compiler) is set only upon
  * the completion of cxa release.
  *
  * The second "init byte" does the rest of the bookkeeping. It tracks if
  * initialization is complete or pending, and if there are waiting threads.
  *
  * If the guard variable is 64-bits and the platforms supplies a 32-bit thread
  * identifier, it is used to detect recursive initialization. The thread ID of
  * the thread currently performing initialization is stored in the second word.
uint32_t PlatformThreadID() {
   static_assert(sizeof(pid_t) == sizeof(uint32_t), "");
   return static_cast<uint32_t>(syscall(SYS_gettid));
 }
 #else
 constexpr uint32_t (*PlatformThreadID)() = nullptr;
 #endif
 
 //===----------------------------------------------------------------------===//
+//                          GuardByte
+//===----------------------------------------------------------------------===//
+
+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.
+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:
+  /// 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() {
+    // if guard_byte is non-zero, we have already completed initialization
+    // (i.e. release has been called)
+    return guard_byte.load(std::_AO_Acquire) != UNSET;
+  }
+
+  /// 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.store(COMPLETE_BIT, std::_AO_Release); }
+
+  /// The guard byte portion of cxa_guard_abort.
+  void abort() {} // Nothing to do
+
+private:
+  AtomicInt<uint8_t> guard_byte;
+};
+
+//===----------------------------------------------------------------------===//
 //                          GuardBase
 //===----------------------------------------------------------------------===//
 
 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);
-
 template <class Derived>
 struct GuardObject {
   GuardObject() = delete;
   GuardObject(GuardObject const&) = delete;
   GuardObject& operator=(GuardObject const&) = delete;
 
+private:
+  GuardByte guard_byte;
+
+public:
+  /// ARM Constructor
   explicit GuardObject(uint32_t* g)
-      : base_address(g), guard_byte_address(reinterpret_cast<uint8_t*>(g)),
+      : guard_byte(reinterpret_cast<uint8_t*>(g)), base_address(g),
         init_byte_address(reinterpret_cast<uint8_t*>(g) + 1), thread_id_address(nullptr) {}
 
+  /// Itanium Constructor
   explicit GuardObject(uint64_t* g)
-      : base_address(g), guard_byte_address(reinterpret_cast<uint8_t*>(g)),
+      : guard_byte(reinterpret_cast<uint8_t*>(g)), base_address(g),
         init_byte_address(reinterpret_cast<uint8_t*>(g) + 1), thread_id_address(reinterpret_cast<uint32_t*>(g) + 1) {}
 
-public:
-  /// Implements __cxa_guard_acquire
+  /// Implements __cxa_guard_acquire.
   AcquireResult cxa_guard_acquire() {
-    AtomicInt<uint8_t> guard_byte(guard_byte_address);
-    if (guard_byte.load(std::_AO_Acquire) != UNSET)
+    if (guard_byte.acquire())
       return INIT_IS_DONE;
     return derived()->acquire_init_byte();
   }
 
-  /// Implements __cxa_guard_release
+  /// 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);
+    // 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.
+    guard_byte.release();
     derived()->release_init_byte();
   }
 
-  /// Implements __cxa_guard_abort
-  void cxa_guard_abort() { derived()->abort_init_byte(); }
+  /// Implements __cxa_guard_abort.
+  void cxa_guard_abort() {
+    guard_byte.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:
   Derived* derived() { return static_cast<Derived*>(this); }