From d92bb16d0c3ea10d659b9dbce668696e8a283599 Mon Sep 17 00:00:00 2001 From: Heather Anderson Date: Wed, 17 Jun 2020 00:58:57 -0700 Subject: [PATCH] replacing the use of std::mutex with a spinlock, which we can try to lock but exit if the attempt times out. --- interface/src/CrashHandler_Crashpad.cpp | 119 +++++++++++++++++++++--- 1 file changed, 104 insertions(+), 15 deletions(-) diff --git a/interface/src/CrashHandler_Crashpad.cpp b/interface/src/CrashHandler_Crashpad.cpp index 2ddcd3f398..b11a3e119e 100644 --- a/interface/src/CrashHandler_Crashpad.cpp +++ b/interface/src/CrashHandler_Crashpad.cpp @@ -15,16 +15,15 @@ #include -#include #include #include -#include +#include +#include #include #include #include - #if defined(__clang__) #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wc++14-extensions" @@ -33,7 +32,6 @@ #include #include #include -#include #include #if defined(__clang__) @@ -44,16 +42,96 @@ #include #include -using namespace crashpad; - static const std::string BACKTRACE_URL { CMAKE_BACKTRACE_URL }; static const std::string BACKTRACE_TOKEN { CMAKE_BACKTRACE_TOKEN }; -static constexpr DWORD STATUS_MSVC_CPP_EXCEPTION = 0xE06D7363; -static constexpr ULONG_PTR MSVC_CPP_EXCEPTION_SIGNATURE = 0x19930520; +// ------------------------------------------------------------------------------------------------ +// SpinLock - a lock that can timeout attempting to lock a block of code, and is in a busy-wait cycle while trying to acquire +// note that this code will malfunction if you attempt to grab a lock while already holding it -CrashpadClient* client { nullptr }; -std::mutex annotationMutex; +class SpinLock { +public: + SpinLock(); + void lock(); + bool lock(int msecs); + void unlock(); +private: + QAtomicInteger _lock{ 0 }; + + Q_DISABLE_COPY(SpinLock) +}; + +class SpinLockLocker { +public: + SpinLockLocker(SpinLock& lock, int msecs = -1); + ~SpinLockLocker(); + bool isLocked() const; + void unlock(); + bool relock(int msecs = -1); + +private: + SpinLock* _lock; + bool _isLocked; + + Q_DISABLE_COPY(SpinLockLocker) +}; + +SpinLock::SpinLock() { +} + +void SpinLock::lock() { + while (!_lock.testAndSetAcquire(0, 1)) + ; +} + +bool SpinLock::lock(int msecs) { + QDeadlineTimer deadline(msecs); + for (;;) { + if (_lock.testAndSetAcquire(0, 1)) { + return true; + } + if (deadline.hasExpired()) { + return false; + } + } +} + +void SpinLock::unlock() { + _lock.storeRelease(0); +} + +SpinLockLocker::SpinLockLocker(SpinLock& lock, int msecs /* = -1 */ ) : _lock(&lock) { + _isLocked = _lock->lock(msecs); +} + +SpinLockLocker::~SpinLockLocker() { + if (_isLocked) { + _lock->unlock(); + } +} + +bool SpinLockLocker::isLocked() const { + return _isLocked; +} + +void SpinLockLocker::unlock() { + if (_isLocked) { + _lock->unlock(); + _isLocked = false; + } +} + +bool SpinLockLocker::relock(int msecs /* = -1 */ ) { + if (!_isLocked) { + _isLocked = _lock->lock(msecs); + } + return _isLocked; +} + +// ------------------------------------------------------------------------------------------------ + +crashpad::CrashpadClient* client{ nullptr }; +SpinLock crashpadAnnotationsProtect; crashpad::SimpleStringDictionary* crashpadAnnotations { nullptr }; #if defined(Q_OS_WIN) @@ -63,12 +141,18 @@ static const QString CRASHPAD_HANDLER_NAME { "crashpad_handler" }; #endif #ifdef Q_OS_WIN +// ------------------------------------------------------------------------------------------------ +// The area within this #ifdef is specific to the Microsoft C++ compiler + +static constexpr DWORD STATUS_MSVC_CPP_EXCEPTION = 0xE06D7363; +static constexpr ULONG_PTR MSVC_CPP_EXCEPTION_SIGNATURE = 0x19930520; +static constexpr int ANNOTATION_LOCK_WEAK_ATTEMPT = 5000; // attempt to lock the annotations list, but give up if it takes more than 5 seconds + #include #include void fatalCxxException(PEXCEPTION_POINTERS pExceptionInfo); - LONG WINAPI vectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo) { if (!client) { return EXCEPTION_CONTINUE_SEARCH; @@ -123,6 +207,11 @@ struct CatchableTypeArray_internal { // the underlying C++ exception type (or at least its name) before throwing the whole // mess at crashpad void fatalCxxException(PEXCEPTION_POINTERS pExceptionInfo) { + SpinLockLocker guard(crashpadAnnotationsProtect, ANNOTATION_LOCK_WEAK_ATTEMPT); + if (!guard.isLocked()) { + return; + } + PEXCEPTION_RECORD ExceptionRecord = pExceptionInfo->ExceptionRecord; /* Exception arguments for Microsoft C++ exceptions: @@ -160,8 +249,6 @@ void fatalCxxException(PEXCEPTION_POINTERS pExceptionInfo) { } const std::type_info* type = reinterpret_cast(moduleBase + pCatchableType->pType); - // we're crashing, not really sure it matters who's currently holding the lock (although this could in theory really mess things up - annotationMutex.try_lock(); crashpadAnnotations->SetKeyValue("thrownObject", type->name()); // After annotating the name of the actual object type, go through the other entries in CatcahleTypeArray and itemize the list of possible @@ -182,6 +269,8 @@ void fatalCxxException(PEXCEPTION_POINTERS pExceptionInfo) { crashpadAnnotations->SetKeyValue("thrownObjectLike", compatibleObjects.toStdString()); } +// End of code specific to the Microsoft C++ compiler +// ------------------------------------------------------------------------------------------------ #endif bool startCrashHandler(std::string appPath) { @@ -190,7 +279,7 @@ bool startCrashHandler(std::string appPath) { } assert(!client); - client = new CrashpadClient(); + client = new crashpad::CrashpadClient(); std::vector arguments; std::map annotations; @@ -243,7 +332,7 @@ bool startCrashHandler(std::string appPath) { void setCrashAnnotation(std::string name, std::string value) { if (client) { - std::lock_guard guard(annotationMutex); + SpinLockLocker guard(crashpadAnnotationsProtect); if (!crashpadAnnotations) { crashpadAnnotations = new crashpad::SimpleStringDictionary(); // don't free this, let it leak crashpad::CrashpadInfo* crashpad_info = crashpad::CrashpadInfo::GetCrashpadInfo();