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

[Dean Michael Berris via llvm-commits]

dberris added inline comments.

================
Comment at: include/xray/xray_records.h:41-42
@@ +40,4 @@
+  // reading the data in the file.
+  bool ConstantTSC = 0;
+  bool NonstopTSC = 0;
+
----------------
majnemer wrote:
> Should these be bitfields?
Good idea, yep.

================
Comment at: lib/xray/xray_inmemory_log.cc:47
@@ +46,3 @@
+
+static void RetryingWrite(int Fd, char *Begin, char *End) {
+  if (Begin == End)
----------------
majnemer wrote:
> Are we following LLVM convention for XRay? If so, I'd make this function start with a lowercase letter.
Yes, we should be -- sorry I haven't quite internalised the rules. :/

================
Comment at: lib/xray/xray_inmemory_log.cc:52-53
@@ +51,4 @@
+  while (auto Written = write(Fd, Begin, TotalBytes)) {
+    if (Written <= 0) {
+      if (errno == EINTR)
+        continue; // Try again.
----------------
majnemer wrote:
> This should be `Written < 0`.  I don't believe `errno` will be set to anything good if `write` returns 0; I think you'd observe a previous `errno` value.
Good catch, thanks. Done.

================
Comment at: lib/xray/xray_inmemory_log.cc:73
@@ +72,3 @@
+
+  ~ThreadExitFlusher() noexcept {
+    std::lock_guard<std::mutex> L(LogMutex);
----------------
majnemer wrote:
> Why `noexcept`?
Habbit... removed.

================
Comment at: lib/xray/xray_inmemory_log.cc:91
@@ +90,3 @@
+bool ReadValueFromFile(const char *Filename, long *Value) {
+  int Fd = open(Filename, O_RDONLY);
+  if (Fd == -1)
----------------
majnemer wrote:
> Should we pass in O_CLOEXEC?
Good idea -- yes, done.

================
Comment at: lib/xray/xray_inmemory_log.cc:95-96
@@ +94,4 @@
+  char Line[256] = {};
+  read(Fd, Line, 256);
+  close(Fd);
+  char *End = nullptr;
----------------
majnemer wrote:
> How do you feel about these potentially failing?
Not very good... is it better now?


https://reviews.llvm.org/D21982