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

[Serge Rogatch via llvm-commits]

rSerge added a subscriber: rSerge.
rSerge requested changes to this revision.
rSerge added a reviewer: rSerge.
rSerge added a comment.
This revision now requires changes to proceed.

Please, see inline.


================
Comment at: include/xray/xray_records.h:47
@@ +46,3 @@
+  char Padding[15] = {};
+} __attribute__((packed));
+
----------------
So `CycleFrequency` is an 8-byte variable not aligned to 8 bytes. This may cause portability issues to other CPUs, not only performance drops for unaligned memory access, but they say on some CPUs unaligned access is completely forbidden.

================
Comment at: lib/xray/xray_inmemory_log.cc:71
@@ +70,3 @@
+  ssize_t BytesRead;
+  while ((BytesRead = read(Fd, Begin, BytesToRead)) < 0) {
+    if (errno == EINTR)
----------------
So the loop runs while `BytesRead<0` ? Shouldn't it run while `BytesToRead > 0`?
Then in an `if`, before checking `errno`, you can check whether `BytesRead < 0`.

================
Comment at: lib/xray/xray_inmemory_log.cc:90
@@ +89,3 @@
+  char Line[256] = {};
+  if (!retryingRead(Fd, Line, Line + 256))
+    return false;
----------------
So `retryingWrite` writes all bytes, while `retryingRead` reads some bytes up to the given number of bytes, right? Then their parameters should indicate this, or the functions should be renamed to e.g. `retryingWriteAll` and `retryingReadSome` .

================
Comment at: lib/xray/xray_inmemory_log.cc:130
@@ +129,3 @@
+
+void PrintToStdErr(const char *Buffer) { fprintf(stderr, Buffer); }
+
----------------
This crashes if `Buffer` contains formatting, e.g. `%d` (random memory read) or worse `%n` (random memory write), but no formatting arguments are given. Consider changing to `fprintf(stderr, "%s", Buffer);

================
Comment at: lib/xray/xray_inmemory_log.cc:135
@@ +134,3 @@
+      std::aligned_storage<sizeof(XRayRecord), alignof(XRayRecord)>::type;
+  thread_local static Buffer InMemoryBuffer[BuffLen] = {};
+  thread_local static size_t Offset = 0;
----------------
(Minor) Why not to have `BuffLen` as a local `static const`? This would remove "action at a distance" anti-pattern.

================
Comment at: lib/xray/xray_inmemory_log.cc:137
@@ +136,3 @@
+  thread_local static size_t Offset = 0;
+  static int Fd = [] {
+    // FIXME: Figure out how to make this less stderr-dependent.
----------------
Why not `thread_local` ? How do you ensure no 2 threads race to check whether Fd is initialized and, if not, initializing it?

================
Comment at: lib/xray/xray_inmemory_log.cc:139
@@ +138,3 @@
+    // FIXME: Figure out how to make this less stderr-dependent.
+    SetPrintfAndReportCallback(PrintToStdErr);
+    // Open a temporary file once for the log.
----------------
Why do you call it `Printf`? `f` in `printf` stays for `formatted`, I guess. But PrintToStdErr doesn't support formatting. Consider renaming to `SetPrintAndReportCallback` .

================
Comment at: lib/xray/xray_inmemory_log.cc:141
@@ +140,3 @@
+    // Open a temporary file once for the log.
+    static char TmpFilename[256] = {};
+    auto E = internal_strncat(TmpFilename, flags()->xray_logfile_base, 246);
----------------
Would be UB in a race condition.

================
Comment at: lib/xray/xray_inmemory_log.cc:142
@@ +141,3 @@
+    static char TmpFilename[256] = {};
+    auto E = internal_strncat(TmpFilename, flags()->xray_logfile_base, 246);
+    if ((E + 6) - TmpFilename > 255) {
----------------
How the magic 246 number is calculated?

================
Comment at: lib/xray/xray_inmemory_log.cc:143
@@ +142,3 @@
+    auto E = internal_strncat(TmpFilename, flags()->xray_logfile_base, 246);
+    if ((E + 6) - TmpFilename > 255) {
+      Report("XRay log file base too long: %s", flags()->xray_logfile_base);
----------------
Is 255 logically equal to `sizeof(TmpFilename)-1` ? The latter would be preferred over a magic number, and it's easier then to change just one place - the declaration of `TmpFilename` variable.

================
Comment at: lib/xray/xray_inmemory_log.cc:147
@@ +146,3 @@
+    }
+    internal_strncat(TmpFilename, "XXXXXX", 6);
+    int Fd = mkstemp(TmpFilename);
----------------
Consider making "XXXXXX" a constant, replacing 6 with its `sizeof`-1.

================
Comment at: lib/xray/xray_inmemory_log.cc:159-163
@@ +158,7 @@
+    long long CPUFrequency = -1;
+    if (readValueFromFile("/sys/devices/system/cpu/cpu0/tsc_freq_khz",
+                          &CPUFrequency)) {
+      CPUFrequency *= 1000;
+    } else if (readValueFromFile(
+                   "/sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq",
+                   &CPUFrequency)) {
----------------
What if the system has multiple CPU sockets, with different CPUs installed so that TSC frequencies are different for different cores?

================
Comment at: lib/xray/xray_inmemory_log.cc:166
@@ +165,3 @@
+      CPUFrequency *= 1000;
+    }
+
----------------
Why not to report an error?


https://reviews.llvm.org/D21982