Revert "Reland "[vm/ffi] Disallow Pointers and structs in finalizers and expandos""

This reverts commit 3f0ad61daa.

Reason for revert: Conflicts with https://dart-review.googlesource.com/c/sdk/+/194765

Original change's description:
> Reland "[vm/ffi] Disallow `Pointer`s and structs in finalizers and expandos"
>
> `Dart_NewWeakPersistentHandle` and `Dart_NewFinalizableHandle` in
> `dart_api.h` do no longer accept `Pointer`s and subtypes of `Struct`s
> as values passed in to the `object` parameter.
>
> Expandos no longer accept `Pointer`s and subtypes of `Struct`s
> as values passed in to the `object` parameter.
>
> Cleans up unused object_store->ffi_struct_class.
>
> Closes: https://github.com/dart-lang/sdk/issues/45071
> Breaking change: https://github.com/dart-lang/sdk/issues/45072
>
> TEST=vm/cc/DartAPI_FinalizableHandleErrors
> TEST=vm/cc/DartAPI_WeakPersistentHandleErrors
> TEST=tests/ffi(_2)/expando_test.dart
>
> Change-Id: I9af6d0173db60614091068c218391f73756c135f
> Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-debug-x64-try
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/195061
> Reviewed-by: Martin Kustermann <kustermann@google.com>
> Commit-Queue: Daco Harkes <dacoharkes@google.com>

TBR=kustermann@google.com,dacoharkes@google.com

Change-Id: I90064b6b73155a43f37388f987c6b29f72ce9770
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/195076
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>

CHANGELOG.md

@@ -8,15 +8,6 @@
     daylight saving changes that are not precisely one hour.
     (No change on the Web which uses the JavaScript `Date` object.)
 
-### Dart VM
-
-*   **Breaking Change** [#45071][]: `Dart_NewWeakPersistentHandle`'s and
-    `Dart_NewFinalizableHandle`'s `object` parameter no longer accepts
-    `Pointer`s and subtypes of `Struct`. Expandos no longer accept
-    `Pointer`s and subtypes of `Struct`s.
-
-[#45071]: https://github.com/dart-lang/sdk/issues/45071
-
 ## 2.13.0
 
 ### Language

pkg/vm/lib/transformations/ffi_use_sites.dart

@@ -834,9 +834,7 @@ class _FfiUseSiteTransformer extends FfiTransformer {
           : null;
     }
 
-    if (!nativeTypesClasses.contains(klass) &&
-        klass != arrayClass &&
-        klass != arraySizeClass) {
+    if (!nativeTypesClasses.contains(klass) && klass != arrayClass) {
       for (final parent in nativeTypesClasses) {
         if (hierarchy.isSubtypeOf(klass, parent)) {
           return parent;

runtime/include/dart_api.h

@@ -475,7 +475,7 @@ DART_EXPORT void Dart_DeletePersistentHandle(Dart_PersistentHandle object);
  *
  * Requires there to be a current isolate.
  *
- * \param object An object with identity.
+ * \param object An object.
  * \param peer A pointer to a native object or NULL.  This value is
  *   provided to callback when it is invoked.
  * \param external_allocation_size The number of externally allocated
@@ -531,7 +531,7 @@ DART_EXPORT void Dart_UpdateExternalSize(Dart_WeakPersistentHandle object,
  *
  * Requires there to be a current isolate.
  *
- * \param object An object with identity.
+ * \param object An object.
  * \param peer A pointer to a native object or NULL.  This value is
  *   provided to callback when it is invoked.
  * \param external_allocation_size The number of externally allocated

runtime/vm/dart_api_impl.cc

@@ -991,23 +991,6 @@ DART_EXPORT void Dart_SetPersistentHandle(Dart_PersistentHandle obj1,
   obj1_ref->set_ptr(obj2_ref);
 }
 
-// TODO(https://dartbug.com/38491): Reject Unions here as well.
-static bool IsFfiStruct(Thread* T, const Object& obj) {
-  if (obj.IsNull()) {
-    return false;
-  }
-
-  // CFE guarantees we can only have direct subclasses of `Struct`
-  // (no implementations or indirect subclasses are allowed).
-  const auto& klass = Class::Handle(Z, obj.clazz());
-  const auto& super_klass = Class::Handle(Z, klass.SuperClass());
-  if (super_klass.Name() != Symbols::Struct().ptr()) {
-    return false;
-  }
-  const auto& library = Library::Handle(Z, super_klass.library());
-  return library.url() == Symbols::DartFfi().ptr();
-}
-
 static Dart_WeakPersistentHandle AllocateWeakPersistentHandle(
     Thread* thread,
     const Object& ref,
@@ -1017,13 +1000,6 @@ static Dart_WeakPersistentHandle AllocateWeakPersistentHandle(
   if (!ref.ptr()->IsHeapObject()) {
     return NULL;
   }
-  if (ref.IsPointer()) {
-    return NULL;
-  }
-  if (IsFfiStruct(thread, ref)) {
-    return NULL;
-  }
-
   FinalizablePersistentHandle* finalizable_ref =
       FinalizablePersistentHandle::New(thread->isolate_group(), ref, peer,
                                        callback, external_allocation_size,
@@ -1032,14 +1008,16 @@ static Dart_WeakPersistentHandle AllocateWeakPersistentHandle(
 }
 
 static Dart_WeakPersistentHandle AllocateWeakPersistentHandle(
-    Thread* T,
+    Thread* thread,
     Dart_Handle object,
     void* peer,
     intptr_t external_allocation_size,
     Dart_HandleFinalizer callback) {
-  const auto& ref = Object::Handle(Z, Api::UnwrapHandle(object));
-  return AllocateWeakPersistentHandle(T, ref, peer, external_allocation_size,
-                                      callback);
+  REUSABLE_OBJECT_HANDLESCOPE(thread);
+  Object& ref = thread->ObjectHandle();
+  ref = Api::UnwrapHandle(object);
+  return AllocateWeakPersistentHandle(thread, ref, peer,
+                                      external_allocation_size, callback);
 }
 
 static Dart_FinalizableHandle AllocateFinalizableHandle(
@@ -1051,12 +1029,6 @@ static Dart_FinalizableHandle AllocateFinalizableHandle(
   if (!ref.ptr()->IsHeapObject()) {
     return NULL;
   }
-  if (ref.IsPointer()) {
-    return NULL;
-  }
-  if (IsFfiStruct(thread, ref)) {
-    return NULL;
-  }
 
   FinalizablePersistentHandle* finalizable_ref =
       FinalizablePersistentHandle::New(thread->isolate_group(), ref, peer,
@@ -1066,13 +1038,15 @@ static Dart_FinalizableHandle AllocateFinalizableHandle(
 }
 
 static Dart_FinalizableHandle AllocateFinalizableHandle(
-    Thread* T,
+    Thread* thread,
     Dart_Handle object,
     void* peer,
     intptr_t external_allocation_size,
     Dart_HandleFinalizer callback) {
-  const auto& ref = Object::Handle(Z, Api::UnwrapHandle(object));
-  return AllocateFinalizableHandle(T, ref, peer, external_allocation_size,
+  REUSABLE_OBJECT_HANDLESCOPE(thread);
+  Object& ref = thread->ObjectHandle();
+  ref = Api::UnwrapHandle(object);
+  return AllocateFinalizableHandle(thread, ref, peer, external_allocation_size,
                                    callback);
 }
 
@@ -1081,13 +1055,15 @@ Dart_NewWeakPersistentHandle(Dart_Handle object,
                              void* peer,
                              intptr_t external_allocation_size,
                              Dart_HandleFinalizer callback) {
-  DARTSCOPE(Thread::Current());
+  Thread* thread = Thread::Current();
+  CHECK_ISOLATE(thread->isolate());
   if (callback == NULL) {
     return NULL;
   }
+  TransitionNativeToVM transition(thread);
 
-  return AllocateWeakPersistentHandle(T, object, peer, external_allocation_size,
-                                      callback);
+  return AllocateWeakPersistentHandle(thread, object, peer,
+                                      external_allocation_size, callback);
 }
 
 DART_EXPORT Dart_FinalizableHandle
@@ -1095,13 +1071,14 @@ Dart_NewFinalizableHandle(Dart_Handle object,
                           void* peer,
                           intptr_t external_allocation_size,
                           Dart_HandleFinalizer callback) {
-  DARTSCOPE(Thread::Current());
+  Thread* thread = Thread::Current();
+  CHECK_ISOLATE(thread->isolate());
   if (callback == nullptr) {
     return nullptr;
   }
-
-  return AllocateFinalizableHandle(T, object, peer, external_allocation_size,
-                                   callback);
+  TransitionNativeToVM transition(thread);
+  return AllocateFinalizableHandle(thread, object, peer,
+                                   external_allocation_size, callback);
 }
 
 DART_EXPORT void Dart_UpdateExternalSize(Dart_WeakPersistentHandle object,

runtime/vm/dart_api_impl_test.cc

@@ -3368,35 +3368,6 @@ TEST_CASE(DartAPI_WeakPersistentHandleErrors) {
       Dart_NewWeakPersistentHandle(obj2, NULL, 0, NopCallback);
   EXPECT_EQ(ref2, static_cast<void*>(NULL));
 
-  // Pointer object.
-  Dart_Handle ffi_lib = Dart_LookupLibrary(NewString("dart:ffi"));
-  Dart_Handle pointer_type =
-      Dart_GetNonNullableType(ffi_lib, NewString("Pointer"), 0, NULL);
-  Dart_Handle obj3 = Dart_Allocate(pointer_type);
-  EXPECT_VALID(obj3);
-  Dart_WeakPersistentHandle ref3 =
-      Dart_NewWeakPersistentHandle(obj3, nullptr, 0, FinalizableHandleCallback);
-  EXPECT_EQ(ref3, static_cast<void*>(nullptr));
-
-  // Subtype of Struct object.
-  const char* kScriptChars = R"(
-      import 'dart:ffi';
-
-      class MyStruct extends Struct {
-        external Pointer notEmpty;
-      }
-  )";
-  Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, NULL);
-  Dart_Handle my_struct_type =
-      Dart_GetNonNullableType(lib, NewString("MyStruct"), 0, NULL);
-  Dart_Handle obj4 = Dart_Allocate(my_struct_type);
-  EXPECT_VALID(obj4);
-  Dart_WeakPersistentHandle ref4 =
-      Dart_NewWeakPersistentHandle(obj4, nullptr, 0, FinalizableHandleCallback);
-  EXPECT_EQ(ref4, static_cast<void*>(nullptr));
-
-  // TODO(https://dartbug.com/38491): Reject Unions here as well.
-
   Dart_ExitScope();
 }
 
@@ -3417,33 +3388,6 @@ TEST_CASE(DartAPI_FinalizableHandleErrors) {
       Dart_NewFinalizableHandle(obj2, nullptr, 0, FinalizableHandleCallback);
   EXPECT_EQ(ref2, static_cast<void*>(nullptr));
 
-  // Pointer object.
-  Dart_Handle ffi_lib = Dart_LookupLibrary(NewString("dart:ffi"));
-  Dart_Handle pointer_type =
-      Dart_GetNonNullableType(ffi_lib, NewString("Pointer"), 0, NULL);
-  Dart_Handle obj3 = Dart_Allocate(pointer_type);
-  EXPECT_VALID(obj3);
-  Dart_FinalizableHandle ref3 =
-      Dart_NewFinalizableHandle(obj3, nullptr, 0, FinalizableHandleCallback);
-  EXPECT_EQ(ref3, static_cast<void*>(nullptr));
-
-  // Subtype of Struct object.
-  const char* kScriptChars = R"(
-      import 'dart:ffi';
-
-      class MyStruct extends Struct {
-        external Pointer notEmpty;
-      }
-  )";
-  Dart_Handle lib = TestCase::LoadTestScript(kScriptChars, NULL);
-  Dart_Handle my_struct_type =
-      Dart_GetNonNullableType(lib, NewString("MyStruct"), 0, NULL);
-  Dart_Handle obj4 = Dart_Allocate(my_struct_type);
-  EXPECT_VALID(obj4);
-  Dart_FinalizableHandle ref4 =
-      Dart_NewFinalizableHandle(obj4, nullptr, 0, FinalizableHandleCallback);
-  EXPECT_EQ(ref4, static_cast<void*>(nullptr));
-
   Dart_ExitScope();
 }
 

runtime/vm/kernel_loader.cc

@@ -1367,8 +1367,7 @@ void KernelLoader::LoadLibraryImportsAndExports(Library* library,
           "runtime");
     }
     if (!Api::IsFfiEnabled() &&
-        target_library.url() == Symbols::DartFfi().ptr() &&
-        library->url() != Symbols::DartCore().ptr()) {
+        target_library.url() == Symbols::DartFfi().ptr()) {
       H.ReportError(
           "import of dart:ffi is not supported in the current Dart runtime");
     }

runtime/vm/object_store.h

@@ -236,6 +236,7 @@ class ObjectPointerVisitor;
   RW(GrowableObjectArray, ffi_callback_functions)                              \
   RW(Class, ffi_pointer_class)                                                 \
   RW(Class, ffi_native_type_class)                                             \
+  RW(Class, ffi_struct_class)                                                  \
   RW(Object, ffi_as_function_internal)                                         \
   // Please remember the last entry must be referred in the 'to' function below.
 

sdk/lib/_internal/vm/lib/core_patch.dart

@@ -49,8 +49,6 @@ import "dart:collection"
 
 import "dart:convert" show ascii, Encoding, json, latin1, utf8;
 
-import "dart:ffi" show Pointer, Struct;
-
 import "dart:isolate" show Isolate;
 
 import "dart:math" show Random;

sdk/lib/_internal/vm/lib/expando_patch.dart

@@ -140,12 +140,9 @@ class Expando<T> {
     if ((object == null) ||
         (object is bool) ||
         (object is num) ||
-        (object is String) ||
-        (object is Pointer) ||
-        (object is Struct)) {
-      // TODO(https://dartbug.com/38491): Reject Unions here as well.
+        (object is String)) {
       throw new ArgumentError.value(object,
-          "Expandos are not allowed on strings, numbers, booleans, null, Pointers or Structs.");
+          "Expandos are not allowed on strings, numbers, booleans or null");
     }
   }
 

sdk/lib/core/expando.dart

@@ -6,8 +6,7 @@ part of dart.core;
 
 /// An [Expando] allows adding new properties to objects.
 ///
-/// Does not work on numbers, strings, booleans, `null`, `dart:ffi` pointers
-/// or `dart:ffi` structs.
+/// Does not work on numbers, strings, booleans or `null`.
 ///
 /// An `Expando` does not hold on to the added property value after an object
 /// becomes inaccessible.
@@ -39,15 +38,13 @@ class Expando<T extends Object> {
   /// object. If the object hasn't been expanded, the method returns
   /// `null`.
   ///
-  /// The object must not be a number, a string, a boolean, `null`, a
-  /// `dart:ffi` pointer, or a `dart:ffi` struct.
+  /// The object must not be a number, a string or a boolean.
   external T? operator [](Object object);
 
   /// Sets the value of this [Expando]'s property on the given
   /// object. Properties can effectively be removed again by setting
   /// their value to `null`.
   ///
-  /// The object must not be a number, a string, a boolean, `null`, a
-  /// `dart:ffi` pointer, or a `dart:ffi` struct.
+  /// The object must not be a number, a string or a boolean.
   external void operator []=(Object object, T? value);
 }

tests/corelib/expando_test.dart

@@ -1,9 +1,6 @@
 // Copyright (c) 2012, 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.
-//
-// VMOptions=--enable-ffi=true
-// VMOptions=--enable-ffi=false
 
 import "package:expect/expect.dart";
 

tests/corelib_2/expando_test.dart

@@ -1,9 +1,6 @@
 // Copyright (c) 2012, 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.
-//
-// VMOptions=--enable-ffi=true
-// VMOptions=--enable-ffi=false
 
 import "package:expect/expect.dart";
 

tests/ffi/expando_test.dart

@@ -1,43 +0,0 @@
-// Copyright (c) 2021, 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.
-//
-// Dart test program for testing dart:ffi primitive data pointers.
-//
-// SharedObjects=ffi_test_functions
-
-import 'dart:ffi';
-
-import "package:expect/expect.dart";
-
-void main() {
-  testPointer();
-  testStruct();
-}
-
-Expando<int> expando = Expando('myExpando');
-
-void testPointer() {
-  final pointer = Pointer<Int8>.fromAddress(0xdeadbeef);
-  Expect.throws(() {
-    expando[pointer];
-  });
-  Expect.throws(() {
-    expando[pointer] = 1234;
-  });
-}
-
-class MyStruct extends Struct {
-  external Pointer notEmpty;
-}
-
-void testStruct() {
-  final pointer = Pointer<MyStruct>.fromAddress(0xdeadbeef);
-  final struct = pointer.ref;
-  Expect.throws(() {
-    expando[struct];
-  });
-  Expect.throws(() {
-    expando[struct] = 1234;
-  });
-}

tests/ffi_2/expando_test.dart

@@ -1,43 +0,0 @@
-// Copyright (c) 2021, 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.
-//
-// Dart test program for testing dart:ffi primitive data pointers.
-//
-// SharedObjects=ffi_test_functions
-
-import 'dart:ffi';
-
-import "package:expect/expect.dart";
-
-void main() {
-  testPointer();
-  testStruct();
-}
-
-Expando<int> expando = Expando('myExpando');
-
-void testPointer() {
-  final pointer = Pointer<Int8>.fromAddress(0xdeadbeef);
-  Expect.throws(() {
-    expando[pointer];
-  });
-  Expect.throws(() {
-    expando[pointer] = 1234;
-  });
-}
-
-class MyStruct extends Struct {
-  Pointer notEmpty;
-}
-
-void testStruct() {
-  final pointer = Pointer<MyStruct>.fromAddress(0xdeadbeef);
-  final struct = pointer.ref;
-  Expect.throws(() {
-    expando[struct];
-  });
-  Expect.throws(() {
-    expando[struct] = 1234;
-  });
-}