Send an explicit interrupt to cancel an attach waitfor. (#72565)

Currently when you interrupt a:

(lldb) process attach -w -n some_process

lldb just closes the connection to the stub and kills the
lldb_private::Process it made for the attach. The stub at the other end
notices the connection go down and exits because of that. But when
communication to a device is handled through some kind of proxy server
which isn't as well behaved as one would wish, that signal might not be
reliable, causing debugserver to persist on the machine, waiting to
steal the next instance of that process.

We can work around those failures by sending an explicit interrupt
before closing down the connection. The stub will also have to be
waiting for the interrupt for this to make any difference. I changed
debugserver to do that.

I didn't make the equivalent change in lldb-server. So long as you
aren't faced with a flakey connection, this should not be necessary.

Author: jimingham

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

@@ -2371,8 +2371,10 @@ Status ProcessGDBRemote::DoHalt(bool &caused_stop) {
   Status error;
 
   if (m_public_state.GetValue() == eStateAttaching) {
-    // We are being asked to halt during an attach. We need to just close our
-    // file handle and debugserver will go away, and we can be done...
+    // We are being asked to halt during an attach. We used to just close our
+    // file handle and debugserver will go away, but with remote proxies, it
+    // is better to send a positive signal, so let's send the interrupt first...
+    caused_stop = m_gdb_comm.Interrupt(GetInterruptTimeout());
     m_gdb_comm.Disconnect();
   } else
     caused_stop = m_gdb_comm.Interrupt(GetInterruptTimeout());

lldb/source/Target/Process.cpp

@@ -3156,8 +3156,8 @@ Status Process::Halt(bool clear_thread_plans, bool use_run_lock) {
     // Don't hijack and eat the eStateExited as the code that was doing the
     // attach will be waiting for this event...
     RestoreProcessEvents();
-    SetExitStatus(SIGKILL, "Cancelled async attach.");
     Destroy(false);
+    SetExitStatus(SIGKILL, "Cancelled async attach.");
     return Status();
   }
 
@@ -3843,6 +3843,13 @@ thread_result_t Process::RunPrivateStateThread(bool is_secondary_thread) {
                   ") woke up with an interrupt while attaching - "
                   "forwarding interrupt.",
                   __FUNCTION__, static_cast<void *>(this), GetID());
+        // The server may be spinning waiting for a process to appear, in which
+        // case we should tell it to stop doing that.  Normally, we don't NEED
+        // to do that because we will next close the communication to the stub
+        // and that will get it to shut down.  But there are remote debugging
+        // cases where relying on that side-effect causes the shutdown to be 
+        // flakey, so we should send a positive signal to interrupt the wait. 
+        Status error = HaltPrivate();
         BroadcastEvent(eBroadcastBitInterrupt, nullptr);
       } else if (StateIsRunningState(m_last_broadcast_state)) {
         LLDB_LOGF(log,

lldb/test/API/python_api/process/cancel_attach/TestCancelAttach.py

@@ -0,0 +1,57 @@
+"""
+Test using SendAsyncInterrupt to interrupt an "attach wait"
+"""
+
+import lldb
+import sys
+import time
+import threading
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil
+
+
+class AttachCancelTestCase(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test_scripted_implementation(self):
+        """Test that cancelling a stuck "attach waitfor" works."""
+        # First make an empty target for the attach:
+        target = self.dbg.CreateTarget(None)
+
+        # We need to cancel this, so we need to do the attach
+        # on a separate thread:
+        class AttachThread(threading.Thread):
+            def __init__(self, target, error):
+                # Make this a daemon thread so if we don't manage to interrupt,
+                # Python will keep this thread from hanging the test.
+                threading.Thread.__init__(self, daemon=True)
+                self.target = target
+                self.error = error
+                
+            def run(self):
+                self.target.AttachToProcessWithName(lldb.SBListener(), "LLDB-No-Such-Process", True, self.error)
+                
+        error = lldb.SBError()
+        thread = AttachThread(target, error)
+        thread.start()
+
+        # Now wait till the attach on the child thread has made a process
+        # for the attach attempt:
+        while not target.process.IsValid():
+            time.sleep(1)
+        # I don't have a positive signal for "we started the attach attempt"
+        # so the best I can do is sleep a bit more here to give that a chance
+        # to start:
+        time.sleep(1)
+
+        # Now send the attach interrupt:
+        target.process.SendAsyncInterrupt()
+        # We don't want to stall if we can't interrupt, so join with a timeout:
+        thread.join(60)
+        if thread.is_alive():
+          self.fail("The attach thread is alive after timeout interval")
+
+        # Now check the error, should say the attach was interrupted:
+        self.assertTrue(error.Fail(), "We succeeded in not attaching")
+

lldb/tools/debugserver/source/DNB.cpp

@@ -50,6 +50,7 @@
 #include "MacOSX/MachProcess.h"
 #include "MacOSX/MachTask.h"
 #include "MacOSX/ThreadInfo.h"
+#include "RNBRemote.h"
 
 typedef std::shared_ptr<MachProcess> MachProcessSP;
 typedef std::map<nub_process_t, MachProcessSP> ProcessMap;
@@ -745,7 +746,6 @@ DNBProcessAttachWait(RNBContext *ctx, const char *waitfor_process_name,
         break;
       }
     } else {
-
       // Get the current process list, and check for matches that
       // aren't in our original list. If anyone wants to attach
       // to an existing process by name, they should do it with
@@ -799,7 +799,33 @@ DNBProcessAttachWait(RNBContext *ctx, const char *waitfor_process_name,
         break;
       }
 
-      ::usleep(waitfor_interval); // Sleep for WAITFOR_INTERVAL, then poll again
+      // Now we're going to wait a while before polling again.  But we also
+      // need to check whether we've gotten an event from the debugger  
+      // telling us to interrupt the wait.  So we'll use the wait for a possible
+      // next event to also be our short pause...
+      struct timespec short_timeout;
+      DNBTimer::OffsetTimeOfDay(&short_timeout, 0, waitfor_interval);
+      uint32_t event_mask = RNBContext::event_read_packet_available 
+          | RNBContext::event_read_thread_exiting;
+      nub_event_t set_events = ctx->Events().WaitForSetEvents(event_mask, 
+          &short_timeout);
+      if (set_events & RNBContext::event_read_packet_available) {
+        // If we get any packet from the debugger while waiting on the async,
+        // it has to be telling us to interrupt.  So always exit here.
+        // Over here in DNB land we can see that there was a packet, but all
+        // the methods to actually handle it are protected.  It's not worth
+        // rearranging all that just to get which packet we were sent...
+        DNBLogError("Interrupted by packet while waiting for '%s' to appear.\n",
+                   waitfor_process_name);
+        break;
+      }
+      if (set_events & RNBContext::event_read_thread_exiting) {
+        // The packet thread is shutting down, get out of here...
+        DNBLogError("Interrupted by packet thread shutdown while waiting for "
+                    "%s to appear.\n", waitfor_process_name);
+        break;
+      }
+      
     }
   }