[libcxx-commits] [PATCH] D146214: [ASan][libc++] Annotating std::basic_string with all allocators

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

Has this patch been tested against Chromium? Has it been tested elsewhere?
The more testing it's undergone, the more confident I can be.



================
Comment at: libcxx/include/string:1845
         const void* __end = data() + capacity() + 1;
+        #  if _LIBCPP_CLANG_VER < 1600
         if (!__libcpp_is_constant_evaluated() && __begin != nullptr && is_same<allocator_type, __default_allocator_type>::value)
----------------
conditional compilation blocks are the source of a lot of bugs.

Instead of an `#if` and `#else` block here. Why not make it so that `__asan_annonate_container_with_allocator` only returns true when the other conditions `_LIBCPP_CLANG_VER > 1600` etc.

Then you could simply write the condition
```
if (!__libcpp_is_constant_evaluated() && __begin != nullptr &&  __asan_annotate_container_with_allocator<allocator_type, __default_allocator_type>)
```

Where `__asan_annotate_container_allocator_type` can return true in the case that the allocator is the default.




================
Comment at: libcxx/test/libcxx/containers/strings/no_asan.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
It's not clear to me what this test is supposed to do.

Does the name mean it should only run without address sanitizer?


================
Comment at: libcxx/test/support/asan_testing.h:54
   if (c.data() != NULL) {
 #if TEST_CLANG_VER < 16000 || defined(_LIBCPP_ASAN_ANNOTATE_ONLY_LONG)
     // TODO LLVM18: remove special case
----------------
Again, I would much rather see zero code inside this conditional compilation block.

If need-be, please write something sorta like this:

```
#if TEST_CLANG_VER < 16000 || defined(_LIBCPP_ASAN_ANNOTATE_ONLY_LONG)
# define SHORT_STRING_ALLOWED 0
#else
# define SHORT_STRING_ALLOWED 1
#endif

if (SHORT_STRING_ALLOWED || !is_short_string(s))



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146214/new/

https://reviews.llvm.org/D146214