[Lldb-commits] [lldb] Send an explicit interrupt to cancel an attach waitfor. (PR #72565)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: None (jimingham)

<details>
<summary>Changes</summary>

Currently when you interrupt a:

(lldb) process attach -w -n some_process

lldb just closes the connection to the stub and kills the 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, that signal might not be reliable, causing debugserver to persist on the machine.

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.

---
Full diff: https://github.com/llvm/llvm-project/pull/72565.diff


3 Files Affected:

- (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+4-2) 
- (modified) lldb/source/Target/Process.cpp (+8-1) 
- (modified) lldb/tools/debugserver/source/DNB.cpp (+28-2) 


``````````diff
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index f90a561aae2e3b7..b6fe1bd7104b280 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -2372,8 +2372,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());
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 21b80b8240ab64b..f3da2839e262e23 100644
--- a/lldb/source/Target/Process.cpp
+++ b/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,
diff --git a/lldb/tools/debugserver/source/DNB.cpp b/lldb/tools/debugserver/source/DNB.cpp
index f6c1130fdd8054c..0ec50df42d1fedc 100644
--- a/lldb/tools/debugserver/source/DNB.cpp
+++ b/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;
+      }
+      
     }
   }
 

``````````

</details>


https://github.com/llvm/llvm-project/pull/72565