[PATCH] D27038: [XRay][compiler-rt] XRay Flight Data Recorder Mode

[Martin Pelikán via Phabricator via llvm-commits]

pelikan added a comment.

Very quick review based on this code not compiling.



================
Comment at: include/xray/xray_records.h:45
+
+  // The current civiltime timestamp, as retrived from 'gettimeofday'. This
+  // allows readers of the file to determine when the file was created or
----------------
timespec comes from clock_gettime(2), gettimeofday(2) returns timeval which has microseconds (which are no good here).


================
Comment at: lib/xray/xray_fdr_logging.cc:317
+  unsigned CPU;
+  uint64_t TSC = __rdtscp(&CPU);
+
----------------
I really don't see how this *ever* compiled because it isn't #included; it fails for me for native amd64 as well as cross armv7/v8.  My recent refactoring introduced readTSC(&CPU) in xray_x86_64.h or xray_emulate_tsc.h respectively, please use that.  The seven line #ifdef #include chain can probably be put into some (internal) header if it needs to be repeated twice (but doesn't have to).


================
Comment at: lib/xray/xray_fdr_logging.cc:379-384
+  // Before we go setting up writing new function entries, we need to be
+  // really
+  // careful about the pointer math we're doing. This means we need to
+  // ensure
+  // that the record we are about to write is going to fit into the buffer,
+  // without overflowing the buffer.
----------------
Can the formatting follow the 80-character lines properly, instead of having one word per line?


================
Comment at: lib/xray/xray_fdr_logging.h:37
+  // A MetadataRecord must always have a type of 1.
+  RecordType Type : 1;
+
----------------
In file included from /home/pelikan/src/llvm/projects/compiler-rt/lib/xray/xray_fdr_logging.cc:17:0:
/home/pelikan/src/llvm/projects/compiler-rt/lib/xray/xray_fdr_logging.h:37:21: warning: ‘__xray::MetadataRecord::Type’ is too small to hold all values of ‘enum class __xray::RecordType’
   RecordType Type : 1;
                     ^
/home/pelikan/src/llvm/projects/compiler-rt/lib/xray/xray_fdr_logging.h:48:28: warning: ‘__xray::MetadataRecord::RecordKind’ is too small to hold all values of ‘enum class __xray::MetadataRecord::RecordKinds’
   RecordKinds RecordKind : 7; // Use 7 bits to identify this record type.
                            ^
/home/pelikan/src/llvm/projects/compiler-rt/lib/xray/xray_fdr_logging.h:56:21: warning: ‘__xray::FunctionRecord::Type’ is too small to hold all values of ‘enum class __xray::RecordType’
   RecordType Type : 1;
                     ^
/home/pelikan/src/llvm/projects/compiler-rt/lib/xray/xray_fdr_logging.h:62:28: warning: ‘__xray::FunctionRecord::RecordKind’ is too small to hold all values of ‘enum class __xray::FunctionRecord::RecordKinds’
   RecordKinds RecordKind : 3;

Is this a gcc (6.3.0 used) bug?


================
Comment at: lib/xray/xray_inmemory_log.cc:92
   thread_local static size_t Offset = 0;
-  static int Fd = __xray_OpenLogFile();
+  static int Fd = []() {
+    int F = getLogFD();
----------------
Why is this going back into a lambda?  I thought we wanted to separate hot code from one-off initialization for readability.


https://reviews.llvm.org/D27038