[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_fdr_logging.cc:660
+  static bool TSCSupported = true;
+  static uint64_t CycleFrequency = __xray::NanosecondsPerSecond;
+  pthread_once(&OnceInit, +[] {
----------------
kpw wrote:
> 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?
It's the standard that says, that they must be be constant-initialised.

In C++11 this is [stmt.decl] p4 (6.7.4) which references [basic.start.init]p2 bullet 3.


================
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:
> 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.
That's a good idea. The test is supposed to guard against those, because it will turn into a link failure for missing ABI functions.


================
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;
----------------
kpw wrote:
> 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?
Nope, bad rebase/merge. :)


https://reviews.llvm.org/D46998