[lldb-dap] Synchronously wait for breakpoints resolves in tests #140470

eronnen · 2025-05-18T19:00:55Z

Attempt to improve tests by synchronously waiting for breakpoints to resolve. Not sure if it will fix all the tests but I think it should make the tests more stable

llvmbot · 2025-05-18T19:01:26Z

@llvm/pr-subscribers-lldb

Author: Ely Ronnen (eronnen)

Changes

Attempt to improve tests by synchronously waiting for breakpoints to resolve. Not sure if it will fix all the tests but I think it should make the tests more stable

Full diff: https://github.com/llvm/llvm-project/pull/140470.diff

7 Files Affected:

(modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py (+4-2)
(modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py (+24-2)
(modified) lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py (-1)
(modified) lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setFunctionBreakpoints.py (-1)
(modified) lldb/test/API/tools/lldb-dap/module/TestDAP_module.py (+3-1)
(modified) lldb/test/API/tools/lldb-dap/terminated-event/TestDAP_terminatedEvent.py (+3-1)
(modified) lldb/tools/lldb-dap/Handler/TestGetTargetBreakpointsRequestHandler.cpp (+8-2)

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 70fd0b0c419db..1b63ec77abba6 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
@@ -1187,7 +1187,7 @@ def request_locations(self, locationReference):
         }
         return self.send_recv(command_dict)
 
-    def request_testGetTargetBreakpoints(self):
+    def request_testGetTargetBreakpoints(self, only_resolved=False):
         """A request packet used in the LLDB test suite to get all currently
         set breakpoint infos for all breakpoints currently set in the
         target.
@@ -1195,7 +1195,9 @@ def request_testGetTargetBreakpoints(self):
         command_dict = {
             "command": "_testGetTargetBreakpoints",
             "type": "request",
-            "arguments": {},
+            "arguments": {
+                "onlyResolved": only_resolved,
+            },
         }
         return self.send_recv(command_dict)
 
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 afdc746ed0d0d..cc45811bd5f27 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
@@ -48,7 +48,7 @@ def build_and_create_debug_adapter_for_attach(self):
         self.build_and_create_debug_adapter(dictionary={"EXE": unique_name})
         return self.getBuildArtifact(unique_name)
 
-    def set_source_breakpoints(self, source_path, lines, data=None):
+    def set_source_breakpoints(self, source_path, lines, data=None, wait_for_resolve=True):
         """Sets source breakpoints and returns an array of strings containing
         the breakpoint IDs ("1", "2") for each breakpoint that was set.
         Parameter data is array of data objects for breakpoints.
@@ -62,9 +62,11 @@ def set_source_breakpoints(self, source_path, lines, data=None):
         breakpoint_ids = []
         for breakpoint in breakpoints:
             breakpoint_ids.append("%i" % (breakpoint["id"]))
+        if wait_for_resolve:
+            self.wait_for_breakpoints_to_resolve(breakpoint_ids, timeout=10)
         return breakpoint_ids
 
-    def set_function_breakpoints(self, functions, condition=None, hitCondition=None):
+    def set_function_breakpoints(self, functions, condition=None, hitCondition=None, wait_for_resolve=True):
         """Sets breakpoints by function name given an array of function names
         and returns an array of strings containing the breakpoint IDs
         ("1", "2") for each breakpoint that was set.
@@ -78,7 +80,27 @@ def set_function_breakpoints(self, functions, condition=None, hitCondition=None)
         breakpoint_ids = []
         for breakpoint in breakpoints:
             breakpoint_ids.append("%i" % (breakpoint["id"]))
+        if wait_for_resolve:
+            self.wait_for_breakpoints_to_resolve(breakpoint_ids, timeout=10)
         return breakpoint_ids
+    
+    def wait_for_breakpoints_to_resolve(self, breakpoint_ids: list[str], timeout: Optional[float] = None):
+        unresolved_breakpoints = set(breakpoint_ids)
+        
+        # Check already resolved breakpoints
+        resolved_breakpoints = self.dap_server.request_testGetTargetBreakpoints(only_resolved=True)["body"]["breakpoints"]
+        for resolved_breakpoint in resolved_breakpoints:
+            unresolved_breakpoints.discard(str(resolved_breakpoint["id"]))
+
+        while len(unresolved_breakpoints) > 0:
+            breakpoint_event = self.dap_server.wait_for_event("breakpoint", timeout=timeout)
+            if breakpoint_event is None:
+                break
+
+            if breakpoint_event["body"]["reason"] in ["changed", "new"]:
+                unresolved_breakpoints.discard(str(breakpoint_event["body"]["breakpoint"]["id"]))
+
+        self.assertEqual(len(unresolved_breakpoints), 0, f"Expected to resolve all breakpoints. Unresolved breakpoint ids: {unresolved_breakpoints}")
 
     def waitUntil(self, condition_callback):
         for _ in range(20):
diff --git a/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py b/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py
index aae1251b17c93..26df2573555df 100644
--- a/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py
+++ b/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py
@@ -12,7 +12,6 @@
 import os
 
 
-@skip("Temporarily disable the breakpoint tests")
 class TestDAP_setBreakpoints(lldbdap_testcase.DAPTestCaseBase):
     def setUp(self):
         lldbdap_testcase.DAPTestCaseBase.setUp(self)
diff --git a/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setFunctionBreakpoints.py b/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setFunctionBreakpoints.py
index baaca4d974d5d..946595f639edc 100644
--- a/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setFunctionBreakpoints.py
+++ b/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setFunctionBreakpoints.py
@@ -10,7 +10,6 @@
 import lldbdap_testcase
 
 
-@skip("Temporarily disable the breakpoint tests")
 class TestDAP_setFunctionBreakpoints(lldbdap_testcase.DAPTestCaseBase):
     @skipIfWindows
     def test_set_and_clear(self):
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 3fc0f752ee39e..2cea2a94adbbd 100644
--- a/lldb/test/API/tools/lldb-dap/module/TestDAP_module.py
+++ b/lldb/test/API/tools/lldb-dap/module/TestDAP_module.py
@@ -16,7 +16,9 @@ def run_test(self, symbol_basename, expect_debug_info_size):
         program = self.getBuildArtifact(program_basename)
         self.build_and_launch(program)
         functions = ["foo"]
-        breakpoint_ids = self.set_function_breakpoints(functions)
+        
+        # This breakpoint will be resolved only when the libfoo module is loaded
+        breakpoint_ids = self.set_function_breakpoints(functions, wait_for_resolve=False)
         self.assertEqual(len(breakpoint_ids), len(functions), "expect one breakpoint")
         self.continue_to_breakpoints(breakpoint_ids)
         active_modules = self.dap_server.get_modules()
diff --git a/lldb/test/API/tools/lldb-dap/terminated-event/TestDAP_terminatedEvent.py b/lldb/test/API/tools/lldb-dap/terminated-event/TestDAP_terminatedEvent.py
index b0abe2a38dac4..1622fb2d14de2 100644
--- a/lldb/test/API/tools/lldb-dap/terminated-event/TestDAP_terminatedEvent.py
+++ b/lldb/test/API/tools/lldb-dap/terminated-event/TestDAP_terminatedEvent.py
@@ -35,7 +35,9 @@ def test_terminated_event(self):
         self.build_and_launch(program)
         # Set breakpoints
         functions = ["foo"]
-        breakpoint_ids = self.set_function_breakpoints(functions)
+        
+        # This breakpoint will be resolved only when the libfoo module is loaded
+        breakpoint_ids = self.set_function_breakpoints(functions, wait_for_resolve=False)
         self.assertEqual(len(breakpoint_ids), len(functions), "expect one breakpoint")
         main_bp_line = line_number("main.cpp", "// main breakpoint 1")
         breakpoint_ids.append(self.set_source_breakpoints("main.cpp", [main_bp_line]))
diff --git a/lldb/tools/lldb-dap/Handler/TestGetTargetBreakpointsRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/TestGetTargetBreakpointsRequestHandler.cpp
index 5f4f016f6a1ef..129eb31b8356b 100644
--- a/lldb/tools/lldb-dap/Handler/TestGetTargetBreakpointsRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/TestGetTargetBreakpointsRequestHandler.cpp
@@ -15,12 +15,18 @@ namespace lldb_dap {
 
 void TestGetTargetBreakpointsRequestHandler::operator()(
     const llvm::json::Object &request) const {
+  const auto *arguments = request.getObject("arguments");
+  bool only_resolved = GetBoolean(arguments, "onlyResolved").value_or(false);
+
   llvm::json::Object response;
   FillResponse(request, response);
   llvm::json::Array response_breakpoints;
   for (uint32_t i = 0; dap.target.GetBreakpointAtIndex(i).IsValid(); ++i) {
-    auto bp = Breakpoint(dap, dap.target.GetBreakpointAtIndex(i));
-    response_breakpoints.push_back(bp.ToProtocolBreakpoint());
+    const auto target_bp = dap.target.GetBreakpointAtIndex(i);
+    if (!only_resolved || target_bp.GetNumResolvedLocations() > 0) {
+      auto bp = Breakpoint(dap, target_bp);
+      response_breakpoints.push_back(bp.ToProtocolBreakpoint());
+    }
   }
   llvm::json::Object body;
   body.try_emplace("breakpoints", std::move(response_breakpoints));

github-actions · 2025-05-18T19:03:11Z

✅ With the latest revision this PR passed the Python code formatter.

ashgti · 2025-05-18T23:47:36Z

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

@@ -1187,15 +1187,17 @@ def request_locations(self, locationReference):
        }
        return self.send_recv(command_dict)

-    def request_testGetTargetBreakpoints(self):
+    def request_testGetTargetBreakpoints(self, only_resolved=False):


I'm not sure we need to extend this, we could track the current state of breakpoints in DebugCommunication by updating _handle_recv_packet to check for breakpoint events and by updating request_setBreakpoints (and other kinds of breakpoints functions) to update the state when they get a response from the server.

We could have a helper for getting the current state of a breakpoint by id and move wait_for_breakpoints_to_resolve into to DebugCommunication.

Good idea to move this to DebugCommunication but I think we still need to have this request because the breakpoints that resolve immediately don't generate the breakpoint async event

They should be marked verified in the response to the setBreakpoint request, so if we update our state in that call when we get the response then it should be consistent.

Nice catch 💯

ashgti · 2025-05-18T23:48:26Z

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

@@ -62,9 +64,13 @@ def set_source_breakpoints(self, source_path, lines, data=None):
        breakpoint_ids = []
        for breakpoint in breakpoints:
            breakpoint_ids.append("%i" % (breakpoint["id"]))
+        if wait_for_resolve:
+            self.wait_for_breakpoints_to_resolve(breakpoint_ids, timeout=10)


Can the timeout be a parameter that defaults to DEFAULT_TIMEOUT?

ashgti · 2025-05-18T23:50:12Z

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

        return breakpoint_ids

+    def wait_for_breakpoints_to_resolve(


Should we move this to DebugCommunication in dap_server.py? We already have helpers like wait_for_stopped, so this feels similar.

ashgti · 2025-05-18T23:51:18Z

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

+        # Check already resolved breakpoints
+        resolved_breakpoints = self.dap_server.request_testGetTargetBreakpoints(
+            only_resolved=True
+        )["body"]["breakpoints"]
+        for resolved_breakpoint in resolved_breakpoints:
+            unresolved_breakpoints.discard(str(resolved_breakpoint["id"]))


I think if the we kept track of the client state in the DebugCommunication class we wouldn't need to make this custom request.

ashgti · 2025-05-21T15:56:06Z

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

@@ -136,6 +136,7 @@ def __init__(
        self.initialized = False
        self.frame_scopes = {}
        self.init_commands = init_commands
+        self.resolved_breakpoints = set([])


Should we use self.breakpoints = {} and we can key them by id? In case we want to inspect other properties of the breakpoints.

ashgti · 2025-05-21T15:58:02Z

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

@@ -422,8 +431,27 @@ def wait_for_breakpoint_events(self, timeout: Optional[float] = None):
            if not event:
                break
            breakpoint_events.append(event)
+
+        self._update_verified_breakpoints(


We should call this from _handle_recv_packet function so it always is applied.

ashgti · 2025-05-21T15:59:34Z

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

@@ -985,7 +1013,10 @@ def request_setBreakpoints(self, file_path, line_array, data=None):
            "type": "request",
            "arguments": args_dict,
        }
-        return self.send_recv(command_dict)
+        response = self.send_recv(command_dict)
+        breakpoints = response["body"]["breakpoints"]


We should check if the response is a success first, otherwise this may be a missing key exception.

ashgti · 2025-05-21T15:59:47Z

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

@@ -1011,7 +1042,10 @@ def request_setFunctionBreakpoints(self, names, condition=None, hitCondition=Non
            "type": "request",
            "arguments": args_dict,
        }
-        return self.send_recv(command_dict)
+        response = self.send_recv(command_dict)
+        breakpoints = response["body"]["breakpoints"]


Same as above, we should check success first

…ldb-dap tests in order to stabilize the tests

eronnen requested review from ashgti and da-viper May 18, 2025 19:00

eronnen requested a review from JDevlieghere as a code owner May 18, 2025 19:00

llvmbot added lldb lldb-dap labels May 18, 2025

eronnen force-pushed the lldb-dap-wait-for-breakpoints-resolve-in-tests branch from 0bb4631 to 3f08a36 Compare May 18, 2025 19:11

ashgti reviewed May 18, 2025

View reviewed changes

eronnen force-pushed the lldb-dap-wait-for-breakpoints-resolve-in-tests branch from 3f08a36 to bee3890 Compare May 21, 2025 05:53

eronnen requested a review from ashgti May 21, 2025 06:19

ashgti reviewed May 21, 2025

View reviewed changes

eronnen added 6 commits May 21, 2025 21:46

[lldb-dap] Attempt to synchronously wait for breakpoints resolve in l…

5c1d185

…ldb-dap tests in order to stabilize the tests

python format

f44ea3f

keep track of verified breakpoints in tests

a87cf3e

fix default value

9292797

wait for resolve in request_setFunctionBreakpoints

4607cf1

maintain a map of resolved breakpoints

64a42f6

eronnen force-pushed the lldb-dap-wait-for-breakpoints-resolve-in-tests branch from 4a2c4d5 to 64a42f6 Compare May 21, 2025 20:21

eronnen requested a review from ashgti May 21, 2025 20:22

fix missing id handling

a5672ce

[lldb-dap] Synchronously wait for breakpoints resolves in tests #140470

Are you sure you want to change the base?

[lldb-dap] Synchronously wait for breakpoints resolves in tests #140470

Uh oh!

Conversation

eronnen commented May 18, 2025

Uh oh!

llvmbot commented May 18, 2025

Uh oh!

github-actions bot commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented May 18, 2025 •

edited

Loading