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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.

Looks pretty good. I haven't looked closely at the annotation placements and tests yet.



================
Comment at: libcxx/include/deque:1537
+   // The following functions are no-ops outside of AddressSanitizer mode.
+   // We call annotatations only for the default Allocator because other allocators
+   // may not meet the AddressSanitizer alignment constraints.
----------------



================
Comment at: libcxx/include/deque:563
     deque() _NOEXCEPT_(is_nothrow_default_constructible<allocator_type>::value)
-        : __start_(0), __size_(0, __default_init_tag()) {}
+        : __start_(0), __size_(0, __default_init_tag()) {__annotate_new(0);}
 
----------------
Same below.


================
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
----------------
As in other patches, let's give the parameters proper names.


================
Comment at: libcxx/include/deque:944
+        const void* __old_b = (__mp == __old_c_beg_mp) ?
+          _VSTD::__to_address(*__mp + (__old_c_beg_index % __block_size)) : __mem_b;
+
----------------
throughout the patch


================
Comment at: libcxx/include/deque:1023
+  public:
+    bool __verify_asan_annotations() const _NOEXCEPT {
+            // This function tests deque object annotations.
----------------
Missing `_LIBCPP_HIDE_FROM_ABI`


================
Comment at: libcxx/include/deque:1028
+                   __it != __map_.end();
+                   ++__it)
+                if (!__sanitizer_verify_double_ended_contiguous_container(
----------------
Please add braces.


================
Comment at: libcxx/include/deque:1081
     _LIBCPP_HIDE_FROM_ABI
-    bool __maybe_remove_back_spare(bool __keep_one = true) {
-      if (__back_spare_blocks() >= 2 || (!__keep_one && __back_spare_blocks())) {
-        __alloc_traits::deallocate(__alloc(), __map_.back(),
-                                   __block_size);
-        __map_.pop_back();
-        return true;
-      }
-      return false;
+        bool __maybe_remove_front_spare(bool __keep_one = true) {
+            if (__front_spare_blocks() >= 2 || (!__keep_one && __front_spare_blocks())) {
----------------
Please don't add a 10th version of formatting. We are trying to reduce the formatting differences, not increase them.


================
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(),
----------------
You have specialized functions for there IIUC. Why don't you use them here? There are a few more cases below.


================
Comment at: libcxx/include/deque:2048
+          __map_.push_front(__alloc_traits::allocate(__a, __block_size));
+        }
         else
----------------
Please avoid NFC changes in such a large patch.


================
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()),
----------------
Maybe add a function `__annotate_whole_block`?


================
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]);
----------------
Maybe just call both in the same loop?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132092