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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

Thanks @pelikan -- PTAL?



================
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
----------------
pelikan wrote:
> timespec comes from clock_gettime(2), gettimeofday(2) returns timeval which has microseconds (which are no good here).
This is fine, we use this at the header to indicate some rough time when a trace was created. It's not used anywhere else.


================
Comment at: lib/xray/xray_fdr_logging.cc:317
+  unsigned CPU;
+  uint64_t TSC = __rdtscp(&CPU);
+
----------------
pelikan wrote:
> 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).
Right -- this code was written before that refactoring. I'll do that now. FWIW, clang doesn't complain about this.


================
Comment at: lib/xray/xray_fdr_logging.h:37
+  // A MetadataRecord must always have a type of 1.
+  RecordType Type : 1;
+
----------------
pelikan wrote:
> 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?
Unsure whether this is a bug, but that's a good question. RecordType is an enum class whose base is an unsigned 8-bit integral type -- and only has two values. 1 bit ought to be enough, no?


================
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();
----------------
pelikan wrote:
> Why is this going back into a lambda?  I thought we wanted to separate hot code from one-off initialization for readability.
Ah, this was a bad merge. Fixed.


https://reviews.llvm.org/D27038