From 670d40d8081d31d210a867feecbd4aab59063ab2 Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Wed, 21 Aug 2019 08:10:34 +0000 Subject: [PATCH] [vm/ffi] regression test for 37511 I tested this test manually by reverting 48d92b317657718ae764d494e26bd567f14c71bd to confirm that it makes the test segfault. Closes: https://github.com/dart-lang/sdk/issues/37511 Change-Id: I62cb2b83775780a2fccfd9ee4ebff793de82090a Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try, app-kernel-linux-debug-x64-try, vm-kernel-linux-debug-simdbc64-try,vm-kernel-linux-debug-ia32-try,vm-dartkb-linux-debug-simarm64-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-dartkb-linux-debug-x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-dartkb-linux-release-x64-abi-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/109703 Commit-Queue: Daco Harkes Reviewed-by: Samir Jindel --- runtime/bin/ffi_test/ffi_test_functions.cc | 17 ++- runtime/lib/ffi_dynamic_library_patch.dart | 2 +- runtime/vm/heap/heap.cc | 19 ++-- runtime/vm/heap/heap.h | 8 +- runtime/vm/native_api_impl.cc | 7 +- tests/ffi/analysis_options.yaml | 3 + tests/ffi/ffi.status | 1 + tests/ffi/ffi_test_helpers.dart | 8 ++ tests/ffi/gc_helpers.dart | 16 --- tests/ffi/regress_37511_callbacks_test.dart | 54 ++++++++++ tests/ffi/regress_37511_test.dart | 108 ++++++++++++++++++++ 11 files changed, 211 insertions(+), 32 deletions(-) create mode 100644 tests/ffi/analysis_options.yaml delete mode 100644 tests/ffi/gc_helpers.dart create mode 100644 tests/ffi/regress_37511_callbacks_test.dart create mode 100644 tests/ffi/regress_37511_test.dart diff --git a/runtime/bin/ffi_test/ffi_test_functions.cc b/runtime/bin/ffi_test/ffi_test_functions.cc index fe44259e194..4c203a381fd 100644 --- a/runtime/bin/ffi_test/ffi_test_functions.cc +++ b/runtime/bin/ffi_test/ffi_test_functions.cc @@ -499,24 +499,28 @@ DART_EXPORT float InventFloatValue() { // Functions for stress-testing. DART_EXPORT int64_t MinInt64() { - Dart_ExecuteInternalCommand("gc-on-next-allocation", nullptr); + Dart_ExecuteInternalCommand("gc-on-nth-allocation", + reinterpret_cast(1)); return 0x8000000000000000; } DART_EXPORT int64_t MinInt32() { - Dart_ExecuteInternalCommand("gc-on-next-allocation", nullptr); + Dart_ExecuteInternalCommand("gc-on-nth-allocation", + reinterpret_cast(1)); return 0x80000000; } DART_EXPORT double SmallDouble() { - Dart_ExecuteInternalCommand("gc-on-next-allocation", nullptr); + Dart_ExecuteInternalCommand("gc-on-nth-allocation", + reinterpret_cast(1)); 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", nullptr); + Dart_ExecuteInternalCommand("gc-on-nth-allocation", + reinterpret_cast(1)); uint64_t origin = 0x8100000082000000; return reinterpret_cast(origin); } @@ -525,6 +529,11 @@ DART_EXPORT void TriggerGC(uint64_t count) { Dart_ExecuteInternalCommand("gc-now", nullptr); } +DART_EXPORT void CollectOnNthAllocation(intptr_t num_allocations) { + Dart_ExecuteInternalCommand("gc-on-nth-allocation", + reinterpret_cast(num_allocations)); +} + // Triggers GC. Has 11 dummy arguments as unboxed odd integers which should be // ignored by GC. DART_EXPORT void Regress37069(uint64_t a, diff --git a/runtime/lib/ffi_dynamic_library_patch.dart b/runtime/lib/ffi_dynamic_library_patch.dart index 32cabc6a7d3..c92dc1bd57a 100644 --- a/runtime/lib/ffi_dynamic_library_patch.dart +++ b/runtime/lib/ffi_dynamic_library_patch.dart @@ -29,7 +29,7 @@ class DynamicLibrary { Pointer lookup(String symbolName) native "Ffi_dl_lookup"; - // The real implementation of this function lives in FfiUseSitesTransformer + // The real implementation of this function lives in FfiUseSiteTransformer // for interface calls. Only dynamic calls (which are illegal) reach this // implementation. @patch diff --git a/runtime/vm/heap/heap.cc b/runtime/vm/heap/heap.cc index 04b5b849dc0..59a7e0b6d17 100644 --- a/runtime/vm/heap/heap.cc +++ b/runtime/vm/heap/heap.cc @@ -44,7 +44,7 @@ Heap::Heap(Isolate* isolate, read_only_(false), gc_new_space_in_progress_(false), gc_old_space_in_progress_(false), - gc_on_next_allocation_(false) { + gc_on_nth_allocation_(kNoForcedGarbageCollection) { UpdateGlobalMaxUsed(); for (int sel = 0; sel < kNumWeakSelectors; sel++) { new_weak_tables_[sel] = new WeakTable(); @@ -678,15 +678,22 @@ void Heap::AddRegionsToObjectSet(ObjectSet* set) const { old_space_.AddRegionsToObjectSet(set); } -void Heap::CollectOnNextAllocation() { +void Heap::CollectOnNthAllocation(intptr_t num_allocations) { + // Prevent generated code from using the TLAB fast path on next allocation. AbandonRemainingTLAB(Thread::Current()); - gc_on_next_allocation_ = true; + gc_on_nth_allocation_ = num_allocations; } void Heap::CollectForDebugging() { - if (!gc_on_next_allocation_) return; - CollectAllGarbage(kDebugging); - gc_on_next_allocation_ = false; + if (gc_on_nth_allocation_ == kNoForcedGarbageCollection) return; + gc_on_nth_allocation_--; + if (gc_on_nth_allocation_ == 0) { + CollectAllGarbage(kDebugging); + gc_on_nth_allocation_ = kNoForcedGarbageCollection; + } else { + // Prevent generated code from using the TLAB fast path on next allocation. + AbandonRemainingTLAB(Thread::Current()); + } } ObjectSet* Heap::CreateAllocatedObjectSet( diff --git a/runtime/vm/heap/heap.h b/runtime/vm/heap/heap.h index edfc176e027..74144679b59 100644 --- a/runtime/vm/heap/heap.h +++ b/runtime/vm/heap/heap.h @@ -311,7 +311,7 @@ class Heap { void AbandonRemainingTLAB(Thread* thread); Space SpaceForExternal(intptr_t size) const; - void CollectOnNextAllocation(); + void CollectOnNthAllocation(intptr_t num_allocations); private: class GCStats : public ValueObject { @@ -384,7 +384,7 @@ class Heap { void AddRegionsToObjectSet(ObjectSet* set) const; - // Trigger major GC if 'gc_on_next_allocation_' is set. + // Trigger major GC if 'gc_on_nth_allocation_' is set. void CollectForDebugging(); Isolate* isolate_; @@ -410,10 +410,12 @@ class Heap { bool gc_new_space_in_progress_; bool gc_old_space_in_progress_; + static const intptr_t kNoForcedGarbageCollection = -1; + // 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_; + intptr_t gc_on_nth_allocation_; friend class Become; // VisitObjectPointers friend class GCCompactor; // VisitObjectPointers diff --git a/runtime/vm/native_api_impl.cc b/runtime/vm/native_api_impl.cc index d87bb5a8abd..2739b4369ad 100644 --- a/runtime/vm/native_api_impl.cc +++ b/runtime/vm/native_api_impl.cc @@ -251,12 +251,15 @@ struct RunInSafepointAndRWCodeArgs { DART_EXPORT void* Dart_ExecuteInternalCommand(const char* command, void* arg) { if (!FLAG_enable_testing_pragmas) return nullptr; - if (!strcmp(command, "gc-on-next-allocation")) { + if (!strcmp(command, "gc-on-nth-allocation")) { TransitionNativeToVM _(Thread::Current()); - Isolate::Current()->heap()->CollectOnNextAllocation(); + intptr_t argument = reinterpret_cast(arg); + ASSERT(argument > 0); + Isolate::Current()->heap()->CollectOnNthAllocation(argument); return nullptr; } else if (!strcmp(command, "gc-now")) { + ASSERT(arg == nullptr); // Don't pass an argument to this command. TransitionNativeToVM _(Thread::Current()); Isolate::Current()->heap()->CollectAllGarbage(); return nullptr; diff --git a/tests/ffi/analysis_options.yaml b/tests/ffi/analysis_options.yaml new file mode 100644 index 00000000000..f8701d72382 --- /dev/null +++ b/tests/ffi/analysis_options.yaml @@ -0,0 +1,3 @@ +analyzer: + exclude: + # Do analyze this subfolder in the tests/ even if tests/ is fully excluded. diff --git a/tests/ffi/ffi.status b/tests/ffi/ffi.status index 76f5ff2ecbb..352517c8929 100644 --- a/tests/ffi/ffi.status +++ b/tests/ffi/ffi.status @@ -20,6 +20,7 @@ function_callbacks_test/03: Skip [ $runtime == dart_precompiled ] function_callbacks_test: Skip # Issue dartbug.com/37295 +regress_37511_callbacks_test: Skip # Issue dartbug.com/37295 [ $arch == arm && $system != android ] *: Skip # "hardfp" calling convention is not yet supported (iOS is also supported but not tested): dartbug.com/36309 diff --git a/tests/ffi/ffi_test_helpers.dart b/tests/ffi/ffi_test_helpers.dart index 5d14d4ad8c9..84d318568fb 100644 --- a/tests/ffi/ffi_test_helpers.dart +++ b/tests/ffi/ffi_test_helpers.dart @@ -5,12 +5,20 @@ // Helpers for tests which trigger GC in delicate places. import 'dart:ffi' as ffi; + import 'dylib_utils.dart'; typedef NativeNullaryOp = ffi.Void Function(); typedef NullaryOpVoid = void Function(); +typedef NativeUnaryOp = ffi.Void Function(ffi.IntPtr); +typedef UnaryOpVoid = void Function(int); + final ffi.DynamicLibrary ffiTestFunctions = dlopenPlatformSpecific("ffi_test_functions"); + final triggerGc = ffiTestFunctions .lookupFunction("TriggerGC"); + +final collectOnNthAllocation = ffiTestFunctions + .lookupFunction("CollectOnNthAllocation"); diff --git a/tests/ffi/gc_helpers.dart b/tests/ffi/gc_helpers.dart deleted file mode 100644 index 5d14d4ad8c9..00000000000 --- a/tests/ffi/gc_helpers.dart +++ /dev/null @@ -1,16 +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. -// -// Helpers for tests which trigger GC in delicate places. - -import 'dart:ffi' as ffi; -import 'dylib_utils.dart'; - -typedef NativeNullaryOp = ffi.Void Function(); -typedef NullaryOpVoid = void Function(); - -final ffi.DynamicLibrary ffiTestFunctions = - dlopenPlatformSpecific("ffi_test_functions"); -final triggerGc = ffiTestFunctions - .lookupFunction("TriggerGC"); diff --git a/tests/ffi/regress_37511_callbacks_test.dart b/tests/ffi/regress_37511_callbacks_test.dart new file mode 100644 index 00000000000..37407eed4e6 --- /dev/null +++ b/tests/ffi/regress_37511_callbacks_test.dart @@ -0,0 +1,54 @@ +// 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. +// +// Dart test program for testing dart:ffi struct pointers. +// +// VMOptions=--deterministic --enable-testing-pragmas +// +// SharedObjects=ffi_test_functions +// +// TODO(37295): Merge this file with regress_37511_test.dart when callback +// support lands. + +library FfiTest; + +import 'dart:ffi'; + +import 'ffi_test_helpers.dart'; + +/// Estimate of how many allocations functions in `functionsToTest` do at most. +const gcAfterNAllocationsMax = 10; + +void main() { + for (Function() f in functionsToTest) { + f(); // Ensure code is compiled. + + for (int n = 1; n <= gcAfterNAllocationsMax; n++) { + collectOnNthAllocation(n); + f(); + } + } +} + +final List functionsToTest = [ + // Callback trampolines. + doFromFunction, + () => callbackSmallDouble(dartFunctionPointer), +]; + +// Callback trampoline helpers. +typedef NativeCallbackTest = Int32 Function(Pointer); +typedef NativeCallbackTestFn = int Function(Pointer); + +final callbackSmallDouble = + ffiTestFunctions.lookupFunction( + "TestSimpleMultiply"); + +typedef SimpleMultiplyType = Double Function(Double); +double simpleMultiply(double x) => x * 1.337; + +final doFromFunction = + () => Pointer.fromFunction(simpleMultiply, 0.0); + +final dartFunctionPointer = doFromFunction(); diff --git a/tests/ffi/regress_37511_test.dart b/tests/ffi/regress_37511_test.dart new file mode 100644 index 00000000000..03403e4390c --- /dev/null +++ b/tests/ffi/regress_37511_test.dart @@ -0,0 +1,108 @@ +// 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. +// +// Dart test program for testing dart:ffi struct pointers. +// +// VMOptions=--deterministic --enable-testing-pragmas +// +// SharedObjects=ffi_test_functions + +library FfiTest; + +import 'dart:ffi'; + +import 'dylib_utils.dart'; +import 'ffi_test_helpers.dart'; + +/// Estimate of how many allocations functions in `functionsToTest` do at most. +const gcAfterNAllocationsMax = 10; + +void main() { + for (Function() f in functionsToTest) { + f(); // Ensure code is compiled. + + for (int n = 1; n <= gcAfterNAllocationsMax; n++) { + collectOnNthAllocation(n); + f(); + } + } +} + +final List functionsToTest = [ + // Pointer operations. + () => highAddressPointer.cast(), + () => Pointer.fromAddress(highAddressPointer.address), + () => highAddressPointer.address, + () => highAddressPointer.elementAt(1), + () => highAddressPointer.offsetBy(1), + () => highAddressPointer.asExternalTypedData(), + + // DynamicLibrary operations. + doDlopen, + doDlsym, // Includes `asFunction`. + () => ffiTestFunctions.handle, + + // Trampolines. + () => sumManyIntsOdd(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, mint64bit), + () => sumManyDoubles(1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0), + minInt64, + minInt32, + smallDouble, + largePointer, + + // Callback trampolines. + // + // In regress_37511_callbacks_test.dart because callbacks are not supported + // in AOT yet. +]; + +// Pointer operation helpers. +const mint32bit = 0xFFFFFFF0; +const mint64bit = 0x7FFFFFFFFFFFFFF0; + +final int highAddress = sizeOf() == 4 ? mint32bit : mint64bit; + +final Pointer highAddressPointer = Pointer.fromAddress(highAddress); + +// Dynamic library operation helpers. +final doDlopen = () => dlopenPlatformSpecific("ffi_test_functions"); + +final doDlsym = () => ffiTestFunctions + .lookupFunction("TriggerGC"); + +// Trampoline helpers. +typedef NativeUndenaryOp = IntPtr Function(IntPtr, IntPtr, IntPtr, IntPtr, + IntPtr, IntPtr, IntPtr, IntPtr, IntPtr, IntPtr, IntPtr); +typedef UndenaryOp = int Function( + int, int, int, int, int, int, int, int, int, int, int); + +final UndenaryOp sumManyIntsOdd = ffiTestFunctions + .lookupFunction("SumManyIntsOdd"); + +typedef NativeDoubleDecenaryOp = Double Function(Double, Double, Double, Double, + Double, Double, Double, Double, Double, Double); +typedef DoubleDecenaryOp = double Function(double, double, double, double, + double, double, double, double, double, double); + +final DoubleDecenaryOp sumManyDoubles = ffiTestFunctions + .lookupFunction("SumManyDoubles"); + +typedef NativeNullaryOp64 = Int64 Function(); +typedef NativeNullaryOp32 = Int32 Function(); +typedef NativeNullaryOpDouble = Double Function(); +typedef NullaryOpPtr = Pointer Function(); +typedef NullaryOp = int Function(); +typedef NullaryOpDbl = double Function(); + +final minInt64 = + ffiTestFunctions.lookupFunction("MinInt64"); + +final minInt32 = + ffiTestFunctions.lookupFunction("MinInt32"); + +final smallDouble = ffiTestFunctions + .lookupFunction("SmallDouble"); + +final largePointer = + ffiTestFunctions.lookupFunction("LargePointer");