[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:47
@@ +46,3 @@
+  char Padding[15] = {};
+} __attribute__((packed));
+
----------------
rSerge wrote:
> 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.
Are you aware of specific CPUs where this would be an issue?

I'm happy to change it, but I'd like to understand whether this is an actual problem, or just a hypothetical one.

Note that we don't read this, we only ever write it down, so the performance hit isn't as important as maximising the packing of the bytes that eventually make it to disk. The 15 byte padding gives us 15 more bytes to use for later extending the information in the file header.

================
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.
----------------
rSerge wrote:
> Why not `thread_local` ? How do you ensure no 2 threads race to check whether Fd is initialized and, if not, initializing it?
C++11 ensures this for us, that the first time this function is called, this static variable is initialised exactly once -- this is meant to be synchronised across all threads too.

================
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.
----------------
rSerge wrote:
> Why do you call it `Printf`? `f` in `printf` stays for `formatted`, I guess. But PrintToStdErr doesn't support formatting. Consider renaming to `SetPrintAndReportCallback` .
This API is defined by the common sanitizer tools -- I'm just a using it. :)

There are places where Report(...) is called with a format string and additional arguments (like printf), and underneath it dispatches to a callback when these formatted print's are actually invoked. We're providing something that deals with the string that's post-formatted.

================
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);
----------------
rSerge wrote:
> Would be UB in a race condition.
C++11 ensures this is not a racy initialisation.

================
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) {
----------------
rSerge wrote:
> How the magic 246 number is calculated?
I was being conservative here, giving an allowance of 10 bytes difference between the size of the temporary filename's buffer size total and the base filename provided as a flag. Fixed it to do relative calculations instead of magic numbers.

================
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)) {
----------------
rSerge wrote:
> What if the system has multiple CPU sockets, with different CPUs installed so that TSC frequencies are different for different cores?
I'm not sure how this works in non-x86, but we assume that all the CPUs will run in the same rate, and use that as an approximation for the TSC frequency rate. This means if we're on this branch of the conditional, we don't get the reliable number and take a guess anyway.


https://reviews.llvm.org/D21982