[vm/ffi] Update exception behavior of FFI callbacks and fix callbacks returning void

Issues dartbug.com/36856 and dartbug.com/37301

Change-Id: I96595b0781a3a6ddf73bba5595ed5c1aa08be257
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/106352
Reviewed-by: Martin Kustermann <[email protected]>
Reviewed-by: Daco Harkes <[email protected]>

Author: sjindel-google

pkg/front_end/lib/src/api_unstable/vm.dart

@@ -43,7 +43,8 @@ export '../fasta/fasta_codes.dart'
         templateFfiTypeMismatch,
         templateFfiTypeUnsized,
         templateFfiFieldInitializer,
-        templateIllegalRecursiveType;
+        templateIllegalRecursiveType,
+        templateFfiDartTypeMismatch;
 
 export '../fasta/hybrid_file_system.dart' show HybridFileSystem;
 

pkg/front_end/lib/src/fasta/fasta_codes_generated.dart

@@ -3641,6 +3641,34 @@ const MessageCode messageFastaUsageShort =
   -o <file> Generate the output into <file>.
   -h        Display this message (add -v for information about all options).""");
 
+// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
+const Template<Message Function(DartType _type, DartType _type2)>
+    templateFfiDartTypeMismatch =
+    const Template<Message Function(DartType _type, DartType _type2)>(
+        messageTemplate: r"""Expected '#type' to be a subtype of '#type2'.""",
+        withArguments: _withArgumentsFfiDartTypeMismatch);
+
+// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
+const Code<Message Function(DartType _type, DartType _type2)>
+    codeFfiDartTypeMismatch =
+    const Code<Message Function(DartType _type, DartType _type2)>(
+  "FfiDartTypeMismatch",
+  templateFfiDartTypeMismatch,
+);
+
+// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
+Message _withArgumentsFfiDartTypeMismatch(DartType _type, DartType _type2) {
+  TypeLabeler labeler = new TypeLabeler();
+  List<Object> typeParts = labeler.labelType(_type);
+  List<Object> type2Parts = labeler.labelType(_type2);
+  String type = typeParts.join();
+  String type2 = type2Parts.join();
+  return new Message(codeFfiDartTypeMismatch,
+      message: """Expected '${type}' to be a subtype of '${type2}'.""" +
+          labeler.originMessages,
+      arguments: {'type': _type, 'type2': _type2});
+}
+
 // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
 const Template<
     Message Function(String name)> templateFfiFieldAnnotation = const Template<

pkg/front_end/messages.status

@@ -253,6 +253,7 @@ FfiStructAnnotation/analyzerCode: Fail
 FfiTypeInvalid/analyzerCode: Fail
 FfiTypeMismatch/analyzerCode: Fail
 FfiTypeUnsized/analyzerCode: Fail
+FfiDartTypeMismatch/analyzerCode: Fail
 FieldInitializedOutsideDeclaringClass/part_wrapped_script1: Fail
 FieldInitializedOutsideDeclaringClass/script1: Fail
 FieldInitializerOutsideConstructor/part_wrapped_script1: Fail

pkg/front_end/messages.yaml

@@ -3477,6 +3477,11 @@ FfiStructAnnotation:
   template: "Class '#name' is a dart:ffi Pointer but has no struct annotation. Only struct Pointers can have fields."
   external: test/ffi_test.dart
 
+FfiDartTypeMismatch:
+  # Used by dart:ffi
+  template: "Expected '#type' to be a subtype of '#type2'."
+  external: test/ffi_test.dart
+
 SpreadTypeMismatch:
   template: "Unexpected type '#type' of a spread.  Expected 'dynamic' or an Iterable."
   script:

pkg/vm/lib/transformations/ffi_use_sites.dart

@@ -8,6 +8,7 @@ import 'package:front_end/src/api_unstable/vm.dart'
     show
         templateFfiTypeInvalid,
         templateFfiTypeMismatch,
+        templateFfiDartTypeMismatch,
         templateFfiTypeUnsized,
         templateFfiNotStatic;
 
@@ -120,6 +121,20 @@ class _FfiUseSiteTransformer extends FfiTransformer {
         _ensureIsStatic(func);
         _ensureNativeTypeValid(nativeType, node);
         _ensureNativeTypeToDartType(nativeType, dartType, node);
+
+        // Check `exceptionalReturn`'s type.
+        final FunctionType funcType = dartType;
+        final Expression exceptionalReturn = node.arguments.positional[1];
+        final DartType returnType = exceptionalReturn.getStaticType(env);
+
+        if (!env.isSubtypeOf(returnType, funcType.returnType)) {
+          diagnosticReporter.report(
+              templateFfiDartTypeMismatch.withArguments(
+                  returnType, funcType.returnType),
+              exceptionalReturn.fileOffset,
+              1,
+              exceptionalReturn.location.file);
+        }
       }
     } catch (_FfiStaticTypeError) {}
 

runtime/bin/ffi_test/ffi_test_functions.cc

@@ -632,8 +632,8 @@ DART_EXPORT int TestStore(int64_t* (*fn)(int64_t* a)) {
   return 0;
 }
 
-DART_EXPORT int TestReturnNull(int32_t fn()) {
-  CHECK_EQ(fn(), 0);
+DART_EXPORT int TestReturnNull(int32_t (*fn)()) {
+  CHECK_EQ(fn(), 42);
   return 0;
 }
 
@@ -662,6 +662,26 @@ DART_EXPORT int TestGC(void (*do_gc)()) {
   return 0;
 }
 
+DART_EXPORT int TestReturnVoid(int (*return_void)()) {
+  CHECK_EQ(return_void(), 0);
+  return 0;
+}
+
+DART_EXPORT int TestThrowExceptionDouble(double (*fn)()) {
+  CHECK_EQ(fn(), 42.0);
+  return 0;
+}
+
+DART_EXPORT int TestThrowExceptionPointer(void* (*fn)()) {
+  CHECK_EQ(fn(), reinterpret_cast<void*>(42));
+  return 0;
+}
+
+DART_EXPORT int TestThrowException(int (*fn)()) {
+  CHECK_EQ(fn(), 42);
+  return 0;
+}
+
 struct CallbackTestData {
   int success;
   void (*callback)();

runtime/lib/ffi.cc

@@ -8,6 +8,7 @@
 #include "platform/globals.h"
 #include "vm/bootstrap_natives.h"
 #include "vm/class_finalizer.h"
+#include "vm/class_id.h"
 #include "vm/compiler/assembler/assembler.h"
 #include "vm/compiler/ffi.h"
 #include "vm/compiler/jit/compiler.h"
@@ -527,11 +528,11 @@ DEFINE_NATIVE_ENTRY(Ffi_asFunction, 1, 1) {
 }
 
 // Generates assembly to trampoline from native code into Dart.
+#if !defined(DART_PRECOMPILED_RUNTIME) && !defined(DART_PRECOMPILER)
 static uword CompileNativeCallback(const Function& c_signature,
-                                   const Function& dart_target) {
-#if defined(DART_PRECOMPILED_RUNTIME) || defined(DART_PRECOMPILER)
-  UNREACHABLE();
-#elif defined(TARGET_ARCH_DBC)
+                                   const Function& dart_target,
+                                   const Instance& exceptional_return) {
+#if defined(TARGET_ARCH_DBC)
   // https://github.com/dart-lang/sdk/issues/35774
   // FFI is supported, but callbacks are not.
   Exceptions::ThrowUnsupportedError(
@@ -564,6 +565,33 @@ static uword CompileNativeCallback(const Function& c_signature,
   function.SetFfiCallbackId(callback_id);
   function.SetFfiCallbackTarget(dart_target);
 
+  // We require that the exceptional return value for functions returning 'Void'
+  // must be 'null', since native code should not look at the result.
+  if (compiler::ffi::NativeTypeIsVoid(
+          AbstractType::Handle(c_signature.result_type())) &&
+      !exceptional_return.IsNull()) {
+    Exceptions::ThrowUnsupportedError(
+        "Only 'null' may be used as the exceptional return value for a "
+        "callback returning void.");
+  }
+
+  // We need to load the exceptional return value as a constant in the generated
+  // function. This means we need to ensure that it's in old space and has no
+  // (transitively) mutable fields. This is done by checking (asserting) that
+  // it's a built-in FFI class, whose fields are all immutable, or a
+  // user-defined Pointer class, which has no fields.
+  //
+  // TODO(36730): We'll need to extend this when we support passing/returning
+  // structs by value.
+  ASSERT(exceptional_return.IsNull() || exceptional_return.IsNumber() ||
+         exceptional_return.IsPointer());
+  if (!exceptional_return.IsSmi() && exceptional_return.IsNew()) {
+    function.SetFfiCallbackExceptionalReturn(
+        Instance::Handle(exceptional_return.CopyShallowToOldSpace(thread)));
+  } else {
+    function.SetFfiCallbackExceptionalReturn(exceptional_return);
+  }
+
   // We compile the callback immediately because we need to return a pointer to
   // the entry-point. Native calls do not use patching like Dart calls, so we
   // cannot compile it lazily.
@@ -578,23 +606,36 @@ static uword CompileNativeCallback(const Function& c_signature,
   thread->SetFfiCallbackCode(callback_id, code);
 
   return code.EntryPoint();
-#endif
+#endif  // defined(TARGET_ARCH_DBC)
 }
+#endif  // !defined(DART_PRECOMPILED_RUNTIME) && !defined(DART_PRECOMPILER)
 
-DEFINE_NATIVE_ENTRY(Ffi_fromFunction, 1, 1) {
+DEFINE_NATIVE_ENTRY(Ffi_fromFunction, 1, 2) {
+#if defined(DART_PRECOMPILED_RUNTIME) || defined(DART_PRECOMPILER)
+  UNREACHABLE();
+#else
   GET_NATIVE_TYPE_ARGUMENT(type_arg, arguments->NativeTypeArgAt(0));
   GET_NON_NULL_NATIVE_ARGUMENT(Closure, closure, arguments->NativeArgAt(0));
+  GET_NON_NULL_NATIVE_ARGUMENT(Instance, exceptional_return,
+                               arguments->NativeArgAt(1));
+
+  if (!type_arg.IsInstantiated() || !type_arg.IsFunctionType()) {
+    // TODO(35902): Remove this when dynamic invocations of fromFunction are
+    // prohibited.
+    Exceptions::ThrowUnsupportedError(
+        "Type argument to fromFunction must an instantiated function type.");
+  }
 
   const Function& native_signature =
-      Function::Handle(((Type&)type_arg).signature());
+      Function::Handle(Type::Cast(type_arg).signature());
   Function& func = Function::Handle(closure.function());
   TypeArguments& type_args = TypeArguments::Handle(zone);
   type_args = TypeArguments::New(1);
   type_args.SetTypeAt(Pointer::kNativeTypeArgPos, type_arg);
   type_args = type_args.Canonicalize();
 
-  Class& native_function_class = Class::Handle(
-      Isolate::Current()->class_table()->At(kFfiNativeFunctionCid));
+  Class& native_function_class =
+      Class::Handle(isolate->class_table()->At(kFfiNativeFunctionCid));
   native_function_class.EnsureIsFinalized(Thread::Current());
 
   Type& native_function_type = Type::Handle(
@@ -613,12 +654,30 @@ DEFINE_NATIVE_ENTRY(Ffi_fromFunction, 1, 1) {
   func = func.parent_function();
   ASSERT(func.is_static());
 
-  const uword address = CompileNativeCallback(native_signature, func);
+  const AbstractType& return_type =
+      AbstractType::Handle(native_signature.result_type());
+  if (compiler::ffi::NativeTypeIsVoid(return_type)) {
+    if (!exceptional_return.IsNull()) {
+      const String& error = String::Handle(
+          String::NewFormatted("Exceptional return argument to 'fromFunction' "
+                               "must be null for functions returning void."));
+      Exceptions::ThrowArgumentError(error);
+    }
+  } else if (!compiler::ffi::NativeTypeIsPointer(return_type) &&
+             exceptional_return.IsNull()) {
+    const String& error = String::Handle(String::NewFormatted(
+        "Exceptional return argument to 'fromFunction' must not be null."));
+    Exceptions::ThrowArgumentError(error);
+  }
+
+  const uword address =
+      CompileNativeCallback(native_signature, func, exceptional_return);
 
   const Pointer& result = Pointer::Handle(Pointer::New(
       native_function_type, Integer::Handle(zone, Integer::New(address))));
 
   return result.raw();
+#endif
 }
 
 #if defined(TARGET_ARCH_DBC)

runtime/lib/ffi_patch.dart

@@ -15,7 +15,8 @@ int sizeOf<T extends NativeType>() native "Ffi_sizeOf";
 
 @patch
 Pointer<NativeFunction<T>> fromFunction<T extends Function>(
-    @DartRepresentationOf("T") Function f) native "Ffi_fromFunction";
+    @DartRepresentationOf("T") Function f,
+    Object exceptionalReturn) native "Ffi_fromFunction";
 
 @patch
 @pragma("vm:entry-point")
@@ -53,21 +54,3 @@ class Pointer<T extends NativeType> {
   @patch
   void free() native "Ffi_free";
 }
-
-// This method gets called when an exception bubbles up to the native -> Dart
-// boundary from an FFI native callback. Since native code does not have any
-// concept of exceptions, the exception cannot be propagated any further.
-// Instead, print a warning with the exception and return 0/0.0 from the
-// callback.
-//
-// TODO(36856): Iron out the story behind exceptions.
-@pragma("vm:entry-point")
-void _handleExposedException(dynamic exception, dynamic stackTrace) {
-  print(
-      "==================== UNHANDLED EXCEPTION FROM FFI CALLBACK ====================");
-  print(
-      """ ** Native callbacks should not throw exceptions because they cannot be
-    propagated into native code. **""");
-  print("EXCEPTION: $exception");
-  print(stackTrace);
-}

runtime/vm/bootstrap_natives.h

@@ -384,7 +384,7 @@ namespace dart {
   V(Ffi_cast, 1)                                                               \
   V(Ffi_sizeOf, 0)                                                             \
   V(Ffi_asFunction, 1)                                                         \
-  V(Ffi_fromFunction, 1)                                                       \
+  V(Ffi_fromFunction, 2)                                                       \
   V(Ffi_dl_open, 1)                                                            \
   V(Ffi_dl_lookup, 2)                                                          \
   V(Ffi_dl_getHandle, 1)                                                       \

runtime/vm/compiler/frontend/kernel_to_il.cc

@@ -2454,22 +2454,6 @@ Fragment FlowGraphBuilder::FfiUnboxedExtend(Representation representation,
   return Fragment(extend);
 }
 
-Fragment FlowGraphBuilder::FfiExceptionalReturnValue(
-    const AbstractType& result_type,
-    Representation representation) {
-  ASSERT(optimizing_);
-  Object& result = Object::ZoneHandle(Z, Object::null());
-  if (representation == kUnboxedFloat || representation == kUnboxedDouble) {
-    result = Double::New(0.0, Heap::kOld);
-  } else {
-    result = Integer::New(0, Heap::kOld);
-  }
-  Fragment code;
-  code += Constant(result);
-  code += UnboxTruncate(representation);
-  return code;
-}
-
 #if !defined(TARGET_ARCH_DBC)
 Fragment FlowGraphBuilder::NativeReturn(Representation result) {
   auto* instr = new (Z)
@@ -2568,6 +2552,15 @@ Fragment FlowGraphBuilder::FfiConvertArgumentToNative(
     const AbstractType& ffi_type,
     const Representation native_representation) {
   Fragment body;
+
+  // Return 0 for void.
+  if (compiler::ffi::NativeTypeIsVoid(ffi_type)) {
+    body += Drop();
+    body += IntConstant(0);
+    body += UnboxTruncate(kUnboxedFfiIntPtr);
+    return body;
+  }
+
   // Check for 'null'. Only ffi.Pointers are allowed to be null.
   if (!compiler::ffi::NativeTypeIsPointer(ffi_type)) {
     body += LoadLocal(MakeTemporary());
@@ -2724,24 +2717,12 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiCallback(const Function& function) {
   ++catch_depth_;
   Fragment catch_body =
       CatchBlockEntry(Array::empty_array(), try_handler_index,
-                      /*needs_stacktrace=*/true, /*is_synthesized=*/true);
-
-  catch_body += LoadLocal(CurrentException());
-  catch_body += PushArgument();
-  catch_body += LoadLocal(CurrentStackTrace());
-  catch_body += PushArgument();
-
-  // Find '_handleExposedException(e, st)' from ffi_patch.dart and call it.
-  const Library& ffi_lib =
-      Library::Handle(Z, Library::LookupLibrary(thread_, Symbols::DartFfi()));
-  const Function& handler = Function::ZoneHandle(
-      Z, ffi_lib.LookupFunctionAllowPrivate(Symbols::HandleExposedException()));
-  ASSERT(!handler.IsNull());
-  catch_body += StaticCall(TokenPosition::kNoSource, handler, /*num_args=*/2,
-                           /*arg_names=*/Array::empty_array(), ICData::kStatic);
-  catch_body += Drop();
-
-  catch_body += FfiExceptionalReturnValue(ffi_type, result_rep);
+                      /*needs_stacktrace=*/false, /*is_synthesized=*/true);
+
+  // Return the "exceptional return" value given in 'fromFunction'.
+  catch_body += Constant(
+      Instance::ZoneHandle(Z, function.FfiCallbackExceptionalReturn()));
+  catch_body += FfiConvertArgumentToNative(function, ffi_type, result_rep);
   catch_body += NativeReturn(result_rep);
   --catch_depth_;