[tsan] Detect races on modifying accesses in Swift code

[kubamracek]

Summary

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.

[kubamracek]

See https://groups.google.com/forum/#!topic/thread-sanitizer/r7HmBkKQiIw for more information about this patch.

[kubamracek]

Updating the patch with a solution to address the problem described at https://groups.google.com/forum/#!topic/thread-sanitizer/r7HmBkKQiIw:

There is a problem is that we often detect the race on the "regular" access (called from LLVM IR instrumentation), and we can't tell that this is a Swift-type race, because we don't store the information that the previous access way a sizeless, external memory access. Currently, the __tsan_external_write API simply does MemoryWrite(addr, kSizeLog8), so we track those accesses as 8-byte long, which is not really true. We'd like to instead treat them as "sizeless".

This patch stores the external tag information in a fake stack frame in thr->shadow_stack, which is later retrieved during issue reporting. I'm open to better suggestions.

[dvyukov]

Hi Kuba,

I've seen a bunch of changes from you and they all are marked in my inbox. The past 1.5 weeks I was travelling and next week I am OOO so won't be able to review them. Just wanted to let you know.

[dvyukov]

In D31630#721583, @dvyukov wrote:

Hi Kuba,

I've seen a bunch of changes from you and they all are marked in my inbox. The past 1.5 weeks I was travelling and next week I am OOO so won't be able to review them. Just wanted to let you know.

I am back. Slowly working on backlog.

[dvyukov]

I don't like that we sprinkle swift-specific bits throughout the code when the external API was meant to be general enough to cover all such cases.
We have the special tag for swift, so it really seems to me that it should be the only swift-specific bit here and the rest must be handled by the external API on general basis. We have 3 potential users for the external API (Go, Java, races on fd, potentially more in future). So I would be really interested in making the external API general and flexible enough to support all of them, rather than later sprinkle Go/Java/fd/etc-specific bits throughout the code again later.

Why do we say "Mutating" for external and "Modifying" for swift? Please settle on 1 form.
Why is the tag for switch includes "modifying"? Modifying/non-modifying is orthogonal to the object type and is controlled by __tsan_external_read/write. It really should be kExternalTagSwiftAccess.
If we want to say "swift" in header line, why don't we want the same level of support for other external objects?

I propose we say for usual data races:

ThreadSanitizer: data race

Write of size X at P by S
Previous write of size X at P by S

and for external accesses:

ThreadSanitizer: race on TAG_NAME

Mutating access at P by S
Previous mutating access at P by S

(or Modifying, I don't care too much)

This will allow us to have:

ThreadSanitizer: race on file descriptor
ThreadSanitizer: race on curl handle
ThreadSanitizer: race on Swift array
ThreadSanitizer: race on Swift map
ThreadSanitizer: race on Go map

Also, could swift runtime register a tag at startup? Or performance overhead is a concern?

test/tsan/Darwin/external-swift.cc

10 ↗

(On Diff #94144)

Declare them in test.h. They will be used by other tests in future.

[dvyukov]

We have 3 potential users for the external API (Go, Java, races on fd, potentially more in future).

Also add races on vptr to the list.

lib/tsan/rtl/tsan_rtl.h

651 ↗

(On Diff #94144)

Why is it 128? It seems it should be 2.

[dvyukov]

In D31630#730957, @dvyukov wrote:

In D31630#721583, @dvyukov wrote:

Hi Kuba,

I've seen a bunch of changes from you and they all are marked in my inbox. The past 1.5 weeks I was travelling and next week I am OOO so won't be able to review them. Just wanted to let you know.

I am back. Slowly working on backlog.

I now think that I replied to all pending reviews. If there is anything left, please ping them.

lib/tsan/rtl/tsan_report.cc

215 ↗

(On Diff #94144)

It's better to drop "object" here to make it more general. A user can always add "object" to tag name if necessary.

lib/tsan/rtl/tsan_suppressions.cc

79 ↗

(On Diff #94144)

It seems to me that we now won't see ReportTypeExternalRace/ReportTypeSwiftAccessRace here.

[kubamracek]

First, let me split out the conversion of tag tracking in ThreadState into tracking in thread event logs: https://reviews.llvm.org/D32382.

[kubamracek]

lib/tsan/rtl/tsan_report.cc

215 ↗

(On Diff #94144)

https://reviews.llvm.org/D32383

lib/tsan/rtl/tsan_rtl.h

651 ↗

(On Diff #94144)

https://reviews.llvm.org/D32382

test/tsan/Darwin/external-swift.cc

10 ↗

(On Diff #94144)

https://reviews.llvm.org/D32384

[kubamracek]

Uploading with without the extracted standalone parts.

[kubamracek]

Addressing review comments.

[kubamracek]

ThreadSanitizer: race on Swift array
ThreadSanitizer: race on Swift map

Unfortunately, we won't be able to tell the type of the variable (at least not now). I've updated the patch to say "race on Swift variable", but that's not really exact and it's not describing the problem well. Swift is getting exact threading rules that are very different from C/C++ and I'm trying to focus on making the reports readable and understandable by Swift users.

Is there a way we could still special-case the Swift reports? I understand the desire to keep the API generic, but I think we need to make sure the users understand the report. E.g. "Swift access race" much better describes what the problem is, and users words that define the Swift threading model.

Also, could swift runtime register a tag at startup? Or performance overhead is a concern?

It's quite hard to make the runtime call the registration at process startup, because it would need to dynamically figure out whether we're running with TSan or not, plus we'd need to add an indirection to each access (to read the value of the tag). I'd really prefer to keep the value as a well-defined constant in TSan.

[dvyukov]

In D31630#734474, @kubamracek wrote:

Also, could swift runtime register a tag at startup? Or performance overhead is a concern?

It's quite hard to make the runtime call the registration at process startup, because it would need to dynamically figure out whether we're running with TSan or not, plus we'd need to add an indirection to each access (to read the value of the tag). I'd really prefer to keep the value as a well-defined constant in TSan.

Agree, I think, a constant is the right way to do it.
Any other uses that I think of will also be much simpler/faster with a constant.

[dvyukov]

In D31630#734474, @kubamracek wrote:

ThreadSanitizer: race on Swift array
ThreadSanitizer: race on Swift map

Unfortunately, we won't be able to tell the type of the variable (at least not now). I've updated the patch to say "race on Swift variable", but that's not really exact and it's not describing the problem well. Swift is getting exact threading rules that are very different from C/C++ and I'm trying to focus on making the reports readable and understandable by Swift users.

Is there a way we could still special-case the Swift reports? I understand the desire to keep the API generic, but I think we need to make sure the users understand the report. E.g. "Swift access race" much better describes what the problem is, and users words that define the Swift threading model.

Humm... what is so special about Swift? Isn't it that concurrent accesses to some objects are a bug? I would expect that it is, and then it's no different from C/C++ variables, C++ containers, Go variables and containers, Java containers, file descriptors and everything else.

I don't understand why saying race or data race on something is confusing or not understand-able. What's ambiguous/obscure/wrong about it?
On the other hand I have problems understanding meaning of "Swift access race". Does it mean "race during access to a Swift object"? If so it's semantically equivalent to "race on Swift object".

[kubamracek]

In D31630#736670, @dvyukov wrote:

In D31630#734474, @kubamracek wrote:

ThreadSanitizer: race on Swift array
ThreadSanitizer: race on Swift map

Unfortunately, we won't be able to tell the type of the variable (at least not now). I've updated the patch to say "race on Swift variable", but that's not really exact and it's not describing the problem well. Swift is getting exact threading rules that are very different from C/C++ and I'm trying to focus on making the reports readable and understandable by Swift users.

Is there a way we could still special-case the Swift reports? I understand the desire to keep the API generic, but I think we need to make sure the users understand the report. E.g. "Swift access race" much better describes what the problem is, and users words that define the Swift threading model.

Humm... what is so special about Swift? Isn't it that concurrent accesses to some objects are a bug? I would expect that it is, and then it's no different from C/C++ variables, C++ containers, Go variables and containers, Java containers, file descriptors and everything else.

I don't understand why saying race or data race on something is confusing or not understand-able. What's ambiguous/obscure/wrong about it?
On the other hand I have problems understanding meaning of "Swift access race". Does it mean "race during access to a Swift object"? If so it's semantically equivalent to "race on Swift object".

The memory/threading model of Swift defines races in terms of accesses: Starting a new "modifying" access requires that there are no outstanding accesses. And "access" here means the formal duration over which the variable is accessed, e.g. the whole function call for inout arguments. This is all pretty new and still being worked on: https://github.com/apple/swift/blob/master/docs/OwnershipManifesto.md.

[dvyukov]

In D31630#736944, @kubamracek wrote:

In D31630#736670, @dvyukov wrote:

In D31630#734474, @kubamracek wrote:

ThreadSanitizer: race on Swift array
ThreadSanitizer: race on Swift map

Unfortunately, we won't be able to tell the type of the variable (at least not now). I've updated the patch to say "race on Swift variable", but that's not really exact and it's not describing the problem well. Swift is getting exact threading rules that are very different from C/C++ and I'm trying to focus on making the reports readable and understandable by Swift users.

Is there a way we could still special-case the Swift reports? I understand the desire to keep the API generic, but I think we need to make sure the users understand the report. E.g. "Swift access race" much better describes what the problem is, and users words that define the Swift threading model.

Humm... what is so special about Swift? Isn't it that concurrent accesses to some objects are a bug? I would expect that it is, and then it's no different from C/C++ variables, C++ containers, Go variables and containers, Java containers, file descriptors and everything else.

I don't understand why saying race or data race on something is confusing or not understand-able. What's ambiguous/obscure/wrong about it?
On the other hand I have problems understanding meaning of "Swift access race". Does it mean "race during access to a Swift object"? If so it's semantically equivalent to "race on Swift object".

The memory/threading model of Swift defines races in terms of accesses: Starting a new "modifying" access requires that there are no outstanding accesses. And "access" here means the formal duration over which the variable is accessed, e.g. the whole function call for inout arguments. This is all pretty new and still being worked on: https://github.com/apple/swift/blob/master/docs/OwnershipManifesto.md.

Then let's register report header for external tags.

[kubamracek]

The memory/threading model of Swift defines races in terms of accesses: Starting a new "modifying" access requires that there are no outstanding accesses. And "access" here means the formal duration over which the variable is accessed, e.g. the whole function call for inout arguments. This is all pretty new and still being worked on: https://github.com/apple/swift/blob/master/docs/OwnershipManifesto.md.

Then let's register report header for external tags.

What do you mean by that? Can you explain?

[dvyukov]

In D31630#736979, @kubamracek wrote:

The memory/threading model of Swift defines races in terms of accesses: Starting a new "modifying" access requires that there are no outstanding accesses. And "access" here means the formal duration over which the variable is accessed, e.g. the whole function call for inout arguments. This is all pretty new and still being worked on: https://github.com/apple/swift/blob/master/docs/OwnershipManifesto.md.

Then let's register report header for external tags.

What do you mean by that? Can you explain?

Currently we have:

void *__tsan_external_register_tag(const char *object_type);

I am proposing:

void *__tsan_external_register_tag(const char *report_header, const char *object_type);

Then we can have customized report header for external tags.
For the dedicated swift tag, you will need to pre-populate the global tag descriptor with necessary report header and object name. Then we don't need any swift-specific changes in ReportRace/PrintReport.
To make this work we will also need to associate tag with the whole report (rather than with memory access descriptors), but it looks like the right thing to do regardless.
I am not sure if we will still need object_type then.

[kubamracek]

Updating the patch, addressing review comments. I added a separate API to register report header strings to avoid breaking existing users of the API.

[dvyukov]

Thanks!
I think this now provides a solid foundation for future extensions.

compiler-rt/trunk/include/sanitizer/tsan_interface.h

 //   - __tsan_external_read/__tsan_external_write annotates the logical reads
 //       and writes of the object at the specified address. 'caller_pc' should
 //       be the PC of the library user, which the library can obtain with e.g.
 //       `__builtin_return_address(0)`.
 //   - __tsan_external_register_tag registers a 'tag' with the specified name,
 //       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);
 
 #ifdef __cplusplus
 }  // extern "C"
 #endif
 
 #endif  // SANITIZER_TSAN_INTERFACE_H

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

struct MBlock {
   u64  siz : 48;
   u64  tag : 16;
   u32  stk;
   u16  tid;
 };
 
 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/trunk/lib/tsan/rtl/tsan_external.cc

 //===----------------------------------------------------------------------===//
 #include "tsan_rtl.h"
 #include "tsan_interceptors.h"
 
 namespace __tsan {
 
 #define CALLERPC ((uptr)__builtin_return_address(0))
 
-const char *registered_tags[kExternalTagMax];
+struct TagData {
+  const char *object_type;
+  const char *header;
+};
+
+static TagData registered_tags[kExternalTagMax] = {
+  {},
+  {"Swift variable", "Swift access race"},
+};
 static atomic_uint32_t used_tags{kExternalTagFirstUserAvailable};  // NOLINT.
-
-const char *GetObjectTypeFromTag(uptr tag) {
-  if (tag == 0) return nullptr;
+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) {
   FuncEntry(thr, (uptr)&registered_tags[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
 
 typedef void(*AccessFunc)(ThreadState *, uptr, uptr, int);
 void ExternalAccess(void *addr, void *caller_pc, void *tag, AccessFunc access) {
   CHECK_LT(tag, atomic_load(&used_tags, memory_order_relaxed));
   ThreadState *thr = cur_thread();
   if (caller_pc) FuncEntry(thr, (uptr)caller_pc);
   InsertShadowStackFrameForTag(thr, (uptr)tag);
   bool in_ignored_lib;
   if (!caller_pc || !libignore()->IsIgnored((uptr)caller_pc, &in_ignored_lib)) {
     access(thr, CALLERPC, (uptr)addr, kSizeLog1);
   }
   FuncExit(thr);
   if (caller_pc) FuncExit(thr);
 }
 
 extern "C" {
 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));
   Allocator *a = allocator();
   MBlock *b = nullptr;
   if (a->PointerIsMine((void *)addr)) {
     void *block_begin = a->GetBlockBegin((void *)addr);
     if (block_begin) b = ctx->metamap.GetBlock((uptr)block_begin);
   }

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

 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_func_exit();
 
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_ignore_thread_begin();
 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);
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_external_write(void *addr, void *caller_pc, void *tag);
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_read_range(void *addr, unsigned long size);  // NOLINT

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

struct ReportMutex {
   uptr addr;
   bool destroyed;
   ReportStack *stack;
 };
 
 class ReportDesc {
  public:
   ReportType typ;
+  uptr tag;
   Vector<ReportStack*> stacks;
   Vector<ReportMop*> mops;
   Vector<ReportLocation*> locs;
   Vector<ReportMutex*> mutexes;
   Vector<ReportThread*> threads;
   Vector<int> unique_tids;
   ReportStack *sleep;
   int count;

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

public:
   const char *EndLocation() { return Default(); }
   const char *Sleep()   { return Yellow(); }
   const char *EndSleep() { return Default(); }
   const char *Mutex()   { return Magenta(); }
   const char *EndMutex() { return Default(); }
 };
 
 ReportDesc::ReportDesc()
-    : stacks(MBlockReportStack)
+    : tag(kExternalTagNone)
+    , stacks(MBlockReportStack)
     , mops(MBlockReportMop)
     , locs(MBlockReportLoc)
     , mutexes(MBlockReportMutex)
     , threads(MBlockReportThread)
     , unique_tids(MBlockReportThread)
     , sleep()
     , count() {
 }

 const int kThreadBufSize = 32;
 const char *thread_name(char *buf, int tid) {
   if (tid == 0)
     return "main thread";
   internal_snprintf(buf, kThreadBufSize, "thread T%d", 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)
     return "data race on vptr (ctor/dtor vs virtual call)";
   if (typ == ReportTypeUseAfterFree)
     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)
     return "destroy of a locked mutex";
   if (typ == ReportTypeMutexDoubleLock)
     return "double lock of a mutex";
   if (typ == ReportTypeMutexInvalidAccess)
     return "use of an invalid mutex (e.g. uninitialized or destroyed)";

 static const char *MopDesc(bool first, bool write, bool atomic) {
   return atomic ? (first ? (write ? "Atomic write" : "Atomic read")
                 : (write ? "Previous atomic write" : "Previous atomic read"))
                 : (first ? (write ? "Write" : "Read")
                 : (write ? "Previous write" : "Previous read"));
 }
 
 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));
   }
   PrintMutexSet(mop->mset);
   Printf(":\n");
   Printf("%s", d.EndAccess());
   PrintStack(mop->stack);
static SymbolizedStack *SkipTsanInternalFrames(SymbolizedStack *frames) {
   while (FrameIsInternal(frames) && frames->next)
     frames = frames->next;
   return 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());
   Printf("%s", d.EndWarning());
 
   if (rep->typ == ReportTypeDeadlock) {
     char thrbuf[kThreadBufSize];
     Printf("  Cycle in lock order graph: ");

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

#endif
 
   ~ScopedIgnoreInterceptors() {
 #if !SANITIZER_GO
     cur_thread()->ignore_interceptors--;
 #endif
   }
 };
 
-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,
                        const MutexSet *mset);
   void AddStack(StackTrace stack, bool suppressable = false);
   void AddThread(const ThreadContext *tctx, bool suppressable = false);
   void AddThread(int unique_tid, bool suppressable = false);
   void AddUniqueTid(int unique_tid);

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

static ReportStack *SymbolizeStack(StackTrace trace) {
   }
   StackStripMain(top);
 
   ReportStack *stack = ReportStack::New();
   stack->frames = top;
   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();
 }
 
 ScopedReport::~ScopedReport() {
   CommonSanitizerReportMutex.Unlock();
   ctx->report_mtx.Unlock();
   DestroyAndFree(rep_);
void ReportRace(ThreadState *thr) {
   Shadow s2(thr->racy_state[1]);
   RestoreStack(s2.tid(), s2.epoch(), &traces[1], mset2, &tags[1]);
   if (IsFiredSuppression(ctx, typ, traces[1]))
     return;
 
   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)
+  // 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],
                         i == 0 ? &thr->mset : mset2);
   }
 
   for (uptr i = 0; i < kMop; i++) {
     FastState s(thr->racy_state[i]);

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

for (int i = 0; i < 4; i++) {
     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: race on a library object
+    // CHECK: WARNING: ThreadSanitizer: race on HelloWorld
     t1.join();
     t2.join();
   }
   
   fprintf(stderr, "First phase done.\n");
   // CHECK: First phase done.
 
   for (int i = 0; i < 4; i++) {
     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: race on a library object
+    // CHECK: WARNING: ThreadSanitizer: race on HelloWorld
     t1.join();
     t2.join();
   }
 
   fprintf(stderr, "Second phase done.\n");
   // CHECK: Second phase done.
 }
 
 // CHECK: ThreadSanitizer: reported 2 warnings

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

+// 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/trunk/test/tsan/Darwin/external.cc

int main(int argc, char *argv[]) {
   
   // TEST1: WARNING: ThreadSanitizer: data race
   // TEST1: {{Write|Read}} of size 8 at
   // TEST1: Previous {{write|read}} of size 8 at
   // TEST1: Location is heap block of size 16 at
   
   // 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
 
   {
     MyObjectRef ref = ObjectCreate();
     std::thread t1([ref]{ ObjectWrite(ref, 76); });
     std::thread t2([ref]{ ObjectWriteAnother(ref, 77); });
     t1.join();
     t2.join();
   }
   
   // TEST1-NOT: WARNING: ThreadSanitizer: data race
   
   // 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
 }