From ef168de0007f2ced45cd514f82a357eb89cad5a9 Mon Sep 17 00:00:00 2001 From: Jens Johansen Date: Thu, 28 Mar 2019 08:36:51 +0000 Subject: [PATCH] [vm/kernel] Index all source and line starts before using them (2nd try) By indexing all sources in a concatenated dill file and then using that index to find the sources and line starts we can fix missing information (leading to crashes when collecting coverage) in for instance circular instances. This reverts commit 57321c1590fae672cc5993a11d422a0bd7724df6 and adds a fix. Change-Id: I7a91801fb318cad0218cf3101d6a15f1ec929175 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/98006 Commit-Queue: Jens Johansen Reviewed-by: Vyacheslav Egorov --- pkg/vm/test/incremental_compiler_test.dart | 2 +- runtime/vm/bootstrap.cc | 2 +- runtime/vm/kernel_loader.cc | 123 +++++++++++++++------ runtime/vm/kernel_loader.h | 36 +++++- runtime/vm/object.cc | 2 +- 5 files changed, 128 insertions(+), 37 deletions(-) diff --git a/pkg/vm/test/incremental_compiler_test.dart b/pkg/vm/test/incremental_compiler_test.dart index 6e0fa6fa786..a280c939464 100644 --- a/pkg/vm/test/incremental_compiler_test.dart +++ b/pkg/vm/test/incremental_compiler_test.dart @@ -437,7 +437,7 @@ main() { }); await portLineCompleter.future; print("Compiler terminated with ${await vm.exitCode} exit code"); - }, skip: "Currently fails"); + }); }); group('reload', () { diff --git a/runtime/vm/bootstrap.cc b/runtime/vm/bootstrap.cc index fc59b64b527..8c05417bd18 100644 --- a/runtime/vm/bootstrap.cc +++ b/runtime/vm/bootstrap.cc @@ -89,7 +89,7 @@ static RawError* BootstrapFromKernel(Thread* thread, const String& msg = String::Handle(String::New(message_buffer, Heap::kOld)); return ApiError::New(msg, Heap::kOld); } - kernel::KernelLoader loader(program); + kernel::KernelLoader loader(program, /*uri_to_source_table=*/nullptr); Isolate* isolate = thread->isolate(); diff --git a/runtime/vm/kernel_loader.cc b/runtime/vm/kernel_loader.cc index fdc73910f49..b0183c20c8d 100644 --- a/runtime/vm/kernel_loader.cc +++ b/runtime/vm/kernel_loader.cc @@ -175,7 +175,10 @@ void ClassIndex::Init(intptr_t class_offset, intptr_t class_size) { class_offset + class_size - 4 - (procedure_count_ + 1) * 4; } -KernelLoader::KernelLoader(Program* program) +using UriToSourceTable = DirectChainedHashMap; + +KernelLoader::KernelLoader(Program* program, + UriToSourceTable* uri_to_source_table) : program_(program), thread_(Thread::Current()), zone_(thread_->zone()), @@ -209,7 +212,7 @@ KernelLoader::KernelLoader(Program* program) "Trying to load a concatenated dill file at a time where that is " "not allowed"); } - InitializeFields(); + InitializeFields(uri_to_source_table); } void KernelLoader::ReadObfuscationProhibitions() { @@ -223,7 +226,7 @@ Object& KernelLoader::LoadEntireProgram(Program* program, TIMELINE_DURATION(thread, Isolate, "LoadKernel"); if (program->is_single_program()) { - KernelLoader loader(program); + KernelLoader loader(program, /*uri_to_source_table=*/nullptr); return Object::Handle(loader.LoadProgram(process_pending_classes)); } @@ -233,8 +236,50 @@ Object& KernelLoader::LoadEntireProgram(Program* program, Zone* zone = thread->zone(); Library& library = Library::Handle(zone); - // Create "fake programs" for each sub-program. intptr_t subprogram_count = subprogram_file_starts.length() - 1; + + // First index all source tables. + UriToSourceTable uri_to_source_table; + UriToSourceTableEntry wrapper; + for (intptr_t i = subprogram_count - 1; i >= 0; --i) { + intptr_t subprogram_start = subprogram_file_starts.At(i); + intptr_t subprogram_end = subprogram_file_starts.At(i + 1); + Thread* thread_ = Thread::Current(); + Zone* zone_ = thread_->zone(); + TranslationHelper translation_helper(thread); + KernelReaderHelper helper_(zone_, &translation_helper, + program->kernel_data() + subprogram_start, + subprogram_end - subprogram_start, 0); + const intptr_t source_table_size = helper_.SourceTableSize(); + for (intptr_t index = 0; index < source_table_size; ++index) { + const String& uri_string = helper_.SourceTableUriFor(index); + wrapper.uri = &uri_string; + TypedData& line_starts = + TypedData::Handle(Z, helper_.GetLineStartsFor(index)); + if (line_starts.Length() == 0) continue; + const String& script_source = helper_.GetSourceFor(index); + wrapper.uri = &uri_string; + UriToSourceTableEntry* pair = uri_to_source_table.LookupValue(&wrapper); + if (pair != NULL) { + // At least two entries with content. Unless the content is the same + // that's not valid. + if (pair->sources->CompareTo(script_source) != 0 || + !pair->line_starts->CanonicalizeEquals(line_starts)) { + FATAL( + "Invalid kernel binary: Contains at least two source entries " + "that do not agree."); + } + } else { + UriToSourceTableEntry* tmp = new UriToSourceTableEntry(); + tmp->uri = &uri_string; + tmp->sources = &script_source; + tmp->line_starts = &line_starts; + uri_to_source_table.Insert(tmp); + } + } + } + + // Create "fake programs" for each sub-program. for (intptr_t i = subprogram_count - 1; i >= 0; --i) { intptr_t subprogram_start = subprogram_file_starts.At(i); intptr_t subprogram_end = subprogram_file_starts.At(i + 1); @@ -243,7 +288,7 @@ Object& KernelLoader::LoadEntireProgram(Program* program, reader.set_offset(0); Program* subprogram = Program::ReadFrom(&reader); ASSERT(subprogram->is_single_program()); - KernelLoader loader(subprogram); + KernelLoader loader(subprogram, &uri_to_source_table); Object& load_result = Object::Handle(loader.LoadProgram(false)); if (load_result.IsError()) return load_result; @@ -301,7 +346,7 @@ RawString* KernelLoader::FindSourceForScript(const uint8_t* kernel_buffer, return String::null(); } -void KernelLoader::InitializeFields() { +void KernelLoader::InitializeFields(UriToSourceTable* uri_to_source_table) { const intptr_t source_table_size = helper_.SourceTableSize(); const Array& scripts = Array::Handle(Z, Array::New(source_table_size, Heap::kOld)); @@ -370,7 +415,7 @@ void KernelLoader::InitializeFields() { Script& script = Script::Handle(Z); for (intptr_t index = 0; index < source_table_size; ++index) { - script = LoadScriptAt(index); + script = LoadScriptAt(index, uri_to_source_table); scripts.SetAt(index, script); } @@ -813,7 +858,7 @@ void KernelLoader::FindModifiedLibraries(Program* program, // kernel files, these will constitute the modified libraries. *is_empty_program = true; if (program->is_single_program()) { - KernelLoader loader(program); + KernelLoader loader(program, /*uri_to_source_table=*/nullptr); loader.walk_incremental_kernel(modified_libs, is_empty_program, p_num_classes, p_num_procedures); } @@ -831,7 +876,7 @@ void KernelLoader::FindModifiedLibraries(Program* program, reader.set_offset(0); Program* subprogram = Program::ReadFrom(&reader); ASSERT(subprogram->is_single_program()); - KernelLoader loader(subprogram); + KernelLoader loader(subprogram, /*uri_to_source_table=*/nullptr); loader.walk_incremental_kernel(modified_libs, is_empty_program, p_num_classes, p_num_procedures); delete subprogram; @@ -1977,33 +2022,49 @@ const Object& KernelLoader::ClassForScriptAt(const Class& klass, return klass; } -RawScript* KernelLoader::LoadScriptAt(intptr_t index) { +RawScript* KernelLoader::LoadScriptAt(intptr_t index, + UriToSourceTable* uri_to_source_table) { const String& uri_string = helper_.SourceTableUriFor(index); const String& import_uri_string = helper_.SourceTableImportUriFor(index, program_->binary_version()); - const String& script_source = helper_.GetSourceFor(index); + String& sources = String::Handle(Z); - TypedData& line_starts = - TypedData::Handle(Z, helper_.GetLineStartsFor(index)); - if (script_source.raw() == Symbols::Empty().raw() && - line_starts.Length() == 0 && uri_string.Length() > 0) { - // Entry included only to provide URI - actual source should already exist - // in the VM, so try to find it. - Library& lib = Library::Handle(Z); - Script& script = Script::Handle(Z); - const GrowableObjectArray& libs = - GrowableObjectArray::Handle(isolate_->object_store()->libraries()); - for (intptr_t i = 0; i < libs.Length(); i++) { - lib ^= libs.At(i); - script = lib.LookupScript(uri_string, /* useResolvedUri = */ true); - if (!script.IsNull() && script.kind() == RawScript::kKernelTag) { - sources ^= script.Source(); - line_starts ^= script.line_starts(); - break; - } + TypedData& line_starts = TypedData::Handle(Z); + + if (uri_to_source_table != nullptr) { + UriToSourceTableEntry wrapper; + wrapper.uri = &uri_string; + UriToSourceTableEntry* pair = uri_to_source_table->LookupValue(&wrapper); + if (pair != nullptr) { + sources ^= pair->sources->raw(); + line_starts ^= pair->line_starts->raw(); + } + } + + if (sources.IsNull() || line_starts.IsNull()) { + const String& script_source = helper_.GetSourceFor(index); + line_starts ^= helper_.GetLineStartsFor(index); + + if (script_source.raw() == Symbols::Empty().raw() && + line_starts.Length() == 0 && uri_string.Length() > 0) { + // Entry included only to provide URI - actual source should already exist + // in the VM, so try to find it. + Library& lib = Library::Handle(Z); + Script& script = Script::Handle(Z); + const GrowableObjectArray& libs = + GrowableObjectArray::Handle(isolate_->object_store()->libraries()); + for (intptr_t i = 0; i < libs.Length(); i++) { + lib ^= libs.At(i); + script = lib.LookupScript(uri_string, /* useResolvedUri = */ true); + if (!script.IsNull() && script.kind() == RawScript::kKernelTag) { + sources ^= script.Source(); + line_starts ^= script.line_starts(); + break; + } + } + } else { + sources = script_source.raw(); } - } else { - sources = script_source.raw(); } const Script& script = diff --git a/runtime/vm/kernel_loader.h b/runtime/vm/kernel_loader.h index 09693c84074..4e64fcf6359 100644 --- a/runtime/vm/kernel_loader.h +++ b/runtime/vm/kernel_loader.h @@ -147,9 +147,36 @@ class ClassIndex { DISALLOW_COPY_AND_ASSIGN(ClassIndex); }; +struct UriToSourceTableEntry : public ZoneAllocated { + UriToSourceTableEntry() {} + + const String* uri = nullptr; + const String* sources = nullptr; + const TypedData* line_starts = nullptr; +}; + +struct UriToSourceTableTrait { + typedef UriToSourceTableEntry* Value; + typedef const UriToSourceTableEntry* Key; + typedef UriToSourceTableEntry* Pair; + + static Key KeyOf(Pair kv) { return kv; } + + static Value ValueOf(Pair kv) { return kv; } + + static inline intptr_t Hashcode(Key key) { return key->uri->Hash(); } + + static inline bool IsKeyEqual(Pair kv, Key key) { + // Only compare uri. + return kv->uri->CompareTo(*key->uri) == 0; + } +}; + class KernelLoader : public ValueObject { public: - explicit KernelLoader(Program* program); + explicit KernelLoader( + Program* program, + DirectChainedHashMap* uri_to_source_table); static Object& LoadEntireProgram(Program* program, bool process_pending_classes = true); @@ -252,7 +279,8 @@ class KernelLoader : public ValueObject { const ExternalTypedData& kernel_data, intptr_t data_program_offset); - void InitializeFields(); + void InitializeFields( + DirectChainedHashMap* uri_to_source_table); static void index_programs(kernel::Reader* reader, GrowableArray* subprogram_file_starts); void walk_incremental_kernel(BitVector* modified_libs, @@ -288,7 +316,9 @@ class KernelLoader : public ValueObject { RawArray* MakeFieldsArray(); RawArray* MakeFunctionsArray(); - RawScript* LoadScriptAt(intptr_t index); + RawScript* LoadScriptAt( + intptr_t index, + DirectChainedHashMap* uri_to_source_table); // If klass's script is not the script at the uri index, return a PatchClass // for klass whose script corresponds to the uri index. diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index 632465d3a92..32a74ef24c8 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -11228,7 +11228,7 @@ static RawObject* EvaluateCompiledExpressionHelper( String::New("Kernel isolate returned ill-formed kernel."))); } - kernel::KernelLoader loader(kernel_pgm); + kernel::KernelLoader loader(kernel_pgm, /*uri_to_source_table=*/nullptr); const Object& result = Object::Handle( loader.LoadExpressionEvaluationFunction(library_url, klass));