-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[rtsan] Intercept aligned_alloc on all versions of OSX if available on build machine #112780
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
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Chris Apple (cjappl) ChangesThis follows the pattern seen here, where tsan uses libdispatch, even if the functions aren't available in our minimum version. llvm-project/compiler-rt/lib/tsan/rtl/tsan_interceptors_libdispatch.cpp Lines 226 to 247 in f7669ba
I think it would be good to propagate this change to the other sanitizers, as currently, no sanitizer actually intercepts aligned alloc on mac, stemming from:
Ideally, if this pattern looked good, we could add it to sanitizer_malloc_mac.inc, and the other sanitizers would have this enabled. Full diff: https://github.com/llvm/llvm-project/pull/112780.diff 2 Files Affected:
diff --git a/compiler-rt/lib/rtsan/rtsan_interceptors.cpp b/compiler-rt/lib/rtsan/rtsan_interceptors.cpp
index 63b0ca28a1f409..7e2e9c71786c06 100644
--- a/compiler-rt/lib/rtsan/rtsan_interceptors.cpp
+++ b/compiler-rt/lib/rtsan/rtsan_interceptors.cpp
@@ -461,7 +461,17 @@ INTERCEPTOR(void *, valloc, SIZE_T size) {
return REAL(valloc)(size);
}
-#if SANITIZER_INTERCEPT_ALIGNED_ALLOC
+// aligned_alloc was introduced in OSX 10.15
+// Linking will fail when using an older SDK
+#if defined(__MAC_10_15)
+// macOS 10.15 is greater than our minimal deployment target. To ensure we
+// generate a weak reference so the dylib continues to work on older
+// systems, we need to forward declare the intercepted function as "weak
+// imports".
+SANITIZER_WEAK_IMPORT void *aligned_alloc(SIZE_T __alignment, SIZE_T __size);
+#endif // defined(__MAC_10_15)
+
+#if SANITIZER_INTERCEPT_ALIGNED_ALLOC || SANITIZER_APPLE
INTERCEPTOR(void *, aligned_alloc, SIZE_T alignment, SIZE_T size) {
__rtsan_notify_intercepted_call("aligned_alloc");
return REAL(aligned_alloc)(alignment, size);
@@ -471,6 +481,8 @@ INTERCEPTOR(void *, aligned_alloc, SIZE_T alignment, SIZE_T size) {
#define RTSAN_MAYBE_INTERCEPT_ALIGNED_ALLOC
#endif
+#undef RTSAN_DARWIN_INTERCEPT_ALIGNED_ALLOC
+
INTERCEPTOR(int, posix_memalign, void **memptr, size_t alignment, size_t size) {
__rtsan_notify_intercepted_call("posix_memalign");
return REAL(posix_memalign)(memptr, alignment, size);
diff --git a/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors.cpp b/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors.cpp
index f7a281f13e2ff6..fb227a1d1bc05a 100644
--- a/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors.cpp
+++ b/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors.cpp
@@ -120,13 +120,17 @@ TEST(TestRtsanInterceptors, VallocDiesWhenRealtime) {
ExpectNonRealtimeSurvival(Func);
}
-#if SANITIZER_INTERCEPT_ALIGNED_ALLOC
TEST(TestRtsanInterceptors, AlignedAllocDiesWhenRealtime) {
- auto Func = []() { EXPECT_NE(nullptr, aligned_alloc(16, 32)); };
- ExpectRealtimeDeath(Func, "aligned_alloc");
- ExpectNonRealtimeSurvival(Func);
-}
+#if SANITIZER_APPLE
+ if (__builtin_available(macOS 10.15, *)) {
+#endif // SANITIZER_APPLE
+ auto Func = []() { EXPECT_NE(nullptr, aligned_alloc(16, 32)); };
+ ExpectRealtimeDeath(Func, "aligned_alloc");
+ ExpectNonRealtimeSurvival(Func);
+#if SANITIZER_APPLE
+ }
#endif
+}
// free_sized and free_aligned_sized (both C23) are not yet supported
TEST(TestRtsanInterceptors, FreeDiesWhenRealtime) {
|
9ac834e
to
d5b090a
Compare
@yln specifically would love your eyes on this, as you implemented this pattern for TSAN. Does this seem reasonable to you? Any pitfalls I should worry about? |
ExpectNonRealtimeSurvival(Func); | ||
} | ||
#if SANITIZER_APPLE | ||
if (__builtin_available(macOS 10.15, *)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only place in compiler-rt that __builtin_available is used, is there a more preferable way to do this? If I don't, I get a warning that suggests this:
warning: 'aligned_alloc' is only available on macOS 10.15 or newer [-Wu
nguarded-availability-new]
...
_malloc.h:65:7: note: 'aligned_alloc' has been marked as being
introduced in macOS 10.15 here, but the deployment target is macOS 10.13.0
65 | void *aligned_alloc(size_t __alignment, size_t __size) __result_use_check __alloc_align(1) __alloc_size(2) _MALLOC_TYPED(malloc_type_aligned_alloc, 2) __OSX_AVAILABLE(10.15)
__IOS_AVAILABLE(13.0) __TVOS_AVAILABLE(13.0) __WATCHOS_AVAILABLE(6.0);
...
note: enclose 'aligned_alloc' in a __builtin_available check to silence
this warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my catch-up reading about this issue I found this documentation page, which suggests comparing the weak function pointer to NULL
as a check. Not sure if this would be preferable here, but thought I'd drop it here just in case you hadn't seen it already 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great find on those docs, I'll spend some time digging through them.
Taking the very first naive crack at it, it appears to have similar errors (please enclose in __builtin_available).
That was repro'd applying this patch:
diff --git a/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors.cpp b/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors.cpp
index fb227a1d1bc0..ad5c52fe7afc 100644
--- a/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors.cpp
+++ b/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors.cpp
@@ -120,9 +120,11 @@ TEST(TestRtsanInterceptors, VallocDiesWhenRealtime) {
ExpectNonRealtimeSurvival(Func);
}
+extern void* aligned_alloc(size_t, size_t) __attribute__((weak_import));
+
TEST(TestRtsanInterceptors, AlignedAllocDiesWhenRealtime) {
#if SANITIZER_APPLE
- if (__builtin_available(macOS 10.15, *)) {
+ if (aligned_alloc != nullptr)
#endif // SANITIZER_APPLE
auto Func = []() { EXPECT_NE(nullptr, aligned_alloc(16, 32)); };
ExpectRealtimeDeath(Func, "aligned_alloc");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, doing some additional digging here, it seems like this __builtin_available is used in almost all subprojects of LLVM except compiler-rt, so there is reasonable support for it in the superproject at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, in that case it sounds like __builtin_available
is the recommended way forward, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All signs seem to be pointing that way. I definitely worry (since the std::variant
debacle) about using something in compiler-rt that hasn't been used before, but seeing as it is only in a test file it seems less risky.
Also to be clear, this test passes on my mac that has aligned_alloc
available, so this all works as intended :)
@@ -461,6 +461,19 @@ INTERCEPTOR(void *, valloc, SIZE_T size) { | |||
return REAL(valloc)(size); | |||
} | |||
|
|||
// aligned_alloc was introduced in OSX 10.15 | |||
// Linking will fail when using an older SDK | |||
#if SANITIZER_APPLE && defined(__MAC_10_15) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just put this into sanitizer_platform_interceptors.h ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it up in the latest commit. I initially did not put it on the top level to avoid any possible breakages with other sanitizers.
After looking, it seems like the only sanitizer that interacts with aligned_alloc outside of a *_linux.cpp file is lsan, and I feel reasonably confident this should work for that sanitizer with the weak import defined.
1e261bd
to
1e127da
Compare
Weekly reviewer ping for outstanding. Specifically would love @yln 's thoughts. I'll merge this early next week if no new info |
Unfortunately this breaks the build on our (automerger) bots, which have -mmacosx-version-min=10.13 and also -Werror=unguarded-availability-new . I was thinking about patching it via wrapping in __builtin_available check (which I believe is the right one to use, as it should match the -mmacosx-version-min ) - but can't actually think of a quick fix, due to interceptors being defined via C macros. Can I ask you to revert this for a while, so we can come up with a right remedy? |
Sorry for the breakage @wrotki ! Can you share the exact error message that is the problem? I was hopeful that the __builtin_available I used in the unit test was enough to cover us. After I have a look at that, happy to revert if there is no clear solution. |
Here goes:
|
Ah, what a pain. Ok, I'll revert and ping you in the reversion. I would love your help in trying to get a fix that works, as I don't know how I would repro this on my machine. |
Great, thanks a lot. I'll be watching here. The repro - I guess you could specify these clang options: |
…ilable on build machine (llvm#112780)" This reverts commit 97fb21a.
…ilable on build machine (#112780)" (#113982) This reverts commit 97fb21a. Due to issue brought up in #112780 > Unfortunately this breaks the build on our (automerger) bots, which have -mmacosx-version-min=10.13 and also -Werror=unguarded-availability-new . I was thinking about patching it via wrapping in __builtin_available check (which I believe is the right one to use, as it should match the -mmacosx-version-min ) - but can't actually think of a quick fix, due to interceptors being defined via C macros. > llvm-project/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp:475:21: error: 'aligned_alloc' is only available on macOS 10.15 or newer [-Werror,-Wunguarded-availability-new] 475 | INTERCEPTOR(void *, aligned_alloc, SIZE_T alignment, SIZE_T size) {
That revert has been submitted. I'll try and get a more substantial fix for you to try, let me know if you have any breakthroughs |
@wrotki - a few thoughts here, wondering if I can get your input. First, I can't repro this issue still. Strangely I do get the warning in the unit tests (which is why I used __builtin_available in the tests) but not in the source. It appears I'm just using the default sanitizer min OSX version (10.13) Second, what if we just disabled this warning for aligned_alloc? (took this original patch, and additionally) diff --git a/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp b/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
index 890d6c11c407..7be1f07733f0 100644
--- a/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
@@ -464,10 +464,13 @@ INTERCEPTOR(void *, valloc, SIZE_T size) {
}
#if SANITIZER_INTERCEPT_ALIGNED_ALLOC
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wunguarded-availability-new"
INTERCEPTOR(void *, aligned_alloc, SIZE_T alignment, SIZE_T size) {
__rtsan_notify_intercepted_call("aligned_alloc");
return REAL(aligned_alloc)(alignment, size);
}
+#pragma clang diagnostic pop
#define RTSAN_MAYBE_INTERCEPT_ALIGNED_ALLOC INTERCEPT_FUNCTION(aligned_alloc)
#else
#define RTSAN_MAYBE_INTERCEPT_ALIGNED_ALLOC
Why is this OK? (Theory) Third (as a completely different thought) This approach was cribbed from the tsan approach, which pre-declares some libdispatch API only available in 10.14. However, it seems as if your build bot has been happy with it. So far, I haven't been able to see the difference between my approach and the tsan approach, maybe there is some clue there you can see? The code I based this off of is here: llvm-project/compiler-rt/lib/tsan/rtl/tsan_interceptors_libdispatch.cpp Lines 226 to 247 in f7669ba
|
Ah, yes, that's a good idea, silence the warning so it won't trigger build failure on the bot which upgrades to error. I applied this to my local llvm-project clone and the build worked. I agree that this will never work on 10.13, and fine with it. If you want to reproduce yourself, you might want to clone llvm-project from Apple fork on Github: [email protected]:swiftlang/llvm-project.git |
Sounds good, I'm going to momentarily put up a PR re-landing this. I'll add you as a reviewer. Thanks for running it on your fork and ensuring it works!! |
…ailable on build machine (llvm#112780)" (llvm#113982) This reverts commit 7c55426.
…ilable on build machine (llvm#112780)" (llvm#113982) This reverts commit 97fb21a. Due to issue brought up in llvm#112780 > Unfortunately this breaks the build on our (automerger) bots, which have -mmacosx-version-min=10.13 and also -Werror=unguarded-availability-new . I was thinking about patching it via wrapping in __builtin_available check (which I believe is the right one to use, as it should match the -mmacosx-version-min ) - but can't actually think of a quick fix, due to interceptors being defined via C macros. > llvm-project/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp:475:21: error: 'aligned_alloc' is only available on macOS 10.15 or newer [-Werror,-Wunguarded-availability-new] 475 | INTERCEPTOR(void *, aligned_alloc, SIZE_T alignment, SIZE_T size) {
This follows the pattern seen here, where tsan uses libdispatch, even if the functions aren't available in our minimum version.
llvm-project/compiler-rt/lib/tsan/rtl/tsan_interceptors_libdispatch.cpp
Lines 226 to 247 in f7669ba
I think it would be good to propagate this change to the other sanitizers, as currently, no sanitizer actually intercepts aligned alloc on mac, stemming from:
llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
Line 497 in f7669ba
Ideally, if this pattern looked good, we could add it to sanitizer_malloc_mac.inc, and the other sanitizers would have this enabled.