Skip to content

Conversation

cjappl
Copy link
Contributor

@cjappl cjappl commented Oct 29, 2024

This commit reverts commit 7c55426 (relands commit 97fb21a)

The additional change can be seen in the second commit - disable the problematic warning by putting a pragma push/pop.

As described on #112780:

Why is this OK? (to disable this warning)
We know that it will never be called on systems that do not have aligned_alloc defined because the client OSX won't provide it. This function is actually guarded on OS's lower than 10.15 because aligned_alloc declaration won't exist on that version. There will be no way to call this function from code.

@llvmbot
Copy link
Member

llvmbot commented Oct 29, 2024

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

Author: Chris Apple (cjappl)

Changes

This commit reverts commit 7c55426 (relands commit 97fb21a)

The additional change can be seen in the second commit - disable the problematic warning by putting a pragma push/pop.

As described on #112780:

> Why is this OK? (to disable this warning)
> We know that it will never be called on systems that do not have aligned_alloc defined because the client OSX won't provide it. This function is actually guarded on OS's lower than 10.15 because aligned_alloc declaration won't exist on that version. There will be no way to call this function from code.


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

3 Files Affected:

  • (modified) compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp (+3)
  • (modified) compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp (+12-5)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h (+21-1)
diff --git a/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp b/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
index 890d6c11c40762..7be1f07733f0d6 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
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 6233c3e91800e1..38274485c29f66 100644
--- a/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
+++ b/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
@@ -122,13 +122,20 @@ TEST(TestRtsanInterceptors, VallocDiesWhenRealtime) {
   ExpectNonRealtimeSurvival(Func);
 }
 
-#if SANITIZER_INTERCEPT_ALIGNED_ALLOC
+#if __has_builtin(__builtin_available) && SANITIZER_APPLE
+#define ALIGNED_ALLOC_AVAILABLE() (__builtin_available(macOS 10.15, *))
+#else
+// We are going to assume this is true until we hit systems where it isn't
+#define ALIGNED_ALLOC_AVAILABLE() (true)
+#endif
+
 TEST(TestRtsanInterceptors, AlignedAllocDiesWhenRealtime) {
-  auto Func = []() { EXPECT_NE(nullptr, aligned_alloc(16, 32)); };
-  ExpectRealtimeDeath(Func, "aligned_alloc");
-  ExpectNonRealtimeSurvival(Func);
+  if (ALIGNED_ALLOC_AVAILABLE()) {
+    auto Func = []() { EXPECT_NE(nullptr, aligned_alloc(16, 32)); };
+    ExpectRealtimeDeath(Func, "aligned_alloc");
+    ExpectNonRealtimeSurvival(Func);
+  }
 }
-#endif
 
 // free_sized and free_aligned_sized (both C23) are not yet supported
 TEST(TestRtsanInterceptors, FreeDiesWhenRealtime) {
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h b/compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
index 6959a6d52d604e..3fd6b595ef197f 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
@@ -84,6 +84,25 @@
 #define SI_NOT_MAC 1
 #endif
 
+#if SANITIZER_APPLE
+#  include <Availability.h>
+
+// 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(__sanitizer::usize __alignment,
+                                          __sanitizer::usize __size);
+#    define SI_MAC_SDK_10_15_AVAILABLE 1
+#  else
+#    define SI_MAC_SDK_10_15_AVAILABLE 0
+#  endif  // defined(__MAC_10_15)
+
+#endif  // SANITIZER_APPLE
+
 #if SANITIZER_IOS
 #define SI_IOS 1
 #else
@@ -500,7 +519,8 @@
 #define SANITIZER_INTERCEPT_PVALLOC (SI_GLIBC || SI_ANDROID)
 #define SANITIZER_INTERCEPT_CFREE (SI_GLIBC && !SANITIZER_RISCV64)
 #define SANITIZER_INTERCEPT_REALLOCARRAY SI_POSIX
-#define SANITIZER_INTERCEPT_ALIGNED_ALLOC (!SI_MAC)
+#define SANITIZER_INTERCEPT_ALIGNED_ALLOC \
+  (!SI_MAC || SI_MAC_SDK_10_15_AVAILABLE)
 #define SANITIZER_INTERCEPT_MALLOC_USABLE_SIZE (!SI_MAC && !SI_NETBSD)
 #define SANITIZER_INTERCEPT_MCHECK_MPROBE SI_LINUX_NOT_ANDROID
 #define SANITIZER_INTERCEPT_WCSLEN 1

Copy link
Contributor

@wrotki wrotki left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@fmayer fmayer left a comment

Choose a reason for hiding this comment

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

Thanks for bearing with me

@cjappl
Copy link
Contributor Author

cjappl commented Oct 30, 2024

Thanks for bearing with me

Any time!! Thanks for the careful eye, would rather get it right 😄

Will submit when builds are green

@cjappl cjappl merged commit 4ccd2b0 into llvm:main Oct 31, 2024
7 checks passed
@cjappl cjappl deleted the reland_aligned_alloc branch October 31, 2024 14:52
smallp-o-p pushed a commit to smallp-o-p/llvm-project that referenced this pull request Nov 3, 2024
…ilable on the build machine" (llvm#114153)

This commit reverts commit 7c55426 (relands commit 97fb21a)

With the additional step of ignoring a warning that we don't care about. (-Wunguarded-availability-new)

> We know that aligned_alloc will never be called on systems that do not have aligned_alloc defined because the client OSX won't provide it. This function is actually guarded on OS's lower than 10.15 because aligned_alloc declaration won't exist on that version. There will be no way to call this function from code.
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…ilable on the build machine" (llvm#114153)

This commit reverts commit 7c55426 (relands commit 97fb21a)

With the additional step of ignoring a warning that we don't care about. (-Wunguarded-availability-new)

> We know that aligned_alloc will never be called on systems that do not have aligned_alloc defined because the client OSX won't provide it. This function is actually guarded on OS's lower than 10.15 because aligned_alloc declaration won't exist on that version. There will be no way to call this function from code.
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