[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 flutter/flutter#20255
Issue flutter/flutter#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>

Author: mkustermann

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_;