[lldb-dap] Correct lldb-dap seq handling. (llvm#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.

Author: ashgti

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

lldb/tools/lldb-dap/DAP.cpp

@@ -266,40 +266,49 @@ 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)) {
+Id DAP::Send(const Message &message) {
+  std::lock_guard<std::mutex> guard(call_mutex);
+  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<protocol::Event>(&msg)) {
     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 (const Request *req = std::get_if<Request>(&msg)) {
     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 *resp = std::get_if<Response>(&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,
-                              /*success=*/false,
-                              /*message=*/eResponseMessageCancelled,
-                              /*body=*/std::nullopt})
-            : transport.Send(*resp);
-    if (err) {
+    llvm::Error err = (debugger.InterruptRequested())
+                          ? transport.Send({
+                                /*request_seq=*/resp->request_seq,
+                                /*command=*/resp->command,
+                                /*success=*/false,
+                                /*message=*/eResponseMessageCancelled,
+                                /*body=*/std::nullopt,
+                                /*seq=*/resp->seq,
+                            })
+                          : 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");

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;
+  protocol::Id 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,13 @@ 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{
+        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/Handler/RequestHandler.cpp

@@ -157,11 +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{/*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;
   }

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<uint64_t>(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;

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 && "invalid seq");
+
   json::Object Result{
       {"type", "request"},
       {"seq", R.seq},
@@ -103,8 +105,10 @@ bool operator==(const Request &a, const Request &b) {
 }
 
 json::Value toJSON(const Response &R) {
+  assert(R.seq != kCalculateSeq && "invalid seq");
+
   json::Object Result{{"type", "response"},
-                      {"seq", 0},
+                      {"seq", R.seq},
                       {"command", R.command},
                       {"request_seq", R.request_seq},
                       {"success", R.success}};
@@ -132,8 +136,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 +162,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 +172,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;
   }
 
@@ -217,9 +216,11 @@ bool fromJSON(json::Value const &Params, ErrorMessage &EM, json::Path P) {
 }
 
 json::Value toJSON(const Event &E) {
+  assert(E.seq != kCalculateSeq && "invalid seq");
+
   json::Object Result{
       {"type", "event"},
-      {"seq", 0},
+      {"seq", E.seq},
       {"event", E.event},
   };
 
@@ -235,22 +236,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

@@ -30,27 +30,32 @@ 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;
-
   /// The command to execute.
   std::string command;
 
   /// Object containing arguments for the command.
   ///
   /// Request handlers are expected to validate the arguments, which is handled
   /// by `RequestHandler`.
-  std::optional<llvm::json::Value> arguments;
+  std::optional<llvm::json::Value> 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);
@@ -62,7 +67,16 @@ struct Event {
   std::string event;
 
   /// Event-specific information.
-  std::optional<llvm::json::Value> body;
+  std::optional<llvm::json::Value> 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;
 };
 llvm::json::Value toJSON(const Event &);
 bool fromJSON(const llvm::json::Value &, Event &, llvm::json::Path);
@@ -78,7 +92,7 @@ enum ResponseMessage : unsigned {
 /// Response for a request.
 struct Response {
   /// Sequence number of the corresponding request.
-  Id request_seq;
+  Id request_seq = 0;
 
   /// The command requested.
   std::string command;
@@ -87,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<std::variant<ResponseMessage, std::string>> message;
+  std::optional<std::variant<ResponseMessage, std::string>> 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<llvm::json::Value> body;
+  std::optional<llvm::json::Value> 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 &);