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

[Mitch Phillips via Phabricator via llvm-commits]

hctim 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 *>(
----------------
mcgrathr wrote:
> 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.
> 
This sounds reasonable to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90195