[PATCH] D21982: WIP: Implement a per-thread inmemory log

Mehdi AMINI via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 5 11:52:41 PDT 2016


mehdi_amini added inline comments.

================
Comment at: lib/xray/xray_inmemory_log.cc:63
@@ +62,3 @@
+static void RetryingWrite(int Fd, XRayRecord *Start, size_t Offset) {
+  if (Offset) {
+    auto TotalBytes = Offset * sizeof(XRayRecord);
----------------
Early exit:

```
if (!Offset) return;
```

================
Comment at: lib/xray/xray_inmemory_log.cc:65
@@ +64,3 @@
+    auto TotalBytes = Offset * sizeof(XRayRecord);
+    while (auto Written = write(Fd, Start, sizeof(XRayRecord) * Offset)) {
+      if (Written == -1) {
----------------
This while condition isn't clear to me: Written would be 0 if nothing is written, which seem unexpected.
What about a `while (1)`?

================
Comment at: lib/xray/xray_inmemory_log.cc:74
@@ +73,3 @@
+      if (TotalBytes == 0)
+        break;
+    }
----------------
If you want to handle partial write properly, I think you need to update start and offset as well.

I feel the interface for `RetryingWrite` would be more usual of C++ std algorithm with a Begin / End instead of start/offset.

================
Comment at: lib/xray/xray_inmemory_log.cc:108
@@ +107,3 @@
+      printf("Failed opening temporary file '%s'; not logging events.",
+             TmpFilename);
+      return -1;
----------------
Shouldn't error be printed to stderr?

================
Comment at: lib/xray/xray_inmemory_log.cc:111
@@ +110,3 @@
+    }
+    printf("XRay: Log file in '%s'\n", TmpFilename);
+    return Fd;
----------------
This looks like debugging, is it intended to be here? 
Do you really want unconditionally always on? And is stdout the right output?


http://reviews.llvm.org/D21982





More information about the llvm-commits mailing list