tsan: don't pass bogus PCs to __tsan_symbolize_external

[dvyukov]

Summary

See the added comment for an explanation.

[dvyukov]

I am open to suggestions. This is total mess.

[vitalybuka]

Definitely looks weird, but I have no better idea.
So just couple of nits

lib/tsan/rtl/tsan_rtl_report.cc

638

Could this be just > INT64_MAX

test/tsan/double_race.cc

17

misaligned

19

same

[alekseyshl]

Have no better ideas either.

[dvyukov]

lib/tsan/rtl/tsan_rtl_report.cc

638

This is 61, not 63, so INT64_MAX won't do.
I've added a constant for this. It was duplicated in multiple places.

[dvyukov]

Submitted in 311768.

lib/tsan/rtl/tsan_rtl.h

 
 extern "C" void __tsan_trace_switch();
 void ALWAYS_INLINE TraceAddEvent(ThreadState *thr, FastState fs,
                                         EventType typ, u64 addr) {
   if (!kCollectHistory)
     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)) {
 #if !SANITIZER_GO
     HACKY_CALL(__tsan_trace_switch);
 #else
     TraceSwitch(thr);
 #endif
   }
   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;
 }
 
 #if !SANITIZER_GO
 uptr ALWAYS_INLINE HeapEnd() {
   return HeapMemEnd() + PrimaryAllocator::AdditionalSize();
 }
 #endif
 
 }  // namespace __tsan
 
 #endif  // TSAN_RTL_H

lib/tsan/rtl/tsan_rtl_report.cc

for (uptr i = 0; i < hdr->stack0.size; i++) {
     DPrintf2("  #%02zu: pc=%zx\n", i, stack[i]);
   }
   if (mset)
     *mset = hdr->mset0;
   uptr pos = hdr->stack0.size;
   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;
     } else if (typ == EventTypeFuncEnter) {
       if (stack.Size() < pos + 2)
         stack.Resize(pos + 2);
       stack[pos++] = pc;
     } else if (typ == EventTypeFuncExit) {
else if (freed)
     typ = ReportTypeUseAfterFree;
 
   if (IsFiredSuppression(ctx, typ, addr))
     return;
 
   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;
 
   // MutexSet is too large to live on stack.
   Vector<u64> mset_buffer(MBlockScopedBuf);
   mset_buffer.Resize(sizeof(MutexSet) / sizeof(u64) + 1);
   MutexSet *mset2 = new(&mset_buffer[0]) MutexSet();

lib/tsan/rtl/tsan_trace.h

enum EventType {
   EventTypeRUnlock
 };
 
 // Represents a thread event (from most significant bit):
 // u64 typ  : 3;   // 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.
 #else
   VarSizeStackTrace stack0;
 #endif
   u64        epoch0;  // Start epoch for the trace.
   MutexSet   mset0;

test/tsan/double_race.cc

+// 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