llvm dev - Aug 2015 - TSAN hack on AArch64 for Android

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/
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

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

> -----Original Message-----
 > From: Renato Golin [mailto:renato.golin at
linaro.org]
> > 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.

[ jasonk --> ] The tests, AFAIK, currently "pass" for a somewhat
squiggly definition of "pass".
I get occasional fails on a local x86_64 build, w/o my patches.

 > If your patch introduces the instability, you'll have to fix them
before

[ jasonk --> ] The instabilities were there to begin with, AFAIK. Some of the
tests are run on a "deflake" regimen where they are repeated 10x,
looking for a specific output.

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

[ jasonk --> ]  Sorry, but I don't understand what you want here. The
tests I touched are marked unsupported (i.e. won't be run at all) ONLY for
aarch64-linux-androideabi. Is there something else you want? It should not
impact any other configuration of TSAN/compiler-rt as-is. I also guard TSAN with
a CMAKE variable so that it only gets built for aarch64 iff
COMPILER_RT_FORCE_TSAN_AARCH64=1
 


 > cheers,
 > -renato 

Thanks!
-Jason

IMO having to disable 2/3 of the tests means the patch isn't ready yet.

On Fri, Aug 28, 2015 at 9:31 AM, Jason Kim <jasonk at codeaurora.org>
wrote:
>
>
>  > -----Original Message-----
>  > From: Renato Golin [mailto:renato.golin at linaro.org]
> > > 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.
>
> [ jasonk --> ] The tests, AFAIK, currently "pass" for a
somewhat squiggly
> definition of "pass".
> I get occasional fails on a local x86_64 build, w/o my patches.
>
>  > If your patch introduces the instability, you'll have to fix them
before
>
> [ jasonk --> ] The instabilities were there to begin with, AFAIK. Some
of
> the tests are run on a "deflake" regimen where they are repeated
10x,
> looking for a specific output.
>
>  > 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.
>
> [ jasonk --> ]  Sorry, but I don't understand what you want here.
The
> tests I touched are marked unsupported (i.e. won't be run at all) ONLY
for
> aarch64-linux-androideabi. Is there something else you want? It should not
> impact any other configuration of TSAN/compiler-rt as-is. I also guard TSAN
> with a CMAKE variable so that it only gets built for aarch64 iff
> COMPILER_RT_FORCE_TSAN_AARCH64=1
>
>
>
>  > cheers,
>  > -renato
>
> Thanks!
> -Jason
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150828/4c45101f/attachment.html>