[libcxx-commits] [PATCH] D136765: [ASan][libcxx] Annotating std::vector with all allocators

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik accepted this revision.
philnik added a comment.

A couple small things, but nothing major. LGTM with nits addressed.



================
Comment at: libcxx/test/libcxx/containers/sequences/vector/asan.pass.cpp:32
 {
-#if TEST_STD_VER >= 11
-    {
-        typedef int T;
-        typedef std::vector<T, min_allocator<T>> C;
-        const T t[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
-        C c(std::begin(t), std::end(t));
-        c.reserve(2*c.size());
-        volatile T foo = c[c.size()];    // bad, but not caught by ASAN
-        ((void)foo);
-    }
+#if TEST_STD_VER >= 11 && TEST_CLANG_VER < 1600
+  {
----------------
Could you also add a TODO here?


================
Comment at: libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_iter_size_value.pass.cpp:100
     {
-        std::vector<int, min_allocator<int>> v(100);
-        std::vector<int, min_allocator<int>>::iterator i = v.insert(v.cbegin() + 10, 5, 1);
-        assert(v.size() == 105);
-        assert(is_contiguous_container_asan_correct(v));
-        assert(i == v.begin() + 10);
-        int j;
-        for (j = 0; j < 10; ++j)
-            assert(v[j] == 0);
-        for (; j < 15; ++j)
-            assert(v[j] == 1);
-        for (++j; j < 105; ++j)
-            assert(v[j] == 0);
+      std::vector<int, safe_allocator<int>> v(100);
+      std::vector<int, safe_allocator<int>>::iterator i = v.insert(v.cbegin() + 10, 5, 1);
----------------
Why are you replacing `min_allocator` here? Everywhere else you added a test.


================
Comment at: libcxx/test/std/containers/sequences/vector/vector.special/swap.pass.cpp:181
     }
+    // safe_allocator
+    {
----------------
Could you make the allocator a template parameter instead to avoid the duplication? The other cases are small enough that I don't really care, but would of course be a plus to deduplicate them as well.


================
Comment at: libcxx/test/support/min_allocator.h:447
+
+  TEST_CONSTEXPR_CXX20 T* allocate(std::size_t n) { return static_cast<T*>(std::allocator<T>().allocate(n + 1)) + 1; }
+
----------------
This `static_cast` shouldn't be necessary, right?


================
Comment at: libcxx/test/support/min_allocator.h:475
+    if (!std::__libcpp_is_constant_evaluated())
+      std::memset(p, 0, sizeof(T) * n);
+    std::allocator<T>().deallocate(p, n);
----------------
The compiler probably knows that after freeing the memory it's values can't be read, so this `memset` could probably get optimized away. Let's add a `DoNotOptimize(p)` just to be safe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136765