From e3e355e16a8b67ba6a1d7a83f837e8e69cf71534 Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Mon, 12 Dec 2022 15:46:33 +0000 Subject: [PATCH] [vm/ffi] Fix FfiTrampolineData GC bug This lets the GC visit FfiTrampolineData::c_signature again. https://dart-review.googlesource.com/c/sdk/+/272201 stopped adding FfiTrampolineData::c_signature to snapshots. However, instead of skipping it manually in app_shapshot.cc, we skipped it in raw_object.h, which also caused the GC to skip it. This CL adds it back in as we need it in JIT snapshots. This way we keep it consistent between AOT/JIT snapshots. TEST=tests/ffi/regress_b_261224444_test.dart The c signatures of FFI trampolines were not properly traced in the precompiler, causing us to hit an assert when the classes mentioned in those types where only referenced from a signature and not retained for any other reason. TEST=tests/ffi/native_assets/process_test.dart (dartkp) Closes: https://github.com/dart-lang/sdk/issues/50678 Bug: b/261224444 Change-Id: I84fc880744c2045ea3e2ef4f37df454b80b2faeb Cq-Include-Trybots: luci.dart.try:vm-precomp-ffi-qemu-linux-release-arm-try,vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64c-try,vm-kernel-reload-rollback-linux-debug-x64-try,vm-kernel-reload-linux-debug-x64-try,app-kernel-linux-debug-x64-try,vm-kernel-precomp-linux-debug-x64c-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-nnbd-linux-debug-x64-try,vm-kernel-linux-debug-x64-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/274387 Reviewed-by: Martin Kustermann Auto-Submit: Daco Harkes Commit-Queue: Daco Harkes --- runtime/lib/object.cc | 3 ++- runtime/vm/compiler/aot/precompiler.cc | 5 +++++ runtime/vm/raw_object.h | 6 ++---- tests/ffi/regress_b_261224444_test.dart | 19 +++++++++++++++++++ tests/ffi_2/regress_b_261224444_test.dart | 21 +++++++++++++++++++++ 5 files changed, 49 insertions(+), 5 deletions(-) create mode 100644 tests/ffi/regress_b_261224444_test.dart create mode 100644 tests/ffi_2/regress_b_261224444_test.dart diff --git a/runtime/lib/object.cc b/runtime/lib/object.cc index c16ddb20c82..768d8af4078 100644 --- a/runtime/lib/object.cc +++ b/runtime/lib/object.cc @@ -330,7 +330,8 @@ DEFINE_NATIVE_ENTRY(Internal_nativeEffect, 0, 1) { } DEFINE_NATIVE_ENTRY(Internal_collectAllGarbage, 0, 0) { - isolate->group()->heap()->CollectAllGarbage(GCReason::kDebugging); + isolate->group()->heap()->CollectAllGarbage(GCReason::kDebugging, + /*compact=*/true); return Object::null(); } diff --git a/runtime/vm/compiler/aot/precompiler.cc b/runtime/vm/compiler/aot/precompiler.cc index 10f4dc30aa7..c6a80e04465 100644 --- a/runtime/vm/compiler/aot/precompiler.cc +++ b/runtime/vm/compiler/aot/precompiler.cc @@ -1043,6 +1043,7 @@ void Precompiler::AddCalleesOfHelper(const Object& entry, if (!callback_target.IsNull()) { AddFunction(callback_target, RetainReasons::kFfiCallbackTarget); } + AddTypesOf(target); } break; } @@ -1109,6 +1110,10 @@ void Precompiler::AddTypesOf(const Function& function) { const Class& owner = Class::Handle(Z, function.Owner()); AddTypesOf(owner); + if (function.IsFfiTrampoline()) { + AddType(FunctionType::Handle(Z, function.FfiCSignature())); + } + const auto& parent_function = Function::Handle(Z, function.parent_function()); if (parent_function.IsNull()) { return; diff --git a/runtime/vm/raw_object.h b/runtime/vm/raw_object.h index 42e39694b7a..c4f3ef33256 100644 --- a/runtime/vm/raw_object.h +++ b/runtime/vm/raw_object.h @@ -1404,13 +1404,11 @@ class UntaggedFfiTrampolineData : public UntaggedObject { private: RAW_HEAP_OBJECT_IMPLEMENTATION(FfiTrampolineData); - // Not traced. We don't need this info after precompilation, and FFI - // trampolines are not supported in JIT snapshots. - COMPRESSED_POINTER_FIELD(FunctionTypePtr, c_signature) - COMPRESSED_POINTER_FIELD(TypePtr, signature_type) VISIT_FROM(signature_type) + COMPRESSED_POINTER_FIELD(FunctionTypePtr, c_signature) + // Target Dart method for callbacks, otherwise null. COMPRESSED_POINTER_FIELD(FunctionPtr, callback_target) diff --git a/tests/ffi/regress_b_261224444_test.dart b/tests/ffi/regress_b_261224444_test.dart new file mode 100644 index 00000000000..7f2c5deb380 --- /dev/null +++ b/tests/ffi/regress_b_261224444_test.dart @@ -0,0 +1,19 @@ +// Copyright (c) 2022, 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:ffi'; + +import 'ffi_test_helpers.dart'; + +main() { + // Ensure we have FfiTrampolineData in heap. + final foo = DynamicLibrary.process() + .lookup Function(IntPtr)>>("malloc") + .asFunction Function(int)>(); + print(foo); + + triggerGc(); + + print(foo(100).address); +} diff --git a/tests/ffi_2/regress_b_261224444_test.dart b/tests/ffi_2/regress_b_261224444_test.dart new file mode 100644 index 00000000000..430418e9f22 --- /dev/null +++ b/tests/ffi_2/regress_b_261224444_test.dart @@ -0,0 +1,21 @@ +// Copyright (c) 2022, 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=2.9 + +import 'dart:ffi'; + +import 'ffi_test_helpers.dart'; + +main() { + // Ensure we have FfiTrampolineData in heap. + final foo = DynamicLibrary.process() + .lookup Function(IntPtr)>>("malloc") + .asFunction Function(int)>(); + print(foo); + + triggerGc(); + + print(foo(100).address); +}