[PATCH] D36078: [XRay][compiler-rt] Remove use of std::mutex and std::shared_ptr from global scope.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lib/xray/xray_fdr_logging.cc:152
   // Release the in-memory buffer queue.
-  BQ.reset();
+  (*BQ).reset();
 
----------------
->


================
Comment at: lib/xray/xray_fdr_logging.cc:275
+  if (BQ == nullptr)
+    BQ = new std::shared_ptr<BufferQueue>(nullptr);
+
----------------
Could drop the nullptr here


================
Comment at: lib/xray/xray_fdr_logging_impl.h:173
+thread_local std::shared_ptr<BufferQueue>* LocalBQ =
+    new std::shared_ptr<BufferQueue>(nullptr);
+thread_local ThreadExitBufferCleanup Cleanup(*LocalBQ, Buffer);
----------------
and here


================
Comment at: lib/xray/xray_fdr_logging_impl.h:471
     writeEOBMetadata();
-    if (!releaseThreadLocalBuffer(LocalBQ.get()))
+    if (!releaseThreadLocalBuffer(LocalBQ->get()))
       return false;
----------------
Might be a good opportunity to change releaseThreadLocalBuffer take the BufferQueeu by mutable reference, since it's required to be nonnull anyway? Then this parameter would be '**LocalBQ' (which at least I treat with less suspicion than a function called 'release' that takes a raw pointer to data that's owned by a smart pointer... )


================
Comment at: lib/xray/xray_inmemory_log.cc:46
 
-std::mutex LogMutex;
+__sanitizer::SpinMutex LogMutex;
 
----------------
Might be worth separating these two changes (the mutex from the shared-ptr) - they seem pretty independent & this one looks like it's good forward progress to move away from standard library dependencies as is required by compiler-rt, the other's more of a stop-gap (or at least neutral on the progress in that direction).


================
Comment at: lib/xray/xray_inmemory_log.cc:85-86
   static bool TSCSupported = probeRequiredCPUFeatures();
-  static uint64_t CycleFrequency = TSCSupported ? getTSCFrequency()
-                                   : __xray::NanosecondsPerSecond;
+  static uint64_t CycleFrequency =
+      TSCSupported ? getTSCFrequency() : __xray::NanosecondsPerSecond;
 
----------------
unnecessary reformatting? or did something change in this line that I can't quite spot.


https://reviews.llvm.org/D36078