[PATCH] D46998: [XRay][compiler-rt] Remove reliance on C++ ABI features

[Keith via Phabricator via llvm-commits]

kpw added inline comments.


================
Comment at: compiler-rt/lib/xray/xray_basic_logging.cc:500-501
+    pthread_once(&DynamicOnce, +[] {
+      FakeTLD = &getThreadLocalData();
+      Atexit(+[] { TLDDestructor(FakeTLD); });
+    });
----------------
I don't get why there is a mix of static FakeTLD and thread_local variables. Won't this only access and subsequently destroy the thread_locals for only a single thread?


================
Comment at: compiler-rt/lib/xray/xray_fdr_logging.cc:660
+  static bool TSCSupported = true;
+  static uint64_t CycleFrequency = __xray::NanosecondsPerSecond;
+  pthread_once(&OnceInit, +[] {
----------------
I'm a bit surprised that using static variables with their initialized exactly once by the first caller semantics doesn't depend on the ABI. Are you sure these are OK?


================
Comment at: compiler-rt/lib/xray/xray_fdr_logging.cc:714
   // buffers to expect).
-  static XRayFileHeader Header = fdrCommonHeaderInfo();
+  static std::aligned_storage<sizeof(XRayFileHeader)>::type HeaderStorage;
+  static pthread_once_t HeaderOnce = PTHREAD_ONCE_INIT;
----------------
What's the ABI reason to change this to aligned_storage instead of just using the type?


================
Comment at: compiler-rt/lib/xray/xray_fdr_logging.cc:785-786
                  memory_order_release);
+    atomic_store(&LoggingStatus, XRayLogInitStatus::XRAY_LOG_UNINITIALIZED,
+                 memory_order_release);
     return XRayLogFlushStatus::XRAY_LOG_FLUSHED;
----------------
Are you adding the ability to cycle through states? I'd say that's a separate patch than the ABI stuff.


================
Comment at: compiler-rt/lib/xray/xray_fdr_logging.cc:789-791
-    // Clean up the buffer queue, and do not bother writing out the files!
-    delete BQ;
-    BQ = nullptr;
----------------
Removing this does not look related to ABI. This should be a separate patch if it's a behavior change. The ABI change would be to translate to destructor + internal free if I'm not mistaken.


================
Comment at: compiler-rt/test/xray/TestCases/Posix/c-test.cc:3-4
+// RUN: rm xray-log.c-test.* || true
+// RUN: XRAY_OPTIONS=patch_premain=true:verbosity=1:xray_mode=xray-basic %t \
+// RUN:     2>&1 | FileCheck %s
+// RUN: rm xray-log.c-test.* || true
----------------
Shouldn't a different prefix run FDR to check that that mode works as well?


https://reviews.llvm.org/D46998