[PATCH] D20746: [sanitizer] Add iterator to AddrHashMap

[Dmitry Vyukov via llvm-commits]

dvyukov added inline comments.

================
Comment at: lib/sanitizer_common/sanitizer_addrhashmap.h:91
@@ -90,1 +90,3 @@
 
+  template<typename U>
+  struct HashValue {
----------------
remove the template, and just use T*
simpler and less typing

================
Comment at: lib/sanitizer_common/sanitizer_addrhashmap.h:92
@@ +91,3 @@
+  template<typename U>
+  struct HashValue {
+    HashValue<U>(uptr a, U v) : addr(a), value(v) {}
----------------
s/HashValue/Value/

It's already in the scope of Addr_Hash_Map.

================
Comment at: lib/sanitizer_common/sanitizer_addrhashmap.h:101
@@ +100,3 @@
+    Iterator(AddrHashMap<T, kSize>* map) :
+      map_(map), bucket_idx_(0), embed_idx_(0), add_idx_(0) {
+      if (operator*().addr == 0)
----------------
please move all the code out of the class declaration to the bottom of the file

you can see that it is done for all member functions here, I wanted the declaration to remain at least minimally readable (to the point it is possible with C++)

================
Comment at: lib/sanitizer_common/sanitizer_addrhashmap.h:105
@@ +104,3 @@
+    }
+    Iterator(const Iterator& src) = default;
+    Iterator& operator=(const Iterator& src) = default;
----------------
this won't work: iterator can hold mutex lock

if you copy the iterator, you will double unlock the mutex and corrupt its state

do you plan to use them? if not, = delete


================
Comment at: lib/sanitizer_common/sanitizer_addrhashmap.h:124
@@ +123,3 @@
+      }
+      uptr addr = atomic_load(&c->addr, memory_order_relaxed);
+      return HashValue<T*>(addr, &c->val);
----------------
this must be acquire to pair with release in the release function

but unfortunately that's not enough: elements can be concurrently removed and overwritten
that's ok for normal usages because lookup for an element should not race with its removal, embed elements are not compacted, and accesses to add elements are protected with read lock

I think the simplest fix would be to read lock the cell whenever you access it (both embed and add elements).

================
Comment at: lib/sanitizer_common/sanitizer_addrhashmap.h:136
@@ +135,3 @@
+          // We must hold the read lock across the iteration.
+          b->mtx.ReadLock();
+          AddBucket *add = (AddBucket*)atomic_load(&b->add,
----------------
vitalybuka wrote:
> Iterating concurrent hash map looks scary in general, and not very efficient.
> This will hold bucket locked entire iterator lifetime.
> Maybe safer to consider some alternatives, like exporting entire table as array.
> So you can get array under the log, and do whatever you want after release.
> 
>  
unless I am missing something, this can double-lock the mutex if a cell contains more than 1 add element

need more tests to catch it

================
Comment at: lib/sanitizer_common/tests/sanitizer_addrhashmap_test.cc:1
@@ +1,2 @@
+//===-- sanitizer_bitvector_test.cc ---------------------------------------===//
+//
----------------
s/bitvector/addrhashmap/

================
Comment at: lib/sanitizer_common/tests/sanitizer_addrhashmap_test.cc:11
@@ +10,3 @@
+// This file is a part of Sanitizer runtime.
+// Tests for sanitizer_bitvector.h.
+//
----------------
s/bitvector/addrhashmap/


http://reviews.llvm.org/D20746