[llvm-dev] TSAN hack on AArch64 for Android

[Jason Kim via llvm-dev]

Hi Renato and gang.

I guess I need to back up a few steps. I missed this thread before I took off last week.

I had been discussing the TSAN on android/aarch64 issues with the folks at thread-sanitizer at googlegroups.com for several months now. I had not explicitly been including llvm-dev (and you), and that was a mistake on my part. Palm-on-face? :-(

Secondly, the "early release" of the gigantic patch for tsan+android+aarch64 was due to not being familiar with how phabricator does things by default. Sorry for the noise.  I expected it to add an audience only after I did an expliclit "make-public" step. I was wrong on that, but I should not have been. Bad tool! :-)

Now for what remains:

As far as I can tell, there are several big hurdles to this work landing on compiler-rt

Patch Size!
The patch in question is a bit smaller now thanks to some dovetailing with http://reviews.llvm.org/D11484 being merged.
I have already split up the patch into three chunks. I can do more as needed.

TLS!
I am willing to redo the TLS emulation part.
So there are several possible strategies for doing this -- this was explicltly mentioned in the commit message, but it seems to have gotten lost in phabricator somehow.

1. use pthread_get/setspecific()  (more on this below!!)
2. use some other workaround that does not use pthread API
3. use the new TLS slot from Dan's recent push to android. https://android-review.googlesource.com/#/c/167420/
4. use the emutls work http://reviews.llvm.org/D12001

The first approach fails because pthread_key maintenance happens at thread/process destruction time, which means that
a NEW key was being generated for the thread undergoing death, resulting in a new ThreadState being allocated for the dying thread. 
The second approach was what I selected (and what is on display at phabricator). I works, but isn't necessarily very pretty.
The third and fourth approaches  are recent, and I have not yet had a chance to take a look.
At first blush, emutls seems the most problematic, as it uses intercepted calls like malloc(). 

TESTS!
 Currently, about 2/3 tests for tsan fail/flake on android+aarch64.
They aren't xfailed, because some of them are flaky, and thus unexpectedly pass at times. 
So I decided to mark them as unsupported on android+aarch64, and thus is "green". 
These tests will need to triaged individually. 

*If anyone has a simple hack to mark a set of tests as unsupported that does not touch individual test files, please let me know!*
*Whether I move them to a directory or prepend 2 lines is roughly the same size patch!*

ANDROID-BIONIC!

The required patches on bionic adds private symbols to select APIs in bionic so that TSAN can not intercept bionic-to-bionic calls. https://code.google.com/p/android/issues/detail?id=179564

Now, it turns out that some of the early workarounds specific to android (such as early addition of REAL versions of memset() and friends even before dlsym() is called()) becomes unnecessary with the bionic patches. This also implies that I should be able to go back to using the pthread API for TLS, because TSAN will no longer be intercepting calls at thread/process destruction time
I have not yet had a chance to test this out. Assuming this works,  what is your opinion on the "proper" TLS emulation in TSAN?

a) always use pthread api?
b) leave the use of __thread keyword there and use some variant of arch+os  conditionals to select the appropriate technique?

If it's (b), what is the appropriate technique?
i.e. use pthread API selectively,
or  use some other arch+OS specific workaround?

Thanks
-Jason

p.s.
I handily dismiss the "nuclear" option, which is a complete reimplementation of tsan to avoid weak-symbol aliasing, and to do manual interception/patching of dynamically linked routines. Yeah, ignore this one, please.


 > -----Original Message-----
 > From: Dmitry Vyukov [mailto:dvyukov at google.com]
 > Sent: Wednesday, August 19, 2015 4:21 AM
 > To: Renato Golin
 > Cc: Kostya Serebryany; Jason Kim; Tamas Berghammer; Dan Albert; Stephen
 > Hines; Tim Northover; kanheim at a-bix.com; Chad Rosier; Evgenii Stepanov;
 > Adhemerval Zanella; LLVM Dev; Kristof Beyls; James Molloy
 > Subject: Re: TSAN hack on AArch64 for Android
 > 
 > On Wed, Aug 19, 2015 at 1:15 PM, Renato Golin <renato.golin at linaro.org>
 > wrote:
 > > On 19 August 2015 at 11:29, Dmitry Vyukov <dvyukov at google.com>
 > wrote:
 > >> Wait, this change is not submitted yet, right? Or you mean mailing of
 > >> this change in bad shape?
 > >
 > > Right.
 > >
 > > Jason has submitted high quality patches before, so this is in no way
 > > a reprimand to his submission. I am specifically worried about the
 > > lack of higher level questions from the reviewers / code owners about
 > > the general idea of the patch, not with the submission itself.
 > >
 > >
 > >> I consider this change as work-in-progress where author is looking
 > >> for feedback on his ongoing progress. I guess the change description
 > >> should have been spelled this very explicitly.
 > >
 > > For that kind of work, we normally follow the guidelines:
 > >
 > > 1. Send an email to the list explaining the problem / design, have
 > > some high level discussions 2. Send an "[RFC]" patch, explicitly
 > > identified as "not-for-commit"
 > > quality, have some specific discussions 3. Send *another* patch, for
 > > commit, with loads of tests, comments, FIXMEs, have final review
 > >
 > > People normally ignore most spurious errors from RFC patches, but if
 > > it's not marked as such, you'll get a lot of heat.
 > >
 > > Also, it is good practice to have a few steps *before* sending a big
 > > final patch:
 > >
 > > 1. Compile your code in different ways (debug/release/asserts) on at
 > > least x86_64/ARM/AArch64 and run check-all with at least
 > > LLVM+Clang+"your-component".
 > > 2. If you're targeting a different platform, make sure you run the
 > > tests on them, too. (native/cross, doesn't matter much) 3. Make sure
 > > you add enough tests, or describe extensively what tests you have made
 > > to be sure the patch is good 4. Review your diff cautiously, to make
 > > sure no left-overs remain:
 > >   * no commented out code
 > >   * no spurious whitespace changes
 > >   * no wrong comments, or fixed FIXME's left
 > >
 > >
 > >> This change definitely needs more work to be submitted, splitting
 > >> into smaller patches with a plan for submission order, refactoring,
 > >> cleanup, etc. That is no question.
 > >
 > > More importantly, this patch may be moot, because Joerg's work already
 > > does that, and Dan's patch to Android makes the remaining step a
 > > 5-line patch.
 > >
 > > *That* is the question. How comfortable are we to add work-arounds
 > > before even questioning if the proper solution is possible /
 > > available.
 > >
 > > The submission is explicit: "This is a work-around, a better
 > > implementation would be X". It is our job to evaluate the real cost of
 > > X and see if it's worth putting a hack.
 > >
 > > It seems the cost is low, or paid already, so the patch should have
 > > been refused before any real review was made.
 > >
 > >
 > >> As for the design, there is not much to design upfront, as you
 > >> discover most of the issue only when you hit them (I would not
 > >> foresee them).
 > >
 > > TLS emulation vs proper implementation? Emulation in TSAN or RT or
 > > some other lib?
 > >
 > > TSAN is hardly the place for run-time library calls, RT is a much
 > > better place. Work arounds in LLVM is hardly a good replacement for a
 > > proper implementation in the Android linker.
 > >
 > > All those are design decisions. In the end, RT has already a TLS
 > > emulation coming about and Android just got a TLS slot for TSAN. Good
 > > design decisions stemmed from design discussions in the list.
 > >
 > >
 > >> Refactoring of tsan per-thread structures in preparation for the
 > >> "emulation" should go in independently. Agree.
 > >> Aarch64 support is already submitted to tsan as far as I see.
 > >
 > > Not yet. This is still work in progress.
 > >
 > > Adhemerval has implemented the functionality and we have tested on
 > our
 > > hardware. But there are issues with other hardware we don't have
 > > access to, so progress has slowed down considerably. We're working on
 > > some AArch64 buildbots to speed that up a lot, but as usual,
 > > unforeseen complications hunt our sleep. :)
 > >
 > > Despite the experimental nature of our TSAN and Joerg's TLS emulation
 > > implementations, I think it's enough to give Jason a working base to
 > > start from scratch and use that to make TSAN work on Android (with
 > > Dan's change).
 > 
 > Sounds good to me.