[vm/reload] Update is_implemented after reload

When doing fine grained invalidation and reload we might end up losing
`is_implemented` bit and some other class-hierarchy related bits
on a class if we only reload the library where class is declared
but not transitive closure which depends on that library.

Consider:

    library a;
    class A {}

    library b;
    class B implements A {}

If we do a change to library `a` which does not impact the outline
then we do not need to reload `b`. This will cause class A to
be replaced with a new `Class` object in the class table - and lead
to us losing various CHA related bits.

This CL update ProgramReloadContext::RebuildDirectSubclasses to
handle this correctly, we also rename it to
RestoreClassHierarchyInvariants and make it share implementation
with ClassFinalizer (which contained almost identical function).

Fixes https://github.com/flutter/flutter/issues/151032

TEST=vm/cc/IsolateReload_IsImplementedBit

Change-Id: Ie620592befecb89897d6e8d46175ef07348bf11d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/374040
Commit-Queue: Slava Egorov <vegorov@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>

runtime/vm/class_finalizer.cc

@@ -521,30 +521,6 @@ void ClassFinalizer::FinalizeMemberTypes(const Class& cls) {
     }
   }
 }
-
-// For a class used as an interface marks this class and all its superclasses
-// implemented.
-//
-// Does not mark its interfaces implemented because those would already be
-// marked as such.
-static void MarkImplemented(Zone* zone, const Class& iface) {
-  if (iface.is_implemented()) {
-    return;
-  }
-
-  Class& cls = Class::Handle(zone, iface.ptr());
-  AbstractType& type = AbstractType::Handle(zone);
-
-  while (!cls.is_implemented()) {
-    cls.set_is_implemented();
-
-    type = cls.super_type();
-    if (type.IsNull() || type.IsObjectType()) {
-      break;
-    }
-    cls = type.type_class();
-  }
-}
 #endif  // !defined(DART_PRECOMPILED_RUNTIME)
 
 void ClassFinalizer::FinalizeTypesInClass(const Class& cls) {
@@ -616,49 +592,73 @@ void ClassFinalizer::FinalizeTypesInClass(const Class& cls) {
     MarkClassHasDynamicallyExtendableSubtypes(zone, cls);
   }
 
-  RegisterClassInHierarchy(zone, cls);
+  ClassHiearchyUpdater(zone).Register(cls);
 #endif  // defined(DART_PRECOMPILED_RUNTIME)
 }
 
 #if !defined(DART_PRECOMPILED_RUNTIME)
-void ClassFinalizer::RegisterClassInHierarchy(Zone* zone, const Class& cls) {
-  auto& type = AbstractType::Handle(zone, cls.super_type());
-  auto& other_cls = Class::Handle(zone);
+// For a class used as an interface marks this class and all its superclasses
+// implemented.
+//
+// Does not mark its interfaces implemented because those would already be
+// marked as such.
+void ClassHiearchyUpdater::MarkImplemented(const Class& interface) {
+  super_ = interface_.ptr();
+  while (!super_.is_implemented()) {
+    super_.set_is_implemented(true);
+    type_ = super_.super_type();
+    if (type_.IsNull() || type_.IsObjectType()) {
+      break;
+    }
+    super_ = type_.type_class();
+  }
+}
+
+void ClassHiearchyUpdater::Register(const Class& cls) {
+  type_ = cls.super_type();
   // Add this class to the direct subclasses of the superclass, unless the
   // superclass is Object.
-  if (!type.IsNull() && !type.IsObjectType()) {
-    other_cls = cls.SuperClass();
-    ASSERT(!other_cls.IsNull());
-    other_cls.AddDirectSubclass(cls);
+  if (!type_.IsNull() && !type_.IsObjectType()) {
+    super_ = cls.SuperClass();
+    ASSERT(!super_.IsNull());
+    super_.AddDirectSubclass(cls);
   }
 
   // Add this class as an implementor to the implemented interface's type
   // classes.
-  auto& interfaces = Array::Handle(zone, cls.interfaces());
-  const intptr_t mixin_index =
-      cls.is_transformed_mixin_application() ? interfaces.Length() - 1 : -1;
-  for (intptr_t i = 0; i < interfaces.Length(); ++i) {
-    type ^= interfaces.At(i);
-    other_cls = type.type_class();
-    MarkImplemented(zone, other_cls);
-    other_cls.AddDirectImplementor(cls, /* is_mixin = */ i == mixin_index);
+  interfaces_ = cls.interfaces();
+  // Class::interfaces() can be null for some VM internal classes.
+  if (!interfaces_.IsNull()) {
+    const intptr_t mixin_index =
+        cls.is_transformed_mixin_application() ? interfaces_.Length() - 1 : -1;
+    for (intptr_t i = 0; i < interfaces_.Length(); ++i) {
+      type_ ^= interfaces_.At(i);
+      interface_ = type_.type_class();
+      const bool is_mixin = i == mixin_index;
+      MarkImplemented(interface_);
+      interface_.AddDirectImplementor(cls, is_mixin);
+    }
   }
 
   // Propagate known concrete implementors to interfaces.
   if (!cls.is_abstract()) {
-    GrowableArray<const Class*> worklist;
-    worklist.Add(&cls);
-    while (!worklist.is_empty()) {
-      const Class& implemented = *worklist.RemoveLast();
-      if (!implemented.NoteImplementor(cls)) continue;
-      type = implemented.super_type();
-      if (!type.IsNull()) {
-        worklist.Add(&Class::Handle(zone, implemented.SuperClass()));
+    worklist_.Add(cls);
+    while (!worklist_.IsEmpty()) {
+      implemented_ = worklist_.RemoveLast();
+      if (!implemented_.NoteImplementor(cls)) continue;
+      type_ = implemented_.super_type();
+      if (!type_.IsNull()) {
+        super_ = type_.type_class();
+        worklist_.Add(super_);
       }
-      interfaces = implemented.interfaces();
-      for (intptr_t i = 0; i < interfaces.Length(); i++) {
-        type ^= interfaces.At(i);
-        worklist.Add(&Class::Handle(zone, type.type_class()));
+      interfaces_ = implemented_.interfaces();
+      // Class::interfaces() can be null for some VM internal classes.
+      if (!interfaces_.IsNull()) {
+        for (intptr_t i = 0; i < interfaces_.Length(); i++) {
+          type_ ^= interfaces_.At(i);
+          interface_ = type_.type_class();
+          worklist_.Add(interface_);
+        }
       }
     }
   }

runtime/vm/class_finalizer.h

@@ -52,9 +52,6 @@ class ClassFinalizer : public AllStatic {
   static void FinalizeTypesInClass(const Class& cls);
 
 #if !defined(DART_PRECOMPILED_RUNTIME)
-  // Register class in the lists of direct subclasses and direct implementors.
-  static void RegisterClassInHierarchy(Zone* zone, const Class& cls);
-
   // Mark [cls], its superclass and superinterfaces as can_be_future().
   static void MarkClassCanBeFuture(Zone* zone, const Class& cls);
 
@@ -109,6 +106,52 @@ class ClassFinalizer : public AllStatic {
 #endif  // !defined(DART_PRECOMPILED_RUNTIME)
 };
 
+class ClassHiearchyUpdater : public ValueObject {
+ public:
+  explicit ClassHiearchyUpdater(Zone* zone) : zone_(zone) {}
+
+  // Register class in the lists of direct subclasses and direct implementors.
+  void Register(const Class& cls);
+
+ private:
+  void MarkImplemented(const Class& interface);
+
+  Zone* const zone_;
+
+  AbstractType& type_ = AbstractType::Handle(zone_);
+  Class& super_ = Class::Handle(zone_);
+  Class& implemented_ = Class::Handle(zone_);
+  Class& interface_ = Class::Handle(zone_);
+  Array& interfaces_ = Array::Handle(zone_);
+
+  class ClassWorklist {
+   public:
+    explicit ClassWorklist(Zone* zone) : zone_(zone) {}
+
+    bool IsEmpty() const { return size_ == 0; }
+
+    void Add(const Class& cls) {
+      if (worklist_.length() == size_) {
+        worklist_.Add(&Class::Handle(zone_));
+      }
+      *worklist_[size_] = cls.ptr();
+      size_++;
+    }
+
+    ClassPtr RemoveLast() {
+      size_--;
+      return worklist_[size_]->ptr();
+    }
+
+   private:
+    Zone* const zone_;
+    GrowableArray<Class*> worklist_;
+    intptr_t size_ = 0;
+  };
+
+  ClassWorklist worklist_{zone_};
+};
+
 }  // namespace dart
 
 #endif  // RUNTIME_VM_CLASS_FINALIZER_H_

runtime/vm/isolate_reload.cc

@@ -972,11 +972,10 @@ bool IsolateGroupReloadContext::Reload(bool force_reload,
     }
 
     // ValidateReload mutates the direct subclass information and does
-    // not remove dead subclasses.  Rebuild the direct subclass
-    // information from scratch.
+    // not remove dead subclasses.
     {
       SafepointWriteRwLocker ml(thread, IG->program_lock());
-      IG->program_reload_context()->RebuildDirectSubclasses();
+      IG->program_reload_context()->RestoreClassHierarchyInvariants();
     }
     const intptr_t final_library_count =
         GrowableObjectArray::Handle(Z, IG->object_store()->libraries())
@@ -2635,7 +2634,7 @@ void ProgramReloadContext::AddBecomeMapping(const Object& old,
   become_.Add(old, neu);
 }
 
-void ProgramReloadContext::RebuildDirectSubclasses() {
+void ProgramReloadContext::RestoreClassHierarchyInvariants() {
   ClassTable* class_table = IG->class_table();
   intptr_t num_cids = class_table->NumCids();
 
@@ -2656,43 +2655,25 @@ void ProgramReloadContext::RebuildDirectSubclasses() {
       if (cls.direct_implementors() != GrowableObjectArray::null()) {
         cls.set_direct_implementors(null_list);
       }
+      if (cls.is_implemented()) {
+        cls.set_is_implemented(false);
+      }
+      if (cls.implementor_cid() != kIllegalCid) {
+        cls.ClearImplementor();
+      }
     }
   }
 
-  // Recompute the direct subclasses / implementors.
-
-  AbstractType& super_type = AbstractType::Handle();
-  Class& super_cls = Class::Handle();
-
-  Array& interface_types = Array::Handle();
-  AbstractType& interface_type = AbstractType::Handle();
-  Class& interface_class = Class::Handle();
-
+  // Recompute class hiearchy.
+  ClassHiearchyUpdater class_hieararchy_updater(zone());
   for (intptr_t i = 1; i < num_cids; i++) {
     if (class_table->HasValidClassAt(i)) {
       cls = class_table->At(i);
       if (!cls.is_declaration_loaded()) {
         continue;  // Will register itself later when loaded.
       }
-      super_type = cls.super_type();
-      if (!super_type.IsNull() && !super_type.IsObjectType()) {
-        super_cls = cls.SuperClass();
-        ASSERT(!super_cls.IsNull());
-        super_cls.AddDirectSubclass(cls);
-      }
 
-      interface_types = cls.interfaces();
-      if (!interface_types.IsNull()) {
-        const intptr_t mixin_index = cls.is_transformed_mixin_application()
-                                         ? interface_types.Length() - 1
-                                         : -1;
-        for (intptr_t j = 0; j < interface_types.Length(); ++j) {
-          interface_type ^= interface_types.At(j);
-          interface_class = interface_type.type_class();
-          interface_class.AddDirectImplementor(
-              cls, /* is_mixin = */ i == mixin_index);
-        }
-      }
+      class_hieararchy_updater.Register(cls);
     }
   }
 }

runtime/vm/isolate_reload.h

@@ -382,7 +382,7 @@ class ProgramReloadContext {
                          const Library& original);
   void AddStaticFieldMapping(const Field& old_field, const Field& new_field);
   void AddBecomeMapping(const Object& old, const Object& neu);
-  void RebuildDirectSubclasses();
+  void RestoreClassHierarchyInvariants();
 
   Become become_;
 

runtime/vm/isolate_reload_test.cc

@@ -6522,6 +6522,113 @@ TEST_CASE(IsolateReload_KeepPragma3) {
                               Symbols::vm_never_inline()));
 }
 
+TEST_CASE(IsolateReload_IsImplementedBit) {
+  const char* kScript = R"(
+import 'file:///lib.dart';
+
+class C1 implements A1 {}
+class C2 extends A2Impl {}
+class C3 implements A4 {}
+abstract class C4 extends A5 {}
+class C5 implements C4 {}
+
+main() {
+  C1();
+  C2();
+  C3();
+  C5();
+  return "ok";
+}
+)";
+
+  TestCase::AddTestLib("file:///lib.dart", R"(
+abstract class A1 { }
+abstract class A2 { }
+abstract class A2Impl implements A2 {}
+abstract class A3 { }
+abstract class A4 extends A3 {}
+abstract class A5 { }
+)");
+  Dart_Handle lib = TestCase::LoadTestScript(kScript, nullptr);
+  EXPECT_VALID(lib);
+  EXPECT_STREQ("ok", SimpleInvokeStr(lib, "main"));
+
+  const auto check_implemented =
+      [](const Library& lib, const char* cls_name, bool expected,
+         std::initializer_list<const char*> expected_direct_implementors,
+         intptr_t expected_implementor_cid) {
+        const auto& cls = Class::Handle(
+            lib.LookupClass(String::Handle(String::New(cls_name))));
+        EXPECT(!cls.IsNull());
+        EXPECT_EQ(expected, cls.is_implemented());
+
+        EXPECT_EQ(expected_implementor_cid, cls.implementor_cid());
+
+        const auto& implementors =
+            GrowableObjectArray::Handle(cls.direct_implementors_unsafe());
+        if (implementors.IsNull()) {
+          EXPECT_EQ(expected_direct_implementors.size(),
+                    static_cast<size_t>(0));
+        } else {
+          auto& implementor = Class::Handle();
+          auto& implementor_name = String::Handle();
+          EXPECT_EQ(expected_direct_implementors.size(),
+                    static_cast<size_t>(implementors.Length()));
+          for (intptr_t i = 0; i < implementors.Length(); i++) {
+            implementor ^= implementors.At(i);
+            implementor_name = implementor.UserVisibleName();
+            bool found = false;
+            for (auto expected_name : expected_direct_implementors) {
+              if (implementor_name.Equals(expected_name)) {
+                found = true;
+                break;
+              }
+            }
+            if (!found) {
+              FAIL("Found unexpected implementor: %s",
+                   implementor_name.ToCString());
+            }
+          }
+        }
+      };
+
+  const auto cid_of = [&](const char* cls_name) {
+    const auto& lib = Library::Handle(Library::LookupLibrary(
+        thread, String::Handle(String::New(RESOLVED_USER_TEST_URI))));
+    const auto& cls =
+        Class::Handle(lib.LookupClass(String::Handle(String::New(cls_name))));
+    EXPECT(!cls.IsNull());
+    return cls.id();
+  };
+
+  const auto check_class_hierarchy_state = [&]() {
+    TransitionNativeToVM transition(thread);
+    const auto& lib = Library::Handle(Library::LookupLibrary(
+        thread, String::Handle(String::New("file:///lib.dart"))));
+    EXPECT(!lib.IsNull());
+
+    check_implemented(lib, "A1", true, {"C1"}, cid_of("C1"));
+    check_implemented(lib, "A2", true, {"A2Impl"}, cid_of("C2"));
+    check_implemented(lib, "A2Impl", false, {}, cid_of("C2"));
+    check_implemented(lib, "A3", true, {}, cid_of("C3"));
+    check_implemented(lib, "A4", true, {"C3"}, cid_of("C3"));
+    check_implemented(lib, "A5", true, {}, cid_of("C5"));
+  };
+
+  check_class_hierarchy_state();
+
+  Dart_SetFileModifiedCallback([](const char* url, int64_t since) {
+    return strcmp(url, "file:///lib.dart") == 0;
+  });
+
+  lib = TestCase::TriggerReload(nullptr, 0);
+  EXPECT_VALID(lib);
+
+  Dart_SetFileModifiedCallback(nullptr);
+
+  check_class_hierarchy_state();
+}
+
 #endif  // !defined(PRODUCT) && !defined(DART_PRECOMPILED_RUNTIME)
 
 }  // namespace dart

runtime/vm/object.cc

@@ -1451,8 +1451,10 @@ class FinalizeVMIsolateVisitor : public ObjectVisitor {
 #if !defined(DART_PRECOMPILED_RUNTIME)
       if (obj->IsClass()) {
         // Won't be able to update read-only VM isolate classes if implementors
-        // are discovered later.
-        static_cast<ClassPtr>(obj)->untag()->implementor_cid_ = kDynamicCid;
+        // are discovered later. We use kVoidCid instead of kDynamicCid here to
+        // be able to distinguish read-only VM isolate classes during reload.
+        // See ProgramReloadContext::RestoreClassHierarchyInvariants.
+        static_cast<ClassPtr>(obj)->untag()->implementor_cid_ = kVoidCid;
       }
 #endif
     }
@@ -5578,6 +5580,14 @@ void Class::set_implementor_cid(intptr_t value) const {
   StoreNonPointer(&untag()->implementor_cid_, value);
 }
 
+void Class::ClearImplementor() const {
+  // Check raw implementor_cid_ without normalization done by
+  // implementor_cid() accessor.
+  if (untag()->implementor_cid_ != kVoidCid) {
+    set_implementor_cid(kIllegalCid);
+  }
+}
+
 bool Class::NoteImplementor(const Class& implementor) const {
   ASSERT(!implementor.is_abstract());
   ASSERT(IsolateGroup::Current()->program_lock()->IsCurrentThreadWriter());
@@ -5611,13 +5621,13 @@ int32_t Class::SourceFingerprint() const {
 #endif  // !defined(DART_PRECOMPILED_RUNTIME)
 }
 
-void Class::set_is_implemented() const {
+void Class::set_is_implemented(bool value) const {
   ASSERT(IsolateGroup::Current()->program_lock()->IsCurrentThreadWriter());
-  set_is_implemented_unsafe();
+  set_is_implemented_unsafe(value);
 }
 
-void Class::set_is_implemented_unsafe() const {
-  set_state_bits(ImplementedBit::update(true, state_bits()));
+void Class::set_is_implemented_unsafe(bool value) const {
+  set_state_bits(ImplementedBit::update(value, state_bits()));
 }
 
 void Class::set_is_abstract() const {

runtime/vm/object.h

@@ -1239,11 +1239,19 @@ class Class : public Object {
   // via `extends` or by `implements`, returns its CID.
   // If it has no implementation, returns kIllegalCid.
   // If it has more than one implementation, returns kDynamicCid.
-  intptr_t implementor_cid() const { return untag()->implementor_cid_; }
+  intptr_t implementor_cid() const {
+    // Classes in VM isolate use kVoidCid instead of kDynamicCid
+    // so that we could distinguish them.
+    const classid_t cid = untag()->implementor_cid_;
+    return cid == kVoidCid ? kDynamicCid : cid;
+  }
 
   // Returns true if the implementor tracking state changes and so must be
   // propagated to this class's superclass and interfaces.
   bool NoteImplementor(const Class& implementor) const;
+
+  // Used by hot reload to reset the state.
+  void ClearImplementor() const;
 #endif
 
   static intptr_t num_type_arguments_offset() {
@@ -1687,8 +1695,8 @@ class Class : public Object {
   // Returns false if all possible implementations of this interface must be
   // instances of this class or its subclasses.
   bool is_implemented() const { return ImplementedBit::decode(state_bits()); }
-  void set_is_implemented() const;
-  void set_is_implemented_unsafe() const;
+  void set_is_implemented(bool value) const;
+  void set_is_implemented_unsafe(bool value) const;
 
   bool is_abstract() const { return AbstractBit::decode(state_bits()); }
   void set_is_abstract() const;

runtime/vm/object_test.cc

@@ -70,11 +70,11 @@ ISOLATE_UNIT_TEST_CASE(Class) {
   interface_name = Symbols::New(thread, "Harley");
   interface = CreateDummyClass(interface_name, script);
   interfaces.SetAt(0, Type::Handle(Type::NewNonParameterizedType(interface)));
-  interface.set_is_implemented_unsafe();
+  interface.set_is_implemented_unsafe(true);
   interface_name = Symbols::New(thread, "Norton");
   interface = CreateDummyClass(interface_name, script);
   interfaces.SetAt(1, Type::Handle(Type::NewNonParameterizedType(interface)));
-  interface.set_is_implemented_unsafe();
+  interface.set_is_implemented_unsafe(true);
   cls.set_interfaces(interfaces);
 
   // Finalization of types happens before the fields and functions have been
@@ -360,7 +360,7 @@ ISOLATE_UNIT_TEST_CASE(InstanceClass) {
             one_field_class.host_instance_size());
   EXPECT_EQ(header_size, field.HostOffset());
   EXPECT(!one_field_class.is_implemented());
-  one_field_class.set_is_implemented_unsafe();
+  one_field_class.set_is_implemented_unsafe(true);
   EXPECT(one_field_class.is_implemented());
 }