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

[Serge Rogatch via Phabricator via llvm-commits]

rSerge added inline comments.


================
Comment at: lib/xray/xray_buffer_queue.cc:25
     void *Tmp = malloc(BufferSize);
-    Buf.Buffer = Tmp;
-    Buf.Size = B;
-    if (Tmp != 0)
-      OwnedBuffers.insert(Tmp);
+    if (Tmp != nullptr) {
+      auto &Buf = std::get<0>(T);
----------------
If `Tmp==nullptr`, it's a memory allocation error that should be reported to the client/outer code (and further `void *Tmp = malloc(BufferSize);` will usually fail).
A good way of doing this would be to throw an exception, but I doubt it's allowed in LLVM because RTTI is banned.
So maybe consider an initialization method returning the error number or true/false to the calling code?


================
Comment at: lib/xray/xray_fdr_logging.cc:186
+    //   - Thread ID (pid_t, 4 bytes)
+    auto &NewBuffer = *reinterpret_cast<MetadataRecord *>(&Records[0]);
+    NewBuffer.Type = RecordType::Metadata;
----------------
Why not to put here simply `auto &NewBuffer = *reinterpret_cast<MetadataRecord *>(Records);` ?


================
Comment at: lib/xray/xray_fdr_logging.cc:195
+    static_assert(sizeof(time_t) == 8, "time_t needs to be 8 bytes");
+    auto &WalltimeMarker = *reinterpret_cast<MetadataRecord *>(&Records[1]);
+    WalltimeMarker.Type = RecordType::Metadata;
----------------
How about `auto &WalltimeMarker = *reinterpret_cast<MetadataRecord *>(Records+1);` ?


================
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);
----------------
dberris wrote:
> 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.
Oh, I missed that it's only since C++14.


================
Comment at: lib/xray/xray_fdr_logging.h:29
+
+enum class RecordType {
+  Function, Metadata
----------------
I think it should be `enum class RecordType : uint8_t {`


================
Comment at: lib/xray/xray_fdr_logging.h:38
+  enum RecordKind : uint8_t {
+    NewBuffer,
+    EndOfBuffer,
----------------
dberris wrote:
> 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).
Ok.


https://reviews.llvm.org/D27038