From a29c5c93ac2d1adedd8cce923693c7f5e78b151e Mon Sep 17 00:00:00 2001 From: John Harrison Date: Mon, 20 Oct 2025 10:58:03 -0700 Subject: [PATCH 1/5] [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(&message)) { - if (llvm::Error err = transport.Send(*event)) +Id DAP::Send(const Message &message) { + std::lock_guard guard(call_mutex); + if (const protocol::Event *e = std::get_if(&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(&message)) { - if (llvm::Error err = transport.Send(*req)) + if (const Request *r = std::get_if(&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(&message)) { + if (const Response *r = std::get_if(&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 thread_ids; - uint32_t reverse_request_seq = 0; + uint32_t seq = 0; std::mutex call_mutex; llvm::SmallDenseMap> 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 void SendReverseRequest(llvm::StringRef command, llvm::json::Value arguments) { - int64_t id; - { - std::lock_guard locker(call_mutex); - id = ++reverse_request_seq; - inflight_reverse_requests[id] = std::make_unique(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 locker(call_mutex); + inflight_reverse_requests[id] = std::make_unique(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 &M, json::Path P) { +static bool fromJSON(json::Value const &Params, + std::variant &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(); } From 89e4f8ed66aa2ca34c3bc21b0c6cfca524f99db6 Mon Sep 17 00:00:00 2001 From: John Harrison Date: Tue, 21 Oct 2025 13:55:14 -0700 Subject: [PATCH 2/5] Moving the `seq` field to the end of the Request, Response and Event structs so it can be initialized to a default value and simplifies creating messages using initializer lists. --- lldb/tools/lldb-dap/DAP.cpp | 26 ++++---- lldb/tools/lldb-dap/DAP.h | 1 - lldb/tools/lldb-dap/EventHelper.cpp | 6 +- .../tools/lldb-dap/Handler/RequestHandler.cpp | 12 ++-- lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp | 5 ++ lldb/tools/lldb-dap/Protocol/ProtocolBase.h | 66 ++++++++++--------- 6 files changed, 63 insertions(+), 53 deletions(-) diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp index 7a960aea115c0..08f2996a249c6 100644 --- a/lldb/tools/lldb-dap/DAP.cpp +++ b/lldb/tools/lldb-dap/DAP.cpp @@ -270,7 +270,8 @@ Id DAP::Send(const Message &message) { std::lock_guard guard(call_mutex); if (const protocol::Event *e = std::get_if(&message)) { protocol::Event event = *e; - event.seq = seq++; + if (event.seq == kCalculateSeq) + event.seq = seq++; if (llvm::Error err = transport.Send(event)) DAP_LOG_ERROR(log, std::move(err), "({0}) sending event failed", m_client_name); @@ -279,7 +280,8 @@ Id DAP::Send(const Message &message) { if (const Request *r = std::get_if(&message)) { Request req = *r; - req.seq = seq++; + if (req.seq == kCalculateSeq) + req.seq = seq++; if (llvm::Error err = transport.Send(req)) DAP_LOG_ERROR(log, std::move(err), "({0}) sending request failed", m_client_name); @@ -288,19 +290,20 @@ Id DAP::Send(const Message &message) { if (const Response *r = std::get_if(&message)) { Response resp = *r; - resp.seq = seq++; + if (resp.seq == kCalculateSeq) + 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({ - /*seq=*/resp.seq, /*request_seq=*/resp.request_seq, /*command=*/resp.command, /*success=*/false, /*message=*/eResponseMessageCancelled, /*body=*/std::nullopt, + /*seq=*/resp.seq, }) : transport.Send(resp); if (err) @@ -1460,20 +1463,17 @@ void DAP::EventThread() { if (remove_module && module_exists) { modules.erase(module_id); Send(protocol::Event{ - 0, "module", - ModuleEventBody{std::move(p_module).value(), - ModuleEventBody::eReasonRemoved}}); + "module", ModuleEventBody{std::move(p_module).value(), + ModuleEventBody::eReasonRemoved}}); } else if (module_exists) { Send(protocol::Event{ - 0, "module", - ModuleEventBody{std::move(p_module).value(), - ModuleEventBody::eReasonChanged}}); + "module", ModuleEventBody{std::move(p_module).value(), + ModuleEventBody::eReasonChanged}}); } else if (!remove_module) { modules.insert(module_id); Send(protocol::Event{ - 0, "module", - ModuleEventBody{std::move(p_module).value(), - ModuleEventBody::eReasonNew}}); + "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 4acb61dd11f59..a718427e1221b 100644 --- a/lldb/tools/lldb-dap/DAP.h +++ b/lldb/tools/lldb-dap/DAP.h @@ -354,7 +354,6 @@ struct DAP final : public DAPTransport::MessageHandler { void SendReverseRequest(llvm::StringRef command, llvm::json::Value arguments) { protocol::Id id = Send(protocol::Request{ - 0, command.str(), std::move(arguments), }); diff --git a/lldb/tools/lldb-dap/EventHelper.cpp b/lldb/tools/lldb-dap/EventHelper.cpp index 0c6a38560e3ae..2b9ed229405a8 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{0, "capabilities", std::move(body)}); + dap.Send(protocol::Event{"capabilities", std::move(body)}); } // "ProcessEvent": { @@ -281,7 +281,7 @@ void SendInvalidatedEvent( return; protocol::InvalidatedEventBody body; body.areas = areas; - dap.Send(protocol::Event{0, "invalidated", std::move(body)}); + dap.Send(protocol::Event{"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{0, "memory", std::move(body)}); + dap.Send(protocol::Event{"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 0ebd607d8163c..d67437ad5b3ae 100644 --- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp @@ -157,12 +157,12 @@ 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{/*seq=*/0, - /*request_seq=*/request.seq, - /*command=*/request.command, - /*success=*/false, - /*message=*/eResponseMessageCancelled, - /*body=*/std::nullopt}; + Response cancelled{ + /*request_seq=*/request.seq, + /*command=*/request.command, + /*success=*/false, + /*message=*/eResponseMessageCancelled, + }; dap.Send(cancelled); return; } diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp index 3b39617b981c0..e0c2a15c23549 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp +++ b/lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp @@ -58,6 +58,8 @@ bool fromJSON(const json::Value &Params, MessageType &M, json::Path P) { } json::Value toJSON(const Request &R) { + assert(R.seq != kCalculateSeq); + json::Object Result{ {"type", "request"}, {"seq", R.seq}, @@ -103,6 +105,7 @@ bool operator==(const Request &a, const Request &b) { } json::Value toJSON(const Response &R) { + assert(R.seq != kCalculateSeq); json::Object Result{{"type", "response"}, {"seq", R.seq}, {"command", R.command}, @@ -212,6 +215,8 @@ bool fromJSON(json::Value const &Params, ErrorMessage &EM, json::Path P) { } json::Value toJSON(const Event &E) { + assert(E.seq != kCalculateSeq); + json::Object Result{ {"type", "event"}, {"seq", E.seq}, diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolBase.h b/lldb/tools/lldb-dap/Protocol/ProtocolBase.h index cf76d506ee054..42c6c8890af24 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolBase.h +++ b/lldb/tools/lldb-dap/Protocol/ProtocolBase.h @@ -30,19 +30,15 @@ namespace lldb_dap::protocol { // MARK: Base Protocol +/// Message unique identifier type. using Id = int64_t; +/// A unique identifier that indicates the `seq` field should be calculated by +/// the current session. +static constexpr Id kCalculateSeq = INT64_MAX; + /// A client or debug adapter initiated request. struct Request { - /// 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; - /// The command to execute. std::string command; @@ -50,7 +46,16 @@ struct Request { /// /// Request handlers are expected to validate the arguments, which is handled /// by `RequestHandler`. - std::optional arguments; + std::optional arguments = std::nullopt; + + /// 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 = kCalculateSeq; }; llvm::json::Value toJSON(const Request &); bool fromJSON(const llvm::json::Value &, Request &, llvm::json::Path); @@ -58,6 +63,12 @@ bool operator==(const Request &, const Request &); /// A debug adapter initiated event. struct Event { + /// Type of event. + std::string event; + + /// Event-specific information. + std::optional body = std::nullopt; + /// 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 @@ -65,13 +76,7 @@ struct Event { /// 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; - - /// Event-specific information. - std::optional body; + Id seq = kCalculateSeq; }; llvm::json::Value toJSON(const Event &); bool fromJSON(const llvm::json::Value &, Event &, llvm::json::Path); @@ -86,17 +91,8 @@ 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; + Id request_seq = 0; /// The command requested. std::string command; @@ -105,21 +101,31 @@ struct Response { /// attribute may contain the result of the request. If the value is false, /// the attribute `message` contains the error in short form and the `body` /// may contain additional information (see `ErrorMessage`). - bool success; + bool success = false; // FIXME: Migrate usage of fallback string to ErrorMessage /// Contains the raw error in short form if `success` is false. This raw error /// might be interpreted by the client and is not shown in the UI. Some /// predefined values exist. - std::optional> message; + std::optional> message = + std::nullopt; /// Contains request result if success is true and error details if success is /// false. /// /// Request handlers are expected to build an appropriate body, see /// `RequestHandler`. - std::optional body; + std::optional body = std::nullopt; + + /// 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 = kCalculateSeq; }; bool fromJSON(const llvm::json::Value &, Response &, llvm::json::Path); llvm::json::Value toJSON(const Response &); From 8917e28dde95f840a7fbdbe10c5a04b35a2ce066 Mon Sep 17 00:00:00 2001 From: John Harrison Date: Tue, 21 Oct 2025 14:41:12 -0700 Subject: [PATCH 3/5] Using `std::visit` to make Send less repeditive and adding some additional checks on `seq=0` from slipping into a message. --- .../test/tools/lldb-dap/dap_server.py | 6 +++ lldb/tools/lldb-dap/DAP.cpp | 41 +++++++++---------- lldb/tools/lldb-dap/JSONUtils.cpp | 5 ++- lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp | 7 ++-- 4 files changed, 33 insertions(+), 26 deletions(-) diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py index a3d924d495fb1..d892c01f0bc71 100644 --- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py +++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py @@ -11,6 +11,7 @@ import signal import sys import threading +import warnings import time from typing import ( Any, @@ -383,6 +384,10 @@ def _process_recv_packets(self) -> None: """Process received packets, updating the session state.""" with self._recv_condition: for packet in self._recv_packets: + if packet and ("seq" not in packet or packet["seq"] == 0): + warnings.warn( + f"received a malformed packet, expected 'seq != 0' for {packet!r}" + ) # Handle events that may modify any stateful properties of # the DAP session. if packet and packet["type"] == "event": @@ -576,6 +581,7 @@ def wait_for_stopped(self) -> Optional[List[Event]]: # If we exited, then we are done if stopped_event["event"] == "exited": break + # Otherwise we stopped and there might be one or more 'stopped' # events for each thread that stopped with a reason, so keep # checking for more 'stopped' events and return all of them diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp index 60d08c85c6033..52c8c6b9092b9 100644 --- a/lldb/tools/lldb-dap/DAP.cpp +++ b/lldb/tools/lldb-dap/DAP.cpp @@ -268,48 +268,47 @@ void DAP::SendJSON(const llvm::json::Value &json) { Id DAP::Send(const Message &message) { std::lock_guard guard(call_mutex); - if (const protocol::Event *e = std::get_if(&message)) { - protocol::Event event = *e; - if (event.seq == kCalculateSeq) - event.seq = seq++; - if (llvm::Error err = transport.Send(event)) + Message msg = std::visit( + [this](auto &&msg) -> Message { + if (msg.seq == kCalculateSeq) + msg.seq = seq++; + return msg; + }, + Message(message)); + + if (const protocol::Event *event = std::get_if(&msg)) { + if (llvm::Error err = transport.Send(*event)) DAP_LOG_ERROR(log, std::move(err), "({0}) sending event failed", m_client_name); - return event.seq; + return event->seq; } - if (const Request *r = std::get_if(&message)) { - Request req = *r; - if (req.seq == kCalculateSeq) - req.seq = seq++; - if (llvm::Error err = transport.Send(req)) + if (const Request *req = std::get_if(&msg)) { + if (llvm::Error err = transport.Send(*req)) DAP_LOG_ERROR(log, std::move(err), "({0}) sending request failed", m_client_name); - return req.seq; + return req->seq; } - if (const Response *r = std::get_if(&message)) { - Response resp = *r; - if (resp.seq == kCalculateSeq) - resp.seq = seq++; + if (const Response *resp = std::get_if(&msg)) { // 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, + /*request_seq=*/resp->request_seq, + /*command=*/resp->command, /*success=*/false, /*message=*/eResponseMessageCancelled, /*body=*/std::nullopt, - /*seq=*/resp.seq, + /*seq=*/resp->seq, }) - : transport.Send(resp); + : transport.Send(*resp); if (err) DAP_LOG_ERROR(log, std::move(err), "({0}) sending response failed", m_client_name); - return resp.seq; + return resp->seq; } llvm_unreachable("Unexpected message type"); diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp index 71e91f8f41a68..2780a5b7748e8 100644 --- a/lldb/tools/lldb-dap/JSONUtils.cpp +++ b/lldb/tools/lldb-dap/JSONUtils.cpp @@ -10,6 +10,7 @@ #include "DAP.h" #include "ExceptionBreakpoint.h" #include "LLDBUtils.h" +#include "Protocol/ProtocolBase.h" #include "ProtocolUtils.h" #include "lldb/API/SBAddress.h" #include "lldb/API/SBCompileUnit.h" @@ -284,7 +285,7 @@ void FillResponse(const llvm::json::Object &request, // Fill in all of the needed response fields to a "request" and set "success" // to true by default. response.try_emplace("type", "response"); - response.try_emplace("seq", (int64_t)0); + response.try_emplace("seq", protocol::kCalculateSeq); EmplaceSafeString(response, "command", GetString(request, "command").value_or("")); const uint64_t seq = GetInteger(request, "seq").value_or(0); @@ -417,7 +418,7 @@ llvm::json::Value CreateScope(const llvm::StringRef name, // } llvm::json::Object CreateEventObject(const llvm::StringRef event_name) { llvm::json::Object event; - event.try_emplace("seq", 0); + event.try_emplace("seq", protocol::kCalculateSeq); event.try_emplace("type", "event"); EmplaceSafeString(event, "event", event_name); return event; diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp index e0c2a15c23549..72359214c8537 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp +++ b/lldb/tools/lldb-dap/Protocol/ProtocolBase.cpp @@ -58,7 +58,7 @@ bool fromJSON(const json::Value &Params, MessageType &M, json::Path P) { } json::Value toJSON(const Request &R) { - assert(R.seq != kCalculateSeq); + assert(R.seq != kCalculateSeq && "invalid seq"); json::Object Result{ {"type", "request"}, @@ -105,7 +105,8 @@ bool operator==(const Request &a, const Request &b) { } json::Value toJSON(const Response &R) { - assert(R.seq != kCalculateSeq); + assert(R.seq != kCalculateSeq && "invalid seq"); + json::Object Result{{"type", "response"}, {"seq", R.seq}, {"command", R.command}, @@ -215,7 +216,7 @@ bool fromJSON(json::Value const &Params, ErrorMessage &EM, json::Path P) { } json::Value toJSON(const Event &E) { - assert(E.seq != kCalculateSeq); + assert(E.seq != kCalculateSeq && "invalid seq"); json::Object Result{ {"type", "event"}, From 0fdf14aee3e97f429eef309fe557801aa4e26a0a Mon Sep 17 00:00:00 2001 From: John Harrison Date: Tue, 21 Oct 2025 14:57:12 -0700 Subject: [PATCH 4/5] Fixing a missing event reference. --- lldb/unittests/DAP/DAPTest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/unittests/DAP/DAPTest.cpp b/lldb/unittests/DAP/DAPTest.cpp index dd610db299134..4fd6cd546e6fa 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{0, /*event=*/"my-event", /*body=*/std::nullopt}); + dap->Send(Event{/*event=*/"my-event", /*body=*/std::nullopt}); EXPECT_CALL(client, Received(IsEvent("my-event"))); Run(); } From 39a2617418cfc9a4505ee49a488d4807ea7f67b5 Mon Sep 17 00:00:00 2001 From: John Harrison Date: Tue, 21 Oct 2025 15:05:20 -0700 Subject: [PATCH 5/5] Fixing the `DAP::seq` type to match the protocol definition. --- lldb/tools/lldb-dap/DAP.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h index 51067d46bc8f4..b4f111e4e720c 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 thread_ids; - uint32_t seq = 0; + protocol::Id seq = 0; std::mutex call_mutex; llvm::SmallDenseMap> inflight_reverse_requests;