Code review from EricWF

This commit addresses some comments from a code review:
- add size check,
- add coments with context to a test,
- add comments explaining what happens in the test,
- remove volatile,
- split an if to constexpr if and normal if.

Author: Advenam Tacet

libcxx/include/string

@@ -1891,8 +1891,9 @@ private:
 #if !defined(_LIBCPP_HAS_NO_ASAN) && defined(_LIBCPP_INSTRUMENTED_WITH_ASAN)
     const void* __begin = data();
     const void* __end   = data() + capacity() + 1;
-    if (!__libcpp_is_constant_evaluated() && __asan_annotate_container_with_allocator<allocator_type>::value)
-      __sanitizer_annotate_contiguous_container(__begin, __end, __old_mid, __new_mid);
+    if _LIBCPP_CONSTEXPR_SINCE_CXX17 (__asan_annotate_container_with_allocator<allocator_type>::value)
+      if(!__libcpp_is_constant_evaluated())
+        __sanitizer_annotate_contiguous_container(__begin, __end, __old_mid, __new_mid);
 #endif
   }
 

libcxx/test/libcxx/containers/strings/basic.string/asan.pass.cpp

@@ -47,8 +47,8 @@ int main(int, char**) {
     assert(is_string_asan_correct(c));
     assert(__sanitizer_verify_contiguous_container(c.data(), c.data() + c.size() + 1, c.data() + c.capacity() + 1) !=
            0);
-    volatile T foo = c[c.size() + 1]; // should trigger ASAN. Use volatile to prevent being optimized away.
-    assert(false);                    // if we got here, ASAN didn't trigger
+    T foo = c[c.size() + 1]; // should trigger ASAN and call do_exit().
+    assert(false);           // if we got here, ASAN didn't trigger
     ((void)foo);
   }
 }

libcxx/test/libcxx/containers/strings/basic.string/asan_turning_off.pass.cpp

@@ -16,12 +16,26 @@
 // In those situations, one may want to deactivate annotations for a specific allocator.
 // It's possible with __asan_annotate_container_with_allocator template class.
 // This test confirms that those allocators work after turning off annotations.
+//
+// A context to this test is a situations when memory is repurposed and destructors are not called.
+//   Related issue: https://github.com/llvm/llvm-project/issues/60384
+//
+// That issue appeared in the past and was addressed here: https://reviews.llvm.org/D145628
+//
+// There is also a question, if it's UB. And it's not clear.
+//   Related discussion: https://reviews.llvm.org/D136765#4155262
+//   Related notes: https://eel.is/c++draft/basic.life#6
+//
+// Even if it is UB, we want to make sure that it works that way, because people rely on this behavior.
+// In similar situations. a user explicitly turns off annotations for a specific allocator.
 
 #include <assert.h>
 #include <stdlib.h>
 #include <string>
 #include <new>
 
+// Allocator with pre-allocated (with malloc in constructor) buffers.
+// Memory may be freed without calling destructors.
 struct reuse_allocator {
   static size_t const N = 100;
   reuse_allocator() {
@@ -50,10 +64,15 @@ struct user_allocator {
   friend bool operator==(user_allocator, user_allocator) { return true; }
   friend bool operator!=(user_allocator x, user_allocator y) { return !(x == y); }
 
-  T* allocate(size_t) { return (T*)reuse_buffers.alloc(); }
+  T* allocate(size_t n) {
+    if (n * sizeof(T) > 8 * 1024)
+      throw std::bad_array_new_length();
+    return (T*)reuse_buffers.alloc();
+  }
   void deallocate(T*, size_t) noexcept {}
 };
 
+// Turn off annotations for user_allocator:
 template <class T>
 struct std::__asan_annotate_container_with_allocator<user_allocator<T>> {
   static bool const value = false;
@@ -63,15 +82,20 @@ int main() {
   using S = std::basic_string<char, std::char_traits<char>, user_allocator<char>>;
 
   {
+    // Create a string with a buffer from reuse allocator object:
     S* s = new (reuse_buffers.alloc()) S();
-    for (int i = 0; i < 100; i++)
+    // Use string, so it's poisoned, if container annotations for that allocator are not turned off:
+    for (int i = 0; i < 40; i++)
       s->push_back('a');
   }
+  // Reset the state of the allocator, don't call destructors, allow memory to be reused:
   reuse_buffers.reset();
   {
+    // Create a next string with the same allocator, so the same buffer due to the reset:
     S s;
-    for (int i = 0; i < 1000; i++)
-      s.push_back('b');
+    // Use memory inside the string again, if it's poisoned, an error will be raised:
+    for (int i = 0; i < 60; i++)
+      s.push_back('a');
   }
 
   return 0;