https://github.com/ashgti created https://github.com/llvm/llvm-project/pull/164306
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. >From a29c5c93ac2d1adedd8cce923693c7f5e78b151e Mon Sep 17 00:00:00 2001 From: John Harrison <[email protected]> Date: Mon, 20 Oct 2025 10:58:03 -0700 Subject: [PATCH] [lldb-dap] Correct lldb-dap `seq` handling. 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. --- lldb/tools/lldb-dap/DAP.cpp | 62 +++++++++++-------- lldb/tools/lldb-dap/DAP.h | 25 +++----- lldb/tools/lldb-dap/EventHelper.cpp | 6 +- .../tools/lldb-dap/Handler/RequestHandler.cpp | 3 +- lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp | 29 +++------ lldb/tools/lldb-dap/Protocol/ProtocolBase.h | 20 +++++- lldb/unittests/DAP/DAPTest.cpp | 2 +- 7 files changed, 80 insertions(+), 67 deletions(-) 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(); } _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
