From 1fb3c9f08ccd54ad28765f46ca04a80062c579d0 Mon Sep 17 00:00:00 2001 From: Hamish Milne Date: Sat, 18 Apr 2020 15:10:54 +0100 Subject: [PATCH 1/4] Fix Address Arbiter serialization --- src/core/hle/kernel/address_arbiter.cpp | 36 ++++++++++++++++++++----- src/core/hle/kernel/address_arbiter.h | 28 +++++++++++++++---- 2 files changed, 53 insertions(+), 11 deletions(-) diff --git a/src/core/hle/kernel/address_arbiter.cpp b/src/core/hle/kernel/address_arbiter.cpp index 5074e352f..97228f153 100644 --- a/src/core/hle/kernel/address_arbiter.cpp +++ b/src/core/hle/kernel/address_arbiter.cpp @@ -16,8 +16,6 @@ //////////////////////////////////////////////////////////////////////////////////////////////////// // Kernel namespace -SERIALIZE_EXPORT_IMPL(Kernel::AddressArbiter) - namespace Kernel { void AddressArbiter::WaitThread(std::shared_ptr thread, VAddr wait_address) { @@ -80,19 +78,36 @@ std::shared_ptr KernelSystem::CreateAddressArbiter(std::string n return address_arbiter; } +class AddressArbiter::Callback : public WakeupCallback { +public: + Callback(AddressArbiter& _parent) : parent(SharedFrom(&_parent)) {} + std::shared_ptr parent; + + void WakeUp(ThreadWakeupReason reason, std::shared_ptr thread, + std::shared_ptr object) override { + parent->WakeUp(reason, thread, object); + } + +private: + Callback() = default; + template + void serialize(Archive& ar, const unsigned int) { + ar& boost::serialization::base_object(*this); + ar& parent; + } + friend class boost::serialization::access; +}; + void AddressArbiter::WakeUp(ThreadWakeupReason reason, std::shared_ptr thread, std::shared_ptr object) { ASSERT(reason == ThreadWakeupReason::Timeout); // Remove the newly-awakened thread from the Arbiter's waiting list. waiting_threads.erase(std::remove(waiting_threads.begin(), waiting_threads.end(), thread), - waiting_threads.end()); + waiting_threads.end()); }; ResultCode AddressArbiter::ArbitrateAddress(std::shared_ptr thread, ArbitrationType type, VAddr address, s32 value, u64 nanoseconds) { - - auto timeout_callback = std::dynamic_pointer_cast(shared_from_this()); - switch (type) { // Signal thread(s) waiting for arbitrate address... @@ -114,6 +129,9 @@ ResultCode AddressArbiter::ArbitrateAddress(std::shared_ptr thread, Arbi } break; case ArbitrationType::WaitIfLessThanWithTimeout: + if (!timeout_callback) { + timeout_callback = std::make_shared(*this); + } if ((s32)kernel.memory.Read32(address) < value) { thread->wakeup_callback = timeout_callback; thread->WakeAfterDelay(nanoseconds); @@ -130,6 +148,9 @@ ResultCode AddressArbiter::ArbitrateAddress(std::shared_ptr thread, Arbi break; } case ArbitrationType::DecrementAndWaitIfLessThanWithTimeout: { + if (!timeout_callback) { + timeout_callback = std::make_shared(*this); + } s32 memory_value = kernel.memory.Read32(address); if (memory_value < value) { // Only change the memory value if the thread should wait @@ -157,3 +178,6 @@ ResultCode AddressArbiter::ArbitrateAddress(std::shared_ptr thread, Arbi } } // namespace Kernel + +SERIALIZE_EXPORT_IMPL(Kernel::AddressArbiter) +SERIALIZE_EXPORT_IMPL(Kernel::AddressArbiter::Callback) diff --git a/src/core/hle/kernel/address_arbiter.h b/src/core/hle/kernel/address_arbiter.h index c7a263d9e..de7631066 100644 --- a/src/core/hle/kernel/address_arbiter.h +++ b/src/core/hle/kernel/address_arbiter.h @@ -59,8 +59,7 @@ public: ResultCode ArbitrateAddress(std::shared_ptr thread, ArbitrationType type, VAddr address, s32 value, u64 nanoseconds); - void WakeUp(ThreadWakeupReason reason, std::shared_ptr thread, - std::shared_ptr object); + class Callback; private: KernelSystem& kernel; @@ -78,20 +77,39 @@ private: /// Threads waiting for the address arbiter to be signaled. std::vector> waiting_threads; + std::shared_ptr timeout_callback; + + void WakeUp(ThreadWakeupReason reason, std::shared_ptr thread, + std::shared_ptr object); + + class DummyCallback : public WakeupCallback { + public: + void WakeUp(ThreadWakeupReason reason, std::shared_ptr thread, + std::shared_ptr object) override {} + }; + friend class boost::serialization::access; template void serialize(Archive& ar, const unsigned int file_version) { ar& boost::serialization::base_object(*this); - if (file_version > 0) { - ar& boost::serialization::base_object(*this); + if (file_version == 1) { + // This rigmarole is needed because in past versions, AddressArbiter inherited WakeupCallback + // But it turns out this breaks shared_from_this, so we split it out. + // Using a dummy class to deserialize a base_object allows compatibility to be maintained. + DummyCallback x; + ar& boost::serialization::base_object(x); } ar& name; ar& waiting_threads; + if (file_version > 1) { + ar& timeout_callback; + } } }; } // namespace Kernel BOOST_CLASS_EXPORT_KEY(Kernel::AddressArbiter) -BOOST_CLASS_VERSION(Kernel::AddressArbiter, 1) +BOOST_CLASS_EXPORT_KEY(Kernel::AddressArbiter::Callback) +BOOST_CLASS_VERSION(Kernel::AddressArbiter, 2) CONSTRUCT_KERNEL_OBJECT(Kernel::AddressArbiter) From c5a3bf97285962335a5fd02b6d20dbebca7b0168 Mon Sep 17 00:00:00 2001 From: Hamish Milne Date: Sat, 18 Apr 2020 15:14:01 +0100 Subject: [PATCH 2/4] Fix clang format --- src/core/hle/kernel/address_arbiter.cpp | 4 ++-- src/core/hle/kernel/address_arbiter.h | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/core/hle/kernel/address_arbiter.cpp b/src/core/hle/kernel/address_arbiter.cpp index 97228f153..dba814b66 100644 --- a/src/core/hle/kernel/address_arbiter.cpp +++ b/src/core/hle/kernel/address_arbiter.cpp @@ -84,7 +84,7 @@ public: std::shared_ptr parent; void WakeUp(ThreadWakeupReason reason, std::shared_ptr thread, - std::shared_ptr object) override { + std::shared_ptr object) override { parent->WakeUp(reason, thread, object); } @@ -103,7 +103,7 @@ void AddressArbiter::WakeUp(ThreadWakeupReason reason, std::shared_ptr t ASSERT(reason == ThreadWakeupReason::Timeout); // Remove the newly-awakened thread from the Arbiter's waiting list. waiting_threads.erase(std::remove(waiting_threads.begin(), waiting_threads.end(), thread), - waiting_threads.end()); + waiting_threads.end()); }; ResultCode AddressArbiter::ArbitrateAddress(std::shared_ptr thread, ArbitrationType type, diff --git a/src/core/hle/kernel/address_arbiter.h b/src/core/hle/kernel/address_arbiter.h index de7631066..4d8b12241 100644 --- a/src/core/hle/kernel/address_arbiter.h +++ b/src/core/hle/kernel/address_arbiter.h @@ -93,9 +93,10 @@ private: void serialize(Archive& ar, const unsigned int file_version) { ar& boost::serialization::base_object(*this); if (file_version == 1) { - // This rigmarole is needed because in past versions, AddressArbiter inherited WakeupCallback - // But it turns out this breaks shared_from_this, so we split it out. - // Using a dummy class to deserialize a base_object allows compatibility to be maintained. + // This rigmarole is needed because in past versions, AddressArbiter inherited + // WakeupCallback But it turns out this breaks shared_from_this, so we split it out. + // Using a dummy class to deserialize a base_object allows compatibility to be + // maintained. DummyCallback x; ar& boost::serialization::base_object(x); } From 55c9162d021075a837ef98083de7ba35316768c4 Mon Sep 17 00:00:00 2001 From: Hamish Milne Date: Mon, 20 Apr 2020 16:29:50 +0100 Subject: [PATCH 3/4] Clean up the Callback (don't store a shared_ptr to parent) --- src/core/hle/kernel/address_arbiter.cpp | 34 ++++++++++++++++--------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/src/core/hle/kernel/address_arbiter.cpp b/src/core/hle/kernel/address_arbiter.cpp index dba814b66..d157143b7 100644 --- a/src/core/hle/kernel/address_arbiter.cpp +++ b/src/core/hle/kernel/address_arbiter.cpp @@ -67,7 +67,8 @@ std::shared_ptr AddressArbiter::ResumeHighestPriorityThread(VAddr addres return thread; } -AddressArbiter::AddressArbiter(KernelSystem& kernel) : Object(kernel), kernel(kernel) {} +AddressArbiter::AddressArbiter(KernelSystem& kernel) + : Object(kernel), kernel(kernel), timeout_callback(std::make_shared(*this)) {} AddressArbiter::~AddressArbiter() {} std::shared_ptr KernelSystem::CreateAddressArbiter(std::string name) { @@ -80,20 +81,18 @@ std::shared_ptr KernelSystem::CreateAddressArbiter(std::string n class AddressArbiter::Callback : public WakeupCallback { public: - Callback(AddressArbiter& _parent) : parent(SharedFrom(&_parent)) {} - std::shared_ptr parent; + Callback(AddressArbiter& _parent) : parent(_parent) {} + AddressArbiter& parent; void WakeUp(ThreadWakeupReason reason, std::shared_ptr thread, std::shared_ptr object) override { - parent->WakeUp(reason, thread, object); + parent.WakeUp(reason, thread, object); } private: - Callback() = default; template void serialize(Archive& ar, const unsigned int) { ar& boost::serialization::base_object(*this); - ar& parent; } friend class boost::serialization::access; }; @@ -129,9 +128,6 @@ ResultCode AddressArbiter::ArbitrateAddress(std::shared_ptr thread, Arbi } break; case ArbitrationType::WaitIfLessThanWithTimeout: - if (!timeout_callback) { - timeout_callback = std::make_shared(*this); - } if ((s32)kernel.memory.Read32(address) < value) { thread->wakeup_callback = timeout_callback; thread->WakeAfterDelay(nanoseconds); @@ -148,9 +144,6 @@ ResultCode AddressArbiter::ArbitrateAddress(std::shared_ptr thread, Arbi break; } case ArbitrationType::DecrementAndWaitIfLessThanWithTimeout: { - if (!timeout_callback) { - timeout_callback = std::make_shared(*this); - } s32 memory_value = kernel.memory.Read32(address); if (memory_value < value) { // Only change the memory value if the thread should wait @@ -179,5 +172,22 @@ ResultCode AddressArbiter::ArbitrateAddress(std::shared_ptr thread, Arbi } // namespace Kernel +namespace boost::serialization { + +template +void save_construct_data(Archive& ar, const Kernel::AddressArbiter::Callback* t, + const unsigned int) { + ar << Kernel::SharedFrom(&t->parent); +} + +template +void load_construct_data(Archive& ar, Kernel::AddressArbiter::Callback* t, const unsigned int) { + std::shared_ptr parent; + ar >> parent; + ::new (t) Kernel::AddressArbiter::Callback(*parent); +} + +} // namespace boost::serialization + SERIALIZE_EXPORT_IMPL(Kernel::AddressArbiter) SERIALIZE_EXPORT_IMPL(Kernel::AddressArbiter::Callback) From 67832899098c1438be1ab4b782a7c9998b6b66de Mon Sep 17 00:00:00 2001 From: Hamish Milne Date: Mon, 20 Apr 2020 16:30:49 +0100 Subject: [PATCH 4/4] CR actions --- src/core/hle/kernel/address_arbiter.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/hle/kernel/address_arbiter.cpp b/src/core/hle/kernel/address_arbiter.cpp index d157143b7..23a9bb8d1 100644 --- a/src/core/hle/kernel/address_arbiter.cpp +++ b/src/core/hle/kernel/address_arbiter.cpp @@ -81,12 +81,12 @@ std::shared_ptr KernelSystem::CreateAddressArbiter(std::string n class AddressArbiter::Callback : public WakeupCallback { public: - Callback(AddressArbiter& _parent) : parent(_parent) {} + explicit Callback(AddressArbiter& _parent) : parent(_parent) {} AddressArbiter& parent; void WakeUp(ThreadWakeupReason reason, std::shared_ptr thread, std::shared_ptr object) override { - parent.WakeUp(reason, thread, object); + parent.WakeUp(reason, std::move(thread), std::move(object)); } private: