From 184e2a8b4c9bd675a32c172014e789832a676f13 Mon Sep 17 00:00:00 2001 From: Samir Jindel Date: Mon, 14 May 2018 14:34:58 +0000 Subject: [PATCH] [vm] Support partial instantiation of noSuchMethod forwarders in VM. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is complicated by the fact that the normal factory method for creating Invocation objects references the arguments descriptor to determine how many type arguments are passed. This doesn't work for partially instantiated closures, because the arguments descriptor will say "0" type arguments even when there may be delayed type arguments attached to the closure from partial instantiation. The arguments descriptor can't be modified, so we have a new factory method which takes the number of delayed type arguments directly. Change-Id: Ic9a35a482b3b7ef80564e674cc6207873e255111 Reviewed-on: https://dart-review.googlesource.com/54245 Commit-Queue: Samir Jindel Reviewed-by: Régis Crelier --- .../type_flow/entry_points.json | 6 ++ runtime/lib/invocation_mirror_patch.dart | 36 ++++++-- runtime/vm/compiler/aot/precompiler.cc | 1 + .../frontend/kernel_binary_flowgraph.cc | 85 +++++++++++++------ runtime/vm/compiler/frontend/kernel_to_il.cc | 13 +++ runtime/vm/compiler/frontend/kernel_to_il.h | 2 + runtime/vm/symbols.h | 1 + tests/language_2/language_2_dart2js.status | 1 + tests/language_2/language_2_vm.status | 2 + ...forwarding_partial_instantiation_test.dart | 25 ++++++ 10 files changed, 137 insertions(+), 35 deletions(-) create mode 100644 tests/language_2/nosuchmethod_forwarding/nosuchmethod_forwarding_partial_instantiation_test.dart diff --git a/pkg/vm/lib/transformations/type_flow/entry_points.json b/pkg/vm/lib/transformations/type_flow/entry_points.json index 95d1f016c29..5465003e097 100644 --- a/pkg/vm/lib/transformations/type_flow/entry_points.json +++ b/pkg/vm/lib/transformations/type_flow/entry_points.json @@ -617,6 +617,12 @@ "name": "_allocateInvocationMirror", "action": "call" }, + { + "library": "dart:core", + "class": "_InvocationMirror", + "name": "_allocateInvocationMirrorForClosure", + "action": "call" + }, { "library": "dart:core", "class": "_TypeError", diff --git a/runtime/lib/invocation_mirror_patch.dart b/runtime/lib/invocation_mirror_patch.dart index 46bc6492349..2d9b9c44551 100644 --- a/runtime/lib/invocation_mirror_patch.dart +++ b/runtime/lib/invocation_mirror_patch.dart @@ -41,6 +41,7 @@ class _InvocationMirror implements Invocation { final List _argumentsDescriptor; final List _arguments; final bool _isSuperInvocation; + final int _delayedTypeArgumentsLen; // External representation of the invocation mirror; populated on demand. Symbol _memberName; @@ -79,16 +80,20 @@ class _InvocationMirror implements Invocation { return _memberName; } + int get _typeArgsLen { + int typeArgsLen = _argumentsDescriptor[_TYPE_ARGS_LEN]; + return typeArgsLen == 0 ? _delayedTypeArgumentsLen : typeArgsLen; + } + List get typeArguments { if (_typeArguments == null) { - int typeArgsLen = _argumentsDescriptor[_TYPE_ARGS_LEN]; - if (typeArgsLen == 0) { + if (_typeArgsLen == 0) { return _typeArguments = const []; } // A TypeArguments object does not have a corresponding Dart class and // cannot be accessed as an array in Dart. Therefore, we need a native // call to unpack the individual types into a list. - _typeArguments = _unpackTypeArguments(_arguments[0], typeArgsLen); + _typeArguments = _unpackTypeArguments(_arguments[0], _typeArgsLen); } return _typeArguments; } @@ -106,7 +111,7 @@ class _InvocationMirror implements Invocation { return _positionalArguments = const []; } // Exclude receiver and type args in the returned list. - int receiverIndex = _argumentsDescriptor[_TYPE_ARGS_LEN] > 0 ? 1 : 0; + int receiverIndex = _typeArgsLen > 0 ? 1 : 0; _positionalArguments = new _ImmutableList._from( _arguments, receiverIndex + 1, numPositionalArguments); } @@ -121,7 +126,7 @@ class _InvocationMirror implements Invocation { if (numNamedArguments == 0) { return _namedArguments = const {}; } - int receiverIndex = _argumentsDescriptor[_TYPE_ARGS_LEN] > 0 ? 1 : 0; + int receiverIndex = _typeArgsLen > 0 ? 1 : 0; _namedArguments = new Map(); for (int i = 0; i < numNamedArguments; i++) { int namedEntryIndex = _FIRST_NAMED_ENTRY + 2 * i; @@ -164,10 +169,12 @@ class _InvocationMirror implements Invocation { } _InvocationMirror(this._functionName, this._argumentsDescriptor, - this._arguments, this._isSuperInvocation, this._type); + this._arguments, this._isSuperInvocation, this._type, + [this._delayedTypeArgumentsLen = 0]); _InvocationMirror._withoutType(this._functionName, this._typeArguments, - this._positionalArguments, this._namedArguments, this._isSuperInvocation); + this._positionalArguments, this._namedArguments, this._isSuperInvocation, + [this._delayedTypeArgumentsLen = 0]); static _allocateInvocationMirror(String functionName, List argumentsDescriptor, List arguments, bool isSuperInvocation, @@ -175,4 +182,19 @@ class _InvocationMirror implements Invocation { return new _InvocationMirror( functionName, argumentsDescriptor, arguments, isSuperInvocation, type); } + + // This factory is used when creating an `Invocation` for a closure call which + // may have delayed type arguments. In that case, the arguments descriptor will + // indicate 0 type arguments, but the actual number of type arguments are + // passed in `delayedTypeArgumentsLen`. If any type arguments are available, + // the type arguments vector will be the first entry in `arguments`. + static _allocateInvocationMirrorForClosure( + String functionName, + List argumentsDescriptor, + List arguments, + int type, + int delayedTypeArgumentsLen) { + return new _InvocationMirror(functionName, argumentsDescriptor, arguments, + false, type, delayedTypeArgumentsLen); + } } diff --git a/runtime/vm/compiler/aot/precompiler.cc b/runtime/vm/compiler/aot/precompiler.cc index 29afb6daa9b..5b2415a8f6d 100644 --- a/runtime/vm/compiler/aot/precompiler.cc +++ b/runtime/vm/compiler/aot/precompiler.cc @@ -510,6 +510,7 @@ static Dart_QualifiedFunctionName vm_entry_points[] = { {"dart:core", "_CastError", "_CastError._create"}, {"dart:core", "_InternalError", "_InternalError."}, {"dart:core", "_InvocationMirror", "_allocateInvocationMirror"}, + {"dart:core", "_InvocationMirror", "_allocateInvocationMirrorForClosure"}, {"dart:core", "_TypeError", "_TypeError._create"}, {"dart:collection", "::", "_rehashObjects"}, {"dart:isolate", "IsolateSpawnException", "IsolateSpawnException."}, diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc index da76aa1d0ea..ba39e0ccfd4 100644 --- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc +++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc @@ -5376,27 +5376,14 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfNoSuchMethodForwarder( body += StoreLocal(TokenPosition::kNoSource, argument_count_var); body += Drop(); if (function.IsGeneric() && Isolate::Current()->reify_generic_functions()) { - Fragment test_generic; - JoinEntryInstr* join = BuildJoinEntry(); - - TargetEntryInstr *passed_type_args, *not_passed_type_args; - test_generic += B->LoadArgDescriptor(); - test_generic += LoadField(ArgumentsDescriptor::type_args_len_offset()); - test_generic += IntConstant(0); - test_generic += BranchIfEqual(¬_passed_type_args, &passed_type_args, - /*negate=*/false); - - Fragment passed_type_args_frag(passed_type_args); - passed_type_args_frag += IntConstant(1); - passed_type_args_frag += - StoreLocal(TokenPosition::kNoSource, argument_count_var); - passed_type_args_frag += Drop(); - passed_type_args_frag += Goto(join); - - Fragment not_passed_type_args_frag(not_passed_type_args); - not_passed_type_args_frag += Goto(join); - - body += Fragment(test_generic.entry, join); + body += flow_graph_builder_->TestAnyTypeArgs( + [&]() { + return Fragment( + {IntConstant(1), + StoreLocal(TokenPosition::kNoSource, argument_count_var), + Drop()}); + }, + Fragment()); } if (function.HasOptionalParameters()) { @@ -5413,7 +5400,13 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfNoSuchMethodForwarder( // // var arguments = new Array(argument_count); // - // for (int i = 0; i < argument_count; ++i) { + // int i = 0; + // if (any type arguments are passed) { + // arguments[0] = function_type_arguments; + // ++i; + // } + // + // for (; i < argument_count; ++i) { // arguments[i] = LoadFpRelativeSlot( // kWordSize * (kParamEndSlotFromFp + argument_count - i)); // } @@ -5429,6 +5422,28 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfNoSuchMethodForwarder( body += StoreLocal(TokenPosition::kNoSource, index); body += Drop(); + // if (any type arguments are passed) { + // arguments[0] = function_type_arguments; + // i = 1; + // } + if (function.IsGeneric() && Isolate::Current()->reify_generic_functions()) { + // clang-format off + std::function store_type_arguments = [&]() { + return Fragment({ + LoadLocal(arguments), + IntConstant(0), + LoadFunctionTypeArguments(), + StoreIndexed(kArrayCid), + Drop(), + IntConstant(1), + StoreLocal(TokenPosition::kNoSource, index), + Drop() + }); + }; + // clang-format on + body += B->TestAnyTypeArgs(store_type_arguments, Fragment()); + } + TargetEntryInstr* body_entry; TargetEntryInstr* loop_exit; @@ -5515,10 +5530,6 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfNoSuchMethodForwarder( body += LoadLocal(arguments); body += PushArgument(); - // false -> this is not super NSM - body += Constant(Bool::False()); - body += PushArgument(); - if (throw_no_such_method_error) { const Function& parent = Function::ZoneHandle(Z, function.parent_function()); @@ -5541,12 +5552,30 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfNoSuchMethodForwarder( } body += PushArgument(); + // Push the number of delayed type arguments. + if (function.IsClosureFunction()) { + LocalVariable* closure = + parsed_function()->node_sequence()->scope()->VariableAt(0); + body += B->TestDelayedTypeArgs( + closure, + Fragment({IntConstant(function.NumTypeParameters()), + StoreLocal(TokenPosition::kNoSource, argument_count_var), + Drop()}), + Fragment({IntConstant(0), + StoreLocal(TokenPosition::kNoSource, argument_count_var), + Drop()})); + body += LoadLocal(argument_count_var); + } else { + body += IntConstant(0); + } + body += PushArgument(); + const Class& mirror_class = Class::Handle(Z, Library::LookupCoreClass(Symbols::InvocationMirror())); ASSERT(!mirror_class.IsNull()); const Function& allocation_function = Function::ZoneHandle( - Z, mirror_class.LookupStaticFunction( - Library::PrivateCoreLibName(Symbols::AllocateInvocationMirror()))); + Z, mirror_class.LookupStaticFunction(Library::PrivateCoreLibName( + Symbols::AllocateInvocationMirrorForClosure()))); ASSERT(!allocation_function.IsNull()); body += StaticCall(TokenPosition::kMinSource, allocation_function, /* argument_count = */ 5, ICData::kStatic); diff --git a/runtime/vm/compiler/frontend/kernel_to_il.cc b/runtime/vm/compiler/frontend/kernel_to_il.cc index aa1cb204b3a..ee05a9c39f0 100644 --- a/runtime/vm/compiler/frontend/kernel_to_il.cc +++ b/runtime/vm/compiler/frontend/kernel_to_il.cc @@ -1385,6 +1385,19 @@ Fragment BaseFlowGraphBuilder::TestDelayedTypeArgs(LocalVariable* closure, return Fragment(test.entry, join); } +Fragment BaseFlowGraphBuilder::TestAnyTypeArgs( + std::function present, + Fragment absent) { + if (parsed_function_->function().IsClosureFunction()) { + LocalVariable* closure = + parsed_function_->node_sequence()->scope()->VariableAt(0); + return TestTypeArgsLen(TestDelayedTypeArgs(closure, present(), absent), + present(), 0); + } else { + return TestTypeArgsLen(absent, present(), 0); + } +} + Fragment FlowGraphBuilder::RethrowException(TokenPosition position, int catch_try_index) { Fragment instructions; diff --git a/runtime/vm/compiler/frontend/kernel_to_il.h b/runtime/vm/compiler/frontend/kernel_to_il.h index 817633fca35..17ca4fae319 100644 --- a/runtime/vm/compiler/frontend/kernel_to_il.h +++ b/runtime/vm/compiler/frontend/kernel_to_il.h @@ -7,6 +7,7 @@ #if !defined(DART_PRECOMPILED_RUNTIME) +#include #include #include "vm/growable_array.h" @@ -647,6 +648,7 @@ class BaseFlowGraphBuilder { Fragment TestDelayedTypeArgs(LocalVariable* closure, Fragment present, Fragment absent); + Fragment TestAnyTypeArgs(std::function present, Fragment absent); JoinEntryInstr* BuildThrowNoSuchMethod(); diff --git a/runtime/vm/symbols.h b/runtime/vm/symbols.h index 4b3f6bc3ad0..74f9c4789ff 100644 --- a/runtime/vm/symbols.h +++ b/runtime/vm/symbols.h @@ -325,6 +325,7 @@ class ObjectPointerVisitor; V(ForwardingCorpse, "ForwardingCorpse") \ V(InvocationMirror, "_InvocationMirror") \ V(AllocateInvocationMirror, "_allocateInvocationMirror") \ + V(AllocateInvocationMirrorForClosure, "_allocateInvocationMirrorForClosure") \ V(toString, "toString") \ V(_lookupHandler, "_lookupHandler") \ V(_handleMessage, "_handleMessage") \ diff --git a/tests/language_2/language_2_dart2js.status b/tests/language_2/language_2_dart2js.status index fc0489ef655..656c095bb73 100644 --- a/tests/language_2/language_2_dart2js.status +++ b/tests/language_2/language_2_dart2js.status @@ -1958,6 +1958,7 @@ wrong_number_type_arguments_test/none: Pass const_constructor3_test/04: MissingCompileTimeError # OK - Subtype check uses JS number semantics. covariant_subtyping_test: Crash ct_const_test: RuntimeError +nosuchmethod_forwarding/nosuchmethod_forwarding_partial_instantiation_test: RuntimeError [ $compiler == dart2js && $fasta && !$strong ] *: SkipByDesign diff --git a/tests/language_2/language_2_vm.status b/tests/language_2/language_2_vm.status index 4c49535d031..05a51766916 100644 --- a/tests/language_2/language_2_vm.status +++ b/tests/language_2/language_2_vm.status @@ -1323,6 +1323,8 @@ mixin_illegal_superclass_test: Skip # Issues 24478 and 23773 mixin_mixin6_test: RuntimeError, OK # Not running in strong mode. multiline_strings_test: Fail # Issue 23020 no_main_test/01: Skip +nosuchmethod_forwarding/nosuchmethod_forwarding_partial_instantiation: RuntimeError # ByDesign +nosuchmethod_forwarding/nosuchmethod_forwarding_partial_instantiation_test: RuntimeError regress_19413_test: MissingCompileTimeError regress_21793_test/01: MissingCompileTimeError super_test: Fail, OK diff --git a/tests/language_2/nosuchmethod_forwarding/nosuchmethod_forwarding_partial_instantiation_test.dart b/tests/language_2/nosuchmethod_forwarding/nosuchmethod_forwarding_partial_instantiation_test.dart new file mode 100644 index 00000000000..69a554fa7d2 --- /dev/null +++ b/tests/language_2/nosuchmethod_forwarding/nosuchmethod_forwarding_partial_instantiation_test.dart @@ -0,0 +1,25 @@ +// 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. + +// Testing that `noSuchMethod` forwarders can be partially instantiated, and +// that their delayed type arguments are transparently passed to `noSuchMethod`. + +import 'package:expect/expect.dart'; + +class C { + T test1(T x); + + dynamic noSuchMethod(Invocation invoke) { + Expect.equals(invoke.typeArguments.length, 1); + Expect.equals(invoke.typeArguments[0].toString(), 'int'); + Expect.equals(invoke.positionalArguments[0], 1); + return null; + } +} + +void main() { + var c = new C(); + int Function(int) k1 = c.test1; + k1(1); +}