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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: compiler-rt/lib/xray/xray_basic_logging.cc:500-501
+    pthread_once(&DynamicOnce, +[] {
+      FakeTLD = &getThreadLocalData();
+      Atexit(+[] { TLDDestructor(FakeTLD); });
+    });
----------------
kpw wrote:
> 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?
Note, that this happens at program exit -- we want to make sure that at program exit we're cleaning up the data for the final exiting thread. This code only runs when the mode installed at premain init. In that case we can't rely on the finalisation routine being invoked, so we force the cleanup and flush one way or another.


================
Comment at: compiler-rt/lib/xray/xray_fdr_logging.cc:660
+  static bool TSCSupported = true;
+  static uint64_t CycleFrequency = __xray::NanosecondsPerSecond;
+  pthread_once(&OnceInit, +[] {
----------------
kpw wrote:
> 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?
Yes, because this is initialised with a constant -- __xray::NanosecondsPerSecond is a constexpr.


================
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;
----------------
kpw wrote:
> What's the ABI reason to change this to aligned_storage instead of just using the type?
Because XRayFileHeader is not trivially constructible, initialising it is guarded to be thread-safe -- that thread-safe init calls C++ ABI specific functions. Using aligned storage here ensures that the storage is static, but that initialisation happens in the pthread_once(...) region.


================
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
----------------
kpw wrote:
> Shouldn't a different prefix run FDR to check that that mode works as well?
Technically unnecessary, since we link in the FDR mode runtime anyway -- the test is just ensuring that we can link and run a C program.


https://reviews.llvm.org/D46998