From 6a7d4e22b5bfaa0fee97e5dae0625cca28ae92ee Mon Sep 17 00:00:00 2001 From: Aske Simon Christensen Date: Thu, 13 Feb 2020 15:32:48 +0000 Subject: [PATCH] [vm/aot] Include entries for null in the dispatch table to avoid check. NullCheck pc descriptors are added to all dispatch table calls where the receiver may be null (and the selector is not one implemented by null). All null entries in the table go to the NullError runtime entry, which reads the NullCheck pc descriptor to get the name of the called member for the error message. Change-Id: I9d2847d0ccdfdb735b06e879916920ec299f39bc Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/134294 Commit-Queue: Aske Simon Christensen Reviewed-by: Alexander Markov Reviewed-by: Martin Kustermann --- pkg/vm/lib/metadata/table_selector.dart | 14 ++++++++--- .../type_flow/table_selector.dart | 11 +++++---- .../type_flow/transformer.dart | 6 +++-- .../vm/compiler/aot/aot_call_specializer.cc | 6 ----- .../compiler/aot/dispatch_table_generator.cc | 24 ++++++++++++------- .../compiler/aot/dispatch_table_generator.h | 20 +++++++++++++--- runtime/vm/compiler/aot/precompiler.cc | 4 ++++ .../compiler/backend/flow_graph_compiler.cc | 17 +++++++++---- .../vm/compiler/backend/flow_graph_compiler.h | 5 ++-- runtime/vm/compiler/backend/il.cc | 14 +++++++---- .../frontend/kernel_translation_helper.cc | 1 + .../frontend/kernel_translation_helper.h | 4 ++++ runtime/vm/compiler/stub_code_compiler_arm.cc | 8 +++++++ .../vm/compiler/stub_code_compiler_arm64.cc | 8 +++++++ .../vm/compiler/stub_code_compiler_ia32.cc | 6 +++++ runtime/vm/compiler/stub_code_compiler_x64.cc | 8 +++++++ runtime/vm/dispatch_table.cc | 7 +++++- runtime/vm/image_snapshot.cc | 2 ++ runtime/vm/object_store.h | 1 + runtime/vm/stub_code_list.h | 1 + 20 files changed, 125 insertions(+), 42 deletions(-) diff --git a/pkg/vm/lib/metadata/table_selector.dart b/pkg/vm/lib/metadata/table_selector.dart index d107a9fba1d..938d5c56457 100644 --- a/pkg/vm/lib/metadata/table_selector.dart +++ b/pkg/vm/lib/metadata/table_selector.dart @@ -8,15 +8,23 @@ import 'procedure_attributes.dart'; // Information associated with a selector, used by the dispatch table generator. class TableSelectorInfo { - int callCount; + static const int kCalledOnNullBit = 1 << 0; + static const int kCallCountShift = 1; - TableSelectorInfo() : callCount = 0; + int callCount; + bool calledOnNull; + + TableSelectorInfo() + : callCount = 0, + calledOnNull = false; TableSelectorInfo.readFromBinary(BinarySource source) - : callCount = source.readUInt(); + : callCount = source.readUInt(), + calledOnNull = source.readByte() != 0; void writeToBinary(BinarySink sink) { sink.writeUInt30(callCount); + sink.writeByte(calledOnNull ? 1 : 0); } } diff --git a/pkg/vm/lib/transformations/type_flow/table_selector.dart b/pkg/vm/lib/transformations/type_flow/table_selector.dart index be07bb6b512..2533d4897fd 100644 --- a/pkg/vm/lib/transformations/type_flow/table_selector.dart +++ b/pkg/vm/lib/transformations/type_flow/table_selector.dart @@ -60,15 +60,16 @@ class TableSelectorAssigner { throw "Unexpected member kind '${member.runtimeType}'"; } - void registerCall(int selectorId) { + void registerCall(int selectorId, bool calledOnNull) { metadata.selectors[selectorId].callCount++; + metadata.selectors[selectorId].calledOnNull |= calledOnNull; } - void registerMethodOrSetterCall(Member member) { - registerCall(methodOrSetterSelectorId(member)); + void registerMethodOrSetterCall(Member member, bool calledOnNull) { + registerCall(methodOrSetterSelectorId(member), calledOnNull); } - void registerGetterCall(Member member) { - registerCall(getterSelectorId(member)); + void registerGetterCall(Member member, bool calledOnNull) { + registerCall(getterSelectorId(member), calledOnNull); } } diff --git a/pkg/vm/lib/transformations/type_flow/transformer.dart b/pkg/vm/lib/transformations/type_flow/transformer.dart index 46a7df53b9d..37622d256bc 100644 --- a/pkg/vm/lib/transformations/type_flow/transformer.dart +++ b/pkg/vm/lib/transformations/type_flow/transformer.dart @@ -263,10 +263,12 @@ class AnnotateKernel extends RecursiveVisitor { final Selector selector = callSite.selector; if (selector is InterfaceSelector && !_callSiteUsesDirectCall(node)) { if (node is PropertyGet) { - _tableSelectorAssigner.registerGetterCall(selector.member); + _tableSelectorAssigner.registerGetterCall( + selector.member, callSite.isNullableReceiver); } else { assertx(node is MethodInvocation || node is PropertySet); - _tableSelectorAssigner.registerMethodOrSetterCall(selector.member); + _tableSelectorAssigner.registerMethodOrSetterCall( + selector.member, callSite.isNullableReceiver); } } } diff --git a/runtime/vm/compiler/aot/aot_call_specializer.cc b/runtime/vm/compiler/aot/aot_call_specializer.cc index 9e52a228ade..00e1c476ab7 100644 --- a/runtime/vm/compiler/aot/aot_call_specializer.cc +++ b/runtime/vm/compiler/aot/aot_call_specializer.cc @@ -1303,12 +1303,6 @@ void AotCallSpecializer::TryReplaceWithDispatchTableCall( return; } - if (!selector->on_null_interface) { - // Selector not implemented by Null. Add null check if receiver is nullable. - AddCheckNull(receiver->CopyWithType(Z), call->function_name(), - DeoptId::kNone, call->env(), call); - } - const AbstractType& target_type = AbstractType::Handle(Class::Handle(interface_target.Owner()).RareType()); const bool receiver_can_be_smi = diff --git a/runtime/vm/compiler/aot/dispatch_table_generator.cc b/runtime/vm/compiler/aot/dispatch_table_generator.cc index f89b3017105..7ce4ea6f4ca 100644 --- a/runtime/vm/compiler/aot/dispatch_table_generator.cc +++ b/runtime/vm/compiler/aot/dispatch_table_generator.cc @@ -109,7 +109,7 @@ class SelectorRow { void DefineSelectorImplementationForInterval(classid_t cid, int16_t depth, const Interval& range, - const Function& function); + const Function* function); bool Finalize(); int32_t CallCount() const { return selector_->call_count; } @@ -168,8 +168,8 @@ void SelectorRow::DefineSelectorImplementationForInterval( classid_t cid, int16_t depth, const Interval& range, - const Function& function) { - CidInterval cid_range(cid, depth, range, &function); + const Function* function) { + CidInterval cid_range(cid, depth, range, function); class_ranges_.Add(cid_range); } @@ -251,7 +251,7 @@ void SelectorRow::FillTable(ClassTable* class_table, DispatchTable* table) { const CidInterval& cid_range = class_ranges_[i]; const Interval& range = cid_range.range(); const Function* function = cid_range.function(); - if (function->HasCode()) { + if (function != nullptr && function->HasCode()) { code = function->CurrentCode(); for (classid_t cid = range.begin(); cid < range.end(); cid++) { table->SetCodeAt(selector()->offset + cid, code); @@ -378,9 +378,10 @@ const TableSelector* SelectorMap::GetSelector( return selector; } -void SelectorMap::AddSelector(int32_t call_count) { +void SelectorMap::AddSelector(int32_t call_count, bool called_on_null) { const int32_t added_sid = selectors_.length(); - selectors_.Add(TableSelector(added_sid, call_count, kInvalidSelectorOffset)); + selectors_.Add(TableSelector(added_sid, call_count, kInvalidSelectorOffset, + called_on_null)); } void SelectorMap::SetSelectorProperties(int32_t sid, @@ -417,7 +418,7 @@ void DispatchTableGenerator::ReadTableSelectorInfo() { RELEASE_ASSERT(metadata != nullptr); for (intptr_t i = 0; i < metadata->selectors.length(); i++) { const kernel::TableSelectorInfo* info = &metadata->selectors[i]; - selector_map_.AddSelector(info->call_count); + selector_map_.AddSelector(info->call_count, info->called_on_null); } } @@ -533,7 +534,12 @@ void DispatchTableGenerator::SetupSelectorRows() { // Initialize selector rows. SelectorRow* selector_rows = Z->Alloc(num_selectors_); for (intptr_t i = 0; i < num_selectors_; i++) { - new (&selector_rows[i]) SelectorRow(Z, &selector_map_.selectors_[i]); + TableSelector* selector = &selector_map_.selectors_[i]; + new (&selector_rows[i]) SelectorRow(Z, selector); + if (selector->called_on_null && !selector->on_null_interface) { + selector_rows[i].DefineSelectorImplementationForInterval( + kNullCid, 0, Interval(kNullCid, kNullCid + 1), nullptr); + } } // Add implementation intervals to the selector rows for all classes that @@ -559,7 +565,7 @@ void DispatchTableGenerator::SetupSelectorRows() { for (intptr_t i = 0; i < subclasss_cid_ranges.length(); i++) { Interval& subclass_cid_range = subclasss_cid_ranges[i]; selector_rows[sid].DefineSelectorImplementationForInterval( - cid, depth, subclass_cid_range, function_handle); + cid, depth, subclass_cid_range, &function_handle); } } } diff --git a/runtime/vm/compiler/aot/dispatch_table_generator.h b/runtime/vm/compiler/aot/dispatch_table_generator.h index bbd9ac6c211..6cbf816725e 100644 --- a/runtime/vm/compiler/aot/dispatch_table_generator.h +++ b/runtime/vm/compiler/aot/dispatch_table_generator.h @@ -19,15 +19,29 @@ namespace compiler { class SelectorRow; struct TableSelector { - TableSelector(int32_t id, int32_t call_count, int32_t offset) - : id(id), call_count(call_count), offset(offset) {} + TableSelector(int32_t _id, + int32_t _call_count, + int32_t _offset, + bool _called_on_null) + : id(_id), + call_count(_call_count), + offset(_offset), + called_on_null(_called_on_null) {} bool IsUsed() const { return call_count > 0; } + // ID assigned to the selector. int32_t id; + // Number of dispatch table call sites with this selector (conservative: + // number may be bigger, but not smaller, than actual number of call sites). int32_t call_count; + // Table offset assigned to the selector by the dispatch table generator. int32_t offset; + // Are there any call sites with this selector where the receiver may be null? + bool called_on_null; + // Is the selector part of the interface on Null (same as Object)? bool on_null_interface = false; + // Do any targets of this selector assume that an args descriptor is passed? bool requires_args_descriptor = false; }; @@ -46,7 +60,7 @@ class SelectorMap { int32_t SelectorId(const Function& interface_target) const; - void AddSelector(int32_t call_count); + void AddSelector(int32_t call_count, bool called_on_null); void SetSelectorProperties(int32_t sid, bool on_null_interface, bool requires_args_descriptor); diff --git a/runtime/vm/compiler/aot/precompiler.cc b/runtime/vm/compiler/aot/precompiler.cc index 7dffac58ba9..9ea3bd70b17 100644 --- a/runtime/vm/compiler/aot/precompiler.cc +++ b/runtime/vm/compiler/aot/precompiler.cc @@ -292,6 +292,10 @@ void Precompiler::DoCompileAll() { StubCode::GetBuildMethodExtractorStub(global_object_pool_builder()); I->object_store()->set_build_method_extractor_code(stub_code); + stub_code = StubCode::BuildIsolateSpecificDispatchTableNullErrorStub( + global_object_pool_builder()); + I->object_store()->set_dispatch_table_null_error_stub(stub_code); + stub_code = StubCode::BuildIsolateSpecificNullErrorSharedWithFPURegsStub( global_object_pool_builder()); diff --git a/runtime/vm/compiler/backend/flow_graph_compiler.cc b/runtime/vm/compiler/backend/flow_graph_compiler.cc index 46cfcb96592..6675ac63a5c 100644 --- a/runtime/vm/compiler/backend/flow_graph_compiler.cc +++ b/runtime/vm/compiler/backend/flow_graph_compiler.cc @@ -745,11 +745,18 @@ void FlowGraphCompiler::AddCurrentDescriptor(RawPcDescriptors::Kind kind, CurrentTryIndex()); } -void FlowGraphCompiler::AddNullCheck(intptr_t pc_offset, - TokenPosition token_pos, - intptr_t null_check_name_idx) { - code_source_map_builder_->NoteNullCheck(pc_offset, token_pos, - null_check_name_idx); +void FlowGraphCompiler::AddNullCheck(TokenPosition token_pos, + const String& name) { + // If we have DWARF stack traces enabled, the AOT runtime is unable to obtain + // the pool index at runtime. There is therefore no reason to put the name + // into the pool in the first place. + // TODO(dartbug.com/40605): Move this info to the pc descriptors. + if (!FLAG_dwarf_stack_traces) { + const intptr_t name_index = + assembler()->object_pool_builder().FindObject(name); + code_source_map_builder_->NoteNullCheck(assembler()->CodeSize(), token_pos, + name_index); + } } void FlowGraphCompiler::AddPcRelativeCallTarget(const Function& function, diff --git a/runtime/vm/compiler/backend/flow_graph_compiler.h b/runtime/vm/compiler/backend/flow_graph_compiler.h index 5c94d9a8e77..1668630cb1c 100644 --- a/runtime/vm/compiler/backend/flow_graph_compiler.h +++ b/runtime/vm/compiler/backend/flow_graph_compiler.h @@ -819,9 +819,8 @@ class FlowGraphCompiler : public ValueObject { intptr_t try_index, intptr_t yield_index = RawPcDescriptors::kInvalidYieldIndex); - void AddNullCheck(intptr_t pc_offset, - TokenPosition token_pos, - intptr_t null_check_name_idx); + // Add NullCheck information for the current PC. + void AddNullCheck(TokenPosition token_pos, const String& name); void RecordSafepoint(LocationSummary* locs, intptr_t slow_path_argument_count = 0); diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc index 3d228e338ca..77c5ca6a309 100644 --- a/runtime/vm/compiler/backend/il.cc +++ b/runtime/vm/compiler/backend/il.cc @@ -4457,6 +4457,14 @@ void DispatchTableCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) { arguments_descriptor); compiler->EmitCallsiteMetadata(token_pos(), DeoptId::kNone, RawPcDescriptors::kOther, locs()); + if (selector()->called_on_null && !selector()->on_null_interface) { + Value* receiver = ArgumentValueAt(FirstArgIndex()); + if (receiver->Type()->is_nullable()) { + const String& function_name = + String::ZoneHandle(interface_target().name()); + compiler->AddNullCheck(token_pos(), function_name); + } + } __ Drop(ArgumentCount()); compiler->AddDispatchTableCallTarget(selector()); @@ -4862,11 +4870,7 @@ LocationSummary* CheckNullInstr::MakeLocationSummary(Zone* zone, void CheckNullInstr::AddMetadataForRuntimeCall(CheckNullInstr* check_null, FlowGraphCompiler* compiler) { - const String& function_name = check_null->function_name(); - const intptr_t name_index = - compiler->assembler()->object_pool_builder().FindObject(function_name); - compiler->AddNullCheck(compiler->assembler()->CodeSize(), - check_null->token_pos(), name_index); + compiler->AddNullCheck(check_null->token_pos(), check_null->function_name()); } void UnboxInstr::EmitLoadFromBoxWithDeopt(FlowGraphCompiler* compiler) { diff --git a/runtime/vm/compiler/frontend/kernel_translation_helper.cc b/runtime/vm/compiler/frontend/kernel_translation_helper.cc index 4d81b875f32..c401baf2263 100644 --- a/runtime/vm/compiler/frontend/kernel_translation_helper.cc +++ b/runtime/vm/compiler/frontend/kernel_translation_helper.cc @@ -1851,6 +1851,7 @@ TableSelectorMetadata* TableSelectorMetadataHelper::GetTableSelectorMetadata( void TableSelectorMetadataHelper::ReadTableSelectorInfo( TableSelectorInfo* info) { info->call_count = helper_->ReadUInt(); + info->called_on_null = helper_->ReadByte() != 0; } intptr_t KernelReaderHelper::ReaderOffset() const { diff --git a/runtime/vm/compiler/frontend/kernel_translation_helper.h b/runtime/vm/compiler/frontend/kernel_translation_helper.h index d836db9a1d1..e87c35682ea 100644 --- a/runtime/vm/compiler/frontend/kernel_translation_helper.h +++ b/runtime/vm/compiler/frontend/kernel_translation_helper.h @@ -1030,6 +1030,7 @@ class CallSiteAttributesMetadataHelper : public MetadataHelper { // Information about a table selector computed by the TFA. struct TableSelectorInfo { int call_count = 0; + bool called_on_null = true; }; // Collection of table selector information for all selectors in the program. @@ -1055,6 +1056,9 @@ class TableSelectorMetadataHelper : public MetadataHelper { TableSelectorMetadata* GetTableSelectorMetadata(Zone* zone); private: + static const int32_t kCalledOnNullBit = 1 << 0; + static const int32_t kCallCountShift = 1; + void ReadTableSelectorInfo(TableSelectorInfo* info); DISALLOW_COPY_AND_ASSIGN(TableSelectorMetadataHelper); diff --git a/runtime/vm/compiler/stub_code_compiler_arm.cc b/runtime/vm/compiler/stub_code_compiler_arm.cc index 0fdf071ad18..72d69f20913 100644 --- a/runtime/vm/compiler/stub_code_compiler_arm.cc +++ b/runtime/vm/compiler/stub_code_compiler_arm.cc @@ -469,6 +469,14 @@ void StubCodeCompiler::GenerateJITCallbackTrampolines( } #endif // !defined(DART_PRECOMPILER) +void StubCodeCompiler::GenerateDispatchTableNullErrorStub( + Assembler* assembler) { + __ EnterStubFrame(); + __ CallRuntime(kNullErrorRuntimeEntry, /*argument_count=*/0); + // The NullError runtime entry does not return. + __ Breakpoint(); +} + void StubCodeCompiler::GenerateNullErrorSharedWithoutFPURegsStub( Assembler* assembler) { GenerateSharedStub( diff --git a/runtime/vm/compiler/stub_code_compiler_arm64.cc b/runtime/vm/compiler/stub_code_compiler_arm64.cc index bf862d6fd3d..3faa3bbf6a7 100644 --- a/runtime/vm/compiler/stub_code_compiler_arm64.cc +++ b/runtime/vm/compiler/stub_code_compiler_arm64.cc @@ -519,6 +519,14 @@ void StubCodeCompiler::GenerateBuildMethodExtractorStub( __ Ret(); } +void StubCodeCompiler::GenerateDispatchTableNullErrorStub( + Assembler* assembler) { + __ EnterStubFrame(); + __ CallRuntime(kNullErrorRuntimeEntry, /*argument_count=*/0); + // The NullError runtime entry does not return. + __ Breakpoint(); +} + void StubCodeCompiler::GenerateNullErrorSharedWithoutFPURegsStub( Assembler* assembler) { GenerateSharedStub( diff --git a/runtime/vm/compiler/stub_code_compiler_ia32.cc b/runtime/vm/compiler/stub_code_compiler_ia32.cc index 6510cb519eb..f29b2db1890 100644 --- a/runtime/vm/compiler/stub_code_compiler_ia32.cc +++ b/runtime/vm/compiler/stub_code_compiler_ia32.cc @@ -287,6 +287,12 @@ void StubCodeCompiler::GenerateJITCallbackTrampolines( #endif } +void StubCodeCompiler::GenerateDispatchTableNullErrorStub( + Assembler* assembler) { + // Only used in AOT. + __ Breakpoint(); +} + void StubCodeCompiler::GenerateNullErrorSharedWithoutFPURegsStub( Assembler* assembler) { __ Breakpoint(); diff --git a/runtime/vm/compiler/stub_code_compiler_x64.cc b/runtime/vm/compiler/stub_code_compiler_x64.cc index a8cf49ddb9c..4febff1f3ba 100644 --- a/runtime/vm/compiler/stub_code_compiler_x64.cc +++ b/runtime/vm/compiler/stub_code_compiler_x64.cc @@ -453,6 +453,14 @@ void StubCodeCompiler::GenerateBuildMethodExtractorStub( __ Ret(); } +void StubCodeCompiler::GenerateDispatchTableNullErrorStub( + Assembler* assembler) { + __ EnterStubFrame(); + __ CallRuntime(kNullErrorRuntimeEntry, /*argument_count=*/0); + // The NullError runtime entry does not return. + __ Breakpoint(); +} + void StubCodeCompiler::GenerateNullErrorSharedWithoutFPURegsStub( Assembler* assembler) { GenerateSharedStub( diff --git a/runtime/vm/dispatch_table.cc b/runtime/vm/dispatch_table.cc index f766024502b..183ff297b56 100644 --- a/runtime/vm/dispatch_table.cc +++ b/runtime/vm/dispatch_table.cc @@ -7,6 +7,7 @@ #include "vm/clustered_snapshot.h" #include "vm/hash_map.h" #include "vm/object.h" +#include "vm/object_store.h" namespace dart { @@ -122,6 +123,10 @@ DispatchTable* DispatchTable::Deserialize(Deserializer* deserializer, Code& code = Code::Handle(); + code = + deserializer->isolate()->object_store()->dispatch_table_null_error_stub(); + uword null_entry = code.EntryPoint(); + uword value = 0; uword recent[kRecentCount] = {0}; intptr_t recent_index = 0; @@ -132,7 +137,7 @@ DispatchTable* DispatchTable::Deserialize(Deserializer* deserializer, } else { int32_t encoded = deserializer->Read(); if (encoded == 0) { - value = 0; + value = null_entry; } else if (encoded < 0) { intptr_t r = ~encoded; ASSERT(r < kRecentCount); diff --git a/runtime/vm/image_snapshot.cc b/runtime/vm/image_snapshot.cc index 6e04ec35c76..b8afa524147 100644 --- a/runtime/vm/image_snapshot.cc +++ b/runtime/vm/image_snapshot.cc @@ -512,6 +512,8 @@ static const char* NameOfStubIsolateSpecificStub(ObjectStore* object_store, const Code& code) { if (code.raw() == object_store->build_method_extractor_code()) { return "_iso_stub_BuildMethodExtractorStub"; + } else if (code.raw() == object_store->dispatch_table_null_error_stub()) { + return "_iso_stub_DispatchTableNullErrorStub"; } else if (code.raw() == object_store->null_error_stub_with_fpu_regs_stub()) { return "_iso_stub_NullErrorSharedWithFPURegsStub"; } else if (code.raw() == diff --git a/runtime/vm/object_store.h b/runtime/vm/object_store.h index 48ff955737f..bb47f4a65fe 100644 --- a/runtime/vm/object_store.h +++ b/runtime/vm/object_store.h @@ -168,6 +168,7 @@ class ObjectPointerVisitor; RW(Array, unique_dynamic_targets) \ RW(GrowableObjectArray, megamorphic_cache_table) \ RW(Code, build_method_extractor_code) \ + RW(Code, dispatch_table_null_error_stub) \ RW(Code, null_error_stub_with_fpu_regs_stub) \ RW(Code, null_error_stub_without_fpu_regs_stub) \ RW(Code, null_arg_error_stub_with_fpu_regs_stub) \ diff --git a/runtime/vm/stub_code_list.h b/runtime/vm/stub_code_list.h index 739443e6d8c..ce41ba0c743 100644 --- a/runtime/vm/stub_code_list.h +++ b/runtime/vm/stub_code_list.h @@ -71,6 +71,7 @@ namespace dart { V(CallClosureNoSuchMethod) \ V(FrameAwaitingMaterialization) \ V(AsynchronousGapMarker) \ + V(DispatchTableNullError) \ V(NullErrorSharedWithFPURegs) \ V(NullErrorSharedWithoutFPURegs) \ V(NullArgErrorSharedWithFPURegs) \