[libc++][NFC] Remove a bunch of redundant ASan existence checks

[philnik777]

There are currently lots of _LIBCPP_HAS_ASAN and __libcpp_is_constant_evaluated() checks which aren't needed, since it is centrally checked inside __debug_utils/sanitizers.h.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

There are currently lots of _LIBCPP_HAS_ASAN and __libcpp_is_constant_evaluated() checks which aren't needed, since it is centrally checked inside __debug_utils/sanitizers.h.

---

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

3 Files Affected:

• (modified) libcxx/include/__vector/vector.h (-15)
• (modified) libcxx/include/string (+5-20)
• (modified) libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp (+1)

diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 8818eb7dfe26e..de15faf34c40b 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -716,47 +716,32 @@ class _LIBCPP_TEMPLATE_VIS vector {
   }
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __annotate_new(size_type __current_size) const _NOEXCEPT {
-    (void)__current_size;
-#if _LIBCPP_HAS_ASAN
     __annotate_contiguous_container(data() + capacity(), data() + __current_size);
-#endif
   }
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __annotate_delete() const _NOEXCEPT {
-#if _LIBCPP_HAS_ASAN
     __annotate_contiguous_container(data() + size(), data() + capacity());
-#endif
   }
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __annotate_increase(size_type __n) const _NOEXCEPT {
-    (void)__n;
-#if _LIBCPP_HAS_ASAN
     __annotate_contiguous_container(data() + size(), data() + size() + __n);
-#endif
   }
 
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __annotate_shrink(size_type __old_size) const _NOEXCEPT {
-    (void)__old_size;
-#if _LIBCPP_HAS_ASAN
     __annotate_contiguous_container(data() + __old_size, data() + size());
-#endif
   }
 
   struct _ConstructTransaction {
     _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI explicit _ConstructTransaction(vector& __v, size_type __n)
         : __v_(__v), __pos_(__v.__end_), __new_end_(__v.__end_ + __n) {
-#if _LIBCPP_HAS_ASAN
       __v_.__annotate_increase(__n);
-#endif
     }
 
     _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI ~_ConstructTransaction() {
       __v_.__end_ = __pos_;
-#if _LIBCPP_HAS_ASAN
       if (__pos_ != __new_end_) {
         __v_.__annotate_shrink(__new_end_ - __v_.__begin_);
       }
-#endif
     }
 
     vector& __v_;
diff --git a/libcxx/include/string b/libcxx/include/string
index fa87dc2fddb59..e1cda66038ec1 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -2153,7 +2153,7 @@ private:
   __annotate_contiguous_container(const void* __old_mid, const void* __new_mid) const {
     (void)__old_mid;
     (void)__new_mid;
-#  if _LIBCPP_HAS_ASAN && _LIBCPP_INSTRUMENTED_WITH_ASAN
+#  if _LIBCPP_INSTRUMENTED_WITH_ASAN
 #    if defined(__APPLE__)
     // TODO: remove after addressing issue #96099 (https://github.com/llvm/llvm-project/issues/96099)
     if (!__is_long())
@@ -2164,34 +2164,19 @@ private:
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __annotate_new(size_type __current_size) const _NOEXCEPT {
-    (void)__current_size;
-#  if _LIBCPP_HAS_ASAN && _LIBCPP_INSTRUMENTED_WITH_ASAN
-    if (!__libcpp_is_constant_evaluated())
-      __annotate_contiguous_container(data() + capacity() + 1, data() + __current_size + 1);
-#  endif
+    __annotate_contiguous_container(data() + capacity() + 1, data() + __current_size + 1);
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __annotate_delete() const _NOEXCEPT {
-#  if _LIBCPP_HAS_ASAN && _LIBCPP_INSTRUMENTED_WITH_ASAN
-    if (!__libcpp_is_constant_evaluated())
-      __annotate_contiguous_container(data() + size() + 1, data() + capacity() + 1);
-#  endif
+    __annotate_contiguous_container(data() + size() + 1, data() + capacity() + 1);
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __annotate_increase(size_type __n) const _NOEXCEPT {
-    (void)__n;
-#  if _LIBCPP_HAS_ASAN && _LIBCPP_INSTRUMENTED_WITH_ASAN
-    if (!__libcpp_is_constant_evaluated())
-      __annotate_contiguous_container(data() + size() + 1, data() + size() + 1 + __n);
-#  endif
+    __annotate_contiguous_container(data() + size() + 1, data() + size() + 1 + __n);
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __annotate_shrink(size_type __old_size) const _NOEXCEPT {
-    (void)__old_size;
-#  if _LIBCPP_HAS_ASAN && _LIBCPP_INSTRUMENTED_WITH_ASAN
-    if (!__libcpp_is_constant_evaluated())
-      __annotate_contiguous_container(data() + __old_size + 1, data() + size() + 1);
-#  endif
+    __annotate_contiguous_container(data() + __old_size + 1, data() + size() + 1);
   }
 
   // Disable ASan annotations and enable them again when going out of scope. It is assumed that the string is in a valid
diff --git a/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp b/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp
index 3d981e8b3a3cd..388b9ffd3b161 100644
--- a/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp
+++ b/libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/string.string_view.pass.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20, c++23
+// ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-steps): -fconstexpr-steps=9000000
 
 // <string>
 

[vitalybuka]

We have a file which took 23s to compile before, and now it's 3min.
And RSS 500Mb -> 15Gb

Both without Asan.

[vitalybuka]

So far I failed to create a reasonable reproduce.
Will try more tomorrow.

[ldionne]

Please let us know. I wouldn't be surprised if that was related to the number of constexpr operations we're doing before/after. If that's the case though, I don't think it makes sense to keep the #ifdef (i.e. revert) just for that because adding any other function calls inside string could in theory trigger the same issue. This probably points to something written very poorly in the user code?

[vitalybuka]

@ldionne @philnik777

static __gnu_cxx::hash_map<std::string, std::string> m;

void Test() {
  m["some text"] = "other text";
  // repeat ~4000 times
}

We compile that file with -fsanitize=undefined

It expands in frontend into function ~100000 basic blocs with calls to std::string members.

Before the patch we had no calls to:
__annotate_contiguous_container(const void* __old_mid, const void* __new_mid) const

After the patch we have the call, but it's not even empty, UBSAN creates =null check for "this", and this explodes "SROA::promoteAllocas".

[ldionne]

Yeah.. that's tricky. I'd argue that a (most likely generated?) function with 4000 assignment statements like this is a degenerate case. I mean something similar could happen with any other legitimate code transformation inside libc++.

I'd suggest rewriting the function as

std::pair<char const*, char const*> data = {{"some text", "other text"}, ...};
m.insert(begin(data), end(data));

or something similar.

[vitalybuka]

Yeah.. that's tricky. I'd argue that a (most likely generated?) function with 4000 assignment statements like this is a degenerate case. I mean something similar could happen with any other legitimate code transformation inside libc++.

I can see benefits of cleaner code, but given that it's a very common library, I also can see benefits of having sometimes less readable code, if it helps compiler.

Either way is fine to me.

I'd suggest rewriting the function as

std::pair<char const*, char const*> data = {{"some text", "other text"}, ...};
m.insert(begin(data), end(data));

Already done.

more like:

m = flat_map<std::string, std::string>({
      {"a", "b"},
       ...
  });

There also is a inefficiency in SROA, I am working on patches.

or something similar.