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

Dean Michael Berris via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 13 21:39:12 PST 2016

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


More information about the llvm-commits mailing list