[llvm-dev] TSAN hack on AArch64 for Android

[Renato Golin via llvm-dev]

On 18 August 2015 at 22:32, Dmitry Vyukov <dvyukov at google.com> wrote:
> So the problem is only with the tls emulation, right?

No. It's with the whole patch, including its contents, how it was
submitted, what is its purpose and how nobody cared about any of this.

Adding TLS emulation is, in its own, a worthy feature. It allows for
platforms that can't do full TLS to use TSAN, which is a great tool.

But when you want to add a feature to a tool that runs on multiple
architectures, you plan the feature, you design it, you send an RFC
email, you discuss in the list.

Then you go about the implementation, submit some preparation patches,
clean up code, and have some more discussions. For a good example, see
Daniel Sanders' "The trouble with triples" and the associated patches.
It takes a lot of time to conjure a new feature in a complex piece of
software, but more than that, needs consensus and people designing it
beforehand.

But when someone dumps a patch that is nowhere near production
quality, or would even compile in most platforms, people should be
backing up and saying: "Wait a minute. What are you trying to do, and
how did you get here?". We had a similar issue a few months back with
the new Stride Vectorizer. Fantastic feature, one that I had worked
before, but it took us a whole month of "code review" to figure out
that the whole patch was just wrong, and had to be completely
re-written, from where we had stopped two years back.

I'm doing the same now. Let's make sure to agree on what's being
proposed, THEN we'll agree if that's worthy or not, THEN we'll agree
what's the best implementation, THEN we'll review code. We can't start
from the last iteration, as that'll invariably lead to poor code going
in.

Furthermore, you don't get TLS+TSAN+AArch64+Android support in a
single patch! The likelihood that we'll end up reverting it a hundred
times is really high, and that will completely unbalance our buildbot
infrastructure and break everyone else's workflow. Slow is faster than
ludicrous. One step at a time, please.

First, let's get TSAN working on AArch64. Adhemerval is working on
that. Then, let's see what's missing for Android. Ok, TLS emulation is
required, at least for the time being. What other architecture would
benefit from TLS emulation? None? Ok, then we set out to change the
code, so that TLS emulation can go in. Meanwhile, a group of people
discuss (in public) the design of such framework, and in time, a
number of patches will start to go in, one by one, until the feature
is on. Only then we'll get Android using it.

The time will also give buildbots enough iterations to pick any bug
that we don't find on code review. Bugs that would be obscured by a
giant patch with many other more obvious bugs exploding our CI
infrastructure and getting reverted by angry bot maintainers.



> I don't know what it worth to add __thread support to android, and
> whether somebody is going to address it soon.

Wouldn't it be better to know that before submitting a large change to
another code base? If there is evidence that this is never going to
happen, or that, for some very strong engineering reasons, it's not
possible to do it, then the reasons to do TLS emulation are thoroughly
justified. Right now, it's just a hunch.


> But I think we can make the tls emulation _much_ cleaner. First, we
> need to put all tsan per-thread data into ThreadState object, accesses
> to ThreadState are already wrapped into cur_thread() function. So now
> we have a single function to modify, no macros spread across the
> codebase, no new files, etc.

I'm no expert in that field, but this sounds like a good plan. Doesn't
seem to be part of this patch, though. Nor it should, as it actually
should have been a preparatory patch, submitted after all interested
parties agreed that TLS emulation is the way to go.


> INLINE ThreadState *cur_thread() {
> #ifdef REAL_TLS
>   return reinterpret_cast<ThreadState *>(&cur_thread_placeholder);
> #else
>   return reinterpret_cast<ThreadState *>(__get_tls()[ANDROID_TSAN_TLS_SLOT]);
> #endif
> }
>
> Which is roughly the same code-wise, but faster and safer.

Dan "Eastwood" Albert has pulled the trigger. :)

This looks reasonable to me. Does this still mean you need the
emulation, or is Android going to give you a "reasonably looking
ThreadState"?

cheers,
--renato