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

[dberris]

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.

[efriedma]

What is the point of this change? The atomic_fetch_add operations already use memory_order_acq_rel, so the explicit fences are redundant.

[jfb]

In D54291#1293418, @efriedma wrote:

What is the point of this change? The atomic_fetch_add operations already use memory_order_acq_rel, so the explicit fences are redundant.

Agreed, these are all redundant. Ideally the compiler just eliminates the fences, and your change was a no-op :-)

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

 
 class FDRLogWriter {
   BufferQueue::Buffer &Buffer;
   char *NextRecord = nullptr;
 
   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);
   }
 
 public:
   explicit FDRLogWriter(BufferQueue::Buffer &B, char *P)
       : Buffer(B), NextRecord(P) {
     DCHECK_NE(Buffer.Data, nullptr);
     DCHECK_NE(NextRecord, nullptr);
bool writeMetadata(Data &&... Ds) {
     writeRecord(createMetadataRecord<Kind>(std::forward<Data>(Ds)...));
     return true;
   }
 
   template <size_t N> size_t writeMetadataRecords(MetadataRecord (&Recs)[N]) {
     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;
   }
 
   enum class FunctionRecordKind : uint8_t {
     Enter = 0x00,
     Exit = 0x01,
     TailExit = 0x02,
bool writeFunctionWithArg(FunctionRecordKind Kind, int32_t FuncId,
     MetadataRecord A =
         createMetadataRecord<MetadataRecord::RecordKinds::CallArgument>(Arg);
     NextRecord = reinterpret_cast<char *>(internal_memcpy(
                      NextRecord, reinterpret_cast<char *>(&R), sizeof(R))) +
                  sizeof(R);
     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;
   }
 
   bool writeCustomEvent(int32_t Delta, const void *Event, int32_t EventSize) {
     // We write the metadata record and the custom event data into the buffer
     // first, before we atomically update the extents for the buffer. This
     // allows us to ensure that any threads reading the extents of the buffer
     // will only ever see the full metadata and custom event payload accounted
     // (no partial writes accounted).
     MetadataRecord R =
         createMetadataRecord<MetadataRecord::RecordKinds::CustomEventMarker>(
             EventSize, Delta);
     NextRecord = reinterpret_cast<char *>(internal_memcpy(
                      NextRecord, reinterpret_cast<char *>(&R), sizeof(R))) +
                  sizeof(R);
     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;
   }
 
   bool writeTypedEvent(int32_t Delta, uint16_t EventType, const void *Event,
                        int32_t EventSize) {
     // We do something similar when writing out typed events, see
     // writeCustomEvent(...) above for details.
     MetadataRecord R =
         createMetadataRecord<MetadataRecord::RecordKinds::TypedEventMarker>(
             EventSize, Delta, EventType);
     NextRecord = reinterpret_cast<char *>(internal_memcpy(
                      NextRecord, reinterpret_cast<char *>(&R), sizeof(R))) +
                  sizeof(R);
     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;
   }
 
   char *getNextRecord() const { return NextRecord; }
 
   void resetRecord() {
     NextRecord = reinterpret_cast<char *>(Buffer.Data);

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

XRayBuffer fdrIterator(const XRayBuffer B) {
 
   if (It == End)
     return {nullptr, 0};
 
   // Set up the current buffer to contain the extents like we would when writing
   // 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);
   if (CurrentBuffer == nullptr)
     return {nullptr, 0};
 
   // Write out the extents as a Metadata Record into the CurrentBuffer.
   MetadataRecord ExtentsRecord;