[PATCH] D27038: [XRay][compiler-rt] XRay Flight Data Recorder Mode

[Serge Rogatch via Phabricator via llvm-commits]

rSerge requested changes to this revision.
rSerge added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/xray/CMakeLists.txt:14-22
+set(x86_64_SOURCES
+    xray_x86_64.cc
+    xray_trampoline_x86_64.S
+    ${XRAY_SOURCES})
 
 set(x86_64_SOURCES
     xray_x86_64.cc
----------------
Is it a hiccup in the diff? Or is there really a duplication of `set(x86_64_SOURCES`?


================
Comment at: lib/xray/xray_buffer_queue.cc:53
   std::lock_guard<std::mutex> Guard(Mutex);
-  Buffers.push_back(Buf);
+  Buffers.emplace(Buffers.end(), Buf, true);
   Buf.Buffer = nullptr;
----------------
I think here should be either a comment on what `true` means, or usage of dedicated enum like `BufferUsageType` with values `Used` and `Unused`.


================
Comment at: lib/xray/xray_buffer_queue.cc:68
+    auto &Buf = std::get<0>(T);
+    if (Buf.Buffer != nullptr)
+      free(Buf.Buffer);
----------------
I think the standard requires that `free(nullptr)` is safe in all C implementations.


================
Comment at: lib/xray/xray_buffer_queue.h:42
   std::size_t BufferSize;
-  std::deque<Buffer> Buffers;
+  std::deque<std::tuple<Buffer, bool>> Buffers;
   std::mutex Mutex;
----------------
A comment here about the meaning of `bool` part would help.


================
Comment at: lib/xray/xray_fdr_logging.h:38
+  enum RecordKind : uint8_t {
+    NewBuffer,
+    EndOfBuffer,
----------------
I think the base value should be specified here, I doubt it defaults to 0 in all C++ implementations. The base value would help ensuring that enum values fit 7 bits.


https://reviews.llvm.org/D27038