[vm] Fix DirectChainedHashMap iterator

BaseDirectChainedHashMap<KeyValueTrait, B, Allocator>::Iterator::Next()
was first checking if array_index_ is larger than the size of map's array,
and only then it was looking at remaining entries in the collision list.

So, when there is a collision in the last bucket, array_index_ was bumped
and the first entry in the list was returned. After that, Next() returned
NULL, skipping remaining entries in the collision list.

This caused incorrect stack state management in BytecodeFlowGraphBuilder
and BytecodeFlowGraphBuilder::DropUnusedValuesFromStack() was removing
too many entries from the stack, which caused crash in
BytecodeFlowGraphBuilder::BuildStoreIndexedTOS().

Issue: #38979
Change-Id: Ie073ca7014da5b04999b7984d508984b4c9743b8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/122176
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>

Author: alexmarkov

runtime/tests/vm/dart/regress38979_test.dart

@@ -0,0 +1,58 @@
+// Copyright (c) 2019, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// Verifies that compiler doesn't crash on a particular piece of code.
+
+import "package:expect/expect.dart";
+import 'dart:typed_data';
+
+Int8List var3 = Int8List(1);
+bool var14 = true;
+Duration var15 = Duration();
+int var16 = 22;
+String var18 = '';
+Map<bool, bool> var25 = {};
+Map<bool, int> var26 = {};
+Map<int, String> var30 = {};
+Map<String, bool> var31 = {};
+
+int foo1(Int8List par1, Map<bool, int> par2) {
+  throw 'err';
+}
+
+class X0 {
+  Int32x4 foo0_0(Duration par1, Set<bool> par2) {
+    if (var14) {
+      {
+        int loc0 = 0;
+        do {
+          var31 = {
+            (String.fromCharCode(((true ? true : true)
+                    ? (var14 ? 33 : var16)
+                    : (-(foo1(var3,
+                        {(false ? false : var14): (true ? 20 : var3[27])})))))):
+                ((((var16 > (-((-(loc0))))) ? Duration() : Duration()) +
+                        (var14 ? (true ? var15 : par1) : Duration())) <
+                    Duration()),
+            (((var30[(Int32x4.yzxw as int)]).toUpperCase()) +
+                (Uri.encodeComponent(('2' ??
+                    (var25[true]
+                        ? (String.fromEnvironment(''))
+                        : var18))))): (!(var14)),
+            (var31['MgOdzM']
+                    ? var30[15]
+                    : (('D9q6Ma').substring(
+                        (~((--var16))), foo1(var3, (false ? {} : var26))))):
+                (!(false)),
+          };
+        } while (++loc0 < 39);
+      }
+    }
+  }
+}
+
+main() {
+  Expect.throws(() => X0().foo0_0(Duration(), {true, false}),
+      (e) => e is NoSuchMethodError);
+}

runtime/vm/hash_map.h

@@ -188,32 +188,27 @@ BaseDirectChainedHashMap<KeyValueTrait, B, Allocator>::Iterator::Next() {
   const typename KeyValueTrait::Value kNoValue =
       KeyValueTrait::ValueOf(typename KeyValueTrait::Pair());
 
-  if (array_index_ < map_.array_size_) {
-    // If we're not in the middle of a list, find the next array slot.
-    if (list_index_ == kNil) {
-      while ((array_index_ < map_.array_size_) &&
-             KeyValueTrait::ValueOf(map_.array_[array_index_].kv) == kNoValue) {
-        array_index_++;
-      }
-      if (array_index_ < map_.array_size_) {
-        // When we're done with the list, we'll continue with the next array
-        // slot.
-        const intptr_t old_array_index = array_index_;
-        array_index_++;
-        list_index_ = map_.array_[old_array_index].next;
-        return &map_.array_[old_array_index].kv;
-      } else {
-        return NULL;
-      }
-    }
-
-    // Otherwise, return the current lists_ entry, advancing list_index_.
+  // Return the current lists_ entry (if any), advancing list_index_.
+  if (list_index_ != kNil) {
     intptr_t current = list_index_;
     list_index_ = map_.lists_[current].next;
     return &map_.lists_[current].kv;
   }
 
-  return NULL;
+  // When we're done with the list, we'll continue with the next array
+  // slot.
+  while ((array_index_ < map_.array_size_) &&
+         KeyValueTrait::ValueOf(map_.array_[array_index_].kv) == kNoValue) {
+    ++array_index_;
+  }
+  if (array_index_ < map_.array_size_) {
+    const intptr_t old_array_index = array_index_;
+    ++array_index_;
+    list_index_ = map_.array_[old_array_index].next;
+    return &map_.array_[old_array_index].kv;
+  }
+
+  return nullptr;
 }
 
 template <typename KeyValueTrait, typename B, typename Allocator>

runtime/vm/hash_map_test.cc

@@ -172,6 +172,40 @@ TEST_CASE(DirectChainedHashMapIterator) {
   EXPECT(sum == 15);
 }
 
+TEST_CASE(DirectChainedHashMapIteratorWithCollisionInLastBucket) {
+  intptr_t values[] = {65,  325, 329, 73,  396, 207, 215, 93,  227, 39,
+                       431, 112, 176, 313, 188, 317, 61,  127, 447};
+  IntMap<intptr_t> map;
+
+  for (intptr_t value : values) {
+    map.Insert(value, value);
+  }
+
+  bool visited[ARRAY_SIZE(values)] = {};
+  intptr_t count = 0;
+  auto it = map.GetIterator();
+  for (auto* p = it.Next(); p != nullptr; p = it.Next()) {
+    ++count;
+    bool found = false;
+    intptr_t i = 0;
+    for (intptr_t v : values) {
+      if (v == p->key) {
+        EXPECT(v == p->value);
+        EXPECT(!visited[i]);
+        visited[i] = true;
+        found = true;
+        break;
+      }
+      ++i;
+    }
+    EXPECT(found);
+  }
+  EXPECT(count == ARRAY_SIZE(values));
+  for (bool is_visited : visited) {
+    EXPECT(is_visited);
+  }
+}
+
 TEST_CASE(CStringMap) {
   const char* const kConst1 = "test";
   const char* const kConst2 = "test 2";