[PATCH] D90195: [GWP-ASan] Abstract the thread local variables access

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:246
+  ThreadLocalPackedVariables *getThreadLocals() {
+#if GWP_ASAN_HAS_PLATFORM_TLS_SLOT
+    return reinterpret_cast<ThreadLocalPackedVariables *>(
----------------
hctim wrote:
> 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.
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.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90195/new/

https://reviews.llvm.org/D90195