VM: Fix memory leak during shutdown

runtime/platform/hashmap.cc

Index: runtime/platform/hashmap.cc
diff --git a/runtime/platform/hashmap.cc b/runtime/platform/hashmap.cc
index 461153398129a270433f45682ffd8f0765885a91..999e511e145d4ad5e83fb95b4c4289829594db5a 100644
--- a/runtime/platform/hashmap.cc
+++ b/runtime/platform/hashmap.cc
@@ -107,10 +107,32 @@ void HashMap::Remove(void* key, uint32_t hash) {
   // Clear the candidate which will not break searching the hash table.
   candidate->key = NULL;
   candidate->value = NULL;
+  candidate->hash = 0;

[Florian Schneider, 2016/11/30 18:08:41]

Why clear the hash code?

[kustermann, 2016/11/30 21:27:15]

For consistency:

The [key] *needs* to be cleared because a NULL key serves as the "free spot"
marker.

The [value] and [hash] don't need to be cleared. But since [value] is cleared,
why not [hash] as well? We could avoid the work and not clear any of
[value]/[hash] (though it's a little weird in the debugger if one examples the
hashmap entries and sees left-over/garbage values in there)

(I thought maybe the reason is ASAN, but then again, [hash] could be the 32-bit
lower bits of [key], so [hash] can contain a valid pointer as well.)

Removed the two assignments now.

[Florian Schneider, 2016/11/30 21:51:48]

Agree, on the other hand, it may be useful to examine historical entries in the
hashmap.

Sounds good to remove them - performance should be better as well this way.

   occupancy_--;
 }
 
 
+HashMap::Entry* HashMap::Remove(Entry* entry) {
+  // Save the key of the entry we would like to remove, since the removal itself
+  // might do a left-rotation of elements following this one, thereby overriding
+  // `entry`.
+  void* key = entry->key;

[Florian Schneider, 2016/11/30 18:08:41]

Where is the saved key used after calling Remove?

[kustermann, 2016/11/30 21:27:15]

Sorry that was a left-over. The indication whether removal made a free spot is
whether 'key == NULL' if it's not the case, a left rotation has happened.

+  Remove(key, entry->hash);
+
+  // A key can only exist once in the map and we just removed `key`. This means
+  // that either a left-rotation has happened (in which case `entry` points
+  // already to the next element (in terms of iteration order)) or alternatively
+  // we can use the normal `Next()` call to move in iteration order.
+  if (entry->key != NULL) {
+    // A left-rotation happened. `entry` points already to the next element in
+    // iteration order.
+    return entry;
+  } else {
+    return Next(entry);
+  }
+}
+
+
 void HashMap::Clear(ClearFun clear) {
   // Mark all entries as empty.
   const Entry* end = map_end();
@@ -120,6 +142,7 @@ void HashMap::Clear(ClearFun clear) {
     }
     p->value = NULL;
     p->key = NULL;
+    p->hash = 0;

[Florian Schneider, 2016/11/30 18:08:41]

Same here. Seems wasted work.

[kustermann, 2016/11/30 21:27:15]

Same answer.

   }
   occupancy_ = 0;
 }