[PATCH] D21982: [compiler-rt][XRay] Initial per-thread inmemory logging implementation

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: include/xray/xray_records.h:40-42
@@ +39,5 @@
+  // What follows are a set of flags that indicate useful things for when
+  // reading the data in the file.
+  unsigned char ConstantTSC : 1;
+  unsigned char NonstopTSC : 1;
+
----------------
Why not give these bitfields an underlying type of bool?

================
Comment at: lib/xray/xray_inmemory_log.cc:71
@@ +70,3 @@
+  explicit ThreadExitFlusher(int Fd, XRayRecord *Start, size_t &Offset)
+      : Fd(Fd), Start(Start), Offset(Offset) {}
+
----------------
Should we assert that Fd > 0?

================
Comment at: lib/xray/xray_inmemory_log.cc:77
@@ +76,3 @@
+                  reinterpret_cast<char *>(Start + Offset));
+    fsync(Fd);
+  }
----------------
Why do we have this `fsync`? Might deserve a comment.

================
Comment at: lib/xray/xray_inmemory_log.cc:95-99
@@ +94,7 @@
+  char Line[256] = {};
+  int BytesRead;
+  while ((BytesRead = read(Fd, Line, 256)) < 0) {
+    if (errno == EINTR)
+      continue;
+  }
+  close(Fd);
----------------
This seems wrong in the face of partial reads.  Perhaps a retryingRead is called for?


https://reviews.llvm.org/D21982