diff --git a/runtime/vm/ffi_callback_metadata.cc b/runtime/vm/ffi_callback_metadata.cc index 34ec4958571..be010604b20 100644 --- a/runtime/vm/ffi_callback_metadata.cc +++ b/runtime/vm/ffi_callback_metadata.cc @@ -59,10 +59,10 @@ void FfiCallbackMetadata::EnsureStubPageLocked() { original_metadata_page_ = VirtualMemory::AllocateAligned( MappingSize(), MappingAlignment(), /*is_executable=*/false, /*is_compressed=*/false, "FfiCallbackMetadata::TrampolinePage"); - Metadata* metadata = reinterpret_cast( + MetadataEntry* metadata_entry = reinterpret_cast( original_metadata_page_->start() + MetadataOffset()); for (intptr_t i = 0; i < NumCallbackTrampolinesPerPage(); ++i) { - AddToFreeListLocked(&metadata[i]); + AddToFreeListLocked(&metadata_entry[i]); } #endif // defined(DART_TARGET_OS_FUCHSIA) } @@ -176,10 +176,10 @@ void FfiCallbackMetadata::EnsureFreeListNotEmptyLocked() { // Add all the trampolines to the free list. const intptr_t trampolines_per_page = NumCallbackTrampolinesPerPage(); - Metadata* metadata = - reinterpret_cast(new_page->start() + MetadataOffset()); + MetadataEntry* metadata_entry = + reinterpret_cast(new_page->start() + MetadataOffset()); for (intptr_t i = 0; i < trampolines_per_page; ++i) { - AddToFreeListLocked(&metadata[i]); + AddToFreeListLocked(&metadata_entry[i]); } } @@ -189,63 +189,64 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::CreateMetadataEntry( TrampolineType trampoline_type, uword target_entry_point, uint64_t context, - Metadata** list_head) { + MetadataEntry** list_head) { MutexLocker locker(&lock_); EnsureFreeListNotEmptyLocked(); ASSERT(free_list_head_ != nullptr); - Metadata* entry = free_list_head_; + MetadataEntry* entry = free_list_head_; free_list_head_ = entry->free_list_next_; if (free_list_head_ == nullptr) { ASSERT(free_list_tail_ == entry); free_list_tail_ = nullptr; } - Metadata* next_entry = *list_head; + MetadataEntry* next_entry = *list_head; if (next_entry != nullptr) { ASSERT(next_entry->list_prev_ == nullptr); next_entry->list_prev_ = entry; } if (target_isolate != nullptr) { - *entry = Metadata(target_isolate, trampoline_type, target_entry_point, - context, nullptr, next_entry); + *entry = MetadataEntry(target_isolate, trampoline_type, target_entry_point, + context, nullptr, next_entry); } else { ASSERT(target_isolate_group != nullptr); - *entry = Metadata(target_isolate_group, trampoline_type, target_entry_point, - context, nullptr, next_entry); + *entry = MetadataEntry(target_isolate_group, trampoline_type, + target_entry_point, context, nullptr, next_entry); } *list_head = entry; - return TrampolineOfMetadata(entry); + return TrampolineOfMetadataEntry(entry); } -void FfiCallbackMetadata::AddToFreeListLocked(Metadata* entry) { +void FfiCallbackMetadata::AddToFreeListLocked(MetadataEntry* entry) { ASSERT(lock_.IsOwnedByCurrentThread()); if (free_list_tail_ == nullptr) { ASSERT(free_list_head_ == nullptr); free_list_head_ = free_list_tail_ = entry; } else { ASSERT(free_list_head_ != nullptr && free_list_tail_ != nullptr); - ASSERT(!free_list_tail_->IsLive()); + ASSERT(!free_list_tail_->metadata()->IsLive()); free_list_tail_->free_list_next_ = entry; free_list_tail_ = entry; } - entry->context_ = 0; - entry->target_isolate_ = nullptr; + entry->metadata()->context_ = 0; + entry->metadata()->target_isolate_ = nullptr; entry->free_list_next_ = nullptr; } -void FfiCallbackMetadata::DeleteCallbackLocked(Metadata* entry) { +void FfiCallbackMetadata::DeleteCallbackLocked(MetadataEntry* entry) { ASSERT(lock_.IsOwnedByCurrentThread()); - if (entry->trampoline_type_ != TrampolineType::kAsync && - entry->context_ != 0) { - ASSERT(entry->target_isolate_ != nullptr); - entry->api_state()->FreePersistentHandle(entry->closure_handle()); + if (entry->metadata()->trampoline_type_ != TrampolineType::kAsync && + entry->metadata()->context_ != 0) { + ASSERT(entry->metadata()->target_isolate_ != nullptr); + entry->metadata()->api_state()->FreePersistentHandle( + entry->metadata()->closure_handle()); } AddToFreeListLocked(entry); } -void FfiCallbackMetadata::DeleteAllCallbacks(Metadata** list_head) { +void FfiCallbackMetadata::DeleteAllCallbacks(MetadataEntry** list_head) { MutexLocker locker(&lock_); - for (Metadata* entry = *list_head; entry != nullptr;) { - Metadata* next = entry->list_next(); + for (MetadataEntry* entry = *list_head; entry != nullptr;) { + MetadataEntry* next = entry->list_next(); DeleteCallbackLocked(entry); entry = next; } @@ -253,10 +254,10 @@ void FfiCallbackMetadata::DeleteAllCallbacks(Metadata** list_head) { } void FfiCallbackMetadata::DeleteCallback(Trampoline trampoline, - Metadata** list_head) { + MetadataEntry** list_head) { MutexLocker locker(&lock_); - auto* entry = MetadataOfTrampoline(trampoline); - ASSERT(entry->IsLive()); + auto* entry = MetadataEntryOfTrampoline(trampoline); + ASSERT(entry->metadata()->IsLive()); auto* prev = entry->list_prev_; auto* next = entry->list_next_; if (prev != nullptr) { @@ -295,7 +296,7 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::CreateLocalFfiCallback( Zone* zone, const Function& function, const Closure& closure, - Metadata** list_head) { + MetadataEntry** list_head) { PersistentHandle* handle = nullptr; if (closure.IsNull()) { // If the closure is null, it means the target is a static function, so is @@ -326,7 +327,7 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::CreateSyncFfiCallbackImpl( Zone* zone, const Function& function, PersistentHandle* closure, - Metadata** list_head) { + MetadataEntry** list_head) { TrampolineType trampoline_type = isolate != nullptr ? TrampolineType::kSync : TrampolineType::kSyncIsolateGroupShared; @@ -355,7 +356,7 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::CreateAsyncFfiCallback( Zone* zone, const Function& send_function, Dart_Port send_port, - Metadata** list_head) { + MetadataEntry** list_head) { ASSERT(send_function.GetFfiCallbackKind() == FfiCallbackKind::kAsyncCallback); return CreateMetadataEntry(isolate, /*isolate_group=*/nullptr, TrampolineType::kAsync, @@ -363,11 +364,12 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::CreateAsyncFfiCallback( static_cast(send_port), list_head); } -FfiCallbackMetadata::Trampoline FfiCallbackMetadata::TrampolineOfMetadata( - Metadata* metadata) const { - const uword start = MappingStart(reinterpret_cast(metadata)); - Metadata* metadatas = reinterpret_cast(start + MetadataOffset()); - const uword index = metadata - metadatas; +FfiCallbackMetadata::Trampoline FfiCallbackMetadata::TrampolineOfMetadataEntry( + MetadataEntry* metadata_entry) const { + const uword start = MappingStart(reinterpret_cast(metadata_entry)); + MetadataEntry* metadata_entries = + reinterpret_cast(start + MetadataOffset()); + const uword index = metadata_entry - metadata_entries; #if defined(SIMULATOR_FFI) && defined(HOST_ARCH_ARM64) return reinterpret_cast(SimulatorFfiCallbackTrampoline) + index * kNativeCallbackTrampolineSize; @@ -380,8 +382,8 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::TrampolineOfMetadata( #endif } -FfiCallbackMetadata::Metadata* FfiCallbackMetadata::MetadataOfTrampoline( - Trampoline trampoline) const { +FfiCallbackMetadata::MetadataEntry* +FfiCallbackMetadata::MetadataEntryOfTrampoline(Trampoline trampoline) const { #if defined(DART_TARGET_OS_FUCHSIA) || \ (defined(SIMULATOR_FFI) && defined(HOST_ARCH_ARM64)) // On Fuchsia the metadata page is separate to the trampoline page. @@ -393,22 +395,23 @@ FfiCallbackMetadata::Metadata* FfiCallbackMetadata::MetadataOfTrampoline( (trampoline - offset_of_first_trampoline_in_page_ - page_start) / kNativeCallbackTrampolineSize; ASSERT(index < NumCallbackTrampolinesPerPage()); - Metadata* metadata_table = reinterpret_cast( + MetadataEntry* metadata_etnry_table = reinterpret_cast( original_metadata_page_->start() + MetadataOffset()); - return metadata_table + index; + return metadata_etnry_table + index; #else const uword start = MappingStart(trampoline); - Metadata* metadatas = reinterpret_cast(start + MetadataOffset()); + MetadataEntry* metadata_entries = + reinterpret_cast(start + MetadataOffset()); const uword index = (trampoline - start - offset_of_first_trampoline_in_page_) / kNativeCallbackTrampolineSize; - return &metadatas[index]; + return &metadata_entries[index]; #endif } FfiCallbackMetadata::Metadata FfiCallbackMetadata::LookupMetadataForTrampoline( Trampoline trampoline) const { - return *MetadataOfTrampoline(trampoline); + return *MetadataEntryOfTrampoline(trampoline)->metadata(); } FfiCallbackMetadata* FfiCallbackMetadata::singleton_ = nullptr; diff --git a/runtime/vm/ffi_callback_metadata.h b/runtime/vm/ffi_callback_metadata.h index 8c7cd1eb1c9..4c57f2856c3 100644 --- a/runtime/vm/ffi_callback_metadata.h +++ b/runtime/vm/ffi_callback_metadata.h @@ -37,6 +37,7 @@ class PersistentHandle; class FfiCallbackMetadata { public: class Metadata; + class MetadataEntry; // The address of the allocated trampoline. using Trampoline = uword; @@ -68,7 +69,7 @@ class FfiCallbackMetadata { Zone* zone, const Function& function, Dart_Port send_port, - Metadata** list_head); + MetadataEntry** list_head); // Creates an isolate- or isolategroup- local callback trampoline for // the given function. @@ -77,13 +78,13 @@ class FfiCallbackMetadata { Zone* zone, const Function& function, const Closure& closure, - Metadata** list_head); + MetadataEntry** list_head); // Deletes a single trampoline. - void DeleteCallback(Trampoline trampoline, Metadata** list_head); + void DeleteCallback(Trampoline trampoline, MetadataEntry** list_head); // Deletes all the trampolines in the list. - void DeleteAllCallbacks(Metadata** list_head); + void DeleteAllCallbacks(MetadataEntry** list_head); // FFI callback metadata for any sync or async trampoline. class Metadata { @@ -93,51 +94,31 @@ class FfiCallbackMetadata { }; TrampolineType trampoline_type_; - union { - // IsLive() - struct { - // Note: This is a pointer into an an Instructions object. This is only - // safe because Instructions objects are never moved by the GC. - uword target_entry_point_; + // Note: This is a pointer into an an Instructions object. This is only + // safe because Instructions objects are never moved by the GC. + uword target_entry_point_; - // For async callbacks, this is the send port. For sync callbacks this - // is a persistent handle to the callback's closure, or null. - uint64_t context_; - - // Links in the Isolate's list of callbacks. - Metadata* list_prev_; - Metadata* list_next_; - }; - - // !IsLive() - Metadata* free_list_next_; - }; + // For async callbacks, this is the send port. For sync callbacks this + // is a persistent handle to the callback's closure, or null. + uint64_t context_; Metadata(Isolate* target_isolate, TrampolineType trampoline_type, uword target_entry_point, - uint64_t context, - Metadata* list_prev, - Metadata* list_next) + uint64_t context) : target_isolate_(target_isolate), trampoline_type_(trampoline_type), target_entry_point_(target_entry_point), - context_(context), - list_prev_(list_prev), - list_next_(list_next) {} + context_(context) {} Metadata(IsolateGroup* target_isolate_group, TrampolineType trampoline_type, uword target_entry_point, - uint64_t context, - Metadata* list_prev, - Metadata* list_next) + uint64_t context) : target_isolate_group_(target_isolate_group), trampoline_type_(trampoline_type), target_entry_point_(target_entry_point), - context_(context), - list_prev_(list_prev), - list_next_(list_next) {} + context_(context) {} public: friend class FfiCallbackMetadata; @@ -208,24 +189,72 @@ class FfiCallbackMetadata { return static_cast(context_); } - // To efficiently delete all the callbacks for a isolate, they are stored in - // a linked list. Since we also need to delete async callbacks at arbitrary - // times, the list must be doubly linked. - Metadata* list_prev() { - ASSERT(IsLive()); - return list_prev_; - } - Metadata* list_next() { - ASSERT(IsLive()); - return list_next_; - } - // Tells FfiCallbackTrampolineStub how to call into the entry point. Mostly // it's just a flag for whether this is a sync or async callback, but on // IA32 it also encodes whether there's a stack delta of 4 to deal with. TrampolineType trampoline_type() const { return trampoline_type_; } }; + // Metadata linked into a double-linked list. + class MetadataEntry { + Metadata metadata_; + + union { + // IsLive() + struct { + // Links in the Isolate's list of callbacks. + MetadataEntry* list_prev_; + MetadataEntry* list_next_; + }; + + // !IsLive() + MetadataEntry* free_list_next_; + }; + + public: + friend class Metadata; + friend class FfiCallbackMetadata; + MetadataEntry(Isolate* target_isolate, + TrampolineType trampoline_type, + uword target_entry_point, + uint64_t context, + MetadataEntry* list_prev, + MetadataEntry* list_next) + : metadata_(target_isolate, + trampoline_type, + target_entry_point, + context), + list_prev_(list_prev), + list_next_(list_next) {} + + MetadataEntry(IsolateGroup* target_isolate_group, + TrampolineType trampoline_type, + uword target_entry_point, + uint64_t context, + MetadataEntry* list_prev, + MetadataEntry* list_next) + : metadata_(target_isolate_group, + trampoline_type, + target_entry_point, + context), + list_prev_(list_prev), + list_next_(list_next) {} + + // To efficiently delete all the callbacks for a isolate, they are stored in + // a linked list. Since we also need to delete async callbacks at arbitrary + // times, the list must be doubly linked. + MetadataEntry* list_prev() { + ASSERT(metadata_.IsLive()); + return list_prev_; + } + MetadataEntry* list_next() { + ASSERT(metadata_.IsLive()); + return list_next_; + } + + Metadata* metadata() { return &metadata_; } + }; + // Returns the Metadata object for the given trampoline. Metadata LookupMetadataForTrampoline(Trampoline trampoline) const; @@ -264,12 +293,12 @@ class FfiCallbackMetadata { // * [RX] 2 pages fully containing [StubCode::FfiCallbackTrampoline()] // * [RW] pages sufficient to hold // - `kNumRuntimeFunctions` x [uword] function pointers - // - `NumCallbackTrampolinesPerPage()` x [Metadata] objects + // - `NumCallbackTrampolinesPerPage()` x [MetadataEntry] objects static constexpr intptr_t RXMappingSize() { return 2 * kPageSize; } static constexpr intptr_t RWMappingSize() { return Utils::RoundUp( kNumRuntimeFunctions * compiler::target::kWordSize + - sizeof(Metadata) * NumCallbackTrampolinesPerPage(), + sizeof(MetadataEntry) * NumCallbackTrampolinesPerPage(), kPageSize); } static constexpr intptr_t MappingSize() { @@ -318,15 +347,15 @@ class FfiCallbackMetadata { #endif // Visible for testing. - Metadata* MetadataOfTrampoline(Trampoline trampoline) const; - Trampoline TrampolineOfMetadata(Metadata* metadata) const; + MetadataEntry* MetadataEntryOfTrampoline(Trampoline trampoline) const; + Trampoline TrampolineOfMetadataEntry(MetadataEntry* metadata) const; private: FfiCallbackMetadata(); ~FfiCallbackMetadata(); void EnsureStubPageLocked(); - void AddToFreeListLocked(Metadata* entry); - void DeleteCallbackLocked(Metadata* entry); + void AddToFreeListLocked(MetadataEntry* entry); + void DeleteCallbackLocked(MetadataEntry* entry); void FillRuntimeFunction(VirtualMemory* page, uword index, void* function); VirtualMemory* AllocateTrampolinePage(); void EnsureFreeListNotEmptyLocked(); @@ -335,19 +364,13 @@ class FfiCallbackMetadata { TrampolineType trampoline_type, uword target_entry_point, uint64_t context, - Metadata** list_head); + MetadataEntry** list_head); Trampoline CreateSyncFfiCallbackImpl(Isolate* isolate, IsolateGroup* isolate_group, Zone* zone, const Function& function, PersistentHandle* closure, - Metadata** list_head); - Trampoline CreateIsolateGroupSharedFfiCallbackImpl( - IsolateGroup* isolate_group, - Zone* zone, - const Function& function, - PersistentHandle* closure, - Metadata** list_head); + MetadataEntry** list_head); Trampoline TryAllocateFromFreeListLocked(); static uword GetEntryPoint(Zone* zone, const Function& function); static PersistentHandle* CreatePersistentHandle(IsolateGroup* isolate_group, @@ -359,8 +382,8 @@ class FfiCallbackMetadata { VirtualMemory* stub_page_ = nullptr; MallocGrowableArray trampoline_pages_; uword offset_of_first_trampoline_in_page_ = 0; - Metadata* free_list_head_ = nullptr; - Metadata* free_list_tail_ = nullptr; + MetadataEntry* free_list_head_ = nullptr; + MetadataEntry* free_list_tail_ = nullptr; #if defined(DART_TARGET_OS_FUCHSIA) || \ (defined(SIMULATOR_FFI) && defined(HOST_ARCH_ARM64)) diff --git a/runtime/vm/ffi_callback_metadata_test.cc b/runtime/vm/ffi_callback_metadata_test.cc index 95dc590af42..515069cd829 100644 --- a/runtime/vm/ffi_callback_metadata_test.cc +++ b/runtime/vm/ffi_callback_metadata_test.cc @@ -111,7 +111,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateSyncFfiCallback) { static_cast(FfiCallbackMetadata::TrampolineType::kSync)); // head -> tramp1 - auto* e1 = fcm->MetadataOfTrampoline(tramp1); + auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1); EXPECT_EQ(isolate->ffi_callback_list_head(), e1); EXPECT_EQ(e1->list_prev(), nullptr); EXPECT_EQ(e1->list_next(), nullptr); @@ -135,8 +135,8 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateSyncFfiCallback) { { // head -> tramp2 -> tramp1 - auto* e1 = fcm->MetadataOfTrampoline(tramp1); - auto* e2 = fcm->MetadataOfTrampoline(tramp2); + auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1); + auto* e2 = fcm->MetadataEntryOfTrampoline(tramp2); EXPECT_EQ(isolate->ffi_callback_list_head(), e2); EXPECT_EQ(e2->list_prev(), nullptr); EXPECT_EQ(e2->list_next(), e1); @@ -151,7 +151,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateSyncFfiCallback) { EXPECT(!m1.IsLive()); // head -> tramp2 - auto* e2 = fcm->MetadataOfTrampoline(tramp2); + auto* e2 = fcm->MetadataEntryOfTrampoline(tramp2); EXPECT_EQ(isolate->ffi_callback_list_head(), e2); EXPECT_EQ(e2->list_prev(), nullptr); EXPECT_EQ(e2->list_next(), nullptr); @@ -206,7 +206,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateAsyncFfiCallback) { static_cast(FfiCallbackMetadata::TrampolineType::kAsync)); // head -> tramp1 - auto* e1 = fcm->MetadataOfTrampoline(tramp1); + auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1); EXPECT_EQ(isolate->ffi_callback_list_head(), e1); EXPECT_EQ(e1->list_prev(), nullptr); EXPECT_EQ(e1->list_next(), nullptr); @@ -230,8 +230,8 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateAsyncFfiCallback) { { // head -> tramp2 -> tramp1 - auto* e1 = fcm->MetadataOfTrampoline(tramp1); - auto* e2 = fcm->MetadataOfTrampoline(tramp2); + auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1); + auto* e2 = fcm->MetadataEntryOfTrampoline(tramp2); EXPECT_EQ(isolate->ffi_callback_list_head(), e2); EXPECT_EQ(e2->list_prev(), nullptr); EXPECT_EQ(e2->list_next(), e1); @@ -246,7 +246,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateAsyncFfiCallback) { EXPECT(!m2.IsLive()); // head -> tramp1 - auto* e1 = fcm->MetadataOfTrampoline(tramp1); + auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1); EXPECT_EQ(isolate->ffi_callback_list_head(), e1); EXPECT_EQ(e1->list_prev(), nullptr); EXPECT_EQ(e1->list_next(), nullptr); @@ -308,7 +308,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateIsolateLocalFfiCallback) { static_cast(FfiCallbackMetadata::TrampolineType::kSync)); // head -> tramp1 - auto* e1 = fcm->MetadataOfTrampoline(tramp1); + auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1); EXPECT_EQ(isolate->ffi_callback_list_head(), e1); EXPECT_EQ(e1->list_prev(), nullptr); EXPECT_EQ(e1->list_next(), nullptr); @@ -334,8 +334,8 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateIsolateLocalFfiCallback) { { // head -> tramp2 -> tramp1 - auto* e1 = fcm->MetadataOfTrampoline(tramp1); - auto* e2 = fcm->MetadataOfTrampoline(tramp2); + auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1); + auto* e2 = fcm->MetadataEntryOfTrampoline(tramp2); EXPECT_EQ(isolate->ffi_callback_list_head(), e2); EXPECT_EQ(e2->list_prev(), nullptr); EXPECT_EQ(e2->list_next(), e1); @@ -350,7 +350,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateIsolateLocalFfiCallback) { EXPECT(!m2.IsLive()); // head -> tramp1 - auto* e1 = fcm->MetadataOfTrampoline(tramp1); + auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1); EXPECT_EQ(isolate->ffi_callback_list_head(), e1); EXPECT_EQ(e1->list_prev(), nullptr); EXPECT_EQ(e1->list_next(), nullptr); @@ -378,7 +378,7 @@ ISOLATE_UNIT_TEST_CASE(FfiCallbackMetadata_TrampolineRecycling) { EXPECT(!code.IsNull()); auto port = PortMap::CreatePort(new FakeMessageHandler()); - FfiCallbackMetadata::Metadata* list_head = nullptr; + FfiCallbackMetadata::MetadataEntry* list_head = nullptr; // Allocate and free one callback at a time, and verify that we don't reuse // them. Allocate enough that the whole page fills up with dead trampolines. @@ -437,7 +437,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_DeleteTrampolines) { auto* fcm = FfiCallbackMetadata::Instance(); std::unordered_set tramps; - FfiCallbackMetadata::Metadata* list_head = nullptr; + FfiCallbackMetadata::MetadataEntry* list_head = nullptr; const auto& sync_func = Function::Handle( CreateTestFunction(FfiCallbackKind::kIsolateLocalStaticCallback)); @@ -470,23 +470,23 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_DeleteTrampolines) { // Verify the list of callbacks. uword list_length = 0; - for (FfiCallbackMetadata::Metadata* m = list_head; m != nullptr;) { + for (FfiCallbackMetadata::MetadataEntry* me = list_head; me != nullptr;) { ++list_length; - auto tramp = fcm->TrampolineOfMetadata(m); - EXPECT(m->IsLive()); - EXPECT_EQ(m->target_isolate(), isolate); + auto tramp = fcm->TrampolineOfMetadataEntry(me); + EXPECT(me->metadata()->IsLive()); + EXPECT_EQ(me->metadata()->target_isolate(), isolate); EXPECT_EQ(tramps.count(tramp), 1u); - auto* next = m->list_next(); - auto* prev = m->list_prev(); + auto* next = me->list_next(); + auto* prev = me->list_prev(); if (prev != nullptr) { - EXPECT_EQ(prev->list_next(), m); + EXPECT_EQ(prev->list_next(), me); } else { - EXPECT_EQ(list_head, m); + EXPECT_EQ(list_head, me); } if (next != nullptr) { - EXPECT_EQ(next->list_prev(), m); + EXPECT_EQ(next->list_prev(), me); } - m = m->list_next(); + me = me->list_next(); } EXPECT_EQ(list_length, tramps.size()); @@ -518,7 +518,7 @@ static void RunBigRandomMultithreadedTest(uint64_t seed) { Random random(seed); std::vector tramps; std::unordered_set tramp_set; - FfiCallbackMetadata::Metadata* list_head = nullptr; + FfiCallbackMetadata::MetadataEntry* list_head = nullptr; const Function& async_func = Function::Handle(CreateTestFunction(FfiCallbackKind::kAsyncCallback)); @@ -589,13 +589,13 @@ static void RunBigRandomMultithreadedTest(uint64_t seed) { // Verify the isolate's list of callbacks. uword list_length = 0; - for (FfiCallbackMetadata::Metadata* m = list_head; m != nullptr;) { + for (FfiCallbackMetadata::MetadataEntry* me = list_head; me != nullptr;) { ++list_length; - auto tramp = fcm->TrampolineOfMetadata(m); - EXPECT(m->IsLive()); - EXPECT_EQ(m->target_isolate(), isolate); + auto tramp = fcm->TrampolineOfMetadataEntry(me); + EXPECT(me->metadata()->IsLive()); + EXPECT_EQ(me->metadata()->target_isolate(), isolate); EXPECT_EQ(tramp_set.count(tramp), 1u); - m = m->list_next(); + me = me->list_next(); } EXPECT_EQ(list_length, tramps.size()); EXPECT_EQ(list_length, tramp_set.size()); diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h index 6e1b25a4948..797806d0604 100644 --- a/runtime/vm/isolate.h +++ b/runtime/vm/isolate.h @@ -1299,7 +1299,7 @@ class Isolate : public IntrusiveDListEntry { void CloseReceivePort(const ReceivePort& receive_port); // Visible for testing. - FfiCallbackMetadata::Metadata* ffi_callback_list_head() { + FfiCallbackMetadata::MetadataEntry* ffi_callback_list_head() { return ffi_callback_list_head_; } @@ -1673,7 +1673,7 @@ class Isolate : public IntrusiveDListEntry { Mutex mutex_; // Protects compiler stats. IsolateMessageHandler* message_handler_ = nullptr; intptr_t defer_finalization_count_ = 0; - FfiCallbackMetadata::Metadata* ffi_callback_list_head_ = nullptr; + FfiCallbackMetadata::MetadataEntry* ffi_callback_list_head_ = nullptr; intptr_t ffi_callback_keep_alive_counter_ = 0; RelaxedAtomic owner_thread_ = OSThread::kInvalidThreadId; diff --git a/samples/ffi/httpIG/lib/http.dart b/samples/ffi/httpIG/lib/http.dart index 492451d0628..59a3d404b8a 100644 --- a/samples/ffi/httpIG/lib/http.dart +++ b/samples/ffi/httpIG/lib/http.dart @@ -49,6 +49,7 @@ Future httpGet(String uri) async { final response = await completer.future; rp.close(); + callback.close(); return response; } @@ -59,20 +60,7 @@ late int counter; // Start a HTTP server on a background thread. ReceivePort httpServe(void Function(String) onRequest) { counter = 0; - final rp = ReceivePort() - ..listen( - (s) { - print('httpServe counter: $counter'); - onRequest(s); - }, - onError: (e, st) { - print('httpServe receiver get error $e $st'); - }, - onDone: () { - nativeHttpStopServing(); - }, - ); - + final rp = ReceivePort(); final callback = NativeCallable.isolateGroupShared(( Pointer requestPointer, ) { @@ -83,6 +71,19 @@ ReceivePort httpServe(void Function(String) onRequest) { final s = utf8.decode(typedList); rp.sendPort.send(s); }); + rp.listen( + (s) { + print('httpServe counter: $counter'); + onRequest(s); + }, + onError: (e, st) { + print('httpServe receiver get error $e $st'); + }, + onDone: () { + nativeHttpStopServing(); + callback.close(); + }, + ); // Invoke the native function to start the HTTP server. Our example // HTTP library will start a server on a background thread, and pass diff --git a/sdk/lib/_internal/vm/lib/ffi_patch.dart b/sdk/lib/_internal/vm/lib/ffi_patch.dart index c7cdc2b8710..a15117c9af6 100644 --- a/sdk/lib/_internal/vm/lib/ffi_patch.dart +++ b/sdk/lib/_internal/vm/lib/ffi_patch.dart @@ -349,9 +349,11 @@ final class _NativeCallableListener final class _NativeCallableIsolateGroupShared extends _NativeCallableBase { - bool _isKeepingIsolateAlive = false; + bool _isKeepingIsolateAlive = true; - _NativeCallableIsolateGroupShared(super._pointer); + _NativeCallableIsolateGroupShared(super._pointer) { + _updateNativeCallableKeepIsolateAliveCounter(1); + } @override void _close() { @@ -367,7 +369,7 @@ final class _NativeCallableIsolateGroupShared } @override - bool get _keepIsolateAlive => false; + bool get _keepIsolateAlive => _isKeepingIsolateAlive; } @patch diff --git a/tests/ffi/isolate_group_shared_callback_test.dart b/tests/ffi/isolate_group_shared_callback_test.dart index 07d0842c919..fadd627b208 100644 --- a/tests/ffi/isolate_group_shared_callback_test.dart +++ b/tests/ffi/isolate_group_shared_callback_test.dart @@ -51,6 +51,10 @@ class NativeLibrary { .lookupFunction( "CallFunctionOnNewThreadNonBlocking", ); + callFunctionOnNewThreadBlocking = ffiTestFunctions + .lookupFunction( + "CallFunctionOnNewThreadBlocking", + ); callTwoIntFunction = ffiTestFunctions .lookupFunction("CallTwoIntFunction"); sleep = ffiTestFunctions.lookupFunction( @@ -112,6 +116,7 @@ Future testNativeCallableHelloWorld() async { } }); Expect.equals(42 + (1001 * 123) * 2, result); + callback.close(); } void simpleFunctionThatThrows(int a, int b) { @@ -128,12 +133,19 @@ Future testNativeCallableThrows() async { result = 42; resultIsReady = false; - lib.callFunctionOnNewThreadNonBlocking(1001, callback.nativeFunction); + // The call is blocking so that tsan does not complain about read/write + // race between invoking the callback and closing it few lines down below. + // So the main thing this test checks is condition variable timeout, + // which is still valuable. + lib.callFunctionOnNewThreadBlocking(1001, callback.nativeFunction); mutexCondvar.runLocked(() { - conditionVariable.wait(mutexCondvar, 10 * sleepForMs); + // Just have short one second sleep - the condition variable is not + // going to be triggered. + conditionVariable.wait(mutexCondvar, 1 * sleepForMs); Expect.isFalse(resultIsReady); }); + callback.close(); } Future testNativeCallableHelloWorldClosure() async { @@ -171,6 +183,7 @@ Future testNativeCallableHelloWorldClosure() async { } }); Expect.equals(42 + (1001 * 123) * 2, result); + callback.close(); } void testNativeCallableSync() { @@ -225,6 +238,51 @@ void testNativeCallableAccessNonSharedVar() { callback.close(); } +Future testKeepIsolateAliveTrue() async { + ReceivePort rpOnExit = ReceivePort("onExit"); + Isolate.spawn( + (_) async { + final callback = NativeCallable.isolateGroupShared( + simpleFunction, + ); + callback.keepIsolateAlive = true; + }, + /*message=*/ null, + onExit: rpOnExit.sendPort, + ); + try { + await rpOnExit.first.timeout(Duration(seconds: 5)); + // should not fall through, should throw TimeoutException + Expect.isTrue(false); + } catch (e) { + print('caught $e'); + Expect.isTrue(e is TimeoutException); + } + rpOnExit.close(); +} + +Future testKeepIsolateAliveFalse() async { + ReceivePort rpOnExit = ReceivePort("onExit"); + Isolate.spawn( + (_) async { + final callback = NativeCallable.isolateGroupShared( + simpleFunction, + ); + callback.keepIsolateAlive = false; + }, + /*message=*/ null, + onExit: rpOnExit.sendPort, + ); + try { + await rpOnExit.first.timeout(Duration(seconds: 30)); + } catch (e) { + // should not throw timeout exception + print('caught $e'); + Expect.isTrue(false); + } + rpOnExit.close(); +} + main(args, message) async { lib = NativeLibrary(); // Simple tests. @@ -234,5 +292,7 @@ main(args, message) async { testNativeCallableSync(); testNativeCallableSyncThrows(); testNativeCallableAccessNonSharedVar(); + await testKeepIsolateAliveTrue(); + await testKeepIsolateAliveFalse(); print("All tests completed :)"); }