[lldb-dap] Migrating breakpointLocations request to use typed RequestHandler #137426

eronnen · 2025-04-26T00:38:26Z

Migrating breakpointLocations request to use typed RequestHandle
Preliminary step in order to implement assembly source breakpoints

llvmbot · 2025-04-26T00:38:59Z

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-lldb

Author: Ely Ronnen (eronnen)

Changes

Migrating breakpointLocations request to use typed RequestHandle as a preliminary step in order to implement assembly source breakpoints

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

7 Files Affected:

(modified) lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp (+18-139)
(modified) lldb/tools/lldb-dap/Handler/RequestHandler.h (+7-3)
(modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp (+17)
(modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.h (+38)
(modified) lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp (+14)
(modified) lldb/tools/lldb-dap/Protocol/ProtocolTypes.h (+21)
(modified) llvm/include/llvm/Support/JSON.h (+8)

diff --git a/lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp b/lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp
index 7a477f3e97875..3798d74ab34be 100644
--- a/lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp
@@ -9,136 +9,25 @@
 #include "DAP.h"
 #include "JSONUtils.h"
 #include "RequestHandler.h"
+#include <vector>
 
 namespace lldb_dap {
 
-// "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 BreakpointLocationsRequestHandler::operator()(
-    const llvm::json::Object &request) const {
-  llvm::json::Object response;
-  FillResponse(request, response);
-  auto *arguments = request.getObject("arguments");
-  auto *source = arguments->getObject("source");
-  std::string path = GetString(source, "path").value_or("").str();
-  const auto start_line = GetInteger<uint64_t>(arguments, "line")
-                              .value_or(LLDB_INVALID_LINE_NUMBER);
-  const auto start_column = GetInteger<uint64_t>(arguments, "column")
-                                .value_or(LLDB_INVALID_COLUMN_NUMBER);
-  const auto end_line =
-      GetInteger<uint64_t>(arguments, "endLine").value_or(start_line);
-  const auto end_column = GetInteger<uint64_t>(arguments, "endColumn")
-                              .value_or(std::numeric_limits<uint64_t>::max());
+/// The `breakpointLocations` request returns all possible locations for source
+/// breakpoints in a given range. Clients should only call this request if the
+/// corresponding capability `supportsBreakpointLocationsRequest` is true.
+llvm::Expected<protocol::BreakpointLocationsResponseBody>
+BreakpointLocationsRequestHandler::Run(
+    const protocol::BreakpointLocationsArguments &args) const {
+  std::vector<protocol::BreakpointLocation> breakpoint_locations;
+
+  std::string path = args.source.path.value_or("");
+  uint32_t start_line = args.line;
+  uint32_t start_column =
+      args.column.value_or(LLDB_INVALID_COLUMN_NUMBER);
+  uint32_t end_line = args.endLine.value_or(start_line);
+  uint32_t end_column =
+      args.endColumn.value_or(std::numeric_limits<uint32_t>::max());
 
   lldb::SBFileSpec file_spec(path.c_str(), true);
   lldb::SBSymbolContextList compile_units =
@@ -191,18 +80,8 @@ void BreakpointLocationsRequestHandler::operator()(
   std::sort(locations.begin(), locations.end());
   locations.erase(llvm::unique(locations), 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));
-  dap.SendJSON(llvm::json::Value(std::move(response)));
+  return protocol::BreakpointLocationsResponseBody{
+      /*breakpoints=*/std::move(breakpoint_locations)};
 }
 
 } // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index fa3d76ed4a125..d4d98220c21d2 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -204,14 +204,18 @@ class AttachRequestHandler : public LegacyRequestHandler {
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class BreakpointLocationsRequestHandler : public LegacyRequestHandler {
+class BreakpointLocationsRequestHandler
+    : public RequestHandler<
+          protocol::BreakpointLocationsArguments,
+          llvm::Expected<protocol::BreakpointLocationsResponseBody>> {
 public:
-  using LegacyRequestHandler::LegacyRequestHandler;
+  using RequestHandler::RequestHandler;
   static llvm::StringLiteral GetCommand() { return "breakpointLocations"; }
   FeatureSet GetSupportedFeatures() const override {
     return {protocol::eAdapterFeatureBreakpointLocationsRequest};
   }
-  void operator()(const llvm::json::Object &request) const override;
+  llvm::Expected<protocol::BreakpointLocationsResponseBody>
+  Run(const protocol::BreakpointLocationsArguments &args) const override;
 };
 
 class CompletionsRequestHandler : public LegacyRequestHandler {
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
index 61fea66490c30..a90d28742c990 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
@@ -245,6 +245,23 @@ bool fromJSON(const json::Value &Params, Configuration &C, json::Path P) {
          parseSourceMap(Params, C.sourceMap, P);
 }
 
+bool fromJSON(const json::Value &Params, BreakpointLocationsArguments &BLA,
+              json::Path P) {
+  json::ObjectMapper O(Params, P);
+  return O && O.map("source", BLA.source) && O.map("line", BLA.line) &&
+         O.mapOptional("column", BLA.column) &&
+         O.mapOptional("endLine", BLA.endLine) &&
+         O.mapOptional("endColumn", BLA.endColumn);
+}
+
+llvm::json::Value toJSON(const BreakpointLocationsResponseBody &BLRB) {
+  llvm::json::Array breakpoints_json;
+  for (const auto &breakpoint : BLRB.breakpoints) {
+    breakpoints_json.push_back(toJSON(breakpoint));
+  }
+  return llvm::json::Object{{"breakpoints", std::move(breakpoints_json)}};
+}
+
 bool fromJSON(const json::Value &Params, LaunchRequestArguments &LRA,
               json::Path P) {
   json::ObjectMapper O(Params, P);
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
index 33f93cc38799a..fd49e585b1dd5 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
@@ -30,6 +30,7 @@
 #include <cstdint>
 #include <optional>
 #include <string>
+#include <vector>
 
 namespace lldb_dap::protocol {
 
@@ -377,6 +378,43 @@ bool fromJSON(const llvm::json::Value &, StepOutArguments &, llvm::json::Path);
 /// body field is required.
 using StepOutResponse = VoidResponse;
 
+/// Arguments for `breakpointLocations` request.
+struct BreakpointLocationsArguments {
+  /// The source location of the breakpoints; either `source.path` or
+  /// `source.sourceReference` must be specified.
+  Source source;
+
+  /// 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.
+  uint32_t line;
+
+  /// 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.
+  std::optional<uint32_t> column;
+
+  /// 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.
+  std::optional<uint32_t> endLine;
+
+  /// 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.
+  std::optional<uint32_t> endColumn;
+};
+bool fromJSON(const llvm::json::Value &, BreakpointLocationsArguments &,
+              llvm::json::Path);
+
+/// Response to `breakpointLocations` request.
+struct BreakpointLocationsResponseBody {
+  /// Content of the source reference.
+  std::vector<BreakpointLocation> breakpoints;
+};
+llvm::json::Value toJSON(const BreakpointLocationsResponseBody &);
+
 } // namespace lldb_dap::protocol
 
 #endif
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
index e64998c4ca488..1056751503461 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
@@ -254,4 +254,18 @@ bool fromJSON(const llvm::json::Value &Params, SteppingGranularity &SG,
   return true;
 }
 
+json::Value toJSON(const BreakpointLocation &B) {
+  json::Object result;
+
+  result.insert({"line", B.line});
+  if (B.column)
+    result.insert({"column", *B.column});
+  if (B.endLine)
+    result.insert({"endLine", *B.endLine});
+  if (B.endColumn)
+    result.insert({"endColumn", *B.endColumn});
+
+  return result;
+}
+
 } // namespace lldb_dap::protocol
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
index 54941f24efbd9..0e8710a736832 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
@@ -322,6 +322,27 @@ enum SteppingGranularity : unsigned {
 bool fromJSON(const llvm::json::Value &, SteppingGranularity &,
               llvm::json::Path);
 
+/// Properties of a breakpoint location returned from the `breakpointLocations`
+/// request.
+struct BreakpointLocation {
+  /// Start line of breakpoint location.
+  uint32_t line;
+
+  /// 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.
+  std::optional<uint32_t> column;
+
+  /// The end line of breakpoint location if the location covers a range.
+  std::optional<uint32_t> endLine;
+
+  /// 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.
+  std::optional<uint32_t> endColumn;
+};
+llvm::json::Value toJSON(const BreakpointLocation &);
+
 } // namespace lldb_dap::protocol
 
 #endif
diff --git a/llvm/include/llvm/Support/JSON.h b/llvm/include/llvm/Support/JSON.h
index 7f7f5f6228763..f1f4f4db709dd 100644
--- a/llvm/include/llvm/Support/JSON.h
+++ b/llvm/include/llvm/Support/JSON.h
@@ -776,6 +776,14 @@ inline bool fromJSON(const Value &E, bool &Out, Path P) {
   P.report("expected boolean");
   return false;
 }
+inline bool fromJSON(const Value &E, unsigned int &Out, Path P) {
+  if (auto S = E.getAsInteger()) {
+    Out = *S;
+    return true;
+  }
+  P.report("expected integer");
+  return false;
+}
 inline bool fromJSON(const Value &E, uint64_t &Out, Path P) {
   if (auto S = E.getAsUINT64()) {
     Out = *S;

eronnen · 2025-04-26T00:40:12Z

CC @ashgti

github-actions · 2025-04-26T00:41:51Z

✅ With the latest revision this PR passed the C/C++ code formatter.

JDevlieghere

LGTM but please wait for @ashgti to sign off too.

JDevlieghere · 2025-04-26T03:51:35Z

lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp

+  for (const auto &breakpoint : BLRB.breakpoints) {
+    breakpoints_json.push_back(toJSON(breakpoint));
+  }


Suggested change

for (const auto &breakpoint : BLRB.breakpoints) {

breakpoints_json.push_back(toJSON(breakpoint));

}

for (const auto &breakpoint : BLRB.breakpoints)

breakpoints_json.push_back(toJSON(breakpoint));

ashgti

Awesome!

eronnen requested a review from JDevlieghere as a code owner April 26, 2025 00:38

llvmbot added lldb llvm:support lldb-dap labels Apr 26, 2025

eronnen force-pushed the lldb-dap-migrate-breakpoint-location-request branch 2 times, most recently from 85c1e0d to 1cc60c1 Compare April 26, 2025 01:06

JDevlieghere requested a review from ashgti April 26, 2025 03:50

JDevlieghere approved these changes Apr 26, 2025

View reviewed changes

eronnen force-pushed the lldb-dap-migrate-breakpoint-location-request branch from 1cc60c1 to 7a3e211 Compare April 27, 2025 00:33

ashgti approved these changes Apr 27, 2025

View reviewed changes

eronnen force-pushed the lldb-dap-migrate-breakpoint-location-request branch from 7a3e211 to 8463897 Compare May 8, 2025 07:35

Migrating breakpointLocations request to use typed RequestHandler

da044f5

eronnen force-pushed the lldb-dap-migrate-breakpoint-location-request branch from 8463897 to da044f5 Compare May 8, 2025 20:24

eronnen merged commit 9818120 into llvm:main May 9, 2025
9 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb-dap] Migrating breakpointLocations request to use typed RequestHandler #137426

[lldb-dap] Migrating breakpointLocations request to use typed RequestHandler #137426

eronnen commented Apr 26, 2025 •

edited

Loading

llvmbot commented Apr 26, 2025 •

edited

Loading

eronnen commented Apr 26, 2025

github-actions bot commented Apr 26, 2025 •

edited

Loading

JDevlieghere left a comment

JDevlieghere Apr 26, 2025

ashgti left a comment

[lldb-dap] Migrating breakpointLocations request to use typed RequestHandler #137426

[lldb-dap] Migrating breakpointLocations request to use typed RequestHandler #137426

Conversation

eronnen commented Apr 26, 2025 • edited Loading

llvmbot commented Apr 26, 2025 • edited Loading

eronnen commented Apr 26, 2025

github-actions bot commented Apr 26, 2025 • edited Loading

JDevlieghere left a comment

Choose a reason for hiding this comment

JDevlieghere Apr 26, 2025

Choose a reason for hiding this comment

ashgti left a comment

Choose a reason for hiding this comment

eronnen commented Apr 26, 2025 •

edited

Loading

llvmbot commented Apr 26, 2025 •

edited

Loading

github-actions bot commented Apr 26, 2025 •

edited

Loading