Author: labath Date: Thu Sep 3 04:36:22 2015 New Revision: 246756 URL: http://llvm.org/viewvc/llvm-project?rev=246756&view=rev Log: Fix deadlock while attaching to inferiors
Summary: There was a race condition in the AsyncThread, where we would end up sending a vAttach notification to the thread before it got a chance set up its listener (this can be reproduced by adding a sleep() at the very beginning of ProcessGDBRemote::AsyncThread()). This event would then get lost and we LLDB would deadlock. I fix this by setting up the listener early on, in the ProcessGDBRemote constructor. This should improve the stability of all attach tests. For now, I am removing XTIMEOUT from TestAttachResume, and will watch the buildbots for signs of trouble. Reviewers: clayborg, ovyalov Subscribers: lldb-commits Differential Revision: http://reviews.llvm.org/D12552 Modified: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h lldb/trunk/test/dosep.py Modified: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp?rev=246756&r1=246755&r2=246756&view=diff ============================================================================== --- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (original) +++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp Thu Sep 3 04:36:22 2015 @@ -385,6 +385,7 @@ ProcessGDBRemote::ProcessGDBRemote(lldb: m_last_stop_packet_mutex (Mutex::eMutexTypeRecursive), m_register_info (), m_async_broadcaster (NULL, "lldb.process.gdb-remote.async-broadcaster"), + m_async_listener("lldb.process.gdb-remote.async-listener"), m_async_thread_state_mutex(Mutex::eMutexTypeRecursive), m_thread_ids (), m_jstopinfo_sp (), @@ -406,6 +407,25 @@ ProcessGDBRemote::ProcessGDBRemote(lldb: m_async_broadcaster.SetEventName (eBroadcastBitAsyncThreadShouldExit, "async thread should exit"); m_async_broadcaster.SetEventName (eBroadcastBitAsyncContinue, "async thread continue"); m_async_broadcaster.SetEventName (eBroadcastBitAsyncThreadDidExit, "async thread did exit"); + + Log *log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet (GDBR_LOG_ASYNC)); + + const uint32_t async_event_mask = eBroadcastBitAsyncContinue | eBroadcastBitAsyncThreadShouldExit; + + if (m_async_listener.StartListeningForEvents(&m_async_broadcaster, async_event_mask) != async_event_mask) + { + if (log) + log->Printf("ProcessGDBRemote::%s failed to listen for m_async_broadcaster events", __FUNCTION__); + } + + const uint32_t gdb_event_mask = Communication::eBroadcastBitReadThreadDidExit | + GDBRemoteCommunication::eBroadcastBitGdbReadThreadGotNotify; + if (m_async_listener.StartListeningForEvents(&m_gdb_comm, gdb_event_mask) != gdb_event_mask) + { + if (log) + log->Printf("ProcessGDBRemote::%s failed to listen for m_gdb_comm events", __FUNCTION__); + } + const uint64_t timeout_seconds = GetGlobalPluginProperties()->GetPacketTimeout(); if (timeout_seconds > 0) m_gdb_comm.SetPacketTimeout(timeout_seconds); @@ -3761,184 +3781,174 @@ ProcessGDBRemote::AsyncThread (void *arg if (log) log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") thread starting...", __FUNCTION__, arg, process->GetID()); - Listener listener ("ProcessGDBRemote::AsyncThread"); EventSP event_sp; - const uint32_t desired_event_mask = eBroadcastBitAsyncContinue | - eBroadcastBitAsyncThreadShouldExit; - - if (listener.StartListeningForEvents (&process->m_async_broadcaster, desired_event_mask) == desired_event_mask) + bool done = false; + while (!done) { - listener.StartListeningForEvents (&process->m_gdb_comm, Communication::eBroadcastBitReadThreadDidExit | - GDBRemoteCommunication::eBroadcastBitGdbReadThreadGotNotify); - - bool done = false; - while (!done) + if (log) + log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") listener.WaitForEvent (NULL, event_sp)...", __FUNCTION__, arg, process->GetID()); + if (process->m_async_listener.WaitForEvent (NULL, event_sp)) { - if (log) - log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") listener.WaitForEvent (NULL, event_sp)...", __FUNCTION__, arg, process->GetID()); - if (listener.WaitForEvent (NULL, event_sp)) + const uint32_t event_type = event_sp->GetType(); + if (event_sp->BroadcasterIs (&process->m_async_broadcaster)) { - const uint32_t event_type = event_sp->GetType(); - if (event_sp->BroadcasterIs (&process->m_async_broadcaster)) + if (log) + log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") Got an event of type: %d...", __FUNCTION__, arg, process->GetID(), event_type); + + switch (event_type) { - if (log) - log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") Got an event of type: %d...", __FUNCTION__, arg, process->GetID(), event_type); + case eBroadcastBitAsyncContinue: + { + const EventDataBytes *continue_packet = EventDataBytes::GetEventDataFromEvent(event_sp.get()); - switch (event_type) - { - case eBroadcastBitAsyncContinue: + if (continue_packet) { - const EventDataBytes *continue_packet = EventDataBytes::GetEventDataFromEvent(event_sp.get()); + const char *continue_cstr = (const char *)continue_packet->GetBytes (); + const size_t continue_cstr_len = continue_packet->GetByteSize (); + if (log) + log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") got eBroadcastBitAsyncContinue: %s", __FUNCTION__, arg, process->GetID(), continue_cstr); - if (continue_packet) + if (::strstr (continue_cstr, "vAttach") == NULL) + process->SetPrivateState(eStateRunning); + StringExtractorGDBRemote response; + + // If in Non-Stop-Mode + if (process->GetTarget().GetNonStopModeEnabled()) { - const char *continue_cstr = (const char *)continue_packet->GetBytes (); - const size_t continue_cstr_len = continue_packet->GetByteSize (); - if (log) - log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") got eBroadcastBitAsyncContinue: %s", __FUNCTION__, arg, process->GetID(), continue_cstr); - - if (::strstr (continue_cstr, "vAttach") == NULL) - process->SetPrivateState(eStateRunning); - StringExtractorGDBRemote response; - - // If in Non-Stop-Mode - if (process->GetTarget().GetNonStopModeEnabled()) + // send the vCont packet + if (!process->GetGDBRemote().SendvContPacket(process, continue_cstr, continue_cstr_len, response)) { - // send the vCont packet - if (!process->GetGDBRemote().SendvContPacket(process, continue_cstr, continue_cstr_len, response)) - { - // Something went wrong - done = true; - break; - } + // Something went wrong + done = true; + break; } - // If in All-Stop-Mode - else - { - StateType stop_state = process->GetGDBRemote().SendContinuePacketAndWaitForResponse (process, continue_cstr, continue_cstr_len, response); + } + // If in All-Stop-Mode + else + { + StateType stop_state = process->GetGDBRemote().SendContinuePacketAndWaitForResponse (process, continue_cstr, continue_cstr_len, response); - // We need to immediately clear the thread ID list so we are sure to get a valid list of threads. - // The thread ID list might be contained within the "response", or the stop reply packet that - // caused the stop. So clear it now before we give the stop reply packet to the process - // using the process->SetLastStopPacket()... - process->ClearThreadIDList (); + // We need to immediately clear the thread ID list so we are sure to get a valid list of threads. + // The thread ID list might be contained within the "response", or the stop reply packet that + // caused the stop. So clear it now before we give the stop reply packet to the process + // using the process->SetLastStopPacket()... + process->ClearThreadIDList (); - switch (stop_state) - { - case eStateStopped: - case eStateCrashed: - case eStateSuspended: - process->SetLastStopPacket (response); - process->SetPrivateState (stop_state); - break; + switch (stop_state) + { + case eStateStopped: + case eStateCrashed: + case eStateSuspended: + process->SetLastStopPacket (response); + process->SetPrivateState (stop_state); + break; - case eStateExited: + case eStateExited: + { + process->SetLastStopPacket (response); + process->ClearThreadIDList(); + response.SetFilePos(1); + + int exit_status = response.GetHexU8(); + const char *desc_cstr = NULL; + StringExtractor extractor; + std::string desc_string; + if (response.GetBytesLeft() > 0 && response.GetChar('-') == ';') { - process->SetLastStopPacket (response); - process->ClearThreadIDList(); - response.SetFilePos(1); - - int exit_status = response.GetHexU8(); - const char *desc_cstr = NULL; - StringExtractor extractor; - std::string desc_string; - if (response.GetBytesLeft() > 0 && response.GetChar('-') == ';') + std::string desc_token; + while (response.GetNameColonValue (desc_token, desc_string)) { - std::string desc_token; - while (response.GetNameColonValue (desc_token, desc_string)) + if (desc_token == "description") { - if (desc_token == "description") - { - extractor.GetStringRef().swap(desc_string); - extractor.SetFilePos(0); - extractor.GetHexByteString (desc_string); - desc_cstr = desc_string.c_str(); - } + extractor.GetStringRef().swap(desc_string); + extractor.SetFilePos(0); + extractor.GetHexByteString (desc_string); + desc_cstr = desc_string.c_str(); } } - process->SetExitStatus(exit_status, desc_cstr); - done = true; - break; } - case eStateInvalid: + process->SetExitStatus(exit_status, desc_cstr); + done = true; + break; + } + case eStateInvalid: + { + // Check to see if we were trying to attach and if we got back + // the "E87" error code from debugserver -- this indicates that + // the process is not debuggable. Return a slightly more helpful + // error message about why the attach failed. + if (::strstr (continue_cstr, "vAttach") != NULL + && response.GetError() == 0x87) { - // Check to see if we were trying to attach and if we got back - // the "E87" error code from debugserver -- this indicates that - // the process is not debuggable. Return a slightly more helpful - // error message about why the attach failed. - if (::strstr (continue_cstr, "vAttach") != NULL - && response.GetError() == 0x87) - { - process->SetExitStatus(-1, "cannot attach to process due to System Integrity Protection"); - } - // E01 code from vAttach means that the attach failed - if (::strstr (continue_cstr, "vAttach") != NULL - && response.GetError() == 0x1) - { - process->SetExitStatus(-1, "unable to attach"); - } - else - { - process->SetExitStatus(-1, "lost connection"); - } - break; + process->SetExitStatus(-1, "cannot attach to process due to System Integrity Protection"); + } + // E01 code from vAttach means that the attach failed + if (::strstr (continue_cstr, "vAttach") != NULL + && response.GetError() == 0x1) + { + process->SetExitStatus(-1, "unable to attach"); + } + else + { + process->SetExitStatus(-1, "lost connection"); } - - default: - process->SetPrivateState (stop_state); break; - } // switch(stop_state) - } // else // if in All-stop-mode - } // if (continue_packet) - } // case eBroadcastBitAysncContinue - break; + } - case eBroadcastBitAsyncThreadShouldExit: - if (log) - log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") got eBroadcastBitAsyncThreadShouldExit...", __FUNCTION__, arg, process->GetID()); - done = true; - break; + default: + process->SetPrivateState (stop_state); + break; + } // switch(stop_state) + } // else // if in All-stop-mode + } // if (continue_packet) + } // case eBroadcastBitAysncContinue + break; - default: - if (log) - log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") got unknown event 0x%8.8x", __FUNCTION__, arg, process->GetID(), event_type); - done = true; - break; - } + case eBroadcastBitAsyncThreadShouldExit: + if (log) + log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") got eBroadcastBitAsyncThreadShouldExit...", __FUNCTION__, arg, process->GetID()); + done = true; + break; + + default: + if (log) + log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") got unknown event 0x%8.8x", __FUNCTION__, arg, process->GetID(), event_type); + done = true; + break; } - else if (event_sp->BroadcasterIs (&process->m_gdb_comm)) + } + else if (event_sp->BroadcasterIs (&process->m_gdb_comm)) + { + switch (event_type) { - switch (event_type) - { - case Communication::eBroadcastBitReadThreadDidExit: - process->SetExitStatus (-1, "lost connection"); - done = true; - break; - - case GDBRemoteCommunication::eBroadcastBitGdbReadThreadGotNotify: - { - lldb_private::Event *event = event_sp.get(); - const EventDataBytes *continue_packet = EventDataBytes::GetEventDataFromEvent(event); - StringExtractorGDBRemote notify((const char*)continue_packet->GetBytes()); - // Hand this over to the process to handle - process->HandleNotifyPacket(notify); - break; - } + case Communication::eBroadcastBitReadThreadDidExit: + process->SetExitStatus (-1, "lost connection"); + done = true; + break; - default: - if (log) - log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") got unknown event 0x%8.8x", __FUNCTION__, arg, process->GetID(), event_type); - done = true; - break; + case GDBRemoteCommunication::eBroadcastBitGdbReadThreadGotNotify: + { + lldb_private::Event *event = event_sp.get(); + const EventDataBytes *continue_packet = EventDataBytes::GetEventDataFromEvent(event); + StringExtractorGDBRemote notify((const char*)continue_packet->GetBytes()); + // Hand this over to the process to handle + process->HandleNotifyPacket(notify); + break; } + + default: + if (log) + log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") got unknown event 0x%8.8x", __FUNCTION__, arg, process->GetID(), event_type); + done = true; + break; } } - else - { - if (log) - log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") listener.WaitForEvent (NULL, event_sp) => false", __FUNCTION__, arg, process->GetID()); - done = true; - } + } + else + { + if (log) + log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") listener.WaitForEvent (NULL, event_sp) => false", __FUNCTION__, arg, process->GetID()); + done = true; } } Modified: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h?rev=246756&r1=246755&r2=246756&view=diff ============================================================================== --- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h (original) +++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h Thu Sep 3 04:36:22 2015 @@ -358,6 +358,7 @@ protected: Mutex m_last_stop_packet_mutex; GDBRemoteDynamicRegisterInfo m_register_info; Broadcaster m_async_broadcaster; + Listener m_async_listener; HostThread m_async_thread; Mutex m_async_thread_state_mutex; typedef std::vector<lldb::tid_t> tid_collection; Modified: lldb/trunk/test/dosep.py URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/test/dosep.py?rev=246756&r1=246755&r2=246756&view=diff ============================================================================== --- lldb/trunk/test/dosep.py (original) +++ lldb/trunk/test/dosep.py Thu Sep 3 04:36:22 2015 @@ -291,7 +291,6 @@ def getExpectedTimeouts(platform_name): if target.startswith("linux"): expected_timeout |= { "TestAttachDenied.py", - "TestAttachResume.py", "TestProcessAttach.py", "TestConnectRemote.py", "TestCreateAfterAttach.py", _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
