From 9fc13556b4c58c850fde19b306fd01c87c62f5fa Mon Sep 17 00:00:00 2001 From: Greg Clayton Date: Tue, 10 Apr 2012 00:18:59 +0000 Subject: [PATCH] Trying to solve our disappearing thread issues by making thread list updates safer. The current ProcessGDBRemote function that updates the threads could end up with an empty list if any other thread had the sequence mutex. We now don't clear the thread list when we can't access it, and we also have changed how lldb_private::Process handles the return code from the: virtual bool Process::UpdateThreadList (lldb_private::ThreadList &old_thread_list, lldb_private::ThreadList &new_thread_list) = 0; A bool is now returned to indicate if the list was actually updated or not and the lldb_private::Process class will only update the stop ID of the validity of the thread list if "true" is returned. The ProcessGDBRemote also got an extra assertion that will hopefully assert when running debug builds so we can find the source of this issue. llvm-svn: 154365 --- lldb/include/lldb/Target/OperatingSystem.h | 2 +- lldb/include/lldb/Target/Process.h | 2 +- .../OperatingSystemDarwinKernel.cpp | 4 ++-- .../Darwin-Kernel/OperatingSystemDarwinKernel.h | 2 +- .../Process/MacOSX-Kernel/CommunicationKDP.cpp | 2 +- .../Process/MacOSX-Kernel/CommunicationKDP.h | 2 +- .../Process/MacOSX-Kernel/ProcessKDP.cpp | 4 ++-- .../Plugins/Process/MacOSX-Kernel/ProcessKDP.h | 2 +- .../gdb-remote/GDBRemoteCommunication.cpp | 5 +---- .../Process/gdb-remote/GDBRemoteCommunication.h | 2 +- .../gdb-remote/GDBRemoteCommunicationClient.cpp | 8 ++++---- .../gdb-remote/GDBRemoteRegisterContext.cpp | 8 ++++---- .../Process/gdb-remote/ProcessGDBRemote.cpp | 14 ++++++++++---- .../Process/gdb-remote/ProcessGDBRemote.h | 2 +- .../Process/mach-core/ProcessMachCore.cpp | 4 ++-- .../Plugins/Process/mach-core/ProcessMachCore.h | 2 +- lldb/source/Target/Process.cpp | 17 +++++++++-------- 17 files changed, 43 insertions(+), 39 deletions(-) diff --git a/lldb/include/lldb/Target/OperatingSystem.h b/lldb/include/lldb/Target/OperatingSystem.h index 6159e3fbcf35..7c5f060a6ba7 100644 --- a/lldb/include/lldb/Target/OperatingSystem.h +++ b/lldb/include/lldb/Target/OperatingSystem.h @@ -64,7 +64,7 @@ public: //------------------------------------------------------------------ // Plug-in Methods //------------------------------------------------------------------ - virtual uint32_t + virtual bool UpdateThreadList (ThreadList &old_thread_list, ThreadList &new_thread_list) = 0; virtual void diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h index fb6de867fbcd..5901b493bc01 100644 --- a/lldb/include/lldb/Target/Process.h +++ b/lldb/include/lldb/Target/Process.h @@ -2937,7 +2937,7 @@ public: //------------------------------------------------------------------ // Thread Queries //------------------------------------------------------------------ - virtual uint32_t + virtual bool UpdateThreadList (ThreadList &old_thread_list, ThreadList &new_thread_list) = 0; void diff --git a/lldb/source/Plugins/OperatingSystem/Darwin-Kernel/OperatingSystemDarwinKernel.cpp b/lldb/source/Plugins/OperatingSystem/Darwin-Kernel/OperatingSystemDarwinKernel.cpp index 7315a8250718..4598f64ebbc9 100644 --- a/lldb/source/Plugins/OperatingSystem/Darwin-Kernel/OperatingSystemDarwinKernel.cpp +++ b/lldb/source/Plugins/OperatingSystem/Darwin-Kernel/OperatingSystemDarwinKernel.cpp @@ -223,7 +223,7 @@ OperatingSystemDarwinKernel::GetPluginVersion() return 1; } -uint32_t +bool OperatingSystemDarwinKernel::UpdateThreadList (ThreadList &old_thread_list, ThreadList &new_thread_list) { // Make any constant strings once and cache the uniqued C string values @@ -263,7 +263,7 @@ OperatingSystemDarwinKernel::UpdateThreadList (ThreadList &old_thread_list, Thre } next_valobj_sp.swap(valobj_sp); } - return new_thread_list.GetSize(false); + return new_thread_list.GetSize(false) > 0; } void diff --git a/lldb/source/Plugins/OperatingSystem/Darwin-Kernel/OperatingSystemDarwinKernel.h b/lldb/source/Plugins/OperatingSystem/Darwin-Kernel/OperatingSystemDarwinKernel.h index 83f0e017ca4d..4b9d2edbe924 100644 --- a/lldb/source/Plugins/OperatingSystem/Darwin-Kernel/OperatingSystemDarwinKernel.h +++ b/lldb/source/Plugins/OperatingSystem/Darwin-Kernel/OperatingSystemDarwinKernel.h @@ -61,7 +61,7 @@ public: //------------------------------------------------------------------ // lldb_private::OperatingSystem Methods //------------------------------------------------------------------ - virtual uint32_t + virtual bool UpdateThreadList (lldb_private::ThreadList &old_thread_list, lldb_private::ThreadList &new_thread_list); diff --git a/lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.cpp b/lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.cpp index c1d9f9551598..8fc3692c2614 100644 --- a/lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.cpp +++ b/lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.cpp @@ -151,7 +151,7 @@ CommunicationKDP::SendRequestPacketNoLock (const PacketStreamType &request_packe } bool -CommunicationKDP::GetSequenceMutex (Mutex::Locker& locker) +CommunicationKDP::TryLockSequenceMutex (Mutex::Locker& locker) { return locker.TryLock (m_sequence_mutex.GetMutex()); } diff --git a/lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.h b/lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.h index db1e02f605f9..728cddd995d4 100644 --- a/lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.h +++ b/lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.h @@ -102,7 +102,7 @@ public: uint32_t usec); bool - GetSequenceMutex(lldb_private::Mutex::Locker& locker); + TryLockSequenceMutex(lldb_private::Mutex::Locker& locker); bool CheckForPacket (const uint8_t *src, diff --git a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp index 1db2d842dd86..45ed9b5f8543 100644 --- a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp +++ b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp @@ -301,7 +301,7 @@ ProcessKDP::DoResume () return error; } -uint32_t +bool ProcessKDP::UpdateThreadList (ThreadList &old_thread_list, ThreadList &new_thread_list) { // locker will keep a mutex locked until it goes out of scope @@ -323,7 +323,7 @@ ProcessKDP::UpdateThreadList (ThreadList &old_thread_list, ThreadList &new_threa thread_sp.reset(new ThreadKDP (shared_from_this(), tid)); new_thread_list.AddThread(thread_sp); } - return new_thread_list.GetSize(false); + return new_thread_list.GetSize(false) > 0; } diff --git a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h index bfdea7bcc0f3..297d854cd9cf 100644 --- a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h +++ b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h @@ -231,7 +231,7 @@ protected: void Clear ( ); - uint32_t + virtual bool UpdateThreadList (lldb_private::ThreadList &old_thread_list, lldb_private::ThreadList &new_thread_list); diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp index 5b589dce7951..caa70258e300 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp @@ -201,10 +201,7 @@ GDBRemoteCommunication::SendPacketNoLock (const char *payload, size_t payload_le // logs all of the packet will set a boolean so that we don't dump this more // than once if (!m_history.DidDumpToLog ()) - { - DumpHistory("/tmp/foo.txt"); m_history.Dump (log.get()); - } log->Printf ("<%4zu> send packet: %.*s", bytes_written, (int)packet.GetSize(), packet.GetData()); } @@ -243,7 +240,7 @@ GDBRemoteCommunication::GetAck () } bool -GDBRemoteCommunication::GetSequenceMutex (Mutex::Locker& locker) +GDBRemoteCommunication::TryLockSequenceMutex (Mutex::Locker& locker) { return locker.TryLock (m_sequence_mutex.GetMutex()); } diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h index 0e9ea0dcb1d5..d7986d5b278c 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h @@ -74,7 +74,7 @@ public: size_t payload_length); bool - GetSequenceMutex(lldb_private::Mutex::Locker& locker); + TryLockSequenceMutex(lldb_private::Mutex::Locker& locker); bool CheckForPacket (const uint8_t *src, diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp index e308f915d1cc..4fbfa982e0de 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp @@ -240,7 +240,7 @@ GDBRemoteCommunicationClient::SendPacketAndWaitForResponse Mutex::Locker locker; LogSP log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet (GDBR_LOG_PROCESS)); size_t response_len = 0; - if (GetSequenceMutex (locker)) + if (TryLockSequenceMutex (locker)) { if (SendPacketNoLock (payload, payload_length)) response_len = WaitForPacketWithTimeoutMicroSecondsNoLock (response, GetPacketTimeoutInMicroSeconds ()); @@ -628,7 +628,7 @@ GDBRemoteCommunicationClient::SendAsyncSignal (int signo) return false; } -// This function takes a mutex locker as a parameter in case the GetSequenceMutex +// This function takes a mutex locker as a parameter in case the TryLockSequenceMutex // actually succeeds. If it doesn't succeed in acquiring the sequence mutex // (the expected result), then it will send the halt packet. If it does succeed // then the caller that requested the interrupt will want to keep the sequence @@ -653,7 +653,7 @@ GDBRemoteCommunicationClient::SendInterrupt if (IsRunning()) { // Only send an interrupt if our debugserver is running... - if (GetSequenceMutex (locker) == false) + if (TryLockSequenceMutex (locker) == false) { // Someone has the mutex locked waiting for a response or for the // inferior to stop, so send the interrupt on the down low... @@ -1842,7 +1842,7 @@ GDBRemoteCommunicationClient::GetCurrentThreadIDs (std::vector &thr Mutex::Locker locker; thread_ids.clear(); - if (GetSequenceMutex (locker)) + if (TryLockSequenceMutex (locker)) { sequence_mutex_unavailable = false; StringExtractorGDBRemote response; diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp index 77051fffc6e6..120caf3d7a78 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp @@ -181,7 +181,7 @@ GDBRemoteRegisterContext::ReadRegisterBytes (const RegisterInfo *reg_info, DataE if (!m_reg_valid[reg]) { Mutex::Locker locker; - if (gdb_comm.GetSequenceMutex (locker)) + if (gdb_comm.TryLockSequenceMutex (locker)) { const bool thread_suffix_supported = gdb_comm.GetThreadSuffixSupported(); ProcessSP process_sp (m_thread.GetProcess()); @@ -331,7 +331,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.TryLockSequenceMutex (locker)) { const bool thread_suffix_supported = gdb_comm.GetThreadSuffixSupported(); ProcessSP process_sp (m_thread.GetProcess()); @@ -440,7 +440,7 @@ GDBRemoteRegisterContext::ReadAllRegisterValues (lldb::DataBufferSP &data_sp) StringExtractorGDBRemote response; Mutex::Locker locker; - if (gdb_comm.GetSequenceMutex (locker)) + if (gdb_comm.TryLockSequenceMutex (locker)) { char packet[32]; const bool thread_suffix_supported = gdb_comm.GetThreadSuffixSupported(); @@ -496,7 +496,7 @@ GDBRemoteRegisterContext::WriteAllRegisterValues (const lldb::DataBufferSP &data StringExtractorGDBRemote response; Mutex::Locker locker; - if (gdb_comm.GetSequenceMutex (locker)) + if (gdb_comm.TryLockSequenceMutex (locker)) { const bool thread_suffix_supported = gdb_comm.GetThreadSuffixSupported(); ProcessSP process_sp (m_thread.GetProcess()); diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index 8a1f120f3083..8b667dffd8d3 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -1129,7 +1129,7 @@ ProcessGDBRemote::DoResume () return error; } -uint32_t +bool ProcessGDBRemote::UpdateThreadList (ThreadList &old_thread_list, ThreadList &new_thread_list) { // locker will keep a mutex locked until it goes out of scope @@ -1151,11 +1151,17 @@ ProcessGDBRemote::UpdateThreadList (ThreadList &old_thread_list, ThreadList &new thread_sp.reset (new ThreadGDBRemote (shared_from_this(), tid)); new_thread_list.AddThread(thread_sp); } + SetThreadStopInfo (m_last_stop_packet); + } + else 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 } - if (sequence_mutex_unavailable == false) - SetThreadStopInfo (m_last_stop_packet); - return new_thread_list.GetSize(false); + return true; } diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h index 9f3b75874ea2..a41957fdc0a2 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h @@ -265,7 +265,7 @@ protected: return m_flags; } - uint32_t + virtual bool UpdateThreadList (lldb_private::ThreadList &old_thread_list, lldb_private::ThreadList &new_thread_list); diff --git a/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp b/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp index a111fb8d343a..abff31688d02 100644 --- a/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp +++ b/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp @@ -337,7 +337,7 @@ ProcessMachCore::GetDynamicLoader () return m_dyld_ap.get(); } -uint32_t +bool ProcessMachCore::UpdateThreadList (ThreadList &old_thread_list, ThreadList &new_thread_list) { if (old_thread_list.GetSize(false) == 0) @@ -362,7 +362,7 @@ ProcessMachCore::UpdateThreadList (ThreadList &old_thread_list, ThreadList &new_ for (uint32_t i=0; i 0; } void diff --git a/lldb/source/Plugins/Process/mach-core/ProcessMachCore.h b/lldb/source/Plugins/Process/mach-core/ProcessMachCore.h index 924f549475eb..e20276f1da27 100644 --- a/lldb/source/Plugins/Process/mach-core/ProcessMachCore.h +++ b/lldb/source/Plugins/Process/mach-core/ProcessMachCore.h @@ -116,7 +116,7 @@ protected: void Clear ( ); - uint32_t + virtual bool UpdateThreadList (lldb_private::ThreadList &old_thread_list, lldb_private::ThreadList &new_thread_list); diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index d17449128d01..6ad0ed05c6e8 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -1236,13 +1236,15 @@ Process::UpdateThreadListIfNeeded () // and the os->UpdateThreadList(...) so it doesn't change on us ThreadList new_thread_list(this); // Always update the thread list with the protocol specific - // thread list - UpdateThreadList (m_thread_list, new_thread_list); - OperatingSystem *os = GetOperatingSystem (); - if (os) - os->UpdateThreadList (m_thread_list, new_thread_list); - m_thread_list.Update (new_thread_list); - m_thread_list.SetStopID (stop_id); + // thread list, but only update if "true" is returned + if (UpdateThreadList (m_thread_list, new_thread_list)) + { + OperatingSystem *os = GetOperatingSystem (); + if (os) + os->UpdateThreadList (m_thread_list, new_thread_list); + m_thread_list.Update (new_thread_list); + m_thread_list.SetStopID (stop_id); + } } } } @@ -4281,7 +4283,6 @@ Process::RunThreadPlan (ExecutionContext &exe_ctx, if (IS_VALID_LLDB_HOST_THREAD(backup_private_state_thread)) { StopPrivateStateThread(); - lldb::thread_result_t thread_result; Error error; // Host::ThreadJoin(m_private_state_thread, &thread_result, &error); m_private_state_thread = backup_private_state_thread;