[RTSan][Darwin] Adjust OSSpinLock/_os_nospin_lock interceptor and tests #132867

thetruestblue · 2025-03-25T02:50:54Z

These changes align with these lock types and allows builds and tests to pass with various SDKS.

rdar://147067322

llvmbot · 2025-03-25T02:51:29Z

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

Author: None (thetruestblue)

Changes

These changes align with these lock types and allows builds and tests to pass with various SDKS.

rdar://147067322

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

2 Files Affected:

(modified) compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp (+13-23)
(modified) compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp (+14-21)

diff --git a/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp b/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
index 9d1a689a5a070..aed7f13635235 100644
--- a/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
@@ -21,24 +21,6 @@
 #include "rtsan/rtsan.h"
 
 #if SANITIZER_APPLE
-
-#if TARGET_OS_MAC
-// On MacOS OSSpinLockLock is deprecated and no longer present in the headers,
-// but the symbol still exists on the system. Forward declare here so we
-// don't get compilation errors.
-#include <stdint.h>
-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
-
 #include <libkern/OSAtomic.h>
 #include <os/lock.h>
 #endif // SANITIZER_APPLE
@@ -706,26 +688,34 @@ INTERCEPTOR(mode_t, umask, mode_t cmask) {
 #pragma clang diagnostic push
 // OSSpinLockLock is deprecated, but still in use in libc++
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
+#undef OSSpinLockLock
+
 INTERCEPTOR(void, OSSpinLockLock, volatile OSSpinLock *lock) {
   __rtsan_notify_intercepted_call("OSSpinLockLock");
   return REAL(OSSpinLockLock)(lock);
 }
-#pragma clang diagnostic pop
+
 #define RTSAN_MAYBE_INTERCEPT_OSSPINLOCKLOCK INTERCEPT_FUNCTION(OSSpinLockLock)
 #else
 #define RTSAN_MAYBE_INTERCEPT_OSSPINLOCKLOCK
 #endif // SANITIZER_APPLE
 
 #if SANITIZER_APPLE
-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);
-}
+typedef volatile OSSpinLock *_os_nospin_lock_t;
 
 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);
 }
+#endif // SANITIZER_APPLE
+#pragma clang diagnostic pop // "-Wdeprecated-declarations"
+
+#if SANITIZER_APPLE
+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);
+}
+
 #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 f12df9ea90855..7bde293a5fe34 100644
--- a/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
+++ b/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
@@ -1113,6 +1113,11 @@ TEST(TestRtsanInterceptors, PthreadJoinDiesWhenRealtime) {
 #pragma clang diagnostic push
 // OSSpinLockLock is deprecated, but still in use in libc++
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
+#undef OSSpinLockLock
+extern "C" {
+  typedef int32_t OSSpinLock;
+  void OSSpinLockLock(volatile OSSpinLock *__lock);
+}
 TEST(TestRtsanInterceptors, OsSpinLockLockDiesWhenRealtime) {
   auto Func = []() {
     OSSpinLock spin_lock{};
@@ -1121,7 +1126,15 @@ TEST(TestRtsanInterceptors, OsSpinLockLockDiesWhenRealtime) {
   ExpectRealtimeDeath(Func, "OSSpinLockLock");
   ExpectNonRealtimeSurvival(Func);
 }
-#pragma clang diagnostic pop
+
+TEST(TestRtsanInterceptors, OsNoSpinLockLockDiesWhenRealtime) {
+  OSSpinLock lock{};
+  auto Func = [&]() { _os_nospin_lock_lock(&lock); };
+  ExpectRealtimeDeath(Func, "_os_nospin_lock_lock");
+  ExpectNonRealtimeSurvival(Func);
+}
+#endif // SANITIZER_APPLE
+#pragma clang diagnostic pop //"-Wdeprecated-declarations"
 
 TEST(TestRtsanInterceptors, OsUnfairLockLockDiesWhenRealtime) {
   auto Func = []() {
@@ -1132,26 +1145,6 @@ TEST(TestRtsanInterceptors, OsUnfairLockLockDiesWhenRealtime) {
   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
 TEST(TestRtsanInterceptors, SpinLockLockDiesWhenRealtime) {
   pthread_spinlock_t spin_lock;

thetruestblue · 2025-03-25T02:52:32Z

@cjappl @davidtrevelyan could you confirm this works for you on both the OSes?

github-actions · 2025-03-25T02:54:13Z

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

davidtrevelyan · 2025-03-25T09:37:27Z

@cjappl @davidtrevelyan could you confirm this works for you on both the OSes?

Hi @thetruestblue - many thanks for your help on this. I've tried this patch and unfortunately get the following error:

cd /Users/david/Repositories/rtsan/llvm-project/build/projects/compiler-rt/lib/rtsan/tests && /Users/david/Repositories/rtsan/llvm-project/build/./bin/clang++ -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta -I/Users/david/Repositories/rtsan/llvm-project/compiler-rt/include -g -Wno-covered-switch-default -Wno-suggest-override -DGTEST_NO_LLVM_SUPPORT=1 -DGTEST_HAS_RTTI=0 -I/Users/david/Repositories/rtsan/llvm-project/llvm/../third-party/unittest/googletest/include -I/Users/david/Repositories/rtsan/llvm-project/llvm/../third-party/unittest/googletest -DGTEST_NO_LLVM_SUPPORT=1 -DGTEST_HAS_RTTI=0 -I/Users/david/Repositories/rtsan/llvm-project/llvm/../third-party/unittest/googlemock/include -I/Users/david/Repositories/rtsan/llvm-project/llvm/../third-party/unittest/googlemock -I/Users/david/Repositories/rtsan/llvm-project/compiler-rt/lib/ -I/Users/david/Repositories/rtsan/llvm-project/compiler-rt/include/ -I/Users/david/Repositories/rtsan/llvm-project/compiler-rt/lib/rtsan -I/Users/david/Repositories/rtsan/llvm-project/compiler-rt/lib/sanitizer_common/tests -DSANITIZER_COMMON_NO_REDEFINE_BUILTINS -O2 -stdlib=libc++ -mmacosx-version-min=15.1 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -fsanitize=realtime -arch arm64 -c -o RtsanTestObjects.rtsan_test_interceptors_posix.cpp.arm64.o /Users/david/Repositories/rtsan/llvm-project/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
/Users/david/Repositories/rtsan/llvm-project/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp:1132:23: error: use of undeclared identifier '_os_nospin_lock_lock'; did you mean 'os_unfair_lock_lock'?
 1132 |   auto Func = [&]() { _os_nospin_lock_lock(&lock); };
      |                       ^~~~~~~~~~~~~~~~~~~~
      |                       os_unfair_lock_lock
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/os/lock.h:115:6: note: 'os_unfair_lock_lock' declared here
  115 | void os_unfair_lock_lock(os_unfair_lock_t lock);
      |      ^
/Users/david/Repositories/rtsan/llvm-project/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp:1132:44: error: cannot initialize a parameter of type 'os_unfair_lock_t _Nonnull' (aka 'os_unfair_lock_s *') with an rvalue of type 'OSSpinLock *' (aka 'int *')
 1132 |   auto Func = [&]() { _os_nospin_lock_lock(&lock); };
      |                                            ^~~~~
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/os/lock.h:115:43: note: passing argument to parameter 'lock' here
  115 | void os_unfair_lock_lock(os_unfair_lock_t lock);
      |                                           ^
2 errors generated.

In the above, MacOSX.sdk is a soft link to the 15.2 SDK. Hope this is helpful for your investigations!

These changes align with these lock types and allows builds and tests to pass with various SDKS. rdar://147067322

thetruestblue · 2025-04-15T20:24:06Z

@cjappl @davidtrevelyan

Updated and requesting review.

cjappl

Thanks for circling back to this! What was the big difference maker in this version?

LGTM

thetruestblue · 2025-04-15T21:46:31Z

Thanks for circling back to this! What was the big difference maker in this version?

LGTM

Previously we were getting name duplication errors with our SDKs. This just makes the forward declaration match whats in the header.

compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp

davidtrevelyan · 2025-04-16T21:33:25Z

compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp

+extern "C" {
+typedef int32_t OSSpinLock;
+void OSSpinLockLock(volatile OSSpinLock *__lock);
+typedef volatile OSSpinLock *_os_nospin_lock_t;


Would it be possible for the team to share some context about this line? I don't yet understand why we're aliasing a lock named Spin with a type that has the (seemingly contradictory) nospin in its name. Any insight much appreciated here.

This was done as part of the process for deprecating OSSpinLock years ago. What seems to have changed is the replacement happening in atomic_load seems to be due to the build min version of compiler rt library changing, which is why the behavior changed in a specific OS. I don't have info on why we deprecated the lock in this way.

Theoretically this should be a transparent interceptor, and you should only need to intercept OSSpinLock. But because atomic_load calls the alias and we expect to be able to intercept the atomic load behavior, we need to add this for that singular case.

All good - that makes sense - thanks for the info. Would you mind adding this context as a comment into the code at the points where the typedef is made (here and in rtsan_interceptors_posix.cpp)? I'd be happy to add the context comments myself, but I would want to get your approval on the wording before merging. Let me know - then I think this PR should be good to go in 👍

davidtrevelyan · 2025-04-17T18:43:16Z

Many thanks for your efforts here @thetruestblue - this LGTM. I'll merge this when I have a bit of time ahead of me to handle any updates if necessary after merge. If @cjappl is happy to handle this - I'm happy for this to be merged as soon as he's ready 👍

thetruestblue · 2025-04-18T17:47:48Z

Let me know, if I can merge @davidtrevelyan @cjappl would be nice to land this for our own building sanity and to avoid needing work arounds. I'd also like to get appropriate cherry-pick opened for release branch since theres CI test failures there.

davidtrevelyan · 2025-04-18T18:25:57Z

Thanks for the reminder @thetruestblue - all good to go from here.

…ts (llvm#132867) These changes align with these lock types and allows builds and tests to pass with various SDKS. rdar://147067322 (cherry picked from commit 7cc4472)

thetruestblue requested review from cjappl and davidtrevelyan March 25, 2025 02:51

llvmbot added compiler-rt compiler-rt:sanitizer labels Mar 25, 2025

[RTSan][Darwin] Fix OSSpinLock interceptors and tests

60a85e9

These changes align with these lock types and allows builds and tests to pass with various SDKS. rdar://147067322

thetruestblue force-pushed the rtsanfixspin branch from 236820c to 60a85e9 Compare April 15, 2025 20:23

cjappl approved these changes Apr 15, 2025

View reviewed changes

thetruestblue force-pushed the rtsanfixspin branch 2 times, most recently from 8c6018b to 35152aa Compare April 16, 2025 21:08

davidtrevelyan reviewed Apr 16, 2025

View reviewed changes

thetruestblue force-pushed the rtsanfixspin branch from 35152aa to 97914ae Compare April 17, 2025 01:46

clang-format

fe3bb4b

thetruestblue force-pushed the rtsanfixspin branch from 97914ae to fe3bb4b Compare April 17, 2025 16:40

davidtrevelyan approved these changes Apr 17, 2025

View reviewed changes

davidtrevelyan merged commit 7cc4472 into llvm:main Apr 18, 2025
10 checks passed

thetruestblue deleted the rtsanfixspin branch April 25, 2025 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RTSan][Darwin] Adjust OSSpinLock/_os_nospin_lock interceptor and tests #132867

[RTSan][Darwin] Adjust OSSpinLock/_os_nospin_lock interceptor and tests #132867

thetruestblue commented Mar 25, 2025

llvmbot commented Mar 25, 2025

thetruestblue commented Mar 25, 2025

github-actions bot commented Mar 25, 2025 •

edited

Loading

davidtrevelyan commented Mar 25, 2025

thetruestblue commented Apr 15, 2025 •

edited

Loading

cjappl left a comment

thetruestblue commented Apr 15, 2025

davidtrevelyan Apr 16, 2025

thetruestblue Apr 17, 2025 •

edited

Loading

davidtrevelyan Apr 17, 2025

davidtrevelyan commented Apr 17, 2025

thetruestblue commented Apr 18, 2025

davidtrevelyan commented Apr 18, 2025

[RTSan][Darwin] Adjust OSSpinLock/_os_nospin_lock interceptor and tests #132867

[RTSan][Darwin] Adjust OSSpinLock/_os_nospin_lock interceptor and tests #132867

Conversation

thetruestblue commented Mar 25, 2025

llvmbot commented Mar 25, 2025

thetruestblue commented Mar 25, 2025

github-actions bot commented Mar 25, 2025 • edited Loading

davidtrevelyan commented Mar 25, 2025

thetruestblue commented Apr 15, 2025 • edited Loading

cjappl left a comment

Choose a reason for hiding this comment

thetruestblue commented Apr 15, 2025

davidtrevelyan Apr 16, 2025

Choose a reason for hiding this comment

thetruestblue Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

davidtrevelyan Apr 17, 2025

Choose a reason for hiding this comment

davidtrevelyan commented Apr 17, 2025

thetruestblue commented Apr 18, 2025

davidtrevelyan commented Apr 18, 2025

github-actions bot commented Mar 25, 2025 •

edited

Loading

thetruestblue commented Apr 15, 2025 •

edited

Loading

thetruestblue Apr 17, 2025 •

edited

Loading