[vm/ffi] Remove hacks and instability from FFI GC tests.

This is based on https://dart-review.googlesource.com/c/sdk/+/100583/2,
but instead of relying on private symbols, we expose an API function which
is a no-op outside specific tests.

Fixes https://github.com/dart-lang/sdk/issues/36723

Change-Id: Ieb78caa2d14956a2b5f985533b9b73e941d2d9c9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/103801
Commit-Queue: Samir Jindel <sjindel@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Daco Harkes <dacoharkes@google.com>

runtime/bin/ffi_test_functions.cc

@@ -22,6 +22,7 @@
 #include <limits>
 
 #include "include/dart_api.h"
+#include "include/dart_native_api.h"
 
 namespace dart {
 
@@ -458,42 +459,39 @@ DART_EXPORT float InventFloatValue() {
 // Functions for stress-testing.
 
 DART_EXPORT int64_t MinInt64() {
+  Dart_ExecuteInternalCommand("gc-on-next-allocation");
   return 0x8000000000000000;
 }
 
 DART_EXPORT int64_t MinInt32() {
+  Dart_ExecuteInternalCommand("gc-on-next-allocation");
   return 0x80000000;
 }
 
 DART_EXPORT double SmallDouble() {
+  Dart_ExecuteInternalCommand("gc-on-next-allocation");
   return 0x80000000 * -1.0;
 }
 
 // Requires boxing on 32-bit and 64-bit systems, even if the top 32-bits are
 // truncated.
 DART_EXPORT void* LargePointer() {
+  Dart_ExecuteInternalCommand("gc-on-next-allocation");
   uint64_t origin = 0x8100000082000000;
   return reinterpret_cast<void*>(origin);
 }
 
-// Allocates 'count'-many Mint boxes, to stress-test GC during an FFI call.
-DART_EXPORT void AllocateMints(uint64_t count) {
-  Dart_EnterScope();
-  for (uint64_t i = 0; i < count; ++i) {
-    Dart_NewInteger(0x8000000000000001);
-  }
-  Dart_ExitScope();
+DART_EXPORT void TriggerGC(uint64_t count) {
+  Dart_ExecuteInternalCommand("gc-now");
 }
 
 // Calls a Dart function to allocate 'count' objects.
 // Used for stress-testing GC when re-entering the API.
-DART_EXPORT void AllocateThroughDart(uint64_t count) {
+DART_EXPORT void AllocateThroughDart() {
   Dart_EnterScope();
   Dart_Handle root = Dart_RootLibrary();
-  Dart_Handle arguments[1] = {Dart_NewIntegerFromUint64(count)};
   Dart_Handle result = Dart_Invoke(
-      root, Dart_NewStringFromCString("testAllocationsInDartHelper"), 1,
-      arguments);
+      root, Dart_NewStringFromCString("testAllocationsInDartHelper"), 0, NULL);
   const char* error;
   if (Dart_IsError(result)) {
     Dart_StringToCString(Dart_ToString(result), &error);
@@ -506,21 +504,6 @@ DART_EXPORT void AllocateThroughDart(uint64_t count) {
   Dart_ExitScope();
 }
 
-#if !defined(_WIN32)
-DART_EXPORT int RedirectStderr() {
-  char filename[256];
-  snprintf(filename, sizeof(filename), "/tmp/captured_stderr_%d", getpid());
-  FILE* f = freopen(filename, "w", stderr);
-  if (f == nullptr) {
-    fprintf(stderr, "Could not open temp file.\n");
-    Dart_PrepareToAbort();
-    abort();
-  }
-  printf("Got file %s\n", filename);
-  return getpid();
-}
-#endif
-
 ////////////////////////////////////////////////////////////////////////////////
 // Tests for callbacks.
 

runtime/include/dart_native_api.h

@@ -175,4 +175,10 @@ DART_EXPORT DART_WARN_UNUSED_RESULT Dart_Handle Dart_ReadAllBytecode();
  */
 DART_EXPORT DART_WARN_UNUSED_RESULT Dart_Handle Dart_FinalizeAllClasses();
 
+/*  This function is intentionally undocumented.
+ *
+ *  It should not be used outside internal tests.
+ */
+DART_EXPORT void Dart_ExecuteInternalCommand(const char* command);
+
 #endif /* INCLUDE_DART_NATIVE_API_H_ */ /* NOLINT */

runtime/vm/heap/heap.cc

@@ -43,7 +43,8 @@ Heap::Heap(Isolate* isolate,
       barrier_done_(),
       read_only_(false),
       gc_new_space_in_progress_(false),
-      gc_old_space_in_progress_(false) {
+      gc_old_space_in_progress_(false),
+      gc_on_next_allocation_(false) {
   UpdateGlobalMaxUsed();
   for (int sel = 0; sel < kNumWeakSelectors; sel++) {
     new_weak_tables_[sel] = new WeakTable();
@@ -80,6 +81,7 @@ void Heap::AbandonRemainingTLAB(Thread* thread) {
 
 uword Heap::AllocateNew(intptr_t size) {
   ASSERT(Thread::Current()->no_safepoint_scope_depth() == 0);
+  CollectForDebugging();
   Thread* thread = Thread::Current();
   uword addr = new_space_.TryAllocateInTLAB(thread, size);
   if (addr != 0) {
@@ -125,6 +127,7 @@ uword Heap::AllocateNew(intptr_t size) {
 
 uword Heap::AllocateOld(intptr_t size, HeapPage::PageType type) {
   ASSERT(Thread::Current()->no_safepoint_scope_depth() == 0);
+  CollectForDebugging();
   uword addr = old_space_.TryAllocate(size, type);
   if (addr != 0) {
     return addr;
@@ -652,6 +655,17 @@ void Heap::AddRegionsToObjectSet(ObjectSet* set) const {
   old_space_.AddRegionsToObjectSet(set);
 }
 
+void Heap::CollectOnNextAllocation() {
+  AbandonRemainingTLAB(Thread::Current());
+  gc_on_next_allocation_ = true;
+}
+
+void Heap::CollectForDebugging() {
+  if (!gc_on_next_allocation_) return;
+  CollectAllGarbage(kDebugging);
+  gc_on_next_allocation_ = false;
+}
+
 ObjectSet* Heap::CreateAllocatedObjectSet(
     Zone* zone,
     MarkExpectation mark_expectation) const {

runtime/vm/heap/heap.h

@@ -302,6 +302,8 @@ class Heap {
   void MakeTLABIterable(Thread* thread);
   void AbandonRemainingTLAB(Thread* thread);
 
+  void CollectOnNextAllocation();
+
  private:
   class GCStats : public ValueObject {
    public:
@@ -373,6 +375,9 @@ class Heap {
 
   void AddRegionsToObjectSet(ObjectSet* set) const;
 
+  // Trigger major GC if 'gc_on_next_allocation_' is set.
+  void CollectForDebugging();
+
   Isolate* isolate_;
 
   // The different spaces used for allocation.
@@ -396,6 +401,11 @@ class Heap {
   bool gc_new_space_in_progress_;
   bool gc_old_space_in_progress_;
 
+  // Whether the next heap allocation (new or old) should trigger
+  // CollectAllGarbage. Used within unit tests for testing GC on certain
+  // sensitive codepaths.
+  bool gc_on_next_allocation_;
+
   friend class Become;       // VisitObjectPointers
   friend class GCCompactor;  // VisitObjectPointers
   friend class Precompiler;  // VisitObjects

runtime/vm/lockers.h

@@ -46,7 +46,11 @@ const bool kDontAssertNoSafepointScope = false;
 class MutexLocker : public ValueObject {
  public:
   explicit MutexLocker(Mutex* mutex, bool no_safepoint_scope = true)
-      : mutex_(mutex), no_safepoint_scope_(no_safepoint_scope) {
+      :
+#if defined(DEBUG)
+        no_safepoint_scope_(no_safepoint_scope),
+#endif
+        mutex_(mutex) {
     ASSERT(mutex != NULL);
 #if defined(DEBUG)
     if (no_safepoint_scope_) {
@@ -88,8 +92,8 @@ class MutexLocker : public ValueObject {
   }
 
  private:
+  DEBUG_ONLY(bool no_safepoint_scope_;)
   Mutex* const mutex_;
-  bool no_safepoint_scope_;
 
   DISALLOW_COPY_AND_ASSIGN(MutexLocker);
 };

runtime/vm/native_api_impl.cc

@@ -237,4 +237,17 @@ DART_EXPORT Dart_Handle Dart_FinalizeAllClasses() {
 #endif  // defined(DART_PRECOMPILED_RUNTIME)
 }
 
+DART_EXPORT void Dart_ExecuteInternalCommand(const char* command) {
+  if (!FLAG_enable_testing_pragmas) return;
+
+  TransitionNativeToVM _(Thread::Current());
+  if (!strcmp(command, "gc-on-next-allocation")) {
+    Isolate::Current()->heap()->CollectOnNextAllocation();
+  } else if (!strcmp(command, "gc-now")) {
+    Isolate::Current()->heap()->CollectAllGarbage(Heap::kDebugging);
+  } else {
+    UNREACHABLE();
+  }
+}
+
 }  // namespace dart

tests/ffi/function_gc_test.dart

@@ -2,8 +2,8 @@
 // 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=--deterministic --optimization-counter-threshold=500 --verbose-gc
-// VMOptions=--deterministic --optimization-counter-threshold=-1 --verbose-gc
+// VMOptions=--deterministic --optimization-counter-threshold=500 --enable-testing-pragmas
+// VMOptions=--deterministic --optimization-counter-threshold=-1 --enable-testing-pragmas
 //
 // Dart test program for stress-testing boxing and GC in return paths from FFI
 // trampolines.
@@ -16,45 +16,14 @@
 import 'dart:ffi' as ffi;
 import 'dylib_utils.dart';
 import "package:expect/expect.dart";
-import 'gc_helper.dart';
-
-test(GCWatcher watcher, Function testee,
-    {bool mustTriggerGC: true, bool batched: false}) async {
-  // Warmup.
-  for (int i = 0; i < 1000; ++i) {
-    batched ? testee(1) : testee();
-  }
-  int size = await watcher.size();
-  for (int i = 0; i < 1000000;) {
-    if (batched) {
-      testee(1000);
-      i += 1000;
-    } else {
-      testee();
-      i++;
-    }
-  }
-  int new_size = await watcher.size();
-  if (mustTriggerGC) {
-    print("Expect $new_size > $size.");
-    Expect.isTrue(new_size > size);
-  }
-}
 
 main() async {
-  final watcher = GCWatcher.ifAvailable();
-  try {
-    await test(watcher, testBoxInt64);
-    // On 64-bit platforms this won't trigger GC because the result fits into a
-    // Smi.
-    await test(watcher, testBoxInt32, mustTriggerGC: false);
-    await test(watcher, testBoxDouble);
-    await test(watcher, testBoxPointer);
-    await test(watcher, testAllocateMints, batched: true);
-    await test(watcher, testAllocationsInDart, batched: true);
-  } finally {
-    watcher.dispose();
-  }
+  testBoxInt64();
+  testBoxInt32();
+  testBoxDouble();
+  testBoxPointer();
+  testAllocateInNative();
+  testAllocateInDart();
 }
 
 ffi.DynamicLibrary ffiTestFunctions =
@@ -64,11 +33,13 @@ typedef NativeNullaryOp64 = ffi.Int64 Function();
 typedef NativeNullaryOp32 = ffi.Int32 Function();
 typedef NativeNullaryOpDouble = ffi.Double Function();
 typedef NativeNullaryOpPtr = ffi.Pointer<ffi.Void> Function();
+typedef NativeNullaryOp = ffi.Void Function();
 typedef NativeUnaryOp = ffi.Void Function(ffi.Uint64);
 typedef NullaryOp = int Function();
 typedef NullaryOpDbl = double Function();
 typedef NullaryOpPtr = ffi.Pointer<ffi.Void> Function();
 typedef UnaryOp = void Function(int);
+typedef NullaryOpVoid = void Function();
 
 //// These functions return values that require boxing into different types.
 
@@ -111,33 +82,19 @@ void testBoxPointer() {
   }
 }
 
-final allocateMint =
-    ffiTestFunctions.lookupFunction<NativeUnaryOp, UnaryOp>("AllocateMints");
+final triggerGc = ffiTestFunctions
+    .lookupFunction<NativeNullaryOp, NullaryOpVoid>("TriggerGC");
 
-// Test GC in the FFI call path by calling a C function which allocates through
-// the Dart API.
-void testAllocateMints(int batchSize) {
-  allocateMint(batchSize);
-}
+// Test GC in the FFI call path by calling a C function which triggers GC
+// directly.
+void testAllocateInNative() => triggerGc();
 
-class C {
-  final int i;
-  C(this.i);
-}
-
-C c = null;
 @pragma("vm:entry-point", "call")
-void testAllocationsInDartHelper(int count) {
-  for (int i = 0; i < count; ++i) {
-    c = C(i);
-  }
-}
+void testAllocationsInDartHelper() => triggerGc();
 
 final allocateThroughDart = ffiTestFunctions
-    .lookupFunction<NativeUnaryOp, UnaryOp>("AllocateThroughDart");
+    .lookupFunction<NativeNullaryOp, NullaryOpVoid>("AllocateThroughDart");
 
 // Test GC in the FFI call path by calling a C function which allocates by
 // calling back into Dart ('testAllocationsInDartHelper').
-void testAllocationsInDart(int batchSize) {
-  allocateThroughDart(batchSize * 10);
-}
+void testAllocateInDart() => allocateThroughDart();

tests/ffi/gc_helper.dart

@@ -1,54 +0,0 @@
-// 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.
-
-import 'dart:io';
-import 'dylib_utils.dart';
-import 'dart:ffi';
-import 'dart:io' show Platform;
-
-DynamicLibrary ffiTestFunctions = dlopenPlatformSpecific("ffi_test_functions");
-
-const bool isProduct = const bool.fromEnvironment("dart.vm.product");
-
-abstract class GCWatcher {
-  factory GCWatcher() => _GCWatcherImpl();
-  factory GCWatcher.dummy() => _MockGCWatcher();
-  factory GCWatcher.ifAvailable() =>
-      (Platform.isWindows || Platform.isAndroid || isProduct)
-          ? GCWatcher.dummy()
-          : GCWatcher();
-
-  Future<int> size();
-  void dispose();
-}
-
-// Requires --verbose-gc.
-class _GCWatcherImpl implements GCWatcher {
-  int _suffix;
-
-  Future<int> size() async {
-    return await File("/tmp/captured_stderr_$_suffix").length();
-  }
-
-  _GCWatcherImpl() {
-    print("Starting...");
-    _suffix = ffiTestFunctions
-        .lookupFunction<Int32 Function(), int Function()>("RedirectStderr")();
-  }
-
-  dispose() {
-    try {
-      File("/tmp/captured_stderr_$_suffix").deleteSync();
-    } catch (e) {
-      print("deleting file failed");
-    }
-  }
-}
-
-class _MockGCWatcher implements GCWatcher {
-  int _ctr = 0;
-
-  Future<int> size() async => ++_ctr;
-  dispose() {}
-}

tests/ffi/subtype_test.dart

@@ -4,6 +4,7 @@
 //
 // Dart test program for testing dart:ffi Pointer subtypes.
 //
+// SharedObjects=ffi_test_functions
 // VMOptions=--verbose-gc
 
 library FfiTest;
@@ -12,21 +13,21 @@ import 'dart:ffi' as ffi;
 
 import "package:expect/expect.dart";
 
-import 'gc_helper.dart';
 import 'cstring.dart';
+import 'dylib_utils.dart';
+
+ffi.DynamicLibrary ffiTestFunctions =
+    dlopenPlatformSpecific("ffi_test_functions");
+
+final triggerGc = ffiTestFunctions
+    .lookupFunction<ffi.Void Function(), void Function()>("TriggerGC");
 
 void main() async {
   testAllocate();
   testSizeOf();
-  await testGC();
+  testGC();
 }
 
-class X {
-  X(this.i);
-  int i;
-}
-
-dynamic foo;
 dynamic bar;
 
 void testAllocate() {
@@ -36,15 +37,10 @@ void testAllocate() {
 }
 
 Future<void> testGC() async {
-  CString cs = ffi.fromAddress<CString>(11);
-  bar = cs;
-  foo = "";
-  final watcher = GCWatcher.ifAvailable();
-  int counts = await watcher.size();
-  for (int i = 0; i < 1000000; ++i) {
-    foo = new X(i);
-  }
-  Expect.isTrue(await watcher.size() > counts);
+  bar = ffi.fromAddress<CString>(11);
+  // Verify that the objects manufactured by 'fromAddress' can be scanned by the
+  // GC.
+  triggerGc();
 }
 
 void testSizeOf() {