[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 <dacoharkes@google.com> Reviewed-by: Martin Kustermann <kustermann@google.com> Reviewed-by: Alexander Markov <alexmarkov@google.com>
dart-lang · Jun 26, 2020 · da39a4a · da39a4a
1 parent 15464a4
commit da39a4a
Show file tree

Hide file tree

Showing 8 changed files with 109 additions and 35 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<Handle Function(Handle),
@@ -247,3 +261,8 @@ final handleRecursion = testLibrary.lookupFunction<
         Handle, Pointer<NativeFunction<Handle Function(Int64)>>, Int64),
     Object Function(Object, Pointer<NativeFunction<Handle Function(Int64)>>,
         int)>("HandleRecursion");
+
+final propagateErrorWithoutHandle = testLibrary.lookupFunction<
+        Int64 Function(Pointer<NativeFunction<Handle Function()>>),
+        int Function(Pointer<NativeFunction<Handle Function()>>)>(
+    "PropagateErrorWithoutHandle");
diff --git 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<Handle Function(Handle),
@@ -247,3 +261,8 @@ final handleRecursion = testLibrary.lookupFunction<
         Handle, Pointer<NativeFunction<Handle Function(Int64)>>, Int64),
     Object Function(Object, Pointer<NativeFunction<Handle Function(Int64)>>,
         int)>("HandleRecursion");
+
+final propagateErrorWithoutHandle = testLibrary.lookupFunction<
+        Int64 Function(Pointer<NativeFunction<Handle Function()>>),
+        int Function(Pointer<NativeFunction<Handle Function()>>)>(
+    "PropagateErrorWithoutHandle");