[Lldb-commits] [lldb] [llvm] [lldb-dap] Support column breakpoints (PR #113787)

[Pavel Labath via lldb-commits]

================
@@ -910,6 +911,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);
----------------
labath wrote:

I don't think this will be correct for header files. You also need to check that `line_entry.GetFileSpec()` matches the file you're searching for.

That said, I'm somewhat worried about the performance of this code. What's the largest file you've tested it on?

Doing the lookup in the lldb internal representation could be faster (at least we wouldn't need to do the filespec comparison each time, as we could translate it to a "support file index"). For that, I guess we'd need to expose a new SB function to support these kinds of queries.

https://github.com/llvm/llvm-project/pull/113787