[libcxx-commits] [PATCH] D132092: [2a/3][ASan][libcxx] std::deque annotations

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/deque:872
+// We call annotations only for the default Allocator.
+#if !defined(_LIBCPP_HAS_NO_ASAN) && _LIBCPP_CLANG_VER >= 1600
+    _LIBCPP_HIDE_FROM_ABI void __annotate_double_ended_contiguous_container(
----------------
Let's add a LLVM 18 TODO to clean this up.


================
Comment at: libcxx/include/deque:890
+    _LIBCPP_HIDE_FROM_ABI
+    void __annotate_from_to(size_type __beg, size_type __end, bool __poisoning, bool __front) const _NOEXCEPT {
+        // __beg - index of the first item to annotate
----------------
Maybe having two enums like
```lang=c++
enum {
  __asan_poinson,
  __asan_unposion
};

enum {
  __asan_front_moved,
  __asan_back_moved,
};
```

would make this more readable? (and remove the need for the comments below)


================
Comment at: libcxx/include/deque:897
+        if (__beg == __end)
+            // Nothing to do here.
+            return;
----------------
I think it's pretty clear what a `return;` does.


================
Comment at: libcxx/include/deque:902
+        // NOTE: if __end % __block_size == 0, __annotations_end_mp points at the next block, which may not exist
+        typename __map::const_iterator __annotations_beg_mp = __map_.begin() + __beg / __block_size;
+        typename __map::const_iterator __annotations_end_mp = __map_.begin() + __end / __block_size;
----------------
Maybe use `__map_const_pointer`?


================
Comment at: libcxx/include/deque:903
+        typename __map::const_iterator __annotations_beg_mp = __map_.begin() + __beg / __block_size;
+        typename __map::const_iterator __annotations_end_mp = __map_.begin() + __end / __block_size;
+
----------------
What does the `mp` stand for?


================
Comment at: libcxx/include/deque:993-997
+        if (!empty()) {
+            __annotate_from_to(0, __start_, false, true);
+            __annotate_from_to(__start_ + size(), __map_.size() * __block_size, false, false);
+        } else
+            __annotate_from_to(0, __map_.size() * __block_size, false, false);
----------------
I would reverse the condition here to match `__annotate_new`.


================
Comment at: libcxx/include/deque:2319
                 __buf.push_back(__alloc_traits::allocate(__a, __block_size));
+                // ASan: this is empty container, we have to poison whole
+                __annotate_poison_block(
----------------



================
Comment at: libcxx/include/deque:2150-2158
+                // ASan: this is empty container, we have to poison whole
+                __annotate_double_ended_contiguous_container(
+                    _VSTD::__to_address(__buf.back()),
+                    _VSTD::__to_address(__buf.back() + __block_size),
+                    _VSTD::__to_address(__buf.back()),
+                    _VSTD::__to_address(__buf.back() + __block_size),
+                    _VSTD::__to_address(__buf.back()),
----------------
AdvenamTacet wrote:
> philnik wrote:
> > Maybe add a function `__annotate_whole_block`?
> I added `__annotate_poison_block` for this. It's not very similar to other helpers, because here we are annotating a memory block in `__buf` and not in `__map_`.
Ah. Makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132092