[lldb] [llgs] Fix disabling non-stop mode

[mgorny]

Summary

Stop all processes and clear notification queues when disabling non-stop
mode. Ensure that no stop notifications are sent for processes stopped
due to the mode switch.

Sponsored by: The FreeBSD Foundation

[mgorny]

FYI, I don't feel very strongly about this. If you really dislike adding m_inhibit_stop_reason_processes, I suppose we could alternatively just declare disabling non-stop as unsupported and error out.

[labath]

In D128893#3621673, @mgorny wrote:

FYI, I don't feel very strongly about this. If you really dislike adding m_inhibit_stop_reason_processes, I suppose we could alternatively just declare disabling non-stop as unsupported and error out.

The problem is I "really disliked" the vKill part as well. I didn't want to make a big deal out of it, as I though it was an isolated use case, but now it seems like this may be a regular occurrence.

I think things would look better if we at least grouped all of the various bits of information about a single process. So, instead of three maps, we would have a single map, whose values would contain the process pointer, and any other state data we need. That could be a flag (whether the process is being killed), or maybe a callback (specifying the action to perform when the process stops). WDYT?

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

1922–1924

if (m_inhibit_stop_reason_processes.erase(process.GetID()) != 0)

3951

Should we maybe send this *after* the stop dance is completed?

[mgorny]

In D128893#3647663, @labath wrote:

In D128893#3621673, @mgorny wrote:

FYI, I don't feel very strongly about this. If you really dislike adding m_inhibit_stop_reason_processes, I suppose we could alternatively just declare disabling non-stop as unsupported and error out.

The problem is I "really disliked" the vKill part as well. I didn't want to make a big deal out of it, as I though it was an isolated use case, but now it seems like this may be a regular occurrence.

I think things would look better if we at least grouped all of the various bits of information about a single process. So, instead of three maps, we would have a single map, whose values would contain the process pointer, and any other state data we need. That could be a flag (whether the process is being killed), or maybe a callback (specifying the action to perform when the process stops). WDYT?

I suppose that makes sense.

[mgorny]

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

1922–1924

This is now irrelevant with the flag field ;-).

[mgorny]

Implemented the requested changes.

[labath]

Actually, on second thought (:D) do we even need a per-process flag for this?

Turning off non-stop should stop _all_ processes, so instead of checking whether there are any processes with the 'stopping-due-to-qnonstop` flag around, couldn't we just check if we have any unstopped processes altogether?

(This will require a global qnonstop-in-progress flag, but that's still better than a per-process flag.)

[mgorny]

Hmm, I suppose that makes sense.

[mgorny]

Use a single bool member to indicate whether we're still waiting for any process to stop. Turned out pretty easy.

[labath]

Looks much better. Thanks.

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

protected:
 
   llvm::StringMap<std::unique_ptr<llvm::MemoryBuffer>> m_xfer_buffer_map;
   std::mutex m_saved_registers_mutex;
   std::unordered_map<uint32_t, lldb::DataBufferSP> m_saved_registers_map;
   uint32_t m_next_saved_registers_id = 1;
   bool m_thread_suffix_supported = false;
   bool m_list_threads_in_stop_reply = false;
   bool m_non_stop = false;
+  bool m_disabling_non_stop = false;
   std::deque<std::string> m_stdio_notification_queue;
   std::deque<std::string> m_stop_notification_queue;
 
   NativeProcessProtocol::Extension m_extensions_supported = {};
 
   PacketResult SendONotification(const char *buffer, uint32_t len);
 
   PacketResult SendWResponse(NativeProcessProtocol *process);

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

 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::SendStopReasonForState(
     NativeProcessProtocol &process, lldb::StateType process_state,
     bool force_synchronous) {
   Log *log = GetLog(LLDBLog::Process);
 
+  if (m_disabling_non_stop) {
+    // Check if we are waiting for any more processes to stop.  If we are,
+    // do not send the OK response yet.
+    for (const auto &it : m_debugged_processes) {
+      if (it.second.process_up->IsRunning())
+        return PacketResult::Success;
+    }
+
+    // If all expected processes were stopped after a QNonStop:0 request,
+    // send the OK response.
+    m_disabling_non_stop = false;
+    return SendOKResponse();
+  }
+
   switch (process_state) {
   case eStateAttaching:
   case eStateLaunching:
   case eStateRunning:
   case eStateStepping:
   case eStateDetached:
     // NOTE: gdb protocol doc looks like it should return $OK
     // when everything is running (i.e. no stopped result).
GDBRemoteCommunicationServerLLGS::Handle_qSaveCore(
   response.PutCString("core-path:");
   response.PutStringAsRawHex8(ret.get());
   return SendPacketNoLock(response.GetString());
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_QNonStop(
     StringExtractorGDBRemote &packet) {
+  Log *log = GetLog(LLDBLog::Process);
+
   StringRef packet_str{packet.GetStringRef()};
   assert(packet_str.startswith("QNonStop:"));
   packet_str.consume_front("QNonStop:");
   if (packet_str == "0") {
+    for (auto &process_it : m_debugged_processes) {
+      if (process_it.second.process_up->IsRunning()) {
+        assert(m_non_stop);
+        Status error = process_it.second.process_up->Interrupt();
+        if (error.Fail()) {
+          LLDB_LOG(log,
+                   "while disabling nonstop, failed to halt process {0}: {1}",
+                   process_it.first, error);
+          return SendErrorResponse(0x41);
+        }
+        // we must not send stop reasons after QNonStop
+        m_disabling_non_stop = true;
+      }
+    }
+    m_stdio_notification_queue.clear();
+    m_stop_notification_queue.clear();
     m_non_stop = false;
-    // TODO: stop all threads
+    // If we are stopping anything, defer sending the OK response until we're
+    // done.
+    if (m_disabling_non_stop)
+      return PacketResult::Success;
   } else if (packet_str == "1") {
     m_non_stop = true;
   } else
     return SendErrorResponse(Status("Invalid QNonStop packet"));
   return SendOKResponse();
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::HandleNotificationAck(
     std::deque<std::string> &queue) {
   // Per the protocol, the first message put into the queue is sent
   // immediately.  However, it remains the queue until the client ACKs it --
   // then we pop it and send the next message.  The process repeats until

lldb/test/API/tools/lldb-server/TestNonStop.py

def test_stop_reason_while_running(self):
              "send packet: $OK#00",
              {"direction": "send", "regex": "%Stop:T.*"},
              "read packet: $c#63",
              "send packet: $OK#00",
              "read packet: $?#00",
              "send packet: $OK#00",
              ], True)
         self.expect_gdbremote_sequence()
+
+    @add_test_categories(["llgs"])
+    def test_leave_nonstop(self):
+        self.build()
+        self.set_inferior_startup_launch()
+        procs = self.prep_debug_monitor_and_inferior(
+                inferior_args=["thread:new", "thread:new", "stop", "sleep:15"])
+        self.test_sequence.add_log_lines(
+            ["read packet: $QNonStop:1#00",
+             "send packet: $OK#00",
+             # stop is used to synchronize starting threads
+             "read packet: $c#63",
+             "send packet: $OK#00",
+             {"direction": "send", "regex": "%Stop:T.*"},
+             "read packet: $c#63",
+             "send packet: $OK#00",
+             # verify that the threads are running now
+             "read packet: $?#00",
+             "send packet: $OK#00",
+             "read packet: $QNonStop:0#00",
+             "send packet: $OK#00",
+             # we should issue some random request now to verify that the stub
+             # did not send stop reasons -- we may verify whether notification
+             # queue was cleared while at it
+             "read packet: $vStopped#00",
+             "send packet: $Eff#00",
+             "read packet: $?#00",
+             {"direction": "send", "regex": "[$]T.*"},
+             ], True)
+        self.expect_gdbremote_sequence()