[compiler-rt] r311768 - tsan: don't pass bogus PCs to __tsan_symbolize_external

Fri Aug 25 08:20:58 PDT 2017

On Fri, Aug 25, 2017 at 4:53 PM, Adrian Prantl <aprantl at apple.com> wrote:
> It looks like this commit broke the green dragon bot. Could you please revert and investigate?
>
> http://lab.llvm.org:8080/green/job/clang-stage1-configure-RA_check/34882/consoleFull#7286766538254eaf0-7326-4999-85b0-388101f2d404

Moved the test to Linux dir:
http://llvm.org/viewvc/llvm-project?rev=311776&view=rev
Thanks


>> On Aug 25, 2017, at 1:52 AM, Dmitry Vyukov via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>> Author: dvyukov
>> Date: Fri Aug 25 01:52:28 2017
>> New Revision: 311768
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=311768&view=rev
>> Log:
>> tsan: don't pass bogus PCs to __tsan_symbolize_external
>>
>> See the added comment for an explanation.
>>
>> Reviewed in https://reviews.llvm.org/D37107
>>
>>
>> Added:
>>    compiler-rt/trunk/test/tsan/double_race.cc
>> Modified:
>>    compiler-rt/trunk/lib/tsan/rtl/tsan_rtl.h
>>    compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc
>>    compiler-rt/trunk/lib/tsan/rtl/tsan_trace.h
>>
>> Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_rtl.h
>> URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_rtl.h?rev=311768&r1=311767&r2=311768&view=diff
>> ==============================================================================
>> --- compiler-rt/trunk/lib/tsan/rtl/tsan_rtl.h (original)
>> +++ compiler-rt/trunk/lib/tsan/rtl/tsan_rtl.h Fri Aug 25 01:52:28 2017
>> @@ -825,7 +825,7 @@ void ALWAYS_INLINE TraceAddEvent(ThreadS
>>     return;
>>   DCHECK_GE((int)typ, 0);
>>   DCHECK_LE((int)typ, 7);
>> -  DCHECK_EQ(GetLsb(addr, 61), addr);
>> +  DCHECK_EQ(GetLsb(addr, kEventPCBits), addr);
>>   StatInc(thr, StatEvents);
>>   u64 pos = fs.GetTracePos();
>>   if (UNLIKELY((pos % kTracePartSize) == 0)) {
>> @@ -837,7 +837,7 @@ void ALWAYS_INLINE TraceAddEvent(ThreadS
>>   }
>>   Event *trace = (Event*)GetThreadTrace(fs.tid());
>>   Event *evp = &trace[pos];
>> -  Event ev = (u64)addr | ((u64)typ << 61);
>> +  Event ev = (u64)addr | ((u64)typ << kEventPCBits);
>>   *evp = ev;
>> }
>>
>>
>> Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc
>> URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc?rev=311768&r1=311767&r2=311768&view=diff
>> ==============================================================================
>> --- compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc (original)
>> +++ compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc Fri Aug 25 01:52:28 2017
>> @@ -406,8 +406,8 @@ void RestoreStack(int tid, const u64 epo
>>   Event *events = (Event*)GetThreadTrace(tid);
>>   for (uptr i = ebegin; i <= eend; i++) {
>>     Event ev = events[i];
>> -    EventType typ = (EventType)(ev >> 61);
>> -    uptr pc = (uptr)(ev & ((1ull << 61) - 1));
>> +    EventType typ = (EventType)(ev >> kEventPCBits);
>> +    uptr pc = (uptr)(ev & ((1ull << kEventPCBits) - 1));
>>     DPrintf2("  %zu typ=%d pc=%zx\n", i, typ, pc);
>>     if (typ == EventTypeMop) {
>>       stack[pos] = pc;
>> @@ -634,7 +634,27 @@ void ReportRace(ThreadState *thr) {
>>   const uptr kMop = 2;
>>   VarSizeStackTrace traces[kMop];
>>   uptr tags[kMop] = {kExternalTagNone};
>> -  const uptr toppc = TraceTopPC(thr);
>> +  uptr toppc = TraceTopPC(thr);
>> +  if (toppc >> kEventPCBits) {
>> +    // This is a work-around for a known issue.
>> +    // The scenario where this happens is rather elaborate and requires
>> +    // an instrumented __sanitizer_report_error_summary callback and
>> +    // a __tsan_symbolize_external callback and a race during a range memory
>> +    // access larger than 8 bytes. MemoryAccessRange adds the current PC to
>> +    // the trace and starts processing memory accesses. A first memory access
>> +    // triggers a race, we report it and call the instrumented
>> +    // __sanitizer_report_error_summary, which adds more stuff to the trace
>> +    // since it is intrumented. Then a second memory access in MemoryAccessRange
>> +    // also triggers a race and we get here and call TraceTopPC to get the
>> +    // current PC, however now it contains some unrelated events from the
>> +    // callback. Most likely, TraceTopPC will now return a EventTypeFuncExit
>> +    // event. Later we subtract -1 from it (in GetPreviousInstructionPc)
>> +    // and the resulting PC has kExternalPCBit set, so we pass it to
>> +    // __tsan_symbolize_external. __tsan_symbolize_external is within its rights
>> +    // to crash since the PC is completely bogus.
>> +    // test/tsan/double_race.cc contains a test case for this.
>> +    toppc = 0;
>> +  }
>>   ObtainCurrentStack(thr, toppc, &traces[0], &tags[0]);
>>   if (IsFiredSuppression(ctx, typ, traces[0]))
>>     return;
>>
>> Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_trace.h
>> URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_trace.h?rev=311768&r1=311767&r2=311768&view=diff
>> ==============================================================================
>> --- compiler-rt/trunk/lib/tsan/rtl/tsan_trace.h (original)
>> +++ compiler-rt/trunk/lib/tsan/rtl/tsan_trace.h Fri Aug 25 01:52:28 2017
>> @@ -41,6 +41,8 @@ enum EventType {
>> // u64 addr : 61;  // Associated pc.
>> typedef u64 Event;
>>
>> +const uptr kEventPCBits = 61;
>> +
>> struct TraceHeader {
>> #if !SANITIZER_GO
>>   BufferedStackTrace stack0;  // Start stack for the trace.
>>
>> Added: compiler-rt/trunk/test/tsan/double_race.cc
>> URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/tsan/double_race.cc?rev=311768&view=auto
>> ==============================================================================
>> --- compiler-rt/trunk/test/tsan/double_race.cc (added)
>> +++ compiler-rt/trunk/test/tsan/double_race.cc Fri Aug 25 01:52:28 2017
>> @@ -0,0 +1,52 @@
>> +// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t 2>&1 | FileCheck %s
>> +#include "test.h"
>> +#include <memory.h>
>> +
>> +// A reproducer for a known issue.
>> +// See reference to double_race.cc in tsan_rtl_report.cc for an explanation.
>> +
>> +char buf[16];
>> +volatile int nreport;
>> +
>> +void __sanitizer_report_error_summary(const char *summary) {
>> +  nreport++;
>> +}
>> +
>> +const int kEventPCBits = 61;
>> +
>> +extern "C" bool __tsan_symbolize_external(unsigned long pc, char *func_buf,
>> +                                          unsigned long func_siz,
>> +                                          char *file_buf,
>> +                                          unsigned long file_siz, int *line,
>> +                                          int *col) {
>> +  if (pc >> kEventPCBits) {
>> +    printf("bad PC passed to __tsan_symbolize_external: %lx\n", pc);
>> +    _exit(1);
>> +  }
>> +  return true;
>> +}
>> +
>> +void *Thread(void *arg) {
>> +  barrier_wait(&barrier);
>> +  memset(buf, 2, sizeof(buf));
>> +  return 0;
>> +}
>> +
>> +int main() {
>> +  barrier_init(&barrier, 2);
>> +  pthread_t t;
>> +  pthread_create(&t, 0, Thread, 0);
>> +  memset(buf, 1, sizeof(buf));
>> +  barrier_wait(&barrier);
>> +  pthread_join(t, 0);
>> +  return 0;
>> +}
>> +
>> +// CHECK: WARNING: ThreadSanitizer: data race
>> +// CHECK:   Write of size 8 at {{.*}} by thread T1:
>> +// CHECK:     #0 memset
>> +// CHECK:     #1 Thread
>> +// CHECK-NOT: bad PC passed to __tsan_symbolize_external
>> +// CHECK: WARNING: ThreadSanitizer: data race
>> +// CHECK:   Write of size 8 at {{.*}} by thread T1:
>> +// CHECK:     #0 Thread
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>