[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.

Author: ashgti

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}});
             }
           }
         }

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.

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

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,

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,22 +230,16 @@ 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) {
     P.field("type").report("expected to be 'event'");
     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;
   }
 

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;
 

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();
 }