[compiler-rt] [xray] Disable alignas() for thread_local objects on NetBSD

[mgorny]

Summary

Disable enforcing alignas() for structs that are used as thread_local
data on NetBSD. The NetBSD ld.so implementation is buggy and does
not enforce correct alignment; however, clang seems to take it for
granted and generates instructions that segv on wrongly aligned objects.
Therefore, disable those alignas() statements on NetBSD until we can
establish a better fix.

Apparently, std::aligned_storage<> does not have any real effect
at the moment, so we can leave it as-is.

[krytarowski]

lib/xray/xray_basic_logging.cc

58

Can we introduce a macro like:

#if SANITIZER_NETBSD
#define XRAY_TLS_ALIGNAS64 /* Nor supported */
#else
#define XRAY_TLS_ALIGNED_SUPPORTED
#define XRAY_TLS_ALIGNAS64 alignas(64)
#endif

And later:

struct XRAY_TLS_ALIGNAS64 ThreadLocalData {

and

#ifdef XRAY_TLS_ALIGNED_SUPPORTED
static_assert(alignof(ThreadLocalData) >= 64,
              "ThreadLocalData must be cache line aligned.");
#endif

[mgorny]

lib/xray/xray_basic_logging.cc

58

Maybe. Though i suppose it'd make more sense to make 64 an argument to the macro.

[krytarowski]

lib/xray/xray_basic_logging.cc

58

#if !SANITIZER_NETBSD
#define XRAY_TLS_ALIGNAS(x) alignas(x)
#endif

struct XRAY_TLS_ALIGNAS(64) ThreadLocalData {

and

#ifdef XRAY_TLS_ALIGNAS
static_assert(alignof(ThreadLocalData) >= 64,
              "ThreadLocalData must be cache line aligned.");
#endif

58

Oops, actually not fully correct, but some variation of the above.

[mgorny]

Updated as requested.

[krytarowski]

lib/xray/xray_defs.h

22

I would switch the order, in order to remove unneeded negation.

#if SANITIZER_NETBSD
...
#else
...
#endif

#define XRAY_HAS_TLS_ALIGNAS 0 is not needed as #if XRAY_HAS_TLS_ALIGNAS will evaluate to false anyway and #define XRAY_HAS_TLS_ALIGNAS 1 is equivalent to #define XRAY_HAS_TLS_ALIGNAS. But it's just a matter of style.

[dberris]

lib/xray/xray_defs.h

22

FWIW, I agree to this suggestion. I also prefer:

#if SANITIZER_NETBSD
...
#else
...

But I do like the explicit definition of XRAY_HAS_TLS_ALIGNAS.

[mgorny]

lib/xray/xray_defs.h

22

I've chosen the explicit method to match what's already done e.g. in sanitizer_platform.h. I'll swap the order.

[mgorny]

Reordered.

lib/xray/xray_basic_logging.cc

struct alignas(16) StackEntry {
   uint16_t Type;
   uint8_t CPU;
   uint8_t Padding;
   uint64_t TSC;
 };
 
 static_assert(sizeof(StackEntry) == 16, "Wrong size for StackEntry");
 
-struct alignas(64) ThreadLocalData {
+struct XRAY_TLS_ALIGNAS(64) ThreadLocalData {
   void *InMemoryBuffer = nullptr;
   size_t BufferSize = 0;
   size_t BufferOffset = 0;
   void *ShadowStack = nullptr;
   size_t StackSize = 0;
   size_t StackEntries = 0;
   __xray::LogWriter *LogWriter = nullptr;
 };

lib/xray/xray_defs.h

 #define XRAY_XRAY_DEFS_H
 
 #if XRAY_SUPPORTED
 #define XRAY_NEVER_INSTRUMENT __attribute__((xray_never_instrument))
 #else
 #define XRAY_NEVER_INSTRUMENT
 #endif
 
+#if SANITIZER_NETBSD
+// NetBSD: thread_local is not aligned properly, and the code relying
+// on it segfaults
+#define XRAY_TLS_ALIGNAS(x)
+#define XRAY_HAS_TLS_ALIGNAS 0
+#else
+#define XRAY_TLS_ALIGNAS(x) alignas(x)
+#define XRAY_HAS_TLS_ALIGNAS 1
+#endif
+
 #endif  // XRAY_XRAY_DEFS_H

lib/xray/xray_fdr_logging.cc

static atomic_sint32_t LoggingStatus = {
     XRayLogInitStatus::XRAY_LOG_UNINITIALIZED};
 
 namespace {
 
 // 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. We
 // force the alignment to 64-bytes for x86 cache line alignment, as this
 // structure is used in the hot path of implementation.
-struct alignas(64) ThreadLocalData {
+struct XRAY_TLS_ALIGNAS(64) ThreadLocalData {
   BufferQueue::Buffer Buffer{};
   BufferQueue *BQ = nullptr;
 
   using LogWriterStorage =
       typename std::aligned_storage<sizeof(FDRLogWriter),
                                     alignof(FDRLogWriter)>::type;
 
   LogWriterStorage LWStorage;

 // 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).
+#if XRAY_HAS_TLS_ALIGNAS
 static_assert(alignof(ThreadLocalData) >= 64,
               "ThreadLocalData must be cache line aligned.");
+#endif
 static ThreadLocalData &getThreadLocalData() {
   thread_local typename std::aligned_storage<
       sizeof(ThreadLocalData), alignof(ThreadLocalData)>::type TLDStorage{};
 
   if (pthread_getspecific(Key) == NULL) {
     new (reinterpret_cast<ThreadLocalData *>(&TLDStorage)) ThreadLocalData{};
     pthread_setspecific(Key, &TLDStorage);
   }