[vm][ffi] Passes FFI handles on stack.

Passing handles in FFI calls has significant overhead due to
how each handle requires a runtime entry to allocate in the
handle scope.

This change removes that runtime entry by relying on the
register allocator to allocate all handle arguments on the stack,
so that we don't need to allocate them separately.
To pass the stack handles to the native call we then pass a pointer
to the stack slot as the native argument.

Testing:
- We already have comprehensive tests for correctness in the form
  of the FFI tests. These make calls with various combinations of
  Handle and non-handle arguments.
- Correct GC behaviour is likewise covered in
  `vmspecific_handle_test.dart` which makes calls with handles
  arguments and triggers GC in-flight.
  In case we do not correctly pass the handles on the stack, the
  GC will trash them during the call.

TEST=Existing.
Bug: #47624
Change-Id: Ic837bad5484daaa5534b7c2e8707ac2c5dfa480f
Cq-Include-Trybots: luci.dart.try:vm-kernel-gcc-linux-try,vm-kernel-linux-debug-simriscv64-try,vm-kernel-nnbd-mac-release-arm64-try,vm-kernel-precomp-android-release-arm64c-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-precomp-dwarf-linux-product-x64-try,vm-kernel-precomp-linux-debug-simarm_x64-try,vm-kernel-precomp-linux-debug-simriscv64-try,vm-precomp-ffi-qemu-linux-release-arm-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/243320
Reviewed-by: Slava Egorov <[email protected]>
Commit-Queue: Clement Skau <[email protected]>

Author: Clement Skau

runtime/vm/compiler/backend/il.cc

@@ -6506,6 +6506,11 @@ void BitCastInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
 
 Representation FfiCallInstr::RequiredInputRepresentation(intptr_t idx) const {
   if (idx < TargetAddressIndex()) {
+    // All input handles are passed as Tagged values on the stack to
+    // FfiCallInstr, which passes "handles", i.e. pointers, to these.
+    if (marshaller_.IsHandle(marshaller_.ArgumentIndex(idx))) {
+      return kTagged;
+    }
     return marshaller_.RepInFfiCall(idx);
   } else if (idx == TargetAddressIndex()) {
     return kUnboxedFfiIntPtr;
@@ -6621,6 +6626,28 @@ void FfiCallInstr::EmitParamMoves(FlowGraphCompiler* compiler,
         // http://dartbug.com/45055), which means all incomming arguments
         // originate from parameters and thus are non-constant.
         UNREACHABLE();
+      }
+
+      // Handles are passed into FfiCalls as Tagged values on the stack, and
+      // then we pass pointers to these handles to the native function here.
+      if (marshaller_.IsHandle(arg_index)) {
+        ASSERT(compiler::target::LocalHandle::ptr_offset() == 0);
+        ASSERT(compiler::target::LocalHandle::InstanceSize() ==
+               compiler::target::kWordSize);
+        ASSERT(num_defs == 1);
+        ASSERT(origin.IsStackSlot());
+        if (def_target.IsRegisters()) {
+          __ AddImmediate(def_target.AsLocation().reg(), origin.base_reg(),
+                          origin.stack_index() * compiler::target::kWordSize);
+        } else {
+          ASSERT(def_target.IsStack());
+          const auto& target_stack = def_target.AsStack();
+          __ AddImmediate(temp0, origin.base_reg(),
+                          origin.stack_index() * compiler::target::kWordSize);
+          __ StoreToOffset(temp0,
+                           compiler::Address(target_stack.base_register(),
+                                             target_stack.offset_in_bytes()));
+        }
       } else {
 #if defined(INCLUDE_IL_PRINTER)
         __ Comment("def_target %s <- origin %s %s", def_target.ToCString(),

runtime/vm/compiler/ffi/marshaller.cc

@@ -389,6 +389,15 @@ Location CallMarshaller::LocInFfiCall(intptr_t def_index_global) const {
     return loc.AsLocation();
   }
 
+  // Force all handles to be Stack locations.
+  // Since non-leaf calls block all registers, Any locations effectively mean
+  // Stack.
+  // TODO(dartbug.com/38985): Once we start inlining FFI trampolines, the inputs
+  // can be constants as well.
+  if (IsHandle(arg_index)) {
+    return Location::Any();
+  }
+
   if (loc.IsMultiple()) {
     const intptr_t def_index_in_arg =
         def_index_global - FirstDefinitionIndex(arg_index);

runtime/vm/compiler/frontend/kernel_to_il.cc

@@ -4007,15 +4007,10 @@ Fragment FlowGraphBuilder::ExitHandleScope() {
   return Fragment(instr);
 }
 
-Fragment FlowGraphBuilder::AllocateHandle(LocalVariable* api_local_scope) {
+Fragment FlowGraphBuilder::AllocateHandle() {
   Fragment code;
-  if (api_local_scope != nullptr) {
-    // Use the reference the scope we created in the trampoline.
-    code += LoadLocal(api_local_scope);
-  } else {
-    // Or get a reference to the top handle scope.
-    code += GetTopHandleScope();
-  }
+  // Get a reference to the top handle scope.
+  code += GetTopHandleScope();
   Value* api_local_scope_value = Pop();
   auto* instr = new (Z) AllocateHandleInstr(api_local_scope_value);
   Push(instr);
@@ -4039,10 +4034,10 @@ Fragment FlowGraphBuilder::RawStoreField(int32_t offset) {
   return code;
 }
 
-Fragment FlowGraphBuilder::WrapHandle(LocalVariable* api_local_scope) {
+Fragment FlowGraphBuilder::WrapHandle() {
   Fragment code;
   LocalVariable* object = MakeTemporary();
-  code += AllocateHandle(api_local_scope);
+  code += AllocateHandle();
 
   code += LoadLocal(MakeTemporary());  // Duplicate handle pointer.
   code += ConvertUnboxedToUntagged(kUnboxedIntPtr);
@@ -4528,8 +4523,7 @@ Fragment FlowGraphBuilder::FfiConvertPrimitiveToDart(
 
 Fragment FlowGraphBuilder::FfiConvertPrimitiveToNative(
     const compiler::ffi::BaseMarshaller& marshaller,
-    intptr_t arg_index,
-    LocalVariable* api_local_scope) {
+    intptr_t arg_index) {
   ASSERT(!marshaller.IsCompound(arg_index));
 
   Fragment body;
@@ -4538,7 +4532,7 @@ Fragment FlowGraphBuilder::FfiConvertPrimitiveToNative(
     body += LoadUntagged(compiler::target::PointerBase::data_offset());
     body += ConvertUntaggedToUnboxed(kUnboxedFfiIntPtr);
   } else if (marshaller.IsHandle(arg_index)) {
-    body += WrapHandle(api_local_scope);
+    body += WrapHandle();
   } else {
     if (marshaller.IsBool(arg_index)) {
       body += BoolToInt();
@@ -4621,16 +4615,17 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiNative(const Function& function) {
 
   Fragment body;
   intptr_t try_handler_index = -1;
-  LocalVariable* api_local_scope = nullptr;
   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_;
-
+    // TODO(dartbug.com/48989): Remove scope for calls where we don't actually
+    // need it.
+    // We no longer need the scope for passing in Handle arguments, but the
+    // native function might for instance be relying on this scope for Dart API.
     body += EnterHandleScope();
-    api_local_scope = MakeTemporary("api_local_scope");
   }
 
   // Allocate typed data before FfiCall and pass it in to ffi call if needed.
@@ -4652,7 +4647,12 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiNative(const Function& function) {
     } else {
       body += LoadLocal(parsed_function_->ParameterVariable(
           kFirstArgumentParameterOffset + i));
-      body += FfiConvertPrimitiveToNative(marshaller, i, api_local_scope);
+      // FfiCallInstr specifies all handle locations as Stack, and will pass a
+      // pointer to the stack slot as the native handle argument.
+      // Therefore we do not need to wrap handles.
+      if (!marshaller.IsHandle(i)) {
+        body += FfiConvertPrimitiveToNative(marshaller, i);
+      }
     }
   }
 
@@ -4703,6 +4703,8 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiNative(const Function& function) {
   }
 
   if (signature_contains_handles) {
+    // TODO(dartbug.com/48989): Remove scope for calls where we don't actually
+    // need it.
     body += DropTempsPreserveTop(1);  // Drop api_local_scope.
     body += ExitHandleScope();
   }
@@ -4721,6 +4723,8 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiNative(const Function& function) {
         CatchBlockEntry(Array::empty_array(), try_handler_index,
                         /*needs_stacktrace=*/true, /*is_synthesized=*/true);
 
+    // TODO(dartbug.com/48989): Remove scope for calls where we don't actually
+    // need it.
     // TODO(41984): If we want to pass in the handle scope, move it out
     // of the try catch.
     catch_body += ExitHandleScope();
@@ -4805,8 +4809,8 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiCallback(const Function& function) {
     body += FfiCallbackConvertCompoundReturnToNative(
         marshaller, compiler::ffi::kResultIndex);
   } else {
-    body += FfiConvertPrimitiveToNative(marshaller, compiler::ffi::kResultIndex,
-                                        /*api_local_scope=*/nullptr);
+    body +=
+        FfiConvertPrimitiveToNative(marshaller, compiler::ffi::kResultIndex);
   }
 
   body += NativeReturn(marshaller);
@@ -4830,8 +4834,7 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiCallback(const Function& function) {
   } else if (marshaller.IsHandle(compiler::ffi::kResultIndex)) {
     catch_body += UnhandledException();
     catch_body +=
-        FfiConvertPrimitiveToNative(marshaller, compiler::ffi::kResultIndex,
-                                    /*api_local_scope=*/nullptr);
+        FfiConvertPrimitiveToNative(marshaller, compiler::ffi::kResultIndex);
 
   } else if (marshaller.IsCompound(compiler::ffi::kResultIndex)) {
     ASSERT(function.FfiCallbackExceptionalReturn() == Object::null());
@@ -4853,8 +4856,7 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiCallback(const Function& function) {
     catch_body += Constant(
         Instance::ZoneHandle(Z, function.FfiCallbackExceptionalReturn()));
     catch_body +=
-        FfiConvertPrimitiveToNative(marshaller, compiler::ffi::kResultIndex,
-                                    /*api_local_scope=*/nullptr);
+        FfiConvertPrimitiveToNative(marshaller, compiler::ffi::kResultIndex);
   }
 
   catch_body += NativeReturn(marshaller);

runtime/vm/compiler/frontend/kernel_to_il.h

@@ -305,8 +305,7 @@ class FlowGraphBuilder : public BaseFlowGraphBuilder {
   // Works for FFI call arguments, and FFI callback return values.
   Fragment FfiConvertPrimitiveToNative(
       const compiler::ffi::BaseMarshaller& marshaller,
-      intptr_t arg_index,
-      LocalVariable* api_local_scope);
+      intptr_t arg_index);
 
   // Pops an unboxed native value, and pushes a Dart object, according to the
   // semantics of FFI argument translation.
@@ -388,7 +387,7 @@ class FlowGraphBuilder : public BaseFlowGraphBuilder {
   Fragment ExitHandleScope();
 
   // Leaves a `LocalHandle` on the stack.
-  Fragment AllocateHandle(LocalVariable* api_local_scope);
+  Fragment AllocateHandle();
 
   // Loads a tagged value from an untagged base + offset from outside the heap.
   Fragment RawLoadField(int32_t offset);
@@ -399,7 +398,7 @@ class FlowGraphBuilder : public BaseFlowGraphBuilder {
   Fragment RawStoreField(int32_t offset);
 
   // Wraps an `Object` from the stack and leaves a `LocalHandle` on the stack.
-  Fragment WrapHandle(LocalVariable* api_local_scope);
+  Fragment WrapHandle();
 
   // Unwraps a `LocalHandle` from the stack and leaves the object on the stack.
   Fragment UnwrapHandle();

runtime/vm/compiler/runtime_api.h

@@ -667,6 +667,7 @@ class ArgumentsDescriptor : public AllStatic {
 class LocalHandle : public AllStatic {
  public:
   static word ptr_offset();
+  static word InstanceSize();
 };
 
 class Pointer : public AllStatic {