[lldb] [gdb-remote] Send interrupt packets from async thread

[mgorny]

Summary

Refactor the mechanism for sending interrupt packets to send them
from async thread (i.e. the same thread that sends the continue packet
preceding them and that waits for the response), rather than from
the thread requesting the interrupt. This is going to become especially
important when using the vCtrlC packet as part of the non-stop protocol,
as -- unlike the simple ^c sent in the all-stop mode -- this packet
involves an explicit reply.

Suggested by Pavel Labath in D126614.

Sponsored by: The FreeBSD Foundation

[labath]

Looks good. Please pay special attention to the windows bot when landing.

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

55–65

I think this could be simplified now (to avoid the intermittent wakeups), but those wakeups could be the only thing that makes windows work...

[mgorny]

In D131075#3696782, @labath wrote:

Looks good. Please pay special attention to the windows bot when landing.

Thanks. Will do.

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

55–65

If Windows relies on this, we'll soon find out by seeing a few tests failing due to interrupts taking too long (I've seen this myself prior to adding the InterruptRead() call).

[mgorny]

Yep, you were right. It broke on Windows ;-).

https://lab.llvm.org/buildbot/#/builders/83/builds/22075

Failed Tests (6):
  lldb-unit :: Process/gdb-remote/./ProcessGdbRemoteTests.exe/GDBRemoteClientBaseTest/InterruptNoResponse
  lldb-unit :: Process/gdb-remote/./ProcessGdbRemoteTests.exe/GDBRemoteClientBaseTest/SendContinueAndAsyncPacket
  lldb-unit :: Process/gdb-remote/./ProcessGdbRemoteTests.exe/GDBRemoteClientBaseTest/SendContinueAndAsyncSignal
  lldb-unit :: Process/gdb-remote/./ProcessGdbRemoteTests.exe/GDBRemoteClientBaseTest/SendContinueAndInterrupt
  lldb-unit :: Process/gdb-remote/./ProcessGdbRemoteTests.exe/GDBRemoteClientBaseTest/SendContinueAndInterrupt2PacketBug
  lldb-unit :: Process/gdb-remote/./ProcessGdbRemoteTests.exe/GDBRemoteClientBaseTest/SendContinueAndLateInterrupt

[labath]

Yeah, I can't say I'm surprised by that. It's fairly annoying that such basic functionality is missing there. Let me see what I can do about that.

[labath]

Ok, I was able to concoct D131159 and D131160 this morning. They need some more cleanup, but I tried them on windows, and it worked. The general idea is to add a platform-neutral way to wait on "events" and FDs (unfortunately, this is still limited to sockets on windows) at the same time. This could then be used instead of the pipe+select interruption implementation in the Connection class.

[emaste]

With D131159 and D131160 in, where does that leave us wrt this change?

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

private:
   /// @{
   std::mutex m_mutex;
   std::condition_variable m_cv;
 
   /// Packet with which to resume after an async interrupt. Can be changed by
   /// an async thread e.g. to inject a signal.
   std::string m_continue_packet;
 
-  /// 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;
-
   /// Number of threads interested in sending.
   uint32_t m_async_count;
 
   /// Whether the continue thread has control.
   bool m_is_running;
 
   /// Whether we should resume after a stop.
   bool m_should_stop;

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

 //===-- GDBRemoteClientBase.cpp -------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "GDBRemoteClientBase.h"
 
 #include "llvm/ADT/StringExtras.h"
 
 #include "lldb/Target/UnixSignals.h"
+#include "lldb/Utility/Connection.h"
 #include "lldb/Utility/LLDBAssert.h"
 
 #include "ProcessGDBRemoteLog.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::process_gdb_remote;
 using namespace std::chrono;
StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(
   {
     std::lock_guard<std::mutex> lock(m_mutex);
     m_continue_packet = std::string(payload);
     m_should_stop = false;
   }
   ContinueLock cont_lock(*this);
   if (!cont_lock)
     return eStateInvalid;
   OnRunPacketSent(true);
-  // The main ReadPacket loop wakes up at computed_timeout intervals, just to 
+  // 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 
+  // 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);
+  std::chrono::seconds computed_timeout =
+      std::min(interrupt_timeout, kWakeupInterval);
+  std::chrono::time_point<std::chrono::steady_clock> interrupt_endpoint;
+  bool interrupt_sent = false;
   for (;;) {
     PacketResult read_result = ReadPacket(response, computed_timeout, false);
     // Reset the computed_timeout to the default value in case we are going
     // round again.
     computed_timeout = std::min(interrupt_timeout, kWakeupInterval);
     switch (read_result) {
     case PacketResult::ErrorReplyTimeout: {
       std::lock_guard<std::mutex> lock(m_mutex);
       if (m_async_count == 0) {
         continue;
       }
+      if (!interrupt_sent) {
+        const char ctrl_c = '\x03';
+        ConnectionStatus status = eConnectionStatusSuccess;
+        size_t bytes_written = Write(&ctrl_c, 1, status, nullptr);
+        if (bytes_written == 0) {
+          LLDB_LOG(log, "failed to send interrupt packet");
+          return eStateInvalid;
+        }
+        interrupt_endpoint = steady_clock::now() + interrupt_timeout;
+        if (log)
+          log->PutCString(
+              "GDBRemoteClientBase::SendContinuePacketAndWaitForResponse sent "
+              "packet: \\x03");
+
+        interrupt_sent = true;
+        continue;
+      }
+
       auto cur_time = steady_clock::now();
-      if (cur_time >= m_interrupt_endpoint)
+      if (cur_time >= 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,
+        auto new_wait = interrupt_endpoint - cur_time;
+        computed_timeout = std::min(
+            kWakeupInterval,
             std::chrono::duration_cast<std::chrono::seconds>(new_wait));
         continue;
       }
       break;
     }
     case PacketResult::Success:
       break;
     default:
: 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();
   if (m_acquired)
     m_async_lock.lock();
 }
 
 void GDBRemoteClientBase::Lock::SyncWithContinueThread() {
-  Log *log = GetLog(GDBRLog::Process|GDBRLog::Packets);
   std::unique_lock<std::mutex> lock(m_comm.m_mutex);
   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.
 
   ++m_comm.m_async_count;
   if (m_comm.m_is_running) {
-    if (m_comm.m_async_count == 1) {
-      // The sender has sent the continue packet and we are the first async
-      // packet. Let's interrupt it.
-      const char ctrl_c = '\x03';
-      ConnectionStatus status = eConnectionStatusSuccess;
-      size_t bytes_written = m_comm.Write(&ctrl_c, 1, status, nullptr);
-      if (bytes_written == 0) {
-        --m_comm.m_async_count;
-        LLDB_LOGF(log, "GDBRemoteClientBase::Lock::Lock failed to send "
-                       "interrupt packet");
-        return;
-      }
-      m_comm.m_interrupt_endpoint = steady_clock::now() + m_interrupt_timeout;
-      if (log)
-        log->PutCString("GDBRemoteClientBase::Lock::Lock sent packet: \\x03");
-    }
+    // SendContinuePacketAndWaitForResponse() takes care of sending
+    // the actual interrupt packet since we've increased m_async_count.
+    // Interrupt the ReadPacket() call to avoid having to wait for
+    // the interrupt timeout.
+    m_comm.GetConnection()->InterruptRead();
     m_comm.m_cv.wait(lock, [this] { return !m_comm.m_is_running; });
     m_did_interrupt = true;
   }
   m_acquired = true;
 }
 
 GDBRemoteClientBase::Lock::~Lock() {
   if (!m_acquired)
     return;
   {
     std::unique_lock<std::mutex> lock(m_comm.m_mutex);
     --m_comm.m_async_count;
   }
   m_comm.m_cv.notify_one();
 }