[PATCH] D34381: [XRay] [compiler-rt] switch FDR implementation to the TSC API [NFC]

Martin Pelikán via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 19 20:42:37 PDT 2017


pelikan created this revision.

The current code unnecessarily checks for CPU features twice and doesn't
take advantage of the infrastructure in xray_tsc.h at all.  At the cost
of going back to the old API, unify FDR's TSC handling with the rest of
the code base and get rid of two arguments to processFunctionHook which
will be subsequently used in https://reviews.llvm.org/D32844 for the first function call argument.

Should we decide to go from (return value + reference) to std::tuple<>
is a separate matter (for a separate review).


https://reviews.llvm.org/D34381

Files:
  lib/xray/xray_fdr_logging.cc
  lib/xray/xray_fdr_logging_impl.h


Index: lib/xray/xray_fdr_logging_impl.h
===================================================================
--- lib/xray/xray_fdr_logging_impl.h
+++ lib/xray/xray_fdr_logging_impl.h
@@ -98,7 +98,6 @@
 /// polluting the log and may use the buffer queue to obtain or release a
 /// buffer.
 static void processFunctionHook(int32_t FuncId, XRayEntryType Entry,
-                                uint64_t TSC, unsigned char CPU,
                                 int (*wall_clock_reader)(clockid_t,
                                                          struct timespec *),
                                 __sanitizer::atomic_sint32_t &LoggingStatus,
@@ -545,7 +544,7 @@
 }
 
 inline void processFunctionHook(
-    int32_t FuncId, XRayEntryType Entry, uint64_t TSC, unsigned char CPU,
+    int32_t FuncId, XRayEntryType Entry,
     int (*wall_clock_reader)(clockid_t, struct timespec *),
     __sanitizer::atomic_sint32_t &LoggingStatus,
     const std::shared_ptr<BufferQueue> &BQ) XRAY_NEVER_INSTRUMENT {
@@ -566,6 +565,8 @@
   if (LocalBQ == nullptr)
     LocalBQ = BQ;
 
+  uint8_t CPU;
+  uint64_t TSC = readTSC(CPU);
   if (!isLogInitializedAndReady(LocalBQ, TSC, CPU, wall_clock_reader))
     return;
 
Index: lib/xray/xray_fdr_logging.cc
===================================================================
--- lib/xray/xray_fdr_logging.cc
+++ lib/xray/xray_fdr_logging.cc
@@ -163,47 +163,17 @@
   return XRayLogInitStatus::XRAY_LOG_UNINITIALIZED;
 }
 
-static std::tuple<uint64_t, unsigned char>
-getTimestamp() XRAY_NEVER_INSTRUMENT {
-  // We want to get the TSC as early as possible, so that we can check whether
-  // we've seen this CPU before. We also do it before we load anything else, to
-  // allow for forward progress with the scheduling.
-  unsigned char CPU;
-  uint64_t TSC;
-
-  // Test once for required CPU features
-  static bool TSCSupported = probeRequiredCPUFeatures();
-
-  if (TSCSupported) {
-    TSC = __xray::readTSC(CPU);
-  } else {
-    // FIXME: This code needs refactoring as it appears in multiple locations
-    timespec TS;
-    int result = clock_gettime(CLOCK_REALTIME, &TS);
-    if (result != 0) {
-      Report("clock_gettime(2) return %d, errno=%d", result, int(errno));
-      TS = {0, 0};
-    }
-    CPU = 0;
-    TSC = TS.tv_sec * __xray::NanosecondsPerSecond + TS.tv_nsec;
-  }
-  return std::make_tuple(TSC, CPU);
-}
-
 void fdrLoggingHandleArg0(int32_t FuncId,
                           XRayEntryType Entry) XRAY_NEVER_INSTRUMENT {
-  auto TSC_CPU = getTimestamp();
-  __xray_fdr_internal::processFunctionHook(FuncId, Entry, std::get<0>(TSC_CPU),
-                                           std::get<1>(TSC_CPU), clock_gettime,
+  __xray_fdr_internal::processFunctionHook(FuncId, Entry, clock_gettime,
                                            LoggingStatus, BQ);
 }
 
 void fdrLoggingHandleCustomEvent(void *Event,
                                  std::size_t EventSize) XRAY_NEVER_INSTRUMENT {
   using namespace __xray_fdr_internal;
-  auto TSC_CPU = getTimestamp();
-  auto &TSC = std::get<0>(TSC_CPU);
-  auto &CPU = std::get<1>(TSC_CPU);
+  uint8_t CPU;
+  uint64_t TSC = __xray::readTSC(CPU);
   thread_local bool Running = false;
   RecursionGuard Guard{Running};
   if (!Guard) {
@@ -240,9 +210,8 @@
   CustomEvent.Type = uint8_t(RecordType::Metadata);
   CustomEvent.RecordKind =
       uint8_t(MetadataRecord::RecordKinds::CustomEventMarker);
-  constexpr auto TSCSize = sizeof(std::get<0>(TSC_CPU));
   std::memcpy(&CustomEvent.Data, &ReducedEventSize, sizeof(int32_t));
-  std::memcpy(&CustomEvent.Data[sizeof(int32_t)], &TSC, TSCSize);
+  std::memcpy(&CustomEvent.Data[sizeof(int32_t)], &TSC, sizeof(TSC));
   std::memcpy(RecordPtr, &CustomEvent, sizeof(CustomEvent));
   RecordPtr += sizeof(CustomEvent);
   std::memcpy(RecordPtr, Event, ReducedEventSize);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D34381.103141.patch
Type: text/x-patch
Size: 3830 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170620/733f4212/attachment.bin>


More information about the llvm-commits mailing list