[lldb-dap] Take two at refactoring the startup sequence. #140331
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
71c7830 to
5c77f71
Compare
JDevlieghere
approved these changes
May 17, 2025
There was a problem hiding this comment.
I think you can move this out of draft. I'm thoroughly convinced that my first stab at this was wrong. The code LGTM and I like that we don't have to launch everything with stopOnEntry.
The only thing I don't like is that the configurationDone is now once again implicit. In a future PR, I'd love to move the tests to a model where we specify that explicitly after doing the breakpoints (and automatically catch cases where someone forgets, like your PR from earlier this week). I'm happy to sign myself up to do that work.
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis 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 The key is that during the startup flow there are 2 parallel operations happening in the DAP that have different triggers.
I moved the 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. 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:
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]
|
|
Looks like all the tests are passing on linux and for me locally on macOS as well.
That sounds great! I think that would help clean up the tests a bit by making it a bit more explicit about when they're done configuring things. |
All the macOS tests are passing for me as well. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
AsyncReqeustHandlerbecause 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.
initializerequest is sent and once the response is received thelaunchorattachis sent.initializedevent is recieved thesetBreakpiontsand other config requests are made followed by theconfigurationDoneevent.I moved the
initializedevent back to happen in thePostRunof thelaunchorattachrequest 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 theconfigurationeDonehandler 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.