Skip to content

Commit

Permalink
[vm/kernel] Index all source and line starts before using them (2nd try)
Browse files Browse the repository at this point in the history 
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 57321c1 and adds a
fix.

Change-Id: I7a91801fb318cad0218cf3101d6a15f1ec929175
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/98006
Commit-Queue: Jens Johansen <jensj@google.com>
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
  • Loading branch information
@jensjoha
jensjoha authored and commit-bot@chromium.org committed Mar 28, 2019
1 parent add5a27 commit ef168de
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 36 deletions.
2 changes: 1 addition & 1 deletion pkg/vm/test/incremental_compiler_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ main() {
});
await portLineCompleter.future;
print("Compiler terminated with ${await vm.exitCode} exit code");
}, skip: "Currently fails");
});
});

group('reload', () {
Expand Down
2 changes: 1 addition & 1 deletion runtime/vm/bootstrap.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
121 changes: 91 additions & 30 deletions runtime/vm/kernel_loader.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<UriToSourceTableTrait>;

KernelLoader::KernelLoader(Program* program,
UriToSourceTable* uri_to_source_table)
: program_(program),
thread_(Thread::Current()),
zone_(thread_->zone()),
Expand Down Expand Up @@ -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() {
Expand All @@ -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));
}

Expand All @@ -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);
Expand All @@ -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;

Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}
Expand All @@ -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;
Expand Down Expand Up @@ -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 =
Expand Down
36 changes: 33 additions & 3 deletions runtime/vm/kernel_loader.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<UriToSourceTableTrait>* uri_to_source_table);
static Object& LoadEntireProgram(Program* program,
bool process_pending_classes = true);

Expand Down Expand Up @@ -252,7 +279,8 @@ class KernelLoader : public ValueObject {
const ExternalTypedData& kernel_data,
intptr_t data_program_offset);

void InitializeFields();
void InitializeFields(
DirectChainedHashMap<UriToSourceTableTrait>* uri_to_source_table);
static void index_programs(kernel::Reader* reader,
GrowableArray<intptr_t>* subprogram_file_starts);
void walk_incremental_kernel(BitVector* modified_libs,
Expand Down Expand Up @@ -288,7 +316,9 @@ class KernelLoader : public ValueObject {
RawArray* MakeFieldsArray();
RawArray* MakeFunctionsArray();

RawScript* LoadScriptAt(intptr_t index);
RawScript* LoadScriptAt(
intptr_t index,
DirectChainedHashMap<UriToSourceTableTrait>* 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.
Expand Down
2 changes: 1 addition & 1 deletion runtime/vm/object.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand Down

0 comments on commit ef168de

Please sign in to comment.