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

llvm-svn: 246756

1 files changed, 158 insertions, 148 deletions

diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 17036d9eea4..4a60d68dd23 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -385,6 +385,7 @@ ProcessGDBRemote::ProcessGDBRemote(lldb::TargetSP target_sp, Listener &listener)
     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::TargetSP target_sp, Listener &listener)
     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 (::strstr (continue_cstr, "vAttach") == NULL)
+                                    process->SetPrivateState(eStateRunning);
+                                StringExtractorGDBRemote response;
 
-                                if (continue_packet)
+                                // 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);
-
-                                        // 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 ();
+                                }
+                                // If in All-Stop-Mode
+                                else
+                                {
+                                    StateType stop_state = process->GetGDBRemote().SendContinuePacketAndWaitForResponse (process, continue_cstr, continue_cstr_len, response);
 
-                                        switch (stop_state)
-                                        {
-                                        case eStateStopped:
-                                        case eStateCrashed:
-                                        case eStateSuspended:
-                                            process->SetLastStopPacket (response);
-                                            process->SetPrivateState (stop_state);
-                                            break;
+                                    // 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 ();
 
-                                        case eStateExited:
+                                    switch (stop_state)
+                                    {
+                                    case eStateStopped:
+                                    case eStateCrashed:
+                                    case eStateSuspended:
+                                        process->SetLastStopPacket (response);
+                                        process->SetPrivateState (stop_state);
+                                        break;
+
+                                    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;
         }
     }