[vm] Deoptimize code if can-be-Future property of a class changes due to dynamic loading

Sometimes compiler omits type checks for the 'await' and 'return from
async' by relying on the fact that certain classes cannot hold
instances of Future.

In JIT mode, the property 'class-can-be-Future' may change when a new
class is loaded dynamically (e.g. through IsolateMirror.loadUri).

In such a case, the generated code assuming that certain class
cannot be Future should be correctly deoptimized.
This CL implements this deoptimization by adding such classes into
the list of CHA guarded classes.

TEST=runtime/tests/vm/dart/await_type_check_with_dynamic_loading_test.dart

Change-Id: Ib3a3768d073e8562b794186e72cb9d61e9f496fa
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/309822
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Alexander Markov <alexmarkov@google.com>

runtime/tests/vm/dart/await_type_check_with_dynamic_loading_lib.dart

@@ -0,0 +1,34 @@
+// Copyright (c) 2023, 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.
+
+// Helper library for await_type_check_with_dynamic_loading_test.dart.
+
+import 'await_type_check_with_dynamic_loading_test.dart';
+
+class D implements A, Future<A> {
+  final Future<A> impl;
+
+  D(A value) : impl = Future.value(value);
+
+  @override
+  asStream() => impl.asStream();
+
+  @override
+  catchError(error, {test}) => impl.catchError(error, test: test);
+
+  @override
+  then<R>(onValue, {onError}) => impl.then(onValue, onError: onError);
+
+  @override
+  timeout(timeLimit, {onTimeout}) =>
+      impl.timeout(timeLimit, onTimeout: onTimeout);
+
+  @override
+  whenComplete(action) => impl.whenComplete(action);
+}
+
+makeNewFuture() {
+  final obj = B();
+  return (obj, D(obj));
+}

runtime/tests/vm/dart/await_type_check_with_dynamic_loading_test.dart

@@ -0,0 +1,44 @@
+// Copyright (c) 2023, 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 optimized code is correctly deoptimized
+// when a new implementation of Future is dynamically loaded and
+// existing classes now have a new subtype which is a Future.
+
+// VMOptions=--optimization-counter-threshold=100 --deterministic
+
+import 'dart:async';
+import 'dart:mirrors';
+
+import 'package:expect/expect.dart';
+
+class A {}
+
+class B implements A {}
+
+class C implements A {}
+
+Future<A> test1(Object expected, A x) async {
+  Expect.identical(expected, await x);
+  return x;
+}
+
+Future<void> test2(Object expected, A x) async {
+  Expect.identical(expected, await test1(expected, x));
+}
+
+void main() async {
+  for (int i = 0; i < 120; ++i) {
+    final obj1 = B();
+    await test2(obj1, obj1);
+    final obj2 = C();
+    await test2(obj2, obj2);
+  }
+
+  final isolate = currentMirrorSystem().isolate;
+  final library = await isolate
+      .loadUri(Uri.parse("await_type_check_with_dynamic_loading_lib.dart"));
+  final (Object expected, A x) = library.invoke(#makeNewFuture, []).reflectee;
+  await test2(expected, x);
+}

runtime/tests/vm/vm.status

@@ -139,6 +139,7 @@ dart_2/null_safety_autodetection_in_kernel_compiler_test: SkipByDesign # Test ne
 dart_2/snapshot_depfile_test: SkipByDesign # Test needs to run from source
 
 [ $compiler == dartkp ]
+dart/await_type_check_with_dynamic_loading_test: SkipByDesign # Uses dart:mirrors.
 dart/causal_stacks/async_throws_stack_no_causal_non_symbolic_test: SkipByDesign # --no-lazy... does nothing on precompiler.
 dart/causal_stacks/async_throws_stack_no_causal_test: SkipByDesign # --no-lazy... does nothing on precompiler.
 dart/finalizer/finalizer_isolate_groups_run_gc_test: SkipByDesign # Isolate.spawnUri is not supported in AOT.

runtime/vm/compiler/backend/flow_graph.cc

@@ -588,7 +588,7 @@ FlowGraph::ToCheck FlowGraph::CheckForInstanceCall(
           method_name.ToCString(), receiver_class.ToCString());
     }
     if (FLAG_use_cha_deopt) {
-      cha.AddToGuardedClasses(receiver_class, subclass_count);
+      cha.AddToGuardedClassesForSubclassCount(receiver_class, subclass_count);
     }
     return receiver_maybe_null ? ToCheck::kCheckNull : ToCheck::kNoCheck;
   }

runtime/vm/compiler/backend/il.cc

@@ -7303,7 +7303,7 @@ const Code& ReturnInstr::GetReturnStub(FlowGraphCompiler* compiler) const {
   const Function& function = compiler->parsed_function().function();
   ASSERT(function.IsSuspendableFunction());
   if (function.IsAsyncFunction()) {
-    if (!value()->Type()->CanBeFuture()) {
+    if (compiler->is_optimizing() && !value()->Type()->CanBeFuture()) {
       return Code::ZoneHandle(compiler->zone(),
                               compiler->isolate_group()
                                   ->object_store()

runtime/vm/compiler/backend/type_propagator.cc

@@ -978,7 +978,8 @@ bool CompileType::CanBeFuture() {
       (type_class_id == kNeverCid)) {
     return false;
   }
-  return Class::Handle(zone, type.type_class()).can_be_future();
+  const auto& cls = Class::Handle(zone, type.type_class());
+  return CHA::ClassCanBeFuture(cls);
 }
 
 void CompileType::PrintTo(BaseTextBuffer* f) const {
@@ -1219,9 +1220,10 @@ CompileType ParameterInstr::ComputeType() const {
                     type_class.ToCString());
               }
               if (FLAG_use_cha_deopt) {
-                thread->compiler_state().cha().AddToGuardedClasses(
-                    type_class,
-                    /*subclass_count=*/0);
+                thread->compiler_state()
+                    .cha()
+                    .AddToGuardedClassesForSubclassCount(type_class,
+                                                         /*subclass_count=*/0);
               }
               cid = type_class.id();
             }

runtime/vm/compiler/cha.cc

@@ -13,37 +13,45 @@
 
 namespace dart {
 
-void CHA::AddToGuardedClasses(const Class& cls, intptr_t subclass_count) {
-  ASSERT(subclass_count >= 0);
+void CHA::AddToGuardedClasses(const Class& cls,
+                              intptr_t subclass_count,
+                              intptr_t implementor_cid,
+                              bool track_future) {
   for (intptr_t i = 0; i < guarded_classes_.length(); i++) {
     if (guarded_classes_[i].cls->ptr() == cls.ptr()) {
-      // Was added as an interface guard.
-      if (guarded_classes_[i].subclass_count == -1) {
+      if ((subclass_count >= 0) && (guarded_classes_[i].subclass_count == -1)) {
         guarded_classes_[i].subclass_count = subclass_count;
       }
+      if ((implementor_cid != kIllegalCid) &&
+          (guarded_classes_[i].implementor_cid == kIllegalCid)) {
+        guarded_classes_[i].implementor_cid = implementor_cid;
+      }
+      if (track_future && !guarded_classes_[i].track_future) {
+        guarded_classes_[i].track_future = track_future;
+      }
       return;
     }
   }
   GuardedClassInfo info = {&Class::ZoneHandle(thread_->zone(), cls.ptr()),
-                           subclass_count, kIllegalCid};
+                           subclass_count, implementor_cid, track_future};
   guarded_classes_.Add(info);
 }
 
-void CHA::AddToGuardedInterfaces(const Class& cls, intptr_t implementor_cid) {
+void CHA::AddToGuardedClassesForSubclassCount(const Class& cls,
+                                              intptr_t subclass_count) {
+  ASSERT(subclass_count >= 0);
+  AddToGuardedClasses(cls, subclass_count, kIllegalCid, false);
+}
+
+void CHA::AddToGuardedClassesForImplementorCid(const Class& cls,
+                                               intptr_t implementor_cid) {
   ASSERT(implementor_cid != kIllegalCid);
   ASSERT(implementor_cid != kDynamicCid);
-  for (intptr_t i = 0; i < guarded_classes_.length(); i++) {
-    if (guarded_classes_[i].cls->ptr() == cls.ptr()) {
-      // Was added as a subclass guard.
-      if (guarded_classes_[i].implementor_cid == kIllegalCid) {
-        guarded_classes_[i].implementor_cid = implementor_cid;
-      }
-      return;
-    }
-  }
-  GuardedClassInfo info = {&Class::ZoneHandle(thread_->zone(), cls.ptr()), -1,
-                           implementor_cid};
-  guarded_classes_.Add(info);
+  AddToGuardedClasses(cls, -1, implementor_cid, false);
+}
+
+void CHA::AddToGuardedClassesToTrackFuture(const Class& cls) {
+  AddToGuardedClasses(cls, -1, kIllegalCid, true);
 }
 
 bool CHA::IsGuardedClass(intptr_t cid) const {
@@ -133,7 +141,7 @@ bool CHA::HasSingleConcreteImplementation(const Class& interface,
     }
     if (FLAG_use_cha_deopt) {
       CHA& cha = thread->compiler_state().cha();
-      cha.AddToGuardedInterfaces(interface, cid);
+      cha.AddToGuardedClassesForImplementorCid(interface, cid);
     }
     *implementation_cid = cid;
     return true;
@@ -143,6 +151,29 @@ bool CHA::HasSingleConcreteImplementation(const Class& interface,
   }
 }
 
+bool CHA::ClassCanBeFuture(const Class& cls) {
+  if (cls.can_be_future()) {
+    return true;
+  }
+
+  // Class cannot be Future with the current set of
+  // finalized classes. However, as new classes are loaded
+  // and finalized, there could be a new subtype of [cls]
+  // which is also a subtype of Future.
+  // We should deoptimize in such cases.
+  Thread* thread = Thread::Current();
+  if (FLAG_use_cha_deopt || thread->isolate_group()->all_classes_finalized()) {
+    if (FLAG_use_cha_deopt) {
+      CHA& cha = thread->compiler_state().cha();
+      cha.AddToGuardedClassesToTrackFuture(cls);
+    }
+    return false;
+  }
+
+  // Conservatively assume that class can be Future.
+  return true;
+}
+
 static intptr_t CountFinalizedSubclasses(Thread* thread, const Class& cls) {
   intptr_t count = 0;
   const GrowableObjectArray& cls_direct_subclasses =
@@ -178,6 +209,11 @@ bool CHA::IsConsistentWithCurrentHierarchy() const {
         return false;  // New implementor appeared during compilation.
       }
     }
+    if (guarded_classes_[i].track_future) {
+      if (guarded_classes_[i].cls->can_be_future()) {
+        return false;
+      }
+    }
   }
   return true;
 }

runtime/vm/compiler/cha.h

@@ -46,6 +46,10 @@ class CHA : public ValueObject {
   static bool HasSingleConcreteImplementation(const Class& interface,
                                               intptr_t* implementation_cid);
 
+  // Return true if variable of static type based on [cls] may hold
+  // a Future instance.
+  static bool ClassCanBeFuture(const Class& cls);
+
   // Returns true if any subclass of 'cls' contains the function.
   // If no override was found subclass_count would contain total count of
   // finalized subclasses that CHA looked at.
@@ -55,12 +59,15 @@ class CHA : public ValueObject {
                    const String& function_name,
                    intptr_t* subclass_count);
 
-  // Adds class 'cls' to the list of guarded classes / interfaces.
+  // Adds class 'cls' to the list of guarded classes.
   // Deoptimization occurs if any of those classes gets subclassed or
   // implemented through later loaded/finalized libraries. Only classes that
   // were used for CHA optimizations are added.
-  void AddToGuardedClasses(const Class& cls, intptr_t subclass_count);
-  void AddToGuardedInterfaces(const Class& cls, intptr_t implementor_cid);
+  void AddToGuardedClassesForSubclassCount(const Class& cls,
+                                           intptr_t subclass_count);
+  void AddToGuardedClassesForImplementorCid(const Class& cls,
+                                            intptr_t implementor_cid);
+  void AddToGuardedClassesToTrackFuture(const Class& cls);
 
   // When compiling in background we need to check that no new finalized
   // subclasses were added to guarded classes.
@@ -72,6 +79,11 @@ class CHA : public ValueObject {
   bool IsGuardedClass(intptr_t cid) const;
 
  private:
+  void AddToGuardedClasses(const Class& cls,
+                           intptr_t subclass_count,
+                           intptr_t implementor_cid,
+                           bool track_future);
+
   Thread* thread_;
 
   struct GuardedClassInfo {
@@ -88,6 +100,9 @@ class CHA : public ValueObject {
     // Used to validate correctness of background compilation: if
     // any implementors were added we will discard compiled code.
     intptr_t implementor_cid;
+
+    // Whether we should track that this class cannot be future.
+    bool track_future;
   };
 
   GrowableArray<GuardedClassInfo> guarded_classes_;

runtime/vm/compiler/cha_test.cc

@@ -89,9 +89,9 @@ TEST_CASE(ClassHierarchyAnalysis) {
   EXPECT(CHA::HasSubclasses(class_a));
   EXPECT(CHA::HasSubclasses(class_b));
   EXPECT(!CHA::HasSubclasses(class_c));
-  cha.AddToGuardedClasses(class_c, /*subclass_count=*/0);
+  cha.AddToGuardedClassesForSubclassCount(class_c, /*subclass_count=*/0);
   EXPECT(!CHA::HasSubclasses(class_d));
-  cha.AddToGuardedClasses(class_d, /*subclass_count=*/0);
+  cha.AddToGuardedClassesForSubclassCount(class_d, /*subclass_count=*/0);
 
   EXPECT(!cha.IsGuardedClass(class_a.id()));
   EXPECT(!cha.IsGuardedClass(class_b.id()));

runtime/vm/object.h

@@ -2040,6 +2040,7 @@ class Class : public Object {
 
   void set_is_future_subtype(bool value) const;
   bool is_future_subtype() const {
+    ASSERT(is_type_finalized());
     return IsFutureSubtypeBit::decode(state_bits());
   }