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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

Thanks @rserge -- you made me look into the C++11 standard, and find a good way of doing the bit-fields. PTAL



================
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
----------------
rSerge wrote:
> Is it a hiccup in the diff? Or is there really a duplication of `set(x86_64_SOURCES`?
Nope that's a merge failure on my part. :)


================
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;
----------------
rSerge wrote:
> I think here should be either a comment on what `true` means, or usage of dedicated enum like `BufferUsageType` with values `Used` and `Unused`.
An enum seems overkill here. Added a comment in both the data member declaration and here.


================
Comment at: lib/xray/xray_fdr_logging.cc:57
+  BQ = std::make_shared<BufferQueue>(BufferSize, BufferMax);
+  FDROptions.reset(new FDRLoggingOptions());
+  *FDROptions = *reinterpret_cast<FDRLoggingOptions *>(Options);
----------------
rSerge wrote:
> Wouldn't it be better to implement both via `.reset(new ...)`, or use `std::make_unique` here?
We don't have `std::make_unique` yet, since I think we're still stuck with C++11.


================
Comment at: lib/xray/xray_fdr_logging.h:38
+  enum RecordKind : uint8_t {
+    NewBuffer,
+    EndOfBuffer,
----------------
rSerge wrote:
> 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.
A standard-conforming C++ implementation ought to set the first element's value to be zero (in [decl.enum]p2 on the C++11 FCD).


================
Comment at: lib/xray/xray_fdr_logging.h:44
+  };
+  int RecordKind : 7; // Use 7 bits to identify this record type.
+  char Data[15];
----------------
rSerge wrote:
> If `int` is used here and for `Type` member, doesn't the structure take 19 bytes at least with some compilers? (because `int` bitfields take at least 4 bytes, I suppose). Can you change the type to `uint8_t` for these 2 members?
Good point -- I did one better I think, since we can actually use enum classes for the fields that do need enums -- so instead of working around on raw values and bytes, we can use the compiler's help to make this work "better" or at least be semantically correct.


https://reviews.llvm.org/D27038