This is an archive of the discontinued LLVM Phabricator instance.

[GWP-ASan] Attempt to fix Android/ARM platforms.
AbandonedPublic

Authored by hctim on Jul 9 2019, 5:18 PM.

Details

Reviewers
cryptoad
eugenis
Summary

This patch re-enabled GWP-ASan for Android and other ARM platforms. It does two
things:

  1. Adds -fno-emulated-tls, as the clang driver still default compiles using emulated TLS on Android.
  2. Also uses sanitizer common unwinder on Android, as glibc backtrace() isn't available.

This patch should be monitored closely when submitting. I don't have the ARM32
machines that the patches initially broke to verify that this fixes the
problem, but it's a good attempt.

Event Timeline

hctim created this revision.Jul 9 2019, 5:18 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 9 2019, 5:18 PM
Herald added subscribers: llvm-commits, Restricted Project, jfb and 5 others. · View Herald Transcript

LGTM

This adds sanitizer_common libraries to scudo_minimal (but not to scudo_standalone). Is it OK with you, Kostya?

It's OK with me. Part of the reason for splitting out the symbolizer was some memory overhead as well, which we probably want to look into.
Is COMPILER_RT_HAS_GWP_ASAN going to be true by default for Android? If so we probably should loop in the toolchain team so that there is no surprise on their next toolchain update.

Can this be abandoned? Looks stale.

hctim planned changes to this revision.Nov 27 2019, 12:34 PM

Can this be abandoned? Looks stale.

I'll need to take a look and rethink this - is planned changes okay to get it off your dashboard?

Can this be abandoned? Looks stale.

I'll need to take a look and rethink this - is planned changes okay to get it off your dashboard?

Works thanks!

hctim updated this revision to Diff 240298.Jan 24 2020, 2:47 PM
  • Updates.
hctim marked an inline comment as done.Jan 24 2020, 2:52 PM

Reviving this a little. check-gwp_asan in my aarch64 crostool now WAI, with some warnings during compilation from ar and ranlib: unsupported GNU_PROPERTY_TYPE (5) type: 0xc0000000. Looks like this is the PAC/BTI properties from D62609, but doesn't really hurt us here.

@cryptoad - I hear through the grapevine that you're having similar initial-exec TLS alignment issues in the linker (as per the inline comment in this patch). Is that what's stopping you from adding scudo_standalone to aarch64/Android on the upstream (LLVM) bots?

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
258 ↗(On Diff #240298)

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).

Unit tests: fail. 62146 tests passed, 5 failed and 811 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: fail. clang-tidy found 1 errors and 0 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

hctim marked an inline comment as done.Jan 30 2020, 10:47 AM
hctim added a subscriber: pcc.
hctim added inline comments.
compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
258 ↗(On Diff #240298)

@pcc - do you think this is a reasonable solution to get it to build without pushing the new NDK/API version to the bots?

pcc added inline comments.Jan 30 2020, 11:05 AM
compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
258 ↗(On Diff #240298)

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?

eugenis added inline comments.Jan 30 2020, 11:22 AM
compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
258 ↗(On Diff #240298)

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.

hctim added inline comments.Feb 5 2020, 7:08 AM
compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
258 ↗(On Diff #240298)

Are we happy with this workaround for now folks?

pcc added inline comments.Feb 5 2020, 8:02 AM
compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
258 ↗(On Diff #240298)

This is fine for now but please add a comment.

eugenis accepted this revision.Feb 5 2020, 11:29 AM

LGTM

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
258 ↗(On Diff #240298)

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.

This revision is now accepted and ready to land.Feb 5 2020, 11:29 AM
hctim marked 6 inline comments as done.Feb 5 2020, 3:26 PM
hctim added inline comments.
compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
258 ↗(On Diff #240298)

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.

Is there anything holding this up?

On our end, we're able to build GWP-ASAN and Scudo for Android with this change. We haven't actually tried running them yet though :D

I filed https://bugs.llvm.org/show_bug.cgi?id=45014 for an Android compiler-rt compilation error I ran into for API 29. (I needed to build with API 29 because it's only API 29's libc.so that has __tls_get_addr and __aeabi_read_tp, which are needed for non-emulated TLS.)

hctim abandoned this revision.Jan 12 2021, 1:39 PM
hctim marked an inline comment as done.

(no longer relevant - much fresher patch should be coming soon to do the same thing)