[lldb] [llgs] Improve stdio forwarding in multiprocess+nonstop

[mgorny]

Summary

Enable stdio forwarding when nonstop mode is enabled, and disable it
once it is disabled. This makes it possible to cleanly handle stdio
forwarding while running multiple processes in non-stop mode.

Sponsored by: The FreeBSD Foundation

[mgorny]

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

119

I really dislike these sleeps but I can't think of a better way of doing it (short of using IPC for synchronization). The goal is to 1) ensure that both processes start before they start outputting, and 2) ensure that both output before the first stop reason comes.

[mgorny]

Use semaphores to sync instead of relying on ugly sleeps.

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

119

Ok, semaphores are not scary after all, and I suppose we can expect them to work if we expect fork() to work.

[mgorny]

Fix sem_destroy() assertions.

[labath]

I have a feeling the semaphores will not work (compile) on darwin. I didn't find any sem_init call there -- just sem_open. Maybe instead of semaphores we could use files for the synchronization? Something similar to the wait_for_file_on_target function, just in the other direction. Then the test could create a file to move the processes forward? (This might be easier to achieve with a dedicated inferior, instead of trying to fit it into the universal LLGS inferior.)

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

1116–1117

In the multiprocess all-stop mode, if one process stops, we are supposed to stop all of them, right? I take it that's not something we do right now, is it?

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

119

ensure that both processes start before they start outputting

But we should be able to see the output from the first process (if it had any) even it is the only process running. Do we have a test for that? Could you add a step where the first process outputs something before it waits to synchronize with the second process?

[mgorny]

In D128932#3631903, @labath wrote:

I have a feeling the semaphores will not work (compile) on darwin. I didn't find any sem_init call there -- just sem_open. Maybe instead of semaphores we could use files for the synchronization? Something similar to the wait_for_file_on_target function, just in the other direction. Then the test could create a file to move the processes forward? (This might be easier to achieve with a dedicated inferior, instead of trying to fit it into the universal LLGS inferior.)

Damn, and I thought POSIX actually means something to Darwin. Yeah, I'll look into using some other synchronization mechanism.

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

1116–1117

We don't support resuming multiple processes in all-stop mode. vCont returns an error if you try to do that, and other continue packets are blocking, so it can't happen ;-).

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

119

Yes, I suppose covering that with a test also makes sense.

[mgorny]

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

119

Actually, this is already covered by TestNonStop.py::test_stdio.

[labath]

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

1116–1117

ok, got it.

[mgorny]

Replace semaphores with completely non-fancy file-based locking.

[labath]

Ok, let's give this a shot.

lldb/test/API/tools/lldb-server/main.cpp

351

I'm not convinced this will be enough. How about 125 * (1<<i) ?

[mgorny]

lldb/test/API/tools/lldb-server/main.cpp

351

Sure, I just tried to keep it simple ;-).

[mgorny]

Rebased.

[labath]

Flake here: https://lab.llvm.org/buildbot/#/builders/68/builds/36967.

Presumably the same problem that cd18e2ea3f4e87f8804a7d6661d5596ef1f07b81 fixed for TestNonStop.

[mgorny]

In D128932#3690798, @labath wrote:

Flake here: https://lab.llvm.org/buildbot/#/builders/68/builds/36967.

Presumably the same problem that cd18e2ea3f4e87f8804a7d6661d5596ef1f07b81 fixed for TestNonStop.

Thanks for the ping and the suggestion. I'm testing a fix right now and will push if it works ;-).

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

case StateType::eStateRunning:
     break;
 
   case StateType::eStateStopped:
     // Make sure we get all of the pending stdout/stderr from the inferior and
     // send it to the lldb host before we send the state change notification
     SendProcessOutput();
     // Then stop the forwarding, so that any late output (see llvm.org/pr25652)
     // does not interfere with our protocol.
+    if (!m_non_stop)
       StopSTDIOForwarding();
     HandleInferiorState_Stopped(process);
     break;
 
   case StateType::eStateExited:
     // Same as above
     SendProcessOutput();
+    if (!m_non_stop)
       StopSTDIOForwarding();
     HandleInferiorState_Exited(process);
     break;
 
   default:
     if (log) {
       LLDB_LOGF(log,
                 "GDBRemoteCommunicationServerLLGS::%s didn't handle state "
                 "change for pid %" PRIu64 ", new state: %s",
GDBRemoteCommunicationServerLLGS::Handle_qC(StringExtractorGDBRemote &packet) {
 
   return SendPacketNoLock(response.GetString());
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_k(StringExtractorGDBRemote &packet) {
   Log *log = GetLog(LLDBLog::Process);
 
+  if (!m_non_stop)
     StopSTDIOForwarding();
 
   if (m_debugged_processes.empty()) {
     LLDB_LOG(log, "No debugged process found.");
     return PacketResult::Success;
   }
 
   for (auto it = m_debugged_processes.begin(); it != m_debugged_processes.end();
        ++it) {
GDBRemoteCommunicationServerLLGS::Handle_k(StringExtractorGDBRemote &packet) {
   // in all-stop mode, and "OK" in non-stop mode; in both cases this
   // is followed by the actual stop reason.
   return SendContinueSuccessResponse();
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_vKill(
     StringExtractorGDBRemote &packet) {
+  if (!m_non_stop)
     StopSTDIOForwarding();
 
   packet.SetFilePos(6); // vKill;
   uint32_t pid = packet.GetU32(LLDB_INVALID_PROCESS_ID, 16);
   if (pid == LLDB_INVALID_PROCESS_ID)
     return SendIllFormedResponse(packet,
                                  "vKill failed to parse the process id");
 
   auto it = m_debugged_processes.find(pid);
if (!argv.empty()) {
     LLDB_LOG(log, "failed to launch exe: {0}", m_process_launch_error);
   }
   return SendErrorResponse(8);
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_D(StringExtractorGDBRemote &packet) {
   Log *log = GetLog(LLDBLog::Process);
+  if (!m_non_stop)
     StopSTDIOForwarding();
 
   lldb::pid_t pid = LLDB_INVALID_PROCESS_ID;
 
   // Consume the ';' after D.
   packet.SetFilePos(1);
   if (packet.GetBytesLeft()) {
     if (packet.GetChar() != ';')
       return SendIllFormedResponse(packet, "D missing expected ';'");

 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") {
+    if (m_non_stop)
+      StopSTDIOForwarding();
     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;
     // 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") {
+    if (!m_non_stop)
+      StartSTDIOForwarding();
     m_non_stop = true;
   } else
     return SendErrorResponse(Status("Invalid QNonStop packet"));
   return SendOKResponse();
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::HandleNotificationAck(
void GDBRemoteCommunicationServerLLGS::SetEnabledExtensions(
     NativeProcessProtocol &process) {
   NativeProcessProtocol::Extension flags = m_extensions_supported;
   assert(!bool(flags & ~m_process_factory.GetSupportedExtensions()));
   process.SetEnabledExtensions(flags);
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::SendContinueSuccessResponse() {
-  // TODO: how to handle forwarding in non-stop mode?
+  if (m_non_stop)
+    return SendOKResponse();
   StartSTDIOForwarding();
-  return m_non_stop ? SendOKResponse() : PacketResult::Success;
+  return PacketResult::Success;
 }
 
 void GDBRemoteCommunicationServerLLGS::AppendThreadIDToResponse(
     Stream &response, lldb::pid_t pid, lldb::tid_t tid) {
   if (bool(m_extensions_supported &
            NativeProcessProtocol::Extension::multiprocess))
     response.Format("p{0:x-}.", pid);
   response.Format("{0:x-}", tid);

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

class TestGdbRemoteForkNonStop(GdbRemoteForkTestBase):
     def test_c_interspersed_nonstop(self):
         self.resume_one_test(run_order=["parent", "child", "parent", "child"],
                              nonstop=True)
 
     @add_test_categories(["fork"])
     def test_vCont_interspersed_nonstop(self):
         self.resume_one_test(run_order=["parent", "child", "parent", "child"],
                              use_vCont=True, nonstop=True)
+
+    @add_test_categories(["fork"])
+    def test_c_both_nonstop(self):
+        lock1 = self.getBuildArtifact("lock1")
+        lock2 = self.getBuildArtifact("lock2")
+        parent_pid, parent_tid, child_pid, child_tid = (
+            self.start_fork_test(["fork", "process:sync:" + lock1, "print-pid",
+                                  "process:sync:" + lock2, "stop"],
+                                 nonstop=True))
+
+        self.test_sequence.add_log_lines([
+            "read packet: $Hcp{}.{}#00".format(parent_pid, parent_tid),
+            "send packet: $OK#00",
+            "read packet: $c#00",
+            "send packet: $OK#00",
+            "read packet: $Hcp{}.{}#00".format(child_pid, child_tid),
+            "send packet: $OK#00",
+            "read packet: $c#00",
+            "send packet: $OK#00",
+            {"direction": "send", "regex": "%Stop:T.*"},
+            # see the comment in TestNonStop.py, test_stdio
+            "read packet: $vStdio#00",
+            "read packet: $vStdio#00",
+            "send packet: $OK#00",
+            ], True)
+        ret = self.expect_gdbremote_sequence()
+        self.assertIn("PID: {}".format(int(parent_pid, 16)).encode(),
+                      ret["O_content"])
+        self.assertIn("PID: {}".format(int(child_pid, 16)).encode(),
+                      ret["O_content"])

lldb/test/API/tools/lldb-server/main.cpp

 int main(int argc, char **argv) {
   lldb_enable_attach();
 
   std::vector<std::thread> threads;
   std::unique_ptr<uint8_t[]> heap_array_up;
   int return_value = 0;
 
 #if !defined(_WIN32)
+  bool is_child = false;
+
   // Set the signal handler.
   sig_t sig_result = signal(SIGALRM, signal_handler);
   if (sig_result == SIG_ERR) {
     fprintf(stderr, "failed to set SIGALRM signal handler: errno=%d\n", errno);
     exit(1);
   }
 
   sig_result = signal(SIGUSR1, signal_handler);
if (consume_front(arg, "stderr:")) {
 
       if (arg == "hello")
         func_p = hello;
       else if (arg == "swap_chars")
         func_p = swap_chars;
       func_p();
 #if !defined(_WIN32) && !defined(TARGET_OS_WATCH) && !defined(TARGET_OS_TV)
     } else if (arg == "fork") {
-      assert (fork() != -1);
+      pid_t fork_pid = fork();
+      assert(fork_pid != -1);
+      is_child = fork_pid == 0;
     } else if (arg == "vfork") {
       if (vfork() == 0)
         _exit(0);
+    } else if (consume_front(arg, "process:sync:")) {
+      // this is only valid after fork
+      const char *filenames[] = {"parent", "child"};
+      std::string my_file = arg + "." + filenames[is_child];
+      std::string other_file = arg + "." + filenames[!is_child];
+
+      // indicate that we're ready
+      FILE *f = fopen(my_file.c_str(), "w");
+      assert(f);
+      fclose(f);
+
+      // wait for the other process to be ready
+      for (int i = 0; i < 5; ++i) {
+        f = fopen(other_file.c_str(), "r");
+        if (f)
+          break;
+        std::this_thread::sleep_for(std::chrono::milliseconds(125 * (1<<i)));
+      }
+      assert(f);
+      fclose(f);
 #endif
     } else if (consume_front(arg, "thread:new")) {
       std::promise<void> promise;
       std::future<void> ready = promise.get_future();
       threads.push_back(std::thread(thread_func, std::move(promise)));
       ready.wait();
     } else if (consume_front(arg, "thread:print-ids")) {
       // Turn on thread id announcing.