[PATCH] D38073: [XRay][compiler-rt] Use pthread for initializing thread-local data

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lib/xray/xray_fdr_logging_impl.h:110
 // call so that it can be initialized on first use instead of as a global.
-struct ThreadLocalData {
+struct alignas(64) ThreadLocalData {
   BufferQueue::Buffer Buffer;
----------------
Presumably this should use LLVM_ALIGNAS?

Is this related to the rest of the patch, if so, how? Is it for correctness or performance?


================
Comment at: lib/xray/xray_fdr_logging_impl.h:145
+// the FDR logging implementation. The implementation details require a bit of
+// care to maintain, so document the semantics that we want to achieve here.
+//
----------------
"so document the semantics that we want to achieve here" seems weird - that sounds like a patch note or feature request (& somewhat redundant, it feels like - the reader can probably see that the semantics are documented below), rather than a statement of fact.

"the semantics are documented below" or maybe "The following semantics are intended:" or skip this comment entirely? Not sure.


================
Comment at: lib/xray/xray_fdr_logging_impl.h:149
+//
+//   - XRay handlers should strive to not call any memory allocation routines
+//     that may delegate to an instrumented implementation. This means we should
----------------
"strive to" may be removed if this is a hard requirement? "should not call ... "


================
Comment at: lib/xray/xray_fdr_logging_impl.h:150-152
+//     that may delegate to an instrumented implementation. This means we should
+//     not be calling functions like malloc() and free() while we're
+//     instrumenting.
----------------
Maybe remove the personalization (here and in the following)? "This means functions like malloc() and free() should not be called while instrumenting"? Though I think the first sentence probably already says that?


================
Comment at: lib/xray/xray_fdr_logging_impl.h:175-183
+// We're doing this to avoid using a `thread_local` object that has a
+// non-trivial destructor, because the C++ runtime might call std::malloc(...)
+// to register calls to destructors. Deadlocks may arise when, for example, an
+// externally provided malloc implementation is XRay instrumented, and
+// initializing the thread-locals involves calling into malloc. A malloc
+// implementation that does global synchronization might be holding a lock for a
+// critical section, calling a function that might be XRay instrumented (and
----------------
Maybe this goes into more detail than needed if the XRay runtime in general shouldn't be using the standard library for all these reasons - that's a general requirement that wouldn't need to be rejustified at every use case/implementation detail?


================
Comment at: lib/xray/xray_fdr_logging_impl.h:190-191
+  thread_local pthread_once_t key_once = PTHREAD_ONCE_INIT;
+  thread_local std::aligned_storage<sizeof(ThreadLocalData),
+                                    alignof(ThreadLocalData)>::type TLSBuffer;
+  // Ensure that we only actually ever do the pthread initialization once.
----------------
Seems the goal in general was/is to move away from C++ standard library usage in the runtime library, right? Is there an attempt to pick and choose which parts can be relied upon, or is the overall goal to actually remove C++ standard library usage entirely, like other parts of compiler-rt?

If not, then maybe std::aligned_union<1, ThreadLocalData> might be marginally simpler here, but only marginally. (& I wouldn't say it's strictly better - since a union of only one thing is a bit strange)


https://reviews.llvm.org/D38073