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

Serge Rogatch via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 13 05:44:48 PST 2016

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
+    xray_x86_64.cc
+    xray_trampoline_x86_64.S
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.


More information about the llvm-commits mailing list