Revert "[vm, reload] Delay enum forwarding until after instance morphing."

This reverts commit 6ef967fb07.

Reason for revert: https://github.com/dart-lang/sdk/issues/53039

Original change's description:
> [vm, reload] Delay enum forwarding until after instance morphing.
>
> Enum forwarding requires evaluating and canonicalizing the new enum instances. If these in turn refer to other instances of classes with shape changes, canonicalization may try to compare instances of the old and new shapes and dereference past the end of an object. By waiting for instance morphing to complete, canonicalization can only see instances with the new shape.
>
> Also, don't attempt to forward Enum.values. When an enum member is added or removed, this has the same merging problem as 40442.
>
> Cf. bad074cc49.
>
> TEST=ci
> Bug: https://github.com/flutter/flutter/issues/129177
> Change-Id: I19d0508059bade8496000eeea257bb0730f11d17
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/316041
> Reviewed-by: Ben Konyi <bkonyi@google.com>
> Commit-Queue: Ryan Macnak <rmacnak@google.com>

Bug: https://github.com/flutter/flutter/issues/129177
Change-Id: I8aa09be5d8fd72460ab26294ed94a5075135d48b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/316501
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>

runtime/vm/class_finalizer.cc

@@ -994,10 +994,8 @@ void ClassFinalizer::SortClasses() {
   }
   ASSERT(next_new_cid == num_cids);
   RemapClassIds(old_to_new_cid.get());
-  // Types use cid's as part of their hashes.
-  RehashTypes();
-  // Const objects use cid's as part of their hashes.
-  IG->RehashConstants(nullptr);
+  RehashTypes();          // Types use cid's as part of their hashes.
+  IG->RehashConstants();  // Const objects use cid's as part of their hashes.
 }
 
 class CidRewriteVisitor : public ObjectVisitor {
@@ -1244,7 +1242,7 @@ void ClassFinalizer::RehashTypes() {
 
   // The canonical constant tables use canonical hashcodes which can change
   // due to cid-renumbering.
-  IG->RehashConstants(nullptr);
+  IG->RehashConstants();
 
   dict_size = Utils::RoundUpToPowerOfTwo(typeargs.Length() * 4 / 3);
   CanonicalTypeArgumentsSet typeargs_table(

runtime/vm/heap/become.cc

@@ -274,10 +274,6 @@ static void InvalidForwarding(ObjectPtr before,
 }
 
 void Become::Forward() {
-  if (pointers_.length() == 0) {
-    return;
-  }
-
   Thread* thread = Thread::Current();
   auto heap = thread->isolate_group()->heap();
 

runtime/vm/isolate.cc

@@ -837,7 +837,7 @@ void IsolateGroup::ExitTemporaryIsolate() {
   Dart::ShutdownIsolate(thread);
 }
 
-void IsolateGroup::RehashConstants(Become* become) {
+void IsolateGroup::RehashConstants() {
   // Even though no individual constant contains a cycle, there can be "cycles"
   // between the canonical tables if some const instances of A have fields that
   // are const instance of B and vice versa. So set all the old tables to the
@@ -872,7 +872,9 @@ void IsolateGroup::RehashConstants(Become* become) {
     }
 
     cls = class_table()->At(cid);
+
     if (cls.constants() == Array::null()) continue;
+
     old_constant_tables[cid] = &Array::Handle(zone, cls.constants());
     cls.set_constants(Object::null_array());
   }
@@ -881,12 +883,6 @@ void IsolateGroup::RehashConstants(Become* become) {
   heap()->ResetCanonicalHashTable();
 
   Instance& constant = Instance::Handle(zone);
-  Field& field = Field::Handle(zone);
-  String& name = String::Handle(zone);
-  Array& new_values = Array::Handle(zone);
-  Instance& old_value = Instance::Handle(zone);
-  Instance& new_value = Instance::Handle(zone);
-  Instance& deleted = Instance::Handle(zone);
   for (intptr_t cid = kInstanceCid; cid < num_cids; cid++) {
     Array* old_constants = old_constant_tables[cid];
     if (old_constants == nullptr) continue;
@@ -894,61 +890,14 @@ void IsolateGroup::RehashConstants(Become* become) {
     cls = class_table()->At(cid);
     CanonicalInstancesSet set(zone, old_constants->ptr());
     CanonicalInstancesSet::Iterator it(&set);
-
-    if (cls.is_enum_class() && (become != nullptr)) {
-      field = cls.LookupStaticField(Symbols::_DeletedEnumSentinel());
-      deleted ^= field.StaticConstFieldValue();
-      if (deleted.IsNull()) {
-        deleted = Instance::New(cls, Heap::kOld);
-        field = object_store()->enum_name_field();
-        name = cls.ScrubbedName();
-        name = Symbols::FromConcat(thread, Symbols::_DeletedEnumPrefix(), name);
-        deleted.SetField(field, name);
-        field = object_store()->enum_index_field();
-        new_value = Smi::New(-1);
-        deleted.SetField(field, new_value);
-        // The static const field contains `Object::null()` instead of
-        // `Object::sentinel()` - so it's not considered an initializing store.
-        field.SetStaticConstFieldValue(deleted,
-                                       /*assert_initializing_store*/ false);
-      }
-
-      field = cls.LookupField(Symbols::Values());
-      new_values ^= field.StaticConstFieldValue();
-
-      field = object_store()->enum_name_field();
-      while (it.MoveNext()) {
-        old_value ^= set.GetKey(it.Current());
-        ASSERT(old_value.GetClassId() == cid);
-        bool found = false;
-        for (intptr_t j = 0; j < new_values.Length(); j++) {
-          new_value ^= new_values.At(j);
-          ASSERT(new_value.GetClassId() == cid);
-          if (old_value.GetField(field) == new_value.GetField(field)) {
-            found = true;
-            break;
-          }
-        }
-        if (!found) {
-          new_value = deleted.ptr();
-        }
-
-        if (old_value.ptr() != new_value.ptr()) {
-          become->Add(old_value, new_value);
-        }
-        if (new_value.IsCanonical()) {
-          cls.InsertCanonicalConstant(zone, new_value);
-        }
-      }
-    } 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);
-      }
+    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() ||
+                   IsolateGroup::Current()->HasAttemptedReload());
+      cls.InsertCanonicalConstant(zone, constant);
     }
     set.Release();
   }

runtime/vm/isolate.h

@@ -285,7 +285,7 @@ class IsolateGroup : public IntrusiveDListEntry<IsolateGroup> {
                Dart_IsolateFlags api_flags);
   ~IsolateGroup();
 
-  void RehashConstants(Become* become);
+  void RehashConstants();
 #if defined(DEBUG)
   void ValidateClassTable();
 #endif

runtime/vm/isolate_reload.cc

@@ -237,6 +237,111 @@ void InstanceMorpher::AddObject(ObjectPtr object) {
 }
 
 void InstanceMorpher::CreateMorphedCopies(Become* become) {
+  // Enum migriation needs to run in same phase as instance morphing since it
+  // may also involve a shape change.
+  if (old_class_.is_enum_class()) {
+    Field& field = Field::Handle(Z);
+    String& name = String::Handle(Z);
+    Array& old_values = Array::Handle(Z);
+    Array& new_values = Array::Handle(Z);
+    Instance& old_value = Instance::Handle(Z);
+    Instance& new_value = Instance::Handle(Z);
+    Instance& old_deleted = Instance::Handle(Z);
+    Instance& new_deleted = Instance::Handle(Z);
+
+    field = old_class_.LookupStaticField(Symbols::_DeletedEnumSentinel());
+    old_deleted ^= field.StaticConstFieldValue();
+
+    new_deleted = Instance::New(new_class_, Heap::kOld);
+    Thread* thread = Thread::Current();
+    field = thread->isolate_group()->object_store()->enum_name_field();
+    name = new_class_.ScrubbedName();
+    name = Symbols::FromConcat(thread, Symbols::_DeletedEnumPrefix(), name);
+    new_deleted.SetField(field, name);
+    field = thread->isolate_group()->object_store()->enum_index_field();
+    new_value = Smi::New(-1);
+    new_deleted.SetField(field, new_value);
+    field = new_class_.LookupStaticField(Symbols::_DeletedEnumSentinel());
+    // The static const field contains `Object::null()` instead of
+    // `Object::sentinel()` - so it's not considered an initializing store.
+    field.SetStaticConstFieldValue(new_deleted,
+                                   /*assert_initializing_store*/ false);
+
+    field = old_class_.LookupField(Symbols::Values());
+    old_values ^= field.StaticConstFieldValue();
+
+    field = new_class_.LookupField(Symbols::Values());
+    new_values ^= field.StaticConstFieldValue();
+
+    field = thread->isolate_group()->object_store()->enum_name_field();
+    for (intptr_t i = 0; i < old_values.Length(); i++) {
+      old_value ^= old_values.At(i);
+      ASSERT(old_value.GetClassId() == cid_);
+      bool found = false;
+      for (intptr_t j = 0; j < new_values.Length(); j++) {
+        new_value ^= new_values.At(j);
+        ASSERT(new_value.GetClassId() == cid_);
+        if (old_value.GetField(field) == new_value.GetField(field)) {
+          found = true;
+          break;
+        }
+      }
+      if (!found) {
+        new_value = new_deleted.ptr();
+      }
+
+      if (old_value.ptr() == new_value.ptr()) {
+        // This probably shouldn't happen and means canonicalization is mixing
+        // before and after. At any rate, don't submit a self-fowarding to
+        // become.
+        RELEASE_ASSERT(old_value.ptr()->untag()->HeapSize() ==
+                       new_class_.host_instance_size());
+      } else {
+        // Convert the old instance into a filler object. We will switch to the
+        // new class table before the next heap walk, so there must be no
+        // instances of any class with the old size.
+        Become::MakeDummyObject(old_value);
+
+        become->Add(old_value, new_value);
+      }
+    }
+
+    // The deleted sentinel is not part of Enum.values. It doesn't exist yet for
+    // the first reload.
+    if (!old_deleted.IsNull()) {
+      // Convert the old instance into a filler object. We will switch to the
+      // new class table before the next heap walk, so there must be no
+      // instances of any class with the old size.
+      Become::MakeDummyObject(old_deleted);
+
+      become->Add(old_deleted, new_deleted);
+    }
+
+    // We also forward Enum.values. No filler is needed because arrays never
+    // change shape.
+    if (old_value.ptr() == new_value.ptr()) {
+      // This probably shouldn't happen and means canonicalization is mixing
+      // before and after. At any rate, don't submit a self-fowarding to
+      // become.
+      RELEASE_ASSERT(old_class_.host_instance_size() ==
+                     new_class_.host_instance_size());
+    } else {
+      become->Add(old_values, new_values);
+    }
+
+#if defined(DEBUG)
+    for (intptr_t i = 0; i < before_.length(); i++) {
+      const Instance& before = *before_.At(i);
+      // All instances are accounted for.
+      ASSERT((before.GetClassId() == kForwardingCorpse) ||
+             (before.ptr()->untag()->HeapSize() ==
+              new_class_.host_instance_size()));
+    }
+#endif
+
+    return;
+  }
+
   Instance& after = Instance::Handle(Z);
   Object& value = Object::Handle(Z);
   for (intptr_t i = 0; i < before_.length(); i++) {
@@ -874,7 +979,6 @@ bool IsolateGroupReloadContext::Reload(bool force_reload,
     // we have accepted the compilation to clear some state in the incremental
     // compiler.
     if (did_kernel_compilation) {
-      TIMELINE_SCOPE(AcceptCompilation);
       const auto& result = Object::Handle(Z, AcceptCompilation(thread));
       if (result.IsError()) {
         const auto& error = Error::Cast(result);
@@ -891,8 +995,7 @@ bool IsolateGroupReloadContext::Reload(bool force_reload,
         // layout).
         ObjectLocator locator(this);
         {
-          TIMELINE_SCOPE(CollectInstances);
-          HeapIterationScope iteration(thread);
+          HeapIterationScope iteration(Thread::Current());
           iteration.IterateObjects(&locator);
         }
 
@@ -1263,7 +1366,9 @@ void ProgramReloadContext::RegisterClass(const Class& new_cls) {
   VTIR_Print("Registering class: %s\n", new_cls.ToCString());
   new_cls.set_id(old_cls.id());
   IG->class_table()->SetAt(old_cls.id(), new_cls.ptr());
-  new_cls.CopyCanonicalConstants(old_cls);
+  if (!old_cls.is_enum_class()) {
+    new_cls.CopyCanonicalConstants(old_cls);
+  }
   new_cls.CopyDeclarationType(old_cls);
   AddBecomeMapping(old_cls, new_cls);
   AddClassMapping(new_cls, old_cls);
@@ -1741,11 +1846,7 @@ void ProgramReloadContext::CommitAfterInstanceMorphing() {
   // content may have changed from fields being added or removed.
   {
     TIMELINE_SCOPE(RehashConstants);
-    IG->RehashConstants(&become_);
-  }
-  {
-    TIMELINE_SCOPE(ForwardEnums);
-    become_.Forward();
+    IG->RehashConstants();
   }
 
   if (FLAG_identity_reload) {
@@ -1997,7 +2098,6 @@ void ProgramReloadContext::RunInvalidationVisitors() {
   GrowableArray<const Instance*> instances(4 * KB);
 
   {
-    TIMELINE_SCOPE(CollectInvalidations);
     HeapIterationScope iteration(thread);
     InvalidationCollector visitor(zone, &functions, &kernel_infos, &fields,
                                   &suspend_states, &instances);

runtime/vm/isolate_reload_test.cc

@@ -2367,57 +2367,27 @@ TEST_CASE(IsolateReload_EnumShapeChangeRemove) {
                SimpleInvokeStr(lib, "main"));
 }
 
-TEST_CASE(IsolateReload_EnumReferentShapeChangeAdd) {
+TEST_CASE(IsolateReload_EnumShapeChangeValues) {
   const char* kScript =
-      "class Box {\n"
-      "  final x;\n"
-      "  const Box(this.x);\n"
-      "}\n"
-      "enum Fruit {\n"
-      "  Apple('Apple', const Box('A')),\n"
-      "  Banana('Banana', const Box('B')),\n"
-      "  Cherry('Cherry', const Box('C')),\n"
-      "  Durian('Durian', const Box('D')),\n"
-      "  Elderberry('Elderberry', const Box('E')),\n"
-      "  Fig('Fig', const Box('F')),\n"
-      "  Grape('Grape', const Box('G')),\n"
-      "  Huckleberry('Huckleberry', const Box('H')),\n"
-      "  Jackfruit('Jackfruit', const Box('J'));\n"
-      "  const Fruit(this.name, this.initial);\n"
-      "  final String name;\n"
-      "  final Box initial;\n"
-      "}\n"
+      "enum Fruit { Apple, Banana }\n"
       "var retained;\n"
       "main() {\n"
-      "  retained = Fruit.Apple;\n"
+      "  retained = Fruit.values;\n"
       "  return retained.toString();\n"
       "}\n";
 
   Dart_Handle lib = TestCase::LoadTestScript(kScript, nullptr);
   EXPECT_VALID(lib);
-  EXPECT_STREQ("Fruit.Apple", SimpleInvokeStr(lib, "main"));
+  EXPECT_STREQ("[Fruit.Apple, Fruit.Banana]", SimpleInvokeStr(lib, "main"));
 
   const char* kReloadScript =
-      "class Box {\n"
-      "  final x;\n"
-      "  final y;\n"
-      "  final z;\n"
-      "  const Box(this.x, this.y, this.z);\n"
-      "}\n"
       "enum Fruit {\n"
-      "  Apple('Apple', const Box('A', 0, 0)),\n"
-      "  Banana('Banana', const Box('B', 0, 0)),\n"
-      "  Cherry('Cherry', const Box('C', 0, 0)),\n"
-      "  Durian('Durian', const Box('D', 0, 0)),\n"
-      "  Elderberry('Elderberry', const Box('E', 0, 0)),\n"
-      "  Fig('Fig', const Box('F', 0, 0)),\n"
-      "  Grape('Grape', const Box('G', 0, 0)),\n"
-      "  Huckleberry('Huckleberry', const Box('H', 0, 0)),\n"
-      "  Jackfruit('Jackfruit', const Box('J', 0, 0)),\n"
-      "  Lemon('Lemon', const Box('L', 0, 0));\n"
+      "  Apple('Apple', 'A'),\n"
+      "  Banana('Banana', 'B'),\n"
+      "  Cherry('Cherry', 'C');\n"
       "  const Fruit(this.name, this.initial);\n"
       "  final String name;\n"
-      "  final Box initial;\n"
+      "  final String initial;\n"
       "}\n"
       "var retained;\n"
       "main() {\n"
@@ -2426,7 +2396,8 @@ TEST_CASE(IsolateReload_EnumReferentShapeChangeAdd) {
 
   lib = TestCase::ReloadTestScript(kReloadScript);
   EXPECT_VALID(lib);
-  EXPECT_STREQ("Fruit.Apple", SimpleInvokeStr(lib, "main"));
+  EXPECT_STREQ("[Fruit.Apple, Fruit.Banana, Fruit.Cherry]",
+               SimpleInvokeStr(lib, "main"));
 }
 
 TEST_CASE(IsolateReload_ConstantIdentical) {

runtime/vm/object_reload.cc

@@ -227,6 +227,11 @@ void Class::CopyStaticFieldValues(ProgramReloadContext* reload_context,
 }
 
 void Class::CopyCanonicalConstants(const Class& old_cls) const {
+  if (is_enum_class()) {
+    // We do not copy enum classes's canonical constants because we explicitly
+    // become the old enum values to the new enum values.
+    return;
+  }
 #if defined(DEBUG)
   {
     // Class has no canonical constants allocated.