<div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span style="font-size:12.8000001907349px">Also we could try to ask android to dedicate a real tls slot for</span><br style="font-size:12.8000001907349px"><span style="font-size:12.8000001907349px">sanitizers (I don't know what are the chances, but ART obviously get a</span><br style="font-size:12.8000001907349px"><span style="font-size:12.8000001907349px">one).</span></blockquote><div><br></div><div>Done: <a href="https://android-review.googlesource.com/#/c/167420/">https://android-review.googlesource.com/#/c/167420/</a></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 18, 2015 at 2:32 PM, Dmitry Vyukov <span dir="ltr"><<a href="mailto:dvyukov@google.com" target="_blank">dvyukov@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Aug 18, 2015 at 10:53 PM, Renato Golin <<a href="mailto:renato.golin@linaro.org">renato.golin@linaro.org</a>> wrote:<br>
> On 18 August 2015 at 19:57, Dmitry Vyukov <<a href="mailto:dvyukov@google.com">dvyukov@google.com</a>> wrote:<br>
>> Hi Renato,<br>
>><br>
>> What exactly hack/hacks do you mean?<br>
><br>
> Hi Dmitry,<br>
><br>
> The sanitizer code seems to be more accepting to adding complicated<br>
> pre-processor logic, as well as a number of alternative functions to<br>
> work around various inefficiencies in other systems, but in other LLVM<br>
> projects, such as Clang and the back-ends, we tend to be less<br>
> accepting of work-arounds being implemented just for the same of a<br>
> test.<br>
><br>
> This case adds an immense complexity (see the size of the patch), and<br>
> it has an immense number of odd assumptions (see the amount of<br>
> negative feedback on the patch), that may never have been tested in<br>
> other architectures (see some comments about generalising the wrong<br>
> stuff). All of that, to be able to "experiment" with TSAN on Android<br>
> in AArch64.<br>
><br>
> See, if TSAN was completely broken in AArch64, or if other people<br>
> weren't trying to push it upstream, I'd understand someone throwing a<br>
> bunch of hacked files that "works on their boxes", if that's disabled<br>
> by default and is only supposed to work when invoked. But none of that<br>
> is true.<br>
><br>
> TSAN works to an extent in AArch64, and we have been pushing some<br>
> patches to prove that. With ASAN live on the buildbots, and the<br>
> builtins working, we'll now focus on TSAN to get it being tested in<br>
> the buildbots, too. That's Linux AArch64, of course, as we don't test<br>
> Android at all.<br>
><br>
> Now, the oddities with Android is that bionic and the rest of the<br>
> run-time is crippled in ways that no other system I know gets close.<br>
> Layers upon layers of libraries, aliases, asm hackery, just to make it<br>
> build. Adding Clang to the list of compilers did not make it better.<br>
><br>
> This comment baffles me: "Ideally, this should be supported with the<br>
> __thread keyword, but that is not yet supported, and it is a much<br>
> bigger chunk of work to get it integrated into the default android<br>
> cross compiler."<br>
><br>
> AFAIK, __thread is supported by GCC and Clang, and this seems to be a<br>
> problem in Android's linker. So, to work around a problem in Android's<br>
> linker, we're adding a huge pile of code to LLVM for a temporary fix<br>
> because of a possible experimentation in Android?<br>
><br>
> The amount of work being done, and still required, to get this patch<br>
> upstream in any decent shape or form will take many engineer-months,<br>
> probably the same as it would to actually fix the linker in the first<br>
> place. Especially if you add up the work that will be required to<br>
> *remove* all that after the linker is fixed.<br>
><br>
> But I'm being over-optimistic here. I'm old enough to know that,<br>
> whenever someone introduces a "work-around that works", the<br>
> probability of anyone fixing the original problem is zero. And the<br>
> Android code that I had the displeasure to see, so far, has shown me<br>
> that this is ultimately true on their side. And that's why I'm<br>
> baffled, because we're moving that mentality, that work arounds are<br>
> ok, no matter how ugly they are, into LLVM, and everyone seems to be<br>
> ok with it.<br>
><br>
> If everyone is ok with it, I'll just shut up. But I'd like to make<br>
> sure that everyone is ok to do so.<br>
><br>
> My tuppence.<br>
><br>
> --renato<br>
<br>
<br>
</div></div>Hi Renato,<br>
<br>
<br>
So the problem is only with the tls emulation, right?<br>
<br>
I don't know what it worth to add __thread support to android, and<br>
whether somebody is going to address it soon.<br>
But I think we can make the tls emulation _much_ cleaner. First, we<br>
need to put all tsan per-thread data into ThreadState object, accesses<br>
to ThreadState are already wrapped into cur_thread() function. So now<br>
we have a single function to modify, no macros spread across the<br>
codebase, no new files, etc. Then, I've looked at pthread_getspecific<br>
code in bionic and it is actually quite fast and does not call any<br>
other system function (except get_thread() which is basically an<br>
access to real tls). So we can use it, and then the whole support<br>
becomes merely:<br>
<br>
INLINE ThreadState *cur_thread() {<br>
#ifdef REAL_TLS<br>
  return reinterpret_cast<ThreadState *>(&cur_thread_placeholder);<br>
#else<br>
  return reinterpret_cast<ThreadState *>(pthread_getspecific(thread_key));<br>
#endif<br>
}<br>
<br>
Which looks quite acceptable to me.<br>
<br>
Also we could try to ask android to dedicate a real tls slot for<br>
sanitizers (I don't know what are the chances, but ART obviously get a<br>
one). Then we can change the support to:<br>
<br>
INLINE ThreadState *cur_thread() {<br>
#ifdef REAL_TLS<br>
  return reinterpret_cast<ThreadState *>(&cur_thread_placeholder);<br>
#else<br>
  return reinterpret_cast<ThreadState *>(__get_tls()[ANDROID_TSAN_TLS_SLOT]);<br>
#endif<br>
}<br>
<br>
Which is roughly the same code-wise, but faster and safer.<br>
<br>
Am I missing something?<br>
</blockquote></div><br></div>