[compiler-rt] [TSAN] Add __tsan_check_no_mutexes_held helper (PR #69372)
Kenny Yu via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 2 15:18:20 PDT 2023
https://github.com/kennyyu updated https://github.com/llvm/llvm-project/pull/69372
>From 0f923a9c9e689f8cbd0f6ed348a44cc05d5e9b2d Mon Sep 17 00:00:00 2001
From: Kenny Yu <kennyyu at meta.com>
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 f19c79d79ba62ef..cd118441cb5ce24 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 a5bd17176b12bfa..6416e5d47fc4131 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 1e61c31c5a970cc..026d5b43299355b 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 6bd72e18d942580..712ce2d1a45a868 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 4028d1810784095..673449a9e70af9c 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<void *>(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<void *>(rm->addr));
+ if (typ != ReportTypeMutexCannotBeLocked) {
+ Printf(" Mutex M%u (%p) created at:\n", rm->id,
+ reinterpret_cast<void *>(rm->addr));
+ } else {
+ Printf(" Mutex M%u (%p) acquired at:\n", rm->id,
+ reinterpret_cast<void *>(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 3c88864af147704..0e1a27655bfa4ca 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 9cdfa32a9343029..3b249a716ed2c31 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 000000000000000..77ce2d7053374ea
--- /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 000000000000000..7834dcc89477806
--- /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 <kennyyu at meta.com>
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 cd118441cb5ce24..9ea7e8f004b0cf6 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 026d5b43299355b..41fa293dbaaad0b 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 712ce2d1a45a868..5154662034c56d9 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 673449a9e70af9c..35cb6710a54fa42 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<void *>(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<void *>(rm->addr));
- } else {
- Printf(" Mutex M%u (%p) acquired at:\n", rm->id,
- reinterpret_cast<void *>(rm->addr));
- }
+ Printf(" Mutex M%u (%p) created at:\n", rm->id,
+ reinterpret_cast<void *>(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 0e1a27655bfa4ca..bfe470797f8f77a 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 3b249a716ed2c31..70642124990d7bd 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 77ce2d7053374ea..000000000000000
--- 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 7834dcc89477806..000000000000000
--- 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 000000000000000..e9ded8fc1dc54d4
--- /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
More information about the llvm-commits
mailing list