Change the Mutex::Locker class so that it takes the Mutex object and locks it, rather

than being given the pthread_mutex_t from the Mutex and locks that.  That allows us to
track ownership of the Mutex better.  

Used this to switch the LLDB_CONFIGURATION_DEBUG enabled assert when we can't get the
gdb-remote sequence mutex to assert when the thread that had the mutex releases it.  This
is generally more useful information than saying just who failed to get it (since the
code that had it locked often had released it by the time the assert fired.)

llvm-svn: 158240

7 files changed, 127 insertions, 119 deletions

diff --git a/lldb/include/lldb/Host/Mutex.h b/lldb/include/lldb/Host/Mutex.h
index b848c151285..38f4f315507 100644
--- a/lldb/include/lldb/Host/Mutex.h
+++ b/lldb/include/lldb/Host/Mutex.h
@@ -14,6 +14,10 @@
 #include <pthread.h>
 #include <assert.h>
 
+#ifdef LLDB_CONFIGURATION_DEBUG
+#include <string>
+#endif
+
 namespace lldb_private {
 
 //----------------------------------------------------------------------
@@ -121,13 +125,13 @@ public:
         ///     returns \b false otherwise.
         //--------------------------------------------------------------
         bool
-        TryLock (Mutex &mutex);
+        TryLock (Mutex &mutex, const char *failure_message = NULL);
         
         bool
-        TryLock (Mutex *mutex)
+        TryLock (Mutex *mutex, const char *failure_message = NULL)
         {
             if (mutex)
-                return TryLock(*mutex);
+                return TryLock(*mutex, failure_message);
             else
                 return false;
         }
@@ -139,9 +143,7 @@ public:
         //--------------------------------------------------------------
         /// Member variables
         //--------------------------------------------------------------
-        pthread_mutex_t *m_mutex_ptr;   ///< A pthread mutex that is locked when
-                                        ///< acquired and unlocked when destroyed
-                                        ///< or reset.
+        Mutex *m_mutex_ptr;
 
     private:
         Locker(const Locker&);
@@ -176,6 +178,9 @@ public:
     ///
     /// Destroys the mutex owned by this object.
     //------------------------------------------------------------------
+#ifdef LLDB_CONFIGURATION_DEBUG
+    virtual
+#endif
     ~Mutex();
 
     //------------------------------------------------------------------
@@ -201,8 +206,11 @@ public:
     /// @return
     ///     The error code from \c pthread_mutex_trylock().
     //------------------------------------------------------------------
+#ifdef LLDB_CONFIGURATION_DEBUG
+    virtual
+#endif
     int
-    TryLock();
+    TryLock(const char *failure_message = NULL);
 
     //------------------------------------------------------------------
     /// Unlock the mutex.
@@ -215,22 +223,18 @@ public:
     /// @return
     ///     The error code from \c pthread_mutex_unlock().
     //------------------------------------------------------------------
+#ifdef LLDB_CONFIGURATION_DEBUG
+    virtual
+#endif
     int
     Unlock();
 
-    static
-    int Lock (pthread_mutex_t *mutex_ptr);
-
-    static
-    int TryLock (pthread_mutex_t *mutex_ptr);
-
-    static
-    int Unlock (pthread_mutex_t *mutex_ptr);
-
 protected:
     //------------------------------------------------------------------
     // Member variables
     //------------------------------------------------------------------
+    // TODO: Hide the mutex in the implementation file in case we ever need to port to an
+    // architecture that doesn't have pthread mutexes.
     pthread_mutex_t m_mutex; ///< The pthread mutex object.
 
 private:
@@ -247,6 +251,37 @@ private:
     const Mutex& operator=(const Mutex&);
 };
 
+#ifdef LLDB_CONFIGURATION_DEBUG
+class TrackingMutex : public Mutex
+{
+public:
+    TrackingMutex() : Mutex()  {}
+    TrackingMutex(Mutex::Type type) : Mutex (type) {}
+    
+    virtual
+    ~TrackingMutex() {}
+    
+    virtual int
+    Unlock ();
+
+    virtual int
+    TryLock (const char *failure_message = NULL)
+    {
+        int return_value = Mutex::TryLock();
+        if (return_value != 0 && failure_message != NULL)
+        {
+            m_failure_message.assign(failure_message);
+            m_thread_that_tried = pthread_self();
+        }
+        return return_value;
+    }
+    
+protected:
+    pthread_t m_thread_that_tried;
+    std::string m_failure_message;
+};
+#endif
+
 } // namespace lldb_private
 
 #endif  // #if defined(__cplusplus)
diff --git a/lldb/source/Host/common/Mutex.cpp b/lldb/source/Host/common/Mutex.cpp
index d519baf45b6..e8091bd3e0c 100644
--- a/lldb/source/Host/common/Mutex.cpp
+++ b/lldb/source/Host/common/Mutex.cpp
@@ -141,19 +141,14 @@ Mutex::Locker::~Locker ()
 void
 Mutex::Locker::Lock (Mutex &mutex)
 {
-    pthread_mutex_t *mutex_ptr = mutex.GetMutex();
-
     // We already have this mutex locked or both are NULL...
-    if (m_mutex_ptr == mutex_ptr)
+    if (m_mutex_ptr == &mutex)
         return;
 
     Unlock ();
 
-    if (mutex_ptr)
-    {
-        m_mutex_ptr = mutex_ptr;
-        Mutex::Lock (m_mutex_ptr);
-    }
+    m_mutex_ptr = &mutex;
+    m_mutex_ptr->Lock();
 }
 
 void
@@ -161,27 +156,23 @@ Mutex::Locker::Unlock ()
 {
     if (m_mutex_ptr)
     {
-        Mutex::Unlock (m_mutex_ptr);
+        m_mutex_ptr->Unlock ();
         m_mutex_ptr = NULL;
     }
 }
 
 bool
-Mutex::Locker::TryLock (Mutex &mutex)
+Mutex::Locker::TryLock (Mutex &mutex, const char *failure_message)
 {
-    pthread_mutex_t *mutex_ptr = mutex.GetMutex();
-    
     // We already have this mutex locked!
-    if (m_mutex_ptr == mutex_ptr)
-        return m_mutex_ptr != NULL;
+    if (m_mutex_ptr == &mutex)
+        return true;
 
     Unlock ();
 
-    if (mutex_ptr)
-    {
-        if (Mutex::TryLock (mutex_ptr) == 0)
-            m_mutex_ptr = mutex_ptr;
-    }
+    if (mutex.TryLock(failure_message) == 0)
+        m_mutex_ptr = &mutex;
+
     return m_mutex_ptr != NULL;
 }
 
@@ -273,100 +264,98 @@ Mutex::GetMutex()
     return &m_mutex;
 }
 
+//----------------------------------------------------------------------
+// Locks the mutex owned by this object, if the mutex is already
+// locked, the calling thread will block until the mutex becomes
+// available.
+//
+// RETURNS
+//  The error code from the pthread_mutex_lock() function call.
+//----------------------------------------------------------------------
 int
-Mutex::Lock (pthread_mutex_t *mutex_ptr)
+Mutex::Lock()
 {
     DEBUG_LOG ("[%4.4x/%4.4x] pthread_mutex_lock (%p)...\n", Host::GetCurrentProcessID(), Host::GetCurrentThreadID(), mutex_ptr);
 
 #if ENABLE_MUTEX_ERROR_CHECKING
-    error_check_mutex (mutex_ptr, eMutexActionAssertInitialized);
+    error_check_mutex (&m_mutex, eMutexActionAssertInitialized);
 #endif
 
-    int err = ::pthread_mutex_lock (mutex_ptr);
+    int err = ::pthread_mutex_lock (&m_mutex);
     
 
 #if ENABLE_MUTEX_ERROR_CHECKING
     if (err)
     {
-        Host::SetCrashDescriptionWithFormat ("%s error: pthread_mutex_lock(%p) => err = %i (%s)", __PRETTY_FUNCTION__, mutex_ptr, err, strerror(err));
+        Host::SetCrashDescriptionWithFormat ("%s error: pthread_mutex_lock(%p) => err = %i (%s)", __PRETTY_FUNCTION__, &m_mutex, err, strerror(err));
         assert(err == 0);
     }
 #endif
-    DEBUG_LOG ("[%4.4x/%4.4x] pthread_mutex_lock (%p) => %i\n", Host::GetCurrentProcessID(), Host::GetCurrentThreadID(), mutex_ptr, err);
+    DEBUG_LOG ("[%4.4x/%4.4x] pthread_mutex_lock (%p) => %i\n", Host::GetCurrentProcessID(), Host::GetCurrentThreadID(), &m_mutex, err);
     return err;
 }
 
+//----------------------------------------------------------------------
+// Attempts to lock the mutex owned by this object without blocking.
+// If the mutex is already locked, TryLock() will not block waiting
+// for the mutex, but will return an error condition.
+//
+// RETURNS
+//  The error code from the pthread_mutex_trylock() function call.
+//----------------------------------------------------------------------
 int
-Mutex::TryLock (pthread_mutex_t *mutex_ptr)
+Mutex::TryLock(const char *failure_message)
 {
 #if ENABLE_MUTEX_ERROR_CHECKING
-    error_check_mutex (mutex_ptr, eMutexActionAssertInitialized);
+    error_check_mutex (&m_mutex, eMutexActionAssertInitialized);
 #endif
 
-    int err = ::pthread_mutex_trylock (mutex_ptr);
-    DEBUG_LOG ("[%4.4x/%4.4x] pthread_mutex_trylock (%p) => %i\n", Host::GetCurrentProcessID(), Host::GetCurrentThreadID(), mutex_ptr, err);
+    int err = ::pthread_mutex_trylock (&m_mutex);
+    DEBUG_LOG ("[%4.4x/%4.4x] pthread_mutex_trylock (%p) => %i\n", Host::GetCurrentProcessID(), Host::GetCurrentThreadID(), &m_mutex, err);
     return err;
 }
 
+//----------------------------------------------------------------------
+// If the current thread holds the lock on the owned mutex, then
+// Unlock() will unlock the mutex. Calling Unlock() on this object
+// that the calling thread does not hold will result in undefined
+// behavior.
+//
+// RETURNS
+//  The error code from the pthread_mutex_unlock() function call.
+//----------------------------------------------------------------------
 int
-Mutex::Unlock (pthread_mutex_t *mutex_ptr)
+Mutex::Unlock()
 {
 #if ENABLE_MUTEX_ERROR_CHECKING
-    error_check_mutex (mutex_ptr, eMutexActionAssertInitialized);
+    error_check_mutex (&m_mutex, eMutexActionAssertInitialized);
 #endif
 
-    int err = ::pthread_mutex_unlock (mutex_ptr);
+    int err = ::pthread_mutex_unlock (&m_mutex);
 
 #if ENABLE_MUTEX_ERROR_CHECKING
     if (err)
     {
-        Host::SetCrashDescriptionWithFormat ("%s error: pthread_mutex_unlock(%p) => err = %i (%s)", __PRETTY_FUNCTION__, mutex_ptr, err, strerror(err));
+        Host::SetCrashDescriptionWithFormat ("%s error: pthread_mutex_unlock(%p) => err = %i (%s)", __PRETTY_FUNCTION__, &m_mutex, err, strerror(err));
         assert(err == 0);
     }
 #endif
-    DEBUG_LOG ("[%4.4x/%4.4x] pthread_mutex_unlock (%p) => %i\n", Host::GetCurrentProcessID(), Host::GetCurrentThreadID(), mutex_ptr, err);
+    DEBUG_LOG ("[%4.4x/%4.4x] pthread_mutex_unlock (%p) => %i\n", Host::GetCurrentProcessID(), Host::GetCurrentThreadID(), &m_mutex, err);
     return err;
 }
 
-//----------------------------------------------------------------------
-// Locks the mutex owned by this object, if the mutex is already
-// locked, the calling thread will block until the mutex becomes
-// available.
-//
-// RETURNS
-//  The error code from the pthread_mutex_lock() function call.
-//----------------------------------------------------------------------
-int
-Mutex::Lock()
-{
-    return Mutex::Lock (&m_mutex);
-}
-
-//----------------------------------------------------------------------
-// Attempts to lock the mutex owned by this object without blocking.
-// If the mutex is already locked, TryLock() will not block waiting
-// for the mutex, but will return an error condition.
-//
-// RETURNS
-//  The error code from the pthread_mutex_trylock() function call.
-//----------------------------------------------------------------------
+#ifdef LLDB_CONFIGURATION_DEBUG
 int
-Mutex::TryLock()
+TrackingMutex::Unlock ()
 {
-    return Mutex::TryLock (&m_mutex);
+    if (!m_failure_message.empty())
+        Host::SetCrashDescriptionWithFormat ("Unlocking lock (on thread %p) that thread: %p failed to get: %s",
+                                             pthread_self(),
+                                             m_thread_that_tried,
+                                             m_failure_message.c_str());
+    assert (m_failure_message.empty());
+    return Mutex::Unlock();
 }
+#endif
+    
 
-//----------------------------------------------------------------------
-// If the current thread holds the lock on the owned mutex, then
-// Unlock() will unlock the mutex. Calling Unlock() on this object
-// that the calling thread does not hold will result in undefined
-// behavior.
-//
-// RETURNS
-//  The error code from the pthread_mutex_unlock() function call.
-//----------------------------------------------------------------------
-int
-Mutex::Unlock()
-{
-    return Mutex::Unlock (&m_mutex);
-}
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index fc83724fb25..0712ed4e1f7 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -263,10 +263,10 @@ GDBRemoteCommunication::GetAck ()
 }
 
 bool
-GDBRemoteCommunication::GetSequenceMutex (Mutex::Locker& locker)
+GDBRemoteCommunication::GetSequenceMutex (Mutex::Locker& locker, const char *failure_message)
 {
     if (IsRunning())
-        return locker.TryLock (m_sequence_mutex);
+        return locker.TryLock (m_sequence_mutex, failure_message);
 
     locker.Lock (m_sequence_mutex);
     return true;
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
index b3bad7ede5c..a1077957c6a 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
@@ -59,7 +59,7 @@ public:
                         size_t payload_length);
 
     bool
-    GetSequenceMutex (lldb_private::Mutex::Locker& locker);
+    GetSequenceMutex (lldb_private::Mutex::Locker& locker, const char *failure_message = NULL);
 
     bool
     CheckForPacket (const uint8_t *src, 
@@ -242,7 +242,11 @@ protected:
     // Classes that inherit from GDBRemoteCommunication can see and modify these
     //------------------------------------------------------------------
     uint32_t m_packet_timeout;
+#ifdef LLDB_CONFIGURATION_DEBUG
+    lldb_private::TrackingMutex m_sequence_mutex;
+#else
     lldb_private::Mutex m_sequence_mutex;    // Restrict access to sending/receiving packets to a single thread at a time
+#endif
     lldb_private::Predicate<bool> m_public_is_running;
     lldb_private::Predicate<bool> m_private_is_running;
     History m_history;
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 90fb47eb7a3..b08350a5ec8 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -1939,7 +1939,7 @@ GDBRemoteCommunicationClient::GetCurrentThreadIDs (std::vector<lldb::tid_t> &thr
     Mutex::Locker locker;
     thread_ids.clear();
     
-    if (GetSequenceMutex (locker))
+    if (GetSequenceMutex (locker, "ProcessGDBRemote::UpdateThreadList() failed due to not getting the sequence mutex"))
     {
         sequence_mutex_unavailable = false;
         StringExtractorGDBRemote response;
@@ -1968,9 +1968,13 @@ GDBRemoteCommunicationClient::GetCurrentThreadIDs (std::vector<lldb::tid_t> &thr
     }
     else
     {
+#if defined (LLDB_CONFIGURATION_DEBUG)
+        // assert(!"ProcessGDBRemote::UpdateThreadList() failed due to not getting the sequence mutex");
+#else
         LogSP log (ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet (GDBR_LOG_PROCESS | GDBR_LOG_PACKETS));
         if (log)
             log->Printf("error: failed to get packet sequence mutex, not sending packet 'qfThreadInfo'");
+#endif
         sequence_mutex_unavailable = true;
     }
     return thread_ids.size();
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
index 9eec09d0b5c..4247add8554 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
@@ -183,7 +183,7 @@ GDBRemoteRegisterContext::ReadRegisterBytes (const RegisterInfo *reg_info, DataE
     if (!m_reg_valid[reg])
     {
         Mutex::Locker locker;
-        if (gdb_comm.GetSequenceMutex (locker))
+        if (gdb_comm.GetSequenceMutex (locker, "Didn't get sequence mutex for read register."))
         {
             const bool thread_suffix_supported = gdb_comm.GetThreadSuffixSupported();
             ProcessSP process_sp (m_thread.GetProcess());
@@ -357,7 +357,7 @@ GDBRemoteRegisterContext::WriteRegisterBytes (const lldb_private::RegisterInfo *
                                   m_reg_data.GetByteOrder()))   // dst byte order
     {
         Mutex::Locker locker;
-        if (gdb_comm.GetSequenceMutex (locker))
+        if (gdb_comm.GetSequenceMutex (locker, "Didn't get sequence mutex for write register."))
         {
             const bool thread_suffix_supported = gdb_comm.GetThreadSuffixSupported();
             ProcessSP process_sp (m_thread.GetProcess());
@@ -445,12 +445,6 @@ GDBRemoteRegisterContext::WriteRegisterBytes (const lldb_private::RegisterInfo *
         else
         {
             LogSP log (ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet (GDBR_LOG_THREAD | GDBR_LOG_PACKETS));
-#if LLDB_CONFIGURATION_DEBUG
-            StreamString strm;
-            gdb_comm.DumpHistory(strm);
-            Host::SetCrashDescription (strm.GetData());
-            assert (!"Didn't get sequence mutex for write register.");
-#else
             if (log)
             {
                 if (log->GetVerbose())
@@ -462,7 +456,6 @@ GDBRemoteRegisterContext::WriteRegisterBytes (const lldb_private::RegisterInfo *
                 else
                     log->Printf("error: failed to get packet sequence mutex, not sending write register for \"%s\"", reg_info->name);
             }
-#endif
         }
     }
     return false;
@@ -484,7 +477,7 @@ GDBRemoteRegisterContext::ReadAllRegisterValues (lldb::DataBufferSP &data_sp)
     StringExtractorGDBRemote response;
 
     Mutex::Locker locker;
-    if (gdb_comm.GetSequenceMutex (locker))
+    if (gdb_comm.GetSequenceMutex (locker, "Didn't get sequence mutex for read all registers."))
     {
         char packet[32];
         const bool thread_suffix_supported = gdb_comm.GetThreadSuffixSupported();
@@ -522,12 +515,6 @@ GDBRemoteRegisterContext::ReadAllRegisterValues (lldb::DataBufferSP &data_sp)
     else
     {
         LogSP log (ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet (GDBR_LOG_THREAD | GDBR_LOG_PACKETS));
-#if LLDB_CONFIGURATION_DEBUG
-        StreamString strm;
-        gdb_comm.DumpHistory(strm);
-        Host::SetCrashDescription (strm.GetData());
-        assert (!"Didn't get sequence mutex for read all registers.");
-#else
         if (log)
         {
             if (log->GetVerbose())
@@ -539,7 +526,6 @@ GDBRemoteRegisterContext::ReadAllRegisterValues (lldb::DataBufferSP &data_sp)
             else
                 log->Printf("error: failed to get packet sequence mutex, not sending read all registers");
         }
-#endif
     }
 
     data_sp.reset();
@@ -563,7 +549,7 @@ GDBRemoteRegisterContext::WriteAllRegisterValues (const lldb::DataBufferSP &data
 
     StringExtractorGDBRemote response;
     Mutex::Locker locker;
-    if (gdb_comm.GetSequenceMutex (locker))
+    if (gdb_comm.GetSequenceMutex (locker, "Didn't get sequence mutex for write all registers."))
     {
         const bool thread_suffix_supported = gdb_comm.GetThreadSuffixSupported();
         ProcessSP process_sp (m_thread.GetProcess());
@@ -662,12 +648,6 @@ GDBRemoteRegisterContext::WriteAllRegisterValues (const lldb::DataBufferSP &data
     else
     {
         LogSP log (ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet (GDBR_LOG_THREAD | GDBR_LOG_PACKETS));
-#if LLDB_CONFIGURATION_DEBUG
-        StreamString strm;
-        gdb_comm.DumpHistory(strm);
-        Host::SetCrashDescription (strm.GetData());
-        assert (!"Didn't get sequence mutex for write all registers.");
-#else
         if (log)
         {
             if (log->GetVerbose())
@@ -679,7 +659,6 @@ GDBRemoteRegisterContext::WriteAllRegisterValues (const lldb::DataBufferSP &data
             else
                 log->Printf("error: failed to get packet sequence mutex, not sending write all registers");
         }
-#endif
     }
     return false;
 }
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index be5ccab7b90..ed826bac7c2 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1190,9 +1190,6 @@ ProcessGDBRemote::UpdateThreadIDList ()
     m_gdb_comm.GetCurrentThreadIDs (m_thread_ids, sequence_mutex_unavailable);
     if (sequence_mutex_unavailable)
     {
-#if defined (LLDB_CONFIGURATION_DEBUG)
-        assert(!"ProcessGDBRemote::UpdateThreadList() failed due to not getting the sequence mutex");
-#endif
         return false; // We just didn't get the list
     }
     return true;