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

[fjricci]

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.

[fjricci]

Remove duplication of index variable

[jingham]

Yeah, I'm beginning to wonder whether we're doing ourselves a favor by putting off computing the ObjC runtime symbols till some time that's bound to be more inconvenient than right when we've stopped, but that's beyond the scope of this patch.

For this patch, there's no guarantee that running the JIT'ed code won't cause a thread to exit, so that by the time you got around calling HandleOneThread in this loop it had no real backing . The Thread your ThreadSP is keeping alive should have been Clear()ed in that case, so IsValid will return false. It would be good to put an explicit check for that in this loop, and if the thread is no longer valid, report some useful message ("thread disappeared while computing the backtraces.") Actually, that check should be done before the HandleOneThread in the the other branch of the if as well.

[jingham]

Another even safer way to do this is to take the thread list, turn it into a vector of TID's, then iterate over the TID's looking them up one at a time as you go. That way you don't have to worry about your ThreadSP going stale. You could even change HandleOneThread to take a tid, so you could centralize the lookup.

[jingham]

Oh, yeah, and note the other branch of the if holds the thread list mutex the whole time. So you'll need to change that to get the list of threads, then drop the mutex & iterate over them or you'll get into the same problem if somebody does:

(lldb) thread backtrace 1 2 3 4 5

or whatever.

[fjricci]

Use vector of tids for iteration, rather than iterating over unlocked threadlist

[jingham]

That looks good, thanks.

[clayborg]

Looks good.

[sas]

Committed as r263735.

source/Commands/CommandObjectThread.cpp

DoExecute (Args& command, CommandReturnObject &result) override
         {
             Thread *thread = m_exe_ctx.GetThreadPtr();
             if (!HandleOneThread (*thread, result))
                 return false;
         }
         else 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())
+
+            // Manually iterate to avoid locking the threadlist,
+            // which can cause deadlocks when JIT-ing code
+            ThreadList thread_list = process->GetThreadList();
+            for (uint32_t idx = 0; idx < thread_list.GetSize(); ++idx)
             {
+                ThreadSP thread_sp = thread_list.GetThreadAtIndex(idx);
                 if (idx != 0 && m_add_return)
                     result.AppendMessage("");
 
                 if (!HandleOneThread(*(thread_sp.get()), result))
                     return false;
-                ++idx;
             }
         }
         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;