llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Sergei Druzhkov (DrSergei) <details> <summary>Changes</summary> This patch migrates `restart` request to structured types. Also, I added some checks that at least one of the required fields was provided for `launch` and `attach` requests. Maybe I missed some possible configurations, so please double check. --- Full diff: https://github.com/llvm/llvm-project/pull/172488.diff 5 Files Affected: - (modified) lldb/tools/lldb-dap/Handler/RequestHandler.h (+6-3) - (modified) lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp (+27-90) - (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp (+68-20) - (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.h (+13) - (modified) lldb/unittests/DAP/ProtocolRequestsTest.cpp (+40) ``````````diff diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h index 0669be50597e9..a18a8049c804d 100644 --- a/lldb/tools/lldb-dap/Handler/RequestHandler.h +++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h @@ -340,11 +340,14 @@ class LaunchRequestHandler void PostRun() const override; }; -class RestartRequestHandler : public LegacyRequestHandler { +class RestartRequestHandler + : public RequestHandler<std::optional<protocol::RestartArguments>, + protocol::RestartResponse> { public: - using LegacyRequestHandler::LegacyRequestHandler; + using RequestHandler::RequestHandler; static llvm::StringLiteral GetCommand() { return "restart"; } - void operator()(const llvm::json::Object &request) const override; + llvm::Error + Run(const std::optional<protocol::RestartArguments> &args) const override; }; class NextRequestHandler diff --git a/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp index 100173bfc3082..3a2be3f30c7cf 100644 --- a/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp @@ -7,91 +7,37 @@ //===----------------------------------------------------------------------===// #include "DAP.h" +#include "DAPError.h" #include "EventHelper.h" -#include "JSONUtils.h" #include "LLDBUtils.h" #include "Protocol/ProtocolRequests.h" #include "RequestHandler.h" -#include "llvm/Support/JSON.h" -#include "llvm/Support/raw_ostream.h" -namespace lldb_dap { +using namespace lldb_dap; +using namespace lldb_dap::protocol; -// "RestartRequest": { -// "allOf": [ { "$ref": "#/definitions/Request" }, { -// "type": "object", -// "description": "Restarts a debug session. Clients should only call this -// request if the corresponding capability `supportsRestartRequest` is -// true.\nIf the capability is missing or has the value false, a typical -// client emulates `restart` by terminating the debug adapter first and then -// launching it anew.", -// "properties": { -// "command": { -// "type": "string", -// "enum": [ "restart" ] -// }, -// "arguments": { -// "$ref": "#/definitions/RestartArguments" -// } -// }, -// "required": [ "command" ] -// }] -// }, -// "RestartArguments": { -// "type": "object", -// "description": "Arguments for `restart` request.", -// "properties": { -// "arguments": { -// "oneOf": [ -// { "$ref": "#/definitions/LaunchRequestArguments" }, -// { "$ref": "#/definitions/AttachRequestArguments" } -// ], -// "description": "The latest version of the `launch` or `attach` -// configuration." -// } -// } -// }, -// "RestartResponse": { -// "allOf": [ { "$ref": "#/definitions/Response" }, { -// "type": "object", -// "description": "Response to `restart` request. This is just an -// acknowledgement, so no body field is required." -// }] -// }, -void RestartRequestHandler::operator()( - const llvm::json::Object &request) const { - llvm::json::Object response; - FillResponse(request, response); - if (!dap.target.GetProcess().IsValid()) { - response["success"] = llvm::json::Value(false); - EmplaceSafeString(response, "message", - "Restart request received but no process was launched."); - dap.SendJSON(llvm::json::Value(std::move(response))); - return; - } +/// Restarts a debug session. Clients should only call this request if the +/// corresponding capability `supportsRestartRequest` is true. +/// If the capability is missing or has the value false, a typical client +/// emulates `restart` by terminating the debug adapter first and then launching +/// it anew. +llvm::Error +RestartRequestHandler::Run(const std::optional<RestartArguments> &args) const { + if (!dap.target.GetProcess().IsValid()) + return llvm::make_error<DAPError>( + "Restart request received but no process was launched."); - const llvm::json::Object *arguments = request.getObject("arguments"); - if (arguments) { - // The optional `arguments` field in RestartRequest can contain an updated - // version of the launch arguments. If there's one, use it. - if (const llvm::json::Value *restart_arguments = - arguments->get("arguments")) { - protocol::LaunchRequestArguments updated_arguments; - llvm::json::Path::Root root; - if (!fromJSON(*restart_arguments, updated_arguments, root)) { - response["success"] = llvm::json::Value(false); - EmplaceSafeString( - response, "message", - llvm::formatv("Failed to parse updated launch arguments: {0}", - llvm::toString(root.getError())) - .str()); - dap.SendJSON(llvm::json::Value(std::move(response))); - return; - } - dap.last_launch_request = updated_arguments; + if (args) { + if (std::holds_alternative<AttachRequestArguments>(args->arguments)) + return llvm::make_error<DAPError>( + "Restarting an AttachRequest is not supported."); + if (const auto *arguments = + std::get_if<LaunchRequestArguments>(&args->arguments); + arguments) { + dap.last_launch_request = *arguments; // Update DAP configuration based on the latest copy of the launch // arguments. - dap.SetConfiguration(updated_arguments.configuration, false); + dap.SetConfiguration(arguments->configuration, false); dap.ConfigureSourceMaps(); } } @@ -117,12 +63,8 @@ void RestartRequestHandler::operator()( // FIXME: Should we run 'preRunCommands'? // FIXME: Should we add a 'preRestartCommands'? - if (llvm::Error err = LaunchProcess(*dap.last_launch_request)) { - response["success"] = llvm::json::Value(false); - EmplaceSafeString(response, "message", llvm::toString(std::move(err))); - dap.SendJSON(llvm::json::Value(std::move(response))); - return; - } + if (llvm::Error err = LaunchProcess(*dap.last_launch_request)) + return llvm::make_error<DAPError>(llvm::toString(std::move(err))); SendProcessEvent(dap, Launch); @@ -130,16 +72,11 @@ void RestartRequestHandler::operator()( // Because we're restarting, configuration has already happened so we can // continue the process right away. if (dap.stop_at_entry) { - if (llvm::Error err = SendThreadStoppedEvent(dap, /*on_entry=*/true)) { - EmplaceSafeString(response, "message", llvm::toString(std::move(err))); - dap.SendJSON(llvm::json::Value(std::move(response))); - return; - } + if (llvm::Error err = SendThreadStoppedEvent(dap, /*on_entry=*/true)) + return llvm::make_error<DAPError>(llvm::toString(std::move(err))); } else { dap.target.GetProcess().Continue(); } - dap.SendJSON(llvm::json::Value(std::move(response))); + return llvm::Error::success(); } - -} // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp index bf470b78077df..1c2cd158ffd29 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp +++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp @@ -296,31 +296,53 @@ bool fromJSON(const json::Value &Params, Console &C, json::Path P) { bool fromJSON(const json::Value &Params, LaunchRequestArguments &LRA, json::Path P) { json::ObjectMapper O(Params, P); - return O && fromJSON(Params, LRA.configuration, P) && - O.mapOptional("noDebug", LRA.noDebug) && - O.mapOptional("launchCommands", LRA.launchCommands) && - O.mapOptional("cwd", LRA.cwd) && O.mapOptional("args", LRA.args) && - O.mapOptional("detachOnError", LRA.detachOnError) && - O.mapOptional("disableASLR", LRA.disableASLR) && - O.mapOptional("disableSTDIO", LRA.disableSTDIO) && - O.mapOptional("shellExpandArguments", LRA.shellExpandArguments) && - O.mapOptional("runInTerminal", LRA.console) && - O.mapOptional("console", LRA.console) && - O.mapOptional("stdio", LRA.stdio) && parseEnv(Params, LRA.env, P); + bool success = + O && fromJSON(Params, LRA.configuration, P) && + O.mapOptional("noDebug", LRA.noDebug) && + O.mapOptional("launchCommands", LRA.launchCommands) && + O.mapOptional("cwd", LRA.cwd) && O.mapOptional("args", LRA.args) && + O.mapOptional("detachOnError", LRA.detachOnError) && + O.mapOptional("disableASLR", LRA.disableASLR) && + O.mapOptional("disableSTDIO", LRA.disableSTDIO) && + O.mapOptional("shellExpandArguments", LRA.shellExpandArguments) && + O.mapOptional("runInTerminal", LRA.console) && + O.mapOptional("console", LRA.console) && + O.mapOptional("stdio", LRA.stdio) && parseEnv(Params, LRA.env, P); + if (!success) + return false; + if (LRA.configuration.program.empty() && LRA.launchCommands.empty()) { + P.report("`program` or `launchCommands` should be provided"); + return false; + } + return true; } bool fromJSON(const json::Value &Params, AttachRequestArguments &ARA, json::Path P) { json::ObjectMapper O(Params, P); - return O && fromJSON(Params, ARA.configuration, P) && - O.mapOptional("attachCommands", ARA.attachCommands) && - O.mapOptional("pid", ARA.pid) && - O.mapOptional("waitFor", ARA.waitFor) && - O.mapOptional("gdb-remote-port", ARA.gdbRemotePort) && - O.mapOptional("gdb-remote-hostname", ARA.gdbRemoteHostname) && - O.mapOptional("coreFile", ARA.coreFile) && - O.mapOptional("targetId", ARA.targetId) && - O.mapOptional("debuggerId", ARA.debuggerId); + bool success = O && fromJSON(Params, ARA.configuration, P) && + O.mapOptional("attachCommands", ARA.attachCommands) && + O.mapOptional("pid", ARA.pid) && + O.mapOptional("waitFor", ARA.waitFor) && + O.mapOptional("gdb-remote-port", ARA.gdbRemotePort) && + O.mapOptional("gdb-remote-hostname", ARA.gdbRemoteHostname) && + O.mapOptional("coreFile", ARA.coreFile) && + O.mapOptional("targetId", ARA.targetId) && + O.mapOptional("debuggerId", ARA.debuggerId); + if (!success) + return false; + if (ARA.debuggerId.has_value()) + return true; + if (ARA.targetId.has_value()) + return true; + if ((ARA.pid == LLDB_INVALID_PROCESS_ID) && + ARA.configuration.program.empty() && ARA.attachCommands.empty() && + ARA.coreFile.empty() && (ARA.gdbRemotePort == LLDB_DAP_INVALID_PORT)) { + P.report("`pid` or `program` or `attachCommands` or `coreFile` or " + "`gdbRemotePort` should be provided"); + return false; + } + return true; } bool fromJSON(const json::Value &Params, ContinueArguments &CA, json::Path P) { @@ -737,4 +759,30 @@ llvm::json::Value toJSON(const TestGetTargetBreakpointsResponseBody &Body) { return result; } +bool fromJSON(const llvm::json::Value &Params, RestartArguments &Args, + llvm::json::Path Path) { + const json::Object *O = Params.getAsObject(); + if (!O) { + Path.report("expected object"); + return false; + } + const json::Value *arguments = O->get("arguments"); + if (arguments == nullptr) + return true; + LaunchRequestArguments launchArguments; + llvm::json::Path::Root root; + if (fromJSON(*arguments, launchArguments, root)) { + Args.arguments = std::move(launchArguments); + return true; + } + AttachRequestArguments attachArguments; + if (fromJSON(*arguments, attachArguments, root)) { + Args.arguments = std::move(attachArguments); + return true; + } + Path.report( + "failed to parse arguments, expected `launch` or `attach` arguments"); + return false; +} + } // namespace lldb_dap::protocol diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h index cc123943ec0b6..33fcaae1710b5 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h +++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h @@ -31,6 +31,7 @@ #include <cstdint> #include <optional> #include <string> +#include <variant> #include <vector> namespace lldb_dap::protocol { @@ -1255,6 +1256,18 @@ struct TestGetTargetBreakpointsResponseBody { }; llvm::json::Value toJSON(const TestGetTargetBreakpointsResponseBody &); +/// Arguments for `restart` request. +struct RestartArguments { + /// The latest version of the `launch` or `attach` configuration. + std::variant<std::monostate, LaunchRequestArguments, AttachRequestArguments> + arguments = std::monostate{}; +}; +bool fromJSON(const llvm::json::Value &, RestartArguments &, llvm::json::Path); + +/// Response to `restart` request. This is just an acknowledgement, so no body +/// field is required. +using RestartResponse = VoidResponse; + } // namespace lldb_dap::protocol #endif diff --git a/lldb/unittests/DAP/ProtocolRequestsTest.cpp b/lldb/unittests/DAP/ProtocolRequestsTest.cpp index 710b78960ef09..c639e40453fb0 100644 --- a/lldb/unittests/DAP/ProtocolRequestsTest.cpp +++ b/lldb/unittests/DAP/ProtocolRequestsTest.cpp @@ -304,3 +304,43 @@ TEST(ProtocolRequestsTest, TestGetTargetBreakpointsResponseBody) { ASSERT_THAT_EXPECTED(expected, llvm::Succeeded()); EXPECT_EQ(PrettyPrint(*expected), PrettyPrint(body)); } + +TEST(ProtocolRequestsTest, RestartArguments) { + llvm::Expected<RestartArguments> expected = parse<RestartArguments>(R"({})"); + ASSERT_THAT_EXPECTED(expected, llvm::Succeeded()); + EXPECT_TRUE(std::holds_alternative<std::monostate>(expected->arguments)); + + // Check missed keys. + expected = parse<RestartArguments>(R"({ + "arguments": {} + })"); + EXPECT_THAT_EXPECTED(expected, + FailedWithMessage("failed to parse arguments, expected " + "`launch` or `attach` arguments")); + + // Check launch arguments. + expected = parse<RestartArguments>(R"({ + "arguments": { + "program": "main.exe", + "cwd": "/home/root" + } + })"); + ASSERT_THAT_EXPECTED(expected, llvm::Succeeded()); + const LaunchRequestArguments *launch_args = + std::get_if<LaunchRequestArguments>(&expected->arguments); + EXPECT_NE(launch_args, nullptr); + EXPECT_EQ(launch_args->configuration.program, "main.exe"); + EXPECT_EQ(launch_args->cwd, "/home/root"); + + // Check attach arguments. + expected = parse<RestartArguments>(R"({ + "arguments": { + "pid": 123 + } + })"); + ASSERT_THAT_EXPECTED(expected, llvm::Succeeded()); + const AttachRequestArguments *attach_args = + std::get_if<AttachRequestArguments>(&expected->arguments); + EXPECT_NE(attach_args, nullptr); + EXPECT_EQ(attach_args->pid, 123U); +} `````````` </details> https://github.com/llvm/llvm-project/pull/172488 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
