Change IdentityMap::Delete signature

The existing signature is problematic for two reasons:

1. The void* -> V cast is invalid if sizeof(V) < sizeof(void*)
2. It's impossible to distinguish between a returned value of 0 and
   nullptr, designating failure.

Bug: v8:6666
Change-Id: I71e8fc9119256c24a15b5bb73438f024f1af4f88
Reviewed-on: https://chromium-review.googlesource.com/1018466Reviewed-by: Peter Marshall <petermarshall@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52708}

src/compiler-dispatcher/compiler-dispatcher.cc

@@ -709,8 +709,8 @@ CompilerDispatcher::JobMap::const_iterator CompilerDispatcher::RemoveJob(
     Handle<SharedFunctionInfo> shared =
         job->AsUnoptimizedCompileJob()->shared();
     if (!shared.is_null()) {
-      JobId deleted_id = shared_to_unoptimized_job_id_.Delete(shared);
-      USE(deleted_id);
+      JobId deleted_id;
+      shared_to_unoptimized_job_id_.Delete(shared, &deleted_id);
       DCHECK_EQ(it->first, deleted_id);
     }
   }

src/identity-map.cc

@@ -78,8 +78,8 @@ int IdentityMapBase::InsertKey(Object* address) {
   UNREACHABLE();
 }
 
-void* IdentityMapBase::DeleteIndex(int index) {
-  void* ret_value = values_[index];
+bool IdentityMapBase::DeleteIndex(int index, void** deleted_value) {
+  if (deleted_value != nullptr) *deleted_value = values_[index];
   Object* not_mapped = heap_->not_mapped_symbol();
   DCHECK_NE(keys_[index], not_mapped);
   keys_[index] = not_mapped;
@@ -90,7 +90,7 @@ void* IdentityMapBase::DeleteIndex(int index) {
   if (capacity_ > kInitialIdentityMapSize &&
       size_ * kResizeFactor < capacity_ / kResizeFactor) {
     Resize(capacity_ / kResizeFactor);
-    return ret_value;  // No need to fix collisions as resize reinserts keys.
+    return true;  // No need to fix collisions as resize reinserts keys.
   }
 
   // Move any collisions to their new correct location.
@@ -115,7 +115,7 @@ void* IdentityMapBase::DeleteIndex(int index) {
     index = next_index;
   }
 
-  return ret_value;
+  return true;
 }
 
 int IdentityMapBase::Lookup(Object* key) const {
@@ -184,15 +184,14 @@ IdentityMapBase::RawEntry IdentityMapBase::FindEntry(Object* key) const {
 }
 
 // Deletes the given key from the map using the object's address as the
-// identity, returning:
-//    found => the value
-//    not found => {nullptr}
-void* IdentityMapBase::DeleteEntry(Object* key) {
+// identity, returning true iff the key was found (in which case, the value
+// argument will be set to the deleted entry's value).
+bool IdentityMapBase::DeleteEntry(Object* key, void** deleted_value) {
   CHECK(!is_iterable());  // Don't allow deletion by key while iterable.
-  if (size_ == 0) return nullptr;
+  if (size_ == 0) return false;
   int index = Lookup(key);
-  if (index < 0) return nullptr;  // No entry found.
-  return DeleteIndex(index);
+  if (index < 0) return false;  // No entry found.
+  return DeleteIndex(index, deleted_value);
 }
 
 Object* IdentityMapBase::KeyAtIndex(int index) const {

src/identity-map.h

@@ -43,7 +43,7 @@ class IdentityMapBase {
 
   RawEntry GetEntry(Object* key);
   RawEntry FindEntry(Object* key) const;
-  void* DeleteEntry(Object* key);
+  bool DeleteEntry(Object* key, void** deleted_value);
   void Clear();
 
   Object* KeyAtIndex(int index) const;
@@ -63,7 +63,7 @@ class IdentityMapBase {
   int InsertKey(Object* address);
   int Lookup(Object* key) const;
   int LookupOrInsert(Object* key);
-  void* DeleteIndex(int index);
+  bool DeleteIndex(int index, void** deleted_value);
   void Rehash();
   void Resize(int new_capacity);
   int Hash(Object* address) const;
@@ -113,8 +113,17 @@ class IdentityMap : public IdentityMapBase {
   void Set(Handle<Object> key, V v) { Set(*key, v); }
   void Set(Object* key, V v) { *(reinterpret_cast<V*>(GetEntry(key))) = v; }
 
-  V Delete(Handle<Object> key) { return Delete(*key); }
-  V Delete(Object* key) { return reinterpret_cast<V>(DeleteEntry(key)); }
+  bool Delete(Handle<Object> key, V* deleted_value) {
+    return Delete(*key, deleted_value);
+  }
+  bool Delete(Object* key, V* deleted_value) {
+    void* v = nullptr;
+    bool deleted_something = DeleteEntry(key, &v);
+    if (deleted_value != nullptr && deleted_something) {
+      *deleted_value = (V) reinterpret_cast<intptr_t>(v);
+    }
+    return deleted_something;
+  }
 
   // Removes all elements from the map.
   void Clear() { IdentityMapBase::Clear(); }

test/cctest/test-identity-map.cc

@@ -98,7 +98,8 @@ class IdentityMapTester : public HandleAndZoneScope {
     }
 
     // Delete {key1}
-    void* deleted_entry_1 = map.Delete(key1);
+    void* deleted_entry_1;
+    CHECK(map.Delete(key1, &deleted_entry_1));
     CHECK_NOT_NULL(deleted_entry_1);
     deleted_entry_1 = val1;
 
@@ -114,7 +115,8 @@ class IdentityMapTester : public HandleAndZoneScope {
     }
 
     // Delete {key2}
-    void* deleted_entry_2 = map.Delete(key2);
+    void* deleted_entry_2;
+    CHECK(map.Delete(key2, &deleted_entry_2));
     CHECK_NOT_NULL(deleted_entry_2);
     deleted_entry_2 = val2;
 
@@ -160,7 +162,8 @@ class IdentityMapTester : public HandleAndZoneScope {
   }
 
   void CheckDelete(Handle<Object> key, void* value) {
-    void* entry = map.Delete(key);
+    void* entry;
+    CHECK(map.Delete(key, &entry));
     CHECK_NOT_NULL(entry);
     CHECK_EQ(value, entry);
   }
@@ -197,14 +200,18 @@ TEST(Find_num_not_found) {
 TEST(Delete_smi_not_found) {
   IdentityMapTester t;
   for (int i = 0; i < 100; i++) {
-    CHECK_NULL(t.map.Delete(t.smi(i)));
+    void* deleted_value = &t;
+    CHECK(!t.map.Delete(t.smi(i), &deleted_value));
+    CHECK_EQ(&t, deleted_value);
   }
 }
 
 TEST(Delete_num_not_found) {
   IdentityMapTester t;
   for (int i = 0; i < 100; i++) {
-    CHECK_NULL(t.map.Delete(t.num(i + 0.2)));
+    void* deleted_value = &t;
+    CHECK(!t.map.Delete(t.num(i + 0.2), &deleted_value));
+    CHECK_EQ(&t, deleted_value);
   }
 }
 
@@ -311,7 +318,8 @@ TEST(Delete_num_1000) {
 
   // Delete every second value in reverse.
   for (int i = 999; i >= 0; i -= 2) {
-    void* entry = t.map.Delete(t.smi(i * kPrime));
+    void* entry;
+    CHECK(t.map.Delete(t.smi(i * kPrime), &entry));
     CHECK_EQ(reinterpret_cast<void*>(i * kPrime), entry);
   }
 
@@ -327,7 +335,8 @@ TEST(Delete_num_1000) {
 
   // Delete the rest.
   for (int i = 0; i < 1000; i += 2) {
-    void* entry = t.map.Delete(t.smi(i * kPrime));
+    void* entry;
+    CHECK(t.map.Delete(t.smi(i * kPrime), &entry));
     CHECK_EQ(reinterpret_cast<void*>(i * kPrime), entry);
   }