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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

Thanks for the review @rSerge -- please have another look.



================
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);
----------------
rSerge wrote:
> 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?
Good point. Added the bool output parameter instead.


================
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;
----------------
rSerge wrote:
> Why not to put here simply `auto &NewBuffer = *reinterpret_cast<MetadataRecord *>(Records);` ?
The explicit version here makes it clear that Records is an array, and so we'd like to make it clear that we're getting the address of the first element.



================
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;
----------------
rSerge wrote:
> How about `auto &WalltimeMarker = *reinterpret_cast<MetadataRecord *>(Records+1);` ?
The pointer arithmetic seems less clear than taking the address of the object at index 1 of the Records array.


https://reviews.llvm.org/D27038