llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Sergei Druzhkov (DrSergei) <details> <summary>Changes</summary> This patch migrates `locations` request into structured types and adds test for it. --- Full diff: https://github.com/llvm/llvm-project/pull/171099.diff 5 Files Affected: - (modified) lldb/tools/lldb-dap/Handler/LocationsRequestHandler.cpp (+30-132) - (modified) lldb/tools/lldb-dap/Handler/RequestHandler.h (+6-3) - (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp (+19) - (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.h (+35) - (modified) lldb/unittests/DAP/ProtocolRequestsTest.cpp (+50) ``````````diff 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)); +} `````````` </details> https://github.com/llvm/llvm-project/pull/171099 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
