From a7cad4fcb7434e42d1387477c42a7546b9f27259 Mon Sep 17 00:00:00 2001 From: Kuba Mracek Date: Wed, 3 May 2017 16:51:01 +0000 Subject: [PATCH] [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 | 1 + compiler-rt/lib/tsan/rtl/tsan_defs.h | 9 +++ compiler-rt/lib/tsan/rtl/tsan_external.cc | 47 ++++++++++--- compiler-rt/lib/tsan/rtl/tsan_interface.h | 2 + compiler-rt/lib/tsan/rtl/tsan_report.cc | 21 +++--- compiler-rt/lib/tsan/rtl/tsan_report.h | 1 + compiler-rt/lib/tsan/rtl/tsan_rtl.h | 10 +-- compiler-rt/lib/tsan/rtl/tsan_rtl_report.cc | 17 +++-- compiler-rt/test/tsan/Darwin/external-dups.cc | 4 +- compiler-rt/test/tsan/Darwin/external-swift.cc | 92 ++++++++++++++++++++++++++ compiler-rt/test/tsan/Darwin/external.cc | 14 ++-- 11 files changed, 180 insertions(+), 38 deletions(-) create mode 100644 compiler-rt/test/tsan/Darwin/external-swift.cc diff --git a/compiler-rt/include/sanitizer/tsan_interface.h b/compiler-rt/include/sanitizer/tsan_interface.h index a0c7026..5ea09ab 100644 --- a/compiler-rt/include/sanitizer/tsan_interface.h +++ b/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); diff --git a/compiler-rt/lib/tsan/rtl/tsan_defs.h b/compiler-rt/lib/tsan/rtl/tsan_defs.h index 8a0381e..8977fea 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_defs.h +++ b/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 diff --git a/compiler-rt/lib/tsan/rtl/tsan_external.cc b/compiler-rt/lib/tsan/rtl/tsan_external.cc index 2d32b6d..6c0e947 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_external.cc +++ b/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 ®istered_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 < ®istered_tags[0] || pc_ptr >= ®istered_tags[tag_count]) + if (pc_ptr < GetTagData(0) || pc_ptr > GetTagData(tag_count - 1)) return 0; - return (const char **)pc_ptr - ®istered_tags[0]; + return (TagData *)pc_ptr - GetTagData(0); } #if !SANITIZER_GO @@ -60,11 +76,26 @@ 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(); diff --git a/compiler-rt/lib/tsan/rtl/tsan_interface.h b/compiler-rt/lib/tsan/rtl/tsan_interface.h index 7198628..a80a489 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_interface.h +++ b/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); diff --git a/compiler-rt/lib/tsan/rtl/tsan_report.cc b/compiler-rt/lib/tsan/rtl/tsan_report.cc index b188af2..2de7ffc 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_report.cc +++ b/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()); diff --git a/compiler-rt/lib/tsan/rtl/tsan_report.h b/compiler-rt/lib/tsan/rtl/tsan_report.h index a0473e8..bc1582f 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_report.h +++ b/compiler-rt/lib/tsan/rtl/tsan_report.h @@ -108,6 +108,7 @@ struct ReportMutex { class ReportDesc { public: ReportType typ; + uptr tag; Vector stacks; Vector mops; Vector locs; diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.h b/compiler-rt/lib/tsan/rtl/tsan_rtl.h index 8bf1c19..e92a0f35 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl.h +++ b/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, diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cc b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cc index 055029b..68b9f50 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cc +++ b/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], diff --git a/compiler-rt/test/tsan/Darwin/external-dups.cc b/compiler-rt/test/tsan/Darwin/external-dups.cc index 79432ba..ca1eb3e7 100644 --- a/compiler-rt/test/tsan/Darwin/external-dups.cc +++ b/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(); } diff --git a/compiler-rt/test/tsan/Darwin/external-swift.cc b/compiler-rt/test/tsan/Darwin/external-swift.cc new file mode 100644 index 0000000..f6f9e7f --- /dev/null +++ b/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 + +#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. +} + diff --git a/compiler-rt/test/tsan/Darwin/external.cc b/compiler-rt/test/tsan/Darwin/external.cc index 211694a..e72281a 100644 --- a/compiler-rt/test/tsan/Darwin/external.cc +++ b/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 -- 2.7.4