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

[Keith via Phabricator via llvm-commits]

kpw accepted this revision.
kpw added inline comments.
This revision is now accepted and ready to land.


================
Comment at: compiler-rt/lib/xray/xray_basic_logging.cc:500-501
+    pthread_once(&DynamicOnce, +[] {
+      FakeTLD = &getThreadLocalData();
+      Atexit(+[] { TLDDestructor(FakeTLD); });
+    });
----------------
dberris wrote:
> 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.
I see. This is to salvage something to log when exiting abnormally. It's not obvious that TLDDestructor calls similar routines to finalize and flush instead of just freeing memory and stuff like that.

If you can come up with a way to make this more obvious, be my guest, otherwise the comment will have to suffice.


================
Comment at: compiler-rt/lib/xray/xray_fdr_logging.cc:660
+  static bool TSCSupported = true;
+  static uint64_t CycleFrequency = __xray::NanosecondsPerSecond;
+  pthread_once(&OnceInit, +[] {
----------------
dberris wrote:
> 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.
So we rely on clang turning these into pre-populated addresses (in bss?) at compilation? Is it the standard or the implementation that lets us rely on that?


================
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;
----------------
dberris wrote:
> 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.
It would be great if you could add a writeup of the common ABI pitfalls you discovered and the xray way of accomplishing similar idioms. I'm not sure where, but otherwise, these details are not going to be obvious to future maintainers/contributors etc.


================
Comment at: compiler-rt/lib/xray/xray_fdr_logging.cc:844
   atomic_store(&LogFlushStatus, XRayLogFlushStatus::XRAY_LOG_FLUSHED,
-               memory_order_release);
+               __sanitizer::memory_order_release);
   return XRayLogFlushStatus::XRAY_LOG_FLUSHED;
----------------
Doesn't the "\_\_xray" namespace have an effective "using namespace \_\_sanitizer" everywhere? <--- differential formatting turns these into underlines.

Are you just doing this to make it obvious this is not a stdlib atomic?


https://reviews.llvm.org/D46998