[tsan] Detect races on modifying accesses in Swift code

This patch allows the Swift compiler to emit calls to `__tsan_external_write` before starting any modifying access, which will cause TSan to detect races on arrays, dictionaries and other classes defined in non-instrumented modules. Races on collections from the Swift standard library and user-defined structs and a frequent cause of subtle bugs and it's important that TSan detects those on top of existing LLVM IR instrumentation, which already detects races in direct memory accesses.

Differential Revision: https://reviews.llvm.org/D31630

llvm-svn: 302050

compiler-rt/include/sanitizer/tsan_interface.h

@@ -126,6 +126,7 @@ void __tsan_mutex_post_divert(void *addr, unsigned flags);
 //       which is later used in read/write annotations to denote the object type
 //   - __tsan_external_assign_tag can optionally mark a heap object with a tag
 void *__tsan_external_register_tag(const char *object_type);
+void __tsan_external_register_header(void *tag, const char *header);
 void __tsan_external_assign_tag(void *addr, void *tag);
 void __tsan_external_read(void *addr, void *caller_pc, void *tag);
 void __tsan_external_write(void *addr, void *caller_pc, void *tag);

compiler-rt/lib/tsan/rtl/tsan_defs.h

@@ -157,6 +157,15 @@ struct MBlock {
 
 COMPILER_CHECK(sizeof(MBlock) == 16);
 
+enum ExternalTag : uptr {
+  kExternalTagNone = 0,
+  kExternalTagSwiftModifyingAccess = 1,
+  kExternalTagFirstUserAvailable = 2,
+  kExternalTagMax = 1024,
+  // Don't set kExternalTagMax over 65,536, since MBlock only stores tags
+  // as 16-bit values, see tsan_defs.h.
+};
+
 }  // namespace __tsan
 
 #endif  // TSAN_DEFS_H

compiler-rt/lib/tsan/rtl/tsan_external.cc

@@ -17,14 +17,30 @@ namespace __tsan {
 
 #define CALLERPC ((uptr)__builtin_return_address(0))
 
-const char *registered_tags[kExternalTagMax];
-static atomic_uint32_t used_tags{kExternalTagFirstUserAvailable};  // NOLINT.
+struct TagData {
+  const char *object_type;
+  const char *header;
+};
 
-const char *GetObjectTypeFromTag(uptr tag) {
-  if (tag == 0) return nullptr;
+static TagData registered_tags[kExternalTagMax] = {
+  {},
+  {"Swift variable", "Swift access race"},
+};
+static atomic_uint32_t used_tags{kExternalTagFirstUserAvailable};  // NOLINT.
+static TagData *GetTagData(uptr tag) {
   // Invalid/corrupted tag?  Better return NULL and let the caller deal with it.
   if (tag >= atomic_load(&used_tags, memory_order_relaxed)) return nullptr;
-  return registered_tags[tag];
+  return &registered_tags[tag];
+}
+
+const char *GetObjectTypeFromTag(uptr tag) {
+  TagData *tag_data = GetTagData(tag);
+  return tag_data ? tag_data->object_type : nullptr;
+}
+
+const char *GetReportHeaderFromTag(uptr tag) {
+  TagData *tag_data = GetTagData(tag);
+  return tag_data ? tag_data->header : nullptr;
 }
 
 void InsertShadowStackFrameForTag(ThreadState *thr, uptr tag) {
@@ -34,9 +50,9 @@ void InsertShadowStackFrameForTag(ThreadState *thr, uptr tag) {
 uptr TagFromShadowStackFrame(uptr pc) {
   uptr tag_count = atomic_load(&used_tags, memory_order_relaxed);
   void *pc_ptr = (void *)pc;
-  if (pc_ptr < &registered_tags[0] || pc_ptr >= &registered_tags[tag_count])
+  if (pc_ptr < GetTagData(0) || pc_ptr > GetTagData(tag_count - 1))
     return 0;
-  return (const char **)pc_ptr - &registered_tags[0];
+  return (TagData *)pc_ptr - GetTagData(0);
 }
 
 #if !SANITIZER_GO
@@ -60,10 +76,25 @@ SANITIZER_INTERFACE_ATTRIBUTE
 void *__tsan_external_register_tag(const char *object_type) {
   uptr new_tag = atomic_fetch_add(&used_tags, 1, memory_order_relaxed);
   CHECK_LT(new_tag, kExternalTagMax);
-  registered_tags[new_tag] = internal_strdup(object_type);
+  GetTagData(new_tag)->object_type = internal_strdup(object_type);
+  char header[127] = {0};
+  internal_snprintf(header, sizeof(header), "race on %s", object_type);
+  GetTagData(new_tag)->header = internal_strdup(header);
   return (void *)new_tag;
 }
 
+SANITIZER_INTERFACE_ATTRIBUTE
+void __tsan_external_register_header(void *tag, const char *header) {
+  CHECK_GE((uptr)tag, kExternalTagFirstUserAvailable);
+  CHECK_LT((uptr)tag, kExternalTagMax);
+  atomic_uintptr_t *header_ptr =
+      (atomic_uintptr_t *)&GetTagData((uptr)tag)->header;
+  header = internal_strdup(header);
+  char *old_header =
+      (char *)atomic_exchange(header_ptr, (uptr)header, memory_order_seq_cst);
+  if (old_header) internal_free(old_header);
+}
+
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_external_assign_tag(void *addr, void *tag) {
   CHECK_LT(tag, atomic_load(&used_tags, memory_order_relaxed));

compiler-rt/lib/tsan/rtl/tsan_interface.h

@@ -82,6 +82,8 @@ SANITIZER_INTERFACE_ATTRIBUTE void __tsan_ignore_thread_end();
 SANITIZER_INTERFACE_ATTRIBUTE
 void *__tsan_external_register_tag(const char *object_type);
 SANITIZER_INTERFACE_ATTRIBUTE
+void __tsan_external_register_header(void *tag, const char *header);
+SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_external_assign_tag(void *addr, void *tag);
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_external_read(void *addr, void *caller_pc, void *tag);

compiler-rt/lib/tsan/rtl/tsan_report.cc

@@ -53,7 +53,8 @@ class Decorator: public __sanitizer::SanitizerCommonDecorator {
 };
 
 ReportDesc::ReportDesc()
-    : stacks(MBlockReportStack)
+    : tag(kExternalTagNone)
+    , stacks(MBlockReportStack)
     , mops(MBlockReportMop)
     , locs(MBlockReportLoc)
     , mutexes(MBlockReportMutex)
@@ -81,7 +82,7 @@ const char *thread_name(char *buf, int tid) {
   return buf;
 }
 
-static const char *ReportTypeString(ReportType typ) {
+static const char *ReportTypeString(ReportType typ, uptr tag) {
   if (typ == ReportTypeRace)
     return "data race";
   if (typ == ReportTypeVptrRace)
@@ -90,8 +91,9 @@ static const char *ReportTypeString(ReportType typ) {
     return "heap-use-after-free";
   if (typ == ReportTypeVptrUseAfterFree)
     return "heap-use-after-free (virtual call vs free)";
-  if (typ == ReportTypeExternalRace)
-    return "race on a library object";
+  if (typ == ReportTypeExternalRace) {
+    return GetReportHeaderFromTag(tag) ?: "race on external object";
+  }
   if (typ == ReportTypeThreadLeak)
     return "thread leak";
   if (typ == ReportTypeMutexDestroyLocked)
@@ -155,20 +157,21 @@ static const char *MopDesc(bool first, bool write, bool atomic) {
 }
 
 static const char *ExternalMopDesc(bool first, bool write) {
-  return first ? (write ? "Mutating" : "Read-only")
-               : (write ? "Previous mutating" : "Previous read-only");
+  return first ? (write ? "Modifying" : "Read-only")
+               : (write ? "Previous modifying" : "Previous read-only");
 }
 
 static void PrintMop(const ReportMop *mop, bool first) {
   Decorator d;
   char thrbuf[kThreadBufSize];
   Printf("%s", d.Access());
-  const char *object_type = GetObjectTypeFromTag(mop->external_tag);
-  if (mop->external_tag == kExternalTagNone || !object_type) {
+  if (mop->external_tag == kExternalTagNone) {
     Printf("  %s of size %d at %p by %s",
            MopDesc(first, mop->write, mop->atomic), mop->size,
            (void *)mop->addr, thread_name(thrbuf, mop->tid));
   } else {
+    const char *object_type =
+        GetObjectTypeFromTag(mop->external_tag) ?: "external object";
     Printf("  %s access of %s at %p by %s",
            ExternalMopDesc(first, mop->write), object_type,
            (void *)mop->addr, thread_name(thrbuf, mop->tid));
@@ -315,7 +318,7 @@ static SymbolizedStack *SkipTsanInternalFrames(SymbolizedStack *frames) {
 void PrintReport(const ReportDesc *rep) {
   Decorator d;
   Printf("==================\n");
-  const char *rep_typ_str = ReportTypeString(rep->typ);
+  const char *rep_typ_str = ReportTypeString(rep->typ, rep->tag);
   Printf("%s", d.Warning());
   Printf("WARNING: ThreadSanitizer: %s (pid=%d)\n", rep_typ_str,
          (int)internal_getpid());

compiler-rt/lib/tsan/rtl/tsan_report.h

@@ -108,6 +108,7 @@ struct ReportMutex {
 class ReportDesc {
  public:
   ReportType typ;
+  uptr tag;
   Vector<ReportStack*> stacks;
   Vector<ReportMop*> mops;
   Vector<ReportLocation*> locs;

compiler-rt/lib/tsan/rtl/tsan_rtl.h

@@ -564,19 +564,13 @@ struct ScopedIgnoreInterceptors {
   }
 };
 
-enum ExternalTag : uptr {
-  kExternalTagNone = 0,
-  kExternalTagFirstUserAvailable = 1,
-  kExternalTagMax = 1024,
-  // Don't set kExternalTagMax over 65,536, since MBlock only stores tags
-  // as 16-bit values, see tsan_defs.h.
-};
 const char *GetObjectTypeFromTag(uptr tag);
+const char *GetReportHeaderFromTag(uptr tag);
 uptr TagFromShadowStackFrame(uptr pc);
 
 class ScopedReport {
  public:
-  explicit ScopedReport(ReportType typ);
+  explicit ScopedReport(ReportType typ, uptr tag = kExternalTagNone);
   ~ScopedReport();
 
   void AddMemoryAccess(uptr addr, uptr external_tag, Shadow s, StackTrace stack,

compiler-rt/lib/tsan/rtl/tsan_rtl_report.cc

@@ -143,11 +143,12 @@ static ReportStack *SymbolizeStack(StackTrace trace) {
   return stack;
 }
 
-ScopedReport::ScopedReport(ReportType typ) {
+ScopedReport::ScopedReport(ReportType typ, uptr tag) {
   ctx->thread_registry->CheckLocked();
   void *mem = internal_alloc(MBlockReport, sizeof(ReportDesc));
   rep_ = new(mem) ReportDesc;
   rep_->typ = typ;
+  rep_->tag = tag;
   ctx->report_mtx.Lock();
   CommonSanitizerReportMutex.Lock();
 }
@@ -651,12 +652,18 @@ void ReportRace(ThreadState *thr) {
   if (HandleRacyStacks(thr, traces, addr_min, addr_max))
     return;
 
-  // If any of the two accesses has a tag, treat this as an "external" race.
-  if (tags[0] != kExternalTagNone || tags[1] != kExternalTagNone)
-    typ = ReportTypeExternalRace;
+  // If any of the accesses has a tag, treat this as an "external" race.
+  uptr tag = kExternalTagNone;
+  for (uptr i = 0; i < kMop; i++) {
+    if (tags[i] != kExternalTagNone) {
+      typ = ReportTypeExternalRace;
+      tag = tags[i];
+      break;
+    }
+  }
 
   ThreadRegistryLock l0(ctx->thread_registry);
-  ScopedReport rep(typ);
+  ScopedReport rep(typ, tag);
   for (uptr i = 0; i < kMop; i++) {
     Shadow s(thr->racy_state[i]);
     rep.AddMemoryAccess(addr, tags[i], s, traces[i],

compiler-rt/test/tsan/Darwin/external-dups.cc

@@ -28,7 +28,7 @@ int main(int argc, char *argv[]) {
       barrier_wait(&barrier);
       ExternalWrite(opaque_object);
     });
-    // CHECK: WARNING: ThreadSanitizer: race on a library object
+    // CHECK: WARNING: ThreadSanitizer: race on HelloWorld
     t1.join();
     t2.join();
   }
@@ -46,7 +46,7 @@ int main(int argc, char *argv[]) {
       barrier_wait(&barrier);
       ExternalWrite(opaque_object);
     });
-    // CHECK: WARNING: ThreadSanitizer: race on a library object
+    // CHECK: WARNING: ThreadSanitizer: race on HelloWorld
     t1.join();
     t2.join();
   }

compiler-rt/test/tsan/Darwin/external-swift.cc

@@ -0,0 +1,92 @@
+// RUN: %clangxx_tsan %s -o %t
+// RUN: %deflake %run %t 2>&1 | FileCheck %s
+
+#include <thread>
+
+#import "../test.h"
+
+extern "C" {
+void __tsan_write8(void *addr);
+}
+
+static void *tag = (void *)0x1;
+
+__attribute__((no_sanitize("thread")))
+void ExternalWrite(void *addr) {
+  __tsan_external_write(addr, nullptr, tag);
+}
+
+__attribute__((no_sanitize("thread")))
+void RegularWrite(void *addr) {
+  __tsan_write8(addr);
+}
+
+int main(int argc, char *argv[]) {
+  barrier_init(&barrier, 2);
+  fprintf(stderr, "Start.\n");
+  // CHECK: Start.
+  
+  {
+    void *opaque_object = malloc(16);
+    std::thread t1([opaque_object] {
+      ExternalWrite(opaque_object);
+      barrier_wait(&barrier);
+    });
+    std::thread t2([opaque_object] {
+      barrier_wait(&barrier);
+      ExternalWrite(opaque_object);
+    });
+    // CHECK: WARNING: ThreadSanitizer: Swift access race
+    // CHECK: Modifying access of Swift variable at {{.*}} by thread {{.*}}
+    // CHECK: Previous modifying access of Swift variable at {{.*}} by thread {{.*}}
+    // CHECK: SUMMARY: ThreadSanitizer: Swift access race
+    t1.join();
+    t2.join();
+  }
+
+  fprintf(stderr, "external+external test done.\n");
+  // CHECK: external+external test done.
+
+  {
+    void *opaque_object = malloc(16);
+    std::thread t1([opaque_object] {
+      ExternalWrite(opaque_object);
+      barrier_wait(&barrier);
+    });
+    std::thread t2([opaque_object] {
+      barrier_wait(&barrier);
+      RegularWrite(opaque_object);
+    });
+    // CHECK: WARNING: ThreadSanitizer: Swift access race
+    // CHECK: Write of size 8 at {{.*}} by thread {{.*}}
+    // CHECK: Previous modifying access of Swift variable at {{.*}} by thread {{.*}}
+    // CHECK: SUMMARY: ThreadSanitizer: Swift access race
+    t1.join();
+    t2.join();
+  }
+  
+  fprintf(stderr, "external+regular test done.\n");
+  // CHECK: external+regular test done.
+  
+  {
+    void *opaque_object = malloc(16);
+    std::thread t1([opaque_object] {
+      RegularWrite(opaque_object);
+      barrier_wait(&barrier);
+    });
+    std::thread t2([opaque_object] {
+      barrier_wait(&barrier);
+      ExternalWrite(opaque_object);
+    });
+    // CHECK: WARNING: ThreadSanitizer: Swift access race
+    // CHECK: Modifying access of Swift variable at {{.*}} by thread {{.*}}
+    // CHECK: Previous write of size 8 at {{.*}} by thread {{.*}}
+    // CHECK: SUMMARY: ThreadSanitizer: Swift access race
+    t1.join();
+    t2.join();
+  }
+  
+  fprintf(stderr, "regular+external test done.\n");
+  // CHECK: regular+external test done.
+}
+

compiler-rt/test/tsan/Darwin/external.cc

@@ -67,13 +67,14 @@ int main(int argc, char *argv[]) {
   
   // TEST2-NOT: WARNING: ThreadSanitizer
   
-  // TEST3: WARNING: ThreadSanitizer: race on a library object
-  // TEST3: {{Mutating|read-only}} access of MyLibrary::MyObject at
+  // TEST3: WARNING: ThreadSanitizer: race on MyLibrary::MyObject
+  // TEST3: {{Modifying|read-only}} access of MyLibrary::MyObject at
   // TEST3: {{ObjectWrite|ObjectRead}}
-  // TEST3: Previous {{mutating|read-only}} access of MyLibrary::MyObject at
+  // TEST3: Previous {{modifying|read-only}} access of MyLibrary::MyObject at
   // TEST3: {{ObjectWrite|ObjectRead}}
   // TEST3: Location is MyLibrary::MyObject of size 16 at
   // TEST3: {{ObjectCreate}}
+  // TEST3: SUMMARY: ThreadSanitizer: race on MyLibrary::MyObject {{.*}} in {{ObjectWrite|ObjectRead}}
 
   fprintf(stderr, "RW test done\n");
   // CHECK: RW test done
@@ -90,13 +91,14 @@ int main(int argc, char *argv[]) {
   
   // TEST2-NOT: WARNING: ThreadSanitizer
   
-  // TEST3: WARNING: ThreadSanitizer: race on a library object
-  // TEST3: Mutating access of MyLibrary::MyObject at
+  // TEST3: WARNING: ThreadSanitizer: race on MyLibrary::MyObject
+  // TEST3: Modifying access of MyLibrary::MyObject at
   // TEST3: {{ObjectWrite|ObjectWriteAnother}}
-  // TEST3: Previous mutating access of MyLibrary::MyObject at
+  // TEST3: Previous modifying access of MyLibrary::MyObject at
   // TEST3: {{ObjectWrite|ObjectWriteAnother}}
   // TEST3: Location is MyLibrary::MyObject of size 16 at
   // TEST3: {{ObjectCreate}}
+  // TEST3: SUMMARY: ThreadSanitizer: race on MyLibrary::MyObject {{.*}} in {{ObjectWrite|ObjectWriteAnother}}
 
   fprintf(stderr, "WW test done\n");
   // CHECK: WW test done