This is an archive of the discontinued LLVM Phabricator instance.

Port stand-alone lsan to Android (revised)
Needs ReviewPublic

Authored by thurston on Jul 21 2016, 9:54 AM.

Details

Reviewers
eugenis
Summary

Revised as per the feedback to D22549: this abstracts cache/disable_counter/current_thread_tid (stored in TLS_SLOT_TSAN on Android, THREADLOCAL variables otherwise) into GetCurrentThread().

Diff Detail

Event Timeline

thurston updated this revision to Diff 64913.Jul 21 2016, 9:54 AM
thurston retitled this revision from to Port stand-alone lsan to Android (revised).
thurston updated this object.
thurston added a reviewer: eugenis.
thurston added a project: Restricted Project.
eugenis added inline comments.Jul 21 2016, 1:18 PM
lsan_allocator.cc
31 ↗(On Diff #64913)

Get rid of this; always go though LsanThread. See how ASan does it (AsanThread, AsanThreadLocalMallocStorage).

49 ↗(On Diff #64913)

remove the printf, or make it a vprintf

55 ↗(On Diff #64913)

why??

lsan_allocator.h
27 ↗(On Diff #64913)

Move this to sanitizer_common.h, smth like void **AndroidGetTls() { return &__get_tls()[TLS_SLOT_TSAN];}

36 ↗(On Diff #64913)

function, not a macro

73 ↗(On Diff #64913)

Call this LsanThread, use it on all platforms.

lsan_common.cc
38 ↗(On Diff #64913)

get rid of this; always go through LsanThread

54 ↗(On Diff #64913)

replace with getCurrentThread()->disable_counter

lsan_thread.cc
29 ↗(On Diff #64913)

LsanThread

thurston updated this revision to Diff 65178.Jul 22 2016, 3:21 PM

There's now an LsanThread struct containing all the thread-local variables. A pointer to the struct is returned from lsan_thread.cc::getCurrentTLS(), which is the single point where we distinguish between Android (use AndroidGetTls()) and non-Android (use THREADLOCAL).

eugenis added inline comments.Jul 26 2016, 11:22 AM
lsan/lsan_allocator.h
20 ↗(On Diff #65178)

looks unnecessary

lsan/lsan_common.h
130

unnecessary

132

Please move this to lsan_thread.h

135

just "tid"

lsan/lsan_flags.inc
34 ↗(On Diff #65178)

this could be written as

LSAN_FLAG(bool, use_tls, !SANITIZER_ANDROID,

but on the second thought, don't we need to handle pthread_setspecific storage on android? did you test that it works?

I think this flag still makes sense for android, and instead of defaulting to false we should modify the code to do the right thing for the platform, i.e. scan TSD and not DTLS.

lsan/lsan_thread.cc
15

no system headers here
why is this needed?

43

merge this into #if SANITIZER_ANDROID .. #else below (line 70).

49

rename this to GetCurrentThread for consistency with asan;
replace existing calls to u32 GetCurrentThread() with GetCurrentThread()->id.

sanitizer_common/sanitizer_common.cc
494 ↗(On Diff #65178)

move this to the header so it can be inlined

sanitizer_common/sanitizer_common.h
845

this will break asan on arm

thurston updated this revision to Diff 65610.Jul 26 2016, 2:51 PM
thurston updated this object.

This addresses all the previous comments
(use_tls is not overridden for Android, GetTls() updated instead to return the bionic TLS slots;
GetCurrentThread() changed to GetCurrentThread()->id, getCurrentTLS() changed to GetCurrentThread();
SIG_SETMASK copied from signal.h;
removed some unnecessary code and moved some code around)

eugenis added inline comments.Jul 26 2016, 5:01 PM
lsan/lsan_allocator.cc
37

() unnecessary

lsan/lsan_common.cc
248

Is the special case for android needed because allocator cache is not inside tls limits?
Could you check that condition here instead?
cache_begin == cache_end || cache_begin >= tls_end || cache_end <= tls_begin

lsan/lsan_thread.cc
26

this should be moved to sanitizer_platform_limits_posix, see __sanitizer::sig_dfl

sanitizer_common/sanitizer_common.h
848

Are you sure this does not break asan on arm still?
__get_tls is undefined.
Please test that asan on arm at least compiles.

eugenis added inline comments.Jul 26 2016, 5:02 PM
sanitizer_common/sanitizer_linux_libcdep.cc
382

this is super scary btw, but I don't see a better way.
BIONIC_TLS_SLOTS may change at any time (and it has, quite recently)

thurston marked 2 inline comments as done.Aug 11 2016, 2:04 PM
thurston added inline comments.
lsan/lsan_allocator.cc
37

In the next revision, I've removed these and some other similar parentheses

sanitizer_common/sanitizer_common.h
848

In the next revision, I've defined __get_tls and tested that asan on arm builds within the Android source tree.

thurston updated this revision to Diff 67744.Aug 11 2016, 2:05 PM

Addresses previous comments

N.B. Some changes in sanitizer_common overlap with changes proposed for msan

eugenis added inline comments.Aug 11 2016, 2:39 PM
sanitizer_common/sanitizer_common.h
848

By next revision, do you mean something that's not yet uploaded?
I don't see this code anywhere.

thurston added inline comments.Aug 11 2016, 2:45 PM
sanitizer_common/sanitizer_common.h
848

My comment is wrong. I ended up taking an alternative approach: if __get_tls is not defined, then define AndroidGetTls to return nullptr (line 850). This lets sanitizer_common compile on 32-bit with SANITIZER_ANDROID, even though AndroidGetTls won't actually be used (since it is only called by lsan or msan, which won't run on 32-bit).

thurston updated this revision to Diff 67750.Aug 11 2016, 2:59 PM

Add back the sanitizer_common/sanitizer_linux_libcdep.cc TLS changes from diff 3 - I accidentally omitted them.
All other files are the same as per diff 4.

thurston added inline comments.Aug 11 2016, 8:26 PM
sanitizer_common/sanitizer_common.h
848

Oops - in revision 3 I'd been missing sanitizer_common/sanitizer_linux_libcdep.cc TLS changes so this compiled, but having added them back, since GetTls in that file directly uses get_tls (not AndroidGetTls), I still need to define get_tls.

thurston updated this revision to Diff 67804.Aug 12 2016, 1:10 AM

On 32-bit Android, GetTls() won't use __get_tls() (which is undefined)
Defines sig_setmask