[vm/ffi] Fix snapshot serialization of FFI objects.

Change-Id: I78557211da79f5282339fbcf30b3f81ab56cb361
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/106345
Commit-Queue: Samir Jindel <[email protected]>
Auto-Submit: Samir Jindel <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>

Author: sjindel-google

runtime/vm/clustered_snapshot.cc

@@ -6,6 +6,7 @@
 
 #include "platform/assert.h"
 #include "vm/bootstrap.h"
+#include "vm/class_id.h"
 #include "vm/compiler/backend/code_statistics.h"
 #include "vm/compiler/frontend/bytecode_reader.h"
 #include "vm/compiler/relocation.h"
@@ -767,6 +768,74 @@ class SignatureDataDeserializationCluster : public DeserializationCluster {
   }
 };
 
+#if !defined(DART_PRECOMPILED_RUNTIME)
+class FfiTrampolineDataSerializationCluster : public SerializationCluster {
+ public:
+  FfiTrampolineDataSerializationCluster()
+      : SerializationCluster("FfiTrampolineData") {}
+  ~FfiTrampolineDataSerializationCluster() {}
+
+  void Trace(Serializer* s, RawObject* object) {
+    RawFfiTrampolineData* data = FfiTrampolineData::RawCast(object);
+    objects_.Add(data);
+    PushFromTo(data);
+  }
+
+  void WriteAlloc(Serializer* s) {
+    s->WriteCid(kFfiTrampolineDataCid);
+    const intptr_t count = objects_.length();
+    s->WriteUnsigned(count);
+    for (intptr_t i = 0; i < count; i++) {
+      s->AssignRef(objects_[i]);
+    }
+  }
+
+  void WriteFill(Serializer* s) {
+    intptr_t count = objects_.length();
+    for (intptr_t i = 0; i < count; i++) {
+      RawFfiTrampolineData* const data = objects_[i];
+      AutoTraceObject(data);
+      WriteFromTo(data);
+
+      // TODO(37295): FFI callbacks shouldn't be written to a snapshot. They
+      // should only be referenced by the callback registry in Thread.
+      ASSERT(data->ptr()->callback_id_ == 0);
+    }
+  }
+
+ private:
+  GrowableArray<RawFfiTrampolineData*> objects_;
+};
+#endif  // !DART_PRECOMPILED_RUNTIME
+
+class FfiTrampolineDataDeserializationCluster : public DeserializationCluster {
+ public:
+  FfiTrampolineDataDeserializationCluster() {}
+  ~FfiTrampolineDataDeserializationCluster() {}
+
+  void ReadAlloc(Deserializer* d) {
+    start_index_ = d->next_index();
+    PageSpace* old_space = d->heap()->old_space();
+    intptr_t count = d->ReadUnsigned();
+    for (intptr_t i = 0; i < count; i++) {
+      d->AssignRef(
+          AllocateUninitialized(old_space, FfiTrampolineData::InstanceSize()));
+    }
+    stop_index_ = d->next_index();
+  }
+
+  void ReadFill(Deserializer* d) {
+    for (intptr_t id = start_index_; id < stop_index_; id++) {
+      RawFfiTrampolineData* data =
+          reinterpret_cast<RawFfiTrampolineData*>(d->Ref(id));
+      Deserializer::InitializeHeader(data, kFfiTrampolineDataCid,
+                                     FfiTrampolineData::InstanceSize());
+      ReadFromTo(data);
+      data->ptr()->callback_id_ = 0;
+    }
+  }
+};
+
 #if !defined(DART_PRECOMPILED_RUNTIME)
 class RedirectionDataSerializationCluster : public SerializationCluster {
  public:
@@ -4308,6 +4377,8 @@ SerializationCluster* Serializer::NewClusterForClass(intptr_t cid) {
       return new (Z) SignatureDataSerializationCluster();
     case kRedirectionDataCid:
       return new (Z) RedirectionDataSerializationCluster();
+    case kFfiTrampolineDataCid:
+      return new (Z) FfiTrampolineDataSerializationCluster();
     case kFieldCid:
       return new (Z) FieldSerializationCluster();
     case kScriptCid:
@@ -4944,6 +5015,8 @@ DeserializationCluster* Deserializer::ReadCluster() {
       return new (Z) SignatureDataDeserializationCluster();
     case kRedirectionDataCid:
       return new (Z) RedirectionDataDeserializationCluster();
+    case kFfiTrampolineDataCid:
+      return new (Z) FfiTrampolineDataDeserializationCluster();
     case kFieldCid:
       return new (Z) FieldDeserializationCluster();
     case kScriptCid:

runtime/vm/object.cc

@@ -8271,7 +8271,7 @@ RawFfiTrampolineData* FfiTrampolineData::New() {
       Object::Allocate(FfiTrampolineData::kClassId,
                        FfiTrampolineData::InstanceSize(), Heap::kOld);
   RawFfiTrampolineData* data = reinterpret_cast<RawFfiTrampolineData*>(raw);
-  data->ptr()->callback_id_ = -1;
+  data->ptr()->callback_id_ = 0;
   return data;
 }
 

runtime/vm/raw_object.h

@@ -1034,13 +1034,17 @@ class RawFfiTrampolineData : public RawObject {
   RawFunction* callback_target_;
 
   VISIT_TO(RawObject*, callback_target_);
+  RawObject** to_snapshot(Snapshot::Kind kind) { return to(); }
 
-  // Callback id for callbacks, otherwise 0.
+  // Callback id for callbacks.
   //
   // The callbacks ids are used so that native callbacks can lookup their own
   // code objects, since native code doesn't pass code objects into function
   // calls. The callback id is also used to for verifying that callbacks are
   // called on the correct isolate. See DLRT_VerifyCallbackIsolate for details.
+  //
+  // Will be 0 for non-callbacks. Check 'callback_target_' to determine if this
+  // is a callback or not.
   uint32_t callback_id_;
 };
 

runtime/vm/snapshot.cc

@@ -1134,7 +1134,12 @@ void SnapshotWriter::WriteMarkedObjectImpl(RawObject* raw,
 
 #define SNAPSHOT_WRITE(clazz) case kFfi##clazz##Cid:
 
-    CLASS_LIST_FFI(SNAPSHOT_WRITE) { UNREACHABLE(); }
+      CLASS_LIST_FFI(SNAPSHOT_WRITE) {
+        SetWriteException(Exceptions::kArgument,
+                          "Native objects (from dart:ffi) such as Pointers and "
+                          "Structs cannot be passed between isolates.");
+        UNREACHABLE();
+      }
 #undef SNAPSHOT_WRITE
     default:
       break;

tests/ffi/ffi.status

@@ -13,14 +13,6 @@ data_not_asan_test: SkipByDesign # This test tries to allocate too much memory o
 function_structs_test: Skip
 structs_test: Skip
 
-# dartbug.com/35934
-[ $compiler == app_jitk ]
-dynamic_library_test: Skip
-function_callbacks_test: Skip
-function_structs_test: Skip
-function_test: Skip
-negative_function_test: Skip
-
 [ $arch == x64 || $arch == arm64 || $arch == simdbc64 ]
 enable_structs_test: SkipByDesign  # Tests that structs don't work on 32-bit systems.
 
@@ -44,3 +36,10 @@ enable_structs_test: SkipByDesign  # Tests that structs don't work on 32-bit sys
 
 [ $arch == simdbc64 ]
 function_callbacks_test: Skip  # Issue 37140
+
+# These tests trigger and catch an abort (intentionally) and terminate the VM
+# before it can generate a snapshot.
+[ $compiler == app_jitk ]
+function_callbacks_test/01: Skip
+function_callbacks_test/02: Skip
+function_callbacks_test/03: Skip

tests/ffi/snapshot_test.dart

@@ -0,0 +1,24 @@
+// Copyright (c) 2019, 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.
+//
+// Checks that the VM throws an appropriate exception when FFI objects are
+// passed between isolates.
+
+import 'dart:async';
+import 'dart:ffi';
+import 'dart:io';
+import 'dart:isolate';
+
+import 'package:expect/expect.dart';
+
+main(args) async {
+  try {
+    await Isolate.spawn(print, fromAddress<Pointer<Void>>(1));
+  } catch (e) {
+    Expect.type<ArgumentError>(e);
+    return;
+  }
+
+  throw "Test didn't throw an exception!";
+}