[llvm-dev] TSAN hack on AArch64 for Android

[Dmitry Vyukov via llvm-dev]

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.