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

[dberris]

Summary

We avoid using C++11's thread_local keyword on non-trivially
destructible objects because it may introduce deadlocks when the C++
runtime registers destructors calling std::malloc(...). The deadlock may
happen when the allocator implementation is itself XRay instrumented.

To avoid having to call malloc(...) and free(...) in particular, we use
pthread_once, pthread_create_key, and pthread_setspecific to instead
manually register the cleanup implementation we want.

The code this replaces used an RAII type that implements the cleanup
functionality in the destructor, that was then initialized as a
function-local thread_local object. While it works in usual situations,
unfortunately it breaks when using a malloc implementation that itself
is XRay-instrumented.

[dblaikie]

lib/xray/xray_fdr_logging_impl.h

110

Presumably this should use LLVM_ALIGNAS?

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

143

"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.

147

"strive to" may be removed if this is a hard requirement? "should not call ... "

148–150

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?

173–181

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?

187–188

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)

[dberris]

• fixup: address comments

[dberris]

Thanks for the review!

lib/xray/xray_fdr_logging_impl.h

110

It's related in that we'd like to use aligned uninitialised storage to store one of these objects. We'd the buffer to not only be correctly sized but also aligned correctly, since we're going to interpret the buffer's address as a different type (punning? aliasing? I can never get the right terminology for that). The performance benefit is a side-effect of the correctness issue (since we have non-trivially destructible things in it).

BTW, I can't use LLVM_ALIGNAS because that seems to be undefined in compiler-rt. :/

143

I like skipping it. Stream of consciousness comments at the end of the day don't come out very well. :)

173–181

This is specifically for the function, since the original implementation (and the potentially more obvious solution) uses a thread-exit guard type (i.e. RAII but using a thread-local). Other parts of the implementation have avoided using memory allocation routines because of performance reasons but not entirely for correctness.

187–188

I think I should document why we're using a thread-local uninitialized memory at all in this situation. :)

First, the type ThreadLocalData isn't trivially destructable. If this was a thread_local object, we end up with the same problem we had in the first place -- having a thread_local object with a non-trivial destructor may cause the C++ runtime to call std::malloc(...) and introduce a deadlock when initialising the implementation as well as when destroying the object at thread-exit time.

Second, having uninitialised buffer space makes the initialisation of the memory trivial. We do want uninitialised bytes because we don't need to zero out the bytes, since we're going to initialize it once using placement new anyway.

So unfortunately, this is the solution that works and fits within the requirements we laid out. Updated with a bit more documentation on specifically why we're using an uninitialised aligned buffer. :)

Removing the dependency on standard C++ library things that have runtime issues I think is a good goal, but is orthogonal to this particular change (since really we're just avoiding the use of thread_local non-trivially destructible objects).

[dberris]

ping @dblaikie?

[dberris]

• fixup: address comments

[dberris]

Rebase.

[pelikan]

lib/xray/xray_fdr_logging_impl.h

193

I reckon this should get the UNUSED sanitizer_common macro, which expands to __attribute__((unused)) to stop the compiler whinging about it.

Plus, CodingStandards.rst wants it to be called "Unused" (same above - Key).

222

This line can go away with the UNUSED macro up there.

[pelikan]

Technically, the pthread functions can return EAGAIN or ENOMEM but it (ehm) "should not happen in this case". Really. I mean, come on!

Maybe a Report() in that case is sufficient, to explain any weirdness in behaviour.

EDIT: Actually, EAGAIN from pthread_key_create(3) is *per process*, so we can run out of them.

[dberris]

• Rebase
  • fixup: use UNUSED, restore alignment for cache-line friendliness, and report on errors found when pthread_create_key fails

[pdouglas]

Hi,

This change appears to be causing three test failures on several buildbots:

FAIL: XRay-Unit :: unit/./XRayBufferQueueTest-powerpc64le-Test/BufferQueueTest.Apply (36360 of 36408)
******************** TEST 'XRay-Unit :: unit/./XRayBufferQueueTest-powerpc64le-Test/BufferQueueTest.Apply' FAILED ********************
Note: Google Test filter = BufferQueueTest.Apply
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from BufferQueueTest
[ RUN      ] BufferQueueTest.Apply
/home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/llvm/projects/compiler-rt/lib/xray/tests/unit/buffer_queue_test.cc:107: Failure
      Expected: Buffers.getBuffer(B)
      Which is: 4-byte object <01-00 00-00>
To be equal to: BufferQueue::ErrorCode::Ok
      Which is: 4-byte object <00-00 00-00>
[  FAILED  ] BufferQueueTest.Apply (0 ms)
[----------] 1 test from BufferQueueTest (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (0 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] BufferQueueTest.Apply

 1 FAILED TEST

********************
PASS: XRay-Unit :: unit/./XRayBufferQueueTest-powerpc64le-Test/BufferQueueTest.ErrorsWhenFinalising (36361 of 36408)
FAIL: XRay-Unit :: unit/./XRayBufferQueueTest-powerpc64le-Test/BufferQueueTest.GetAndRelease (36362 of 36408)
******************** TEST 'XRay-Unit :: unit/./XRayBufferQueueTest-powerpc64le-Test/BufferQueueTest.GetAndRelease' FAILED ********************
Note: Google Test filter = BufferQueueTest.GetAndRelease
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from BufferQueueTest
[ RUN      ] BufferQueueTest.GetAndRelease
/home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/llvm/projects/compiler-rt/lib/xray/tests/unit/buffer_queue_test.cc:36: Failure
      Expected: Buffers.releaseBuffer(Buf)
      Which is: 4-byte object <01-00 00-00>
To be equal to: BufferQueue::ErrorCode::Ok
      Which is: 4-byte object <00-00 00-00>
[  FAILED  ] BufferQueueTest.GetAndRelease (0 ms)
[----------] 1 test from BufferQueueTest (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (0 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] BufferQueueTest.GetAndRelease

 1 FAILED TEST

********************
PASS: XRay-Unit :: unit/./XRayBufferQueueTest-powerpc64le-Test/BufferQueueTest.API (36363 of 36408)
FAIL: XRay-Unit :: unit/./XRayBufferQueueTest-powerpc64le-Test/BufferQueueTest.GetUntilFailed (36364 of 36408)
******************** TEST 'XRay-Unit :: unit/./XRayBufferQueueTest-powerpc64le-Test/BufferQueueTest.GetUntilFailed' FAILED ********************
Note: Google Test filter = BufferQueueTest.GetUntilFailed
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from BufferQueueTest
[ RUN      ] BufferQueueTest.GetUntilFailed
/home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/llvm/projects/compiler-rt/lib/xray/tests/unit/buffer_queue_test.cc:48: Failure
      Expected: Buffers.releaseBuffer(Buf0)
      Which is: 4-byte object <01-00 00-00>
To be equal to: BufferQueue::ErrorCode::Ok
      Which is: 4-byte object <00-00 00-00>
[  FAILED  ] BufferQueueTest.GetUntilFailed (0 ms)
[----------] 1 test from BufferQueueTest (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (0 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] BufferQueueTest.GetUntilFailed

 1 FAILED TEST

********************

[dberris]

That's very interesting. I can't reproduce this locally though (and I suspect it's related to the other change, rL314766).

lib/xray/xray_fdr_logging_impl.h

 #ifndef XRAY_XRAY_FDR_LOGGING_IMPL_H
 #define XRAY_XRAY_FDR_LOGGING_IMPL_H
 
 #include <cassert>
 #include <cstdint>
 #include <cstring>
 #include <limits>
 #include <memory>
+#include <pthread.h>
 #include <string>
 #include <sys/syscall.h>
 #include <time.h>
 #include <unistd.h>
 
 #include "sanitizer_common/sanitizer_common.h"
 #include "xray/xray_log_interface.h"
 #include "xray_buffer_queue.h"
static void processFunctionHook(int32_t FuncId, XRayEntryType Entry,
                                 uint64_t TSC, unsigned char CPU,
                                 int (*wall_clock_reader)(clockid_t,
                                                          struct timespec *),
                                 __sanitizer::atomic_sint32_t &LoggingStatus,
                                 const std::shared_ptr<BufferQueue> &BQ);
 
 // Group together thread-local-data in a struct, then hide it behind a function
 // call so that it can be initialized on first use instead of as a global.
 struct ThreadLocalData {
   BufferQueue::Buffer Buffer;
   char *RecordPtr = nullptr;
   // The number of FunctionEntry records immediately preceding RecordPtr.
   uint8_t NumConsecutiveFnEnters = 0;
 
   // The number of adjacent, consecutive pairs of FunctionEntry, Tail Exit
   // records preceding RecordPtr.
   uint8_t NumTailCalls = 0;

 };
 
 // Forward-declare, defined later.
 static ThreadLocalData &getThreadLocalData();
 
 static constexpr auto MetadataRecSize = sizeof(MetadataRecord);
 static constexpr auto FunctionRecSize = sizeof(FunctionRecord);
 
-class ThreadExitBufferCleanup {
-  std::shared_ptr<BufferQueue> &Buffers;
-  BufferQueue::Buffer &Buffer;
-
-public:
-  explicit ThreadExitBufferCleanup(std::shared_ptr<BufferQueue> &BQ,
-                                   BufferQueue::Buffer &Buffer)
-      XRAY_NEVER_INSTRUMENT : Buffers(BQ),
-                              Buffer(Buffer) {}
+// This function will initialize the thread-local data structure used by the FDR
+// logging implementation and return a reference to it. The implementation
+// details require a bit of care to maintain.
+//
+// First, some requirements on the implementation in general:
+//
+//   - XRay handlers should not call any memory allocation routines that may
+//     delegate to an instrumented implementation. This means functions like
+//     malloc() and free() should not be called while instrumenting.
+//
+//   - We would like to use some thread-local data initialized on first-use of
+//     the XRay instrumentation. These allow us to implement unsynchronized
+//     routines that access resources associated with the thread.
+//
+// The implementation here uses a few mechanisms that allow us to provide both
+// the requirements listed above. We do this by:
+//
+//   1. Using a thread-local aligned storage buffer for representing the
+//      ThreadLocalData struct. This data will be uninitialized memory by
+//      design.
+//
+//   2. Using pthread_once(...) to initialize the thread-local data structures
+//      on first use, for every thread. We don't use std::call_once so we don't
+//      have a reliance on the C++ runtime library.
+//
+//   3. Registering a cleanup function that gets run at the end of a thread's
+//      lifetime through pthread_create_key(...). The cleanup function would
+//      allow us to release the thread-local resources in a manner that would
+//      let the rest of the XRay runtime implementation handle the records
+//      written for this thread's active buffer.
+//
+// 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
+// thus in turn calling into malloc by virtue of registration of the
+// thread_local's destructor).
+//
+// With the approach taken where, we attempt to avoid the potential for
+// deadlocks by relying instead on pthread's memory management routines.
+static ThreadLocalData &getThreadLocalData() {
+  thread_local pthread_key_t key;
+  thread_local pthread_once_t key_once = PTHREAD_ONCE_INIT;
 
-  ~ThreadExitBufferCleanup() noexcept XRAY_NEVER_INSTRUMENT {
-    auto &TLD = getThreadLocalData();
+  // We need aligned, uninitialized storage for the TLS object which is
+  // trivially destructible. We're going to use this as raw storage and
+  // placement-new the ThreadLocalData object into it later.
+  thread_local std::aligned_union<1, ThreadLocalData>::type TLSBuffer;
+
+  // Ensure that we only actually ever do the pthread initialization once.
+  thread_local bool unused = [] {
+    new (&TLSBuffer) ThreadLocalData();
+    pthread_key_create(&key, +[](void *) {
+      auto &TLD = *reinterpret_cast<ThreadLocalData *>(&TLSBuffer);
       auto &RecordPtr = TLD.RecordPtr;
+      auto &Buffers = TLD.LocalBQ;
+      auto &Buffer = TLD.Buffer;
       if (RecordPtr == nullptr)
         return;
 
       // We make sure that upon exit, a thread will write out the EOB
-    // MetadataRecord in the thread-local log, and also release the buffer to
-    // the queue.
-    assert((RecordPtr + MetadataRecSize) - static_cast<char *>(Buffer.Buffer) >=
+      // MetadataRecord in the thread-local log, and also release the buffer
+      // to the queue.
+      assert((RecordPtr + MetadataRecSize) -
+                 static_cast<char *>(Buffer.Buffer) >=
              static_cast<ptrdiff_t>(MetadataRecSize));
       if (Buffers) {
         writeEOBMetadata();
         auto EC = Buffers->releaseBuffer(Buffer);
         if (EC != BufferQueue::ErrorCode::Ok)
           Report("Failed to release buffer at %p; error=%s\n", Buffer.Buffer,
                  BufferQueue::getErrorString(EC));
         Buffers = nullptr;
         return;
       }
-  }
-};
+    });
+    pthread_setspecific(key, &TLSBuffer);
+    return true;
+  }();
+  (void)unused;
 
-static ThreadLocalData &getThreadLocalData() {
-  thread_local ThreadLocalData TLD;
-  thread_local ThreadExitBufferCleanup Cleanup(TLD.LocalBQ, TLD.Buffer);
-  return TLD;
+  return *reinterpret_cast<ThreadLocalData *>(&TLSBuffer);
 }
 
 //-----------------------------------------------------------------------------|
 // The rest of the file is implementation.                                     |
 //-----------------------------------------------------------------------------|
 // Functions are implemented in the header for inlining since we don't want    |
 // to grow the stack when we've hijacked the binary for logging.               |
 //-----------------------------------------------------------------------------|