This is an archive of the discontinued LLVM Phabricator instance.

[GWP-ASan] Abstract the thread local variables access
ClosedPublic

Authored by cryptoad on Oct 26 2020, 2:54 PM.

Details

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.

Diff Detail

Event Timeline

cryptoad created this revision.Oct 26 2020, 2:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2020, 2:54 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
cryptoad requested review of this revision.Oct 26 2020, 2:54 PM
mcgrathr accepted this revision.Oct 26 2020, 4:23 PM

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.

This revision is now accepted and ready to land.Oct 26 2020, 4:23 PM
hctim added inline comments.Oct 27 2020, 2:38 PM
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 added inline comments.Oct 27 2020, 2:52 PM
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 added inline comments.Oct 27 2020, 3:10 PM
compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
211

This sounds reasonable to me.

cryptoad updated this revision to Diff 301297.Oct 28 2020, 8:31 AM

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 accepted this revision.Oct 28 2020, 10:23 AM

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 added inline comments.Oct 28 2020, 10:51 AM
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 updated this revision to Diff 301335.Oct 28 2020, 10:54 AM

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

hctim accepted this revision.Oct 28 2020, 11:02 AM
This revision was automatically updated to reflect the committed changes.