[VM] Fix expression evaluation implementation: Never register temporary/unused classes with the system.

Until now the expression evaluation implementation was using normal kernel loader
functionality, which registered a new class with the isolate. Then the cid of that
class was set to kIllegalCid.

This caused the direct_subclasses/direct_implementors CHA information to contain a
class with kIllegalCid.

This CL fixes this by ensuring we never register the libraries/classes
created for expression evaluation (which are not even used, they are an artifact
of how the expressions are encoded in kernel).

Issue https://github.com/flutter/flutter/issues/20255
Issue https://github.com/flutter/flutter/issues/20307

Change-Id: Ie6dd76c7ff696cd8adf4f27e9a072274afd90136
Reviewed-on: https://dart-review.googlesource.com/68681
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>

runtime/observatory/tests/service/eval_regression_flutter20255_test.dart

@@ -0,0 +1,111 @@
+// Copyright (c) 2018, 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.
+// VMOptions=--no-background-compilation --enable-inlining-annotations
+
+import 'dart:async';
+import 'dart:developer';
+
+import 'package:observatory/service_io.dart';
+import 'package:unittest/unittest.dart';
+
+import 'service_test_common.dart';
+import 'test_helper.dart';
+
+const String NeverInline = 'NeverInline';
+
+class Base<T> {
+  String field;
+
+  Base(this.field);
+  String foo() => 'Base-$field';
+}
+
+class Sub<T> extends Base<T> {
+  String field;
+
+  Sub(this.field) : super(field);
+  String foo() {
+    debugger();
+    return 'Sub-$field';
+  }
+}
+
+class ISub<T> implements Base<T> {
+  String field;
+
+  ISub(this.field);
+  String foo() => 'ISub-$field';
+}
+
+class Box<T> {
+  T value;
+
+  @NeverInline
+  void setValue(T value) {
+    this.value = value;
+  }
+}
+
+final objects = <Base>[
+  new Base<int>('b'),
+  new Sub<double>('a'),
+  new ISub<bool>('c')
+];
+
+String triggerTypeTestingStubGeneration() {
+  final Box<Object> box = new Box<Base>();
+  for (int i = 0; i < 1000000; ++i) {
+    box.setValue(objects.last);
+  }
+  return 'tts-generated';
+}
+
+void testFunction() {
+  // Triggers the debugger, which will evaluate an expression in the context of
+  // [Sub<double>], which will make a subclass of [Base<T>].
+  print(objects[1].foo());
+
+  triggerTypeTestingStubGeneration();
+
+  // Triggers the debugger, which will evaluate an expression in the context of
+  // [Sub<double>], which will make a subclass of [Base<T>].
+  print(objects[1].foo());
+}
+
+Future triggerEvaluation(Isolate isolate) async {
+  ServiceMap stack = await isolate.getStack();
+
+  // Make sure we are in the right place.
+  expect(stack.type, equals('Stack'));
+  expect(stack['frames'].length, greaterThanOrEqualTo(2));
+  expect(stack['frames'][0].function.name, equals('foo'));
+  expect(stack['frames'][0].function.dartOwner.name, equals('Sub'));
+
+  // Trigger an evaluation, which will create a new subclass of Base<T>.
+  final dynamic result =
+      await isolate.evalFrame(0, 'this.field + " world \$T"');
+  if (result is DartError) {
+    throw 'Got an error "$result", expected result of expression evaluation.';
+  }
+  expect(result.valueAsString, equals('a world double'));
+
+  // Trigger an optimization of a type testing stub (and usage of it).
+  final dynamic result2 =
+      await isolate.evalFrame(0, 'triggerTypeTestingStubGeneration()');
+  if (result2 is DartError) {
+    throw 'Got an error "$result", expected result of expression evaluation.';
+  }
+  expect(result2.valueAsString, equals('tts-generated'));
+}
+
+final testSteps = <IsolateTest>[
+  hasStoppedAtBreakpoint,
+  triggerEvaluation,
+  resumeIsolate,
+  hasStoppedAtBreakpoint,
+  triggerEvaluation,
+  resumeIsolate,
+];
+
+main(args) => runIsolateTests(args, testSteps, testeeConcurrent: testFunction);

runtime/observatory/tests/service/service_kernel.status

@@ -227,6 +227,7 @@ complex_reload_test: Skip # Times out on sim architectures, also RuntimeError.
 debugger_inspect_test: RuntimeError, Timeout # Issue #33087
 developer_service_get_isolate_id_test: RuntimeError # Issue #33087
 eval_internal_class_test: RuntimeError # Issue #33087
+eval_regression_flutter20255_test: RuntimeError # "No incremental compiler available for this isolate"
 eval_test: RuntimeError # Issue #33087
 evaluate_activation_test/none: RuntimeError # Issue #33087
 evaluate_async_closure_test: RuntimeError # Issue #33087

runtime/vm/kernel_loader.cc

@@ -183,7 +183,9 @@ KernelLoader::KernelLoader(Program* program)
       external_name_field_(Field::Handle(Z)),
       potential_natives_(GrowableObjectArray::Handle(Z)),
       potential_extension_libraries_(GrowableObjectArray::Handle(Z)),
-      pragma_class_(Class::Handle(Z)) {
+      pragma_class_(Class::Handle(Z)),
+      expression_evaluation_library_(Library::Handle(Z)),
+      expression_evaluation_function_(Function::Handle(Z)) {
   if (!program->is_single_program()) {
     FATAL(
         "Trying to load a concatenated dill file at a time where that is "
@@ -344,7 +346,9 @@ KernelLoader::KernelLoader(const Script& script,
       external_name_field_(Field::Handle(Z)),
       potential_natives_(GrowableObjectArray::Handle(Z)),
       potential_extension_libraries_(GrowableObjectArray::Handle(Z)),
-      pragma_class_(Class::Handle(Z)) {
+      pragma_class_(Class::Handle(Z)),
+      expression_evaluation_library_(Library::Handle(Z)),
+      expression_evaluation_function_(Function::Handle(Z)) {
   ASSERT(T.active_class_ == &active_class_);
   T.finalize_ = false;
 
@@ -617,6 +621,49 @@ RawObject* KernelLoader::LoadProgram(bool process_pending_classes) {
   return error;
 }
 
+RawObject* KernelLoader::LoadExpressionEvaluationFunction(
+    const String& library_url,
+    const String& klass) {
+  // Find the original context, i.e. library/class, in which the evaluation will
+  // happen.
+  const Library& real_library = Library::Handle(
+      Z, Library::LookupLibrary(Thread::Current(), library_url));
+  ASSERT(!real_library.IsNull());
+  const Class& real_class = Class::Handle(
+      Z, klass.IsNull() ? real_library.toplevel_class()
+                        : real_library.LookupClassAllowPrivate(klass));
+  ASSERT(!real_class.IsNull());
+
+  const intptr_t num_cids = I->class_table()->NumCids();
+  const intptr_t num_libs =
+      GrowableObjectArray::Handle(I->object_store()->libraries()).Length();
+
+  // Load the "evaluate:source" expression evaluation library.
+  ASSERT(expression_evaluation_library_.IsNull());
+  ASSERT(expression_evaluation_function_.IsNull());
+  const Object& result = Object::Handle(Z, LoadProgram(true));
+  if (result.IsError()) {
+    return result.raw();
+  }
+  ASSERT(!expression_evaluation_library_.IsNull());
+  ASSERT(!expression_evaluation_function_.IsNull());
+  ASSERT(GrowableObjectArray::Handle(I->object_store()->libraries()).Length() ==
+         num_libs);
+  ASSERT(I->class_table()->NumCids() == num_cids);
+
+  // Make the expression evaluation function have the right kernel data and
+  // parent.
+  auto& eval_data = ExternalTypedData::Handle(
+      Z, expression_evaluation_library_.kernel_data());
+  auto& eval_script =
+      Script::Handle(Z, expression_evaluation_function_.script());
+  expression_evaluation_function_.SetKernelDataAndScript(
+      eval_script, eval_data, expression_evaluation_library_.kernel_offset());
+  expression_evaluation_function_.set_owner(real_class);
+
+  return expression_evaluation_function_.raw();
+}
+
 void KernelLoader::FindModifiedLibraries(Program* program,
                                          Isolate* isolate,
                                          BitVector* modified_libs,
@@ -780,9 +827,16 @@ RawLibrary* KernelLoader::LoadLibrary(intptr_t index) {
   library_helper.SetJustRead(LibraryHelper::kAnnotations);
 
   // Setup toplevel class (which contains library fields/procedures).
+
+  // We do not register expression evaluation classes with the VM:
+  // The expression evaluation functions should be GC-able as soon as
+  // they are not reachable anymore and we never look them up by name.
+  const bool register_class =
+      library.raw() != expression_evaluation_library_.raw();
+
   Class& toplevel_class =
       Class::Handle(Z, Class::New(library, Symbols::TopLevel(), script,
-                                  TokenPosition::kNoSource));
+                                  TokenPosition::kNoSource, register_class));
   toplevel_class.set_is_cycle_free();
   library.set_toplevel_class(toplevel_class);
 
@@ -801,8 +855,10 @@ RawLibrary* KernelLoader::LoadLibrary(intptr_t index) {
   for (intptr_t i = 0; i < class_count; ++i) {
     helper_.SetOffset(next_class_offset);
     next_class_offset = library_index.ClassOffset(i + 1);
-    classes.Add(LoadClass(library, toplevel_class, next_class_offset),
-                Heap::kOld);
+    const Class& klass = LoadClass(library, toplevel_class, next_class_offset);
+    if (register_class) {
+      classes.Add(klass, Heap::kOld);
+    }
   }
   helper_.SetOffset(next_class_offset);
 
@@ -881,7 +937,9 @@ RawLibrary* KernelLoader::LoadLibrary(intptr_t index) {
   }
 
   toplevel_class.SetFunctions(Array::Handle(MakeFunctionsArray()));
-  classes.Add(toplevel_class, Heap::kOld);
+  if (register_class) {
+    classes.Add(toplevel_class, Heap::kOld);
+  }
   if (!library.Loaded()) library.SetLoaded();
 
   return library.raw();
@@ -1161,7 +1219,13 @@ Class& KernelLoader::LoadClass(const Library& library,
                              class_offset - correction_offset_);
   }
 
-  if (loading_native_wrappers_library_) {
+  // We do not register expression evaluation classes with the VM:
+  // The expression evaluation functions should be GC-able as soon as
+  // they are not reachable anymore and we never look them up by name.
+  const bool register_class =
+      library.raw() != expression_evaluation_library_.raw();
+
+  if (loading_native_wrappers_library_ || !register_class) {
     FinishClassLoading(klass, library, toplevel_class, class_offset,
                        class_index, &class_helper);
   }
@@ -1514,6 +1578,12 @@ void KernelLoader::LoadProcedure(const Library& library,
   const Object& script_class =
       ClassForScriptAt(owner, procedure_helper.source_uri_index_);
   RawFunction::Kind kind = GetFunctionType(procedure_helper.kind_);
+
+  // We do not register expression evaluation libraries with the VM:
+  // The expression evaluation functions should be GC-able as soon as
+  // they are not reachable anymore and we never look them up by name.
+  const bool register_function = !name.Equals(Symbols::DebugProcedureName());
+
   Function& function = Function::ZoneHandle(
       Z, Function::New(name, kind,
                        !is_method,  // is_static
@@ -1523,7 +1593,11 @@ void KernelLoader::LoadProcedure(const Library& library,
                        script_class, procedure_helper.start_position_));
   function.set_has_pragma(has_pragma_annotation);
   function.set_end_token_pos(procedure_helper.end_position_);
-  functions_.Add(&function);
+  if (register_function) {
+    functions_.Add(&function);
+  } else {
+    expression_evaluation_function_ = function.raw();
+  }
   function.set_kernel_offset(procedure_offset);
 
   ActiveMemberScope active_member(&active_class_, &function);
@@ -1787,11 +1861,23 @@ Library& KernelLoader::LookupLibraryOrNull(NameIndex library) {
 Library& KernelLoader::LookupLibrary(NameIndex library) {
   Library* handle = NULL;
   if (!libraries_.Lookup(library, &handle)) {
-    const String& url = H.DartString(H.CanonicalNameString(library));
-    handle = &Library::Handle(Z, Library::LookupLibrary(thread_, url));
-    if (handle->IsNull()) {
-      *handle = Library::New(url);
-      handle->Register(thread_);
+    handle = &Library::Handle(Z);
+    const String& url = H.DartSymbolPlain(H.CanonicalNameString(library));
+
+    // We do not register expression evaluation libraries with the VM:
+    // The expression evaluation functions should be GC-able as soon as
+    // they are not reachable anymore and we never look them up by name.
+    if (url.Equals(Symbols::EvalSourceUri())) {
+      if (handle->IsNull()) {
+        *handle = Library::New(url);
+        expression_evaluation_library_ = handle->raw();
+      }
+    } else {
+      *handle = Library::LookupLibrary(thread_, url);
+      if (handle->IsNull()) {
+        *handle = Library::New(url);
+        handle->Register(thread_);
+      }
     }
     ASSERT(!handle->IsNull());
     libraries_.Insert(library, handle);
@@ -1806,9 +1892,17 @@ Class& KernelLoader::LookupClass(NameIndex klass) {
     const String& name = H.DartClassName(klass);
     handle = &Class::Handle(Z, library.LookupLocalClass(name));
     if (handle->IsNull()) {
+      // We do not register expression evaluation classes with the VM:
+      // The expression evaluation functions should be GC-able as soon as
+      // they are not reachable anymore and we never look them up by name.
+      const bool register_class =
+          library.raw() != expression_evaluation_library_.raw();
+
       *handle = Class::New(library, name, Script::Handle(Z),
-                           TokenPosition::kNoSource);
-      library.AddClass(*handle);
+                           TokenPosition::kNoSource, register_class);
+      if (register_class) {
+        library.AddClass(*handle);
+      }
     }
     // Insert the class in the cache before calling ReadPreliminaryClass so
     // we do not risk allocating the class again by calling LookupClass

runtime/vm/kernel_loader.h

@@ -133,6 +133,11 @@ class KernelLoader : public ValueObject {
   // was no main procedure, or a failure object if there was an error.
   RawObject* LoadProgram(bool process_pending_classes = true);
 
+  // Returns the function which will evaluate the expression, or a failure
+  // object if there was an error.
+  RawObject* LoadExpressionEvaluationFunction(const String& library_url,
+                                              const String& klass);
+
   // Finds all libraries that have been modified in this incremental
   // version of the kernel program file.
   static void FindModifiedLibraries(Program* program,
@@ -331,6 +336,33 @@ class KernelLoader : public ValueObject {
   Mapping<Library> libraries_;
   Mapping<Class> classes_;
 
+  // We "re-use" the normal .dill file format for encoding compiled evaluation
+  // expressions from the debugger.  This allows us to also reuse the normal
+  // a) kernel loader b) flow graph building code.  The encoding is either one
+  // of the following two options:
+  //
+  //   * Option a) The expression is evaluated inside an instance method call
+  //               context:
+  //
+  //   Program:
+  //   |> library "evaluate:source"
+  //      |> class "#DebugClass"
+  //         |> procedure ":Eval"
+  //
+  //   * Option b) The expression is evaluated outside an instance method call
+  //               context:
+  //
+  //   Program:
+  //   |> library "evaluate:source"
+  //      |> procedure ":Eval"
+  //
+  // See
+  //   * pkg/front_end/lib/src/fasta/incremental_compiler.dart:compileExpression
+  //   * pkg/front_end/lib/src/fasta/kernel/utils.dart:serializeProcedure
+  //
+  Library& expression_evaluation_library_;
+  Function& expression_evaluation_function_;
+
   GrowableArray<const Function*> functions_;
   GrowableArray<const Field*> fields_;
 

runtime/vm/object.cc

@@ -3529,14 +3529,17 @@ RawClass* Class::New(intptr_t index) {
 RawClass* Class::New(const Library& lib,
                      const String& name,
                      const Script& script,
-                     TokenPosition token_pos) {
+                     TokenPosition token_pos,
+                     bool register_class) {
   Class& result = Class::Handle(NewCommon<Instance>(kIllegalCid));
   result.set_library(lib);
   result.set_name(name);
   result.set_script(script);
   result.set_token_pos(token_pos);
   result.set_kernel_offset(-1);
-  Isolate::Current()->RegisterClass(result);
+  if (register_class) {
+    Isolate::Current()->RegisterClass(result);
+  }
   return result.raw();
 }
 
@@ -11707,9 +11710,6 @@ static RawObject* EvaluateCompiledExpressionHelper(
       String::New("Expression evaluation not available in precompiled mode."));
   return ApiError::New(error_str);
 #else
-  Isolate* I = Isolate::Current();
-  Thread* T = Thread::Current();
-
   kernel::Program* kernel_pgm =
       kernel::Program::ReadFromBuffer(kernel_bytes, kernel_length);
 
@@ -11718,67 +11718,12 @@ static RawObject* EvaluateCompiledExpressionHelper(
         String::New("Kernel isolate returned ill-formed kernel.")));
   }
 
-  Function& callee = Function::Handle();
-  intptr_t num_cids = I->class_table()->NumCids();
-  GrowableObjectArray& libraries =
-      GrowableObjectArray::Handle(T->zone(), I->object_store()->libraries());
-  intptr_t num_libs = libraries.Length();
-
-  // Load the program with the debug procedure as a regular, independent
-  // program.
   kernel::KernelLoader loader(kernel_pgm);
-  const Object& result = Object::Handle(loader.LoadProgram());
+  const Object& result = Object::Handle(
+      loader.LoadExpressionEvaluationFunction(library_url, klass));
   if (result.IsError()) return result.raw();
-  ASSERT(I->class_table()->NumCids() > num_cids &&
-         libraries.Length() == num_libs + 1);
-  const Library& loaded =
-      Library::Handle(Library::LookupLibrary(T, Symbols::EvalSourceUri()));
-  ASSERT(!loaded.IsNull());
 
-  String& debug_name = String::Handle(
-      String::New(Symbols::Symbol(Symbols::kDebugProcedureNameId)));
-  Class& fake_class = Class::Handle();
-  if (!klass.IsNull()) {
-    fake_class = loaded.LookupClass(Symbols::DebugClassName());
-    ASSERT(!fake_class.IsNull());
-    callee = fake_class.LookupFunctionAllowPrivate(debug_name);
-  } else {
-    callee = loaded.LookupFunctionAllowPrivate(debug_name);
-  }
-  ASSERT(!callee.IsNull());
-
-  // Save the loaded library's kernel data to the generic "data" field of the
-  // callee, so it doesn't require access it's parent library during
-  // compilation.
-  callee.SetKernelDataAndScript(Script::Handle(callee.script()),
-                                ExternalTypedData::Handle(loaded.kernel_data()),
-                                loaded.kernel_offset());
-
-  // Reparent the callee to the real enclosing class so we can remove the fake
-  // class and library from the object store.
-  const Library& real_library =
-      Library::Handle(Library::LookupLibrary(T, library_url));
-  ASSERT(!real_library.IsNull());
-  Class& real_class = Class::Handle();
-  if (!klass.IsNull()) {
-    real_class = real_library.LookupClassAllowPrivate(klass);
-  } else {
-    real_class = real_library.toplevel_class();
-  }
-  ASSERT(!real_class.IsNull());
-
-  callee.set_owner(real_class);
-
-  // Unlink the fake library and class from the object store.
-  libraries.SetLength(num_libs);
-  I->class_table()->SetNumCids(num_cids);
-  if (!fake_class.IsNull()) {
-    fake_class.set_id(kIllegalCid);
-  }
-  LibraryLookupMap libraries_map(I->object_store()->libraries_map());
-  bool removed = libraries_map.Remove(Symbols::EvalSourceUri());
-  ASSERT(removed);
-  I->object_store()->set_libraries_map(libraries_map.Release());
+  const Function& callee = Function::Cast(result);
 
   if (type_definitions.Length() == 0) {
     return DartEntry::InvokeFunction(callee, arguments);

runtime/vm/object.h

@@ -1404,7 +1404,8 @@ class Class : public Object {
   static RawClass* New(const Library& lib,
                        const String& name,
                        const Script& script,
-                       TokenPosition token_pos);
+                       TokenPosition token_pos,
+                       bool register_class = true);
   static RawClass* NewNativeWrapper(const Library& library,
                                     const String& name,
                                     int num_fields);