https://github.com/vogelsgesang updated https://github.com/llvm/llvm-project/pull/113787
>From af45bc2e24623d7225d24a4680a28630d67d636e Mon Sep 17 00:00:00 2001 From: Adrian Vogelsgesang <avogelsges...@salesforce.com> Date: Sat, 26 Oct 2024 14:34:31 +0000 Subject: [PATCH 01/10] [lldb-dap] Support column breakpoints This commit adds support for column breakpoints to lldb-dap. To do so, support for `breakpointLocations` 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. See http://jonasdevlieghere.com/post/lldb-column-breakpoints/ for a high-level introduction to column breakpoints. --- .../test/tools/lldb-dap/dap_server.py | 30 ++- .../API/tools/lldb-dap/breakpoint/Makefile | 2 +- .../breakpoint/TestDAP_breakpointLocations.py | 74 +++++++ .../breakpoint/TestDAP_setBreakpoints.py | 172 +++++++++------ lldb/tools/lldb-dap/DAP.h | 2 +- lldb/tools/lldb-dap/lldb-dap.cpp | 197 +++++++++++++++++- .../llvm/DebugInfo/DWARF/DWARFDebugLine.h | 2 +- 7 files changed, 393 insertions(+), 86 deletions(-) create mode 100644 lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_breakpointLocations.py 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 63748a71f1122d..659408c709a249 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 @@ -612,6 +612,22 @@ 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", @@ -912,18 +928,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 diff --git a/lldb/test/API/tools/lldb-dap/breakpoint/Makefile b/lldb/test/API/tools/lldb-dap/breakpoint/Makefile index 7634f513e85233..06438b3e6e3139 100644 --- a/lldb/test/API/tools/lldb-dap/breakpoint/Makefile +++ b/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 diff --git a/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_breakpointLocations.py b/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_breakpointLocations.py new file mode 100644 index 00000000000000..d84ce843e3fba8 --- /dev/null +++ b/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_breakpointLocations.py @@ -0,0 +1,74 @@ +""" +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_setBreakpoints(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"]) + self.assertEqual(response["body"]["breakpoints"], [ + {'column': 39, 'line': 40}, + {'column': 51, 'line': 40}, + {'column': 3, 'line': 42}, + {'column': 18, 'line': 42} + ]) 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 123fea79c5cda8..2a2b7bbf6d29f2 100644 --- a/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py +++ b/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,70 @@ 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", + ) + self.assertTrue( + breakpoint["verified"], "expect breakpoint still verified" ) - 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" - ) # 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", + ) + self.assertIn(line, lines, "line expected in lines array") + self.assertTrue( + breakpoint["verified"], "expect breakpoint still verified" ) - 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" - ) # 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 +274,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 +348,47 @@ 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") + + # 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") diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h index acc10ade75fd14..49c2b5b34afc3d 100644 --- a/lldb/tools/lldb-dap/DAP.h +++ b/lldb/tools/lldb-dap/DAP.h @@ -67,7 +67,7 @@ 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; diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp index f70b0d3d4cbee0..9b84da08d442a9 100644 --- a/lldb/tools/lldb-dap/lldb-dap.cpp +++ b/lldb/tools/lldb-dap/lldb-dap.cpp @@ -9,7 +9,10 @@ #include "DAP.h" #include "Watchpoint.h" #include "lldb/API/SBDeclaration.h" +#include "lldb/API/SBFileSpec.h" +#include "lldb/API/SBLineEntry.h" #include "lldb/API/SBMemoryRegionInfo.h" +#include "lldb/lldb-defines.h" #include "llvm/Support/Base64.h" #include <cassert> @@ -18,6 +21,7 @@ #include <cstdio> #include <cstdlib> #include <cstring> +#include <limits> #include <optional> #include <sys/stat.h> #include <sys/types.h> @@ -910,6 +914,183 @@ void request_attach(const llvm::json::Object &request) { } } +// "BreakpointLocationsRequest": { +// "allOf": [ { "$ref": "#/definitions/Request" }, { +// "type": "object", +// "description": "The `breakpointLocations` request returns all possible +// locations for source breakpoints in a given range.\nClients should only +// call this request if the corresponding capability +// `supportsBreakpointLocationsRequest` is true.", +// "properties": { +// "command": { +// "type": "string", +// "enum": [ "breakpointLocations" ] +// }, +// "arguments": { +// "$ref": "#/definitions/BreakpointLocationsArguments" +// } +// }, +// "required": [ "command" ] +// }] +// }, +// "BreakpointLocationsArguments": { +// "type": "object", +// "description": "Arguments for `breakpointLocations` request.", +// "properties": { +// "source": { +// "$ref": "#/definitions/Source", +// "description": "The source location of the breakpoints; either +// `source.path` or `source.sourceReference` must be specified." +// }, +// "line": { +// "type": "integer", +// "description": "Start line of range to search possible breakpoint +// locations in. If only the line is specified, the request returns all +// possible locations in that line." +// }, +// "column": { +// "type": "integer", +// "description": "Start position within `line` to search possible +// breakpoint locations in. It is measured in UTF-16 code units and the +// client capability `columnsStartAt1` determines whether it is 0- or +// 1-based. If no column is given, the first position in the start line is +// assumed." +// }, +// "endLine": { +// "type": "integer", +// "description": "End line of range to search possible breakpoint +// locations in. If no end line is given, then the end line is assumed to +// be the start line." +// }, +// "endColumn": { +// "type": "integer", +// "description": "End position within `endLine` to search possible +// breakpoint locations in. It is measured in UTF-16 code units and the +// client capability `columnsStartAt1` determines whether it is 0- or +// 1-based. If no end column is given, the last position in the end line +// is assumed." +// } +// }, +// "required": [ "source", "line" ] +// }, +// "BreakpointLocationsResponse": { +// "allOf": [ { "$ref": "#/definitions/Response" }, { +// "type": "object", +// "description": "Response to `breakpointLocations` request.\nContains +// possible locations for source breakpoints.", +// "properties": { +// "body": { +// "type": "object", +// "properties": { +// "breakpoints": { +// "type": "array", +// "items": { +// "$ref": "#/definitions/BreakpointLocation" +// }, +// "description": "Sorted set of possible breakpoint locations." +// } +// }, +// "required": [ "breakpoints" ] +// } +// }, +// "required": [ "body" ] +// }] +// }, +// "BreakpointLocation": { +// "type": "object", +// "description": "Properties of a breakpoint location returned from the +// `breakpointLocations` request.", +// "properties": { +// "line": { +// "type": "integer", +// "description": "Start line of breakpoint location." +// }, +// "column": { +// "type": "integer", +// "description": "The start position of a breakpoint location. Position +// is measured in UTF-16 code units and the client capability +// `columnsStartAt1` determines whether it is 0- or 1-based." +// }, +// "endLine": { +// "type": "integer", +// "description": "The end line of breakpoint location if the location +// covers a range." +// }, +// "endColumn": { +// "type": "integer", +// "description": "The end position of a breakpoint location (if the +// location covers a range). Position is measured in UTF-16 code units and +// the client capability `columnsStartAt1` determines whether it is 0- or +// 1-based." +// } +// }, +// "required": [ "line" ] +// }, +void request_breakpointLocations(const llvm::json::Object &request) { + llvm::json::Object response; + FillResponse(request, response); + auto *arguments = request.getObject("arguments"); + auto *source = arguments->getObject("source"); + std::string path = GetString(source, "path").str(); + uint64_t start_line = GetUnsigned(arguments, "line", 0); + uint64_t start_column = GetUnsigned(arguments, "column", 0); + uint64_t end_line = GetUnsigned(arguments, "endLine", start_line); + uint64_t end_column = + GetUnsigned(arguments, "endColumn", std::numeric_limits<uint64_t>::max()); + + lldb::SBFileSpec file_spec(path.c_str(), true); + lldb::SBSymbolContextList compile_units = + g_dap.target.FindCompileUnits(file_spec); + + // Find all relevant lines & columns + llvm::SmallVector<std::pair<uint32_t, uint32_t>, 8> locations; + for (uint32_t c_idx = 0, c_limit = compile_units.GetSize(); c_idx < c_limit; + ++c_idx) { + const lldb::SBCompileUnit &compile_unit = + compile_units.GetContextAtIndex(c_idx).GetCompileUnit(); + if (!compile_units.IsValid()) + continue; + + // Go through the line table and find all matching lines / columns + for (uint32_t l_idx = 0, l_limit = compile_unit.GetNumLineEntries(); + l_idx < l_limit; ++l_idx) { + lldb::SBLineEntry line_entry = compile_unit.GetLineEntryAtIndex(l_idx); + + // Filter by line / column + uint32_t line = line_entry.GetLine(); + if (line < start_line || line > end_line) + continue; + uint32_t column = line_entry.GetColumn(); + if (column == LLDB_INVALID_COLUMN_NUMBER) + continue; + if (line == start_line && column < start_column) + continue; + if (line == end_line && column > end_column) + continue; + locations.emplace_back(line, column); + } + } + + // The line entries are sorted by addresses, but we must return the list + // ordered by line / column position. + std::sort(locations.begin(), locations.end()); + locations.erase(std::unique(locations.begin(), locations.end()), + locations.end()); + + llvm::json::Array locations_json; + for (auto &l : locations) { + llvm::json::Object location; + location.try_emplace("line", l.first); + location.try_emplace("column", l.second); + locations_json.emplace_back(std::move(location)); + } + + llvm::json::Object body; + body.try_emplace("breakpoints", std::move(locations_json)); + response.try_emplace("body", std::move(body)); + g_dap.SendJSON(llvm::json::Value(std::move(response))); +} + // "ContinueRequest": { // "allOf": [ { "$ref": "#/definitions/Request" }, { // "type": "object", @@ -1941,6 +2122,8 @@ void request_initialize(const llvm::json::Object &request) { body.try_emplace("supportsCompletionsRequest", true); // The debug adapter supports the disassembly request. body.try_emplace("supportsDisassembleRequest", true); + // The debug adapter supports the `breakpointLocations` request. + body.try_emplace("supportsBreakpointLocationsRequest", true); // The debug adapter supports stepping granularities (argument `granularity`) // for the stepping requests. body.try_emplace("supportsSteppingGranularity", true); @@ -2701,16 +2884,16 @@ void request_setBreakpoints(const llvm::json::Object &request) { // to an empty array. if (breakpoints) { for (const auto &bp : *breakpoints) { - auto bp_obj = bp.getAsObject(); + auto *bp_obj = bp.getAsObject(); if (bp_obj) { SourceBreakpoint src_bp(*bp_obj); - request_bps[src_bp.line] = src_bp; + std::pair<uint32_t, uint32_t> bp_pos(src_bp.line, src_bp.column); + request_bps[bp_pos] = src_bp; // We check if this breakpoint already exists to update it auto existing_source_bps = g_dap.source_breakpoints.find(path); if (existing_source_bps != g_dap.source_breakpoints.end()) { - const auto &existing_bp = - existing_source_bps->second.find(src_bp.line); + const auto &existing_bp = existing_source_bps->second.find(bp_pos); if (existing_bp != existing_source_bps->second.end()) { existing_bp->second.UpdateBreakpoint(src_bp); AppendBreakpoint(&existing_bp->second, response_breakpoints, path, @@ -2719,8 +2902,8 @@ void request_setBreakpoints(const llvm::json::Object &request) { } } // At this point the breakpoint is new - g_dap.source_breakpoints[path][src_bp.line] = src_bp; - SourceBreakpoint &new_bp = g_dap.source_breakpoints[path][src_bp.line]; + g_dap.source_breakpoints[path][bp_pos] = src_bp; + SourceBreakpoint &new_bp = g_dap.source_breakpoints[path][bp_pos]; new_bp.SetBreakpoint(path.data()); AppendBreakpoint(&new_bp, response_breakpoints, path, new_bp.line); } @@ -4810,6 +4993,8 @@ void request_setInstructionBreakpoints(const llvm::json::Object &request) { void RegisterRequestCallbacks() { g_dap.RegisterRequestCallback("attach", request_attach); + g_dap.RegisterRequestCallback("breakpointLocations", + request_breakpointLocations); g_dap.RegisterRequestCallback("completions", request_completions); g_dap.RegisterRequestCallback("continue", request_continue); g_dap.RegisterRequestCallback("configurationDone", request_configurationDone); diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h index ff7bf87d8e6b5a..81c19e0acf602f 100644 --- a/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h +++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h @@ -78,7 +78,7 @@ class DWARFDebugLine { /// The maximum number of individual operations that may be encoded in an /// instruction. uint8_t MaxOpsPerInst; - /// The initial value of theis_stmtregister. + /// The initial value of the is_stmt register. uint8_t DefaultIsStmt; /// This parameter affects the meaning of the special opcodes. See below. int8_t LineBase; >From 69376869bfbe8caff51e937f2ca39926d7522b48 Mon Sep 17 00:00:00 2001 From: Adrian Vogelsgesang <avogelsges...@salesforce.com> Date: Sun, 27 Oct 2024 03:50:27 +0000 Subject: [PATCH 02/10] Fix formatting --- .../test/tools/lldb-dap/dap_server.py | 12 +++- .../breakpoint/TestDAP_breakpointLocations.py | 71 +++++++++++-------- .../breakpoint/TestDAP_setBreakpoints.py | 12 ++-- lldb/tools/lldb-dap/DAP.h | 3 +- 4 files changed, 57 insertions(+), 41 deletions(-) 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 659408c709a249..0384e257a6868d 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 @@ -612,20 +612,26 @@ 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): + 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 + 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} + command_dict = { + "command": "breakpointLocations", + "type": "request", + "arguments": args_dict, + } return self.send_recv(command_dict) def request_configurationDone(self): diff --git a/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_breakpointLocations.py b/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_breakpointLocations.py index d84ce843e3fba8..4803451bdc8aaa 100644 --- a/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_breakpointLocations.py +++ b/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_breakpointLocations.py @@ -28,47 +28,58 @@ def test_column_breakpoints(self): 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) + 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 } - ]) + 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, + 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 }, - ]) + 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, + line=loop_line, + end_line=loop_line + 2, + column=39, ) - self.maxDiff=None + self.maxDiff = None self.assertTrue(response["success"]) - self.assertEqual(response["body"]["breakpoints"], [ - {'column': 39, 'line': 40}, - {'column': 51, 'line': 40}, - {'column': 3, 'line': 42}, - {'column': 18, 'line': 42} - ]) + self.assertEqual( + response["body"]["breakpoints"], + [ + {"column": 39, "line": 40}, + {"column": 51, "line": 40}, + {"column": 3, "line": 42}, + {"column": 18, "line": 42}, + ], + ) 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 2a2b7bbf6d29f2..d408b72dc4975b 100644 --- a/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py +++ b/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py @@ -165,9 +165,7 @@ def test_set_and_clear(self): breakpoint["id"], "verify previous breakpoints stayed the same", ) - 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 @@ -190,9 +188,7 @@ def test_set_and_clear(self): "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 clear all breakpoints for the source file by passing down an # empty lines array @@ -359,7 +355,9 @@ def test_column_breakpoints(self): # 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)) + 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"] diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h index 49c2b5b34afc3d..8ca46a7ea7337b 100644 --- a/lldb/tools/lldb-dap/DAP.h +++ b/lldb/tools/lldb-dap/DAP.h @@ -67,7 +67,8 @@ namespace lldb_dap { -typedef llvm::DenseMap<std::pair<uint32_t, 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; >From 1f655ecf35add94b4c194119e324a48c05cd9a76 Mon Sep 17 00:00:00 2001 From: Adrian Vogelsgesang <avogelsges...@salesforce.com> Date: Sun, 27 Oct 2024 13:21:00 +0000 Subject: [PATCH 03/10] Fix test case --- .../Python/lldbsuite/test/tools/lldb-dap/dap_server.py | 2 ++ .../Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py | 3 ++- .../API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py | 4 ++-- 3 files changed, 6 insertions(+), 3 deletions(-) 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 0384e257a6868d..aaa1c93098b025 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 @@ -874,6 +874,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, 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 a25466f07fa557..d3faab99059c86 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 @@ -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 ) + assert response["success"] is True if waitForStop: return self.dap_server.wait_for_stopped() return None 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 d408b72dc4975b..c62feda64a1254 100644 --- a/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py +++ b/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py @@ -380,7 +380,7 @@ def test_column_breakpoints(self): # Step into and check we are inside `twelve`. self.stepIn() func_name = self.get_stackFrames()[0]["name"] - self.assertEqual(func_name, "twelve") + self.assertEqual(func_name, "twelve(int)") # Continue to the second breakpoint. self.continue_to_breakpoints([breakpoint_ids[1]]) @@ -389,4 +389,4 @@ def test_column_breakpoints(self): # Step into and check we are inside `fourteen`. self.stepIn() func_name = self.get_stackFrames()[0]["name"] - self.assertEqual(func_name, "a::fourteen") + self.assertEqual(func_name, "a::fourteen(int)") >From 46ce2bf10a95f649aeca5f46ec614cc7c8570db5 Mon Sep 17 00:00:00 2001 From: Adrian Vogelsgesang <avogelsges...@salesforce.com> Date: Thu, 31 Oct 2024 11:54:34 +0000 Subject: [PATCH 04/10] Addres review comment re `self.assertTrue` --- .../Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 d3faab99059c86..34e9b96dbcc3f5 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 @@ -241,7 +241,7 @@ def stepIn( response = self.dap_server.request_stepIn( threadId=threadId, targetId=targetId, granularity=granularity ) - assert response["success"] is True + self.assertTrue(response["success"]) if waitForStop: return self.dap_server.wait_for_stopped() return None >From 50d9ce8856f281f8aa2821d6d8a2a6541c38bfe4 Mon Sep 17 00:00:00 2001 From: Adrian Vogelsgesang <avogelsges...@salesforce.com> Date: Thu, 31 Oct 2024 11:56:03 +0000 Subject: [PATCH 05/10] Undo unrelated comment change --- llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h index 81c19e0acf602f..ff7bf87d8e6b5a 100644 --- a/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h +++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h @@ -78,7 +78,7 @@ class DWARFDebugLine { /// The maximum number of individual operations that may be encoded in an /// instruction. uint8_t MaxOpsPerInst; - /// The initial value of the is_stmt register. + /// The initial value of theis_stmtregister. uint8_t DefaultIsStmt; /// This parameter affects the meaning of the special opcodes. See below. int8_t LineBase; >From 51659141944390f066d457a0d8a01cae8ad8cce8 Mon Sep 17 00:00:00 2001 From: Adrian Vogelsgesang <avogelsges...@salesforce.com> Date: Sun, 10 Nov 2024 16:32:09 +0000 Subject: [PATCH 06/10] Fix merge conflict --- lldb/tools/lldb-dap/lldb-dap.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp index 731878cfed006d..0d5e5ac4eaab34 100644 --- a/lldb/tools/lldb-dap/lldb-dap.cpp +++ b/lldb/tools/lldb-dap/lldb-dap.cpp @@ -2882,9 +2882,10 @@ void request_setBreakpoints(const llvm::json::Object &request) { const auto *bp_obj = bp.getAsObject(); if (bp_obj) { SourceBreakpoint src_bp(g_dap, *bp_obj); - request_bps.try_emplace(src_bp.line, src_bp); + std::pair<uint32_t, uint32_t> bp_pos(src_bp.line, src_bp.column); + request_bps.try_emplace(bp_pos, src_bp); const auto [iv, inserted] = - g_dap.source_breakpoints[path].try_emplace(src_bp.line, src_bp); + g_dap.source_breakpoints[path].try_emplace(bp_pos, src_bp); // We check if this breakpoint already exists to update it if (inserted) iv->getSecond().SetBreakpoint(path.data()); >From e3131b4e522457c4557ceac4714e3621ec5662c7 Mon Sep 17 00:00:00 2001 From: Adrian Vogelsgesang <avogelsges...@salesforce.com> Date: Sun, 10 Nov 2024 16:33:55 +0000 Subject: [PATCH 07/10] Check file spec --- lldb/tools/lldb-dap/lldb-dap.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp index 0d5e5ac4eaab34..2ca1eda2006148 100644 --- a/lldb/tools/lldb-dap/lldb-dap.cpp +++ b/lldb/tools/lldb-dap/lldb-dap.cpp @@ -1045,6 +1045,7 @@ void request_breakpointLocations(const llvm::json::Object &request) { compile_units.GetContextAtIndex(c_idx).GetCompileUnit(); if (!compile_units.IsValid()) continue; + lldb::SBFileSpec primary_file_spec = compile_unit.GetFileSpec(); // Go through the line table and find all matching lines / columns for (uint32_t l_idx = 0, l_limit = compile_unit.GetNumLineEntries(); @@ -1062,6 +1063,16 @@ void request_breakpointLocations(const llvm::json::Object &request) { continue; if (line == end_line && column > end_column) continue; + + // Make sure we are in the right file. + // We might have a match on line & column range and still + // be in the wrong file, e.g. for included files. + if (std::string_view(line_entry.GetFileSpec().GetFilename()) != + primary_file_spec.GetFilename() || + std::string_view(line_entry.GetFileSpec().GetDirectory()) != + primary_file_spec.GetDirectory()) + continue; + locations.emplace_back(line, column); } } >From 3a4fb4369ba226a6a9fdebff5d608a0bd7896edb Mon Sep 17 00:00:00 2001 From: Adrian Vogelsgesang <avogelsges...@salesforce.com> Date: Sat, 16 Nov 2024 16:44:34 +0000 Subject: [PATCH 08/10] Minor: Fix `IsValid` check --- lldb/tools/lldb-dap/lldb-dap.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp index 2ca1eda2006148..9685486f4828b2 100644 --- a/lldb/tools/lldb-dap/lldb-dap.cpp +++ b/lldb/tools/lldb-dap/lldb-dap.cpp @@ -1043,7 +1043,7 @@ void request_breakpointLocations(const llvm::json::Object &request) { ++c_idx) { const lldb::SBCompileUnit &compile_unit = compile_units.GetContextAtIndex(c_idx).GetCompileUnit(); - if (!compile_units.IsValid()) + if (!compile_unit.IsValid()) continue; lldb::SBFileSpec primary_file_spec = compile_unit.GetFileSpec(); >From c60ec26380a480f94bcda3d0861615fcddc41751 Mon Sep 17 00:00:00 2001 From: Adrian Vogelsgesang <avogelsges...@salesforce.com> Date: Sat, 16 Nov 2024 16:48:51 +0000 Subject: [PATCH 09/10] Fix rebase conflict --- lldb/tools/lldb-dap/lldb-dap.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp index 040da4b06839c0..77ec610d115e91 100644 --- a/lldb/tools/lldb-dap/lldb-dap.cpp +++ b/lldb/tools/lldb-dap/lldb-dap.cpp @@ -1019,7 +1019,7 @@ void request_attach(DAP &dap, const llvm::json::Object &request) { // }, // "required": [ "line" ] // }, -void request_breakpointLocations(const llvm::json::Object &request) { +void request_breakpointLocations(DAP &dap, const llvm::json::Object &request) { llvm::json::Object response; FillResponse(request, response); auto *arguments = request.getObject("arguments"); @@ -1033,7 +1033,7 @@ void request_breakpointLocations(const llvm::json::Object &request) { lldb::SBFileSpec file_spec(path.c_str(), true); lldb::SBSymbolContextList compile_units = - g_dap.target.FindCompileUnits(file_spec); + dap.target.FindCompileUnits(file_spec); // Find all relevant lines & columns llvm::SmallVector<std::pair<uint32_t, uint32_t>, 8> locations; @@ -1092,7 +1092,7 @@ void request_breakpointLocations(const llvm::json::Object &request) { llvm::json::Object body; body.try_emplace("breakpoints", std::move(locations_json)); response.try_emplace("body", std::move(body)); - g_dap.SendJSON(llvm::json::Value(std::move(response))); + dap.SendJSON(llvm::json::Value(std::move(response))); } // "ContinueRequest": { >From d811551b4cc05bd2d7b4f7212c7fe5c3fd1dd654 Mon Sep 17 00:00:00 2001 From: Adrian Vogelsgesang <avogelsges...@salesforce.com> Date: Sat, 16 Nov 2024 17:30:48 +0000 Subject: [PATCH 10/10] Use raw pointer comparison --- lldb/tools/lldb-dap/lldb-dap.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp index 77ec610d115e91..1590a668ad9008 100644 --- a/lldb/tools/lldb-dap/lldb-dap.cpp +++ b/lldb/tools/lldb-dap/lldb-dap.cpp @@ -1065,9 +1065,11 @@ void request_breakpointLocations(DAP &dap, const llvm::json::Object &request) { // Make sure we are in the right file. // We might have a match on line & column range and still // be in the wrong file, e.g. for included files. - if (std::string_view(line_entry.GetFileSpec().GetFilename()) != + // Given that the involved pointers point into LLDB's string pool, + // we can directly compare the `const char*` pointers. + if (line_entry.GetFileSpec().GetFilename() != primary_file_spec.GetFilename() || - std::string_view(line_entry.GetFileSpec().GetDirectory()) != + line_entry.GetFileSpec().GetDirectory() != primary_file_spec.GetDirectory()) continue; _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
