[lldb-dap] Refactor lldb-dap event handling.

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py

@@ -7,6 +7,7 @@
 import pprint
 import socket
 import string
+import signal
 import subprocess
 import sys
 import threading
@@ -97,37 +98,15 @@ def dump_dap_log(log_file):
     print("========= END =========", file=sys.stderr)
 
 
-def read_packet_thread(vs_comm, log_file):
-    done = False
-    try:
-        while not done:
-            packet = read_packet(vs_comm.recv, trace_file=vs_comm.trace_file)
-            # `packet` will be `None` on EOF. We want to pass it down to
-            # handle_recv_packet anyway so the main thread can handle unexpected
-            # termination of lldb-dap and stop waiting for new packets.
-            done = not vs_comm.handle_recv_packet(packet)
-    finally:
-        # Wait for the process to fully exit before dumping the log file to
-        # ensure we have the entire log contents.
-        if vs_comm.process is not None:
-            try:
-                # Do not wait forever, some logs are better than none.
-                vs_comm.process.wait(timeout=20)
-            except subprocess.TimeoutExpired:
-                pass
-        dump_dap_log(log_file)
-
-
 class DebugCommunication(object):
     def __init__(self, recv, send, init_commands, log_file=None):
+        self.log_file = log_file
         self.trace_file = None
         self.send = send
         self.recv = recv
         self.recv_packets = []
         self.recv_condition = threading.Condition()
-        self.recv_thread = threading.Thread(
-            target=read_packet_thread, args=(self, log_file)
-        )
+        self.recv_thread = threading.Thread(target=self._read_packet_thread)
         self.process_event_body = None
         self.exit_status = None
         self.initialize_body = None
@@ -155,6 +134,15 @@ def validate_response(cls, command, response):
         if command["seq"] != response["request_seq"]:
             raise ValueError("seq mismatch in response")
 
+    def _read_packet_thread(self):
+        done = False
+        while not done:
+            packet = read_packet(self.recv, trace_file=self.trace_file)
+            # `packet` will be `None` on EOF. We want to pass it down to
+            # handle_recv_packet anyway so the main thread can handle unexpected
+            # termination of lldb-dap and stop waiting for new packets.
+            done = not self.handle_recv_packet(packet)
+
     def get_modules(self):
         module_list = self.request_modules()["body"]["modules"]
         modules = {}
@@ -247,10 +235,12 @@ def handle_recv_packet(self, packet):
                 # and 'progressEnd' events. Keep these around in case test
                 # cases want to verify them.
                 self.progress_events.append(packet)
-
-        elif packet_type == "response":
-            if packet["command"] == "disconnect":
+            elif event == "terminated":
+                # The terminated event corresponds to the last message we expect
+                # on the DAP. See section 'Debug session end' on
+                # https://microsoft.github.io/debug-adapter-protocol/overview
                 keepGoing = False
+
         self.enqueue_recv_packet(packet)
         return keepGoing
 
@@ -307,7 +297,6 @@ def recv_packet(self, filter_type=None, filter_event=None, timeout=None):
                 return None
             finally:
                 self.recv_condition.release()
-
         return None
 
     def send_recv(self, command):
@@ -1263,7 +1252,7 @@ def launch(cls, /, executable, env=None, log_file=None, connection=None):
             args,
             stdin=subprocess.PIPE,
             stdout=subprocess.PIPE,
-            stderr=subprocess.PIPE,
+            stderr=sys.stderr,
             env=adapter_env,
         )
 
@@ -1294,17 +1283,42 @@ def get_pid(self):
         return -1
 
     def terminate(self):
-        super(DebugAdapterServer, self).terminate()
         if self.process is not None:
-            self.process.terminate()
+            process = self.process
+            self.process = None
             try:
-                self.process.wait(timeout=20)
+                # When we close stdin it should signal the lldb-dap that no
+                # new messages will arrive and it should shutdown on its own.
+                process.stdin.close()
+                process.wait(timeout=30.0)
             except subprocess.TimeoutExpired:
-                self.process.kill()
-                self.process.wait()
-            self.process = None
+                process.kill()
+                process.wait()
+            finally:
+                dump_dap_log(self.log_file)
+            if process.returncode != 0:
+                raise DebugAdapterProcessError(process.returncode)
 
 
+class DebugAdapterError(Exception):
+    pass
+
+
+class DebugAdapterProcessError(DebugAdapterError):
+    """Raised when the lldb-dap process exits with a non-zero exit status."""
+
+    def __init__(self, returncode):
+        self.returncode = returncode
+
+    def __str__(self):
+        if self.returncode and self.returncode < 0:
+            try:
+                return f"lldb-dap died with {signal.Signals(-self.returncode).name}."
+            except ValueError:
+                return f"lldb-dap died with unknown signal {-self.returncode}."
+        else:
+            return f"lldb-dap returned non-zero exit status {self.returncode}."
+
 def attach_options_specified(options):
     if options.pid is not None:
         return True

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py

@@ -146,7 +146,9 @@ def verify_commands(self, flavor, output, commands):
                     found = True
                     break
             self.assertTrue(
-                found, "verify '%s' found in console output for '%s'" % (cmd, flavor)
+                found,
+                "verify '%s' found in console output for '%s' in %s"
+                % (cmd, flavor, output),
             )
 
     def get_dict_value(self, d, key_path):

lldb/test/API/tools/lldb-dap/attach/TestDAP_attach.py

@@ -139,7 +139,9 @@ def test_commands(self):
             exitCommands=exitCommands,
             terminateCommands=terminateCommands,
             postRunCommands=postRunCommands,
+            disconnectAutomatically=False,
         )
+        self.dap_server.wait_for_stopped()
         # Get output from the console. This should contain both the
         # "initCommands" and the "preRunCommands".
         output = self.get_console()
@@ -169,6 +171,9 @@ def test_commands(self):
 
         # Continue until the program exits
         self.continue_to_exit()
+
+        self.dap_server.request_disconnect(terminateDebuggee=True)
+
         # Get output from the console. This should contain both the
         # "exitCommands" that were run after the second breakpoint was hit
         # and the "terminateCommands" due to the debugging session ending

lldb/test/API/tools/lldb-dap/cancel/TestDAP_cancel.py

@@ -45,7 +45,7 @@ def test_pending_request(self):
         """
         program = self.getBuildArtifact("a.out")
         self.build_and_launch(program, stopOnEntry=True)
-        self.continue_to_next_stop()
+        self.dap_server.wait_for_stopped()
 
         # Use a relatively short timeout since this is only to ensure the
         # following request is queued.
@@ -78,7 +78,7 @@ def test_inflight_request(self):
         """
         program = self.getBuildArtifact("a.out")
         self.build_and_launch(program, stopOnEntry=True)
-        self.continue_to_next_stop()
+        self.dap_server.wait_for_stopped()
 
         blocking_seq = self.async_blocking_request(duration=self.timeoutval / 2)
         # Wait for the sleep to start to cancel the inflight request.

lldb/test/API/tools/lldb-dap/console/TestDAP_console.py

@@ -169,14 +169,17 @@ def test_exit_status_message_ok(self):
     def test_diagnositcs(self):
         program = self.getBuildArtifact("a.out")
         self.build_and_launch(program, stopOnEntry=True)
+        self.dap_server.wait_for_stopped()
 
         core = self.getBuildArtifact("minidump.core")
         self.yaml2obj("minidump.yaml", core)
         self.dap_server.request_evaluate(
             f"target create --core  {core}", context="repl"
         )
 
-        output = self.get_important()
+        output = self.collect_important(
+            self.timeoutval, pattern="process ID from minidump file"
+        )
         self.assertIn(
             "warning: unable to retrieve process ID from minidump file",
             output,

lldb/test/API/tools/lldb-dap/io/TestDAP_io.py

@@ -22,13 +22,9 @@ def cleanup():
                 process.terminate()
                 process.wait()
             stdout_data = process.stdout.read().decode()
-            stderr_data = process.stderr.read().decode()
             print("========= STDOUT =========", file=sys.stderr)
             print(stdout_data, file=sys.stderr)
             print("========= END =========", file=sys.stderr)
-            print("========= STDERR =========", file=sys.stderr)
-            print(stderr_data, file=sys.stderr)
-            print("========= END =========", file=sys.stderr)
             print("========= DEBUG ADAPTER PROTOCOL LOGS =========", file=sys.stderr)
             with open(log_file_path, "r") as file:
                 print(file.read(), file=sys.stderr)
@@ -52,13 +48,13 @@ def test_invalid_header(self):
         lldb-dap handles invalid message headers.
         """
         process = self.launch()
-        process.stdin.write(b"not the corret message header")
+        process.stdin.write(b"not the correct message header")
         process.stdin.close()
         self.assertEqual(process.wait(timeout=5.0), 1)
 
     def test_partial_header(self):
         """
-        lldb-dap handles parital message headers.
+        lldb-dap handles partial message headers.
         """
         process = self.launch()
         process.stdin.write(b"Content-Length: ")

lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py

@@ -75,9 +75,7 @@ def test_termination(self):
         self.dap_server.request_disconnect()
 
         # Wait until the underlying lldb-dap process dies.
-        self.dap_server.process.wait(
-            timeout=lldbdap_testcase.DAPTestCaseBase.timeoutval
-        )
+        self.dap_server.process.wait(timeout=self.timeoutval)
 
         # Check the return code
         self.assertEqual(self.dap_server.process.poll(), 0)
@@ -536,12 +534,13 @@ def test_terminate_commands(self):
             terminateCommands=terminateCommands,
             disconnectAutomatically=False,
         )
+        self.dap_server.wait_for_stopped()
         self.get_console()
         # Once it's disconnected the console should contain the
         # "terminateCommands"
         self.dap_server.request_disconnect(terminateDebuggee=True)
         output = self.collect_console(
-            timeout_secs=1.0,
+            timeout_secs=self.timeoutval,
             pattern=terminateCommands[0],
         )
         self.verify_commands("terminateCommands", output, terminateCommands)
@@ -553,7 +552,7 @@ def test_version(self):
         as the one returned by "version" command.
         """
         program = self.getBuildArtifact("a.out")
-        self.build_and_launch(program)
+        self.build_and_launch(program, stopOnEntry=True)
 
         source = "main.c"
         breakpoint_line = line_number(source, "// breakpoint 1")

lldb/test/API/tools/lldb-dap/terminated-event/TestDAP_terminatedEvent.py

@@ -32,7 +32,7 @@ def test_terminated_event(self):
 
         program_basename = "a.out.stripped"
         program = self.getBuildArtifact(program_basename)
-        self.build_and_launch(program)
+        self.build_and_launch(program, stopOnEntry=True, disconnectAutomatically=False)
         # Set breakpoints
         functions = ["foo"]
         breakpoint_ids = self.set_function_breakpoints(functions)

lldb/tools/lldb-dap/Breakpoint.cpp

@@ -7,16 +7,13 @@
 //===----------------------------------------------------------------------===//
 
 #include "Breakpoint.h"
-#include "DAP.h"
 #include "JSONUtils.h"
 #include "lldb/API/SBAddress.h"
 #include "lldb/API/SBBreakpointLocation.h"
 #include "lldb/API/SBLineEntry.h"
-#include "lldb/API/SBMutex.h"
 #include "llvm/ADT/StringExtras.h"
 #include <cstddef>
 #include <cstdint>
-#include <mutex>
 #include <string>
 
 using namespace lldb_dap;
@@ -81,9 +78,6 @@ bool Breakpoint::MatchesName(const char *name) {
 }
 
 void Breakpoint::SetBreakpoint() {
-  lldb::SBMutex lock = m_dap.GetAPIMutex();
-  std::lock_guard<lldb::SBMutex> guard(lock);
-
   m_bp.AddName(kDAPBreakpointLabel);
   if (!m_condition.empty())
     SetCondition();