Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 18, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60a85e9
[RTSan][Darwin] Fix OSSpinLock interceptors and tests
thetruestblue Mar 25, 2025
fe3bb4b
clang-format
thetruestblue Apr 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 14 additions & 23 deletions compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -706,26 +688,35 @@ 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);
}
// _os_nospin_lock_lock may replace OSSpinLockLock due to deprecation macro.
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);
}
#pragma clang diagnostic pop // "-Wdeprecated-declarations"
#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);
}

#define RTSAN_MAYBE_INTERCEPT_OS_UNFAIR_LOCK_LOCK \
INTERCEPT_FUNCTION(os_unfair_lock_lock)
#else
Expand Down
40 changes: 18 additions & 22 deletions compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,10 +1109,18 @@ TEST(TestRtsanInterceptors, PthreadJoinDiesWhenRealtime) {
}

#if SANITIZER_APPLE

#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);
// _os_nospin_lock_lock may replace OSSpinLockLock due to deprecation macro.
typedef volatile OSSpinLock *_os_nospin_lock_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@thetruestblue thetruestblue Apr 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

void _os_nospin_lock_lock(_os_nospin_lock_t lock);
}

TEST(TestRtsanInterceptors, OsSpinLockLockDiesWhenRealtime) {
auto Func = []() {
OSSpinLock spin_lock{};
Expand All @@ -1121,7 +1129,14 @@ 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);
}
#pragma clang diagnostic pop //"-Wdeprecated-declarations"

TEST(TestRtsanInterceptors, OsUnfairLockLockDiesWhenRealtime) {
auto Func = []() {
Expand All @@ -1131,26 +1146,7 @@ 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
#endif // SANITIZER_APPLE

#if SANITIZER_LINUX
TEST(TestRtsanInterceptors, SpinLockLockDiesWhenRealtime) {
Expand Down
Loading