Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TSan] Increase the number of simultaneously locked mutexes that a thread can hold #116409

Merged

Conversation

gbMattN
Copy link
Contributor

@gbMattN gbMattN commented Nov 15, 2024

I've run into an issue where TSan can't be used on some code without turning off deadlock detection because a thread tries to hold too many mutexes. It would be preferable to be able to use deadlock detection as that is a major benefit of TSan.

Its mentioned in google/sanitizers#950 that the 64 mutex limit was an arbitrary number. I've increased it to 128 and all the tests still pass. Considering the increasing number of cores on CPUs and how programs can now use more threads to take advantage of it, I think raising the limit to 128 would be some good future proofing

Copy link

github-actions bot commented Nov 15, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@llvmbot
Copy link
Member

llvmbot commented Nov 16, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: None (gbMattN)

Changes

I've run into an issue where TSan can't be used on some code without turning off deadlock detection because a thread tries to hold too many mutexes. It would be preferable to be able to use deadlock detection as that is a major benefit of TSan.

Its mentioned in google/sanitizers#950 that the 64 mutex limit was an arbitrary number. I've increased it to 128 and all the tests still pass. Considering the increasing number of cores on CPUs and how programs can now use more threads to take advantage of it, I think raising the limit to 128 would be some good future proofing


Full diff: https://github.com/llvm/llvm-project/pull/116409.diff

2 Files Affected:

  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_deadlock_detector.h (+1-1)
  • (added) compiler-rt/test/tsan/many_held_mutex.cpp (+23)
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_deadlock_detector.h b/compiler-rt/lib/sanitizer_common/sanitizer_deadlock_detector.h
index 0749f633b4bcf5..1664b92b213692 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_deadlock_detector.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_deadlock_detector.h
@@ -120,7 +120,7 @@ class DeadlockDetectorTLS {
     u32 lock;
     u32 stk;
   };
-  LockWithContext all_locks_with_contexts_[64];
+  LockWithContext all_locks_with_contexts_[128];
   uptr n_all_locks_;
 };
 
diff --git a/compiler-rt/test/tsan/many_held_mutex.cpp b/compiler-rt/test/tsan/many_held_mutex.cpp
new file mode 100644
index 00000000000000..479aa20a7b9df3
--- /dev/null
+++ b/compiler-rt/test/tsan/many_held_mutex.cpp
@@ -0,0 +1,23 @@
+// RUN: %clangxx_tsan %s -fsanitize=thread -o %t && %run %t 2>&1 | Filecheck %s
+
+#include <mutex>
+#include <stdio.h>
+
+int main(){
+    const unsigned short NUM_OF_MTX = 128;
+    std::mutex mutexes[NUM_OF_MTX];
+
+    for(int i = 0; i < NUM_OF_MTX; i++){
+        mutexes[i].lock();
+    }
+    for(int i = 0; i < NUM_OF_MTX; i++){
+        mutexes[i].unlock();
+    }
+
+    printf("Success\n");
+
+    return 0;
+}
+
+// CHECK: Success
+// CHECK-NOT: ThreadSanitizer: CHECK failed

@gbMattN
Copy link
Contributor Author

gbMattN commented Nov 18, 2024

@vitalybuka

@vitalybuka vitalybuka requested a review from dvyukov November 19, 2024 01:28
#include <mutex>
#include <stdio.h>

int main() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make it command line parameter
so

&& %run % 128
&& not %run % 129

mutexes[i].unlock();
}

printf("Success\n");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need printf, and filecheck here
exit code 0 is enough

@vitalybuka
Copy link
Collaborator

LGTM,
CC @dvyukov is case there is hidden context

@dvyukov
Copy link
Collaborator

dvyukov commented Nov 19, 2024

I don't remember any problems that this can cause. Should be fine to increase.

@vitalybuka
Copy link
Collaborator

Looks like related failure on CI? https://buildkite.com/llvm-project/github-pull-requests/builds/120991

@vitalybuka vitalybuka self-requested a review November 20, 2024 20:39
vitalybuka added a commit that referenced this pull request Nov 24, 2024
@vitalybuka vitalybuka force-pushed the users/nagym/increase_tsan_consecutive_mutex_limit branch from 34bd2a5 to 79993af Compare November 24, 2024 08:50
@vitalybuka vitalybuka merged commit 4d4a353 into llvm:main Nov 24, 2024
7 checks passed
@gbMattN
Copy link
Contributor Author

gbMattN commented Nov 25, 2024

Thanks!

@gbMattN gbMattN deleted the users/nagym/increase_tsan_consecutive_mutex_limit branch January 20, 2025 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants