This is an archive of the discontinued LLVM Phabricator instance.

Port msan and stand-alone lsan to Android
Needs ReviewPublic

Authored by thurston on Jul 19 2016, 4:19 PM.

Details

Reviewers
eugenis
Summary

thread cannot be used for lsan or msan on Android, because its emutls implementation relies on functions that will be intercepted. This revision replaces thread with a struct stored in TLS_SLOT_TSAN.

msan requires corresponding changes (D22550) to LLVM MemorySanitizer.cpp.

Diff Detail

Event Timeline

thurston updated this revision to Diff 64596.Jul 19 2016, 4:19 PM
thurston retitled this revision from to Port msan and stand-alone lsan to Android.
thurston updated this object.
thurston added a reviewer: eugenis.
thurston added a project: Restricted Project.
eugenis edited edge metadata.Jul 19 2016, 5:19 PM

Please reupload with context, see http://llvm.org/docs/Phabricator.html (git diff -U999999).

thurston updated this revision to Diff 64616.Jul 19 2016, 5:24 PM
thurston edited edge metadata.

Reuploaded with context

This is way too intrusive.
How about something like this:

keep LSan allocator cache thread-local on non-Android, store a pointer to it in TLS_SLOT_TSAN on Android. Implement Cache *getAllocatorCache() function that would do the right thing depending on the platform.

Move the contents of MsanTLSSlotStruct directly into MsanThread so that they can be accessed as, for example, getCurrentThread()->in_interceptor_scope. This might slow down other platforms a little bit, but that's acceptable. Again, no #ifdefs - the same code on all platforms. Change __msan::GetCurrentThread to use TLS_SLOT_TSAN on Android and a usual thread-local var on other platforms.

It would be easier to review if you did one of the tools first (maybe LSan?) and uploaded that as a separate change.

lib/lsan/lsan.cc
16

No system headers in this file.
This code should go in sanitizer_common as something similar to InstallDeadlySignalHandlers. We can use it in other sanitizers to dump allocator stats, for example.

Please upload it as a separate change.

111

This should be done after the flags are initialized.
Add a flag "handle_sighup" to sanitizer_flags.inc.

133

No need for #if / #endif

lib/msan/msan.cc
424

I imagine this would be very bad for performance and code size.

Instead, move these arrays to the beginning of MsanThread so that their offsets for TLS_SLOT_TSAN can be hardcoded and compile-time verified. Then make the compiler (MemorySanitizer.cpp) emit the tls access code inline. Search for "getIRStackGuard" for inspiration.

lib/sanitizer_common/sanitizer_platform_limits_posix.cc
238

move from under #if

lib/sanitizer_common/sanitizer_platform_limits_posix.h
177

There is the same declaration 9 lines below.

thurston added a comment.EditedJul 27 2016, 7:53 AM

This is way too intrusive.
How about something like this:

Move the contents of MsanTLSSlotStruct directly into MsanThread so that they can be accessed as, for example, getCurrentThread()->in_interceptor_scope. This might slow down other platforms a little bit, but that's acceptable. Again, no #ifdefs - the same code on all platforms. Change __msan::GetCurrentThread to use TLS_SLOT_TSAN on Android and a usual thread-local var on other platforms.

For MSan, would it be ok if I:

  • changed the compiler instrumentation to the emit the MsanTLSSlot access code inline
  • and used a GetTLS()-like abstraction to access MsanTLSSlot variables
  • but not move the MsanTLSSlot contents into MsanThread (i.e., keep the current separation of variables between struct MsanTLSSlot and MsanThread)?

There's a few variables (e.g., in_interceptor_scope) that are accessed very early on, before MsanThread::Create() has been (or can be) called. If I moved all TLS variables into MsanThread, I could allocate MsanThread in the TLS_SLOT as soon as in_interceptor_scope is needed, but some of the MsanThread contents are not initialized until MsanThread::Create()+SetCurrentThread(). This means Create and SetCurrentThread would no longer be handling the entire MsanThread state, which is a bit confusing, and difficult for the pthread_create interceptor (which uses Create+SetCurrentThread).

This is way too intrusive.
How about something like this:

Move the contents of MsanTLSSlotStruct directly into MsanThread so that they can be accessed as, for example, getCurrentThread()->in_interceptor_scope. This might slow down other platforms a little bit, but that's acceptable. Again, no #ifdefs - the same code on all platforms. Change __msan::GetCurrentThread to use TLS_SLOT_TSAN on Android and a usual thread-local var on other platforms.

For MSan, would it be ok if I:

  • changed the compiler instrumentation to the emit the MsanTLSSlot access code inline
  • and used a GetTLS()-like abstraction to access MsanTLSSlot variables
  • but not move the MsanTLSSlot contents into MsanThread (i.e., keep the current separation of variables between struct MsanTLSSlot and MsanThread)?

There's a few variables (e.g., in_interceptor_scope) that are accessed very early on, before MsanThread::Create() has been (or can be) called. If I moved all TLS variables into MsanThread, I could allocate MsanThread in the TLS_SLOT as soon as in_interceptor_scope is needed, but some of the MsanThread contents are not initialized until MsanThread::Create()+SetCurrentThread(). This means Create and SetCurrentThread would no longer be handling the entire MsanThread state, which is a bit confusing, and difficult for the pthread_create interceptor (which uses Create+SetCurrentThread).

OK, sounds reasonable.