Fix deadlock due to thread list locking in 'bt all' with obj-c

sas · sas · commit f81049184a7d · 2016-03-17T18:52:41.000Z
Summary: The gdb-remote async thread cannot modify thread state while the main thread holds a lock on the state. Don't use locking thread iteration for bt all. Specifically, the deadlock manifests when lldb attempts to JIT code to symbolicate objective c while backtracing. As part of this code path, SetPrivateState() is called on an async thread. This async thread will block waiting for the thread_list lock held by the main thread in CommandObjectIterateOverThreads. The main thread will also block on the async thread during DoResume (although with a timeout), leading to a deadlock. Due to the timeout, the deadlock is not immediately apparent, but the inferior will be left in an invalid state after the bt all completes, and objective-c symbols will not be successfully resolved in the backtrace. Reviewers: andrew.w.kaylor, jingham, clayborg Subscribers: sas, lldb-commits Differential Revision: http://reviews.llvm.org/D18075 Change by Francis Ricci <fjricci@fb.com> llvm-svn: 263735
diff --git a/lldb/source/Commands/CommandObjectThread.cpp b/lldb/source/Commands/CommandObjectThread.cpp
@@ -66,61 +66,65 @@ class CommandObjectIterateOverThreads : public CommandObjectParsed
         if (command.GetArgumentCount() == 0)
         {
             Thread *thread = m_exe_ctx.GetThreadPtr();
-            if (!HandleOneThread (*thread, result))
+            if (!HandleOneThread (thread->GetID(), result))
                 return false;
+            return result.Succeeded();
         }
-        else if (command.GetArgumentCount() == 1 && ::strcmp (command.GetArgumentAtIndex(0), "all") == 0)
+
+        // Use tids instead of ThreadSPs to prevent deadlocking problems which result from JIT-ing
+        // code while iterating over the (locked) ThreadSP list.
+        std::vector<lldb::tid_t> tids;
+
+        if (command.GetArgumentCount() == 1 && ::strcmp (command.GetArgumentAtIndex(0), "all") == 0)
         {
             Process *process = m_exe_ctx.GetProcessPtr();
-            uint32_t idx = 0;
-            for (ThreadSP thread_sp : process->Threads())
-            {
-                if (idx != 0 && m_add_return)
-                    result.AppendMessage("");
 
-                if (!HandleOneThread(*(thread_sp.get()), result))
-                    return false;
-                ++idx;
-            }
+            for (ThreadSP thread_sp : process->Threads())
+                tids.push_back(thread_sp->GetID());
         }
         else
         {
             const size_t num_args = command.GetArgumentCount();
             Process *process = m_exe_ctx.GetProcessPtr();
+
             Mutex::Locker locker (process->GetThreadList().GetMutex());
-            std::vector<ThreadSP> thread_sps;
 
             for (size_t i = 0; i < num_args; i++)
             {
                 bool success;
-                
+
                 uint32_t thread_idx = StringConvert::ToUInt32(command.GetArgumentAtIndex(i), 0, 0, &success);
                 if (!success)
                 {
                     result.AppendErrorWithFormat ("invalid thread specification: \"%s\"\n", command.GetArgumentAtIndex(i));
                     result.SetStatus (eReturnStatusFailed);
                     return false;
                 }
-                
-                thread_sps.push_back(process->GetThreadList().FindThreadByIndexID(thread_idx));
-                
-                if (!thread_sps[i])
+
+                ThreadSP thread = process->GetThreadList().FindThreadByIndexID(thread_idx);
+
+                if (!thread)
                 {
                     result.AppendErrorWithFormat ("no thread with index: \"%s\"\n", command.GetArgumentAtIndex(i));
                     result.SetStatus (eReturnStatusFailed);
                     return false;
                 }
-            }
-            
-            for (uint32_t i = 0; i < num_args; i++)
-            {
-                if (!HandleOneThread (*(thread_sps[i].get()), result))
-                    return false;
 
-                if (i < num_args - 1 && m_add_return)
-                    result.AppendMessage("");
+                tids.push_back(thread->GetID());
             }
         }
+
+        uint32_t idx = 0;
+        for (const lldb::tid_t &tid : tids)
+        {
+            if (idx != 0 && m_add_return)
+                result.AppendMessage("");
+
+            if (!HandleOneThread (tid, result))
+                return false;
+
+            ++idx;
+        }
         return result.Succeeded();
     }
 
@@ -133,7 +137,7 @@ class CommandObjectIterateOverThreads : public CommandObjectParsed
     // If m_add_return is true, a blank line will be inserted between each of the listings (except the last one.)
 
     virtual bool
-    HandleOneThread (Thread &thread, CommandReturnObject &result) = 0;
+    HandleOneThread (lldb::tid_t, CommandReturnObject &result) = 0;
 
     ReturnStatus m_success_return = eReturnStatusSuccessFinishResult;
     bool m_add_return = true;
@@ -275,25 +279,35 @@ class CommandObjectThreadBacktrace : public CommandObjectIterateOverThreads
     }
 
     bool
-    HandleOneThread (Thread &thread, CommandReturnObject &result) override
+    HandleOneThread (lldb::tid_t tid, CommandReturnObject &result) override
     {
+        ThreadSP thread_sp = m_exe_ctx.GetProcessPtr()->GetThreadList().FindThreadByID(tid);
+        if (!thread_sp)
+        {
+            result.AppendErrorWithFormat ("thread disappeared while computing backtraces: 0x%" PRIx64 "\n", tid);
+            result.SetStatus (eReturnStatusFailed);
+            return false;
+        }
+
+        Thread *thread = thread_sp.get();
+
         Stream &strm = result.GetOutputStream();
 
         // Don't show source context when doing backtraces.
         const uint32_t num_frames_with_source = 0;
 
-        if (!thread.GetStatus (strm,
-                                   m_options.m_start,
-                                   m_options.m_count,
-                                   num_frames_with_source))
+        if (!thread->GetStatus (strm,
+                                m_options.m_start,
+                                m_options.m_count,
+                                num_frames_with_source))
         {
-            result.AppendErrorWithFormat ("error displaying backtrace for thread: \"0x%4.4x\"\n", thread.GetIndexID());
+            result.AppendErrorWithFormat ("error displaying backtrace for thread: \"0x%4.4x\"\n", thread->GetIndexID());
             result.SetStatus (eReturnStatusFailed);
             return false;
         }
         if (m_options.m_extended_backtrace)
         {
-            DoExtendedBacktrace (&thread, result);
+            DoExtendedBacktrace (thread, result);
         }
 
         return true;
@@ -1537,12 +1551,22 @@ class CommandObjectThreadInfo : public CommandObjectIterateOverThreads
     }
 
     bool
-    HandleOneThread (Thread &thread, CommandReturnObject &result) override
+    HandleOneThread (lldb::tid_t tid, CommandReturnObject &result) override
     {
+        ThreadSP thread_sp = m_exe_ctx.GetProcessPtr()->GetThreadList().FindThreadByID(tid);
+        if (!thread_sp)
+        {
+            result.AppendErrorWithFormat ("thread no longer exists: 0x%" PRIx64 "\n", tid);
+            result.SetStatus (eReturnStatusFailed);
+            return false;
+        }
+
+        Thread *thread = thread_sp.get();
+
         Stream &strm = result.GetOutputStream();
-        if (!thread.GetDescription (strm, eDescriptionLevelFull, m_options.m_json_thread, m_options.m_json_stopinfo))
+        if (!thread->GetDescription (strm, eDescriptionLevelFull, m_options.m_json_thread, m_options.m_json_stopinfo))
         {
-            result.AppendErrorWithFormat ("error displaying info for thread: \"%d\"\n", thread.GetIndexID());
+            result.AppendErrorWithFormat ("error displaying info for thread: \"%d\"\n", thread->GetIndexID());
             result.SetStatus (eReturnStatusFailed);
             return false;
         }
@@ -2044,14 +2068,24 @@ class CommandObjectThreadPlanList : public CommandObjectIterateOverThreads
 
 protected:
     bool
-    HandleOneThread (Thread &thread, CommandReturnObject &result) override
+    HandleOneThread (lldb::tid_t tid, CommandReturnObject &result) override
     {
+        ThreadSP thread_sp = m_exe_ctx.GetProcessPtr()->GetThreadList().FindThreadByID(tid);
+        if (!thread_sp)
+        {
+            result.AppendErrorWithFormat ("thread no longer exists: 0x%" PRIx64 "\n", tid);
+            result.SetStatus (eReturnStatusFailed);
+            return false;
+        }
+
+        Thread *thread = thread_sp.get();
+
         Stream &strm = result.GetOutputStream();
         DescriptionLevel desc_level = eDescriptionLevelFull;
         if (m_options.m_verbose)
             desc_level = eDescriptionLevelVerbose;
 
-        thread.DumpThreadPlans (&strm, desc_level, m_options.m_internal, true);
+        thread->DumpThreadPlans (&strm, desc_level, m_options.m_internal, true);
         return true;
     }
 
