From c956b82d27d49a0a9a49d29420156e2c9f659fcf Mon Sep 17 00:00:00 2001 From: Tony Wasserka Date: Tue, 25 Jul 2023 10:40:49 +0200 Subject: [PATCH 1/4] ScopedSignalMask/DeferredSignalMutex: Clean up API and use std::unique_lock/shared_lock --- FEXCore/Source/Interface/Context/Context.h | 4 +- FEXCore/Source/Interface/Core/Core.cpp | 8 +- .../Source/Utils/Allocator/64BitAllocator.cpp | 6 +- .../FEXCore/Utils/DeferredSignalMutex.h | 188 ++++++------------ .../FEXHeaderUtils/ScopedSignalMask.h | 144 +++++--------- .../LinuxSyscalls/SyscallsSMCTracking.cpp | 24 +-- 6 files changed, 122 insertions(+), 252 deletions(-) diff --git a/FEXCore/Source/Interface/Context/Context.h b/FEXCore/Source/Interface/Context/Context.h index dd053719d..3b3572fce 100644 --- a/FEXCore/Source/Interface/Context/Context.h +++ b/FEXCore/Source/Interface/Context/Context.h @@ -295,7 +295,7 @@ namespace FEXCore::Context { template static uint64_t ThreadExitFunctionLink(FEXCore::Core::CpuStateFrame *Frame, uint64_t *record) { auto Thread = Frame->Thread; - ScopedDeferredSignalWithForkableSharedLock lk(static_cast(Thread->CTX)->CodeInvalidationMutex, Thread); + auto lk = GuardSignalDeferringSection(static_cast(Thread->CTX)->CodeInvalidationMutex, Thread); return Fn(Frame, record); } @@ -306,7 +306,7 @@ namespace FEXCore::Context { auto Thread = Frame->Thread; LogMan::Throw::AFmt(Thread->ThreadManager.GetTID() == FHU::Syscalls::gettid(), "Must be called from owning thread {}, not {}", Thread->ThreadManager.GetTID(), FHU::Syscalls::gettid()); - ScopedDeferredSignalWithForkableUniqueLock lk(static_cast(Thread->CTX)->CodeInvalidationMutex, Thread); + auto lk = GuardSignalDeferringSection(static_cast(Thread->CTX)->CodeInvalidationMutex, Thread); ThreadRemoveCodeEntry(Thread, GuestRIP); } diff --git a/FEXCore/Source/Interface/Core/Core.cpp b/FEXCore/Source/Interface/Core/Core.cpp index 76713c291..0cc0b0602 100644 --- a/FEXCore/Source/Interface/Core/Core.cpp +++ b/FEXCore/Source/Interface/Core/Core.cpp @@ -1097,7 +1097,7 @@ namespace FEXCore::Context { auto Thread = Frame->Thread; // Invalidate might take a unique lock on this, to guarantee that during invalidation no code gets compiled - ScopedDeferredSignalWithForkableSharedLock lk(CodeInvalidationMutex, Thread); + auto lk = GuardSignalDeferringSection(CodeInvalidationMutex, Thread); // Is the code in the cache? // The backends only check L1 and L2, not L3 @@ -1280,7 +1280,7 @@ namespace FEXCore::Context { // Potential deferred since Thread might not be valid. // Thread object isn't valid very early in frontend's initialization. // To be more optimal the frontend should provide this code with a valid Thread object earlier. - ScopedPotentialDeferredSignalWithForkableUniqueLock lk(CodeInvalidationMutex, Thread); + auto lk = GuardSignalDeferringSectionWithFallback(CodeInvalidationMutex, Thread); InvalidateGuestCodeRangeInternal(this, Start, Length); } @@ -1289,7 +1289,7 @@ namespace FEXCore::Context { // Potential deferred since Thread might not be valid. // Thread object isn't valid very early in frontend's initialization. // To be more optimal the frontend should provide this code with a valid Thread object earlier. - ScopedPotentialDeferredSignalWithForkableUniqueLock lk(CodeInvalidationMutex, Thread); + auto lk = GuardSignalDeferringSectionWithFallback(CodeInvalidationMutex, Thread); InvalidateGuestCodeRangeInternal(this, Start, Length); CallAfter(Start, Length); @@ -1317,7 +1317,7 @@ namespace FEXCore::Context { } void ContextImpl::ThreadAddBlockLink(FEXCore::Core::InternalThreadState *Thread, uint64_t GuestDestination, uintptr_t HostLink, const std::function &delinker) { - ScopedDeferredSignalWithForkableSharedLock lk(static_cast(Thread->CTX)->CodeInvalidationMutex, Thread); + auto lk = GuardSignalDeferringSection(static_cast(Thread->CTX)->CodeInvalidationMutex, Thread); Thread->LookupCache->AddBlockLink(GuestDestination, HostLink, delinker); } diff --git a/FEXCore/Source/Utils/Allocator/64BitAllocator.cpp b/FEXCore/Source/Utils/Allocator/64BitAllocator.cpp index acf236409..2116b45ce 100644 --- a/FEXCore/Source/Utils/Allocator/64BitAllocator.cpp +++ b/FEXCore/Source/Utils/Allocator/64BitAllocator.cpp @@ -272,7 +272,7 @@ void *OSAllocator_64Bit::Mmap(void *addr, size_t length, int prot, int flags, in size_t NumberOfPages = length / FHU::FEX_PAGE_SIZE; // This needs a mutex to be thread safe - FEXCore::ScopedPotentialDeferredSignalWithForkableMutex lk(AllocationMutex, TLSThread); + auto lk = FEXCore::GuardSignalDeferringSectionWithFallback(AllocationMutex, TLSThread); uint64_t AllocatedOffset{}; LiveVMARegion *LiveRegion{}; @@ -460,7 +460,7 @@ int OSAllocator_64Bit::Munmap(void *addr, size_t length) { } // This needs a mutex to be thread safe - FEXCore::ScopedPotentialDeferredSignalWithForkableMutex lk(AllocationMutex, TLSThread); + auto lk = FEXCore::GuardSignalDeferringSectionWithFallback(AllocationMutex, TLSThread); length = FEXCore::AlignUp(length, FHU::FEX_PAGE_SIZE); @@ -585,7 +585,7 @@ OSAllocator_64Bit::OSAllocator_64Bit() { OSAllocator_64Bit::~OSAllocator_64Bit() { // This needs a mutex to be thread safe - FEXCore::ScopedPotentialDeferredSignalWithForkableMutex lk(AllocationMutex, TLSThread); + auto lk = FEXCore::GuardSignalDeferringSectionWithFallback(AllocationMutex, TLSThread); // Walk the pages and deallocate // First walk the live regions diff --git a/FEXCore/include/FEXCore/Utils/DeferredSignalMutex.h b/FEXCore/include/FEXCore/Utils/DeferredSignalMutex.h index 1069f6469..3ffcc8fda 100644 --- a/FEXCore/include/FEXCore/Utils/DeferredSignalMutex.h +++ b/FEXCore/include/FEXCore/Utils/DeferredSignalMutex.h @@ -6,7 +6,7 @@ #include #include #include -#include +#include #include #ifndef _WIN32 #include @@ -162,38 +162,25 @@ namespace FEXCore { }; #endif - template - class ScopedDeferredSignalWithMutexBase final { + class DeferredSignalRefCountGuard final { public: - - ScopedDeferredSignalWithMutexBase(MutexType &_Mutex, FEXCore::Core::InternalThreadState *Thread) - : Mutex {&_Mutex} - , Thread {Thread} { + explicit DeferredSignalRefCountGuard(FEXCore::Core::InternalThreadState *Thread) : Thread(Thread) { // Needs to be atomic so that operations can't end up getting reordered around this. Thread->CurrentFrame->State.DeferredSignalRefCount.Increment(1); - // Lock the mutex - (Mutex->*lock_fn)(); } - // No copy or assignment possible - ScopedDeferredSignalWithMutexBase(const ScopedDeferredSignalWithMutexBase&) = delete; - ScopedDeferredSignalWithMutexBase& operator=(ScopedDeferredSignalWithMutexBase&) = delete; - - // Only move - ScopedDeferredSignalWithMutexBase(ScopedDeferredSignalWithMutexBase &&rhs) - : Mutex {rhs.Mutex} - , Thread {rhs.Thread} { - rhs.Mutex = nullptr; + // Move-only type + DeferredSignalRefCountGuard(const DeferredSignalRefCountGuard&) = delete; + DeferredSignalRefCountGuard& operator=(DeferredSignalRefCountGuard&) = delete; + DeferredSignalRefCountGuard(DeferredSignalRefCountGuard&& rhs) : Thread(rhs.Thread) { + rhs.Thread = nullptr; } - ~ScopedDeferredSignalWithMutexBase() { - if (Mutex != nullptr) { - // Unlock the mutex - (Mutex->*unlock_fn)(); - + ~DeferredSignalRefCountGuard() { + if (Thread) { #ifdef _M_X86_64 // Needs to be atomic so that operations can't end up getting reordered around this. - // Without this, the recount and the signal access could get reordered. + // Without this, the refcount and the signal access could get reordered. auto Result = Thread->CurrentFrame->State.DeferredSignalRefCount.Decrement(1); // X86-64 must do an additional check around the store. @@ -208,131 +195,68 @@ namespace FEXCore { } } private: - MutexType *Mutex; FEXCore::Core::InternalThreadState *Thread; }; - using ScopedDeferredSignalWithMutex = ScopedDeferredSignalWithMutexBase; - using ScopedDeferredSignalWithSharedLock = ScopedDeferredSignalWithMutexBase; - using ScopedDeferredSignalWithUniqueLock = ScopedDeferredSignalWithMutexBase; - - // Forkable variant - using ScopedDeferredSignalWithForkableMutex = ScopedDeferredSignalWithMutexBase< - FEXCore::ForkableUniqueMutex, - &FEXCore::ForkableUniqueMutex::lock, - &FEXCore::ForkableUniqueMutex::unlock>; - using ScopedDeferredSignalWithForkableSharedLock = ScopedDeferredSignalWithMutexBase< - FEXCore::ForkableSharedMutex, - &FEXCore::ForkableSharedMutex::lock_shared, - &FEXCore::ForkableSharedMutex::unlock_shared>; - using ScopedDeferredSignalWithForkableUniqueLock = ScopedDeferredSignalWithMutexBase< - FEXCore::ForkableSharedMutex, - &FEXCore::ForkableSharedMutex::lock, - &FEXCore::ForkableSharedMutex::unlock>; - +#ifndef _WIN32 + // TODO: Duplicated, unify with ScopedSignalMask class ScopedSignalMasker final { public: - ScopedSignalMasker() = default; - - void Mask(uint64_t Mask) { -#ifndef _WIN32 + explicit ScopedSignalMasker(uint64_t Mask) : OriginalMask(0) { // Mask all signals, storing the original incoming mask - ::syscall(SYS_rt_sigprocmask, SIG_SETMASK, &Mask, &OriginalMask, sizeof(OriginalMask)); -#endif + ::syscall(SYS_rt_sigprocmask, SIG_SETMASK, &Mask, &*OriginalMask, sizeof(*OriginalMask)); } // Move-only type ScopedSignalMasker(const ScopedSignalMasker&) = delete; ScopedSignalMasker& operator=(ScopedSignalMasker&) = delete; - ScopedSignalMasker(ScopedSignalMasker &&rhs) = default; - ScopedSignalMasker& operator=(ScopedSignalMasker &&) = default; - - void Unmask() { -#ifndef _WIN32 - ::syscall(SYS_rt_sigprocmask, SIG_SETMASK, &OriginalMask, nullptr, sizeof(OriginalMask)); -#endif - } - private: -#ifndef _WIN32 - uint64_t OriginalMask{}; -#endif - }; - - template - class ScopedPotentialDeferredSignalWithMutexBase final { - public: - ScopedPotentialDeferredSignalWithMutexBase(MutexType &_Mutex, FEXCore::Core::InternalThreadState *Thread, uint64_t Mask = ~0ULL) - : Mutex {&_Mutex} - , Thread {Thread} { - if (Thread) { - Thread->CurrentFrame->State.DeferredSignalRefCount.Increment(1); - } - else { - Masker.Mask(Mask); - } - // Lock the mutex - (Mutex->*lock_fn)(); + ScopedSignalMasker(ScopedSignalMasker&& rhs) : OriginalMask(rhs.OriginalMask) { + rhs.OriginalMask.reset(); } - // No copy or assignment possible - ScopedPotentialDeferredSignalWithMutexBase(const ScopedPotentialDeferredSignalWithMutexBase&) = delete; - ScopedPotentialDeferredSignalWithMutexBase& operator=(ScopedPotentialDeferredSignalWithMutexBase&) = delete; - - // Only move - ScopedPotentialDeferredSignalWithMutexBase(ScopedPotentialDeferredSignalWithMutexBase &&rhs) - : Mutex {rhs.Mutex} - , Thread {rhs.Thread} { - rhs.Mutex = nullptr; - } - - ~ScopedPotentialDeferredSignalWithMutexBase() { - if (Mutex != nullptr) { - // Unlock the mutex - (Mutex->*unlock_fn)(); - - if (Thread) { -#ifdef _M_X86_64 - // Needs to be atomic so that operations can't end up getting reordered around this. - // Without this, the refcount and the signal access could get reordered. - auto Result = Thread->CurrentFrame->State.DeferredSignalRefCount.Decrement(1); - - // X86-64 must do an additional check around the store. - if ((Result - 1) == 0) { - // Must happen after the refcount store - Thread->CurrentFrame->State.DeferredSignalFaultAddress->Store(0); - } -#else - Thread->CurrentFrame->State.DeferredSignalRefCount.Decrement(1); - Thread->CurrentFrame->State.DeferredSignalFaultAddress->Store(0); -#endif - } - else { - // Unmask back to the original signal mask - Masker.Unmask(); - } + ~ScopedSignalMasker() { + if (OriginalMask) { + ::syscall(SYS_rt_sigprocmask, SIG_SETMASK, &OriginalMask, nullptr, sizeof(*OriginalMask)); } } private: - MutexType *Mutex; - ScopedSignalMasker Masker; - FEXCore::Core::InternalThreadState *Thread; + std::optional OriginalMask{}; }; +#endif - using ScopedPotentialDeferredSignalWithMutex = ScopedPotentialDeferredSignalWithMutexBase; - using ScopedPotentialDeferredSignalWithSharedLock = ScopedPotentialDeferredSignalWithMutexBase; - using ScopedPotentialDeferredSignalWithUniqueLock = ScopedPotentialDeferredSignalWithMutexBase; + /** + * @brief Produces a wrapper object around a scoped lock of the given mutex + * while bumping the Thread's deferred signal refcount while the mutex is + * locked. + */ + template class LockType = std::unique_lock, typename MutexType> + [[nodiscard]] static auto GuardSignalDeferringSection(MutexType& mutex, FEXCore::Core::InternalThreadState *Thread, uint64_t Mask = ~0ULL) { + // Refcount is incremented first, and then the lock is acquired. + struct { + std::optional refcount; + LockType lock; + } scope_guard = { DeferredSignalRefCountGuard { Thread }, LockType { mutex } }; + return scope_guard; + } - // Forkable variant - using ScopedPotentialDeferredSignalWithForkableMutex = ScopedPotentialDeferredSignalWithMutexBase< - FEXCore::ForkableUniqueMutex, - &FEXCore::ForkableUniqueMutex::lock, - &FEXCore::ForkableUniqueMutex::unlock>; - using ScopedPotentialDeferredSignalWithForkableSharedLock = ScopedPotentialDeferredSignalWithMutexBase< - FEXCore::ForkableSharedMutex, - &FEXCore::ForkableSharedMutex::lock_shared, - &FEXCore::ForkableSharedMutex::unlock_shared>; - using ScopedPotentialDeferredSignalWithForkableUniqueLock = ScopedPotentialDeferredSignalWithMutexBase< - FEXCore::ForkableSharedMutex, - &FEXCore::ForkableSharedMutex::lock, - &FEXCore::ForkableSharedMutex::unlock>; + // Like GuardSignalDeferringSection but falls back to masking signals when Thread is nullptr + template class LockType = std::unique_lock, typename MutexType> + [[nodiscard]] static auto GuardSignalDeferringSectionWithFallback(MutexType& mutex, FEXCore::Core::InternalThreadState *Thread, uint64_t Mask = ~0ULL) { + struct { + std::optional refcount; +#ifndef _WIN32 + std::optional mask; +#endif + LockType lock; + } scope_guard; + if (Thread) { + scope_guard.refcount.emplace(Thread); + } else { +#ifndef _WIN32 + scope_guard.mask.emplace(Mask); +#endif + } + scope_guard.lock = LockType { mutex }; + return scope_guard; + } } diff --git a/FEXHeaderUtils/FEXHeaderUtils/ScopedSignalMask.h b/FEXHeaderUtils/FEXHeaderUtils/ScopedSignalMask.h index a03045b98..74f66bd36 100644 --- a/FEXHeaderUtils/FEXHeaderUtils/ScopedSignalMask.h +++ b/FEXHeaderUtils/FEXHeaderUtils/ScopedSignalMask.h @@ -1,12 +1,10 @@ // SPDX-License-Identifier: MIT #pragma once -#include - #include #include #include -#include +#include #ifndef _WIN32 #include #include @@ -14,110 +12,58 @@ #include namespace FHU { +#ifndef _WIN32 /** - * @brief A drop-in replacement for std::lock_guard that masks POSIX signals while the mutex is locked + * Masks POSIX signals for the scope the object is active in + */ + class ScopedSignalMasker final { + public: + explicit ScopedSignalMasker(uint64_t Mask) : OriginalMask(0) { + // Mask all signals, storing the original incoming mask + ::syscall(SYS_rt_sigprocmask, SIG_SETMASK, &Mask, &*OriginalMask, sizeof(*OriginalMask)); + } + + // Move-only type + ScopedSignalMasker(const ScopedSignalMasker&) = delete; + ScopedSignalMasker& operator=(ScopedSignalMasker&) = delete; + ScopedSignalMasker(ScopedSignalMasker&& rhs) : OriginalMask(rhs.OriginalMask) { + rhs.OriginalMask.reset(); + } + + ~ScopedSignalMasker() { + if (OriginalMask) { + ::syscall(SYS_rt_sigprocmask, SIG_SETMASK, &OriginalMask, nullptr, sizeof(*OriginalMask)); + } + } + private: + std::optional OriginalMask{}; + }; +#endif + + /** + * @brief Produces a wrapper object around a scoped lock of the given mutex + * while ensuring POSIX signals are masked while the mutex is locked * - * Use this class to prevent reentrancy issues of C++ mutexes with certain signal handlers. + * Use this to prevent reentrancy issues of C++ mutexes with certain signal handlers. * Common examples of such issues are: - * - C++ mutexes not unlocking due to a signal handler longjmping out of a scope owning the mutex + * - C++ mutexes not unlocking due to a signal handler calling longjmp from within a scope owning the mutex * - The signal handler itself using a mutex that would be re-locked if the handler gets invoked * again before unlocking * - * Ownership of this object may be moved, but it is NOT SAFE to move across threads. - * - * Constructor order: - * 1) Mask signals - * 2) Lock Mutex - * - * Destructor Order: - * 1) Unlock Mutex - * 2) Unmask signals + * Ownership of the returned object may be moved, but it is NOT SAFE to move across threads. */ + template class LockType = std::unique_lock, typename MutexType> + [[nodiscard]] static auto MaskSignalsAndLockMutex(MutexType& mutex, uint64_t Mask = ~0ULL) { #ifndef _WIN32 - template - class ScopedSignalMaskWithMutexBase final { - public: - - ScopedSignalMaskWithMutexBase(MutexType &_Mutex, uint64_t Mask = ~0ULL) - : Mutex {&_Mutex} { - // Mask all signals, storing the original incoming mask - ::syscall(SYS_rt_sigprocmask, SIG_SETMASK, &Mask, &OriginalMask, sizeof(OriginalMask)); - - // Lock the mutex - (Mutex->*lock_fn)(); - } - - // No copy or assignment possible - ScopedSignalMaskWithMutexBase(const ScopedSignalMaskWithMutexBase&) = delete; - ScopedSignalMaskWithMutexBase& operator=(ScopedSignalMaskWithMutexBase&) = delete; - - // Only move - ScopedSignalMaskWithMutexBase(ScopedSignalMaskWithMutexBase &&rhs) - : OriginalMask {rhs.OriginalMask}, Mutex {rhs.Mutex} { - rhs.Mutex = nullptr; - } - - ~ScopedSignalMaskWithMutexBase() { - if (Mutex != nullptr) { - // Unlock the mutex - (Mutex->*unlock_fn)(); - - // Unmask back to the original signal mask - ::syscall(SYS_rt_sigprocmask, SIG_SETMASK, &OriginalMask, nullptr, sizeof(OriginalMask)); - } - } - private: - uint64_t OriginalMask{}; - MutexType *Mutex; - }; + // Signals are masked first, and then the lock is acquired + struct { + ScopedSignalMasker mask; + LockType lock; + } scope_guard { ScopedSignalMasker { Mask }, LockType { mutex } }; + return scope_guard; #else - // TODO: Doesn't block signals which may or may not cause issues. - template - class ScopedSignalMaskWithMutexBase final { - public: - - ScopedSignalMaskWithMutexBase(MutexType &_Mutex, [[maybe_unused]] uint64_t Mask = ~0ULL) - : Mutex {&_Mutex} { - // Lock the mutex - (Mutex->*lock_fn)(); - } - - // No copy or assignment possible - ScopedSignalMaskWithMutexBase(const ScopedSignalMaskWithMutexBase&) = delete; - ScopedSignalMaskWithMutexBase& operator=(ScopedSignalMaskWithMutexBase&) = delete; - - // Only move - ScopedSignalMaskWithMutexBase(ScopedSignalMaskWithMutexBase &&rhs) - : Mutex {rhs.Mutex} { - rhs.Mutex = nullptr; - } - - ~ScopedSignalMaskWithMutexBase() { - if (Mutex != nullptr) { - // Unlock the mutex - (Mutex->*unlock_fn)(); - } - } - private: - MutexType *Mutex; - }; - + // TODO: Doesn't block signals which may or may not cause issues. + return LockType { mutex }; #endif - - using ScopedSignalMaskWithMutex = ScopedSignalMaskWithMutexBase; - using ScopedSignalMaskWithSharedLock = ScopedSignalMaskWithMutexBase; - using ScopedSignalMaskWithUniqueLock = ScopedSignalMaskWithMutexBase; - - using ScopedSignalMaskWithForkableMutex = ScopedSignalMaskWithMutexBase< - FEXCore::ForkableUniqueMutex, - &FEXCore::ForkableUniqueMutex::lock, - &FEXCore::ForkableUniqueMutex::unlock>; - using ScopedSignalMaskWithForkableSharedLock = ScopedSignalMaskWithMutexBase< - FEXCore::ForkableSharedMutex, - &FEXCore::ForkableSharedMutex::lock_shared, - &FEXCore::ForkableSharedMutex::unlock_shared>; - using ScopedSignalMaskWithForkableUniqueLock = ScopedSignalMaskWithMutexBase< - FEXCore::ForkableSharedMutex, - &FEXCore::ForkableSharedMutex::lock, - &FEXCore::ForkableSharedMutex::unlock>; + } } diff --git a/Source/Tools/FEXLoader/LinuxSyscalls/SyscallsSMCTracking.cpp b/Source/Tools/FEXLoader/LinuxSyscalls/SyscallsSMCTracking.cpp index de0a57a22..b3b92dea4 100644 --- a/Source/Tools/FEXLoader/LinuxSyscalls/SyscallsSMCTracking.cpp +++ b/Source/Tools/FEXLoader/LinuxSyscalls/SyscallsSMCTracking.cpp @@ -55,7 +55,7 @@ bool SyscallHandler::HandleSegfault(FEXCore::Core::InternalThreadState *Thread, { // Can't use the deferred signal lock in the SIGSEGV handler. - FHU::ScopedSignalMaskWithForkableSharedLock lk(_SyscallHandler->VMATracking.Mutex); + auto lk = FHU::MaskSignalsAndLockMutex(_SyscallHandler->VMATracking.Mutex); auto VMATracking = &_SyscallHandler->VMATracking; @@ -112,7 +112,7 @@ void SyscallHandler::MarkGuestExecutableRange(FEXCore::Core::InternalThreadState return; } - FEXCore::ScopedDeferredSignalWithForkableSharedLock lk(VMATracking.Mutex, Thread); + auto lk = FEXCore::GuardSignalDeferringSection(VMATracking.Mutex, Thread); // Find the first mapping at or after the range ends, or ::end(). // Top points to the address after the end of the range @@ -167,7 +167,7 @@ void SyscallHandler::MarkGuestExecutableRange(FEXCore::Core::InternalThreadState // Used for AOT FEXCore::HLE::AOTIRCacheEntryLookupResult SyscallHandler::LookupAOTIRCacheEntry(FEXCore::Core::InternalThreadState *Thread, uint64_t GuestAddr) { - FEXCore::ScopedDeferredSignalWithForkableSharedLock lk(VMATracking.Mutex, Thread); + auto lk = FEXCore::GuardSignalDeferringSection(VMATracking.Mutex, Thread); // Get the first mapping after GuestAddr, or end // GuestAddr is inclusive @@ -194,8 +194,8 @@ void SyscallHandler::TrackMmap(FEXCore::Core::InternalThreadState *Thread, uintp { // NOTE: Frontend calls this with a nullptr Thread during initialization, but // providing this code with a valid Thread object earlier would allow - // us to be more optimal by using ScopedDeferredSignalWithUniqueLock instead - FEXCore::ScopedPotentialDeferredSignalWithForkableUniqueLock lk(VMATracking.Mutex, Thread); + // us to be more optimal by using GuardSignalDeferringSection instead + auto lk = FEXCore::GuardSignalDeferringSectionWithFallback(VMATracking.Mutex, Thread); static uint64_t AnonSharedId = 1; @@ -244,9 +244,9 @@ void SyscallHandler::TrackMunmap(FEXCore::Core::InternalThreadState *Thread, uin { // Frontend calls this with nullptr Thread during initialization. - // This is why `ScopedPotentialDeferredSignalWithUniqueLock` is used here. + // This is why `GuardSignalDeferringSectionWithFallback` is used here. // To be more optimal the frontend should provide this code with a valid Thread object earlier. - FEXCore::ScopedPotentialDeferredSignalWithForkableUniqueLock lk(VMATracking.Mutex, Thread); + auto lk = FEXCore::GuardSignalDeferringSectionWithFallback(VMATracking.Mutex, Thread); VMATracking.ClearUnsafe(CTX, Base, Size); } @@ -260,7 +260,7 @@ void SyscallHandler::TrackMprotect(FEXCore::Core::InternalThreadState *Thread, u Size = FEXCore::AlignUp(Size, FHU::FEX_PAGE_SIZE); { - FEXCore::ScopedDeferredSignalWithForkableUniqueLock lk(VMATracking.Mutex, Thread); + auto lk = FEXCore::GuardSignalDeferringSection(VMATracking.Mutex, Thread); VMATracking.ChangeUnsafe(Base, Size, VMAProt::fromProt(Prot)); } @@ -275,7 +275,7 @@ void SyscallHandler::TrackMremap(FEXCore::Core::InternalThreadState *Thread, uin NewSize = FEXCore::AlignUp(NewSize, FHU::FEX_PAGE_SIZE); { - FEXCore::ScopedDeferredSignalWithForkableUniqueLock lk(VMATracking.Mutex, Thread); + auto lk = FEXCore::GuardSignalDeferringSection(VMATracking.Mutex, Thread); const auto OldVMA = VMATracking.LookupVMAUnsafe(OldAddress); @@ -333,7 +333,7 @@ void SyscallHandler::TrackShmat(FEXCore::Core::InternalThreadState *Thread, int uint64_t Length = stat.shm_segsz; { - FEXCore::ScopedDeferredSignalWithForkableUniqueLock lk(VMATracking.Mutex, Thread); + auto lk = FEXCore::GuardSignalDeferringSection(VMATracking.Mutex, Thread); // TODO MRID mrid{SpecialDev::SHM, static_cast(shmid)}; @@ -355,7 +355,7 @@ void SyscallHandler::TrackShmat(FEXCore::Core::InternalThreadState *Thread, int void SyscallHandler::TrackShmdt(FEXCore::Core::InternalThreadState *Thread, uintptr_t Base) { uintptr_t Length = 0; { - FEXCore::ScopedDeferredSignalWithForkableUniqueLock lk(VMATracking.Mutex, Thread); + auto lk = FEXCore::GuardSignalDeferringSection(VMATracking.Mutex, Thread); Length = VMATracking.ClearShmUnsafe(CTX, Base); } @@ -369,7 +369,7 @@ void SyscallHandler::TrackShmdt(FEXCore::Core::InternalThreadState *Thread, uint void SyscallHandler::TrackMadvise(FEXCore::Core::InternalThreadState *Thread, uintptr_t Base, uintptr_t Size, int advice) { Size = FEXCore::AlignUp(Size, FHU::FEX_PAGE_SIZE); { - FEXCore::ScopedDeferredSignalWithForkableUniqueLock lk(VMATracking.Mutex, Thread); + auto lk = FEXCore::GuardSignalDeferringSection(VMATracking.Mutex, Thread); // TODO } } From 5ca35bf77cf3ece882dc9f6c9ee7ec369b30a7cf Mon Sep 17 00:00:00 2001 From: Tony Wasserka Date: Tue, 25 Jul 2023 10:40:51 +0200 Subject: [PATCH 2/4] ForkableMutex: Simplify WIN32 implementation --- .../FEXCore/Utils/DeferredSignalMutex.h | 55 +------------------ 1 file changed, 2 insertions(+), 53 deletions(-) diff --git a/FEXCore/include/FEXCore/Utils/DeferredSignalMutex.h b/FEXCore/include/FEXCore/Utils/DeferredSignalMutex.h index 3ffcc8fda..f250edefb 100644 --- a/FEXCore/include/FEXCore/Utils/DeferredSignalMutex.h +++ b/FEXCore/include/FEXCore/Utils/DeferredSignalMutex.h @@ -96,69 +96,18 @@ namespace FEXCore { }; #else // Windows doesn't support forking, so these can be standard mutexes. - class ForkableUniqueMutex final { + class ForkableUniqueMutex final : public std::mutex { public: - ForkableUniqueMutex() = default; - - // Non-moveable - ForkableUniqueMutex(const ForkableUniqueMutex&) = delete; - ForkableUniqueMutex& operator=(const ForkableUniqueMutex&) = delete; - ForkableUniqueMutex(ForkableUniqueMutex &&rhs) = delete; - ForkableUniqueMutex& operator=(ForkableUniqueMutex &&) = delete; - - void lock() { - Mutex.lock(); - } - void unlock() { - Mutex.unlock(); - } - // Initialize the internal pthread object to its default initializer state. - // Should only ever be used in the child process when a Linux fork() has occured. void StealAndDropActiveLocks() { LogMan::Msg::AFmt("{} is unsupported on WIN32 builds!", __func__); } - private: - std::mutex Mutex; }; - class ForkableSharedMutex final { + class ForkableSharedMutex final : public std::shared_mutex { public: - ForkableSharedMutex() = default; - - // Non-moveable - ForkableSharedMutex(const ForkableSharedMutex&) = delete; - ForkableSharedMutex& operator=(const ForkableSharedMutex&) = delete; - ForkableSharedMutex(ForkableSharedMutex &&rhs) = delete; - ForkableSharedMutex& operator=(ForkableSharedMutex &&) = delete; - - void lock() { - Mutex.lock(); - } - void unlock() { - Mutex.unlock(); - } - void lock_shared() { - Mutex.lock_shared(); - } - - void unlock_shared() { - Mutex.unlock_shared(); - } - - bool try_lock() { - return Mutex.try_lock(); - } - - bool try_lock_shared() { - return Mutex.try_lock_shared(); - } - // Initialize the internal pthread object to its default initializer state. - // Should only ever be used in the child process when a Linux fork() has occured. void StealAndDropActiveLocks() { LogMan::Msg::AFmt("{} is unsupported on WIN32 builds!", __func__); } - private: - std::shared_mutex Mutex; }; #endif From 92e4e752178e173a3dd58718210430e21041fe19 Mon Sep 17 00:00:00 2001 From: Tony Wasserka Date: Tue, 25 Jul 2023 10:40:52 +0200 Subject: [PATCH 3/4] Merge DeferredSignalMutex.h and ScopedSignalMask.h into a single file Using a single file makes sense now that the individual files are much shorter and share common utility classes. --- FEXCore/Source/Interface/Context/Context.h | 2 +- FEXCore/Source/Interface/Core/Core.cpp | 2 +- .../Source/Utils/Allocator/64BitAllocator.cpp | 2 +- ...erredSignalMutex.h => SignalScopeGuards.h} | 30 +++++++- .../FEXHeaderUtils/ScopedSignalMask.h | 69 ------------------- .../Tools/FEXLoader/LinuxSyscalls/Syscalls.h | 2 +- .../LinuxSyscalls/SyscallsSMCTracking.cpp | 5 +- 7 files changed, 35 insertions(+), 77 deletions(-) rename FEXCore/include/FEXCore/Utils/{DeferredSignalMutex.h => SignalScopeGuards.h} (85%) delete mode 100644 FEXHeaderUtils/FEXHeaderUtils/ScopedSignalMask.h diff --git a/FEXCore/Source/Interface/Context/Context.h b/FEXCore/Source/Interface/Context/Context.h index 3b3572fce..109f0dbe6 100644 --- a/FEXCore/Source/Interface/Context/Context.h +++ b/FEXCore/Source/Interface/Context/Context.h @@ -14,8 +14,8 @@ #include #include #include -#include #include +#include #include #include #include diff --git a/FEXCore/Source/Interface/Core/Core.cpp b/FEXCore/Source/Interface/Core/Core.cpp index 0cc0b0602..b3cce811f 100644 --- a/FEXCore/Source/Interface/Core/Core.cpp +++ b/FEXCore/Source/Interface/Core/Core.cpp @@ -9,7 +9,6 @@ $end_info$ */ #include -#include "FEXCore/Utils/DeferredSignalMutex.h" #include "Interface/Context/Context.h" #include "Interface/Core/LookupCache.h" #include "Interface/Core/CPUID.h" @@ -45,6 +44,7 @@ $end_info$ #include #include #include +#include "FEXCore/Utils/SignalScopeGuards.h" #include #include #include diff --git a/FEXCore/Source/Utils/Allocator/64BitAllocator.cpp b/FEXCore/Source/Utils/Allocator/64BitAllocator.cpp index 2116b45ce..9c2d27133 100644 --- a/FEXCore/Source/Utils/Allocator/64BitAllocator.cpp +++ b/FEXCore/Source/Utils/Allocator/64BitAllocator.cpp @@ -5,8 +5,8 @@ #include #include #include +#include #include -#include #include #include #include diff --git a/FEXCore/include/FEXCore/Utils/DeferredSignalMutex.h b/FEXCore/include/FEXCore/Utils/SignalScopeGuards.h similarity index 85% rename from FEXCore/include/FEXCore/Utils/DeferredSignalMutex.h rename to FEXCore/include/FEXCore/Utils/SignalScopeGuards.h index f250edefb..5e1e86c9b 100644 --- a/FEXCore/include/FEXCore/Utils/DeferredSignalMutex.h +++ b/FEXCore/include/FEXCore/Utils/SignalScopeGuards.h @@ -111,6 +111,7 @@ namespace FEXCore { }; #endif + // Helper class to manage deferred signal refcounting within a block scope class DeferredSignalRefCountGuard final { public: explicit DeferredSignalRefCountGuard(FEXCore::Core::InternalThreadState *Thread) : Thread(Thread) { @@ -148,7 +149,7 @@ namespace FEXCore { }; #ifndef _WIN32 - // TODO: Duplicated, unify with ScopedSignalMask + // Helper class to mask POSIX signals within a block scope class ScopedSignalMasker final { public: explicit ScopedSignalMasker(uint64_t Mask) : OriginalMask(0) { @@ -173,6 +174,33 @@ namespace FEXCore { }; #endif + /** + * @brief Produces a wrapper object around a scoped lock of the given mutex + * while ensuring POSIX signals are masked while the mutex is locked + * + * Use this to prevent reentrancy issues of C++ mutexes with certain signal handlers. + * Common examples of such issues are: + * - C++ mutexes not unlocking due to a signal handler calling longjmp from within a scope owning the mutex + * - The signal handler itself using a mutex that would be re-locked if the handler gets invoked + * again before unlocking + * + * Ownership of the returned object may be moved, but it is NOT SAFE to move across threads. + */ + template class LockType = std::unique_lock, typename MutexType> + [[nodiscard]] static auto MaskSignalsAndLockMutex(MutexType& mutex, uint64_t Mask = ~0ULL) { +#ifndef _WIN32 + // Signals are masked first, and then the lock is acquired + struct { + ScopedSignalMasker mask; + LockType lock; + } scope_guard { ScopedSignalMasker { Mask }, LockType { mutex } }; + return scope_guard; +#else + // TODO: Doesn't block signals which may or may not cause issues. + return LockType { mutex }; +#endif + } + /** * @brief Produces a wrapper object around a scoped lock of the given mutex * while bumping the Thread's deferred signal refcount while the mutex is diff --git a/FEXHeaderUtils/FEXHeaderUtils/ScopedSignalMask.h b/FEXHeaderUtils/FEXHeaderUtils/ScopedSignalMask.h deleted file mode 100644 index 74f66bd36..000000000 --- a/FEXHeaderUtils/FEXHeaderUtils/ScopedSignalMask.h +++ /dev/null @@ -1,69 +0,0 @@ -// SPDX-License-Identifier: MIT -#pragma once - -#include -#include -#include -#include -#ifndef _WIN32 -#include -#include -#endif -#include - -namespace FHU { -#ifndef _WIN32 - /** - * Masks POSIX signals for the scope the object is active in - */ - class ScopedSignalMasker final { - public: - explicit ScopedSignalMasker(uint64_t Mask) : OriginalMask(0) { - // Mask all signals, storing the original incoming mask - ::syscall(SYS_rt_sigprocmask, SIG_SETMASK, &Mask, &*OriginalMask, sizeof(*OriginalMask)); - } - - // Move-only type - ScopedSignalMasker(const ScopedSignalMasker&) = delete; - ScopedSignalMasker& operator=(ScopedSignalMasker&) = delete; - ScopedSignalMasker(ScopedSignalMasker&& rhs) : OriginalMask(rhs.OriginalMask) { - rhs.OriginalMask.reset(); - } - - ~ScopedSignalMasker() { - if (OriginalMask) { - ::syscall(SYS_rt_sigprocmask, SIG_SETMASK, &OriginalMask, nullptr, sizeof(*OriginalMask)); - } - } - private: - std::optional OriginalMask{}; - }; -#endif - - /** - * @brief Produces a wrapper object around a scoped lock of the given mutex - * while ensuring POSIX signals are masked while the mutex is locked - * - * Use this to prevent reentrancy issues of C++ mutexes with certain signal handlers. - * Common examples of such issues are: - * - C++ mutexes not unlocking due to a signal handler calling longjmp from within a scope owning the mutex - * - The signal handler itself using a mutex that would be re-locked if the handler gets invoked - * again before unlocking - * - * Ownership of the returned object may be moved, but it is NOT SAFE to move across threads. - */ - template class LockType = std::unique_lock, typename MutexType> - [[nodiscard]] static auto MaskSignalsAndLockMutex(MutexType& mutex, uint64_t Mask = ~0ULL) { -#ifndef _WIN32 - // Signals are masked first, and then the lock is acquired - struct { - ScopedSignalMasker mask; - LockType lock; - } scope_guard { ScopedSignalMasker { Mask }, LockType { mutex } }; - return scope_guard; -#else - // TODO: Doesn't block signals which may or may not cause issues. - return LockType { mutex }; -#endif - } -} diff --git a/Source/Tools/FEXLoader/LinuxSyscalls/Syscalls.h b/Source/Tools/FEXLoader/LinuxSyscalls/Syscalls.h index a641b8a4d..3da407147 100644 --- a/Source/Tools/FEXLoader/LinuxSyscalls/Syscalls.h +++ b/Source/Tools/FEXLoader/LinuxSyscalls/Syscalls.h @@ -16,7 +16,7 @@ $end_info$ #include #include #include -#include +#include #include #include #include diff --git a/Source/Tools/FEXLoader/LinuxSyscalls/SyscallsSMCTracking.cpp b/Source/Tools/FEXLoader/LinuxSyscalls/SyscallsSMCTracking.cpp index b3b92dea4..e968f3987 100644 --- a/Source/Tools/FEXLoader/LinuxSyscalls/SyscallsSMCTracking.cpp +++ b/Source/Tools/FEXLoader/LinuxSyscalls/SyscallsSMCTracking.cpp @@ -16,11 +16,10 @@ $end_info$ #include "LinuxSyscalls/Syscalls.h" #include -#include #include #include #include -#include +#include namespace FEX::HLE { @@ -55,7 +54,7 @@ bool SyscallHandler::HandleSegfault(FEXCore::Core::InternalThreadState *Thread, { // Can't use the deferred signal lock in the SIGSEGV handler. - auto lk = FHU::MaskSignalsAndLockMutex(_SyscallHandler->VMATracking.Mutex); + auto lk = FEXCore::MaskSignalsAndLockMutex(_SyscallHandler->VMATracking.Mutex); auto VMATracking = &_SyscallHandler->VMATracking; From d33b0cb9e36db1139b80630bec9977e9bf380326 Mon Sep 17 00:00:00 2001 From: Tony Wasserka Date: Sat, 18 Nov 2023 11:34:40 +0100 Subject: [PATCH 4/4] SignalScopeGuards: Improve code gen for GuardSignalDeferringSectionWithFallback --- .../include/FEXCore/Utils/SignalScopeGuards.h | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/FEXCore/include/FEXCore/Utils/SignalScopeGuards.h b/FEXCore/include/FEXCore/Utils/SignalScopeGuards.h index 5e1e86c9b..cf6c63212 100644 --- a/FEXCore/include/FEXCore/Utils/SignalScopeGuards.h +++ b/FEXCore/include/FEXCore/Utils/SignalScopeGuards.h @@ -12,6 +12,7 @@ #include #endif #include +#include namespace FEXCore { #ifndef _WIN32 @@ -219,20 +220,23 @@ namespace FEXCore { // Like GuardSignalDeferringSection but falls back to masking signals when Thread is nullptr template class LockType = std::unique_lock, typename MutexType> [[nodiscard]] static auto GuardSignalDeferringSectionWithFallback(MutexType& mutex, FEXCore::Core::InternalThreadState *Thread, uint64_t Mask = ~0ULL) { +#ifndef _WIN32 + using ExtraGuard = std::variant; +#else + using ExtraGuard = std::variant; +#endif + struct { - std::optional refcount; -#ifndef _WIN32 - std::optional mask; -#endif + ExtraGuard refcount_or_mask; LockType lock; - } scope_guard; - if (Thread) { - scope_guard.refcount.emplace(Thread); - } else { + } scope_guard { + Thread ? ExtraGuard { DeferredSignalRefCountGuard { Thread } } #ifndef _WIN32 - scope_guard.mask.emplace(Mask); + : ExtraGuard { ScopedSignalMasker { Mask } } +#else + : ExtraGuard { } #endif - } + }; scope_guard.lock = LockType { mutex }; return scope_guard; }