-
Notifications
You must be signed in to change notification settings - Fork 15.1k
release/20.x: [rtsan][Apple] Add interceptor for _os_nospin_lock_lock (#131034) #132997
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
Conversation
Follows the discussion here: llvm#129309 Recently, the test `TestRtsan.AccessingALargeAtomicVariableDiesWhenRealtime` has been failing on newer MacOS versions, because the internal locking mechanism in `std::atomic<T>::load` (for types `T` that are larger than the hardware lock-free limit), has changed to a function that wasn't being intercepted by rtsan. This PR introduces an interceptor for `_os_nospin_lock_lock`, which is the new internal locking mechanism. _Note: we'd probably do well to introduce interceptors for `_os_nospin_lock_unlock` (and `os_unfair_lock_unlock`) too, which also appear to have blocking implementations. This can follow in a separate PR._ (cherry picked from commit 481a55a)
@cjappl What do you think about merging this PR to the release branch? |
@llvm/pr-subscribers-compiler-rt-sanitizer Author: None (llvmbot) ChangesBackport 481a55a Requested by: @wrotki Full diff: https://github.com/llvm/llvm-project/pull/132997.diff 2 Files Affected:
diff --git a/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp b/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
index 6816119065263..4d602a88ba9ae 100644
--- a/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
@@ -30,6 +30,12 @@
extern "C" {
typedef int32_t OSSpinLock;
void OSSpinLockLock(volatile OSSpinLock *__lock);
+// A pointer to this type is in the interface for `_os_nospin_lock_lock`, but
+// it's an internal implementation detail of `os/lock.c` on Darwin, and
+// therefore not available in any headers. As a workaround, we forward declare
+// it here, which is enough to facilitate interception of _os_nospin_lock_lock.
+struct _os_nospin_lock_s;
+using _os_nospin_lock_t = _os_nospin_lock_s *;
}
#endif // TARGET_OS_MAC
@@ -642,6 +648,11 @@ INTERCEPTOR(void, os_unfair_lock_lock, os_unfair_lock_t lock) {
__rtsan_notify_intercepted_call("os_unfair_lock_lock");
return REAL(os_unfair_lock_lock)(lock);
}
+
+INTERCEPTOR(void, _os_nospin_lock_lock, _os_nospin_lock_t lock) {
+ __rtsan_notify_intercepted_call("_os_nospin_lock_lock");
+ return REAL(_os_nospin_lock_lock)(lock);
+}
#define RTSAN_MAYBE_INTERCEPT_OS_UNFAIR_LOCK_LOCK \
INTERCEPT_FUNCTION(os_unfair_lock_lock)
#else
diff --git a/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp b/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
index 59663776366bb..75f723081c4b6 100644
--- a/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
+++ b/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
@@ -1058,6 +1058,25 @@ TEST(TestRtsanInterceptors, OsUnfairLockLockDiesWhenRealtime) {
ExpectRealtimeDeath(Func, "os_unfair_lock_lock");
ExpectNonRealtimeSurvival(Func);
}
+
+// We intercept _os_nospin_lock_lock because it's the internal
+// locking mechanism for MacOS's atomic implementation for data
+// types that are larger than the hardware's maximum lock-free size.
+// However, it's a private implementation detail and not visible in any headers,
+// so we must duplicate the required type definitions to forward declaration
+// what we need here.
+extern "C" {
+struct _os_nospin_lock_s {
+ unsigned int oul_value;
+};
+void _os_nospin_lock_lock(_os_nospin_lock_s *);
+}
+TEST(TestRtsanInterceptors, OsNoSpinLockLockDiesWhenRealtime) {
+ _os_nospin_lock_s lock{};
+ auto Func = [&]() { _os_nospin_lock_lock(&lock); };
+ ExpectRealtimeDeath(Func, "_os_nospin_lock_lock");
+ ExpectNonRealtimeSurvival(Func);
+}
#endif
#if SANITIZER_LINUX
|
|
@cjappl I misunderstood @thetruestblue 's request, she asked me to cherry-pick XFAIL on the test, not your change. Apologies - I'll let you cherry-pick this yourself once you decide it's ready 🙂 |
Sounds good! Yes, it seems like there is more to understand about the original fix before we cherry-pick it. :) |
Can we close this? |
@tstellar yes - but I thought this PR was already closed? |
Backport 481a55a
Requested by: @wrotki