llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: John Harrison (ashgti) <details> <summary>Changes</summary> This updates the 'next' request to use well structured types. While working on this I also simplified the 'RequestHandler' implementation to better handle void responses by allowing requests to return a 'llvm::Error' instead of an 'llvm::Expected<std::monostate>'. This makes it easier to write and understand request handles that have simple ack responses. --- Full diff: https://github.com/llvm/llvm-project/pull/136642.diff 12 Files Affected: - (modified) lldb/tools/lldb-dap/DAP.cpp (+5) - (modified) lldb/tools/lldb-dap/DAP.h (+1) - (modified) lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp (+3-4) - (modified) lldb/tools/lldb-dap/Handler/DisconnectRequestHandler.cpp (+2-2) - (modified) lldb/tools/lldb-dap/Handler/NextRequestHandler.cpp (+25-60) - (modified) lldb/tools/lldb-dap/Handler/RequestHandler.h (+44-38) - (modified) lldb/tools/lldb-dap/Protocol/ProtocolBase.h (+1-1) - (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp (+8-1) - (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.h (+20) - (modified) lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp (+21) - (modified) lldb/tools/lldb-dap/Protocol/ProtocolTypes.h (+19) - (modified) lldb/tools/lldb-dap/Transport.cpp (+2-1) ``````````diff diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp index 597fe3a1e323b..134762711b89d 100644 --- a/lldb/tools/lldb-dap/DAP.cpp +++ b/lldb/tools/lldb-dap/DAP.cpp @@ -28,6 +28,7 @@ #include "lldb/Utility/Status.h" #include "lldb/lldb-defines.h" #include "lldb/lldb-enumerations.h" +#include "lldb/lldb-types.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/ScopeExit.h" @@ -499,6 +500,10 @@ ExceptionBreakpoint *DAP::GetExceptionBPFromStopReason(lldb::SBThread &thread) { return exc_bp; } +lldb::SBThread DAP::GetLLDBThread(lldb::tid_t tid) { + return target.GetProcess().GetThreadByID(tid); +} + lldb::SBThread DAP::GetLLDBThread(const llvm::json::Object &arguments) { auto tid = GetInteger<int64_t>(arguments, "threadId") .value_or(LLDB_INVALID_THREAD_ID); diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h index b79a0d9d0f25c..727e5c00623e8 100644 --- a/lldb/tools/lldb-dap/DAP.h +++ b/lldb/tools/lldb-dap/DAP.h @@ -266,6 +266,7 @@ struct DAP { ExceptionBreakpoint *GetExceptionBPFromStopReason(lldb::SBThread &thread); + lldb::SBThread GetLLDBThread(lldb::tid_t id); lldb::SBThread GetLLDBThread(const llvm::json::Object &arguments); lldb::SBFrame GetLLDBFrame(const llvm::json::Object &arguments); diff --git a/lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp index f09de13c3ff72..995fe38362f60 100644 --- a/lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp @@ -10,7 +10,7 @@ #include "Protocol/ProtocolRequests.h" #include "llvm/Support/Error.h" -using namespace lldb_dap; +using namespace llvm; using namespace lldb_dap::protocol; namespace lldb_dap { @@ -45,12 +45,11 @@ namespace lldb_dap { /// /// A client cannot assume that progress just got cancelled after sending /// the `cancel` request. -llvm::Expected<CancelResponseBody> -CancelRequestHandler::Run(const CancelArguments &arguments) const { +Error CancelRequestHandler::Run(const CancelArguments &arguments) const { // Cancel support is built into the DAP::Loop handler for detecting // cancellations of pending or inflight requests. dap.ClearCancelRequest(arguments); - return CancelResponseBody(); + return Error::success(); } } // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/Handler/DisconnectRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/DisconnectRequestHandler.cpp index f12fecfb2ff65..81e94c7551836 100644 --- a/lldb/tools/lldb-dap/Handler/DisconnectRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/DisconnectRequestHandler.cpp @@ -18,7 +18,7 @@ using namespace lldb_dap::protocol; namespace lldb_dap { /// Disconnect request; value of command field is 'disconnect'. -Expected<DisconnectResponse> DisconnectRequestHandler::Run( +Error DisconnectRequestHandler::Run( const std::optional<DisconnectArguments> &arguments) const { bool terminateDebuggee = dap.is_attach ? false : true; @@ -28,6 +28,6 @@ Expected<DisconnectResponse> DisconnectRequestHandler::Run( if (Error error = dap.Disconnect(terminateDebuggee)) return error; - return DisconnectResponse(); + return Error::success(); } } // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/Handler/NextRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/NextRequestHandler.cpp index 216e710035cb1..1603563841005 100644 --- a/lldb/tools/lldb-dap/Handler/NextRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/NextRequestHandler.cpp @@ -8,72 +8,37 @@ #include "DAP.h" #include "EventHelper.h" -#include "JSONUtils.h" +#include "Protocol/ProtocolTypes.h" #include "RequestHandler.h" +#include "llvm/Support/Error.h" + +using namespace llvm; +using namespace lldb_dap::protocol; namespace lldb_dap { -// "NextRequest": { -// "allOf": [ { "$ref": "#/definitions/Request" }, { -// "type": "object", -// "description": "Next request; value of command field is 'next'. The -// request starts the debuggee to run again for one step. -// The debug adapter first sends the NextResponse and then -// a StoppedEvent (event type 'step') after the step has -// completed.", -// "properties": { -// "command": { -// "type": "string", -// "enum": [ "next" ] -// }, -// "arguments": { -// "$ref": "#/definitions/NextArguments" -// } -// }, -// "required": [ "command", "arguments" ] -// }] -// }, -// "NextArguments": { -// "type": "object", -// "description": "Arguments for 'next' request.", -// "properties": { -// "threadId": { -// "type": "integer", -// "description": "Execute 'next' for this thread." -// }, -// "granularity": { -// "$ref": "#/definitions/SteppingGranularity", -// "description": "Stepping granularity. If no granularity is specified, a -// granularity of `statement` is assumed." -// } -// }, -// "required": [ "threadId" ] -// }, -// "NextResponse": { -// "allOf": [ { "$ref": "#/definitions/Response" }, { -// "type": "object", -// "description": "Response to 'next' request. This is just an -// acknowledgement, so no body field is required." -// }] -// } -void NextRequestHandler::operator()(const llvm::json::Object &request) const { - llvm::json::Object response; - FillResponse(request, response); - const auto *arguments = request.getObject("arguments"); - lldb::SBThread thread = dap.GetLLDBThread(*arguments); - if (thread.IsValid()) { - // Remember the thread ID that caused the resume so we can set the - // "threadCausedFocus" boolean value in the "stopped" events. - dap.focus_tid = thread.GetThreadID(); - if (HasInstructionGranularity(*arguments)) { - thread.StepInstruction(/*step_over=*/true); - } else { - thread.StepOver(); - } +/// The request executes one step (in the given granularity) for the specified +/// thread and allows all other threads to run freely by resuming them. If the +/// debug adapter supports single thread execution (see capability +/// `supportsSingleThreadExecutionRequests`), setting the `singleThread` +/// argument to true prevents other suspended threads from resuming. The debug +/// adapter first sends the response and then a `stopped` event (with reason +/// `step`) after the step has completed. +Error NextRequestHandler::Run(const NextArguments &args) const { + lldb::SBThread thread = dap.GetLLDBThread(args.threadId); + if (!thread.IsValid()) + return make_error<DAPError>("invalid thread"); + + // Remember the thread ID that caused the resume so we can set the + // "threadCausedFocus" boolean value in the "stopped" events. + dap.focus_tid = thread.GetThreadID(); + if (args.granularity == eSteppingGranularityInstruction) { + thread.StepInstruction(/*step_over=*/true); } else { - response["success"] = llvm::json::Value(false); + thread.StepOver(); } - dap.SendJSON(llvm::json::Value(std::move(response))); + + return Error::success(); } } // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h index 7e56c258ad78a..edb9de7d0dc20 100644 --- a/lldb/tools/lldb-dap/Handler/RequestHandler.h +++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h @@ -94,10 +94,10 @@ class LegacyRequestHandler : public BaseRequestHandler { /// Base class for handling DAP requests. Handlers should declare their /// arguments and response body types like: /// -/// class MyRequestHandler : public RequestHandler<Arguments, ResponseBody> { +/// class MyRequestHandler : public RequestHandler<Arguments, Response> { /// .... /// }; -template <typename Args, typename Body> +template <typename Args, typename Resp> class RequestHandler : public BaseRequestHandler { using BaseRequestHandler::BaseRequestHandler; @@ -128,41 +128,29 @@ class RequestHandler : public BaseRequestHandler { << "': " << llvm::toString(root.getError()) << "\n"; root.printErrorContext(*request.arguments, OS); - protocol::ErrorMessage error_message; - error_message.format = parse_failure; - - protocol::ErrorResponseBody body; - body.error = error_message; - response.success = false; - response.body = std::move(body); + response.body = ToResponse(llvm::make_error<DAPError>(parse_failure)); dap.Send(response); return; } - llvm::Expected<Body> body = Run(arguments); - if (auto Err = body.takeError()) { - protocol::ErrorMessage error_message; - error_message.sendTelemetry = false; - if (llvm::Error unhandled = llvm::handleErrors( - std::move(Err), [&](const DAPError &E) -> llvm::Error { - error_message.format = E.getMessage(); - error_message.showUser = E.getShowUser(); - error_message.id = E.convertToErrorCode().value(); - error_message.url = E.getURL(); - error_message.urlLabel = E.getURLLabel(); - return llvm::Error::success(); - })) - error_message.format = llvm::toString(std::move(unhandled)); - protocol::ErrorResponseBody body; - body.error = error_message; - response.success = false; - response.body = std::move(body); + if constexpr (std::is_same_v<Resp, llvm::Error>) { + if (llvm::Error err = Run(arguments)) { + response.success = false; + response.body = ToResponse(std::move(err)); + } else { + response.success = true; + } } else { - response.success = true; - if constexpr (!std::is_same_v<Body, std::monostate>) + Resp body = Run(arguments); + if (llvm::Error err = body.takeError()) { + response.success = false; + response.body = ToResponse(std::move(err)); + } else { + response.success = true; response.body = std::move(*body); + } } // Mark the request as 'cancelled' if the debugger was interrupted while @@ -177,7 +165,25 @@ class RequestHandler : public BaseRequestHandler { dap.Send(response); }; - virtual llvm::Expected<Body> Run(const Args &) const = 0; + virtual Resp Run(const Args &) const = 0; + + protocol::ErrorResponseBody ToResponse(llvm::Error err) const { + protocol::ErrorMessage error_message; + error_message.sendTelemetry = false; + if (llvm::Error unhandled = llvm::handleErrors( + std::move(err), [&](const DAPError &E) -> llvm::Error { + error_message.format = E.getMessage(); + error_message.showUser = E.getShowUser(); + error_message.id = E.convertToErrorCode().value(); + error_message.url = E.getURL(); + error_message.urlLabel = E.getURLLabel(); + return llvm::Error::success(); + })) + error_message.format = llvm::toString(std::move(unhandled)); + protocol::ErrorResponseBody body; + body.error = error_message; + return body; + } }; class AttachRequestHandler : public LegacyRequestHandler { @@ -233,7 +239,7 @@ class DisconnectRequestHandler FeatureSet GetSupportedFeatures() const override { return {protocol::eAdapterFeatureTerminateDebuggee}; } - llvm::Expected<protocol::DisconnectResponse> + llvm::Error Run(const std::optional<protocol::DisconnectArguments> &args) const override; }; @@ -259,7 +265,7 @@ class ExceptionInfoRequestHandler : public LegacyRequestHandler { class InitializeRequestHandler : public RequestHandler<protocol::InitializeRequestArguments, - protocol::InitializeResponseBody> { + llvm::Expected<protocol::InitializeResponseBody>> { public: using RequestHandler::RequestHandler; static llvm::StringLiteral GetCommand() { return "initialize"; } @@ -284,11 +290,12 @@ class RestartRequestHandler : public LegacyRequestHandler { void operator()(const llvm::json::Object &request) const override; }; -class NextRequestHandler : public LegacyRequestHandler { +class NextRequestHandler + : public RequestHandler<protocol::NextArguments, protocol::NextResponse> { public: - using LegacyRequestHandler::LegacyRequestHandler; + using RequestHandler::RequestHandler; static llvm::StringLiteral GetCommand() { return "next"; } - void operator()(const llvm::json::Object &request) const override; + llvm::Error Run(const protocol::NextArguments &args) const override; }; class StepInRequestHandler : public LegacyRequestHandler { @@ -418,7 +425,7 @@ class SetVariableRequestHandler : public LegacyRequestHandler { class SourceRequestHandler : public RequestHandler<protocol::SourceArguments, - protocol::SourceResponseBody> { + llvm::Expected<protocol::SourceResponseBody>> { public: using RequestHandler::RequestHandler; static llvm::StringLiteral GetCommand() { return "source"; } @@ -486,8 +493,7 @@ class CancelRequestHandler FeatureSet GetSupportedFeatures() const override { return {protocol::eAdapterFeatureCancelRequest}; } - llvm::Expected<protocol::CancelResponseBody> - Run(const protocol::CancelArguments &args) const override; + llvm::Error Run(const protocol::CancelArguments &args) const override; }; /// A request used in testing to get the details on all breakpoints that are diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolBase.h b/lldb/tools/lldb-dap/Protocol/ProtocolBase.h index 2c647610de11c..bad0e886d94d2 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolBase.h +++ b/lldb/tools/lldb-dap/Protocol/ProtocolBase.h @@ -149,7 +149,7 @@ struct ErrorResponseBody { llvm::json::Value toJSON(const ErrorResponseBody &); /// This is just an acknowledgement, so no body field is required. -using VoidResponse = std::monostate; +using VoidResponse = llvm::Error; } // namespace lldb_dap::protocol diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp index 3523f8ac87ec9..b113299affb0f 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp +++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp @@ -7,7 +7,6 @@ //===----------------------------------------------------------------------===// #include "Protocol/ProtocolRequests.h" -#include "DAP.h" #include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/JSON.h" @@ -114,4 +113,12 @@ json::Value toJSON(const SourceResponseBody &SA) { return std::move(Result); } +bool fromJSON(const llvm::json::Value &Params, NextArguments &NA, + llvm::json::Path P) { + json::ObjectMapper OM(Params, P); + return OM && OM.map("threadId", NA.threadId) && + OM.mapOptional("singleThread", NA.singleThread) && + OM.mapOptional("granularity", NA.granularity); +} + } // namespace lldb_dap::protocol diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h index 6623dfa0db05c..6e3e2c6a9e2c8 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h +++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h @@ -22,6 +22,7 @@ #include "Protocol/ProtocolBase.h" #include "Protocol/ProtocolTypes.h" +#include "lldb/lldb-defines.h" #include "llvm/ADT/DenseSet.h" #include "llvm/Support/JSON.h" #include <cstdint> @@ -236,6 +237,25 @@ struct SourceResponseBody { }; llvm::json::Value toJSON(const SourceResponseBody &); +/// Arguments for `next` request. +struct NextArguments { + /// Specifies the thread for which to resume execution for one step (of the + /// given granularity). + uint64_t threadId = LLDB_INVALID_THREAD_ID; + + /// If this flag is true, all other suspended threads are not resumed. + bool singleThread = false; + + /// Stepping granularity. If no granularity is specified, a granularity of + /// `statement` is assumed. + SteppingGranularity granularity = eSteppingGranularityStatement; +}; +bool fromJSON(const llvm::json::Value &, NextArguments &, llvm::json::Path); + +/// Response to `next` request. This is just an acknowledgement, so no +/// body field is required. +using NextResponse = VoidResponse; + } // namespace lldb_dap::protocol #endif diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp index 4d1e90215bbb4..e64998c4ca488 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp +++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp @@ -233,4 +233,25 @@ json::Value toJSON(const Capabilities &C) { return result; } +bool fromJSON(const llvm::json::Value &Params, SteppingGranularity &SG, + llvm::json::Path P) { + auto raw_granularity = Params.getAsString(); + if (!raw_granularity) { + P.report("expected a string"); + return false; + } + std::optional<SteppingGranularity> granularity = + StringSwitch<std::optional<SteppingGranularity>>(*raw_granularity) + .Case("statement", eSteppingGranularityStatement) + .Case("line", eSteppingGranularityLine) + .Case("instruction", eSteppingGranularityInstruction) + .Default(std::nullopt); + if (!granularity) { + P.report("unexpected value"); + return false; + } + SG = *granularity; + return true; +} + } // namespace lldb_dap::protocol diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h index 8f38c524ea649..54941f24efbd9 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h +++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h @@ -303,6 +303,25 @@ struct Source { }; bool fromJSON(const llvm::json::Value &, Source &, llvm::json::Path); +/// The granularity of one `step` in the stepping requests `next`, `stepIn`, +/// `stepOut` and `stepBack`. +enum SteppingGranularity : unsigned { + /// The step should allow the program to run until the current statement has + /// finished executing. The meaning of a statement is determined by the + /// adapter and it may be considered equivalent to a line. For example + /// `for(int i = 0; i < 10; i++)` could be considered to have 3 statements + /// `int i = 0`, `i < 10`, and `i++`. + eSteppingGranularityStatement, + /// The step should allow the program to run until the current source line has + /// executed. + eSteppingGranularityLine, + /// The step should allow one instruction to execute (e.g. one x86 + /// instruction). + eSteppingGranularityInstruction, +}; +bool fromJSON(const llvm::json::Value &, SteppingGranularity &, + llvm::json::Path); + } // namespace lldb_dap::protocol #endif diff --git a/lldb/tools/lldb-dap/Transport.cpp b/lldb/tools/lldb-dap/Transport.cpp index ffd0c49f1770b..4e322e9ff1358 100644 --- a/lldb/tools/lldb-dap/Transport.cpp +++ b/lldb/tools/lldb-dap/Transport.cpp @@ -137,7 +137,8 @@ Expected<Message> Transport::Read(const std::chrono::microseconds &timeout) { DAP_LOG(m_log, "--> ({0}) {1}", m_client_name, *raw_json); - return json::parse<Message>(*raw_json); + return json::parse<Message>(/*JSON=*/*raw_json, + /*RootName=*/"protocol_message"); } Error Transport::Write(const Message &message) { `````````` </details> https://github.com/llvm/llvm-project/pull/136642 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
