[lldb-dap] Support column breakpoints (#125347)

This commit adds support for column breakpoints to lldb-dap

To do so, support for the `breakpointLocations` request was
added. To find all available breakpoint positions, we iterate over
the line table.

The `setBreakpoints` request already forwarded the column correctly to
`SBTarget::BreakpointCreateByLocation`. However, `SourceBreakpointMap`
did not keep track of multiple breakpoints in the same line. To do so,
the `SourceBreakpointMap` is now indexed by line+column instead of by
line only.

This was previously submitted as #113787, but got reverted due to
failures on ARM and macOS. This second attempt has less strict test
case expectations. Also, I added a release note.

Author: vogelsgesang

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

@@ -612,6 +612,28 @@ def request_attach(
         command_dict = {"command": "attach", "type": "request", "arguments": args_dict}
         return self.send_recv(command_dict)
 
+    def request_breakpointLocations(
+        self, file_path, line, end_line=None, column=None, end_column=None
+    ):
+        (dir, base) = os.path.split(file_path)
+        source_dict = {"name": base, "path": file_path}
+        args_dict = {}
+        args_dict["source"] = source_dict
+        if line is not None:
+            args_dict["line"] = line
+        if end_line is not None:
+            args_dict["endLine"] = end_line
+        if column is not None:
+            args_dict["column"] = column
+        if end_column is not None:
+            args_dict["endColumn"] = end_column
+        command_dict = {
+            "command": "breakpointLocations",
+            "type": "request",
+            "arguments": args_dict,
+        }
+        return self.send_recv(command_dict)
+
     def request_configurationDone(self):
         command_dict = {
             "command": "configurationDone",
@@ -851,6 +873,8 @@ def request_next(self, threadId, granularity="statement"):
     def request_stepIn(self, threadId, targetId, granularity="statement"):
         if self.exit_status is not None:
             raise ValueError("request_stepIn called after process exited")
+        if threadId is None:
+            threadId = self.get_thread_id()
         args_dict = {
             "threadId": threadId,
             "targetId": targetId,
@@ -911,18 +935,14 @@ def request_setBreakpoints(self, file_path, line_array, data=None):
                     breakpoint_data = data[i]
                 bp = {"line": line}
                 if breakpoint_data is not None:
-                    if "condition" in breakpoint_data and breakpoint_data["condition"]:
+                    if breakpoint_data.get("condition"):
                         bp["condition"] = breakpoint_data["condition"]
-                    if (
-                        "hitCondition" in breakpoint_data
-                        and breakpoint_data["hitCondition"]
-                    ):
+                    if breakpoint_data.get("hitCondition"):
                         bp["hitCondition"] = breakpoint_data["hitCondition"]
-                    if (
-                        "logMessage" in breakpoint_data
-                        and breakpoint_data["logMessage"]
-                    ):
+                    if breakpoint_data.get("logMessage"):
                         bp["logMessage"] = breakpoint_data["logMessage"]
+                    if breakpoint_data.get("column"):
+                        bp["column"] = breakpoint_data["column"]
                 breakpoints.append(bp)
             args_dict["breakpoints"] = breakpoints
 

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

@@ -238,9 +238,10 @@ def set_global(self, name, value, id=None):
     def stepIn(
         self, threadId=None, targetId=None, waitForStop=True, granularity="statement"
     ):
-        self.dap_server.request_stepIn(
+        response = self.dap_server.request_stepIn(
             threadId=threadId, targetId=targetId, granularity=granularity
         )
+        self.assertTrue(response["success"])
         if waitForStop:
             return self.dap_server.wait_for_stopped()
         return None

lldb/test/API/tools/lldb-dap/breakpoint/Makefile

@@ -16,4 +16,4 @@ main-copy.cpp: main.cpp
 # The following shared library will be used to test breakpoints under dynamic loading
 libother:  other-copy.c
 	"$(MAKE)" -f $(MAKEFILE_RULES) \
-		DYLIB_ONLY=YES DYLIB_C_SOURCES=other-copy.c DYLIB_NAME=other 
+		DYLIB_ONLY=YES DYLIB_C_SOURCES=other-copy.c DYLIB_NAME=other

lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_breakpointLocations.py

@@ -0,0 +1,88 @@
+"""
+Test lldb-dap breakpointLocations request
+"""
+
+
+import dap_server
+import shutil
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import lldbdap_testcase
+import os
+
+
+class TestDAP_breakpointLocations(lldbdap_testcase.DAPTestCaseBase):
+    def setUp(self):
+        lldbdap_testcase.DAPTestCaseBase.setUp(self)
+
+        self.main_basename = "main-copy.cpp"
+        self.main_path = os.path.realpath(self.getBuildArtifact(self.main_basename))
+
+    @skipIfWindows
+    def test_column_breakpoints(self):
+        """Test retrieving the available breakpoint locations."""
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program, stopOnEntry=True)
+        loop_line = line_number(self.main_path, "// break loop")
+        self.dap_server.request_continue()
+
+        # Ask for the breakpoint locations based only on the line number
+        response = self.dap_server.request_breakpointLocations(
+            self.main_path, loop_line
+        )
+        self.assertTrue(response["success"])
+        self.assertEqual(
+            response["body"]["breakpoints"],
+            [
+                {"line": loop_line, "column": 9},
+                {"line": loop_line, "column": 13},
+                {"line": loop_line, "column": 20},
+                {"line": loop_line, "column": 23},
+                {"line": loop_line, "column": 25},
+                {"line": loop_line, "column": 34},
+                {"line": loop_line, "column": 37},
+                {"line": loop_line, "column": 39},
+                {"line": loop_line, "column": 51},
+            ],
+        )
+
+        # Ask for the breakpoint locations for a column range
+        response = self.dap_server.request_breakpointLocations(
+            self.main_path,
+            loop_line,
+            column=24,
+            end_column=46,
+        )
+        self.assertTrue(response["success"])
+        self.assertEqual(
+            response["body"]["breakpoints"],
+            [
+                {"line": loop_line, "column": 25},
+                {"line": loop_line, "column": 34},
+                {"line": loop_line, "column": 37},
+                {"line": loop_line, "column": 39},
+            ],
+        )
+
+        # Ask for the breakpoint locations for a range of line numbers
+        response = self.dap_server.request_breakpointLocations(
+            self.main_path,
+            line=loop_line,
+            end_line=loop_line + 2,
+            column=39,
+        )
+        self.maxDiff = None
+        self.assertTrue(response["success"])
+        # On some systems, there is an additional breakpoint available
+        # at line 41, column 3, i.e. at the end of the loop. To make this
+        # test more portable, only check that all expected breakpoints are
+        # presented, but also accept additional breakpoints.
+        expected_breakpoints = [
+            {"column": 39, "line": 40},
+            {"column": 51, "line": 40},
+            {"column": 3, "line": 42},
+            {"column": 18, "line": 42},
+        ]
+        for bp in expected_breakpoints:
+            self.assertIn(bp, response["body"]["breakpoints"])

lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py

@@ -125,20 +125,18 @@ def test_set_and_clear(self):
         # Set 3 breakpoints and verify that they got set correctly
         response = self.dap_server.request_setBreakpoints(self.main_path, lines)
         line_to_id = {}
-        if response:
-            breakpoints = response["body"]["breakpoints"]
-            self.assertEqual(
-                len(breakpoints),
-                len(lines),
-                "expect %u source breakpoints" % (len(lines)),
-            )
-            for breakpoint, index in zip(breakpoints, range(len(lines))):
-                line = breakpoint["line"]
-                self.assertTrue(line, lines[index])
-                # Store the "id" of the breakpoint that was set for later
-                line_to_id[line] = breakpoint["id"]
-                self.assertIn(line, lines, "line expected in lines array")
-                self.assertTrue(breakpoint["verified"], "expect breakpoint verified")
+        breakpoints = response["body"]["breakpoints"]
+        self.assertEqual(
+            len(breakpoints),
+            len(lines),
+            "expect %u source breakpoints" % (len(lines)),
+        )
+        for index, breakpoint in enumerate(breakpoints):
+            line = breakpoint["line"]
+            self.assertEqual(line, lines[index])
+            # Store the "id" of the breakpoint that was set for later
+            line_to_id[line] = breakpoint["id"]
+            self.assertTrue(breakpoint["verified"], "expect breakpoint verified")
 
         # There is no breakpoint delete packet, clients just send another
         # setBreakpoints packet with the same source file with fewer lines.
@@ -151,75 +149,66 @@ def test_set_and_clear(self):
         # Set 2 breakpoints and verify that the previous breakpoints that were
         # set above are still set.
         response = self.dap_server.request_setBreakpoints(self.main_path, lines)
-        if response:
-            breakpoints = response["body"]["breakpoints"]
+        breakpoints = response["body"]["breakpoints"]
+        self.assertEqual(
+            len(breakpoints),
+            len(lines),
+            "expect %u source breakpoints" % (len(lines)),
+        )
+        for index, breakpoint in enumerate(breakpoints):
+            line = breakpoint["line"]
+            self.assertEqual(line, lines[index])
+            # Verify the same breakpoints are still set within LLDB by
+            # making sure the breakpoint ID didn't change
             self.assertEqual(
-                len(breakpoints),
-                len(lines),
-                "expect %u source breakpoints" % (len(lines)),
+                line_to_id[line],
+                breakpoint["id"],
+                "verify previous breakpoints stayed the same",
             )
-            for breakpoint, index in zip(breakpoints, range(len(lines))):
-                line = breakpoint["line"]
-                self.assertTrue(line, lines[index])
-                # Verify the same breakpoints are still set within LLDB by
-                # making sure the breakpoint ID didn't change
-                self.assertEqual(
-                    line_to_id[line],
-                    breakpoint["id"],
-                    "verify previous breakpoints stayed the same",
-                )
-                self.assertIn(line, lines, "line expected in lines array")
-                self.assertTrue(
-                    breakpoint["verified"], "expect breakpoint still verified"
-                )
+            self.assertTrue(breakpoint["verified"], "expect breakpoint still verified")
 
         # Now get the full list of breakpoints set in the target and verify
         # we have only 2 breakpoints set. The response above could have told
         # us about 2 breakpoints, but we want to make sure we don't have the
         # third one still set in the target
         response = self.dap_server.request_testGetTargetBreakpoints()
-        if response:
-            breakpoints = response["body"]["breakpoints"]
+        breakpoints = response["body"]["breakpoints"]
+        self.assertEqual(
+            len(breakpoints),
+            len(lines),
+            "expect %u source breakpoints" % (len(lines)),
+        )
+        for breakpoint in breakpoints:
+            line = breakpoint["line"]
+            # Verify the same breakpoints are still set within LLDB by
+            # making sure the breakpoint ID didn't change
             self.assertEqual(
-                len(breakpoints),
-                len(lines),
-                "expect %u source breakpoints" % (len(lines)),
+                line_to_id[line],
+                breakpoint["id"],
+                "verify previous breakpoints stayed the same",
             )
-            for breakpoint in breakpoints:
-                line = breakpoint["line"]
-                # Verify the same breakpoints are still set within LLDB by
-                # making sure the breakpoint ID didn't change
-                self.assertEqual(
-                    line_to_id[line],
-                    breakpoint["id"],
-                    "verify previous breakpoints stayed the same",
-                )
-                self.assertIn(line, lines, "line expected in lines array")
-                self.assertTrue(
-                    breakpoint["verified"], "expect breakpoint still verified"
-                )
+            self.assertIn(line, lines, "line expected in lines array")
+            self.assertTrue(breakpoint["verified"], "expect breakpoint still verified")
 
         # Now clear all breakpoints for the source file by passing down an
         # empty lines array
         lines = []
         response = self.dap_server.request_setBreakpoints(self.main_path, lines)
-        if response:
-            breakpoints = response["body"]["breakpoints"]
-            self.assertEqual(
-                len(breakpoints),
-                len(lines),
-                "expect %u source breakpoints" % (len(lines)),
-            )
+        breakpoints = response["body"]["breakpoints"]
+        self.assertEqual(
+            len(breakpoints),
+            len(lines),
+            "expect %u source breakpoints" % (len(lines)),
+        )
 
         # Verify with the target that all breakpoints have been cleared
         response = self.dap_server.request_testGetTargetBreakpoints()
-        if response:
-            breakpoints = response["body"]["breakpoints"]
-            self.assertEqual(
-                len(breakpoints),
-                len(lines),
-                "expect %u source breakpoints" % (len(lines)),
-            )
+        breakpoints = response["body"]["breakpoints"]
+        self.assertEqual(
+            len(breakpoints),
+            len(lines),
+            "expect %u source breakpoints" % (len(lines)),
+        )
 
         # Now set a breakpoint again in the same source file and verify it
         # was added.
@@ -281,12 +270,11 @@ def test_clear_breakpoints_unset_breakpoints(self):
         self.assertEqual(
             len(breakpoints), len(lines), "expect %u source breakpoints" % (len(lines))
         )
-        for breakpoint, index in zip(breakpoints, range(len(lines))):
+        for index, breakpoint in enumerate(breakpoints):
             line = breakpoint["line"]
-            self.assertTrue(line, lines[index])
+            self.assertEqual(line, lines[index])
             # Store the "id" of the breakpoint that was set for later
             line_to_id[line] = breakpoint["id"]
-            self.assertIn(line, lines, "line expected in lines array")
             self.assertTrue(breakpoint["verified"], "expect breakpoint verified")
 
         # Now clear all breakpoints for the source file by not setting the
@@ -356,3 +344,49 @@ def test_functionality(self):
         self.continue_to_breakpoints(breakpoint_ids)
         i = int(self.dap_server.get_local_variable_value("i"))
         self.assertEqual(i, 7, "i != 7 showing post hitCondition hits every time")
+
+    @skipIfWindows
+    def test_column_breakpoints(self):
+        """Test setting multiple breakpoints in the same line at different columns."""
+        loop_line = line_number("main.cpp", "// break loop")
+
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+
+        # Set two breakpoints on the loop line at different columns.
+        columns = [13, 39]
+        response = self.dap_server.request_setBreakpoints(
+            self.main_path, [loop_line, loop_line], list({"column": c} for c in columns)
+        )
+
+        # Verify the breakpoints were set correctly
+        breakpoints = response["body"]["breakpoints"]
+        breakpoint_ids = []
+        self.assertEqual(
+            len(breakpoints),
+            len(columns),
+            "expect %u source breakpoints" % (len(columns)),
+        )
+        for index, breakpoint in enumerate(breakpoints):
+            self.assertEqual(breakpoint["line"], loop_line)
+            self.assertEqual(breakpoint["column"], columns[index])
+            self.assertTrue(breakpoint["verified"], "expect breakpoint verified")
+            breakpoint_ids.append(breakpoint["id"])
+
+        # Continue to the first breakpoint,
+        self.continue_to_breakpoints([breakpoint_ids[0]])
+
+        # We should have stopped right before the call to `twelve`.
+        # Step into and check we are inside `twelve`.
+        self.stepIn()
+        func_name = self.get_stackFrames()[0]["name"]
+        self.assertEqual(func_name, "twelve(int)")
+
+        # Continue to the second breakpoint.
+        self.continue_to_breakpoints([breakpoint_ids[1]])
+
+        # We should have stopped right before the call to `fourteen`.
+        # Step into and check we are inside `fourteen`.
+        self.stepIn()
+        func_name = self.get_stackFrames()[0]["name"]
+        self.assertEqual(func_name, "a::fourteen(int)")

lldb/tools/lldb-dap/DAP.h

@@ -50,7 +50,8 @@
 
 namespace lldb_dap {
 
-typedef llvm::DenseMap<uint32_t, SourceBreakpoint> SourceBreakpointMap;
+typedef llvm::DenseMap<std::pair<uint32_t, uint32_t>, SourceBreakpoint>
+    SourceBreakpointMap;
 typedef llvm::StringMap<FunctionBreakpoint> FunctionBreakpointMap;
 typedef llvm::DenseMap<lldb::addr_t, InstructionBreakpoint>
     InstructionBreakpointMap;