[lldb][lldb-dap] Basic implementation of a deferred request. #140260
Conversation
almost one to one except not deferring breakpoints.
|
@llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) ChangesFixes #140154 Basic implementation of defering requests. It no longer depends on a white list of request, So if there is any future breakpoint types added to the DAP specification. it will not break existing binary. It can further be extended to defer depending of the request arguments. Full diff: https://github.com/llvm/llvm-project/pull/140260.diff 6 Files Affected:
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 56a0c38b00037..a1f738eef5fcc 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -726,7 +726,7 @@ void DAP::SetTarget(const lldb::SBTarget target) {
}
}
-bool DAP::HandleObject(const Message &M) {
+bool DAP::HandleObject(const Message &M, bool &defer) {
TelemetryDispatcher dispatcher(&debugger);
dispatcher.Set("client_name", transport.GetClientName().str());
if (const auto *req = std::get_if<Request>(&M)) {
@@ -748,7 +748,7 @@ bool DAP::HandleObject(const Message &M) {
dispatcher.Set("client_data",
llvm::Twine("request_command:", req->command).str());
if (handler_pos != request_handlers.end()) {
- handler_pos->second->Run(*req);
+ defer = handler_pos->second->Run(*req);
return true; // Success
}
@@ -918,17 +918,11 @@ llvm::Error DAP::Loop() {
// The launch sequence is special and we need to carefully handle
// packets in the right order. Until we've handled configurationDone,
- bool add_to_pending_queue = false;
-
if (const protocol::Request *req =
std::get_if<protocol::Request>(&*next)) {
llvm::StringRef command = req->command;
if (command == "disconnect")
disconnecting = true;
- if (!configuration_done)
- add_to_pending_queue =
- command != "initialize" && command != "configurationDone" &&
- command != "disconnect" && !command.ends_with("Breakpoints");
}
const std::optional<CancelArguments> cancel_args =
@@ -956,8 +950,7 @@ llvm::Error DAP::Loop() {
{
std::lock_guard<std::mutex> guard(m_queue_mutex);
- auto &queue = add_to_pending_queue ? m_pending_queue : m_queue;
- queue.push_back(std::move(*next));
+ m_queue.push_back(std::move(*next));
}
m_queue_cv.notify_one();
}
@@ -984,9 +977,13 @@ llvm::Error DAP::Loop() {
// Unlock while we're processing the event.
lock.unlock();
- if (!HandleObject(next))
+ bool defer_message = false;
+ if (!HandleObject(next, defer_message))
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"unhandled packet");
+ if (defer_message) {
+ m_pending_queue.push_back(next);
+ }
}
return ToError(queue_reader.get());
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index c1a1130b1e59f..a77a1561c5db2 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -339,7 +339,7 @@ struct DAP {
/// listeing for its breakpoint events.
void SetTarget(const lldb::SBTarget target);
- bool HandleObject(const protocol::Message &M);
+ bool HandleObject(const protocol::Message &M, bool &defer);
/// Disconnect the DAP session.
llvm::Error Disconnect();
diff --git a/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
index 0293ffbd0c922..96fb7682f240a 100644
--- a/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
@@ -160,4 +160,8 @@ void AttachRequestHandler::PostRun() const {
dap.target.GetProcess().Continue();
}
+bool AttachRequestHandler::DeferRequest() const {
+ return !dap.configuration_done;
+}
+
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/LaunchRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/LaunchRequestHandler.cpp
index 22d1a090187d8..e4163827ac69c 100644
--- a/lldb/tools/lldb-dap/Handler/LaunchRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/LaunchRequestHandler.cpp
@@ -88,4 +88,8 @@ void LaunchRequestHandler::PostRun() const {
dap.target.GetProcess().Continue();
}
+bool LaunchRequestHandler::DeferRequest() const {
+ return !dap.configuration_done;
+}
+
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
index 93bc80a38e29d..074a76c928240 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
@@ -126,7 +126,7 @@ RunInTerminal(DAP &dap, const protocol::LaunchRequestArguments &arguments) {
error.GetCString());
}
-void BaseRequestHandler::Run(const Request &request) {
+bool BaseRequestHandler::Run(const Request &request) {
// If this request was cancelled, send a cancelled response.
if (dap.IsCancelled(request)) {
Response cancelled{/*request_seq=*/request.seq,
@@ -135,12 +135,15 @@ void BaseRequestHandler::Run(const Request &request) {
/*message=*/eResponseMessageCancelled,
/*body=*/std::nullopt};
dap.Send(cancelled);
- return;
+ return false;
}
lldb::SBMutex lock = dap.GetAPIMutex();
std::lock_guard<lldb::SBMutex> guard(lock);
+ if (DeferRequest()) {
+ return true;
+ }
// FIXME: After all the requests have migrated from LegacyRequestHandler >
// RequestHandler<> we should be able to move this into
// RequestHandler<>::operator().
@@ -149,6 +152,7 @@ void BaseRequestHandler::Run(const Request &request) {
// FIXME: After all the requests have migrated from LegacyRequestHandler >
// RequestHandler<> we should be able to check `debugger.InterruptRequest` and
// mark the response as cancelled.
+ return false;
}
llvm::Error BaseRequestHandler::LaunchProcess(
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index eaebaf6619bbd..bac193e6ce59e 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -46,7 +46,11 @@ class BaseRequestHandler {
virtual ~BaseRequestHandler() = default;
- void Run(const protocol::Request &);
+ /// Return `true` if the request should be deferred.
+ [[nodiscard]]
+ bool Run(const protocol::Request &);
+
+ virtual bool DeferRequest() const { return false; };
virtual void operator()(const protocol::Request &request) const = 0;
@@ -203,6 +207,7 @@ class AttachRequestHandler
static llvm::StringLiteral GetCommand() { return "attach"; }
llvm::Error Run(const protocol::AttachRequestArguments &args) const override;
void PostRun() const override;
+ bool DeferRequest() const override;
};
class BreakpointLocationsRequestHandler
@@ -302,6 +307,7 @@ class LaunchRequestHandler
llvm::Error
Run(const protocol::LaunchRequestArguments &arguments) const override;
void PostRun() const override;
+ bool DeferRequest() const override;
};
class RestartRequestHandler : public LegacyRequestHandler {
|
| @@ -748,7 +748,7 @@ bool DAP::HandleObject(const Message &M) { | ||
| dispatcher.Set("client_data", | ||
| llvm::Twine("request_command:", req->command).str()); | ||
| if (handler_pos != request_handlers.end()) { | ||
| handler_pos->second->Run(*req); | ||
| defer = handler_pos->second->Run(*req); |
There was a problem hiding this comment.
Should we check the deferred call here like?
if (handler_pos->second->DeferRequest())
m_pending_queue.push_back(M);
else
handler_pos->second->Run(*req);| bool Run(const protocol::Request &); | ||
|
|
||
| [[nodiscard]] | ||
| virtual bool DeferRequest() const { |
There was a problem hiding this comment.
This is specifically about deferring until configurationDone is called. We don't really have support for an async request handler yet. I thought about implementing an AsyncRequestHandler that would allow the request handler to unblock the handler queue before sending a reply, but I haven't needed it just yet so I haven't sent that out for a PR.
Thats a long winded way of saying, should we make this more specific? Maybe DeferUntilConfigurationDone? I'm open to other names, just want to be clear.
There was a problem hiding this comment.
I like the idea of making the request handlers responsible for telling us whether they should be deferred compared to the allow-list I implemented. That's definitely an improvement.
What I don't like is how this introduces more statefulness, especially in the request handlers. The queueing/deferring logic isn't trivial and I would strongly prefer if we could keep it centralized as much as possible. With this PR we end up with both a separate queue and a method that makes the Run operation behaves differently depending on the global DAP state (i.e. if we've send the configuration request).
Would it be possible to have the RequestHandlers advertise whether or not they should be deferred until the configuration is done (e.g. DeferredUntilConfigurationDone), and keep the latter an implementation detail of the DAP class? And strictly handle deferring at the queue level?
PS: Small nit, but your PR is titled "Basic implementation of a deferred request." which isn't accurate because deferred requests are already implemented. A better title would be "Change the way we defer requests" or something similar.
Yes it can,
The plan is to partially resolve the request and then completing the request later on. for example. with the launch argument, the new method will be std::optional<LaunchResponse> LaunchRequestHandler::DeferUntilConfig(
const LaunchRequestArguments &arguments, bool &defer_request) const {
// setups the target and initcommands.
if (LaunchResponse err = PreLaunch(arguments)) {
return err;
}
defer_request = !dap.configuration_done;
return std::nullopt;
}since this it is then deferred it will then complete the remaining request after the configuration is done. like so Error LaunchRequestHandler::Run(const LaunchRequestArguments &arguments) const {
if (Error err = dap.RunPreRunCommands())
return err;
if (Error err = LaunchProcess(arguments))
return err;
dap.RunPostRunCommands();
return Error::success();
}The could potentially fix the breakpoints problem and also launch after configuration. Not 100% sure this is the best way hence why I have not included the commit. |
Fixes #140154
Basic implementation of defering requests.
It no longer depends on a white list of request, So if there is any future breakpoint types added to the DAP specification. it will not break existing binary.
It can further be extended to defer depending of the request arguments.