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

[Vitaly Buka via llvm-commits]

vitalybuka requested changes to this revision.
vitalybuka added a reviewer: vitalybuka.
This revision now requires changes to proceed.

================
Comment at: lib/sanitizer_common/sanitizer_addrhashmap.h:113
@@ +112,3 @@
+    }
+    HashValue<T*> operator*() {
+      Bucket *b = &map_->table_[bucket_idx_];
----------------
I think logic could be simpler if Iterator contained temp copy of HashValue<T*>
and  operator* returns reference to it.

E.g.:
class Iterator {

  const HashValue<T*>& operator*() {
    return curr_item_;
  }

   Iterator& operator++() {
      // all logic here,
      curr_item_ = something.
    }

   HashValue<T*> curr_item_;
}

================
Comment at: lib/sanitizer_common/sanitizer_addrhashmap.h:121
@@ +120,3 @@
+        if (add == nullptr)
+          return HashValue<T*>(0, nullptr);
+        c = &add->cells[add_idx_];
----------------
add == nullptr should not be possible according line 139

================
Comment at: lib/sanitizer_common/sanitizer_addrhashmap.h:130
@@ +129,3 @@
+        if (bucket_idx_ == kSize)
+          return *this;
+        if (embed_idx_ < kBucketSize) {
----------------
This will hide bugs. We should crash here if someone increments end().

================
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,
----------------
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.

 


http://reviews.llvm.org/D20746