Improve error checking in mutex stub (#3167)

* Improve error checking in mutex stub

The default mutex stub is intended to be usable on any platform,
even platforms that do not have threads and cannot block. This stub is
inappropriate for applications that need to contend over mutexes.
However, if inadvertently used in applications in a way that would
result in mutex contention, they would silently allow incorrect and
dangerous behavior. (Notably, they allowed multiple threads to enter the
same critical section.)

The new implementation still works on all platforms, and never blocks.
However, it ensures that only one thread enters each critical section at
a time. Attempting to acquire a mutex that is already taken will result
in a failure to acquire the mutex, and likely an assertion. Because this
should only occur in the case of a coding defect, this is an improvement
over the previous implementation.

* Note mutex identity in lock/unlock status assert

When a mutex fails to be taken or released, and it causes an assertion
to trip, this change makes sure that enough information is provided to
uniquely identify which mutex was at fault.

* Fix assertion cast argument types to be generic

Os/Mutex.cpp

@@ -34,13 +34,13 @@ Mutex::Status Mutex::release() {
 void Mutex::lock() {
     FW_ASSERT(&this->m_delegate == reinterpret_cast<MutexInterface*>(&this->m_handle_storage[0]));
     Mutex::Status status = this->take();
-    FW_ASSERT(status == Mutex::Status::OP_OK, status);
+    FW_ASSERT(status == Mutex::Status::OP_OK, static_cast<FwAssertArgType>(reinterpret_cast<PlatformPointerCastType>(this)), status);
 }
 
 void Mutex::unLock() {
     FW_ASSERT(&this->m_delegate == reinterpret_cast<MutexInterface*>(&this->m_handle_storage[0]));
     Mutex::Status status = this->release();
-    FW_ASSERT(status == Mutex::Status::OP_OK, status);
+    FW_ASSERT(status == Mutex::Status::OP_OK, static_cast<FwAssertArgType>(reinterpret_cast<PlatformPointerCastType>(this)), status);
 }
 
 ScopeLock::ScopeLock(Mutex& mutex) : m_mutex(mutex) {

Os/Stub/Mutex.cpp

@@ -9,10 +9,25 @@ namespace Stub {
 namespace Mutex {
 
     StubMutex::Status StubMutex::take() {
+        // Attempt to mark the mutex as taken.
+        if (this->m_handle.m_mutex_taken.exchange(true)) {
+            // The mutex was already taken, so fail the operation.
+            // (This stub is for platforms without the ability to block.)
+            return Status::ERROR_BUSY;
+        }
+        // The mutex was not already taken.
+        // Now that it has been marked as taken, we have successfully entered the critical section.
         return Status::OP_OK;
     }
 
     StubMutex::Status StubMutex::release() {
+        // Attempt to mark the mutex as not taken.
+        if (!this->m_handle.m_mutex_taken.exchange(false)) {
+            // The mutex was already not taken, which indicates a coding defect.
+            return Status::ERROR_OTHER;
+        }
+        // The mutex was taken.
+        // Now that it has been marked as not taken, we have successfully exited the critical section.
         return Status::OP_OK;
     }
 

Os/Stub/Mutex.hpp

@@ -4,17 +4,27 @@
 // ======================================================================
 #include "Os/Mutex.hpp"
 
+#include <atomic>
+
 #ifndef OS_STUB_MUTEX_HPP
 #define OS_STUB_MUTEX_HPP
 namespace Os {
 namespace Stub {
 namespace Mutex {
 
-struct StubMutexHandle : public MutexHandle {};
+struct StubMutexHandle : public MutexHandle {
+    //! True if the mutex has been acquired without being released.
+    std::atomic<bool> m_mutex_taken = {false};
+};
 
-//! \brief stub implementation of Os::Mutex
+//! \brief Nonblocking stub implementation of Os::Mutex
+//!
+//! Stub implementation of `MutexInterface` for use as a delegate class.
 //!
-//! Stub implementation of `MutexInterface` for use as a delegate class handling error-only file operations.
+//! This mutex will never block, which allows it to be used on any platform without OS dependencies.
+//! It is unsuitable for use in environments where threads need to contend over mutexes.
+//! However, it is appropriate for use in environments with multiple threads that are not intended to
+//! contend over mutexes, and where contention would indicate a coding defect worthy of an assertion.
 //!
 class StubMutex : public MutexInterface {
   public: