[llvm-dev] TSAN hack on AArch64 for Android

[Dmitry Vyukov via llvm-dev]

On Wed, Aug 26, 2015 at 11:12 PM, Renato Golin <renato.golin at linaro.org> wrote:
> On 26 August 2015 at 20:13, Jason Kim <jasonk at codeaurora.org> wrote:
>> 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? :-(
>
> Yup, that would have saved a lot of emails... :)
>
>
>> 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.
>
> Before I have any opinion on the current patch, we need to know what's
> going to change.
>
>
>> 1. use pthread_get/setspecific()  (more on this below!!)
>
> If the re-creation of the thread state disappears after the bionic
> changes, I'd seriously investigate this one, as it seems the most
> logic.
>
>
>> 2. use some other workaround that does not use pthread API
>
> I'm trying to avoid this at all costs. :)
>
>
>> 3. use the new TLS slot from Dan's recent push to android. https://android-review.googlesource.com/#/c/167420/

/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\

This looks like the best option to me.
(provided that we unify tsan thread-local state in ThreadState, so
that we change only cur_thread function)

> I asked somewhere else, but, will this give you a reasonably looking
> thread state without resorting to pthread_get/specific calls? Will
> this be a unique and non-redundant state for each thread?
>
>
>> 4. use the emutls work http://reviews.llvm.org/D12001
>
> I only suggested this because Joerg/Dan mentioned it. I haven't looked
> at that patch in detail. From what I'm seeing, options 1 and 3 above
> may very well suit us.
>
>
>> 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".
>
> So, if they currently pass on the production buildbots, you can't mark
> them as unstable because of your patch, as that will mean your patch
> is making them unstable, and we can't just make dozens of tests
> unstable for any amount of features.
>
> If your patch introduces the instability, you'll have to fix them
> before commit. That's why I suggested splitting it into many small
> chunks, none of which will leave the tree in an unstable state,
> because you'll have time to look at each instability individually, and
> fix them before commit. The bots will also be happier, and every one
> wins.
>
> I have no idea how to split your patch so that this happens, but since
> you know what the instability is, we could work together to test and
> validate *before* the patches go live. I have a fast AArch64 machine
> just for that purpose, so we can do this as fast as possible.
>
>
>> 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?
>
> I'm not familiar with pthread enough to give you a good answer. I'm
> hoping Joerg/Dan could chime in. Having said that, I'd rather use
> something that is arch/os agnostic, even if it slows it down a few
> percent. If the slow down is too great, then we can discuss case by
> case.
>
> cheers,
> -renato