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

Dean Michael Berris via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Dec 18 17:48:16 PST 2016

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.


More information about the llvm-commits mailing list