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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/vector:747
     {
-
+#if  _LIBCPP_CLANG_VER >= 16000
+      if (!__libcpp_is_constant_evaluated() && __beg)
----------------
AdvenamTacet wrote:
> EricWF wrote:
> > philnik wrote:
> > > Could you explain here why this is possible in LLVM16, but not before?
> > Seconded.
> > 
> > Also, I really don't like having the condition be conditionally compiled. It smells weird, and it looks weird, and I think it'll lead to bugs.
> > 
> > Can you find another way to write this?
> Vectors annotations rely on  `__sanitizer_annotate_contiguous_container`, its implementation from LLVM15 (and earlier) requires that beginning of a containers data buffer is aligned to shadow granularity. Now, in effort to extend ASans capabilities, implementation of the function changed with commit rGdd1b7b797a116eed588fd752fbe61d34deeb24e4 and supports:
> - unaligned beginnings of buffers,
> - shared first/last granule (if memory granule is shared with a different object just //after the end of unaligned end/before the unaligned beginning//, memory of that object won't be poisoned).
> Therefore, check for standard allocator is not necessary.
> 
> I don't know how to change the condition, requirements for the functions (`__sanitizer_annotate_contiguous_container`) arguments were weakened now and if older LLVM versions are supported, that check has to be still present for them. There are no new variables, just implementation changed. @EricWF Do you have an idea how it may be done in a different way?
Please make this a comment in the code. Maybe also a `// TODO LLVM18: Remove the special-casing`, so we don't forget removing it.


================
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 && _LIBCPP_CLANG_VER < 1600
+  {
----------------
Let's use `TEST_CLANG_VER` instead.


================
Comment at: libcxx/test/support/min_allocator.h:445
+
+  TEST_CONSTEXPR_CXX20 T* allocate(std::size_t n) { return static_cast<T*>(std::allocator<T>().allocate(n + 1)) + 1; }
+
----------------
This is UB in general. Please add a `static_assert(alignof(T) == 1)` to make sure nobody tries to instantiate it with a type where this produces UB.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136765