From 51fb0a6f9647ba199da10fe4f018ee36e44e65ba Mon Sep 17 00:00:00 2001 From: bunnei Date: Sat, 27 Feb 2021 11:56:04 -0800 Subject: [PATCH] core: Switch to unique_ptr for usage of Common::Fiber. - With using unique_ptr instead of shared_ptr, we have more explicit ownership of the context. - Fixes a memory leak due to circular reference of the shared pointer. --- src/common/fiber.cpp | 14 +++++------ src/common/fiber.h | 4 +-- src/core/cpu_manager.cpp | 11 ++++---- src/core/cpu_manager.h | 2 +- src/core/hle/kernel/k_scheduler.cpp | 18 ++++++------- src/core/hle/kernel/k_scheduler.h | 10 ++++---- src/core/hle/kernel/k_thread.cpp | 6 +---- src/core/hle/kernel/k_thread.h | 10 ++++++-- src/core/hle/kernel/svc.cpp | 3 +-- src/tests/common/fibers.cpp | 39 +++++++++++++---------------- 10 files changed, 58 insertions(+), 59 deletions(-) diff --git a/src/common/fiber.cpp b/src/common/fiber.cpp index 3c1eefcb7..b8e98b12a 100644 --- a/src/common/fiber.cpp +++ b/src/common/fiber.cpp @@ -24,7 +24,7 @@ struct Fiber::FiberImpl { std::function rewind_point; void* rewind_parameter{}; void* start_parameter{}; - std::shared_ptr previous_fiber; + Fiber* previous_fiber; bool is_thread_fiber{}; bool released{}; @@ -47,7 +47,7 @@ void Fiber::Start(boost::context::detail::transfer_t& transfer) { ASSERT(impl->previous_fiber != nullptr); impl->previous_fiber->impl->context = transfer.fctx; impl->previous_fiber->impl->guard.unlock(); - impl->previous_fiber.reset(); + impl->previous_fiber = nullptr; impl->entry_point(impl->start_parameter); UNREACHABLE(); } @@ -116,20 +116,20 @@ void Fiber::Rewind() { boost::context::detail::jump_fcontext(impl->rewind_context, this); } -void Fiber::YieldTo(std::shared_ptr from, std::shared_ptr to) { +void Fiber::YieldTo(Fiber* from, Fiber* to) { ASSERT_MSG(from != nullptr, "Yielding fiber is null!"); ASSERT_MSG(to != nullptr, "Next fiber is null!"); to->impl->guard.lock(); to->impl->previous_fiber = from; - auto transfer = boost::context::detail::jump_fcontext(to->impl->context, to.get()); + auto transfer = boost::context::detail::jump_fcontext(to->impl->context, to); ASSERT(from->impl->previous_fiber != nullptr); from->impl->previous_fiber->impl->context = transfer.fctx; from->impl->previous_fiber->impl->guard.unlock(); - from->impl->previous_fiber.reset(); + from->impl->previous_fiber = nullptr; } -std::shared_ptr Fiber::ThreadToFiber() { - std::shared_ptr fiber = std::shared_ptr{new Fiber()}; +std::unique_ptr Fiber::ThreadToFiber() { + std::unique_ptr fiber = std::unique_ptr{new Fiber()}; fiber->impl->guard.lock(); fiber->impl->is_thread_fiber = true; return fiber; diff --git a/src/common/fiber.h b/src/common/fiber.h index f7f587f8c..6924f7996 100644 --- a/src/common/fiber.h +++ b/src/common/fiber.h @@ -41,8 +41,8 @@ public: /// Yields control from Fiber 'from' to Fiber 'to' /// Fiber 'from' must be the currently running fiber. - static void YieldTo(std::shared_ptr from, std::shared_ptr to); - [[nodiscard]] static std::shared_ptr ThreadToFiber(); + static void YieldTo(Fiber* from, Fiber* to); + [[nodiscard]] static std::unique_ptr ThreadToFiber(); void SetRewindPoint(std::function&& rewind_func, void* rewind_param); diff --git a/src/core/cpu_manager.cpp b/src/core/cpu_manager.cpp index 8f04fb8f5..c35438c6f 100644 --- a/src/core/cpu_manager.cpp +++ b/src/core/cpu_manager.cpp @@ -111,7 +111,7 @@ void CpuManager::MultiCoreRunGuestThread() { auto& kernel = system.Kernel(); kernel.CurrentScheduler()->OnThreadStart(); auto* thread = kernel.CurrentScheduler()->GetCurrentThread(); - auto& host_context = thread->GetHostContext(); + auto host_context = thread->GetHostContext(); host_context->SetRewindPoint(GuestRewindFunction, this); MultiCoreRunGuestLoop(); } @@ -148,7 +148,8 @@ void CpuManager::MultiCoreRunSuspendThread() { auto core = kernel.GetCurrentHostThreadID(); auto& scheduler = *kernel.CurrentScheduler(); Kernel::KThread* current_thread = scheduler.GetCurrentThread(); - Common::Fiber::YieldTo(current_thread->GetHostContext(), core_data[core].host_context); + Common::Fiber::YieldTo(current_thread->GetHostContext(), + core_data[core].host_context.get()); ASSERT(scheduler.ContextSwitchPending()); ASSERT(core == kernel.GetCurrentHostThreadID()); scheduler.RescheduleCurrentCore(); @@ -201,7 +202,7 @@ void CpuManager::SingleCoreRunGuestThread() { auto& kernel = system.Kernel(); kernel.CurrentScheduler()->OnThreadStart(); auto* thread = kernel.CurrentScheduler()->GetCurrentThread(); - auto& host_context = thread->GetHostContext(); + auto host_context = thread->GetHostContext(); host_context->SetRewindPoint(GuestRewindFunction, this); SingleCoreRunGuestLoop(); } @@ -245,7 +246,7 @@ void CpuManager::SingleCoreRunSuspendThread() { auto core = kernel.GetCurrentHostThreadID(); auto& scheduler = *kernel.CurrentScheduler(); Kernel::KThread* current_thread = scheduler.GetCurrentThread(); - Common::Fiber::YieldTo(current_thread->GetHostContext(), core_data[0].host_context); + Common::Fiber::YieldTo(current_thread->GetHostContext(), core_data[0].host_context.get()); ASSERT(scheduler.ContextSwitchPending()); ASSERT(core == kernel.GetCurrentHostThreadID()); scheduler.RescheduleCurrentCore(); @@ -363,7 +364,7 @@ void CpuManager::RunThread(std::size_t core) { auto current_thread = system.Kernel().CurrentScheduler()->GetCurrentThread(); data.is_running = true; - Common::Fiber::YieldTo(data.host_context, current_thread->GetHostContext()); + Common::Fiber::YieldTo(data.host_context.get(), current_thread->GetHostContext()); data.is_running = false; data.is_paused = true; data.exit_barrier->Wait(); diff --git a/src/core/cpu_manager.h b/src/core/cpu_manager.h index 17420c941..5ea149f1f 100644 --- a/src/core/cpu_manager.h +++ b/src/core/cpu_manager.h @@ -83,7 +83,7 @@ private: void RunThread(std::size_t core); struct CoreData { - std::shared_ptr host_context; + std::unique_ptr host_context; std::unique_ptr enter_barrier; std::unique_ptr exit_barrier; std::atomic is_running; diff --git a/src/core/hle/kernel/k_scheduler.cpp b/src/core/hle/kernel/k_scheduler.cpp index bb5f43b53..465036f3d 100644 --- a/src/core/hle/kernel/k_scheduler.cpp +++ b/src/core/hle/kernel/k_scheduler.cpp @@ -608,7 +608,7 @@ void KScheduler::YieldToAnyThread(KernelCore& kernel) { } KScheduler::KScheduler(Core::System& system, s32 core_id) : system(system), core_id(core_id) { - switch_fiber = std::make_shared(OnSwitch, this); + switch_fiber = std::make_unique(OnSwitch, this); state.needs_scheduling.store(true); state.interrupt_task_thread_runnable = false; state.should_count_idle = false; @@ -726,15 +726,15 @@ void KScheduler::ScheduleImpl() { // Save context for previous thread Unload(previous_thread); - std::shared_ptr* old_context; + Common::Fiber* old_context; if (previous_thread != nullptr) { - old_context = &previous_thread->GetHostContext(); + old_context = previous_thread->GetHostContext(); } else { - old_context = &idle_thread->GetHostContext(); + old_context = idle_thread->GetHostContext(); } guard.unlock(); - Common::Fiber::YieldTo(*old_context, switch_fiber); + Common::Fiber::YieldTo(old_context, switch_fiber.get()); /// When a thread wakes up, the scheduler may have changed to other in another core. auto& next_scheduler = *system.Kernel().CurrentScheduler(); next_scheduler.SwitchContextStep2(); @@ -769,13 +769,13 @@ void KScheduler::SwitchToCurrent() { break; } } - std::shared_ptr* next_context; + Common::Fiber* next_context; if (next_thread != nullptr) { - next_context = &next_thread->GetHostContext(); + next_context = next_thread->GetHostContext(); } else { - next_context = &idle_thread->GetHostContext(); + next_context = idle_thread->GetHostContext(); } - Common::Fiber::YieldTo(switch_fiber, *next_context); + Common::Fiber::YieldTo(switch_fiber.get(), next_context); } while (!is_switch_pending()); } } diff --git a/src/core/hle/kernel/k_scheduler.h b/src/core/hle/kernel/k_scheduler.h index f595b9a5c..a4285c595 100644 --- a/src/core/hle/kernel/k_scheduler.h +++ b/src/core/hle/kernel/k_scheduler.h @@ -68,12 +68,12 @@ public: void OnThreadStart(); - [[nodiscard]] std::shared_ptr& ControlContext() { - return switch_fiber; + [[nodiscard]] Common::Fiber* ControlContext() { + return switch_fiber.get(); } - [[nodiscard]] const std::shared_ptr& ControlContext() const { - return switch_fiber; + [[nodiscard]] const Common::Fiber* ControlContext() const { + return switch_fiber.get(); } [[nodiscard]] u64 UpdateHighestPriorityThread(KThread* highest_thread); @@ -178,7 +178,7 @@ private: KThread* idle_thread; - std::shared_ptr switch_fiber{}; + std::unique_ptr switch_fiber{}; struct SchedulingState { std::atomic needs_scheduling; diff --git a/src/core/hle/kernel/k_thread.cpp b/src/core/hle/kernel/k_thread.cpp index 1661afbd9..f49e31b72 100644 --- a/src/core/hle/kernel/k_thread.cpp +++ b/src/core/hle/kernel/k_thread.cpp @@ -991,10 +991,6 @@ void KThread::SetState(ThreadState state) { } } -std::shared_ptr& KThread::GetHostContext() { - return host_context; -} - ResultVal> KThread::Create(Core::System& system, ThreadType type_flags, std::string name, VAddr entry_point, u32 priority, u64 arg, s32 processor_id, @@ -1028,7 +1024,7 @@ ResultVal> KThread::Create(Core::System& system, Thread scheduler.AddThread(thread); thread->host_context = - std::make_shared(std::move(thread_start_func), thread_start_parameter); + std::make_unique(std::move(thread_start_func), thread_start_parameter); return MakeResult>(std::move(thread)); } diff --git a/src/core/hle/kernel/k_thread.h b/src/core/hle/kernel/k_thread.h index c8ac656a4..a2893d939 100644 --- a/src/core/hle/kernel/k_thread.h +++ b/src/core/hle/kernel/k_thread.h @@ -293,7 +293,13 @@ public: return thread_context_64; } - [[nodiscard]] std::shared_ptr& GetHostContext(); + [[nodiscard]] Common::Fiber* GetHostContext() { + return host_context.get(); + } + + [[nodiscard]] const Common::Fiber* GetHostContext() const { + return host_context.get(); + } [[nodiscard]] ThreadState GetState() const { return thread_state & ThreadState::Mask; @@ -719,7 +725,7 @@ private: Common::SpinLock context_guard{}; // For emulation - std::shared_ptr host_context{}; + std::unique_ptr host_context{}; // For debugging std::vector wait_objects_for_debugging; diff --git a/src/core/hle/kernel/svc.cpp b/src/core/hle/kernel/svc.cpp index cc8fa6576..d04116115 100644 --- a/src/core/hle/kernel/svc.cpp +++ b/src/core/hle/kernel/svc.cpp @@ -2626,8 +2626,7 @@ void Call(Core::System& system, u32 immediate) { kernel.ExitSVCProfile(); if (!thread->IsCallingSvc()) { - auto* host_context = thread->GetHostContext().get(); - host_context->Rewind(); + thread->GetHostContext()->Rewind(); } system.EnterDynarmicProfile(); diff --git a/src/tests/common/fibers.cpp b/src/tests/common/fibers.cpp index d94492fc6..b2ded2065 100644 --- a/src/tests/common/fibers.cpp +++ b/src/tests/common/fibers.cpp @@ -67,16 +67,15 @@ void TestControl1::DoWork() { value++; } results[id] = value; - Fiber::YieldTo(work_fibers[id], thread_fibers[id]); + Fiber::YieldTo(work_fibers[id].get(), thread_fibers[id].get()); } void TestControl1::ExecuteThread(u32 id) { thread_ids.Register(id); - auto thread_fiber = Fiber::ThreadToFiber(); - thread_fibers[id] = thread_fiber; + thread_fibers[id] = Fiber::ThreadToFiber(); work_fibers[id] = std::make_shared(std::function{WorkControl1}, this); items[id] = rand() % 256; - Fiber::YieldTo(thread_fibers[id], work_fibers[id]); + Fiber::YieldTo(thread_fibers[id].get(), work_fibers[id].get()); thread_fibers[id]->Exit(); } @@ -117,11 +116,11 @@ public: for (u32 i = 0; i < 12000; i++) { value1 += i; } - Fiber::YieldTo(fiber1, fiber3); + Fiber::YieldTo(fiber1.get(), fiber3.get()); const u32 id = thread_ids.Get(); assert1 = id == 1; value2 += 5000; - Fiber::YieldTo(fiber1, thread_fibers[id]); + Fiber::YieldTo(fiber1.get(), thread_fibers[id].get()); } void DoWork2() { @@ -129,7 +128,7 @@ public: ; value2 = 2000; trap = false; - Fiber::YieldTo(fiber2, fiber1); + Fiber::YieldTo(fiber2.get(), fiber1.get()); assert3 = false; } @@ -137,19 +136,19 @@ public: const u32 id = thread_ids.Get(); assert2 = id == 0; value1 += 1000; - Fiber::YieldTo(fiber3, thread_fibers[id]); + Fiber::YieldTo(fiber3.get(), thread_fibers[id].get()); } void ExecuteThread(u32 id); void CallFiber1() { const u32 id = thread_ids.Get(); - Fiber::YieldTo(thread_fibers[id], fiber1); + Fiber::YieldTo(thread_fibers[id].get(), fiber1.get()); } void CallFiber2() { const u32 id = thread_ids.Get(); - Fiber::YieldTo(thread_fibers[id], fiber2); + Fiber::YieldTo(thread_fibers[id].get(), fiber2.get()); } void Exit(); @@ -185,8 +184,7 @@ static void WorkControl2_3(void* control) { void TestControl2::ExecuteThread(u32 id) { thread_ids.Register(id); - auto thread_fiber = Fiber::ThreadToFiber(); - thread_fibers[id] = thread_fiber; + thread_fibers[id] = Fiber::ThreadToFiber(); } void TestControl2::Exit() { @@ -241,23 +239,23 @@ public: void DoWork1() { value1 += 1; - Fiber::YieldTo(fiber1, fiber2); + Fiber::YieldTo(fiber1.get(), fiber2.get()); const u32 id = thread_ids.Get(); value3 += 1; - Fiber::YieldTo(fiber1, thread_fibers[id]); + Fiber::YieldTo(fiber1.get(), thread_fibers[id].get()); } void DoWork2() { value2 += 1; const u32 id = thread_ids.Get(); - Fiber::YieldTo(fiber2, thread_fibers[id]); + Fiber::YieldTo(fiber2.get(), thread_fibers[id].get()); } void ExecuteThread(u32 id); void CallFiber1() { const u32 id = thread_ids.Get(); - Fiber::YieldTo(thread_fibers[id], fiber1); + Fiber::YieldTo(thread_fibers[id].get(), fiber1.get()); } void Exit(); @@ -266,7 +264,7 @@ public: u32 value2{}; u32 value3{}; ThreadIds thread_ids; - std::vector> thread_fibers; + std::vector> thread_fibers; std::shared_ptr fiber1; std::shared_ptr fiber2; }; @@ -283,8 +281,7 @@ static void WorkControl3_2(void* control) { void TestControl3::ExecuteThread(u32 id) { thread_ids.Register(id); - auto thread_fiber = Fiber::ThreadToFiber(); - thread_fibers[id] = thread_fiber; + thread_fibers[id] = Fiber::ThreadToFiber(); } void TestControl3::Exit() { @@ -332,7 +329,7 @@ public: void Execute() { thread_fiber = Fiber::ThreadToFiber(); - Fiber::YieldTo(thread_fiber, fiber1); + Fiber::YieldTo(thread_fiber.get(), fiber1.get()); thread_fiber->Exit(); } @@ -340,7 +337,7 @@ public: fiber1->SetRewindPoint(std::function{WorkControl4}, this); if (rewinded) { goal_reached = true; - Fiber::YieldTo(fiber1, thread_fiber); + Fiber::YieldTo(fiber1.get(), thread_fiber.get()); } rewinded = true; fiber1->Rewind();