[libc++][libc++abi] Don't provide new/delete when built with ASan, HWASan or TSan

[phosek]

Summary

ASan, HWASan and TSan provide their own operator new/delete to intercept
all allocations and deallocations. Currently, they rely on overriding
weak symbols that are already provided by libc++abi and libc++, but that
solution is fragile and breaks in certain scenarios, e.g. when libc++abi
is statically linked into a shared libc++.

Rather we should avoid providing new/delete operators when built with
ASan, HWASan and TSan altogether and rely on new/delete operators
provided by the sanitizer runtimes.

[mcgrathr]

lgtm

libcxxabi/src/stdlib_new_delete.cpp

268

Comment the conditions

[eugenis]

LGTM

[ldionne]

The right thing to do seems to be to provide -DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=OFF when you configure libc++.

[ldionne]

In D60176#1454062, @ldionne wrote:

The right thing to do seems to be to provide -DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=OFF when you configure libc++.

What I mean here is that I don't see the justification for hardcoding this into the sources. Similarly, I find it wrong for this to be dependent on _LIBCPP_ABI_VCRUNTIME and __GLIBCXX__.

[phosek]

In D60176#1454066, @ldionne wrote:

In D60176#1454062, @ldionne wrote:

The right thing to do seems to be to provide -DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=OFF when you configure libc++.

What I mean here is that I don't see the justification for hardcoding this into the sources. Similarly, I find it wrong for this to be dependent on _LIBCPP_ABI_VCRUNTIME and __GLIBCXX__.

I've changed the implementation to make those independent. D60235 is the alternative that relies entirely on CMake and only disables those for Fuchsia. IMO the problem with that solution is that anyone who builds libc++/libc++abi will need to apply the same solution. I was also thinking about changing the CMake option default, but there's no easy way to detect whether the sanitizer is being enabled e.g. through CFLAGS/LDFLAGS.

[mclow.lists]

I think we're way, way out on the bell curve here.
People who are building their own libc++ and statically linking it with libc++abi and using the sanitizers.
Those people (I believe) will be able to set a CMake flag.

[eugenis]

I think I prefer the cmake flag, too.
It's not unthinkable that hwasan might stop providing operator new and friends in the future. I was a bit surprised that we even do it now in COMPILER_RT_HWASAN_WITH_INTERCEPTORS=OFF configuration - but it looks like we are keeping it.
Anyway, lets not hardcode this decision.

[phosek]

SGTM

libcxx/src/new.cpp

 #endif // !LIBSTDCXX
 
 }  // std
 
 #if !defined(__GLIBCXX__) &&                                                   \
     !defined(_LIBCPP_ABI_VCRUNTIME) &&      \
     !defined(_LIBCPP_DISABLE_NEW_DELETE_DEFINITIONS)
 
+// Sanitizer runtimes provide their own definitions of new and delete operators.
+#if !__has_feature(address_sanitizer) && \
+    !__has_feature(hwaddress_sanitizer) && \
+    !__has_feature(thread_sanitizer)
+
 // Implement all new and delete operators as weak definitions
 // in this shared library, so that they can be overridden by programs
 // that define non-weak copies of the functions.
 
 _LIBCPP_WEAK
 void *
 operator new(std::size_t size) _THROW_BAD_ALLOC
 {

 _LIBCPP_WEAK
 void
 operator delete[] (void* ptr, size_t, std::align_val_t alignment) _NOEXCEPT
 {
     ::operator delete[](ptr, alignment);
 }
 
 #endif // !_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION
+#endif // !address_sanitizer && !hwaddress_sanitizer && !thread_sanitizer
 #endif // !__GLIBCXX__ && !_LIBCPP_ABI_VCRUNTIME && !_LIBCPP_DISABLE_NEW_DELETE_DEFINITIONS

libcxxabi/src/stdlib_new_delete.cpp

 #include "__cxxabi_config.h"
 #include <new>
 #include <cstdlib>
 
 #if !defined(_THROW_BAD_ALLOC) || !defined(_NOEXCEPT) || !defined(_LIBCXXABI_WEAK)
 #error The _THROW_BAD_ALLOC, _NOEXCEPT, and _LIBCXXABI_WEAK libc++ macros must \
        already be defined by libc++.
 #endif
+
+// Sanitizer runtimes provide their own definitions of new and delete operators.
+#if !__has_feature(address_sanitizer) && \
+    !__has_feature(hwaddress_sanitizer) && \
+    !__has_feature(thread_sanitizer)
+
 // Implement all new and delete operators as weak definitions
 // in this shared library, so that they can be overridden by programs
 // that define non-weak copies of the functions.
 
 _LIBCXXABI_WEAK
 void *
 operator new(std::size_t size) _THROW_BAD_ALLOC
 {

 
 _LIBCXXABI_WEAK
 void
 operator delete[] (void* ptr, size_t, std::align_val_t alignment) _NOEXCEPT
 {
     ::operator delete[](ptr, alignment);
 }
 
+#endif // !address_sanitizer && !hwaddress_sanitizer && !thread_sanitizer
 #endif // !_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION