Fix deadlock while attaching to inferiors

[labath]

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.

[clayborg]

Looks good.

[ovyalov]

LGTM

lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

protected:
     
     Flags m_flags;            // Process specific flags (see eFlags enums)
     GDBRemoteCommunicationClient m_gdb_comm;
     std::atomic<lldb::pid_t> m_debugserver_pid;
     std::vector<StringExtractorGDBRemote> m_stop_packet_stack;  // The stop packet stack replaces the last stop packet variable
     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;
     typedef std::vector< std::pair<lldb::tid_t,int> > tid_sig_collection;
     typedef std::map<lldb::addr_t, lldb::addr_t> MMapMap;
     typedef std::map<uint32_t, std::string> ExpeditedRegisterMap;
     tid_collection m_thread_ids; // Thread IDs for all threads. This list gets updated after stopping
     StructuredData::ObjectSP m_jstopinfo_sp; // Stop info only for any threads that have valid stop infos

lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

 ProcessGDBRemote::ProcessGDBRemote(lldb::TargetSP target_sp, Listener &listener) :
     Process (target_sp, listener),
     m_flags (0),
     m_gdb_comm (),
     m_debugserver_pid (LLDB_INVALID_PROCESS_ID),
     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 (),
     m_jthreadsinfo_sp (),
     m_continue_c_tids (),
     m_continue_C_tids (),
     m_continue_s_tids (),
     m_continue_S_tids (),
     m_max_memory_size (0),
     m_remote_stub_max_memory_size (0),
     m_addr_to_mmap_size (),
     m_thread_create_bp_sp (),
     m_waiting_for_attach (false),
     m_destroy_tried_resuming (false),
     m_command_sp (),
     m_breakpoint_pc_offset (0),
     m_initial_tid (LLDB_INVALID_THREAD_ID)
 {
     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);
 }
 
 //----------------------------------------------------------------------
 // Destructor
 //----------------------------------------------------------------------

 ProcessGDBRemote::AsyncThread (void *arg)
 {
     ProcessGDBRemote *process = (ProcessGDBRemote*) arg;
 
     Log *log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet (GDBR_LOG_PROCESS));
     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)
-    {
-        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 (listener.WaitForEvent (NULL, event_sp))
-            {
-                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);
+        if (process->m_async_listener.WaitForEvent (NULL, event_sp))
 
-                    switch (event_type)
-                    {
-                        case eBroadcastBitAsyncContinue:
-                            {
-                                const EventDataBytes *continue_packet = EventDataBytes::GetEventDataFromEvent(event_sp.get());
 
-                                if (continue_packet)
-                                {
-                                    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))
-                                        {
-                                            // 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 ();
 
-                                        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('-') == ';')
-                                            {
-                                                std::string desc_token;
-                                                while (response.GetNameColonValue (desc_token, desc_string))
-                                                {
-                                                    if (desc_token == "description")
-                                                    {
-                                                        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:
-                                        {
-                                            // 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;
-                                        }
 
-                                        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:
-                            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))
-                {
-                    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;
-                        }
 
-                        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;
-            }
-        }
-    }
 
     if (log)
         log->Printf ("ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64 ") thread exiting...", __FUNCTION__, arg, process->GetID());
 
     return NULL;
 }
 
 //uint32_t

lldb/trunk/test/dosep.py

else:
         target = m.group(1)
         remote = True
 
     expected_timeout = set()
 
     if target.startswith("linux"):
         expected_timeout |= {
             "TestAttachDenied.py",
-            "TestAttachResume.py",
             "TestProcessAttach.py",
             "TestConnectRemote.py",
             "TestCreateAfterAttach.py",
             "TestEvents.py",
             "TestExitDuringStep.py",
 
             # Times out in ~10% of the times on the build bot
             "TestHelloWorld.py",