Merge pull request #2972 from jimingham/revert-timeout

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

Author: jimingham

lldb/include/lldb/Target/Process.h

@@ -93,7 +93,6 @@ 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

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

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

@@ -20,10 +20,7 @@ using namespace lldb_private;
 using namespace lldb_private::process_gdb_remote;
 using namespace std::chrono;
 
-// 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);
+static const seconds kInterruptTimeout(5);
 
 /////////////////////////
 // GDBRemoteClientBase //
@@ -38,8 +35,7 @@ GDBRemoteClientBase::GDBRemoteClientBase(const char *comm_name,
 
 StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(
     ContinueDelegate &delegate, const UnixSignals &signals,
-    llvm::StringRef payload, std::chrono::seconds interrupt_timeout,
-    StringExtractorGDBRemote &response) {
+    llvm::StringRef payload, StringExtractorGDBRemote &response) {
   Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
   response.Clear();
 
@@ -52,34 +48,16 @@ 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, computed_timeout, false);
+    PacketResult read_result = ReadPacket(response, kInterruptTimeout, 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;
-      }
-      auto cur_time = steady_clock::now();
-      if (cur_time >= m_interrupt_endpoint)
+      if (steady_clock::now() >= m_interrupt_time + kInterruptTimeout)
         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:
@@ -155,9 +133,8 @@ StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(
   }
 }
 
-bool GDBRemoteClientBase::SendAsyncSignal(
-    int signo, std::chrono::seconds interrupt_timeout) {
-  Lock lock(*this, interrupt_timeout);
+bool GDBRemoteClientBase::SendAsyncSignal(int signo) {
+  Lock lock(*this, true);
   if (!lock || !lock.DidInterrupt())
     return false;
 
@@ -167,26 +144,25 @@ bool GDBRemoteClientBase::SendAsyncSignal(
   return true;
 }
 
-bool GDBRemoteClientBase::Interrupt(std::chrono::seconds interrupt_timeout) {
-  Lock lock(*this, interrupt_timeout);
+bool GDBRemoteClientBase::Interrupt() {
+  Lock lock(*this, true);
   if (!lock.DidInterrupt())
     return false;
   m_should_stop = true;
   return true;
 }
-
 GDBRemoteCommunication::PacketResult
 GDBRemoteClientBase::SendPacketAndWaitForResponse(
     llvm::StringRef payload, StringExtractorGDBRemote &response,
-    std::chrono::seconds interrupt_timeout) {
-  Lock lock(*this, interrupt_timeout);
+    bool send_async) {
+  Lock lock(*this, send_async);
   if (!lock) {
     if (Log *log =
             ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS))
       LLDB_LOGF(log,
                 "GDBRemoteClientBase::%s failed to get mutex, not sending "
-                "packet '%.*s'",
-                __FUNCTION__, int(payload.size()), payload.data());
+                "packet '%.*s' (send_async=%d)",
+                __FUNCTION__, int(payload.size()), payload.data(), send_async);
     return PacketResult::ErrorSendFailed;
   }
 
@@ -196,16 +172,16 @@ GDBRemoteClientBase::SendPacketAndWaitForResponse(
 GDBRemoteCommunication::PacketResult
 GDBRemoteClientBase::SendPacketAndReceiveResponseWithOutputSupport(
     llvm::StringRef payload, StringExtractorGDBRemote &response,
-    std::chrono::seconds interrupt_timeout,
+    bool send_async,
     llvm::function_ref<void(llvm::StringRef)> output_callback) {
-  Lock lock(*this, interrupt_timeout);
+  Lock lock(*this, send_async);
   if (!lock) {
     if (Log *log =
             ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS))
       LLDB_LOGF(log,
                 "GDBRemoteClientBase::%s failed to get mutex, not sending "
-                "packet '%.*s'",
-                __FUNCTION__, int(payload.size()), payload.data());
+                "packet '%.*s' (send_async=%d)",
+                __FUNCTION__, int(payload.size()), payload.data(), send_async);
     return PacketResult::ErrorSendFailed;
   }
 
@@ -246,14 +222,13 @@ GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(
   return packet_result;
 }
 
-bool GDBRemoteClientBase::SendvContPacket(
-    llvm::StringRef payload, std::chrono::seconds interrupt_timeout,
-    StringExtractorGDBRemote &response) {
+bool GDBRemoteClientBase::SendvContPacket(llvm::StringRef payload,
+                                          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, interrupt_timeout);
+  Lock lock(*this, true);
 
   LLDB_LOGF(log,
             "GDBRemoteCommunicationClient::%s () sending vCont packet: %.*s",
@@ -361,20 +336,18 @@ GDBRemoteClientBase::ContinueLock::lock() {
 // GDBRemoteClientBase::Lock //
 ///////////////////////////////
 
-GDBRemoteClientBase::Lock::Lock(GDBRemoteClientBase &comm,
-                                std::chrono::seconds interrupt_timeout)
+GDBRemoteClientBase::Lock::Lock(GDBRemoteClientBase &comm, bool interrupt)
     : m_async_lock(comm.m_async_mutex, std::defer_lock), m_comm(comm),
-      m_interrupt_timeout(interrupt_timeout), m_acquired(false),
-      m_did_interrupt(false) {
-  SyncWithContinueThread();
+      m_acquired(false), m_did_interrupt(false) {
+  SyncWithContinueThread(interrupt);
   if (m_acquired)
     m_async_lock.lock();
 }
 
-void GDBRemoteClientBase::Lock::SyncWithContinueThread() {
+void GDBRemoteClientBase::Lock::SyncWithContinueThread(bool interrupt) {
   Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
   std::unique_lock<std::mutex> lock(m_comm.m_mutex);
-  if (m_comm.m_is_running && m_interrupt_timeout == std::chrono::seconds(0))
+  if (m_comm.m_is_running && !interrupt)
     return; // We were asked to avoid interrupting the sender. Lock is not
             // acquired.
 
@@ -392,9 +365,9 @@ void GDBRemoteClientBase::Lock::SyncWithContinueThread() {
                        "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,46 +33,29 @@ class GDBRemoteClientBase : public GDBRemoteCommunication {
 
   GDBRemoteClientBase(const char *comm_name, const char *listener_name);
 
-  bool SendAsyncSignal(int signo, std::chrono::seconds interrupt_timeout);
+  bool SendAsyncSignal(int signo);
 
-  bool Interrupt(std::chrono::seconds interrupt_timeout);
+  bool Interrupt();
 
   lldb::StateType SendContinuePacketAndWaitForResponse(
       ContinueDelegate &delegate, const UnixSignals &signals,
-      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));
+      llvm::StringRef payload, StringExtractorGDBRemote &response);
+
+  PacketResult SendPacketAndWaitForResponse(llvm::StringRef payload,
+                                            StringExtractorGDBRemote &response,
+                                            bool send_async);
 
   PacketResult SendPacketAndReceiveResponseWithOutputSupport(
       llvm::StringRef payload, StringExtractorGDBRemote &response,
-      std::chrono::seconds interrupt_timeout,
+      bool send_async,
       llvm::function_ref<void(llvm::StringRef)> output_callback);
 
   bool SendvContPacket(llvm::StringRef payload,
-                       std::chrono::seconds interrupt_timeout,
                        StringExtractorGDBRemote &response);
 
   class Lock {
   public:
-    // 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(GDBRemoteClientBase &comm, bool interrupt);
     ~Lock();
 
     explicit operator bool() { return m_acquired; }
@@ -84,11 +67,10 @@ 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();
+    void SyncWithContinueThread(bool interrupt);
   };
 
 protected:
@@ -127,7 +109,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_endpoint;
+  std::chrono::time_point<std::chrono::steady_clock> m_interrupt_time;
 
   /// Number of threads interested in sending.
   uint32_t m_async_count;