From 0f923a9c9e689f8cbd0f6ed348a44cc05d5e9b2d Mon Sep 17 00:00:00 2001 From: Kenny Yu Date: Tue, 17 Oct 2023 11:23:11 -0700 Subject: [PATCH 1/2] [TSAN] Add __tsan_check_no_mutexes_held helper This adds a new helper that can be called from application code to ensure that no mutexes are held on specific code paths. This is useful for multiple scenarios, including ensuring no locks are held: - at thread exit - in peformance-critical code - when a coroutine is suspended (can cause deadlocks) See this discourse thread for more discussion: https://discourse.llvm.org/t/add-threadsanitizer-check-to-prevent-coroutine-suspending-while-holding-a-lock-potential-deadlock/74051 --- .../include/sanitizer/tsan_interface.h | 4 ++++ compiler-rt/lib/tsan/rtl/tsan.syms.extra | 1 + compiler-rt/lib/tsan/rtl/tsan_debugging.cpp | 4 +++- .../lib/tsan/rtl/tsan_interface_ann.cpp | 22 +++++++++++++++++ compiler-rt/lib/tsan/rtl/tsan_report.cpp | 17 +++++++++---- compiler-rt/lib/tsan/rtl/tsan_report.h | 3 ++- .../lib/tsan/rtl/tsan_suppressions.cpp | 1 + .../test/tsan/mutex_cannot_be_locked.cpp | 24 +++++++++++++++++++ .../tsan/mutex_cannot_be_locked_success.cpp | 19 +++++++++++++++ 9 files changed, 88 insertions(+), 7 deletions(-) create mode 100644 compiler-rt/test/tsan/mutex_cannot_be_locked.cpp create mode 100644 compiler-rt/test/tsan/mutex_cannot_be_locked_success.cpp diff --git a/compiler-rt/include/sanitizer/tsan_interface.h b/compiler-rt/include/sanitizer/tsan_interface.h index f19c79d79ba62..cd118441cb5ce 100644 --- a/compiler-rt/include/sanitizer/tsan_interface.h +++ b/compiler-rt/include/sanitizer/tsan_interface.h @@ -126,6 +126,10 @@ void __tsan_mutex_post_signal(void *addr, unsigned flags); void __tsan_mutex_pre_divert(void *addr, unsigned flags); void __tsan_mutex_post_divert(void *addr, unsigned flags); +// Annotate that no mutexes can be held. If we are holding mutexes, then +// TSan will print a bug report. +void __tsan_check_no_mutexes_held(); + // External race detection API. // Can be used by non-instrumented libraries to detect when their objects are // being used in an unsafe manner. diff --git a/compiler-rt/lib/tsan/rtl/tsan.syms.extra b/compiler-rt/lib/tsan/rtl/tsan.syms.extra index a5bd17176b12b..6416e5d47fc41 100644 --- a/compiler-rt/lib/tsan/rtl/tsan.syms.extra +++ b/compiler-rt/lib/tsan/rtl/tsan.syms.extra @@ -22,6 +22,7 @@ __tsan_mutex_pre_signal __tsan_mutex_post_signal __tsan_mutex_pre_divert __tsan_mutex_post_divert +__tsan_check_no_mutexes_held __tsan_get_current_fiber __tsan_create_fiber __tsan_destroy_fiber diff --git a/compiler-rt/lib/tsan/rtl/tsan_debugging.cpp b/compiler-rt/lib/tsan/rtl/tsan_debugging.cpp index 1e61c31c5a970..026d5b4329935 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_debugging.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_debugging.cpp @@ -35,7 +35,9 @@ static const char *ReportTypeDescription(ReportType typ) { case ReportTypeSignalUnsafe: return "signal-unsafe-call"; case ReportTypeErrnoInSignal: return "errno-in-signal-handler"; case ReportTypeDeadlock: return "lock-order-inversion"; - // No default case so compiler warns us if we miss one + case ReportTypeMutexCannotBeLocked: + return "mutex-cannot-be-locked"; + // No default case so compiler warns us if we miss one } UNREACHABLE("missing case"); } diff --git a/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp b/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp index 6bd72e18d9425..712ce2d1a45a8 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp @@ -435,4 +435,26 @@ void __tsan_mutex_post_divert(void *addr, unsigned flagz) { ThreadIgnoreBegin(thr, 0); ThreadIgnoreSyncBegin(thr, 0); } + +static void ReportMutexCannotBeLocked(ThreadState *thr, uptr pc) { + ThreadRegistryLock l(&ctx->thread_registry); + ScopedReport rep(ReportTypeMutexCannotBeLocked); + for (uptr i = 0; i < thr->mset.Size(); ++i) { + MutexSet::Desc desc = thr->mset.Get(i); + rep.AddMutex(desc.addr, desc.stack_id); + } + VarSizeStackTrace trace; + ObtainCurrentStack(thr, pc, &trace); + rep.AddStack(trace, true); + OutputReport(thr, rep); +} + +INTERFACE_ATTRIBUTE +void __tsan_check_no_mutexes_held() { + SCOPED_ANNOTATION(__tsan_check_no_mutexes_held); + if (thr->mset.Size() == 0) { + return; + } + ReportMutexCannotBeLocked(thr, pc); +} } // extern "C" diff --git a/compiler-rt/lib/tsan/rtl/tsan_report.cpp b/compiler-rt/lib/tsan/rtl/tsan_report.cpp index 4028d18107840..673449a9e70af 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_report.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_report.cpp @@ -93,7 +93,9 @@ static const char *ReportTypeString(ReportType typ, uptr tag) { return "signal handler spoils errno"; case ReportTypeDeadlock: return "lock-order-inversion (potential deadlock)"; - // No default case so compiler warns us if we miss one + case ReportTypeMutexCannotBeLocked: + return "mutex cannot be locked on this code path"; + // No default case so compiler warns us if we miss one } UNREACHABLE("missing case"); } @@ -216,11 +218,16 @@ static void PrintMutexShortWithAddress(const ReportMutex *rm, reinterpret_cast(rm->addr), d.Default(), after); } -static void PrintMutex(const ReportMutex *rm) { +static void PrintMutex(const ReportMutex *rm, ReportType typ) { Decorator d; Printf("%s", d.Mutex()); - Printf(" Mutex M%u (%p) created at:\n", rm->id, - reinterpret_cast(rm->addr)); + if (typ != ReportTypeMutexCannotBeLocked) { + Printf(" Mutex M%u (%p) created at:\n", rm->id, + reinterpret_cast(rm->addr)); + } else { + Printf(" Mutex M%u (%p) acquired at:\n", rm->id, + reinterpret_cast(rm->addr)); + } Printf("%s", d.Default()); PrintStack(rm->stack); } @@ -354,7 +361,7 @@ void PrintReport(const ReportDesc *rep) { if (rep->typ != ReportTypeDeadlock) { for (uptr i = 0; i < rep->mutexes.Size(); i++) - PrintMutex(rep->mutexes[i]); + PrintMutex(rep->mutexes[i], rep->typ); } for (uptr i = 0; i < rep->threads.Size(); i++) diff --git a/compiler-rt/lib/tsan/rtl/tsan_report.h b/compiler-rt/lib/tsan/rtl/tsan_report.h index 3c88864af1477..0e1a27655bfa4 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_report.h +++ b/compiler-rt/lib/tsan/rtl/tsan_report.h @@ -34,7 +34,8 @@ enum ReportType { ReportTypeMutexBadReadUnlock, ReportTypeSignalUnsafe, ReportTypeErrnoInSignal, - ReportTypeDeadlock + ReportTypeDeadlock, + ReportTypeMutexCannotBeLocked }; struct ReportStack { diff --git a/compiler-rt/lib/tsan/rtl/tsan_suppressions.cpp b/compiler-rt/lib/tsan/rtl/tsan_suppressions.cpp index 9cdfa32a93430..3b249a716ed2c 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_suppressions.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_suppressions.cpp @@ -81,6 +81,7 @@ static const char *conv(ReportType typ) { case ReportTypeMutexBadUnlock: case ReportTypeMutexBadReadLock: case ReportTypeMutexBadReadUnlock: + case ReportTypeMutexCannotBeLocked: return kSuppressionMutex; case ReportTypeSignalUnsafe: case ReportTypeErrnoInSignal: diff --git a/compiler-rt/test/tsan/mutex_cannot_be_locked.cpp b/compiler-rt/test/tsan/mutex_cannot_be_locked.cpp new file mode 100644 index 0000000000000..77ce2d7053374 --- /dev/null +++ b/compiler-rt/test/tsan/mutex_cannot_be_locked.cpp @@ -0,0 +1,24 @@ +// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s +#include "test.h" + +pthread_mutex_t mtx; + +void *ThreadFunc(void *) { + pthread_mutex_lock(&mtx); + __tsan_check_no_mutexes_held(); +} + +int main() { + pthread_t th; + pthread_create(&th, 0, ThreadFunc, NULL); + pthread_join(th, 0); + return 0; +} + +// CHECK: WARNING: ThreadSanitizer: mutex cannot be locked on this code path +// CHECK: #0 __tsan_check_no_mutexes_held +// CHECK: #1 ThreadFunc +// CHECK: Mutex {{.*}} acquired at: +// CHECK: #0 pthread_mutex_lock +// CHECK: #1 ThreadFunc +// CHECK: SUMMARY: ThreadSanitizer: mutex cannot be locked on this code path {{.*}}mutex_cannot_be_locked.cpp{{.*}}ThreadFunc diff --git a/compiler-rt/test/tsan/mutex_cannot_be_locked_success.cpp b/compiler-rt/test/tsan/mutex_cannot_be_locked_success.cpp new file mode 100644 index 0000000000000..7834dcc894778 --- /dev/null +++ b/compiler-rt/test/tsan/mutex_cannot_be_locked_success.cpp @@ -0,0 +1,19 @@ +// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s +#include "test.h" + +pthread_mutex_t mtx; + +void *ThreadFunc(void *) { + pthread_mutex_lock(&mtx); + pthread_mutex_unlock(&mtx); + __tsan_check_no_mutexes_held(); +} + +int main() { + pthread_t th; + pthread_create(&th, 0, ThreadFunc, NULL); + pthread_join(th, 0); + return 0; +} + +// CHECK-NOT: WARNING: ThreadSanitizer: mutex cannot be locked on this code path From 08bee0461bdd694e8b9e01545f28b66a7a509bcd Mon Sep 17 00:00:00 2001 From: Kenny Yu Date: Thu, 2 Nov 2023 14:31:29 -0700 Subject: [PATCH 2/2] fixup! [TSAN] Add __tsan_check_no_mutexes_held helper - "locked" -> "held". Rename functions, enum value, and test cases to match - init mutex and don't use threads in tests - undo mutex acquired/created change - fix comment on interface function --- .../include/sanitizer/tsan_interface.h | 4 +-- compiler-rt/lib/tsan/rtl/tsan_debugging.cpp | 4 +-- .../lib/tsan/rtl/tsan_interface_ann.cpp | 6 ++-- compiler-rt/lib/tsan/rtl/tsan_report.cpp | 17 ++++------ compiler-rt/lib/tsan/rtl/tsan_report.h | 2 +- .../lib/tsan/rtl/tsan_suppressions.cpp | 2 +- .../test/tsan/mutex_cannot_be_locked.cpp | 24 ------------- .../tsan/mutex_cannot_be_locked_success.cpp | 19 ----------- .../test/tsan/mutex_held_wrong_context.cpp | 34 +++++++++++++++++++ 9 files changed, 49 insertions(+), 63 deletions(-) delete mode 100644 compiler-rt/test/tsan/mutex_cannot_be_locked.cpp delete mode 100644 compiler-rt/test/tsan/mutex_cannot_be_locked_success.cpp create mode 100644 compiler-rt/test/tsan/mutex_held_wrong_context.cpp diff --git a/compiler-rt/include/sanitizer/tsan_interface.h b/compiler-rt/include/sanitizer/tsan_interface.h index cd118441cb5ce..9ea7e8f004b0c 100644 --- a/compiler-rt/include/sanitizer/tsan_interface.h +++ b/compiler-rt/include/sanitizer/tsan_interface.h @@ -126,8 +126,8 @@ void __tsan_mutex_post_signal(void *addr, unsigned flags); void __tsan_mutex_pre_divert(void *addr, unsigned flags); void __tsan_mutex_post_divert(void *addr, unsigned flags); -// Annotate that no mutexes can be held. If we are holding mutexes, then -// TSan will print a bug report. +// Check that the current thread does not hold any mutexes, +// report a bug report otherwise. void __tsan_check_no_mutexes_held(); // External race detection API. diff --git a/compiler-rt/lib/tsan/rtl/tsan_debugging.cpp b/compiler-rt/lib/tsan/rtl/tsan_debugging.cpp index 026d5b4329935..41fa293dbaaad 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_debugging.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_debugging.cpp @@ -35,8 +35,8 @@ static const char *ReportTypeDescription(ReportType typ) { case ReportTypeSignalUnsafe: return "signal-unsafe-call"; case ReportTypeErrnoInSignal: return "errno-in-signal-handler"; case ReportTypeDeadlock: return "lock-order-inversion"; - case ReportTypeMutexCannotBeLocked: - return "mutex-cannot-be-locked"; + case ReportTypeMutexHeldWrongContext: + return "mutex-held-in-wrong-context"; // No default case so compiler warns us if we miss one } UNREACHABLE("missing case"); diff --git a/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp b/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp index 712ce2d1a45a8..5154662034c56 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp @@ -436,9 +436,9 @@ void __tsan_mutex_post_divert(void *addr, unsigned flagz) { ThreadIgnoreSyncBegin(thr, 0); } -static void ReportMutexCannotBeLocked(ThreadState *thr, uptr pc) { +static void ReportMutexHeldWrongContext(ThreadState *thr, uptr pc) { ThreadRegistryLock l(&ctx->thread_registry); - ScopedReport rep(ReportTypeMutexCannotBeLocked); + ScopedReport rep(ReportTypeMutexHeldWrongContext); for (uptr i = 0; i < thr->mset.Size(); ++i) { MutexSet::Desc desc = thr->mset.Get(i); rep.AddMutex(desc.addr, desc.stack_id); @@ -455,6 +455,6 @@ void __tsan_check_no_mutexes_held() { if (thr->mset.Size() == 0) { return; } - ReportMutexCannotBeLocked(thr, pc); + ReportMutexHeldWrongContext(thr, pc); } } // extern "C" diff --git a/compiler-rt/lib/tsan/rtl/tsan_report.cpp b/compiler-rt/lib/tsan/rtl/tsan_report.cpp index 673449a9e70af..35cb6710a54fa 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_report.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_report.cpp @@ -93,8 +93,8 @@ static const char *ReportTypeString(ReportType typ, uptr tag) { return "signal handler spoils errno"; case ReportTypeDeadlock: return "lock-order-inversion (potential deadlock)"; - case ReportTypeMutexCannotBeLocked: - return "mutex cannot be locked on this code path"; + case ReportTypeMutexHeldWrongContext: + return "mutex held in the wrong context"; // No default case so compiler warns us if we miss one } UNREACHABLE("missing case"); @@ -218,16 +218,11 @@ static void PrintMutexShortWithAddress(const ReportMutex *rm, reinterpret_cast(rm->addr), d.Default(), after); } -static void PrintMutex(const ReportMutex *rm, ReportType typ) { +static void PrintMutex(const ReportMutex *rm) { Decorator d; Printf("%s", d.Mutex()); - if (typ != ReportTypeMutexCannotBeLocked) { - Printf(" Mutex M%u (%p) created at:\n", rm->id, - reinterpret_cast(rm->addr)); - } else { - Printf(" Mutex M%u (%p) acquired at:\n", rm->id, - reinterpret_cast(rm->addr)); - } + Printf(" Mutex M%u (%p) created at:\n", rm->id, + reinterpret_cast(rm->addr)); Printf("%s", d.Default()); PrintStack(rm->stack); } @@ -361,7 +356,7 @@ void PrintReport(const ReportDesc *rep) { if (rep->typ != ReportTypeDeadlock) { for (uptr i = 0; i < rep->mutexes.Size(); i++) - PrintMutex(rep->mutexes[i], rep->typ); + PrintMutex(rep->mutexes[i]); } for (uptr i = 0; i < rep->threads.Size(); i++) diff --git a/compiler-rt/lib/tsan/rtl/tsan_report.h b/compiler-rt/lib/tsan/rtl/tsan_report.h index 0e1a27655bfa4..bfe470797f8f7 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_report.h +++ b/compiler-rt/lib/tsan/rtl/tsan_report.h @@ -35,7 +35,7 @@ enum ReportType { ReportTypeSignalUnsafe, ReportTypeErrnoInSignal, ReportTypeDeadlock, - ReportTypeMutexCannotBeLocked + ReportTypeMutexHeldWrongContext }; struct ReportStack { diff --git a/compiler-rt/lib/tsan/rtl/tsan_suppressions.cpp b/compiler-rt/lib/tsan/rtl/tsan_suppressions.cpp index 3b249a716ed2c..70642124990d7 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_suppressions.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_suppressions.cpp @@ -81,7 +81,7 @@ static const char *conv(ReportType typ) { case ReportTypeMutexBadUnlock: case ReportTypeMutexBadReadLock: case ReportTypeMutexBadReadUnlock: - case ReportTypeMutexCannotBeLocked: + case ReportTypeMutexHeldWrongContext: return kSuppressionMutex; case ReportTypeSignalUnsafe: case ReportTypeErrnoInSignal: diff --git a/compiler-rt/test/tsan/mutex_cannot_be_locked.cpp b/compiler-rt/test/tsan/mutex_cannot_be_locked.cpp deleted file mode 100644 index 77ce2d7053374..0000000000000 --- a/compiler-rt/test/tsan/mutex_cannot_be_locked.cpp +++ /dev/null @@ -1,24 +0,0 @@ -// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s -#include "test.h" - -pthread_mutex_t mtx; - -void *ThreadFunc(void *) { - pthread_mutex_lock(&mtx); - __tsan_check_no_mutexes_held(); -} - -int main() { - pthread_t th; - pthread_create(&th, 0, ThreadFunc, NULL); - pthread_join(th, 0); - return 0; -} - -// CHECK: WARNING: ThreadSanitizer: mutex cannot be locked on this code path -// CHECK: #0 __tsan_check_no_mutexes_held -// CHECK: #1 ThreadFunc -// CHECK: Mutex {{.*}} acquired at: -// CHECK: #0 pthread_mutex_lock -// CHECK: #1 ThreadFunc -// CHECK: SUMMARY: ThreadSanitizer: mutex cannot be locked on this code path {{.*}}mutex_cannot_be_locked.cpp{{.*}}ThreadFunc diff --git a/compiler-rt/test/tsan/mutex_cannot_be_locked_success.cpp b/compiler-rt/test/tsan/mutex_cannot_be_locked_success.cpp deleted file mode 100644 index 7834dcc894778..0000000000000 --- a/compiler-rt/test/tsan/mutex_cannot_be_locked_success.cpp +++ /dev/null @@ -1,19 +0,0 @@ -// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s -#include "test.h" - -pthread_mutex_t mtx; - -void *ThreadFunc(void *) { - pthread_mutex_lock(&mtx); - pthread_mutex_unlock(&mtx); - __tsan_check_no_mutexes_held(); -} - -int main() { - pthread_t th; - pthread_create(&th, 0, ThreadFunc, NULL); - pthread_join(th, 0); - return 0; -} - -// CHECK-NOT: WARNING: ThreadSanitizer: mutex cannot be locked on this code path diff --git a/compiler-rt/test/tsan/mutex_held_wrong_context.cpp b/compiler-rt/test/tsan/mutex_held_wrong_context.cpp new file mode 100644 index 0000000000000..e9ded8fc1dc54 --- /dev/null +++ b/compiler-rt/test/tsan/mutex_held_wrong_context.cpp @@ -0,0 +1,34 @@ +// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s +#include "test.h" + +pthread_mutex_t mtx; + +void Func1() { + pthread_mutex_lock(&mtx); + __tsan_check_no_mutexes_held(); + pthread_mutex_unlock(&mtx); +} + +void Func2() { + pthread_mutex_lock(&mtx); + pthread_mutex_unlock(&mtx); + __tsan_check_no_mutexes_held(); +} + +int main() { + pthread_mutex_init(&mtx, NULL); + Func1(); + Func2(); + return 0; +} + +// CHECK: WARNING: ThreadSanitizer: mutex held in the wrong context +// CHECK: #0 __tsan_check_no_mutexes_held +// CHECK: #1 Func1 +// CHECK: #2 main +// CHECK: Mutex {{.*}} created at: +// CHECK: #0 pthread_mutex_init +// CHECK: #1 main +// CHECK: SUMMARY: ThreadSanitizer: mutex held in the wrong context {{.*}}mutex_held_wrong_context.cpp{{.*}}Func1 + +// CHECK-NOT: SUMMARY: ThreadSanitizer: mutex held in the wrong context {{.*}}mutex_held_wrong_context.cpp{{.*}}Func2