[compiler-rt] r346474 - [XRay] Add atomic fences around non-atomic reads and writes

[Dean Michael Berris via llvm-commits]

Author: dberris
Date: Thu Nov  8 22:39:45 2018
New Revision: 346474

URL: http://llvm.org/viewvc/llvm-project?rev=346474&view=rev
Log:
[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

Modified:
    compiler-rt/trunk/lib/xray/xray_fdr_log_writer.h
    compiler-rt/trunk/lib/xray/xray_fdr_logging.cc

Modified: compiler-rt/trunk/lib/xray/xray_fdr_log_writer.h
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/xray/xray_fdr_log_writer.h?rev=346474&r1=346473&r2=346474&view=diff
==============================================================================
--- compiler-rt/trunk/lib/xray/xray_fdr_log_writer.h (original)
+++ compiler-rt/trunk/lib/xray/xray_fdr_log_writer.h Thu Nov  8 22:39:45 2018
@@ -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;
   }

Modified: compiler-rt/trunk/lib/xray/xray_fdr_logging.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/xray/xray_fdr_logging.cc?rev=346474&r1=346473&r2=346474&view=diff
==============================================================================
--- compiler-rt/trunk/lib/xray/xray_fdr_logging.cc (original)
+++ compiler-rt/trunk/lib/xray/xray_fdr_logging.cc Thu Nov  8 22:39:45 2018
@@ -243,6 +243,13 @@ XRayBuffer fdrIterator(const XRayBuffer
   // 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);