Fix a race in ProcessGDBRemote::MonitorDebugServerProcess

[labath]

Summary

MonitorDebugServerProcess went to a lot of effort to make sure its asynchronous invocation does
not cause any mischief, but it was still not race-free. Specifically, in a quick stop-restart
sequence (like the one in TestAddressBreakpoints) the copying of the process shared pointer via
target_sp->GetProcessSP() was racing with the resetting of the pointer in DeleteCurrentProcess,
as they were both accessing the same shared_ptr object.

To avoid this, I simply pass in a weak_ptr to the process when the callback is created. Locking
this pointer is race-free as they are two separate object even though they point to the same
process instance. This also removes the need for the complicated tap-dance around retrieving the
process pointer.

[clayborg]

Looks good.

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

protected:
 
     void
     StopAsyncThread ();
 
     static lldb::thread_result_t
     AsyncThread (void *arg);
 
     static bool
-    MonitorDebugserverProcess(ProcessGDBRemote *process, lldb::pid_t pid, bool exited, int signo, int exit_status);
+    MonitorDebugserverProcess(std::weak_ptr<ProcessGDBRemote> process_wp, lldb::pid_t pid, bool exited, int signo,
+                              int exit_status);
 
     lldb::StateType
     SetThreadStopInfo (StringExtractor& stop_packet);
 
     bool
     GetThreadStopInfoFromJSON (ThreadGDBRemote *thread, const StructuredData::ObjectSP &thread_infos_sp);
 
     lldb::ThreadSP

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

if (m_debugserver_pid == LLDB_INVALID_PROCESS_ID)
         // If we locate debugserver, keep that located version around
         static FileSpec g_debugserver_file_spec;
 
         ProcessLaunchInfo debugserver_launch_info;
         // Make debugserver run in its own session so signals generated by
         // special terminal key sequences (^C) don't affect debugserver.
         debugserver_launch_info.SetLaunchInSeparateProcessGroup(true);
 
-        debugserver_launch_info.SetMonitorProcessCallback(std::bind(MonitorDebugserverProcess, this, _1, _2, _3, _4),
+        const std::weak_ptr<ProcessGDBRemote> this_wp = std::static_pointer_cast<ProcessGDBRemote>(shared_from_this());
+        debugserver_launch_info.SetMonitorProcessCallback(std::bind(MonitorDebugserverProcess, this_wp, _1, _2, _3, _4),
                                                           false);
         debugserver_launch_info.SetUserID(process_info.GetUserID());
 
 #if defined (__APPLE__) && (defined (__arm__) || defined (__arm64__) || defined (__aarch64__))
         // On iOS, still do a local connection using a random port
         const char *hostname = "127.0.0.1";
         uint16_t port = get_random_port ();
 #else
#endif
             error = ConnectToDebugserver (connect_url.GetString().c_str());
         }
 
     }
     return error;
 }
 
 bool
-ProcessGDBRemote::MonitorDebugserverProcess(ProcessGDBRemote *process, lldb::pid_t debugserver_pid,
+ProcessGDBRemote::MonitorDebugserverProcess(std::weak_ptr<ProcessGDBRemote> process_wp, lldb::pid_t debugserver_pid,
                                             bool exited,    // True if the process did exit
                                             int signo,      // Zero for no signal
                                             int exit_status // Exit value of process if signal is zero
                                             )
 {
-    // The baton is a "ProcessGDBRemote *". Now this class might be gone
-    // and might not exist anymore, so we need to carefully try to get the
-    // target for this process first since we have a race condition when
-    // we are done running between getting the notice that the inferior
-    // process has died and the debugserver that was debugging this process.
-    // In our test suite, we are also continually running process after
-    // process, so we must be very careful to make sure:
-    // 1 - process object hasn't been deleted already
-    // 2 - that a new process object hasn't been recreated in its place
-
     // "debugserver_pid" argument passed in is the process ID for
     // debugserver that we are tracking...
     Log *log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
+    const bool handled = true;
+
+    if (log)
+        log->Printf("ProcessGDBRemote::%s(process_wp, pid=%" PRIu64 ", signo=%i (0x%x), exit_status=%i)", __FUNCTION__,
+                    debugserver_pid, signo, signo, exit_status);
+
+    std::shared_ptr<ProcessGDBRemote> process_sp = process_wp.lock();
+    if (log)
+        log->Printf("ProcessGDBRemote::%s(process = %p)", __FUNCTION__, process_sp.get());
+    if (!process_sp || process_sp->m_debugserver_pid != debugserver_pid)
+        return handled;
 
-    // Get a shared pointer to the target that has a matching process pointer.
-    // This target could be gone, or the target could already have a new process
-    // object inside of it
-    TargetSP target_sp (Debugger::FindTargetWithProcess(process));
-
-    if (log)
-        log->Printf("ProcessGDBRemote::%s(process=%p, pid=%" PRIu64 ", signo=%i (0x%x), exit_status=%i)", __FUNCTION__,
-                    process, debugserver_pid, signo, signo, exit_status);
-
-    if (target_sp)
-    {
-        // We found a process in a target that matches, but another thread
-        // might be in the process of launching a new process that will
-        // soon replace it, so get a shared pointer to the process so we
-        // can keep it alive.
-        ProcessSP process_sp (target_sp->GetProcessSP());
-        // Now we have a shared pointer to the process that can't go away on us
-        // so we now make sure it was the same as the one passed in, and also make
-        // sure that our previous "process *" didn't get deleted and have a new
-        // "process *" created in its place with the same pointer. To verify this
-        // we make sure the process has our debugserver process ID. If we pass all
-        // of these tests, then we are sure that this process is the one we were
-        // looking for.
-        if (process_sp && process == process_sp.get() && process->m_debugserver_pid == debugserver_pid)
-        {
-            // Sleep for a half a second to make sure our inferior process has
-            // time to set its exit status before we set it incorrectly when
-            // both the debugserver and the inferior process shut down.
-            usleep (500000);
-            // If our process hasn't yet exited, debugserver might have died.
-            // If the process did exit, the we are reaping it.
-            const StateType state = process->GetState();
+    usleep(500000);
+    // If the process did exit, then we are reaping it.
+    const StateType state = process_sp->GetState();
 
-            if (process->m_debugserver_pid != LLDB_INVALID_PROCESS_ID &&
-                state != eStateInvalid &&
-                state != eStateUnloaded &&
-                state != eStateExited &&
-                state != eStateDetached)
-            {
-                char error_str[1024];
-                if (signo)
-                {
-                    const char *signal_cstr = process->GetUnixSignals()->GetSignalAsCString(signo);
-                    if (signal_cstr)
-                        ::snprintf (error_str, sizeof (error_str), DEBUGSERVER_BASENAME " died with signal %s", signal_cstr);
-                    else
-                        ::snprintf (error_str, sizeof (error_str), DEBUGSERVER_BASENAME " died with signal %i", signo);
-                }
-                else
-                {
-                    ::snprintf (error_str, sizeof (error_str), DEBUGSERVER_BASENAME " died with an exit status of 0x%8.8x", exit_status);
-                }
+    if (state != eStateInvalid && state != eStateUnloaded && state != eStateExited && state != eStateDetached)
+            const char *signal_cstr = process_sp->GetUnixSignals()->GetSignalAsCString(signo);
+                ::snprintf(error_str, sizeof(error_str), DEBUGSERVER_BASENAME " died with signal %s", signal_cstr);
+                ::snprintf(error_str, sizeof(error_str), DEBUGSERVER_BASENAME " died with signal %i", signo);
+            ::snprintf(error_str, sizeof(error_str), DEBUGSERVER_BASENAME " died with an exit status of 0x%8.8x",
+                       exit_status);
 
-                process->SetExitStatus (-1, error_str);
-            }
-            // Debugserver has exited we need to let our ProcessGDBRemote
-            // know that it no longer has a debugserver instance
-            process->m_debugserver_pid = LLDB_INVALID_PROCESS_ID;
-        }
-    }
-    return true;
+        process_sp->SetExitStatus(-1, error_str);
+    process_sp->m_debugserver_pid = LLDB_INVALID_PROCESS_ID;
+    return handled;
 }
 
 void
 ProcessGDBRemote::KillDebugserverProcess ()
 {
     m_gdb_comm.Disconnect();
     if (m_debugserver_pid != LLDB_INVALID_PROCESS_ID)
     {