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

[Tacet via Phabricator via libcxx-commits]

AdvenamTacet added inline comments.


================
Comment at: libcxx/include/deque:888
     _LIBCPP_HIDE_FROM_ABI
-    bool __maybe_remove_front_spare(bool __keep_one = true) {
-      if (__front_spare_blocks() >= 2 || (!__keep_one && __front_spare_blocks())) {
-        __alloc_traits::deallocate(__alloc(), __map_.front(),
-                                   __block_size);
-        __map_.pop_front();
-        __start_ -= __block_size;
-        return true;
+    void __annotate_from_to(size_type __b, size_type __e, bool __poisoning, bool __front) const _NOEXCEPT {
+      // __b - index of the first item to annotate
----------------
philnik wrote:
> As in other patches, let's give the parameters proper names.
I changed `__b` and `__e` to `__beg` and `__end`. I believe same length makes the code easier to read. Updated also local variable names.


================
Comment at: libcxx/include/deque:1083
+            if (__front_spare_blocks() >= 2 || (!__keep_one && __front_spare_blocks())) {
+              __annotate_from_to(0, __block_size, false, false);
+              __alloc_traits::deallocate(__alloc(), __map_.front(),
----------------
philnik wrote:
> You have specialized functions for there IIUC. Why don't you use them here? There are a few more cases below.
There was no helper for a single block and use of `__annotate_shrink_*` is impossible as it requires annotated memory area to be just before or just after the container in-use area, but new block may be added not directly before/after. I added `__annotate_whole_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()),
----------------
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_`.


================
Comment at: libcxx/test/std/containers/sequences/deque/deque.capacity/resize_size.pass.cpp:92-99
+    {
+    int rng[] = {0, 1, 2, 3, 1023, 1024, 1025, 2047, 2048, 2049};
+    const int N = sizeof(rng)/sizeof(rng[0]);
+    for (int i = 0; i < N; ++i)
+        for (int j = 0; j < N; ++j)
+            for (int k = 0; k < N; ++k)
+                testN<std::deque<int, safe_allocator<int>>>(rng[i], rng[j], rng[k]);
----------------
philnik wrote:
> Maybe just call both in the same loop?
I started doing it, but stopped. I don't think it's a good idea to introduce that change in this patch. There is already a lot of copy-paste of tests, so it would require to refactor all of them or have two different styles in one file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132092