[PATCH] D40828: [XRay][compiler-rt] Implement XRay Basic Mode Filtering

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: compiler-rt/lib/xray/xray_inmemory_log.cc:48
+  uint16_t Type;
+  uint8_t CPU;
+  uint8_t Padding;
----------------
eizan wrote:
> is 8 bits enough for a CPU number much longer? E.g., getcpu() on Linux returns an unsigned integer for a cpu number.
Not for long. I was going to change this to a uint32_t, but then that means I'd have to make a larger change moving forward. In the meantime I thought it was better to be consistent with the current record data structure. We can make that change later, independently, and update the version number of the log to support the new larger CPU ID storage. For now, being consistent with the rest of the code is I think more prudent (and more manageable to change).


================
Comment at: compiler-rt/lib/xray/xray_inmemory_log.cc:56
 struct alignas(64) ThreadLocalData {
-  XRayRecord *InMemoryBuffer = nullptr;
+  void *InMemoryBuffer = nullptr;
   size_t BufferSize = 0;
----------------
eizan wrote:
> Why did you change this from XRayRecord * to void *?
Ah, because the original version of this code was violating strict aliasing rules. The changes here make it so that we're explicitly making the in-memory buffer character-aligned, and not running into strict aliasing errors.


================
Comment at: compiler-rt/lib/xray/xray_inmemory_log.cc:123
+    }
+    TLD.TID = syscall(SYS_gettid);
     pthread_setspecific(PThreadKey, &TLD);
----------------
eizan wrote:
> Are there any plans to make this useable on other operating systems? What about pthread_self() instead?
Yep -- found one already in sanitizer_common called `__sanitizer::GetTid()`.


================
Comment at: compiler-rt/lib/xray/xray_inmemory_log.cc:173
+
+  uint8_t CPU = 0;
+  uint64_t TSC = ReadTSC(CPU);
----------------
eizan wrote:
> Why are you hard-coding this to 0 instead of getting the CPU number that the thread is running on?
Ah, `ReadTSC` takes a mutable reference. It will actually update CPU.


https://reviews.llvm.org/D40828