[lldb-dap] Take two at refactoring the startup sequence.

This is more straight forward refactor of the startup sequence that reverts parts of ba29e60. Unlike my previous attempt, I ended up removing the pending request queue and not including an `AsyncReqeustHandler` because I don't think we actually need that at the moment.

The key is that during the startup flow there are 2 parallel operations happening in the DAP that have different triggers.

* The `initialize` request is sent and once the response is received the `launch` or `attach` is sent.
* When the `initialized` event is recieved the `setBreakpionts` and other config requests are made followed by the `configurationDone` event.

I moved the `initialized` event back to happen in the `PostRun` of the `launch` or `attach` request handlers. This ensures that we have a valid target by the time the configuration calls are made. I added also added a few extra validations that to the `configurationeDone` handler to ensure we're in an expected state.

I've also fixed up the tests to match the new flow. With the other additional test fixes in 087a5d2 I think we've narrowed down the main source of test instability that motivated the startup sequence change.

Author: ashgti

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

@@ -133,6 +133,7 @@ def __init__(
         self.output_condition = threading.Condition()
         self.output: dict[str, list[str]] = {}
         self.configuration_done_sent = False
+        self.initialized = False
         self.frame_scopes = {}
         self.init_commands = init_commands
         self.disassembled_instructions = {}
@@ -235,6 +236,8 @@ def _handle_recv_packet(self, packet: Optional[ProtocolMessage]) -> bool:
                 self.output_condition.release()
                 # no need to add 'output' event packets to our packets list
                 return keepGoing
+            elif event == "initialized":
+                self.initialized = True
             elif event == "process":
                 # When a new process is attached or launched, remember the
                 # details that are available in the body of the event
@@ -602,7 +605,7 @@ def request_attach(
         exitCommands: Optional[list[str]] = None,
         terminateCommands: Optional[list[str]] = None,
         coreFile: Optional[str] = None,
-        stopOnAttach=True,
+        stopOnEntry=False,
         sourceMap: Optional[Union[list[tuple[str, str]], dict[str, str]]] = None,
         gdbRemotePort: Optional[int] = None,
         gdbRemoteHostname: Optional[str] = None,
@@ -629,8 +632,8 @@ def request_attach(
             args_dict["attachCommands"] = attachCommands
         if coreFile:
             args_dict["coreFile"] = coreFile
-        if stopOnAttach:
-            args_dict["stopOnEntry"] = stopOnAttach
+        if stopOnEntry:
+            args_dict["stopOnEntry"] = stopOnEntry
         if postRunCommands:
             args_dict["postRunCommands"] = postRunCommands
         if sourceMap:
@@ -640,11 +643,7 @@ def request_attach(
         if gdbRemoteHostname is not None:
             args_dict["gdb-remote-hostname"] = gdbRemoteHostname
         command_dict = {"command": "attach", "type": "request", "arguments": args_dict}
-        response = self.send_recv(command_dict)
-
-        if response["success"]:
-            self.wait_for_event("process")
-        return response
+        return self.send_recv(command_dict)
 
     def request_breakpointLocations(
         self, file_path, line, end_line=None, column=None, end_column=None
@@ -677,6 +676,7 @@ def request_configurationDone(self):
         response = self.send_recv(command_dict)
         if response:
             self.configuration_done_sent = True
+            self.request_threads()
         return response
 
     def _process_stopped(self):
@@ -824,7 +824,7 @@ def request_launch(
         args: Optional[list[str]] = None,
         cwd: Optional[str] = None,
         env: Optional[dict[str, str]] = None,
-        stopOnEntry=True,
+        stopOnEntry=False,
         disableASLR=True,
         disableSTDIO=False,
         shellExpandArguments=False,
@@ -894,11 +894,7 @@ def request_launch(
         if commandEscapePrefix is not None:
             args_dict["commandEscapePrefix"] = commandEscapePrefix
         command_dict = {"command": "launch", "type": "request", "arguments": args_dict}
-        response = self.send_recv(command_dict)
-
-        if response["success"]:
-            self.wait_for_event("process")
-        return response
+        return self.send_recv(command_dict)
 
     def request_next(self, threadId, granularity="statement"):
         if self.exit_status is not None:

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

@@ -56,7 +56,7 @@ def set_source_breakpoints(self, source_path, lines, data=None):
         It contains optional location/hitCondition/logMessage parameters.
         """
         response = self.dap_server.request_setBreakpoints(source_path, lines, data)
-        if response is None:
+        if response is None or not response["success"]:
             return []
         breakpoints = response["body"]["breakpoints"]
         breakpoint_ids = []
@@ -354,13 +354,9 @@ def disassemble(self, threadId=None, frameIndex=None):
     def attach(
         self,
         *,
-        stopOnAttach=True,
         disconnectAutomatically=True,
         sourceInitFile=False,
         expectFailure=False,
-        sourceBreakpoints=None,
-        functionBreakpoints=None,
-        timeout=DEFAULT_TIMEOUT,
         **kwargs,
     ):
         """Build the default Makefile target, create the DAP debug adapter,
@@ -378,49 +374,21 @@ def cleanup():
         self.addTearDownHook(cleanup)
         # Initialize and launch the program
         self.dap_server.request_initialize(sourceInitFile)
-        self.dap_server.wait_for_event("initialized", timeout)
-
-        # 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(stopOnAttach=stopOnAttach, **kwargs)
+        response = self.dap_server.request_attach(**kwargs)
         if expectFailure:
             return response
         if not (response and response["success"]):
             self.assertTrue(
                 response["success"], "attach failed (%s)" % (response["message"])
             )
-        if stopOnAttach:
-            self.dap_server.wait_for_stopped(timeout)
 
     def launch(
         self,
         program=None,
         *,
         sourceInitFile=False,
         disconnectAutomatically=True,
-        sourceBreakpoints=None,
-        functionBreakpoints=None,
         expectFailure=False,
-        stopOnEntry=True,
-        timeout=DEFAULT_TIMEOUT,
         **kwargs,
     ):
         """Sending launch request to dap"""
@@ -437,46 +405,14 @@ def cleanup():
 
         # Initialize and launch the program
         self.dap_server.request_initialize(sourceInitFile)
-        self.dap_server.wait_for_event("initialized", timeout)
-
-        # 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,
-            stopOnEntry=stopOnEntry,
-            **kwargs,
-        )
-
+        response = self.dap_server.request_launch(program, **kwargs)
         if expectFailure:
             return response
         if not (response and response["success"]):
             self.assertTrue(
                 response["success"],
                 "launch failed (%s)" % (response["body"]["error"]["format"]),
             )
-        if stopOnEntry:
-            self.dap_server.wait_for_stopped(timeout)
-
-        return response
 
     def build_and_launch(
         self,

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

@@ -52,7 +52,7 @@ def test_breakpoint_events(self):
         # breakpoint events for these breakpoints but not for ones that are not
         # set via the command interpreter.
         bp_command = "breakpoint set --file foo.cpp --line %u" % (foo_bp2_line)
-        self.build_and_launch(program, stopOnEntry=True, preRunCommands=[bp_command])
+        self.build_and_launch(program, preRunCommands=[bp_command])
         main_bp_id = 0
         foo_bp_id = 0
         # Set breakpoints and verify that they got set correctly

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

@@ -44,7 +44,7 @@ def test_pending_request(self):
         Tests cancelling a pending request.
         """
         program = self.getBuildArtifact("a.out")
-        self.build_and_launch(program, stopOnEntry=True)
+        self.build_and_launch(program)
 
         # Use a relatively short timeout since this is only to ensure the
         # following request is queued.
@@ -76,7 +76,7 @@ def test_inflight_request(self):
         Tests cancelling an inflight request.
         """
         program = self.getBuildArtifact("a.out")
-        self.build_and_launch(program, stopOnEntry=True)
+        self.build_and_launch(program)
 
         blocking_seq = self.async_blocking_request(duration=self.DEFAULT_TIMEOUT / 2)
         # Wait for the sleep to start to cancel the inflight request.

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

@@ -46,17 +46,12 @@ def verify_completions(self, actual_list, expected_list, not_expected_list=[]):
     def setup_debuggee(self):
         program = self.getBuildArtifact("a.out")
         source = "main.cpp"
-        self.build_and_launch(
-            program,
-            stopOnEntry=True,
-            sourceBreakpoints=[
-                (
-                    source,
-                    [
-                        line_number(source, "// breakpoint 1"),
-                        line_number(source, "// breakpoint 2"),
-                    ],
-                ),
+        self.build_and_launch(program)
+        self.set_source_breakpoints(
+            source,
+            [
+                line_number(source, "// breakpoint 1"),
+                line_number(source, "// breakpoint 2"),
             ],
         )
 

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

@@ -53,7 +53,7 @@ def test_scopes_variables_setVariable_evaluate(self):
         character.
         """
         program = self.getBuildArtifact("a.out")
-        self.build_and_launch(program, stopOnEntry=True)
+        self.build_and_launch(program)
         source = "main.cpp"
         breakpoint1_line = line_number(source, "// breakpoint 1")
         lines = [breakpoint1_line]
@@ -66,6 +66,7 @@ def test_scopes_variables_setVariable_evaluate(self):
         # Cause a "scopes" to be sent for frame zero which should update the
         # selected thread and frame to frame 0.
         self.dap_server.get_local_variables(frameIndex=0)
+
         # Verify frame #0 is selected in the command interpreter by running
         # the "frame select" command with no frame index which will print the
         # currently selected frame.
@@ -74,15 +75,15 @@ def test_scopes_variables_setVariable_evaluate(self):
         # Cause a "scopes" to be sent for frame one which should update the
         # selected thread and frame to frame 1.
         self.dap_server.get_local_variables(frameIndex=1)
+
         # Verify frame #1 is selected in the command interpreter by running
         # the "frame select" command with no frame index which will print the
         # currently selected frame.
-
         self.check_lldb_command("frame select", "frame #1", "frame 1 is selected")
 
     def test_custom_escape_prefix(self):
         program = self.getBuildArtifact("a.out")
-        self.build_and_launch(program, stopOnEntry=True, commandEscapePrefix="::")
+        self.build_and_launch(program, commandEscapePrefix="::")
         source = "main.cpp"
         breakpoint1_line = line_number(source, "// breakpoint 1")
         breakpoint_ids = self.set_source_breakpoints(source, [breakpoint1_line])
@@ -97,7 +98,7 @@ def test_custom_escape_prefix(self):
 
     def test_empty_escape_prefix(self):
         program = self.getBuildArtifact("a.out")
-        self.build_and_launch(program, stopOnEntry=True, commandEscapePrefix="")
+        self.build_and_launch(program, commandEscapePrefix="")
         source = "main.cpp"
         breakpoint1_line = line_number(source, "// breakpoint 1")
         breakpoint_ids = self.set_source_breakpoints(source, [breakpoint1_line])
@@ -114,7 +115,7 @@ def test_empty_escape_prefix(self):
     def test_exit_status_message_sigterm(self):
         source = "main.cpp"
         program = self.getBuildArtifact("a.out")
-        self.build_and_launch(program, stopOnEntry=True, commandEscapePrefix="")
+        self.build_and_launch(program, commandEscapePrefix="")
         breakpoint1_line = line_number(source, "// breakpoint 1")
         breakpoint_ids = self.set_source_breakpoints(source, [breakpoint1_line])
         self.continue_to_breakpoints(breakpoint_ids)
@@ -168,7 +169,7 @@ def test_exit_status_message_ok(self):
 
     def test_diagnositcs(self):
         program = self.getBuildArtifact("a.out")
-        self.build_and_launch(program, stopOnEntry=True)
+        self.build_and_launch(program)
 
         core = self.getBuildArtifact("minidump.core")
         self.yaml2obj("minidump.yaml", core)

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

@@ -16,9 +16,7 @@ def test(self):
         """
         program = self.getBuildArtifact("a.out")
         self.build_and_launch(
-            program,
-            stopOnEntry=True,
-            lldbDAPEnv={"LLDB_DAP_TEST_STDOUT_STDERR_REDIRECTION": ""},
+            program, lldbDAPEnv={"LLDB_DAP_TEST_STDOUT_STDERR_REDIRECTION": ""}
         )
 
         source = "main.cpp"

lldb/test/API/tools/lldb-dap/coreFile/TestDAP_coreFile.py

@@ -19,6 +19,7 @@ def test_core_file(self):
 
         self.create_debug_adapter()
         self.attach(program=exe_file, coreFile=core_file)
+        self.dap_server.request_configurationDone()
 
         expected_frames = [
             {

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

@@ -43,7 +43,6 @@ def run_test_evaluate_expressions(
         self.build_and_launch(
             program,
             enableAutoVariableSummaries=enableAutoVariableSummaries,
-            stopOnEntry=True,
         )
         source = "main.cpp"
         self.set_source_breakpoints(

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

@@ -87,7 +87,8 @@ def test_stopOnEntry(self):
         """
         program = self.getBuildArtifact("a.out")
         self.build_and_launch(program, stopOnEntry=True)
-
+        self.dap_server.request_configurationDone()
+        self.dap_server.wait_for_stopped()
         self.assertTrue(
             len(self.dap_server.thread_stop_reasons) > 0,
             "expected stopped event during launch",
@@ -357,7 +358,6 @@ def test_commands(self):
         terminateCommands = ["expr 4+2"]
         self.build_and_launch(
             program,
-            stopOnEntry=True,
             initCommands=initCommands,
             preRunCommands=preRunCommands,
             postRunCommands=postRunCommands,