<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Dec 22, 2014 at 1:11 AM, Dmitry Vyukov <span dir="ltr"><<a href="mailto:dvyukov@google.com" target="_blank">dvyukov@google.com</a>></span> wrote:<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 class="">On Mon, Dec 22, 2014 at 5:28 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Thu, Dec 18, 2014 at 2:19 AM, Dmitry Vyukov <<a href="mailto:dvyukov@google.com">dvyukov@google.com</a>> wrote:<br>
>><br>
>> Author: dvyukov<br>
>> Date: Thu Dec 18 04:19:32 2014<br>
>> New Revision: 224508<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=224508&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=224508&view=rev</a><br>
>> Log:<br>
>> tsan: don't crash with NULL deref during reporting<br>
>><br>
>> tctx==NULL crash observed during deadlock reporting.<br>
>> There seems to be some bugs in the deadlock detector,<br>
>> but it is still useful to be more robust during reporting.<br>
><br>
><br>
> So you don't know why tctx is null in this case - isn't this a bit of a<br>
> hack/workaround, rather than an actual fix, then? It's sort of unfortunate<br>
> to build up cruft to tolerate violations of invariants we believe should<br>
> hold...<br>
<br>
</span>The comment is not very precise. It is not a bug in deadlock detector,<br>
whenever we report something there are chances that involved threads<br>
and mutexes are already destroyed and are forget. In other cases we<br>
check for NULL in callers. So this check is exactly what we want.<br>
<span class=""><br>
<br>
>> Modified:<br>
>> compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc<br>
>><br>
>> Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc?rev=224508&r1=224507&r2=224508&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc?rev=224508&r1=224507&r2=224508&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc (original)<br>
>> +++ compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc Thu Dec 18 04:19:32<br>
>> 2014<br>
>> @@ -251,7 +251,9 @@ ThreadContext *IsThreadStackOrTls(uptr a<br>
>><br>
>> void ScopedReport::AddThread(int unique_tid, bool suppressable) {<br>
>> #ifndef SANITIZER_GO<br>
>> - AddThread(FindThreadByUidLocked(unique_tid), suppressable);<br>
>> + const ThreadContext *tctx = FindThreadByUidLocked(unique_tid);<br>
>> + if (tctx)<br>
><br>
><br>
> Usually we roll the variable declaration into the condition in cases like<br>
> this.<br>
<br>
</span>See no single instance of declaring variables in if's:<br></blockquote><div><br>Maybe some issues with your grep-fu. (mine's never very good - I have to sanity check every time)<br><br>Here's what I could find:<br></div><div><br><div>$ grep -r "\(if\|while\|for\|switch\) ([^()]\+ [^()]\+ = " lib | wc -l</div><div>753<br><div>$ grep -r "if ([^()]\+ [^()]\+ = " lib | wc -l</div><div>23</div><div>blaikie@blaikie-linux:~/dev/llvm/src/projects/compiler-rt$ grep -r "if ([^()]\+ [^()]\+ = " lib | head</div><div>lib/asan/asan_flags.cc: if (const char *env = GetEnv("ASAN_OPTIONS")) {</div><div>lib/asan/asan_stats.cc: if (AsanThread *t = tctx->thread)</div><div>lib/asan/asan_fake_stack.cc: if (FakeStack *fs = GetTLSFakeStack())<br><div>lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc: if (char *res = SendCommandImpl(is_data, module_name, module_offset))</div><div>lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc: if (SymbolizedStack *res = libbacktrace_symbolizer_->SymbolizeCode(</div><div>lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc: if (const char *demangled = libbacktrace_symbolizer_->Demangle(name))</div><div>lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc: if (const char *addr2line_path = FindPathToBinary("addr2line")) {</div><div>lib/sanitizer_common/sanitizer_common_interceptors_format.inc: if (void *argp = va_arg(aq, void *)) {</div><div>lib/sanitizer_common/sanitizer_common_interceptors_format.inc: if (void *argp = va_arg(aq, void *)) {</div><div>lib/sanitizer_common/sanitizer_bvgraph.h: if (uptr res = findPath(idx, targets, path + 1, path_size - 1))<br>...</div></div></div> </div><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">
<br>
$ egrep "if \((uptr|int|unsigned|u8|s8)\ .*\)" rtl/*.cc<br>
../sanitizer_common/*.cc ../asan/*.cc ../msan/*.cc<br>
$ egrep "if \(.*\*\)" rtl/*.cc ../sanitizer_common/*.cc ../asan/*.cc<br>
../msan/*.cc<br>
rtl/tsan_interceptors.cc: if ((res = ((const unsigned char *)s1)[len] -<br>
rtl/tsan_interceptors.cc: if<br>
(pthread_setspecific(g_thread_finalize_key, (void*)(iter - 1))) {<br>
rtl/tsan_rtl_report.cc: if (a->PointerIsMine((void*)addr)) {<br>
rtl/tsan_sync.cc: if<br>
(atomic_compare_exchange_strong((atomic_uint32_t*)meta, &idx0,<br>
../sanitizer_common/sanitizer_libc.cc: if ((char*)aligned_end >= beg)<br>
../sanitizer_common/sanitizer_linux.cc: if ((char *)entry_ >=<br>
&buffer_[bytes_read_] && !GetDirectoryEntries())<br>
../sanitizer_common/sanitizer_procmaps_mac.cc: if (((const<br>
load_command *)lc)->cmd == kLCSegment) {<br>
../asan/asan_allocator.cc: if (*(u8<br>
*)MEM_TO_SHADOW((uptr)allocated) == 0 && CanPoisonMemory()) {<br>
../asan/asan_allocator.cc: if<br>
(!atomic_compare_exchange_strong((atomic_uint8_t*)m, &old_chunk_state,<br>
../asan/asan_report.cc: if (*(char*)(g.beg + g.size - 1) != '\0') return;<br>
../asan/asan_rtl.cc: if (res != (void*)beg) {<br>
../msan/msan_interceptors.cc: if (res != (void*)-1)<br>
../msan/msan_interceptors.cc: if (res != (void*)-1)<br>
../msan/msan_interceptors.cc: if (p != (void *)-1) {<br>
../msan/msan_interceptors.cc: if (!((MSanInterceptorContext<br>
*)ctx)->in_interceptor_scope) \<br>
../msan/msan_interceptors.cc: if (map) ForEachMappedRegion((link_map<br>
*)map, __msan_unpoison);<br>
../msan/msan_interceptors.cc: if (*(u8 *)src_s) *(u32<br>
*)SHADOW_TO_ORIGIN(dst_s &~3UL) = src_origin;<br>
../msan/msan_linux.cc: if (shadow != (void*)kShadowBeg) return false;<br>
../msan/msan_linux.cc: if (origins != (void*)kOriginsBeg) return false;<br>
</blockquote></div></div></div>