[PATCH] D64457: [GWP-ASan] Attempt to fix Android/ARM platforms.

Wed Feb 5 15:30:32 PST 2020

hctim marked 6 inline comments as done.
hctim added inline comments.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:258
   // variables in GWP-ASan.
-  struct alignas(8) ThreadLocalPackedVariables {
+  struct alignas(64) ThreadLocalPackedVariables {
     constexpr ThreadLocalPackedVariables() {}
----------------
eugenis wrote:
> pcc wrote:
> > hctim wrote:
> > > eugenis wrote:
> > > > pcc wrote:
> > > > > hctim wrote:
> > > > > > hctim wrote:
> > > > > > > This alignment is manually required, as otherwise with my NDK (r18b) the bionic linker complains when running the test that the `executable's TLS segment is underaligned: alignment is 8, needs to be at least 64 for ARM64 Bionic`. Apparently this is fixed in the new NDKv21, but we can't update our bots to that at this time as the migration is nontrivial :( (NDKv21 breaks other things in standalone compiler-rt).
> > > > > > @pcc - do you think this is a reasonable solution to get it to build without pushing the new NDK/API version to the bots?
> > > > > I don't recall, did just upgrading the NDK break things or did things only break when you upgraded the API level as well? If the former, maybe you could add `--target=aarch64-linux-android30` and so on to cflags/ldflags for the gwp-asan targets?
> > > > Simply updating the NDK breaks things. It could very well be trivial, I did not look into it at all beyond that fact that the bot turned red.
> > > Are we happy with this workaround for now folks?
> > This is fine for now but please add a comment.
> If we pack the thread local variables to put them on the same cacheline, I'd say this change is not only acceptable but even desirable.
We may take up more static TLS space, but given we should probably be part of the initial set anyway, that's not a problem.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64457