From aeae5450649b6aa58570b74ace273e3bba980d22 Mon Sep 17 00:00:00 2001
From: Dean Michael Berris <dberris@google.com>
Date: Fri, 9 Nov 2018 06:39:45 +0000
Subject: [PATCH] [XRay] Add atomic fences around non-atomic reads and writes

Summary:
We need these fences to ensure that other threads attempting to read
bytes in the buffer will see thw writes committed before the extents are
updated. Without these, the writes can be un-committed by the time the
buffer extents counter is updated -- the fences should ensure that the
records written into the log have completed by the time we observe the
buffer extents from different threads.

Reviewers: mboerger

Subscribers: jfb, llvm-commits

Differential Revision: https://reviews.llvm.org/D54291

llvm-svn: 346474
---
 compiler-rt/lib/xray/xray_fdr_log_writer.h | 22 ++++++++++++++++++++++
 compiler-rt/lib/xray/xray_fdr_logging.cc   |  7 +++++++
 2 files changed, 29 insertions(+)

diff --git a/compiler-rt/lib/xray/xray_fdr_log_writer.h b/compiler-rt/lib/xray/xray_fdr_log_writer.h
index 67675ec3d12f..c18ff7bbf0be 100644
--- a/compiler-rt/lib/xray/xray_fdr_log_writer.h
+++ b/compiler-rt/lib/xray/xray_fdr_log_writer.h
@@ -63,6 +63,10 @@ class FDRLogWriter {
   template <class T> void writeRecord(const T &R) {
     internal_memcpy(NextRecord, reinterpret_cast<const char *>(&R), sizeof(T));
     NextRecord += sizeof(T);
+    // We need this atomic fence here to ensure that other threads attempting to
+    // read the bytes in the buffer will see the writes committed before the
+    // extents are updated.
+    atomic_thread_fence(memory_order_release);
     atomic_fetch_add(&Buffer.Extents, sizeof(T), memory_order_acq_rel);
   }
 
@@ -89,6 +93,10 @@ public:
     constexpr auto Size = sizeof(MetadataRecord) * N;
     internal_memcpy(NextRecord, reinterpret_cast<const char *>(Recs), Size);
     NextRecord += Size;
+    // We need this atomic fence here to ensure that other threads attempting to
+    // read the bytes in the buffer will see the writes committed before the
+    // extents are updated.
+    atomic_thread_fence(memory_order_release);
     atomic_fetch_add(&Buffer.Extents, Size, memory_order_acq_rel);
     return Size;
   }
@@ -129,6 +137,10 @@ public:
     NextRecord = reinterpret_cast<char *>(internal_memcpy(
                      NextRecord, reinterpret_cast<char *>(&A), sizeof(A))) +
                  sizeof(A);
+    // We need this atomic fence here to ensure that other threads attempting to
+    // read the bytes in the buffer will see the writes committed before the
+    // extents are updated.
+    atomic_thread_fence(memory_order_release);
     atomic_fetch_add(&Buffer.Extents, sizeof(R) + sizeof(A),
                      memory_order_acq_rel);
     return true;
@@ -149,6 +161,11 @@ public:
     NextRecord = reinterpret_cast<char *>(
                      internal_memcpy(NextRecord, Event, EventSize)) +
                  EventSize;
+
+    // We need this atomic fence here to ensure that other threads attempting to
+    // read the bytes in the buffer will see the writes committed before the
+    // extents are updated.
+    atomic_thread_fence(memory_order_release);
     atomic_fetch_add(&Buffer.Extents, sizeof(R) + EventSize,
                      memory_order_acq_rel);
     return true;
@@ -167,6 +184,11 @@ public:
     NextRecord = reinterpret_cast<char *>(
                      internal_memcpy(NextRecord, Event, EventSize)) +
                  EventSize;
+
+    // We need this atomic fence here to ensure that other threads attempting to
+    // read the bytes in the buffer will see the writes committed before the
+    // extents are updated.
+    atomic_thread_fence(memory_order_release);
     atomic_fetch_add(&Buffer.Extents, EventSize, memory_order_acq_rel);
     return true;
   }
diff --git a/compiler-rt/lib/xray/xray_fdr_logging.cc b/compiler-rt/lib/xray/xray_fdr_logging.cc
index 83f4f97a2b41..f1eca5ce9269 100644
--- a/compiler-rt/lib/xray/xray_fdr_logging.cc
+++ b/compiler-rt/lib/xray/xray_fdr_logging.cc
@@ -243,6 +243,13 @@ XRayBuffer fdrIterator(const XRayBuffer B) {
   // out to disk. The difference here would be that we still write "empty"
   // buffers, or at least go through the iterators faithfully to let the
   // handlers see the empty buffers in the queue.
+  //
+  // We need this atomic fence here to ensure that writes happening to the
+  // buffer have been committed before we load the extents atomically. Because
+  // the buffer is not explicitly synchronised across threads, we rely on the
+  // fence ordering to ensure that writes we expect to have been completed
+  // before the fence are fully committed before we read the extents.
+  atomic_thread_fence(memory_order_acquire);
   auto BufferSize = atomic_load(&It->Extents, memory_order_acquire);
   SerializedBufferSize = BufferSize + sizeof(MetadataRecord);
   CurrentBuffer = allocateBuffer(SerializedBufferSize);