diff --git a/src/citra_qt/debugger/wait_tree.cpp b/src/citra_qt/debugger/wait_tree.cpp index 8c244b6b2c..ebcc02894c 100644 --- a/src/citra_qt/debugger/wait_tree.cpp +++ b/src/citra_qt/debugger/wait_tree.cpp @@ -257,10 +257,9 @@ std::vector> WaitTreeMutex::GetChildren() const { std::vector> list(WaitTreeWaitObject::GetChildren()); const auto& mutex = static_cast(object); - if (mutex.lock_count) { - list.push_back( - std::make_unique(tr("locked %1 times by thread:").arg(mutex.lock_count))); - list.push_back(std::make_unique(*mutex.holding_thread)); + if (mutex.GetHasWaiters()) { + list.push_back(std::make_unique(tr("locked by thread:"))); + list.push_back(std::make_unique(*mutex.GetHoldingThread())); } else { list.push_back(std::make_unique(tr("free"))); } diff --git a/src/core/hle/kernel/mutex.cpp b/src/core/hle/kernel/mutex.cpp index 1b7cda7403..0ee92a3e3a 100644 --- a/src/core/hle/kernel/mutex.cpp +++ b/src/core/hle/kernel/mutex.cpp @@ -17,8 +17,8 @@ namespace Kernel { void ReleaseThreadMutexes(Thread* thread) { for (auto& mtx : thread->held_mutexes) { - mtx->lock_count = 0; - mtx->holding_thread = nullptr; + mtx->SetHasWaiters(false); + mtx->SetHoldingThread(nullptr); mtx->WakeupAllWaitingThreads(); } thread->held_mutexes.clear(); @@ -31,10 +31,8 @@ SharedPtr Mutex::Create(SharedPtr holding_thread, VAddr g std::string name) { SharedPtr mutex(new Mutex); - mutex->lock_count = 0; mutex->guest_addr = guest_addr; mutex->name = std::move(name); - mutex->holding_thread = nullptr; // If mutex was initialized with a holding thread, acquire it by the holding thread if (holding_thread) { @@ -51,56 +49,32 @@ SharedPtr Mutex::Create(SharedPtr holding_thread, VAddr g } bool Mutex::ShouldWait(Thread* thread) const { - return lock_count > 0 && thread != holding_thread; + auto holding_thread = GetHoldingThread(); + return holding_thread != nullptr && thread != holding_thread; } void Mutex::Acquire(Thread* thread) { ASSERT_MSG(!ShouldWait(thread), "object unavailable!"); - // Actually "acquire" the mutex only if we don't already have it - if (lock_count == 0) { - priority = thread->current_priority; - thread->held_mutexes.insert(this); - holding_thread = thread; - thread->UpdatePriority(); - UpdateGuestState(); - Core::System::GetInstance().PrepareReschedule(); - } - - lock_count++; + priority = thread->current_priority; + thread->held_mutexes.insert(this); + SetHoldingThread(thread); + thread->UpdatePriority(); + Core::System::GetInstance().PrepareReschedule(); } ResultCode Mutex::Release(Thread* thread) { + auto holding_thread = GetHoldingThread(); + ASSERT(holding_thread); + // We can only release the mutex if it's held by the calling thread. - if (thread != holding_thread) { - if (holding_thread) { - LOG_ERROR( - Kernel, - "Tried to release a mutex (owned by thread id %u) from a different thread id %u", - holding_thread->thread_id, thread->thread_id); - } - // TODO(bunnei): Use correct error code - return ResultCode(-1); - } + ASSERT(thread == holding_thread); - // Note: It should not be possible for the situation where the mutex has a holding thread with a - // zero lock count to occur. The real kernel still checks for this, so we do too. - if (lock_count <= 0) { - // TODO(bunnei): Use correct error code - return ResultCode(-1); - } - - lock_count--; - - // Yield to the next thread only if we've fully released the mutex - if (lock_count == 0) { - holding_thread->held_mutexes.erase(this); - holding_thread->UpdatePriority(); - holding_thread = nullptr; - WakeupAllWaitingThreads(); - UpdateGuestState(); - Core::System::GetInstance().PrepareReschedule(); - } + holding_thread->held_mutexes.erase(this); + holding_thread->UpdatePriority(); + SetHoldingThread(nullptr); + WakeupAllWaitingThreads(); + Core::System::GetInstance().PrepareReschedule(); return RESULT_SUCCESS; } @@ -108,19 +82,20 @@ ResultCode Mutex::Release(Thread* thread) { void Mutex::AddWaitingThread(SharedPtr thread) { WaitObject::AddWaitingThread(thread); thread->pending_mutexes.insert(this); + SetHasWaiters(true); UpdatePriority(); - UpdateGuestState(); } void Mutex::RemoveWaitingThread(Thread* thread) { WaitObject::RemoveWaitingThread(thread); thread->pending_mutexes.erase(this); + if (!GetHasWaiters()) + SetHasWaiters(!GetWaitingThreads().empty()); UpdatePriority(); - UpdateGuestState(); } void Mutex::UpdatePriority() { - if (!holding_thread) + if (!GetHoldingThread()) return; u32 best_priority = THREADPRIO_LOWEST; @@ -131,21 +106,35 @@ void Mutex::UpdatePriority() { if (best_priority != priority) { priority = best_priority; - holding_thread->UpdatePriority(); + GetHoldingThread()->UpdatePriority(); } } -void Mutex::UpdateGuestState() { +Handle Mutex::GetOwnerHandle() const { GuestState guest_state{Memory::Read32(guest_addr)}; - guest_state.has_waiters.Assign(!GetWaitingThreads().empty()); - guest_state.holding_thread_handle.Assign(holding_thread ? holding_thread->guest_handle : 0); + return guest_state.holding_thread_handle; +} + +SharedPtr Mutex::GetHoldingThread() const { + GuestState guest_state{Memory::Read32(guest_addr)}; + return g_handle_table.Get(guest_state.holding_thread_handle); +} + +void Mutex::SetHoldingThread(SharedPtr thread) { + GuestState guest_state{Memory::Read32(guest_addr)}; + guest_state.holding_thread_handle.Assign(thread ? thread->guest_handle : 0); Memory::Write32(guest_addr, guest_state.raw); } -void Mutex::VerifyGuestState() { +bool Mutex::GetHasWaiters() const { GuestState guest_state{Memory::Read32(guest_addr)}; - ASSERT(guest_state.has_waiters == !GetWaitingThreads().empty()); - ASSERT(guest_state.holding_thread_handle == holding_thread->guest_handle); + return guest_state.has_waiters != 0; +} + +void Mutex::SetHasWaiters(bool has_waiters) { + GuestState guest_state{Memory::Read32(guest_addr)}; + guest_state.has_waiters.Assign(has_waiters ? 1 : 0); + Memory::Write32(guest_addr, guest_state.raw); } } // namespace Kernel diff --git a/src/core/hle/kernel/mutex.h b/src/core/hle/kernel/mutex.h index 87e3c15ee6..49b6b454e6 100644 --- a/src/core/hle/kernel/mutex.h +++ b/src/core/hle/kernel/mutex.h @@ -41,10 +41,8 @@ public: return HANDLE_TYPE; } - int lock_count; ///< Number of times the mutex has been acquired u32 priority; ///< The priority of the mutex, used for priority inheritance. std::string name; ///< Name of mutex (optional) - SharedPtr holding_thread; ///< Thread that has acquired the mutex VAddr guest_addr; ///< Address of the guest mutex value /** @@ -66,6 +64,19 @@ public: */ ResultCode Release(Thread* thread); + /// Gets the handle to the holding process stored in the guest state. + Handle GetOwnerHandle() const; + + /// Gets the Thread pointed to by the owner handle + SharedPtr GetHoldingThread() const; + /// Sets the holding process handle in the guest state. + void SetHoldingThread(SharedPtr thread); + + /// Returns the has_waiters bit in the guest state. + bool GetHasWaiters() const; + /// Sets the has_waiters bit in the guest state. + void SetHasWaiters(bool has_waiters); + private: Mutex(); ~Mutex() override; @@ -79,12 +90,6 @@ private: BitField<30, 1, u32_le> has_waiters; }; static_assert(sizeof(GuestState) == 4, "GuestState size is incorrect"); - - /// Updates the state of the object tracking this mutex in guest memory - void UpdateGuestState(); - - /// Verifies the state of the object tracking this mutex in guest memory - void VerifyGuestState(); }; /** diff --git a/src/core/hle/kernel/svc.cpp b/src/core/hle/kernel/svc.cpp index 73793955a8..a3ac3d7828 100644 --- a/src/core/hle/kernel/svc.cpp +++ b/src/core/hle/kernel/svc.cpp @@ -204,7 +204,6 @@ static ResultCode LockMutex(Handle holding_thread_handle, VAddr mutex_addr, SharedPtr holding_thread = g_handle_table.Get(holding_thread_handle); SharedPtr requesting_thread = g_handle_table.Get(requesting_thread_handle); - ASSERT(holding_thread); ASSERT(requesting_thread); SharedPtr mutex = g_object_address_table.Get(mutex_addr); @@ -214,6 +213,8 @@ static ResultCode LockMutex(Handle holding_thread_handle, VAddr mutex_addr, mutex->name = Common::StringFromFormat("mutex-%llx", mutex_addr); } + ASSERT(holding_thread == mutex->GetHoldingThread()); + return WaitSynchronization1(mutex, requesting_thread.get()); } @@ -491,6 +492,8 @@ static ResultCode WaitProcessWideKeyAtomic(VAddr mutex_addr, VAddr semaphore_add mutex->name = Common::StringFromFormat("mutex-%llx", mutex_addr); } + ASSERT(mutex->GetOwnerHandle() == thread_handle); + SharedPtr semaphore = g_object_address_table.Get(semaphore_addr); if (!semaphore) { // Create a new semaphore for the specified address if one does not already exist