From da39a4abffa850afc63d6d250a4f13490b42ce70 Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Fri, 26 Jun 2020 12:03:02 +0000 Subject: [PATCH] [vm/ffi] Remove try-catch from ffi trampoline if no handle scope https://dart-review.googlesource.com/c/sdk/+/145591 introduced a try catch into FFI calls to call ExitHandleScope on the exception path. However, we only need this try-catch if we actually need to exit the handle scope on the exception path, which is not the case if we have no handles in the signature. So this CL makes the try catch optional. This speeds up ffi calls without handles (tested on JIT x64): FfiCall.Uint8x01(RunTime): 206.4801280066068 us. -> FfiCall.Uint8x01(RunTime): 203.7240782236708 us. Also adds a test that checks that an exception can still be propagated with Dart_PropagateError from native code when the FFI trampoline has no try catch. Change-Id: I9fac7078381c60fb8055b64fff29ea364fbc948f Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,app-kernel-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-kernel-precomp-linux-debug-x64-try,vm-dartkb-linux-release-x64-abi-try,vm-kernel-precomp-android-release-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-msan-linux-release-x64-try,vm-kernel-precomp-msan-linux-release-x64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-simarm64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,dart-sdk-linux-try,analyzer-analysis-server-linux-try,analyzer-linux-release-try,front-end-linux-release-x64-try,vm-kernel-precomp-win-release-x64-try,vm-kernel-mac-debug-x64-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-nnbd-linux-debug-x64-try,analyzer-nnbd-linux-release-try,front-end-nnbd-linux-release-x64-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/151239 Commit-Queue: Daco Harkes Reviewed-by: Martin Kustermann Reviewed-by: Alexander Markov --- .../ffi_test/ffi_test_functions_vmspecific.cc | 12 +++++ runtime/vm/compiler/ffi/marshaller.cc | 10 +---- runtime/vm/compiler/frontend/kernel_to_il.cc | 45 +++++++++++-------- runtime/vm/compiler/frontend/scope_builder.cc | 21 +++++---- runtime/vm/object.cc | 16 +++++++ runtime/vm/object.h | 2 + tests/ffi/vmspecific_handle_test.dart | 19 ++++++++ tests/ffi_2/vmspecific_handle_test.dart | 19 ++++++++ 8 files changed, 109 insertions(+), 35 deletions(-) diff --git a/runtime/bin/ffi_test/ffi_test_functions_vmspecific.cc b/runtime/bin/ffi_test/ffi_test_functions_vmspecific.cc index 05b8952354c7..e57627707a12 100644 --- a/runtime/bin/ffi_test/ffi_test_functions_vmspecific.cc +++ b/runtime/bin/ffi_test/ffi_test_functions_vmspecific.cc @@ -889,6 +889,18 @@ DART_EXPORT int64_t HandleReadFieldValue(Dart_Handle handle) { return value; } +// Does not have a handle in it's own signature, so does not enter and exit +// scope in the trampoline. +DART_EXPORT int64_t PropagateErrorWithoutHandle(Dart_Handle (*callback)()) { + Dart_EnterScope(); + Dart_Handle result = callback(); + if (Dart_IsError(result)) { + Dart_PropagateError(result); + } + Dart_ExitScope(); + return 0; +} + DART_EXPORT Dart_Handle TrueHandle() { return Dart_True(); } diff --git a/runtime/vm/compiler/ffi/marshaller.cc b/runtime/vm/compiler/ffi/marshaller.cc index 21eff2c9e6b0..09dd56d8745c 100644 --- a/runtime/vm/compiler/ffi/marshaller.cc +++ b/runtime/vm/compiler/ffi/marshaller.cc @@ -18,15 +18,7 @@ namespace compiler { namespace ffi { bool BaseMarshaller::ContainsHandles() const { - if (IsHandle(kResultIndex)) { - return true; - } - for (intptr_t i = 0; i < num_args(); i++) { - if (IsHandle(i)) { - return true; - } - } - return false; + return dart_signature_.FfiCSignatureContainsHandles(); } Location CallMarshaller::LocInFfiCall(intptr_t arg_index) const { diff --git a/runtime/vm/compiler/frontend/kernel_to_il.cc b/runtime/vm/compiler/frontend/kernel_to_il.cc index 1676b91fa47b..00b748e9c221 100644 --- a/runtime/vm/compiler/frontend/kernel_to_il.cc +++ b/runtime/vm/compiler/frontend/kernel_to_il.cc @@ -2953,6 +2953,8 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiNative(const Function& function) { const auto& marshaller = *new (Z) compiler::ffi::CallMarshaller(Z, function); + const bool signature_contains_handles = marshaller.ContainsHandles(); + BuildArgumentTypeChecks(TypeChecksToBuild::kCheckAllTypeParameterBounds, &function_body, &function_body, &function_body); @@ -2976,14 +2978,16 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiNative(const Function& function) { function_body += Drop(); } - // Wrap in Try catch to transition from Native to Generated on a throw from - // the dart_api. - const intptr_t try_handler_index = AllocateTryIndex(); - Fragment body = TryCatch(try_handler_index); - ++try_depth_; - + Fragment body; + intptr_t try_handler_index = -1; LocalVariable* api_local_scope = nullptr; - if (marshaller.ContainsHandles()) { + if (signature_contains_handles) { + // Wrap in Try catch to transition from Native to Generated on a throw from + // the dart_api. + try_handler_index = AllocateTryIndex(); + body += TryCatch(try_handler_index); + ++try_depth_; + body += EnterHandleScope(); api_local_scope = MakeTemporary(); } @@ -3020,29 +3024,34 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiNative(const Function& function) { body += FfiConvertArgumentToDart(marshaller, compiler::ffi::kResultIndex); - if (marshaller.ContainsHandles()) { + if (signature_contains_handles) { body += DropTempsPreserveTop(1); // Drop api_local_scope. body += ExitHandleScope(); } body += Return(TokenPosition::kNoSource); - --try_depth_; + if (signature_contains_handles) { + --try_depth_; + } + function_body += body; - ++catch_depth_; - Fragment catch_body = - CatchBlockEntry(Array::empty_array(), try_handler_index, - /*needs_stacktrace=*/true, /*is_synthesized=*/true); - if (marshaller.ContainsHandles()) { + if (signature_contains_handles) { + ++catch_depth_; + Fragment catch_body = + CatchBlockEntry(Array::empty_array(), try_handler_index, + /*needs_stacktrace=*/true, /*is_synthesized=*/true); + // TODO(41984): If we want to pass in the handle scope, move it out // of the try catch. catch_body += ExitHandleScope(); + + catch_body += LoadLocal(CurrentException()); + catch_body += LoadLocal(CurrentStackTrace()); + catch_body += RethrowException(TokenPosition::kNoSource, try_handler_index); + --catch_depth_; } - catch_body += LoadLocal(CurrentException()); - catch_body += LoadLocal(CurrentStackTrace()); - catch_body += RethrowException(TokenPosition::kNoSource, try_handler_index); - --catch_depth_; return new (Z) FlowGraph(*parsed_function_, graph_entry_, last_used_block_id_, prologue_info); diff --git a/runtime/vm/compiler/frontend/scope_builder.cc b/runtime/vm/compiler/frontend/scope_builder.cc index 35caee99cca2..e8b8a2c3ccbc 100644 --- a/runtime/vm/compiler/frontend/scope_builder.cc +++ b/runtime/vm/compiler/frontend/scope_builder.cc @@ -420,14 +420,19 @@ ScopeBuildingResult* ScopeBuilder::BuildScopes() { : Object::dynamic_type().raw())); scope_->InsertParameterAt(i, variable); } - current_function_async_marker_ = FunctionNodeHelper::kSync; - ++depth_.try_; - AddTryVariables(); - --depth_.try_; - ++depth_.catch_; - AddCatchVariables(); - FinalizeCatchVariables(); - --depth_.catch_; + // Callbacks and calls with handles need try/catch variables. + if (function.IsFfiTrampoline() && + (function.FfiCallbackTarget() != Function::null() || + function.FfiCSignatureContainsHandles())) { + current_function_async_marker_ = FunctionNodeHelper::kSync; + ++depth_.try_; + AddTryVariables(); + --depth_.try_; + ++depth_.catch_; + AddCatchVariables(); + FinalizeCatchVariables(); + --depth_.catch_; + } break; case FunctionLayout::kSignatureFunction: case FunctionLayout::kIrregexpFunction: diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index 29e4c970b2b7..789896a68ed7 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -6939,6 +6939,22 @@ FunctionPtr Function::FfiCSignature() const { return FfiTrampolineData::Cast(obj).c_signature(); } +bool Function::FfiCSignatureContainsHandles() const { + ASSERT(IsFfiTrampoline()); + const Function& c_signature = Function::Handle(FfiCSignature()); + const intptr_t num_params = c_signature.num_fixed_parameters(); + for (intptr_t i = 0; i < num_params; i++) { + const bool is_handle = + AbstractType::Handle(c_signature.ParameterTypeAt(i)).type_class_id() == + kFfiHandleCid; + if (is_handle) { + return true; + } + } + return AbstractType::Handle(c_signature.result_type()).type_class_id() == + kFfiHandleCid; +} + int32_t Function::FfiCallbackId() const { ASSERT(IsFfiTrampoline()); const Object& obj = Object::Handle(raw_ptr()->data_); diff --git a/runtime/vm/object.h b/runtime/vm/object.h index 0193fcd96209..b47cf6d965d8 100644 --- a/runtime/vm/object.h +++ b/runtime/vm/object.h @@ -2504,6 +2504,8 @@ class Function : public Object { // Can only be used on FFI trampolines. FunctionPtr FfiCSignature() const; + bool FfiCSignatureContainsHandles() const; + // Can only be called on FFI trampolines. // -1 for Dart -> native calls. int32_t FfiCallbackId() const; diff --git a/tests/ffi/vmspecific_handle_test.dart b/tests/ffi/vmspecific_handle_test.dart index 4548c70fd4b1..dcb5e3824650 100644 --- a/tests/ffi/vmspecific_handle_test.dart +++ b/tests/ffi/vmspecific_handle_test.dart @@ -23,6 +23,7 @@ void main() { testDeepException2(); testNull(); testDeepRecursive(); + testNoHandlePropagateError(); } void testHandle() { @@ -220,6 +221,19 @@ void testDeepRecursive() { Expect.isTrue(identical(someObject, result)); } +void testNoHandlePropagateError() { + bool throws = false; + try { + final result = propagateErrorWithoutHandle(exceptionHandleCallbackPointer); + print(result); + } catch (e) { + throws = true; + print("caught ($e)"); + Expect.isTrue(identical(someException, e)); + } + Expect.isTrue(throws); +} + final testLibrary = dlopenPlatformSpecific("ffi_test_functions"); final passObjectToC = testLibrary.lookupFunction>, Int64), Object Function(Object, Pointer>, int)>("HandleRecursion"); + +final propagateErrorWithoutHandle = testLibrary.lookupFunction< + Int64 Function(Pointer>), + int Function(Pointer>)>( + "PropagateErrorWithoutHandle"); diff --git a/tests/ffi_2/vmspecific_handle_test.dart b/tests/ffi_2/vmspecific_handle_test.dart index 85afae473076..b7416e7bcc0b 100644 --- a/tests/ffi_2/vmspecific_handle_test.dart +++ b/tests/ffi_2/vmspecific_handle_test.dart @@ -23,6 +23,7 @@ void main() { testDeepException2(); testNull(); testDeepRecursive(); + testNoHandlePropagateError(); } void testHandle() { @@ -220,6 +221,19 @@ void testDeepRecursive() { Expect.isTrue(identical(someObject, result)); } +void testNoHandlePropagateError() { + bool throws = false; + try { + final result = propagateErrorWithoutHandle(exceptionHandleCallbackPointer); + print(result); + } catch (e) { + throws = true; + print("caught ($e)"); + Expect.isTrue(identical(someException, e)); + } + Expect.isTrue(throws); +} + final testLibrary = dlopenPlatformSpecific("ffi_test_functions"); final passObjectToC = testLibrary.lookupFunction>, Int64), Object Function(Object, Pointer>, int)>("HandleRecursion"); + +final propagateErrorWithoutHandle = testLibrary.lookupFunction< + Int64 Function(Pointer>), + int Function(Pointer>)>( + "PropagateErrorWithoutHandle");