[lldb-dap] Change the launch sequence (#138219)

This PR changes how we treat the launch sequence in lldb-dap.

 - Send the initialized event after we finish handling the initialize
   request, rather than after we finish attaching or launching.
 - Delay handling the launch and attach request until we have handled
   the configurationDone request. The latter is now largely a NO-OP and
   only exists to signal lldb-dap that it can handle the launch and
   attach requests.
 - Delay handling the initial threads requests until we have handled
   the launch or attach request.
 - Make all attaching and launching synchronous, including when we have
   attach or launch commands. This removes the need to synchronize
   between the request and event thread.

Background:
https://discourse.llvm.org/t/reliability-of-the-lldb-dap-tests/86125

Author: JDevlieghere

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

@@ -132,7 +132,6 @@ def __init__(self, recv, send, init_commands, log_file=None):
         self.exit_status = None
         self.initialize_body = None
         self.thread_stop_reasons = {}
-        self.breakpoint_events = []
         self.progress_events = []
         self.reverse_requests = []
         self.module_events = []
@@ -244,13 +243,6 @@ def handle_recv_packet(self, packet):
                 self._process_stopped()
                 tid = body["threadId"]
                 self.thread_stop_reasons[tid] = body
-            elif event == "breakpoint":
-                # Breakpoint events come in when a breakpoint has locations
-                # added or removed. Keep track of them so we can look for them
-                # in tests.
-                self.breakpoint_events.append(packet)
-                # no need to add 'breakpoint' event packets to our packets list
-                return keepGoing
             elif event.startswith("progress"):
                 # Progress events come in as 'progressStart', 'progressUpdate',
                 # and 'progressEnd' events. Keep these around in case test
@@ -412,6 +404,15 @@ def wait_for_stopped(self, timeout=None):
             self.threads = []
         return stopped_events
 
+    def wait_for_breakpoint_events(self, timeout=None):
+        breakpoint_events = []
+        while True:
+            event = self.wait_for_event("breakpoint", timeout=timeout)
+            if not event:
+                break
+            breakpoint_events.append(event)
+        return breakpoint_events
+
     def wait_for_exited(self):
         event_dict = self.wait_for_event("exited")
         if event_dict is None:
@@ -591,6 +592,7 @@ def request_attach(
         attachCommands=None,
         terminateCommands=None,
         coreFile=None,
+        stopOnAttach=True,
         postRunCommands=None,
         sourceMap=None,
         gdbRemotePort=None,
@@ -620,6 +622,8 @@ def request_attach(
             args_dict["attachCommands"] = attachCommands
         if coreFile:
             args_dict["coreFile"] = coreFile
+        if stopOnAttach:
+            args_dict["stopOnEntry"] = stopOnAttach
         if postRunCommands:
             args_dict["postRunCommands"] = postRunCommands
         if sourceMap:
@@ -632,7 +636,7 @@ def request_attach(
         response = self.send_recv(command_dict)
 
         if response["success"]:
-            self.wait_for_events(["process", "initialized"])
+            self.wait_for_event("process")
         return response
 
     def request_breakpointLocations(
@@ -666,10 +670,6 @@ def request_configurationDone(self):
         response = self.send_recv(command_dict)
         if response:
             self.configuration_done_sent = True
-            # Client requests the baseline of currently existing threads after
-            # a successful launch or attach.
-            # Kick off the threads request that follows
-            self.request_threads()
         return response
 
     def _process_stopped(self):
@@ -887,7 +887,7 @@ def request_launch(
         response = self.send_recv(command_dict)
 
         if response["success"]:
-            self.wait_for_events(["process", "initialized"])
+            self.wait_for_event("process")
         return response
 
     def request_next(self, threadId, granularity="statement"):
@@ -1325,6 +1325,26 @@ def attach_options_specified(options):
 
 def run_vscode(dbg, args, options):
     dbg.request_initialize(options.sourceInitFile)
+
+    if options.sourceBreakpoints:
+        source_to_lines = {}
+        for file_line in options.sourceBreakpoints:
+            (path, line) = file_line.split(":")
+            if len(path) == 0 or len(line) == 0:
+                print('error: invalid source with line "%s"' % (file_line))
+
+            else:
+                if path in source_to_lines:
+                    source_to_lines[path].append(int(line))
+                else:
+                    source_to_lines[path] = [int(line)]
+        for source in source_to_lines:
+            dbg.request_setBreakpoints(source, source_to_lines[source])
+    if options.funcBreakpoints:
+        dbg.request_setFunctionBreakpoints(options.funcBreakpoints)
+
+    dbg.request_configurationDone()
+
     if attach_options_specified(options):
         response = dbg.request_attach(
             program=options.program,
@@ -1353,23 +1373,6 @@ def run_vscode(dbg, args, options):
         )
 
     if response["success"]:
-        if options.sourceBreakpoints:
-            source_to_lines = {}
-            for file_line in options.sourceBreakpoints:
-                (path, line) = file_line.split(":")
-                if len(path) == 0 or len(line) == 0:
-                    print('error: invalid source with line "%s"' % (file_line))
-
-                else:
-                    if path in source_to_lines:
-                        source_to_lines[path].append(int(line))
-                    else:
-                        source_to_lines[path] = [int(line)]
-            for source in source_to_lines:
-                dbg.request_setBreakpoints(source, source_to_lines[source])
-        if options.funcBreakpoints:
-            dbg.request_setFunctionBreakpoints(options.funcBreakpoints)
-        dbg.request_configurationDone()
         dbg.wait_for_stopped()
     else:
         if "message" in response:

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

@@ -340,6 +340,7 @@ def attach(
         exitCommands=None,
         attachCommands=None,
         coreFile=None,
+        stopOnAttach=True,
         disconnectAutomatically=True,
         terminateCommands=None,
         postRunCommands=None,
@@ -364,6 +365,8 @@ def cleanup():
         self.addTearDownHook(cleanup)
         # Initialize and launch the program
         self.dap_server.request_initialize(sourceInitFile)
+        self.dap_server.wait_for_event("initialized")
+        self.dap_server.request_configurationDone()
         response = self.dap_server.request_attach(
             program=program,
             pid=pid,
@@ -376,6 +379,7 @@ def cleanup():
             attachCommands=attachCommands,
             terminateCommands=terminateCommands,
             coreFile=coreFile,
+            stopOnAttach=stopOnAttach,
             postRunCommands=postRunCommands,
             sourceMap=sourceMap,
             gdbRemotePort=gdbRemotePort,
@@ -434,6 +438,9 @@ def cleanup():
 
         # Initialize and launch the program
         self.dap_server.request_initialize(sourceInitFile)
+        self.dap_server.wait_for_event("initialized")
+        self.dap_server.request_configurationDone()
+
         response = self.dap_server.request_launch(
             program,
             args=args,

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

@@ -27,6 +27,8 @@ def spawn_and_wait(program, delay):
 @skip
 class TestDAP_attach(lldbdap_testcase.DAPTestCaseBase):
     def set_and_hit_breakpoint(self, continueToExit=True):
+        self.dap_server.wait_for_stopped()
+
         source = "main.c"
         breakpoint1_line = line_number(source, "// breakpoint 1")
         lines = [breakpoint1_line]

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

@@ -18,17 +18,17 @@
 import socket
 
 
-@skip
 class TestDAP_attachByPortNum(lldbdap_testcase.DAPTestCaseBase):
     default_timeout = 20
 
     def set_and_hit_breakpoint(self, continueToExit=True):
+        self.dap_server.wait_for_stopped()
+
         source = "main.c"
-        main_source_path = os.path.join(os.getcwd(), source)
-        breakpoint1_line = line_number(main_source_path, "// breakpoint 1")
+        breakpoint1_line = line_number(source, "// breakpoint 1")
         lines = [breakpoint1_line]
         # Set breakpoint in the thread function so we can step the threads
-        breakpoint_ids = self.set_source_breakpoints(main_source_path, lines)
+        breakpoint_ids = self.set_source_breakpoints(source, lines)
         self.assertEqual(
             len(breakpoint_ids), len(lines), "expect correct number of breakpoints"
         )

lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py

@@ -81,52 +81,27 @@ def test_breakpoint_events(self):
                 breakpoint["verified"], "expect foo breakpoint to not be verified"
             )
 
-        # Get the stop at the entry point
-        self.continue_to_next_stop()
+        # Make sure we're stopped.
+        self.dap_server.wait_for_stopped()
 
-        # We are now stopped at the entry point to the program. Shared
-        # libraries are not loaded yet (at least on macOS they aren't) and only
-        # the breakpoint in the main executable should be resolved.
-        self.assertEqual(len(self.dap_server.breakpoint_events), 1)
-        event = self.dap_server.breakpoint_events[0]
-        body = event["body"]
-        self.assertEqual(
-            body["reason"], "changed", "breakpoint event should say changed"
-        )
-        breakpoint = body["breakpoint"]
-        self.assertEqual(breakpoint["id"], main_bp_id)
-        self.assertTrue(breakpoint["verified"], "main breakpoint should be resolved")
-
-        # Clear the list of breakpoint events so we don't see this one again.
-        self.dap_server.breakpoint_events.clear()
+        # Flush the breakpoint events.
+        self.dap_server.wait_for_breakpoint_events(timeout=5)
 
         # Continue to the breakpoint
         self.continue_to_breakpoints(dap_breakpoint_ids)
 
-        # When the process launches, we first expect to see both the main and
-        # foo breakpoint as unresolved.
-        for event in self.dap_server.breakpoint_events[:2]:
-            body = event["body"]
-            self.assertEqual(
-                body["reason"], "changed", "breakpoint event should say changed"
-            )
-            breakpoint = body["breakpoint"]
-            self.assertIn(str(breakpoint["id"]), dap_breakpoint_ids)
-            self.assertFalse(breakpoint["verified"], "breakpoint should be unresolved")
+        verified_breakpoint_ids = []
+        unverified_breakpoint_ids = []
+        for breakpoint_event in self.dap_server.wait_for_breakpoint_events(timeout=5):
+            breakpoint = breakpoint_event["body"]["breakpoint"]
+            id = breakpoint["id"]
+            if breakpoint["verified"]:
+                verified_breakpoint_ids.append(id)
+            else:
+                unverified_breakpoint_ids.append(id)
 
-        # Then, once the dynamic loader has given us a load address, they
-        # should show up as resolved again.
-        for event in self.dap_server.breakpoint_events[3:]:
-            body = event["body"]
-            self.assertEqual(
-                body["reason"], "changed", "breakpoint event should say changed"
-            )
-            breakpoint = body["breakpoint"]
-            self.assertIn(str(breakpoint["id"]), dap_breakpoint_ids)
-            self.assertTrue(breakpoint["verified"], "breakpoint should be resolved")
-            self.assertNotIn(
-                "source",
-                breakpoint,
-                "breakpoint event should not return a source object",
-            )
-            self.assertIn("line", breakpoint, "breakpoint event should have line")
+        self.assertIn(main_bp_id, unverified_breakpoint_ids)
+        self.assertIn(foo_bp_id, unverified_breakpoint_ids)
+
+        self.assertIn(main_bp_id, verified_breakpoint_ids)
+        self.assertIn(foo_bp_id, verified_breakpoint_ids)

lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py

@@ -44,9 +44,9 @@ def verify_completions(self, actual_list, expected_list, not_expected_list=[]):
             self.assertNotIn(not_expected_item, actual_list)
 
 
-    def setup_debugee(self):
+    def setup_debugee(self, stopOnEntry=False):
         program = self.getBuildArtifact("a.out")
-        self.build_and_launch(program)
+        self.build_and_launch(program, stopOnEntry=stopOnEntry)
 
         source = "main.cpp"
         breakpoint1_line = line_number(source, "// breakpoint 1")
@@ -235,7 +235,7 @@ def test_auto_completions(self):
         """
         Tests completion requests in "repl-mode=auto"
         """
-        self.setup_debugee()
+        self.setup_debugee(stopOnEntry=True)
 
         res = self.dap_server.request_evaluate(
             "`lldb-dap repl-mode auto", context="repl"

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

@@ -167,7 +167,7 @@ def test_exit_status_message_ok(self):
 
     def test_diagnositcs(self):
         program = self.getBuildArtifact("a.out")
-        self.build_and_launch(program)
+        self.build_and_launch(program, stopOnEntry=True)
 
         core = self.getBuildArtifact("minidump.core")
         self.yaml2obj("minidump.yaml", core)

lldb/test/API/tools/lldb-dap/disconnect/TestDAP_disconnect.py

@@ -31,15 +31,19 @@ def test_launch(self):
         created.
         """
         program = self.getBuildArtifact("a.out")
-        self.build_and_launch(program, disconnectAutomatically=False)
+        self.build_and_launch(program, stopOnEntry=True, disconnectAutomatically=False)
 
         # We set a breakpoint right before the side effect file is created
         self.set_source_breakpoints(
             self.source, [line_number(self.source, "// breakpoint")]
         )
         self.continue_to_next_stop()
 
+        # verify we haven't produced the side effect file yet
+        self.assertFalse(os.path.exists(program + ".side_effect"))
+
         self.dap_server.request_disconnect()
+
         # verify we didn't produce the side effect file
         time.sleep(1)
         self.assertFalse(os.path.exists(program + ".side_effect"))

lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py

@@ -10,6 +10,7 @@
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 
+
 # DAP tests are flakey, see https://github.com/llvm/llvm-project/issues/137660.
 @skip
 class TestDAP_evaluate(lldbdap_testcase.DAPTestCaseBase):
@@ -42,7 +43,9 @@ def run_test_evaluate_expressions(
         self.context = context
         program = self.getBuildArtifact("a.out")
         self.build_and_launch(
-            program, enableAutoVariableSummaries=enableAutoVariableSummaries
+            program,
+            enableAutoVariableSummaries=enableAutoVariableSummaries,
+            stopOnEntry=True,
         )
         source = "main.cpp"
         self.set_source_breakpoints(

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

@@ -88,8 +88,8 @@ def test_stopOnEntry(self):
         """
         program = self.getBuildArtifact("a.out")
         self.build_and_launch(program, stopOnEntry=True)
-        self.set_function_breakpoints(["main"])
-        stopped_events = self.continue_to_next_stop()
+
+        stopped_events = self.dap_server.wait_for_stopped()
         for stopped_event in stopped_events:
             if "body" in stopped_event:
                 body = stopped_event["body"]

lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py

@@ -50,7 +50,7 @@ def verify_progress_events(
     @skipIfWindows
     def test(self):
         program = self.getBuildArtifact("a.out")
-        self.build_and_launch(program)
+        self.build_and_launch(program, stopOnEntry=True)
         progress_emitter = os.path.join(os.getcwd(), "Progress_emitter.py")
         self.dap_server.request_evaluate(
             f"`command script import {progress_emitter}", context="repl"

lldb/test/API/tools/lldb-dap/repl-mode/TestDAP_repl_mode_detection.py

@@ -20,7 +20,7 @@ def assertEvaluate(self, expression, regex):
 
     def test_completions(self):
         program = self.getBuildArtifact("a.out")
-        self.build_and_launch(program)
+        self.build_and_launch(program, stopOnEntry=True)
 
         source = "main.cpp"
         breakpoint1_line = line_number(source, "// breakpoint 1")

lldb/test/API/tools/lldb-dap/restart/TestDAP_restart.py

@@ -22,7 +22,6 @@ def test_basic_functionality(self):
         [bp_A, bp_B] = self.set_source_breakpoints("main.c", [line_A, line_B])
 
         # Verify we hit A, then B.
-        self.dap_server.request_configurationDone()
         self.verify_breakpoint_hit([bp_A])
         self.dap_server.request_continue()
         self.verify_breakpoint_hit([bp_B])

lldb/test/API/tools/lldb-dap/restart/TestDAP_restart_runInTerminal.py

@@ -74,7 +74,6 @@ def test_stopOnEntry(self):
         program = self.getBuildArtifact("a.out")
         self.build_and_launch(program, runInTerminal=True, stopOnEntry=True)
         [bp_main] = self.set_function_breakpoints(["main"])
-        self.dap_server.request_configurationDone()
 
         # When using stopOnEntry, configurationDone doesn't result in a running
         # process, we should immediately get a stopped event instead.

lldb/test/API/tools/lldb-dap/stop-hooks/TestDAP_stop_hooks.py

@@ -19,7 +19,7 @@ def test_stop_hooks_before_run(self):
         self.build_and_launch(program, stopOnEntry=True, preRunCommands=preRunCommands)
 
         # The first stop is on entry.
-        self.continue_to_next_stop()
+        self.dap_server.wait_for_stopped()
 
         breakpoint_ids = self.set_function_breakpoints(["main"])
         # This request hangs if the race happens, because, in that case, the