[GWP-ASan] Abstract the thread local variables access

[cryptoad]

Summary

In a similar fashion to D87420 for Scudo, this CL introduces a way to
get thread local variables via a platform-specific reserved TLS slot,
since Fuchsia doesn't support ELF TLS from the libc itself.

If needing to use this, a platform will have to define
GWP_ASAN_HAS_PLATFORM_TLS_SLOT and provide gwp_asan_platform_tls_slot.h
which will define a uint64_t *getPlatformGwpAsanTlsSlot() function
that will return the TLS word of storage.

I snuck in a couple of cleanup items as well, moving some static
functions to anonymous namespace for consistency.

[mcgrathr]

This is fine as is but there's really no need to carefully pack it into a uint64_t. It would be fine for Fuchsia's special header to have to meet some less trivial API. The main complexity there would be just figuring out the header arrangement so that Fuchsia's header could #include something to define the types it's supposed to use. But AFAICT it doesn't really need to be in guarded_pool_allocator.h like this. That generic header could just declare static ThreadLocalPackedVariables *getThreadLocals();. Then the platform header can provide the inline defn outside the class scope, and that header only needs to be used in guarded_pool_allocator.cpp itself.

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h

210

Seems like this should be static since it can never return something that's actually specific to the instance.

[hctim]

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h

211

Prefer not to have #ifdefs in generic code.

Can you instead have:

>> here:
#include "gwp_asan/platform_specific/guarded_pool_allocator_tls.h"

>> gwp_asan/platform_specific/guarded_pool_allocator_tls.h:
struct ThreadLocalPackedVariables { ... }
#if defined(__Fuchsia__)
#include "gwp_asan/platform_specific/guarded_pool_allocator_tls_fuchsia.h"
#else if defined(__unix__)
#include "gwp_asan/platform_specific/guarded_pool_allocator_tls_posix.h"
#endif

>> gwp_asan/platform_specific/guarded_pool_allocator_tls_fuchsia.h
static ThreadLocalPackedVariables *getThreadLocals() {
  return reinterpret_cast<ThreadLocalPackedVariables *>(
        getPlatformGwpAsanTlsSlot());
}

>> gwp_asan/platform_specific/guarded_pool_allocator_tls_posix.h:
static ThreadLocalPackedVariables *getThreadLocals() {
    alignas(8) static GWP_ASAN_TLS_INITIAL_EXEC ThreadLocalPackedVariables Locals;
    return &Locals;
}

I think that if we could rely on LTO to inline things correctly then just the implementation would go in a platform-specific cpp file - but unfortunately we don't have that luxury everywhere, so it seems to me like the best solution to ensure inlining is platform-specific headers. In future, I'd like to give mutex.h the same treatment, as I'm really unhappy about the platform ifdefs there as well.

[mcgrathr]

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h

211

I think that's fine, but the Fuchsia header would just be a trivial wrapper around yet another header, since the real implementation will not be in the gwp_asan source tree at all. So it would be most convenient if we could make it:

#ifdef GWP_ASAN_PLATFORM_TLS_HEADER
#include GWP_ASAN_PLATFORM_TLS_HEADER
#else
inline ThreadLocalPackedVariables *GuardedPoolAllocator::getThreadLocals() {
    alignas(8) static GWP_ASAN_TLS_INITIAL_EXEC ThreadLocalPackedVariables Locals;
    return &Locals;
}
#endif

Then the Fuchsia case can just provide the header directly instead of adding yet another layer.

[hctim]

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h

211

This sounds reasonable to me.

[cryptoad]

Address review comments.

This moves the thread local definitions and access wrapper in a
platform specific header, which should capture Mitch's and Roland's
suggestions.

[hctim]

LGTM w/ nits.

compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_tls.h

46

Any ideas why clang-tidy is reporting this?

compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp

27

using old clang-format instead of new?

compiler-rt/lib/gwp_asan/utilities.cpp

40

same thing - need new clang-format?

[cryptoad]

compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_tls.h

46

Maybe because it doesn't have stdint.h in this file so it doesn't know it's constant.

compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp

27

Actually using the tot one :/
I think it's aligning this comment with the #endif one.

[cryptoad]

Adding stdint.h to try and quell some clang-tidy warnings.

compiler-rt/lib/gwp_asan/CMakeLists.txt

 set(GWP_ASAN_HEADERS
   common.h
   crash_handler.h
   definitions.h
   guarded_pool_allocator.h
   mutex.h
   options.h
   options.inc
+  platform_specific/guarded_pool_allocator_tls.h
   stack_trace_compressor.h
   utilities.h
 )
 
 # Ensure that GWP-ASan meets the delegated requirements of some supporting
 # allocators. Some supporting allocators (e.g. scudo standalone) cannot use any
 # parts of the C++ standard library.
 set(GWP_ASAN_CFLAGS ${SANITIZER_COMMON_CFLAGS} -fno-rtti -fno-exceptions

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h

 //===-- guarded_pool_allocator.h --------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef GWP_ASAN_GUARDED_POOL_ALLOCATOR_H_
 #define GWP_ASAN_GUARDED_POOL_ALLOCATOR_H_
 
 #include "gwp_asan/common.h"
 #include "gwp_asan/definitions.h"
 #include "gwp_asan/mutex.h"
 #include "gwp_asan/options.h"
+#include "gwp_asan/platform_specific/guarded_pool_allocator_tls.h"
 #include "gwp_asan/stack_trace_compressor.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 namespace gwp_asan {
 // This class is the primary implementation of the allocator portion of GWP-
 // ASan. It is the sole owner of the pool of sequentially allocated guarded
public:
   // Return whether the allocation should be randomly chosen for sampling.
   GWP_ASAN_ALWAYS_INLINE bool shouldSample() {
     // NextSampleCounter == 0 means we "should regenerate the counter".
     //                   == 1 means we "should sample this allocation".
     // AdjustedSampleRatePlusOne is designed to intentionally underflow. This
     // class must be valid when zero-initialised, and we wish to sample as
     // infrequently as possible when this is the case, hence we underflow to
     // UINT32_MAX.
-    if (GWP_ASAN_UNLIKELY(ThreadLocals.NextSampleCounter == 0))
-      ThreadLocals.NextSampleCounter =
+    if (GWP_ASAN_UNLIKELY(getThreadLocals()->NextSampleCounter == 0))
+      getThreadLocals()->NextSampleCounter =
           ((getRandomUnsigned32() % (AdjustedSampleRatePlusOne - 1)) + 1) &
           ThreadLocalPackedVariables::NextSampleCounterMask;
 
-    return GWP_ASAN_UNLIKELY(--ThreadLocals.NextSampleCounter == 0);
+    return GWP_ASAN_UNLIKELY(--getThreadLocals()->NextSampleCounter == 0);
   }
 
   // Returns whether the provided pointer is a current sampled allocation that
   // is owned by this pool.
   GWP_ASAN_ALWAYS_INLINE bool pointerIsMine(const void *Ptr) const {
     return State.pointerIsMine(Ptr);
   }
 
private:
   // The adjusted sample rate for allocation sampling. Default *must* be
   // nonzero, as dynamic initialisation may call malloc (e.g. from libstdc++)
   // before GPA::init() is called. This would cause an error in shouldSample(),
   // where we would calculate modulo zero. This value is set UINT32_MAX, as when
   // GWP-ASan is disabled, we wish to never spend wasted cycles recalculating
   // the sample rate.
   uint32_t AdjustedSampleRatePlusOne = 0;
 
-  // Pack the thread local variables into a struct to ensure that they're in
-  // the same cache line for performance reasons. These are the most touched
-  // variables in GWP-ASan.
-  struct alignas(8) ThreadLocalPackedVariables {
-    constexpr ThreadLocalPackedVariables()
-        : RandomState(0xff82eb50), NextSampleCounter(0), RecursiveGuard(false) {
-    }
-    // Initialised to a magic constant so that an uninitialised GWP-ASan won't
-    // regenerate its sample counter for as long as possible. The xorshift32()
-    // algorithm used below results in getRandomUnsigned32(0xff82eb50) ==
-    // 0xfffffea4.
-    uint32_t RandomState;
-    // Thread-local decrementing counter that indicates that a given allocation
-    // should be sampled when it reaches zero.
-    uint32_t NextSampleCounter : 31;
-    // The mask is needed to silence conversion errors.
-    static const uint32_t NextSampleCounterMask = (1U << 31) - 1;
-    // Guard against recursivity. Unwinders often contain complex behaviour that
-    // may not be safe for the allocator (i.e. the unwinder calls dlopen(),
-    // which calls malloc()). When recursive behaviour is detected, we will
-    // automatically fall back to the supporting allocator to supply the
-    // allocation.
-    bool RecursiveGuard : 1;
-  };
-  static_assert(sizeof(ThreadLocalPackedVariables) == sizeof(uint64_t),
-                "thread local data does not fit in a uint64_t");
-
   class ScopedRecursiveGuard {
   public:
-    ScopedRecursiveGuard() { ThreadLocals.RecursiveGuard = true; }
-    ~ScopedRecursiveGuard() { ThreadLocals.RecursiveGuard = false; }
+    ScopedRecursiveGuard() { getThreadLocals()->RecursiveGuard = true; }
+    ~ScopedRecursiveGuard() { getThreadLocals()->RecursiveGuard = false; }
   };
 
   // Initialise the PRNG, platform-specific.
   void initPRNG();
 
   // xorshift (32-bit output), extremely fast PRNG that uses arithmetic
   // operations only. Seeded using platform-specific mechanisms by initPRNG().
   uint32_t getRandomUnsigned32();
-
-  static GWP_ASAN_TLS_INITIAL_EXEC ThreadLocalPackedVariables ThreadLocals;
 };
 } // namespace gwp_asan
 
 #endif // GWP_ASAN_GUARDED_POOL_ALLOCATOR_H_

compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp

 namespace gwp_asan {
 namespace {
 // Forward declare the pointer to the singleton version of this class.
 // Instantiated during initialisation, this allows the signal handler
 // to find this class in order to deduce the root cause of failures. Must not be
 // referenced by users outside this translation unit, in order to avoid
 // init-order-fiasco.
 GuardedPoolAllocator *SingletonPtr = nullptr;
+
+size_t roundUpTo(size_t Size, size_t Boundary) {
+  return (Size + Boundary - 1) & ~(Boundary - 1);
+}
+
+uintptr_t getPageAddr(uintptr_t Ptr, uintptr_t PageSize) {
+  return Ptr & ~(PageSize - 1);
+}
 } // anonymous namespace
 
 // Gets the singleton implementation of this class. Thread-compatible until
 // init() is called, thread-safe afterwards.
 GuardedPoolAllocator *GuardedPoolAllocator::getSingleton() {
   return SingletonPtr;
 }
 
-static size_t roundUpTo(size_t Size, size_t Boundary) {
-  return (Size + Boundary - 1) & ~(Boundary - 1);
-}
-
 void GuardedPoolAllocator::init(const options::Options &Opts) {
   // Note: We return from the constructor here if GWP-ASan is not available.
   // This will stop heap-allocation of class members, as well as mmap() of the
   // guarded slots.
   if (!Opts.Enabled || Opts.SampleRate == 0 ||
       Opts.MaxSimultaneousAllocations == 0)
     return;
 
void GuardedPoolAllocator::init(const options::Options &Opts) {
   // Multiply the sample rate by 2 to give a good, fast approximation for (1 /
   // SampleRate) chance of sampling.
   if (Opts.SampleRate != 1)
     AdjustedSampleRatePlusOne = static_cast<uint32_t>(Opts.SampleRate) * 2 + 1;
   else
     AdjustedSampleRatePlusOne = 2;
 
   initPRNG();
-  ThreadLocals.NextSampleCounter =
+  getThreadLocals()->NextSampleCounter =
       ((getRandomUnsigned32() % (AdjustedSampleRatePlusOne - 1)) + 1) &
       ThreadLocalPackedVariables::NextSampleCounterMask;
 
   State.GuardedPagePool = reinterpret_cast<uintptr_t>(GuardedPoolMemory);
   State.GuardedPagePoolEnd =
       reinterpret_cast<uintptr_t>(GuardedPoolMemory) + PoolBytesRequired;
 
   if (Opts.InstallForkHandlers)
void GuardedPoolAllocator::uninitTestOnly() {
   if (FreeSlots) {
     unmap(FreeSlots,
           roundUpTo(State.MaxSimultaneousAllocations * sizeof(*FreeSlots),
                     State.PageSize));
     FreeSlots = nullptr;
   }
 }
 
-static uintptr_t getPageAddr(uintptr_t Ptr, uintptr_t PageSize) {
-  return Ptr & ~(PageSize - 1);
-}
-
 void *GuardedPoolAllocator::allocate(size_t Size) {
   // GuardedPagePoolEnd == 0 when GWP-ASan is disabled. If we are disabled, fall
   // back to the supporting allocator.
   if (State.GuardedPagePoolEnd == 0) {
-    ThreadLocals.NextSampleCounter =
+    getThreadLocals()->NextSampleCounter =
         (AdjustedSampleRatePlusOne - 1) &
         ThreadLocalPackedVariables::NextSampleCounterMask;
     return nullptr;
   }
 
   // Protect against recursivity.
-  if (ThreadLocals.RecursiveGuard)
+  if (getThreadLocals()->RecursiveGuard)
     return nullptr;
   ScopedRecursiveGuard SRG;
 
   if (Size == 0 || Size > State.maximumAllocationSize())
     return nullptr;
 
   size_t Index;
   {
void GuardedPoolAllocator::trapOnAddress(uintptr_t Address, Error E) {
 
   // Raise a SEGV by touching first guard page.
   volatile char *p = reinterpret_cast<char *>(State.GuardedPagePool);
   *p = 0;
   __builtin_unreachable();
 }
 
 void GuardedPoolAllocator::stop() {
-  ThreadLocals.RecursiveGuard = true;
+  getThreadLocals()->RecursiveGuard = true;
   PoolMutex.tryLock();
 }
 
 void GuardedPoolAllocator::deallocate(void *Ptr) {
   assert(pointerIsMine(Ptr) && "Pointer is not mine!");
   uintptr_t UPtr = reinterpret_cast<uintptr_t>(Ptr);
   size_t Slot = State.getNearestSlot(UPtr);
   uintptr_t SlotStart = State.slotToAddr(Slot);
// pool during the expensive markInaccessible() call.
 
     // Ensure that the deallocation is recorded before marking the page as
     // inaccessible. Otherwise, a racy use-after-free will have inconsistent
     // metadata.
     Meta->RecordDeallocation();
 
     // Ensure that the unwinder is not called if the recursive flag is set,
     // otherwise non-reentrant unwinders may deadlock.
-    if (!ThreadLocals.RecursiveGuard) {
+    if (!getThreadLocals()->RecursiveGuard) {
       ScopedRecursiveGuard SRG;
       Meta->DeallocationTrace.RecordBacktrace(Backtrace);
     }
   }
 
   deallocateInGuardedPool(reinterpret_cast<void *>(SlotStart),
                           State.maximumAllocationSize());
 

 }
 
 void GuardedPoolAllocator::freeSlot(size_t SlotIndex) {
   assert(FreeSlotsLength < State.MaxSimultaneousAllocations);
   FreeSlots[FreeSlotsLength++] = SlotIndex;
 }
 
 uint32_t GuardedPoolAllocator::getRandomUnsigned32() {
-  uint32_t RandomState = ThreadLocals.RandomState;
+  uint32_t RandomState = getThreadLocals()->RandomState;
   RandomState ^= RandomState << 13;
   RandomState ^= RandomState >> 17;
   RandomState ^= RandomState << 5;
-  ThreadLocals.RandomState = RandomState;
+  getThreadLocals()->RandomState = RandomState;
   return RandomState;
 }
-
-GWP_ASAN_TLS_INITIAL_EXEC
-GuardedPoolAllocator::ThreadLocalPackedVariables
-    GuardedPoolAllocator::ThreadLocals;
 } // namespace gwp_asan

compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_posix.cpp

#endif // ANDROID
   // Anonymous mapping names are only supported on Android.
   return;
 }
 } // anonymous namespace
 
 namespace gwp_asan {
 
 void GuardedPoolAllocator::initPRNG() {
-  ThreadLocals.RandomState =
+  getThreadLocals()->RandomState =
       static_cast<uint32_t>(time(nullptr) + getThreadID());
 }
 
 void *GuardedPoolAllocator::map(size_t Size, const char *Name) const {
   assert((Size % State.PageSize) == 0);
   void *Ptr = mmap(nullptr, Size, PROT_READ | PROT_WRITE,
                    MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
   Check(Ptr != MAP_FAILED, "Failed to map guarded pool allocator memory");

compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_tls.h

+//===-- guarded_pool_allocator_tls.h ----------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef GWP_ASAN_GUARDED_POOL_ALLOCATOR_TLS_H_
+#define GWP_ASAN_GUARDED_POOL_ALLOCATOR_TLS_H_
+
+#include "gwp_asan/definitions.h"
+
+#include <stdint.h>
+
+namespace gwp_asan {
+// Pack the thread local variables into a struct to ensure that they're in
+// the same cache line for performance reasons. These are the most touched
+// variables in GWP-ASan.
+struct ThreadLocalPackedVariables {
+  constexpr ThreadLocalPackedVariables()
+      : RandomState(0xacd979ce), NextSampleCounter(0), RecursiveGuard(false) {}
+  // Initialised to a magic constant so that an uninitialised GWP-ASan won't
+  // regenerate its sample counter for as long as possible. The xorshift32()
+  // algorithm used below results in getRandomUnsigned32(0xacd979ce) ==
+  // 0xfffffffe.
+  uint32_t RandomState;
+  // Thread-local decrementing counter that indicates that a given allocation
+  // should be sampled when it reaches zero.
+  uint32_t NextSampleCounter : 31;
+  // The mask is needed to silence conversion errors.
+  static const uint32_t NextSampleCounterMask = (1U << 31) - 1;
+  // Guard against recursivity. Unwinders often contain complex behaviour that
+  // may not be safe for the allocator (i.e. the unwinder calls dlopen(),
+  // which calls malloc()). When recursive behaviour is detected, we will
+  // automatically fall back to the supporting allocator to supply the
+  // allocation.
+  bool RecursiveGuard : 1;
+};
+static_assert(sizeof(ThreadLocalPackedVariables) == sizeof(uint64_t),
+              "thread local data does not fit in a uint64_t");
+
+#ifdef GWP_ASAN_PLATFORM_TLS_HEADER
+#include GWP_ASAN_PLATFORM_TLS_HEADER
+#else
+inline ThreadLocalPackedVariables *getThreadLocals() {
+  alignas(8) static GWP_ASAN_TLS_INITIAL_EXEC ThreadLocalPackedVariables Locals;
+  return &Locals;
+}
+#endif
+} // namespace gwp_asan
+
+#endif // GWP_ASAN_GUARDED_POOL_ALLOCATOR_TLS_H_

compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp

 #endif
 
 namespace gwp_asan {
 void die(const char *Message) {
 #ifdef __BIONIC__
   if (&android_set_abort_message != nullptr)
     android_set_abort_message(Message);
   abort();
-#else // __BIONIC__
+#else  // __BIONIC__
   fprintf(stderr, "%s", Message);
   __builtin_trap();
 #endif // __BIONIC__
 }
 } // namespace gwp_asan

compiler-rt/lib/gwp_asan/utilities.cpp

static size_t alignPowerOfTwo(size_t RealAllocationSize) {
   if (RealAllocationSize % 16 == 0)
     return RealAllocationSize;
   return RealAllocationSize + 16 - (RealAllocationSize % 16);
 }
 
 #ifdef __BIONIC__
 static constexpr AlignmentStrategy PlatformDefaultAlignment =
     AlignmentStrategy::BIONIC;
-#else // __BIONIC__
+#else  // __BIONIC__
 static constexpr AlignmentStrategy PlatformDefaultAlignment =
     AlignmentStrategy::POWER_OF_TWO;
 #endif // __BIONIC__
 
 size_t rightAlignedAllocationSize(size_t RealAllocationSize,
                                   AlignmentStrategy Align) {
   assert(RealAllocationSize > 0);
   if (Align == AlignmentStrategy::DEFAULT)