[XRay] Use more portable control block

[dberris]

Summary

In D53560, we assumed a specific layout for memory without using an
explicit structure. This follow-up change uses more portable layout
control by using unions in a struct, and consolidating the memory
management code in the buffer queue.

We also take the opportunity to improve the documentation on the types
and operations, along with simplifying some of the logic in the buffer
queue implementation.

[mboerger]

LGTM

compiler-rt/trunk/lib/xray/tests/unit/buffer_queue_test.cc

 //===-- buffer_queue_test.cc ----------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of XRay, a function call tracing system.
 //
 //===----------------------------------------------------------------------===//
 #include "xray_buffer_queue.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #include <atomic>
 #include <future>
 #include <thread>
 #include <unistd.h>
 
 namespace __xray {
+namespace {
 
 static constexpr size_t kSize = 4096;
 
+using ::testing::Eq;
+
 TEST(BufferQueueTest, API) {
   bool Success = false;
   BufferQueue Buffers(kSize, 1, Success);
   ASSERT_TRUE(Success);
 }
 
 TEST(BufferQueueTest, GetAndRelease) {
   bool Success = false;

 TEST(BufferQueueTest, ReleaseUnknown) {
   bool Success = false;
   BufferQueue Buffers(kSize, 1, Success);
   ASSERT_TRUE(Success);
   BufferQueue::Buffer Buf;
   Buf.Data = reinterpret_cast<void *>(0xdeadbeef);
   Buf.Size = kSize;
   Buf.Generation = Buffers.generation();
-  EXPECT_EQ(BufferQueue::ErrorCode::UnrecognizedBuffer,
-            Buffers.releaseBuffer(Buf));
+
+  BufferQueue::Buffer Known;
+  EXPECT_THAT(Buffers.getBuffer(Known), Eq(BufferQueue::ErrorCode::Ok));
+  EXPECT_THAT(Buffers.releaseBuffer(Buf),
+              Eq(BufferQueue::ErrorCode::UnrecognizedBuffer));
+  EXPECT_THAT(Buffers.releaseBuffer(Known), Eq(BufferQueue::ErrorCode::Ok));
 }
 
 TEST(BufferQueueTest, ErrorsWhenFinalising) {
   bool Success = false;
   BufferQueue Buffers(kSize, 2, Success);
   ASSERT_TRUE(Success);
   BufferQueue::Buffer Buf;
   ASSERT_EQ(Buffers.getBuffer(Buf), BufferQueue::ErrorCode::Ok);
TEST(BufferQueueTest, GenerationalSupportAcrossThreads) {
   EXPECT_EQ(Buffers.init(kSize, 10), BufferQueue::ErrorCode::Ok);
 
   T0.join();
   T1.join();
 
   ASSERT_EQ(Counter.load(std::memory_order_acquire), 0);
 }
 
+} // namespace
 } // namespace __xray

compiler-rt/trunk/lib/xray/xray_buffer_queue.h

 
 /// BufferQueue implements a circular queue of fixed sized buffers (much like a
 /// freelist) but is concerned with making it quick to initialise, finalise, and
 /// get from or return buffers to the queue. This is one key component of the
 /// "flight data recorder" (FDR) mode to support ongoing XRay function call
 /// trace collection.
 class BufferQueue {
 public:
+  /// ControlBlock represents the memory layout of how we interpret the backing
+  /// store for all buffers managed by a BufferQueue instance. The ControlBlock
+  /// has the reference count as the first member, sized according to
+  /// platform-specific cache-line size. We never use the Buffer member of the
+  /// union, which is only there for compiler-supported alignment and sizing.
+  ///
+  /// This ensures that the `Data` member will be placed at least kCacheLineSize
+  /// bytes from the beginning of the structure.
+  struct ControlBlock {
+    union {
+      atomic_uint64_t RefCount;
+      char Buffer[kCacheLineSize];
+    };
+
+    /// We need to make this size 1, to conform to the C++ rules for array data
+    /// members. Typically, we want to subtract this 1 byte for sizing
+    /// information.
+    char Data[1];
+  };
+
   struct Buffer {
     atomic_uint64_t Extents{0};
     uint64_t Generation{0};
     void *Data = nullptr;
     size_t Size = 0;
 
   private:
     friend class BufferQueue;
-    unsigned char *BackingStore = nullptr;
+    ControlBlock *BackingStore = nullptr;
     size_t Count = 0;
   };
 
   struct BufferRep {
     // The managed buffer.
     Buffer Buff;
 
     // This is true if the buffer has been returned to the available queue, and
private:
   size_t BufferSize;
 
   // Amount of pre-allocated buffers.
   size_t BufferCount;
 
   SpinMutex Mutex;
   atomic_uint8_t Finalizing;
 
-  // A pointer to a contiguous block of memory to serve as the backing store for
-  // all the individual buffers handed out.
-  uint8_t *BackingStore;
+  // The collocated ControlBlock and buffer storage.
+  ControlBlock *BackingStore;
 
   // A dynamically allocated array of BufferRep instances.
   BufferRep *Buffers;
 
   // Pointer to the next buffer to be handed out.
   BufferRep *Next;
 
   // Pointer to the entry in the array where the next released buffer will be

compiler-rt/trunk/lib/xray/xray_buffer_queue.cc

 #include <memory>
 #include <sys/mman.h>
 
 using namespace __xray;
 using namespace __sanitizer;
 
 namespace {
 
-void decRefCount(unsigned char *ControlBlock, size_t Size, size_t Count) {
-  if (ControlBlock == nullptr)
+BufferQueue::ControlBlock *allocControlBlock(size_t Size, size_t Count) {
+  auto B =
+      allocateBuffer((sizeof(BufferQueue::ControlBlock) - 1) + (Size * Count));
+  return B == nullptr ? nullptr
+                      : reinterpret_cast<BufferQueue::ControlBlock *>(B);
+}
+
+void deallocControlBlock(BufferQueue::ControlBlock *C, size_t Size,
+                         size_t Count) {
+  deallocateBuffer(reinterpret_cast<unsigned char *>(C),
+                   (sizeof(BufferQueue::ControlBlock) - 1) + (Size * Count));
+}
+
+void decRefCount(BufferQueue::ControlBlock *C, size_t Size, size_t Count) {
+  if (C == nullptr)
     return;
-  auto *RefCount = reinterpret_cast<atomic_uint64_t *>(ControlBlock);
-  if (atomic_fetch_sub(RefCount, 1, memory_order_acq_rel) == 1)
-    deallocateBuffer(ControlBlock, (Size * Count) + kCacheLineSize);
+  if (atomic_fetch_sub(&C->RefCount, 1, memory_order_acq_rel) == 1)
+    deallocControlBlock(C, Size, Count);
 }
 
-void incRefCount(unsigned char *ControlBlock) {
-  if (ControlBlock == nullptr)
+void incRefCount(BufferQueue::ControlBlock *C) {
+  if (C == nullptr)
     return;
-  auto *RefCount = reinterpret_cast<atomic_uint64_t *>(ControlBlock);
-  atomic_fetch_add(RefCount, 1, memory_order_acq_rel);
+  atomic_fetch_add(&C->RefCount, 1, memory_order_acq_rel);
 }
 
 } // namespace
 
 BufferQueue::ErrorCode BufferQueue::init(size_t BS, size_t BC) {
   SpinMutexLock Guard(&Mutex);
 
   if (!finalizing())
     return BufferQueue::ErrorCode::AlreadyInitialized;
 
   cleanupBuffers();
 
   bool Success = false;
   BufferSize = BS;
   BufferCount = BC;
-  BackingStore = allocateBuffer((BufferSize * BufferCount) + kCacheLineSize);
+
+  BackingStore = allocControlBlock(BufferSize, BufferCount);
   if (BackingStore == nullptr)
     return BufferQueue::ErrorCode::NotEnoughMemory;
 
   auto CleanupBackingStore = __sanitizer::at_scope_exit([&, this] {
     if (Success)
       return;
-    deallocateBuffer(BackingStore, (BufferSize * BufferCount) + kCacheLineSize);
+    deallocControlBlock(BackingStore, BufferSize, BufferCount);
     BackingStore = nullptr;
   });
 
   Buffers = initArray<BufferRep>(BufferCount);
   if (Buffers == nullptr)
     return BufferQueue::ErrorCode::NotEnoughMemory;
 
   // At this point we increment the generation number to associate the buffers
   // to the new generation.
   atomic_fetch_add(&Generation, 1, memory_order_acq_rel);
 
-  Success = true;
-
-  // First, we initialize the refcount in the RefCountedBackingStore, which we
-  // treat as being at the start of the BackingStore pointer.
-  auto ControlBlock = reinterpret_cast<atomic_uint64_t *>(BackingStore);
-  atomic_store(ControlBlock, 1, memory_order_release);
-
+  // First, we initialize the refcount in the ControlBlock, which we treat as
+  // being at the start of the BackingStore pointer.
+  atomic_store(&BackingStore->RefCount, 1, memory_order_release);
+
+  // Then we initialise the individual buffers that sub-divide the whole backing
+  // store. Each buffer will start at the `Data` member of the ControlBlock, and
+  // will be offsets from these locations.
   for (size_t i = 0; i < BufferCount; ++i) {
     auto &T = Buffers[i];
     auto &Buf = T.Buff;
     atomic_store(&Buf.Extents, 0, memory_order_release);
     Buf.Generation = generation();
-    Buf.Data = BackingStore + kCacheLineSize + (BufferSize * i);
+    Buf.Data = &BackingStore->Data + (BufferSize * i);
     Buf.Size = BufferSize;
     Buf.BackingStore = BackingStore;
     Buf.Count = BufferCount;
     T.Used = false;
   }
 
   Next = Buffers;
   First = Buffers;
   LiveBuffers = 0;
   atomic_store(&Finalizing, 0, memory_order_release);
+  Success = true;
   return BufferQueue::ErrorCode::Ok;
 }
 
 BufferQueue::BufferQueue(size_t B, size_t N,
                          bool &Success) XRAY_NEVER_INSTRUMENT
     : BufferSize(B),
       BufferCount(N),
       Mutex(),
if (LiveBuffers == BufferCount)
       return ErrorCode::NotEnoughMemory;
     B = Next++;
     if (Next == (Buffers + BufferCount))
       Next = Buffers;
     ++LiveBuffers;
   }
 
   incRefCount(BackingStore);
-  Buf.Data = B->Buff.Data;
+  Buf = B->Buff;
   Buf.Generation = generation();
-  Buf.Size = B->Buff.Size;
-  Buf.BackingStore = BackingStore;
-  Buf.Count = BufferCount;
   B->Used = true;
   return ErrorCode::Ok;
 }
 
 BufferQueue::ErrorCode BufferQueue::releaseBuffer(Buffer &Buf) {
   // Check whether the buffer being referred to is within the bounds of the
   // backing store's range.
   BufferRep *B = nullptr;
   {
     SpinMutexLock Guard(&Mutex);
-    if (Buf.Data < BackingStore ||
-        Buf.Data > reinterpret_cast<char *>(BackingStore) +
-                       (BufferCount * BufferSize)) {
-      if (Buf.Generation != generation()) {
-        decRefCount(Buf.BackingStore, Buf.Size, Buf.Count);
-        Buf.Data = nullptr;
-        Buf.Size = 0;
-        Buf.Generation = 0;
-        Buf.Count = 0;
-        Buf.BackingStore = nullptr;
-        return BufferQueue::ErrorCode::Ok;
-      }
-      return BufferQueue::ErrorCode::UnrecognizedBuffer;
-    }
+    if (Buf.Generation != generation() || LiveBuffers == 0) {
+      Buf = {};
 
-    if (LiveBuffers == 0) {
-      decRefCount(Buf.BackingStore, Buf.Size, Buf.Count);
-      Buf.Data = nullptr;
-      Buf.Size = Buf.Size;
-      Buf.Generation = 0;
-      Buf.BackingStore = nullptr;
-      Buf.Count = 0;
-      return ErrorCode::Ok;
-    }
+    if (Buf.Data < &BackingStore->Data ||
+        Buf.Data > &BackingStore->Data + (BufferCount * BufferSize))
+      return BufferQueue::ErrorCode::UnrecognizedBuffer;
 
     --LiveBuffers;
     B = First++;
     if (First == (Buffers + BufferCount))
       First = Buffers;
   }
 
   // Now that the buffer has been released, we mark it as "used".
-  B->Buff.Data = Buf.Data;
-  B->Buff.Size = Buf.Size;
-  B->Buff.Generation = Buf.Generation;
-  B->Buff.BackingStore = Buf.BackingStore;
-  B->Buff.Count = Buf.Count;
+  B->Buff = Buf;
   B->Used = true;
   decRefCount(Buf.BackingStore, Buf.Size, Buf.Count);
   atomic_store(&B->Buff.Extents,
                atomic_load(&Buf.Extents, memory_order_acquire),
                memory_order_release);
-  Buf.Data = nullptr;
-  Buf.Size = 0;
-  Buf.Generation = 0;
-  Buf.BackingStore = nullptr;
-  Buf.Count = 0;
+  Buf = {};
   return ErrorCode::Ok;
 }
 
 BufferQueue::ErrorCode BufferQueue::finalize() {
   if (atomic_exchange(&Finalizing, 1, memory_order_acq_rel))
     return ErrorCode::QueueFinalizing;
   return ErrorCode::Ok;
 }