[vm, reload] Handle constant rehashing / enum migration not necessarily running bottom up.

Re-evaluating enum constants from kernel may create new const instances of any type via enhanced enum fields, which conflicts with retaining the canonical bit on old const instances.

TEST=manually with Flutter example from bug
TEST=vm/cc/IsolateReload_EnumWithSet
Bug: https://github.com/dart-lang/sdk/issues/55350
Change-Id: I0ec55d66ec6bb1d6512c4d9bedc6a4244f505c29
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/361983
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>

runtime/vm/isolate.cc

@@ -898,6 +898,25 @@ void IsolateGroup::RehashConstants(Become* become) {
   Instance& old_value = Instance::Handle(zone);
   Instance& new_value = Instance::Handle(zone);
   Instance& deleted = Instance::Handle(zone);
+
+  if (become != nullptr) {
+    for (intptr_t cid = kInstanceCid; cid < num_cids; cid++) {
+      Array* old_constants = old_constant_tables[cid];
+      if (old_constants == nullptr) continue;
+
+      cls = class_table()->At(cid);
+      CanonicalInstancesSet set(zone, old_constants->ptr());
+      CanonicalInstancesSet::Iterator it(&set);
+      while (it.MoveNext()) {
+        constant ^= set.GetKey(it.Current());
+        ASSERT(!constant.IsNull());
+        ASSERT(!constant.InVMIsolateHeap());
+        constant.ClearCanonical();
+      }
+      set.Release();
+    }
+  }
+
   for (intptr_t cid = kInstanceCid; cid < num_cids; cid++) {
     Array* old_constants = old_constant_tables[cid];
     if (old_constants == nullptr) continue;
@@ -954,12 +973,18 @@ void IsolateGroup::RehashConstants(Become* become) {
       }
     } else {
       while (it.MoveNext()) {
-        constant ^= set.GetKey(it.Current());
-        ASSERT(!constant.IsNull());
-        // Shape changes lose the canonical bit because they may result/ in
-        // merging constants. E.g., [x1, y1], [x1, y2] -> [x1].
-        DEBUG_ASSERT(constant.IsCanonical() || HasAttemptedReload());
-        cls.InsertCanonicalConstant(zone, constant);
+        old_value ^= set.GetKey(it.Current());
+        ASSERT(!old_value.IsNull());
+
+        if (become == nullptr) {
+          ASSERT(old_value.IsCanonical());
+          cls.InsertCanonicalConstant(zone, old_value);
+        } else {
+          new_value = old_value.Canonicalize(thread);
+          if (old_value.ptr() != new_value.ptr()) {
+            become->Add(old_value, new_value);
+          }
+        }
       }
     }
     set.Release();

runtime/vm/isolate_reload_test.cc

@@ -6454,6 +6454,79 @@ TEST_CASE(IsolateReload_KeepPragma1) {
                               Symbols::vm_prefer_inline()));
 }
 
+TEST_CASE(IsolateReload_EnumWithSet) {
+  const char* kScript =
+      "enum Enum1 {\n"
+      "  member1({Enum2.member1, Enum2.member2}),\n"
+      "  member2({Enum2.member2}),\n"
+      "  member3({Enum2.member1}),\n"
+      "  member4({Enum2.member2, Enum2.member1}),\n"
+      "  member5({Enum2.member1}),\n"
+      "  member6({Enum2.member1});\n"
+      "  const Enum1(this.set);\n"
+      "  final Set<Enum2> set;\n"
+      "}\n"
+      "enum Enum2 { member1, member2; }\n"
+      "var retained;\n"
+      "main() {\n"
+      "  retained = Enum1.member4;\n"
+      "  return 'ok';\n"
+      "}\n";
+  Dart_Handle lib = TestCase::LoadTestScript(kScript, nullptr);
+  EXPECT_VALID(lib);
+  EXPECT_STREQ("ok", SimpleInvokeStr(lib, "main"));
+
+  const char* kReloadScript =
+      "enum Enum2 { member1, member2; }\n"
+      "enum Enum1 {\n"
+      "  member1({Enum2.member1, Enum2.member2}),\n"
+      "  member2({Enum2.member2}),\n"
+      "  member3({Enum2.member1}),\n"
+      "  member4({Enum2.member2, Enum2.member1}),\n"
+      "  member5({Enum2.member1}),\n"
+      "  member6({Enum2.member1});\n"
+      "  const Enum1(this.set);\n"
+      "  final Set<Enum2> set;\n"
+      "}\n"
+      "var retained;\n"
+      "foo(e) {\n"
+      "  return switch (e as Enum1) {\n"
+      "    Enum1.member1 => \"a\",\n"
+      "    Enum1.member2 => \"b\",\n"
+      "    Enum1.member3 => \"c\",\n"
+      "    Enum1.member4 => \"d\",\n"
+      "    Enum1.member5 => \"e\",\n"
+      "    Enum1.member6 => \"f\",\n"
+      "  };\n"
+      "}\n"
+      "main() {\n"
+      "  return foo(retained);\n"
+      "}\n";
+
+  lib = TestCase::ReloadTestScript(kReloadScript);
+  EXPECT_VALID(lib);
+
+  {
+    // Reset the cache to make kernel constant reading happen again and perform
+    // fresh canonicalization, in particular the constants used by the switch
+    // statement. Something about the reproduction in
+    // https://github.com/dart-lang/sdk/issues/55350 causes this happen,
+    // possibly something about the library dependency graph keeps the
+    // equivalent enum use in a something with a separate kernel program info
+    // such that execution after reload already has an empty cache.
+    TransitionNativeToVM transition(thread);
+    Library& libb = Library::Handle(Library::RawCast(Api::UnwrapHandle(lib)));
+    KernelProgramInfo& info =
+        KernelProgramInfo::Handle(libb.kernel_program_info());
+    Array& constants = Array::Handle(info.constants());
+    for (intptr_t i = 0; i < constants.Length(); i++) {
+      constants.SetAt(i, Object::sentinel());
+    }
+  }
+
+  EXPECT_STREQ("d", SimpleInvokeStr(lib, "main"));
+}
+
 TEST_CASE(IsolateReload_KeepPragma2) {
   // Old version of closure function bar() has a pragma.
   const char* kScript =

runtime/vm/object.cc

@@ -20517,7 +20517,7 @@ void Instance::CanonicalizeFieldsLocked(Thread* thread) const {
     // Iterate over all fields, canonicalize numbers and strings, expect all
     // other instances to be canonical otherwise report error (return false).
     Zone* zone = thread->zone();
-    Instance& obj = Instance::Handle(zone);
+    Object& obj = Object::Handle(zone);
     const intptr_t instance_size = SizeFromClass();
     ASSERT(instance_size != 0);
     const auto unboxed_fields_bitmap =
@@ -20527,9 +20527,13 @@ void Instance::CanonicalizeFieldsLocked(Thread* thread) const {
       if (unboxed_fields_bitmap.Get(offset / kCompressedWordSize)) {
         continue;
       }
-      obj ^= this->FieldAddrAtOffset(offset)->Decompress(untag()->heap_base());
-      obj = obj.CanonicalizeLocked(thread);
-      this->SetFieldAtOffset(offset, obj);
+      obj = this->FieldAddrAtOffset(offset)->Decompress(untag()->heap_base());
+      if (obj.IsInstance()) {
+        obj = Instance::Cast(obj).CanonicalizeLocked(thread);
+        this->SetFieldAtOffset(offset, obj);
+      } else {
+        ASSERT(obj.IsNull() || obj.IsSentinel());
+      }
     }
   } else {
 #if defined(DEBUG) && !defined(DART_COMPRESSED_POINTERS)
@@ -25219,6 +25223,7 @@ ArrayPtr Array::MakeFixedLength(const GrowableObjectArray& growable_array,
 }
 
 void Array::CanonicalizeFieldsLocked(Thread* thread) const {
+  ASSERT(IsImmutable());
   intptr_t len = Length();
   if (len > 0) {
     Zone* zone = thread->zone();
@@ -25477,9 +25482,8 @@ void LinkedHashBase::CanonicalizeFieldsLocked(Thread* thread) const {
   data_array ^= data_array.CanonicalizeLocked(thread);
   set_data(data_array);
 
-  // The index should not be set yet. It is populated lazily on first read.
-  const auto& index_td = TypedData::Handle(zone, index());
-  ASSERT(index_td.IsNull());
+  // Ignoring index. It will be initially null, created on first use, and
+  // possibly non-null here if we are rehashing.
 }
 
 ConstMapPtr ConstMap::NewDefault(Heap::Space space) {