[llvm-dev] TSAN hack on AArch64 for Android

[Renato Golin via llvm-dev]

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

cheers,
--renato