llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: John Harrison (ashgti) <details> <summary>Changes</summary> This is more straight forward refactor of the startup sequence that reverts parts of ba29e60f9a2222bd5e883579bb78db13fc5a7588. 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 087a5d2ec7897cd99d3787820711fec76a8e1792 I think we've narrowed down the main source of test instability that motivated the startup sequence change. --- Patch is 42.82 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/140331.diff 30 Files Affected: - (modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py (+10-14) - (modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py (+3-67) - (modified) lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py (+1-1) - (modified) lldb/test/API/tools/lldb-dap/cancel/TestDAP_cancel.py (+2-2) - (modified) lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py (+6-11) - (modified) lldb/test/API/tools/lldb-dap/console/TestDAP_console.py (+7-6) - (modified) lldb/test/API/tools/lldb-dap/console/TestDAP_redirection_to_console.py (+1-3) - (modified) lldb/test/API/tools/lldb-dap/coreFile/TestDAP_coreFile.py (+1) - (modified) lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py (-1) - (modified) lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py (+2-2) - (modified) lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py (+1-1) - (modified) lldb/test/API/tools/lldb-dap/module/TestDAP_module.py (+2-2) - (modified) lldb/test/API/tools/lldb-dap/repl-mode/TestDAP_repl_mode_detection.py (+1-1) - (modified) lldb/test/API/tools/lldb-dap/restart/TestDAP_restart.py (+4-1) - (modified) lldb/test/API/tools/lldb-dap/send-event/TestDAP_sendEvent.py (+2-1) - (modified) lldb/test/API/tools/lldb-dap/stackTrace/TestDAP_stackTrace.py (+1-1) - (modified) lldb/test/API/tools/lldb-dap/stackTraceDisassemblyDisplay/TestDAP_stackTraceDisassemblyDisplay.py (+1-1) - (modified) lldb/test/API/tools/lldb-dap/startDebugging/TestDAP_startDebugging.py (+1-1) - (modified) lldb/test/API/tools/lldb-dap/stop-hooks/TestDAP_stop_hooks.py (+1-1) - (modified) lldb/test/API/tools/lldb-dap/threads/TestDAP_threads.py (+4-1) - (modified) lldb/test/API/tools/lldb-dap/variables/children/TestDAP_variables_children.py (+1-4) - (modified) lldb/tools/lldb-dap/DAP.cpp (+4-25) - (modified) lldb/tools/lldb-dap/DAP.h (-1) - (modified) lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp (+1-18) - (modified) lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp (+43-39) - (modified) lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp (-4) - (modified) lldb/tools/lldb-dap/Handler/LaunchRequestHandler.cpp (+1-19) - (modified) lldb/tools/lldb-dap/Handler/RequestHandler.h (+39-23) - (modified) lldb/tools/lldb-dap/Protocol/ProtocolBase.h (+3) - (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.h (+7) ``````````diff diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py index d3589e78b6bc7..70fd0b0c419db 100644 --- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py +++ b/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: diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py index d7cf8e2864324..afdc746ed0d0d 100644 --- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py +++ b/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,37 +374,13 @@ 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, @@ -416,11 +388,7 @@ def launch( *, sourceInitFile=False, disconnectAutomatically=True, - sourceBreakpoints=None, - functionBreakpoints=None, expectFailure=False, - stopOnEntry=True, - timeout=DEFAULT_TIMEOUT, **kwargs, ): """Sending launch request to dap""" @@ -437,35 +405,7 @@ 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"]): @@ -473,10 +413,6 @@ def cleanup(): 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, diff --git a/lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py b/lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py index 25f031db5cac5..d46fc31d797da 100644 --- a/lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py +++ b/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 diff --git a/lldb/test/API/tools/lldb-dap/cancel/TestDAP_cancel.py b/lldb/test/API/tools/lldb-dap/cancel/TestDAP_cancel.py index 948c146d4da68..824ed8fe3bb97 100644 --- a/lldb/test/API/tools/lldb-dap/cancel/TestDAP_cancel.py +++ b/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. diff --git a/lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py b/lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py index 75876c248f86c..04897acfcf85d 100644 --- a/lldb/test/API/tools/lldb-dap/completions/TestDAP_completions.py +++ b/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"), ], ) diff --git a/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py b/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py index 1f810afdbb667..7b4d1adbb2071 100644 --- a/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py +++ b/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) diff --git a/lldb/test/API/tools/lldb-dap/console/TestDAP_redirection_to_console.py b/lldb/test/API/tools/lldb-dap/console/TestDAP_redirection_to_console.py index 23500bd6fe586..e367c327d4295 100644 --- a/lldb/test/API/tools/lldb-dap/console/TestDAP_redirection_to_console.py +++ b/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" diff --git a/lldb/test/API/tools/lldb-dap/coreFile/TestDAP_coreFile.py b/lldb/test/API/tools/lldb-dap/coreFile/TestDAP_coreFile.py index e678c5ee77fdc..db43dbaf515cf 100644 --- a/lldb/test/API/tools/lldb-dap/coreFile/TestDAP_coreFile.py +++ b/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 = [ { diff --git a/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py b/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py index 372a9bb75e007..2166e88151986 100644 --- a/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py +++ b/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( diff --git a/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py b/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py index 0063954791fd5..8805ce50e6a21 100644 --- a/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py +++ b/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, diff --git a/lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py b/lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py index 19de35b60a3ef..1ef2f2a8235a4 100644 --- a/lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py +++ b/lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py @@ -10,7 +10,7 @@ class TestDAP_module_event(lldbdap_testcase.DAPTestCaseBase): @skipIfWindows def test_module_event(self): 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") diff --git a/lldb/test/API/tools/lldb-dap/module/TestDAP_module.py b/lldb/test/API/tools/lldb-dap/module/TestDAP_module.py index b333efd7bfb1f..3fc0f752ee39e 100644 --- a/lldb/test/API/tools/lldb-dap/module/TestDAP_module.py +++ b/lldb/test/API/tools/lldb-dap/module/TestDAP_module.py @@ -14,7 +14,7 @@ class TestDAP_module(lldbdap_testcase.DAPTestCaseBase): def run_test(self, symbol_basename, expect_debug_info_size): program_basename = "a.out.stripped" program = self.getBuildArtifact(program_basename) - self.build_and_launch(program, stopOnEntry=True) + self.build_and_launch(program) functions = ["foo"] breakpoint_ids = self.set_function_breakpoints(functions) self.assertEqual(len(breakpoint_ids), len(functions), "expect one breakpoint") @@ -108,7 +108,7 @@ def test_modules_dsym(self): @skipIfWindows def test_compile_units(self): program = self.getBuildArtifact("a.out") - self.build_and_launch(program, stopOnEntry=True) + self.build_and_launch(program) source = "main.cpp" main_source_path = self.getSourcePath(source) breakpoint1_line = line_number(source, "// breakpoint 1") diff --git a/lldb/test/API/tools/lldb-dap/repl-mode/TestDAP_repl_mode_detection.py b/lldb/test/API/tools/lldb-dap/repl-mode/TestDAP_repl_mode_detection.py index 81edcdf4bd0f9..c6f59949d668e 100644 --- a/lldb/test/API/tools/lldb-dap/repl-mode/TestDAP_repl_mode_detection.py +++ b/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_lau... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/140331 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
