Add an "interrupt timeout" to Process, and pipe that through the

ProcessGDBRemote plugin layers.

Also fix a bug where if we tried to interrupt, but the ReadPacket
wakeup timer woke us up just after the timeout, we would break out
the switch, but then since we immediately check if the response is
empty & fail if it is, we could end up actually only giving a
small interval to the interrupt.

Differential Revision: https://reviews.llvm.org/D102085

Author: jimingham

lldb/include/lldb/Target/Process.h

@@ -94,6 +94,7 @@ class ProcessProperties : public Properties {
   bool GetWarningsUnsupportedLanguage() const;
   bool GetStopOnExec() const;
   std::chrono::seconds GetUtilityExpressionTimeout() const;
+  std::chrono::seconds GetInterruptTimeout() const;
   bool GetOSPluginReportsAllThreads() const;
   void SetOSPluginReportsAllThreads(bool does_report);
   bool GetSteppingRunsAllThreads() const;

lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp

@@ -740,8 +740,8 @@ const UnixSignalsSP &PlatformRemoteGDBServer::GetRemoteUnixSignals() {
   m_remote_signals_sp = UnixSignals::Create(GetRemoteSystemArchitecture());
 
   StringExtractorGDBRemote response;
-  auto result = m_gdb_client.SendPacketAndWaitForResponse("jSignalsInfo",
-                                                          response, false);
+  auto result =
+      m_gdb_client.SendPacketAndWaitForResponse("jSignalsInfo", response);
 
   if (result != decltype(result)::Success ||
       response.GetResponseType() != response.eResponse)

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

@@ -20,7 +20,10 @@ using namespace lldb_private;
 using namespace lldb_private::process_gdb_remote;
 using namespace std::chrono;
 
-static const seconds kInterruptTimeout(5);
+// When we've sent a continue packet and are waiting for the target to stop,
+// we wake up the wait with this interval to make sure the stub hasn't gone
+// away while we were waiting.
+static const seconds kWakeupInterval(5);
 
 /////////////////////////
 // GDBRemoteClientBase //
@@ -35,7 +38,8 @@ GDBRemoteClientBase::GDBRemoteClientBase(const char *comm_name,
 
 StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(
     ContinueDelegate &delegate, const UnixSignals &signals,
-    llvm::StringRef payload, StringExtractorGDBRemote &response) {
+    llvm::StringRef payload, std::chrono::seconds interrupt_timeout,
+    StringExtractorGDBRemote &response) {
   Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
   response.Clear();
 
@@ -48,16 +52,34 @@ StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(
   if (!cont_lock)
     return eStateInvalid;
   OnRunPacketSent(true);
-
+  // The main ReadPacket loop wakes up at computed_timeout intervals, just to 
+  // check that the connection hasn't dropped.  When we wake up we also check
+  // whether there is an interrupt request that has reached its endpoint.
+  // If we want a shorter interrupt timeout that kWakeupInterval, we need to 
+  // choose the shorter interval for the wake up as well.
+  std::chrono::seconds computed_timeout = std::min(interrupt_timeout, 
+                                                   kWakeupInterval);
   for (;;) {
-    PacketResult read_result = ReadPacket(response, kInterruptTimeout, false);
+    PacketResult read_result = ReadPacket(response, computed_timeout, false);
     switch (read_result) {
     case PacketResult::ErrorReplyTimeout: {
       std::lock_guard<std::mutex> lock(m_mutex);
-      if (m_async_count == 0)
+      if (m_async_count == 0) {
         continue;
-      if (steady_clock::now() >= m_interrupt_time + kInterruptTimeout)
+      }
+      auto cur_time = steady_clock::now();
+      if (cur_time >= m_interrupt_endpoint)
         return eStateInvalid;
+      else {
+        // We woke up and found an interrupt is in flight, but we haven't
+        // exceeded the interrupt wait time.  So reset the wait time to the
+        // time left till the interrupt timeout.  But don't wait longer
+        // than our wakeup timeout.
+        auto new_wait = m_interrupt_endpoint - cur_time;
+        computed_timeout = std::min(kWakeupInterval,
+            std::chrono::duration_cast<std::chrono::seconds>(new_wait));
+        continue;
+      }
       break;
     }
     case PacketResult::Success:
@@ -133,8 +155,9 @@ StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(
   }
 }
 
-bool GDBRemoteClientBase::SendAsyncSignal(int signo) {
-  Lock lock(*this, true);
+bool GDBRemoteClientBase::SendAsyncSignal(
+    int signo, std::chrono::seconds interrupt_timeout) {
+  Lock lock(*this, interrupt_timeout);
   if (!lock || !lock.DidInterrupt())
     return false;
 
@@ -144,25 +167,26 @@ bool GDBRemoteClientBase::SendAsyncSignal(int signo) {
   return true;
 }
 
-bool GDBRemoteClientBase::Interrupt() {
-  Lock lock(*this, true);
+bool GDBRemoteClientBase::Interrupt(std::chrono::seconds interrupt_timeout) {
+  Lock lock(*this, interrupt_timeout);
   if (!lock.DidInterrupt())
     return false;
   m_should_stop = true;
   return true;
 }
+
 GDBRemoteCommunication::PacketResult
 GDBRemoteClientBase::SendPacketAndWaitForResponse(
     llvm::StringRef payload, StringExtractorGDBRemote &response,
-    bool send_async) {
-  Lock lock(*this, send_async);
+    std::chrono::seconds interrupt_timeout) {
+  Lock lock(*this, interrupt_timeout);
   if (!lock) {
     if (Log *log =
             ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS))
       LLDB_LOGF(log,
                 "GDBRemoteClientBase::%s failed to get mutex, not sending "
-                "packet '%.*s' (send_async=%d)",
-                __FUNCTION__, int(payload.size()), payload.data(), send_async);
+                "packet '%.*s'",
+                __FUNCTION__, int(payload.size()), payload.data());
     return PacketResult::ErrorSendFailed;
   }
 
@@ -172,16 +196,16 @@ GDBRemoteClientBase::SendPacketAndWaitForResponse(
 GDBRemoteCommunication::PacketResult
 GDBRemoteClientBase::SendPacketAndReceiveResponseWithOutputSupport(
     llvm::StringRef payload, StringExtractorGDBRemote &response,
-    bool send_async,
+    std::chrono::seconds interrupt_timeout,
     llvm::function_ref<void(llvm::StringRef)> output_callback) {
-  Lock lock(*this, send_async);
+  Lock lock(*this, interrupt_timeout);
   if (!lock) {
     if (Log *log =
             ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS))
       LLDB_LOGF(log,
                 "GDBRemoteClientBase::%s failed to get mutex, not sending "
-                "packet '%.*s' (send_async=%d)",
-                __FUNCTION__, int(payload.size()), payload.data(), send_async);
+                "packet '%.*s'",
+                __FUNCTION__, int(payload.size()), payload.data());
     return PacketResult::ErrorSendFailed;
   }
 
@@ -222,13 +246,14 @@ GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(
   return packet_result;
 }
 
-bool GDBRemoteClientBase::SendvContPacket(llvm::StringRef payload,
-                                          StringExtractorGDBRemote &response) {
+bool GDBRemoteClientBase::SendvContPacket(
+    llvm::StringRef payload, std::chrono::seconds interrupt_timeout,
+    StringExtractorGDBRemote &response) {
   Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
   LLDB_LOGF(log, "GDBRemoteCommunicationClient::%s ()", __FUNCTION__);
 
   // we want to lock down packet sending while we continue
-  Lock lock(*this, true);
+  Lock lock(*this, interrupt_timeout);
 
   LLDB_LOGF(log,
             "GDBRemoteCommunicationClient::%s () sending vCont packet: %.*s",
@@ -336,18 +361,20 @@ GDBRemoteClientBase::ContinueLock::lock() {
 // GDBRemoteClientBase::Lock //
 ///////////////////////////////
 
-GDBRemoteClientBase::Lock::Lock(GDBRemoteClientBase &comm, bool interrupt)
+GDBRemoteClientBase::Lock::Lock(GDBRemoteClientBase &comm,
+                                std::chrono::seconds interrupt_timeout)
     : m_async_lock(comm.m_async_mutex, std::defer_lock), m_comm(comm),
-      m_acquired(false), m_did_interrupt(false) {
-  SyncWithContinueThread(interrupt);
+      m_interrupt_timeout(interrupt_timeout), m_acquired(false),
+      m_did_interrupt(false) {
+  SyncWithContinueThread();
   if (m_acquired)
     m_async_lock.lock();
 }
 
-void GDBRemoteClientBase::Lock::SyncWithContinueThread(bool interrupt) {
+void GDBRemoteClientBase::Lock::SyncWithContinueThread() {
   Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
   std::unique_lock<std::mutex> lock(m_comm.m_mutex);
-  if (m_comm.m_is_running && !interrupt)
+  if (m_comm.m_is_running && m_interrupt_timeout == std::chrono::seconds(0))
     return; // We were asked to avoid interrupting the sender. Lock is not
             // acquired.
 
@@ -365,9 +392,9 @@ void GDBRemoteClientBase::Lock::SyncWithContinueThread(bool interrupt) {
                        "interrupt packet");
         return;
       }
+      m_comm.m_interrupt_endpoint = steady_clock::now() + m_interrupt_timeout;
       if (log)
         log->PutCString("GDBRemoteClientBase::Lock::Lock sent packet: \\x03");
-      m_comm.m_interrupt_time = steady_clock::now();
     }
     m_comm.m_cv.wait(lock, [this] { return !m_comm.m_is_running; });
     m_did_interrupt = true;

lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h

@@ -33,29 +33,46 @@ class GDBRemoteClientBase : public GDBRemoteCommunication {
 
   GDBRemoteClientBase(const char *comm_name, const char *listener_name);
 
-  bool SendAsyncSignal(int signo);
+  bool SendAsyncSignal(int signo, std::chrono::seconds interrupt_timeout);
 
-  bool Interrupt();
+  bool Interrupt(std::chrono::seconds interrupt_timeout);
 
   lldb::StateType SendContinuePacketAndWaitForResponse(
       ContinueDelegate &delegate, const UnixSignals &signals,
-      llvm::StringRef payload, StringExtractorGDBRemote &response);
-
-  PacketResult SendPacketAndWaitForResponse(llvm::StringRef payload,
-                                            StringExtractorGDBRemote &response,
-                                            bool send_async);
+      llvm::StringRef payload, std::chrono::seconds interrupt_timeout,
+      StringExtractorGDBRemote &response);
+
+  // If interrupt_timeout == 0 seconds, don't interrupt the target.
+  // Only send the packet if the target is stopped.
+  // If you want to use this mode, use the fact that the timeout is defaulted
+  // so it's clear from the call-site that you are using no-interrupt.
+  // If it is non-zero, interrupt the target if it is running, and
+  // send the packet.
+  // It the target doesn't respond within the given timeout, it returns
+  // ErrorReplyTimeout.
+  PacketResult SendPacketAndWaitForResponse(
+      llvm::StringRef payload, StringExtractorGDBRemote &response,
+      std::chrono::seconds interrupt_timeout = std::chrono::seconds(0));
 
   PacketResult SendPacketAndReceiveResponseWithOutputSupport(
       llvm::StringRef payload, StringExtractorGDBRemote &response,
-      bool send_async,
+      std::chrono::seconds interrupt_timeout,
       llvm::function_ref<void(llvm::StringRef)> output_callback);
 
   bool SendvContPacket(llvm::StringRef payload,
+                       std::chrono::seconds interrupt_timeout,
                        StringExtractorGDBRemote &response);
 
   class Lock {
   public:
-    Lock(GDBRemoteClientBase &comm, bool interrupt);
+    // If interrupt_timeout == 0 seconds, only take the lock if the target is
+    // not running. If using this option, use the fact that the
+    // interrupt_timeout is defaulted so it will be obvious at the call site
+    // that you are choosing this mode. If it is non-zero, interrupt the target
+    // if it is running, waiting for the given timeout for the interrupt to
+    // succeed.
+    Lock(GDBRemoteClientBase &comm,
+         std::chrono::seconds interrupt_timeout = std::chrono::seconds(0));
     ~Lock();
 
     explicit operator bool() { return m_acquired; }
@@ -67,10 +84,11 @@ class GDBRemoteClientBase : public GDBRemoteCommunication {
   private:
     std::unique_lock<std::recursive_mutex> m_async_lock;
     GDBRemoteClientBase &m_comm;
+    std::chrono::seconds m_interrupt_timeout;
     bool m_acquired;
     bool m_did_interrupt;
 
-    void SyncWithContinueThread(bool interrupt);
+    void SyncWithContinueThread();
   };
 
 protected:
@@ -109,7 +127,7 @@ class GDBRemoteClientBase : public GDBRemoteCommunication {
 
   /// When was the interrupt packet sent. Used to make sure we time out if the
   /// stub does not respond to interrupt requests.
-  std::chrono::time_point<std::chrono::steady_clock> m_interrupt_time;
+  std::chrono::time_point<std::chrono::steady_clock> m_interrupt_endpoint;
 
   /// Number of threads interested in sending.
   uint32_t m_async_count;