[lldb-dap] Correct lldb-dap seq handling.
#164306
Merged
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
We've been treating the `seq` attribute incorrectly in lldb-dap. Previously, we always had `seq=0` for events and responses. We only filled in the `seq` field on reverse requests. However, looking at the spec and other DAP implementations, we are supposed to fill in the `seq` field for each request we send to the DAP client. I've updated our usage to fill in `seq` in `DAP::Send` so that all events/requests/responses have a properly filled `seq` value.
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesWe've been treating the However, looking at the spec and other DAP implementations, we are supposed to fill in the I've updated our usage to fill in Full diff: https://github.com/llvm/llvm-project/pull/164306.diff 7 Files Affected:
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index f76656e98ca01..7a960aea115c0 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -266,40 +266,47 @@ void DAP::SendJSON(const llvm::json::Value &json) {
Send(message);
}
-void DAP::Send(const Message &message) {
- if (const protocol::Event *event = std::get_if<protocol::Event>(&message)) {
- if (llvm::Error err = transport.Send(*event))
+Id DAP::Send(const Message &message) {
+ std::lock_guard<std::mutex> guard(call_mutex);
+ if (const protocol::Event *e = std::get_if<protocol::Event>(&message)) {
+ protocol::Event event = *e;
+ event.seq = seq++;
+ if (llvm::Error err = transport.Send(event))
DAP_LOG_ERROR(log, std::move(err), "({0}) sending event failed",
m_client_name);
- return;
+ return event.seq;
}
- if (const Request *req = std::get_if<Request>(&message)) {
- if (llvm::Error err = transport.Send(*req))
+ if (const Request *r = std::get_if<Request>(&message)) {
+ Request req = *r;
+ req.seq = seq++;
+ if (llvm::Error err = transport.Send(req))
DAP_LOG_ERROR(log, std::move(err), "({0}) sending request failed",
m_client_name);
- return;
+ return req.seq;
}
- if (const Response *resp = std::get_if<Response>(&message)) {
+ if (const Response *r = std::get_if<Response>(&message)) {
+ Response resp = *r;
+ resp.seq = seq++;
// FIXME: After all the requests have migrated from LegacyRequestHandler >
// RequestHandler<> this should be handled in RequestHandler<>::operator().
// If the debugger was interrupted, convert this response into a
// 'cancelled' response because we might have a partial result.
- llvm::Error err =
- (debugger.InterruptRequested())
- ? transport.Send({/*request_seq=*/resp->request_seq,
- /*command=*/resp->command,
- /*success=*/false,
- /*message=*/eResponseMessageCancelled,
- /*body=*/std::nullopt})
- : transport.Send(*resp);
- if (err) {
+ llvm::Error err = (debugger.InterruptRequested())
+ ? transport.Send({
+ /*seq=*/resp.seq,
+ /*request_seq=*/resp.request_seq,
+ /*command=*/resp.command,
+ /*success=*/false,
+ /*message=*/eResponseMessageCancelled,
+ /*body=*/std::nullopt,
+ })
+ : transport.Send(resp);
+ if (err)
DAP_LOG_ERROR(log, std::move(err), "({0}) sending response failed",
m_client_name);
- return;
- }
- return;
+ return resp.seq;
}
llvm_unreachable("Unexpected message type");
@@ -1453,17 +1460,20 @@ void DAP::EventThread() {
if (remove_module && module_exists) {
modules.erase(module_id);
Send(protocol::Event{
- "module", ModuleEventBody{std::move(p_module).value(),
- ModuleEventBody::eReasonRemoved}});
+ 0, "module",
+ ModuleEventBody{std::move(p_module).value(),
+ ModuleEventBody::eReasonRemoved}});
} else if (module_exists) {
Send(protocol::Event{
- "module", ModuleEventBody{std::move(p_module).value(),
- ModuleEventBody::eReasonChanged}});
+ 0, "module",
+ ModuleEventBody{std::move(p_module).value(),
+ ModuleEventBody::eReasonChanged}});
} else if (!remove_module) {
modules.insert(module_id);
Send(protocol::Event{
- "module", ModuleEventBody{std::move(p_module).value(),
- ModuleEventBody::eReasonNew}});
+ 0, "module",
+ ModuleEventBody{std::move(p_module).value(),
+ ModuleEventBody::eReasonNew}});
}
}
}
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index a90ddf59671ee..4acb61dd11f59 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -136,7 +136,7 @@ struct DAP final : public DAPTransport::MessageHandler {
/// unless we send a "thread" event to indicate the thread exited.
llvm::DenseSet<lldb::tid_t> thread_ids;
- uint32_t reverse_request_seq = 0;
+ uint32_t seq = 0;
std::mutex call_mutex;
llvm::SmallDenseMap<int64_t, std::unique_ptr<ResponseHandler>>
inflight_reverse_requests;
@@ -220,8 +220,8 @@ struct DAP final : public DAPTransport::MessageHandler {
/// Serialize the JSON value into a string and send the JSON packet to the
/// "out" stream.
void SendJSON(const llvm::json::Value &json);
- /// Send the given message to the client
- void Send(const protocol::Message &message);
+ /// Send the given message to the client.
+ protocol::Id Send(const protocol::Message &message);
void SendOutput(OutputType o, const llvm::StringRef output);
@@ -353,19 +353,14 @@ struct DAP final : public DAPTransport::MessageHandler {
template <typename Handler>
void SendReverseRequest(llvm::StringRef command,
llvm::json::Value arguments) {
- int64_t id;
- {
- std::lock_guard<std::mutex> locker(call_mutex);
- id = ++reverse_request_seq;
- inflight_reverse_requests[id] = std::make_unique<Handler>(command, id);
- }
-
- SendJSON(llvm::json::Object{
- {"type", "request"},
- {"seq", id},
- {"command", command},
- {"arguments", std::move(arguments)},
+ protocol::Id id = Send(protocol::Request{
+ 0,
+ command.str(),
+ std::move(arguments),
});
+
+ std::lock_guard<std::mutex> locker(call_mutex);
+ inflight_reverse_requests[id] = std::make_unique<Handler>(command, id);
}
/// The set of capabilities supported by this adapter.
diff --git a/lldb/tools/lldb-dap/EventHelper.cpp b/lldb/tools/lldb-dap/EventHelper.cpp
index 2b9ed229405a8..0c6a38560e3ae 100644
--- a/lldb/tools/lldb-dap/EventHelper.cpp
+++ b/lldb/tools/lldb-dap/EventHelper.cpp
@@ -70,7 +70,7 @@ void SendExtraCapabilities(DAP &dap) {
// Only notify the client if supportedFeatures changed.
if (!body.capabilities.supportedFeatures.empty())
- dap.Send(protocol::Event{"capabilities", std::move(body)});
+ dap.Send(protocol::Event{0, "capabilities", std::move(body)});
}
// "ProcessEvent": {
@@ -281,7 +281,7 @@ void SendInvalidatedEvent(
return;
protocol::InvalidatedEventBody body;
body.areas = areas;
- dap.Send(protocol::Event{"invalidated", std::move(body)});
+ dap.Send(protocol::Event{0, "invalidated", std::move(body)});
}
void SendMemoryEvent(DAP &dap, lldb::SBValue variable) {
@@ -292,7 +292,7 @@ void SendMemoryEvent(DAP &dap, lldb::SBValue variable) {
body.count = variable.GetByteSize();
if (body.memoryReference == LLDB_INVALID_ADDRESS)
return;
- dap.Send(protocol::Event{"memory", std::move(body)});
+ dap.Send(protocol::Event{0, "memory", std::move(body)});
}
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
index de63c9d78efd1..0ebd607d8163c 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
@@ -157,7 +157,8 @@ RunInTerminal(DAP &dap, const protocol::LaunchRequestArguments &arguments) {
void BaseRequestHandler::Run(const Request &request) {
// If this request was cancelled, send a cancelled response.
if (dap.IsCancelled(request)) {
- Response cancelled{/*request_seq=*/request.seq,
+ Response cancelled{/*seq=*/0,
+ /*request_seq=*/request.seq,
/*command=*/request.command,
/*success=*/false,
/*message=*/eResponseMessageCancelled,
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp
index 9cd9028d879e9..3b39617b981c0 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp
@@ -104,7 +104,7 @@ bool operator==(const Request &a, const Request &b) {
json::Value toJSON(const Response &R) {
json::Object Result{{"type", "response"},
- {"seq", 0},
+ {"seq", R.seq},
{"command", R.command},
{"request_seq", R.request_seq},
{"success", R.success}};
@@ -132,8 +132,9 @@ json::Value toJSON(const Response &R) {
return std::move(Result);
}
-bool fromJSON(json::Value const &Params,
- std::variant<ResponseMessage, std::string> &M, json::Path P) {
+static bool fromJSON(json::Value const &Params,
+ std::variant<ResponseMessage, std::string> &M,
+ json::Path P) {
auto rawMessage = Params.getAsString();
if (!rawMessage) {
P.report("expected a string");
@@ -157,8 +158,7 @@ bool fromJSON(json::Value const &Params, Response &R, json::Path P) {
return false;
MessageType type;
- int64_t seq;
- if (!O.map("type", type) || !O.map("seq", seq) ||
+ if (!O.map("type", type) || !O.map("seq", R.seq) ||
!O.map("command", R.command) || !O.map("request_seq", R.request_seq))
return false;
@@ -168,12 +168,7 @@ bool fromJSON(json::Value const &Params, Response &R, json::Path P) {
}
if (R.command.empty()) {
- P.field("command").report("expected to not be ''");
- return false;
- }
-
- if (R.request_seq == 0) {
- P.field("request_seq").report("expected to not be '0'");
+ P.field("command").report("expected to not be empty");
return false;
}
@@ -219,7 +214,7 @@ bool fromJSON(json::Value const &Params, ErrorMessage &EM, json::Path P) {
json::Value toJSON(const Event &E) {
json::Object Result{
{"type", "event"},
- {"seq", 0},
+ {"seq", E.seq},
{"event", E.event},
};
@@ -235,8 +230,7 @@ bool fromJSON(json::Value const &Params, Event &E, json::Path P) {
return false;
MessageType type;
- int64_t seq;
- if (!O.map("type", type) || !O.map("seq", seq) || !O.map("event", E.event))
+ if (!O.map("type", type) || !O.map("seq", E.seq) || !O.map("event", E.event))
return false;
if (type != eMessageTypeEvent) {
@@ -244,13 +238,8 @@ bool fromJSON(json::Value const &Params, Event &E, json::Path P) {
return false;
}
- if (seq != 0) {
- P.field("seq").report("expected to be '0'");
- return false;
- }
-
if (E.event.empty()) {
- P.field("event").report("expected to not be ''");
+ P.field("event").report("expected to not be empty");
return false;
}
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolBase.h b/lldb/tools/lldb-dap/Protocol/ProtocolBase.h
index 92e41b1dbf595..cf76d506ee054 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolBase.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolBase.h
@@ -41,7 +41,7 @@ struct Request {
/// associate requests with their corresponding responses. For protocol
/// messages of type `request` the sequence number can be used to cancel the
/// request.
- Id seq;
+ Id seq = 0;
/// The command to execute.
std::string command;
@@ -58,6 +58,15 @@ bool operator==(const Request &, const Request &);
/// A debug adapter initiated event.
struct Event {
+ /// Sequence number of the message (also known as message ID). The `seq` for
+ /// the first message sent by a client or debug adapter is 1, and for each
+ /// subsequent message is 1 greater than the previous message sent by that
+ /// actor. `seq` can be used to order requests, responses, and events, and to
+ /// associate requests with their corresponding responses. For protocol
+ /// messages of type `request` the sequence number can be used to cancel the
+ /// request.
+ Id seq = 0;
+
/// Type of event.
std::string event;
@@ -77,6 +86,15 @@ enum ResponseMessage : unsigned {
/// Response for a request.
struct Response {
+ /// Sequence number of the message (also known as message ID). The `seq` for
+ /// the first message sent by a client or debug adapter is 1, and for each
+ /// subsequent message is 1 greater than the previous message sent by that
+ /// actor. `seq` can be used to order requests, responses, and events, and to
+ /// associate requests with their corresponding responses. For protocol
+ /// messages of type `request` the sequence number can be used to cancel the
+ /// request.
+ Id seq = 0;
+
/// Sequence number of the corresponding request.
Id request_seq;
diff --git a/lldb/unittests/DAP/DAPTest.cpp b/lldb/unittests/DAP/DAPTest.cpp
index 4fd6cd546e6fa..dd610db299134 100644
--- a/lldb/unittests/DAP/DAPTest.cpp
+++ b/lldb/unittests/DAP/DAPTest.cpp
@@ -21,7 +21,7 @@ using namespace testing;
class DAPTest : public TransportBase {};
TEST_F(DAPTest, SendProtocolMessages) {
- dap->Send(Event{/*event=*/"my-event", /*body=*/std::nullopt});
+ dap->Send(Event{0, /*event=*/"my-event", /*body=*/std::nullopt});
EXPECT_CALL(client, Received(IsEvent("my-event")));
Run();
}
|
|
Nice, seems like this should fix #135101 |
JDevlieghere
approved these changes
Oct 20, 2025
There was a problem hiding this comment.
LGTM.
I dislike that we still have to pass 0 as the sequence number when initializing the data types. Do you think it's worthwhile to have a define/constant, like LLDB_INVALID_SEQ or LLDB_COMPUTE_SEQ and have a matching check/assert in the Send method that checks that we use the correct value?
…structs so it can be initialized to a default value and simplifies creating messages using initializer lists.
…ional checks on `seq=0` from slipping into a message.
I moved the |
JDevlieghere
approved these changes
Oct 21, 2025
Lukacma
pushed a commit
to Lukacma/llvm-project
that referenced
this pull request
Oct 29, 2025
We've been treating the `seq` attribute incorrectly in lldb-dap. Previously, we always had `seq=0` for events and responses. We only filled in the `seq` field on reverse requests. However, looking at the spec and other DAP implementations, we are supposed to fill in the `seq` field for each request we send to the DAP client. I've updated our usage to fill in `seq` in `DAP::Send` so that all events/requests/responses have a properly filled `seq` value.
aokblast
pushed a commit
to aokblast/llvm-project
that referenced
this pull request
Oct 30, 2025
We've been treating the `seq` attribute incorrectly in lldb-dap. Previously, we always had `seq=0` for events and responses. We only filled in the `seq` field on reverse requests. However, looking at the spec and other DAP implementations, we are supposed to fill in the `seq` field for each request we send to the DAP client. I've updated our usage to fill in `seq` in `DAP::Send` so that all events/requests/responses have a properly filled `seq` value.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
We've been treating the
seqattribute incorrectly in lldb-dap. Previously, we always hadseq=0for events and responses. We only filled in theseqfield on reverse requests.However, looking at the spec and other DAP implementations, we are supposed to fill in the
seqfield for each request we send to the DAP client.I've updated our usage to fill in
seqinDAP::Sendso that all events/requests/responses have a properly filledseqvalue.