From d0f28884ff554b53326e85c7aa4f18168fcf7ab6 Mon Sep 17 00:00:00 2001 From: Martin Kustermann Date: Thu, 9 Aug 2018 15:31:15 +0000 Subject: [PATCH] [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 Reviewed-by: Vyacheslav Egorov --- .../eval_regression_flutter20255_test.dart | 111 ++++++++++++++++ .../tests/service/service_kernel.status | 1 + runtime/vm/kernel_loader.cc | 124 +++++++++++++++--- runtime/vm/kernel_loader.h | 32 +++++ runtime/vm/object.cc | 71 ++-------- runtime/vm/object.h | 3 +- 6 files changed, 263 insertions(+), 79 deletions(-) create mode 100644 runtime/observatory/tests/service/eval_regression_flutter20255_test.dart diff --git a/runtime/observatory/tests/service/eval_regression_flutter20255_test.dart b/runtime/observatory/tests/service/eval_regression_flutter20255_test.dart new file mode 100644 index 00000000000..806aa0ab3e6 --- /dev/null +++ b/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 { + String field; + + Base(this.field); + String foo() => 'Base-$field'; +} + +class Sub extends Base { + String field; + + Sub(this.field) : super(field); + String foo() { + debugger(); + return 'Sub-$field'; + } +} + +class ISub implements Base { + String field; + + ISub(this.field); + String foo() => 'ISub-$field'; +} + +class Box { + T value; + + @NeverInline + void setValue(T value) { + this.value = value; + } +} + +final objects = [ + new Base('b'), + new Sub('a'), + new ISub('c') +]; + +String triggerTypeTestingStubGeneration() { + final Box box = new Box(); + 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], which will make a subclass of [Base]. + print(objects[1].foo()); + + triggerTypeTestingStubGeneration(); + + // Triggers the debugger, which will evaluate an expression in the context of + // [Sub], which will make a subclass of [Base]. + 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. + 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 = [ + hasStoppedAtBreakpoint, + triggerEvaluation, + resumeIsolate, + hasStoppedAtBreakpoint, + triggerEvaluation, + resumeIsolate, +]; + +main(args) => runIsolateTests(args, testSteps, testeeConcurrent: testFunction); diff --git a/runtime/observatory/tests/service/service_kernel.status b/runtime/observatory/tests/service/service_kernel.status index 11ea88f0f12..7ca721d7d91 100644 --- a/runtime/observatory/tests/service/service_kernel.status +++ b/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 diff --git a/runtime/vm/kernel_loader.cc b/runtime/vm/kernel_loader.cc index cc3ad47cd8d..df6ceafce78 100644 --- a/runtime/vm/kernel_loader.cc +++ b/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 diff --git a/runtime/vm/kernel_loader.h b/runtime/vm/kernel_loader.h index 7b77dd90cae..6aa798db0d0 100644 --- a/runtime/vm/kernel_loader.h +++ b/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 libraries_; Mapping 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 functions_; GrowableArray fields_; diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index dbefe80e3fd..9a289d4faa1 100644 --- a/runtime/vm/object.cc +++ b/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(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); diff --git a/runtime/vm/object.h b/runtime/vm/object.h index 7635773f5fc..fd8ea19814b 100644 --- a/runtime/vm/object.h +++ b/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);