[lldb-dap] Migrate locations request to structured types #171099
Merged
+143
−142
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-lldb Author: Sergei Druzhkov (DrSergei) ChangesThis patch migrates Full diff: https://github.com/llvm/llvm-project/pull/171099.diff 5 Files Affected:
diff --git a/lldb/tools/lldb-dap/Handler/LocationsRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/LocationsRequestHandler.cpp
index cf9b5a3dbd06b..10a6dcf4d8305 100644
--- a/lldb/tools/lldb-dap/Handler/LocationsRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/LocationsRequestHandler.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "DAP.h"
+#include "DAPError.h"
#include "EventHelper.h"
#include "JSONUtils.h"
#include "LLDBUtils.h"
@@ -18,167 +19,64 @@
namespace lldb_dap {
-// "LocationsRequest": {
-// "allOf": [ { "$ref": "#/definitions/Request" }, {
-// "type": "object",
-// "description": "Looks up information about a location reference
-// previously returned by the debug adapter.",
-// "properties": {
-// "command": {
-// "type": "string",
-// "enum": [ "locations" ]
-// },
-// "arguments": {
-// "$ref": "#/definitions/LocationsArguments"
-// }
-// },
-// "required": [ "command", "arguments" ]
-// }]
-// },
-// "LocationsArguments": {
-// "type": "object",
-// "description": "Arguments for `locations` request.",
-// "properties": {
-// "locationReference": {
-// "type": "integer",
-// "description": "Location reference to resolve."
-// }
-// },
-// "required": [ "locationReference" ]
-// },
-// "LocationsResponse": {
-// "allOf": [ { "$ref": "#/definitions/Response" }, {
-// "type": "object",
-// "description": "Response to `locations` request.",
-// "properties": {
-// "body": {
-// "type": "object",
-// "properties": {
-// "source": {
-// "$ref": "#/definitions/Source",
-// "description": "The source containing the location; either
-// `source.path` or `source.sourceReference` must be
-// specified."
-// },
-// "line": {
-// "type": "integer",
-// "description": "The line number of the location. The client
-// capability `linesStartAt1` determines whether it
-// is 0- or 1-based."
-// },
-// "column": {
-// "type": "integer",
-// "description": "Position of the location within the `line`. 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 the location, present if the location
-// refers to a range. The client capability
-// `linesStartAt1` determines whether it is 0- or
-// 1-based."
-// },
-// "endColumn": {
-// "type": "integer",
-// "description": "End position of the location within `endLine`,
-// present if the location refers to a range. It is
-// measured in UTF-16 code units and the client
-// capability `columnsStartAt1` determines whether
-// it is 0- or 1-based."
-// }
-// },
-// "required": [ "source", "line" ]
-// }
-// }
-// }]
-// },
-void LocationsRequestHandler::operator()(
- const llvm::json::Object &request) const {
- llvm::json::Object response;
- FillResponse(request, response);
- auto *arguments = request.getObject("arguments");
-
- const auto location_id =
- GetInteger<uint64_t>(arguments, "locationReference").value_or(0);
+// Looks up information about a location reference previously returned by the
+// debug adapter.
+llvm::Expected<protocol::LocationsResponseBody>
+LocationsRequestHandler::Run(const protocol::LocationsArguments &args) const {
+ protocol::LocationsResponseBody response;
// We use the lowest bit to distinguish between value location and declaration
// location
- auto [var_ref, is_value_location] = UnpackLocation(location_id);
+ auto [var_ref, is_value_location] = UnpackLocation(args.locationReference);
lldb::SBValue variable = dap.variables.GetVariable(var_ref);
- if (!variable.IsValid()) {
- response["success"] = false;
- response["message"] = "Invalid variable reference";
- dap.SendJSON(llvm::json::Value(std::move(response)));
- return;
- }
+ if (!variable.IsValid())
+ return llvm::make_error<DAPError>("Invalid variable reference");
- llvm::json::Object body;
if (is_value_location) {
// Get the value location
if (!variable.GetType().IsPointerType() &&
- !variable.GetType().IsReferenceType()) {
- response["success"] = false;
- response["message"] =
- "Value locations are only available for pointers and references";
- dap.SendJSON(llvm::json::Value(std::move(response)));
- return;
- }
+ !variable.GetType().IsReferenceType())
+ return llvm::make_error<DAPError>(
+ "Value locations are only available for pointers and references");
lldb::addr_t raw_addr = variable.GetValueAsAddress();
lldb::SBAddress addr = dap.target.ResolveLoadAddress(raw_addr);
lldb::SBLineEntry line_entry = GetLineEntryForAddress(dap.target, addr);
- if (!line_entry.IsValid()) {
- response["success"] = false;
- response["message"] = "Failed to resolve line entry for location";
- dap.SendJSON(llvm::json::Value(std::move(response)));
- return;
- }
+ if (!line_entry.IsValid())
+ return llvm::make_error<DAPError>(
+ "Failed to resolve line entry for location");
const std::optional<protocol::Source> source =
CreateSource(line_entry.GetFileSpec());
- if (!source) {
- response["success"] = false;
- response["message"] = "Failed to resolve file path for location";
- dap.SendJSON(llvm::json::Value(std::move(response)));
- return;
- }
+ if (!source)
+ return llvm::make_error<DAPError>(
+ "Failed to resolve file path for location");
- body.try_emplace("source", *source);
+ response.source = std::move(*source);
if (int line = line_entry.GetLine())
- body.try_emplace("line", line);
+ response.line = line;
if (int column = line_entry.GetColumn())
- body.try_emplace("column", column);
+ response.column = column;
} else {
// Get the declaration location
lldb::SBDeclaration decl = variable.GetDeclaration();
- if (!decl.IsValid()) {
- response["success"] = false;
- response["message"] = "No declaration location available";
- dap.SendJSON(llvm::json::Value(std::move(response)));
- return;
- }
+ if (!decl.IsValid())
+ return llvm::make_error<DAPError>("No declaration location available");
const std::optional<protocol::Source> source =
CreateSource(decl.GetFileSpec());
- if (!source) {
- response["success"] = false;
- response["message"] = "Failed to resolve file path for location";
- dap.SendJSON(llvm::json::Value(std::move(response)));
- return;
- }
+ if (!source)
+ return llvm::make_error<DAPError>(
+ "Failed to resolve file path for location");
- body.try_emplace("source", *source);
+ response.source = std::move(*source);
if (int line = decl.GetLine())
- body.try_emplace("line", line);
+ response.line = line;
if (int column = decl.GetColumn())
- body.try_emplace("column", column);
+ response.column = column;
}
- response.try_emplace("body", std::move(body));
- dap.SendJSON(llvm::json::Value(std::move(response)));
+ return response;
}
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index 5d235352b7738..28e79933a191a 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -563,11 +563,14 @@ class VariablesRequestHandler
Run(const protocol::VariablesArguments &) const override;
};
-class LocationsRequestHandler : public LegacyRequestHandler {
+class LocationsRequestHandler
+ : public RequestHandler<protocol::LocationsArguments,
+ llvm::Expected<protocol::LocationsResponseBody>> {
public:
- using LegacyRequestHandler::LegacyRequestHandler;
+ using RequestHandler::RequestHandler;
static llvm::StringLiteral GetCommand() { return "locations"; }
- void operator()(const llvm::json::Object &request) const override;
+ llvm::Expected<protocol::LocationsResponseBody>
+ Run(const protocol::LocationsArguments &) const override;
};
class DisassembleRequestHandler final
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
index 0a1d580bffd68..d503baffe491b 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
@@ -695,4 +695,23 @@ llvm::json::Value toJSON(const EvaluateResponseBody &Body) {
return result;
}
+bool fromJSON(const llvm::json::Value &Params, LocationsArguments &Args,
+ llvm::json::Path Path) {
+ json::ObjectMapper O(Params, Path);
+ return O && O.map("locationReference", Args.locationReference);
+}
+
+llvm::json::Value toJSON(const LocationsResponseBody &Body) {
+ json::Object result{{"source", Body.source}, {"line", Body.line}};
+
+ if (Body.column)
+ result.insert({"column", Body.column});
+ if (Body.endLine)
+ result.insert({"endLine", Body.endLine});
+ if (Body.endColumn)
+ result.insert({"endColumn", Body.endColumn});
+
+ return result;
+}
+
} // namespace lldb_dap::protocol
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
index 6a85033ae7ef2..c9ac5c575abe3 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
@@ -1184,6 +1184,41 @@ struct EvaluateResponseBody {
};
llvm::json::Value toJSON(const EvaluateResponseBody &);
+/// Arguments for `locations` request.
+struct LocationsArguments {
+ /// Location reference to resolve.
+ uint64_t locationReference = LLDB_DAP_INVALID_VALUE_LOC;
+};
+bool fromJSON(const llvm::json::Value &, LocationsArguments &,
+ llvm::json::Path);
+
+/// Response to 'locations' request.
+struct LocationsResponseBody {
+ /// The source containing the location; either `source.path` or
+ /// `source.sourceReference` must be specified.
+ Source source;
+
+ /// The line number of the location. The client capability `linesStartAt1`
+ /// determines whether it is 0- or 1-based.
+ uint32_t line = 0;
+
+ /// Position of the location within the `line`. 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 the location, present if the location refers to a range. The
+ /// client capability `linesStartAt1` determines whether it is 0- or 1-based.
+ std::optional<uint32_t> endLine;
+
+ /// End position of the location within `endLine`, present if the location
+ /// refers to a range. It 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 LocationsResponseBody &);
+
} // namespace lldb_dap::protocol
#endif
diff --git a/lldb/unittests/DAP/ProtocolRequestsTest.cpp b/lldb/unittests/DAP/ProtocolRequestsTest.cpp
index a74c369924b8e..e64aa5bf8179b 100644
--- a/lldb/unittests/DAP/ProtocolRequestsTest.cpp
+++ b/lldb/unittests/DAP/ProtocolRequestsTest.cpp
@@ -182,3 +182,53 @@ TEST(ProtocolRequestsTest, InitializeRequestArguments) {
EXPECT_THAT_EXPECTED(parse<InitializeRequestArguments>(R"({})"),
FailedWithMessage("missing value at (root).adapterID"));
}
+
+TEST(ProtocolRequestsTest, LocationsArguments) {
+ llvm::Expected<LocationsArguments> expected =
+ parse<LocationsArguments>(R"({"locationReference": 123})");
+ ASSERT_THAT_EXPECTED(expected, llvm::Succeeded());
+ EXPECT_EQ(expected->locationReference, 123U);
+
+ // Check required keys.
+ EXPECT_THAT_EXPECTED(
+ parse<LocationsArguments>(R"({})"),
+ FailedWithMessage("missing value at (root).locationReference"));
+}
+
+TEST(ProtocolRequestsTest, LocationsResponseBody) {
+ LocationsResponseBody body;
+ body.source.sourceReference = 123;
+ body.source.name = "test.cpp";
+ body.line = 42;
+
+ // Check required keys.
+ Expected<json::Value> expected = parse(R"({
+ "source": {
+ "sourceReference": 123,
+ "name": "test.cpp"
+ },
+ "line": 42
+ })");
+
+ ASSERT_THAT_EXPECTED(expected, llvm::Succeeded());
+ EXPECT_EQ(PrettyPrint(*expected), PrettyPrint(body));
+
+ // Check optional keys.
+ body.column = 2;
+ body.endLine = 43;
+ body.endColumn = 4;
+
+ expected = parse(R"({
+ "source": {
+ "sourceReference": 123,
+ "name": "test.cpp"
+ },
+ "line": 42,
+ "column": 2,
+ "endLine": 43,
+ "endColumn": 4
+ })");
+
+ ASSERT_THAT_EXPECTED(expected, llvm::Succeeded());
+ EXPECT_EQ(PrettyPrint(*expected), PrettyPrint(body));
+}
|
da-viper
reviewed
Dec 8, 2025
|
|
||
| /// The line number of the location. The client capability `linesStartAt1` | ||
| /// determines whether it is 0- or 1-based. | ||
| uint32_t line = 0; |
Contributor
There was a problem hiding this comment.
Suggested change
| uint32_t line = 0; | |
| uint32_t line = LLDB_INVALID_LINE_NUMBER; |
| /// 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; |
Contributor
There was a problem hiding this comment.
to the optional types, could we use the uint32_t directly and check for LLDB_INVALID_* and 0 during the toJSON
Contributor
Author
There was a problem hiding this comment.
It sounds reasonable for me, but we already use this pattern in Breakpoint and DisassembledInstruction. I can update them in the further MR, if you wish.
| return llvm::make_error<DAPError>( | ||
| "Failed to resolve line entry for location"); | ||
|
|
||
| const std::optional<protocol::Source> source = |
Contributor
There was a problem hiding this comment.
Suggested change
| const std::optional<protocol::Source> source = | |
| std::optional<protocol::Source> source = |
avoid the const here as we are moving it in line 55
ashgti
approved these changes
Dec 8, 2025
| } | ||
|
|
||
| llvm::json::Value toJSON(const LocationsResponseBody &Body) { | ||
| json::Object result{{"source", Body.source}, {"line", Body.line}}; |
Contributor
There was a problem hiding this comment.
Can we add an assert that line is != LLDB_INVALID_LINE_NUMBER, that indicates the response wasn't filled in correctly.
DrSergei
force-pushed
the
migrate-locations-request
branch
from
5af20b2 to
abee0f4
Compare
da-viper
approved these changes
Dec 9, 2025
anonymouspc
pushed a commit
to anonymouspc/llvm
that referenced
this pull request
Dec 15, 2025
This patch migrates `locations` request into structured types and adds test for it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch migrates
locationsrequest into structured types and adds test for it.