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

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,
@@ -348,6 +349,8 @@ def attach(
         expectFailure=False,
         gdbRemotePort=None,
         gdbRemoteHostname=None,
+        sourceBreakpoints=None,
+        functionBreakpoints=None,
     ):
         """Build the default Makefile target, create the DAP debug adapter,
         and attach to the process.
@@ -364,6 +367,28 @@ def cleanup():
         self.addTearDownHook(cleanup)
         # Initialize and launch the program
         self.dap_server.request_initialize(sourceInitFile)
+        self.dap_server.wait_for_event("initialized")
+
+        # Set source breakpoints as part of the launch sequence.
+        if sourceBreakpoints:
+            for source_path, lines in sourceBreakpoints:
+                response = self.dap_server.request_setBreakpoints(source_path, lines)
+                self.assertTrue(
+                    response["success"],
+                    "setBreakpoints failed (%s)" % (response),
+                )
+
+        # Set function breakpoints as part of the launch sequence.
+        if functionBreakpoints:
+            response = self.dap_server.request_setFunctionBreakpoints(
+                functionBreakpoints
+            )
+            self.assertTrue(
+                response["success"],
+                "setFunctionBreakpoint failed (%s)" % (response),
+            )
+
+        self.dap_server.request_configurationDone()
         response = self.dap_server.request_attach(
             program=program,
             pid=pid,
@@ -376,6 +401,7 @@ def cleanup():
             attachCommands=attachCommands,
             terminateCommands=terminateCommands,
             coreFile=coreFile,
+            stopOnAttach=stopOnAttach,
             postRunCommands=postRunCommands,
             sourceMap=sourceMap,
             gdbRemotePort=gdbRemotePort,
@@ -419,6 +445,8 @@ def launch(
         commandEscapePrefix=None,
         customFrameFormat=None,
         customThreadFormat=None,
+        sourceBreakpoints=None,
+        functionBreakpoints=None,
     ):
         """Sending launch request to dap"""
 
@@ -434,6 +462,29 @@ def cleanup():
 
         # Initialize and launch the program
         self.dap_server.request_initialize(sourceInitFile)
+        self.dap_server.wait_for_event("initialized")
+
+        # Set source breakpoints as part of the launch sequence.
+        if sourceBreakpoints:
+            for source_path, lines in sourceBreakpoints:
+                response = self.dap_server.request_setBreakpoints(source_path, lines)
+                self.assertTrue(
+                    response["success"],
+                    "setBreakpoints failed (%s)" % (response),
+                )
+
+        # Set function breakpoints as part of the launch sequence.
+        if functionBreakpoints:
+            response = self.dap_server.request_setFunctionBreakpoints(
+                functionBreakpoints
+            )
+            self.assertTrue(
+                response["success"],
+                "setFunctionBreakpoint failed (%s)" % (response),
+            )
+
+        self.dap_server.request_configurationDone()
+
         response = self.dap_server.request_launch(
             program,
             args=args,
@@ -504,6 +555,8 @@ def build_and_launch(
         customThreadFormat=None,
         launchCommands=None,
         expectFailure=False,
+        sourceBreakpoints=None,
+        functionBreakpoints=None,
     ):
         """Build the default Makefile target, create the DAP debug adapter,
         and launch the process.
@@ -540,6 +593,8 @@ def build_and_launch(
             customThreadFormat=customThreadFormat,
             launchCommands=launchCommands,
             expectFailure=expectFailure,
+            sourceBreakpoints=sourceBreakpoints,
+            functionBreakpoints=functionBreakpoints,
         )
 
     def getBuiltinDebugServerTool(self):

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)