[lldb-dap] Refactor reverse request response handlers (NFC) (#128594)

This refactors the response handlers for reverse request to follow the
same architecture as the request handlers. With only two implementation
that might be overkill, but it reduces code duplication and improves
error reporting by storing the sequence ID. This PR also fixes an
unchecked Expected in the old callback for unknown sequence IDs.

Author: JDevlieghere

lldb/tools/lldb-dap/CMakeLists.txt

@@ -37,6 +37,7 @@ add_lldb_tool(lldb-dap
   SourceBreakpoint.cpp
   Watchpoint.cpp
 
+  Handler/ResponseHandler.cpp
   Handler/AttachRequestHandler.cpp
   Handler/BreakpointLocationsHandler.cpp
   Handler/CompileUnitsRequestHandler.cpp

lldb/tools/lldb-dap/DAP.cpp

@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "DAP.h"
+#include "Handler/ResponseHandler.h"
 #include "JSONUtils.h"
 #include "LLDBUtils.h"
 #include "OutputRedirector.h"
@@ -34,6 +35,7 @@
 #include <cstdarg>
 #include <cstdio>
 #include <fstream>
+#include <memory>
 #include <mutex>
 #include <utility>
 
@@ -769,10 +771,8 @@ bool DAP::HandleObject(const llvm::json::Object &object) {
 
   if (packet_type == "response") {
     auto id = GetSigned(object, "request_seq", 0);
-    ResponseCallback response_handler = [](llvm::Expected<llvm::json::Value>) {
-      llvm::errs() << "Unhandled response\n";
-    };
 
+    std::unique_ptr<ResponseHandler> response_handler;
     {
       std::lock_guard<std::mutex> locker(call_mutex);
       auto inflight = inflight_reverse_requests.find(id);
@@ -782,19 +782,22 @@ bool DAP::HandleObject(const llvm::json::Object &object) {
       }
     }
 
+    if (!response_handler)
+      response_handler = std::make_unique<UnknownResponseHandler>("", id);
+
     // Result should be given, use null if not.
     if (GetBoolean(object, "success", false)) {
       llvm::json::Value Result = nullptr;
       if (auto *B = object.get("body")) {
         Result = std::move(*B);
       }
-      response_handler(Result);
+      (*response_handler)(Result);
     } else {
       llvm::StringRef message = GetString(object, "message");
       if (message.empty()) {
         message = "Unknown error, response failed";
       }
-      response_handler(llvm::createStringError(
+      (*response_handler)(llvm::createStringError(
           std::error_code(-1, std::generic_category()), message));
     }
 
@@ -875,24 +878,6 @@ llvm::Error DAP::Loop() {
   return llvm::Error::success();
 }
 
-void DAP::SendReverseRequest(llvm::StringRef command,
-                             llvm::json::Value arguments,
-                             ResponseCallback callback) {
-  int64_t id;
-  {
-    std::lock_guard<std::mutex> locker(call_mutex);
-    id = ++reverse_request_seq;
-    inflight_reverse_requests.emplace(id, std::move(callback));
-  }
-
-  SendJSON(llvm::json::Object{
-      {"type", "request"},
-      {"seq", id},
-      {"command", command},
-      {"arguments", std::move(arguments)},
-  });
-}
-
 lldb::SBError DAP::WaitForProcessToStop(uint32_t seconds) {
   lldb::SBError error;
   lldb::SBProcess process = target.GetProcess();
@@ -1007,17 +992,10 @@ bool StartDebuggingRequestHandler::DoExecute(
     return false;
   }
 
-  dap.SendReverseRequest(
+  dap.SendReverseRequest<LogFailureResponseHandler>(
       "startDebugging",
       llvm::json::Object{{"request", request},
-                         {"configuration", std::move(*configuration)}},
-      [](llvm::Expected<llvm::json::Value> value) {
-        if (!value) {
-          llvm::Error err = value.takeError();
-          llvm::errs() << "reverse start debugging request failed: "
-                       << llvm::toString(std::move(err)) << "\n";
-        }
-      });
+                         {"configuration", std::move(*configuration)}});
 
   result.SetStatus(lldb::eReturnStatusSuccessFinishNoResult);
 

lldb/tools/lldb-dap/DAP.h

@@ -13,6 +13,7 @@
 #include "ExceptionBreakpoint.h"
 #include "FunctionBreakpoint.h"
 #include "Handler/RequestHandler.h"
+#include "Handler/ResponseHandler.h"
 #include "IOStream.h"
 #include "InstructionBreakpoint.h"
 #include "OutputRedirector.h"
@@ -68,8 +69,6 @@ enum DAPBroadcasterBits {
   eBroadcastBitStopProgressThread = 1u << 1
 };
 
-typedef void (*ResponseCallback)(llvm::Expected<llvm::json::Value> value);
-
 enum class PacketStatus {
   Success = 0,
   EndOfFile,
@@ -197,7 +196,7 @@ struct DAP {
   llvm::DenseSet<lldb::tid_t> thread_ids;
   uint32_t reverse_request_seq;
   std::mutex call_mutex;
-  std::map<int /* request_seq */, ResponseCallback /* reply handler */>
+  llvm::SmallDenseMap<int64_t, std::unique_ptr<ResponseHandler>>
       inflight_reverse_requests;
   ReplMode repl_mode;
   std::string command_escape_prefix = "`";
@@ -327,12 +326,24 @@ struct DAP {
   ///   The reverse request command.
   ///
   /// \param[in] arguments
-  ///   The reverse request arguements.
-  ///
-  /// \param[in] callback
-  ///   A callback to execute when the response arrives.
-  void SendReverseRequest(llvm::StringRef command, llvm::json::Value arguments,
-                          ResponseCallback callback);
+  ///   The reverse request arguments.
+  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)},
+    });
+  }
 
   /// Registers a request handler.
   template <typename Handler> void RegisterRequest() {

lldb/tools/lldb-dap/Handler/RequestHandler.cpp

@@ -101,15 +101,8 @@ static llvm::Error RunInTerminal(DAP &dap,
 #endif
   llvm::json::Object reverse_request = CreateRunInTerminalReverseRequest(
       launch_request, dap.debug_adaptor_path, comm_file.m_path, debugger_pid);
-  dap.SendReverseRequest("runInTerminal", std::move(reverse_request),
-                         [](llvm::Expected<llvm::json::Value> value) {
-                           if (!value) {
-                             llvm::Error err = value.takeError();
-                             llvm::errs()
-                                 << "runInTerminal request failed: "
-                                 << llvm::toString(std::move(err)) << "\n";
-                           }
-                         });
+  dap.SendReverseRequest<LogFailureResponseHandler>("runInTerminal",
+                                                    std::move(reverse_request));
 
   if (llvm::Expected<lldb::pid_t> pid = comm_channel.GetLauncherPid())
     attach_info.SetProcessID(*pid);

lldb/tools/lldb-dap/Handler/ResponseHandler.cpp

@@ -0,0 +1,35 @@
+//===-- ResponseHandler.cpp -----------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ResponseHandler.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/raw_ostream.h"
+
+namespace lldb_dap {
+
+void UnknownResponseHandler::operator()(
+    llvm::Expected<llvm::json::Value> value) const {
+  llvm::errs() << "unexpected response: ";
+  if (value) {
+    if (std::optional<llvm::StringRef> str = value->getAsString())
+      llvm::errs() << *str;
+  } else {
+    llvm::errs() << "error: " << llvm::toString(value.takeError());
+  }
+  llvm::errs() << '\n';
+}
+
+void LogFailureResponseHandler::operator()(
+    llvm::Expected<llvm::json::Value> value) const {
+  if (!value)
+    llvm::errs() << "reverse request \"" << m_command << "\" (" << m_id
+                 << ") failed: " << llvm::toString(value.takeError()) << '\n';
+}
+
+} // namespace lldb_dap

lldb/tools/lldb-dap/Handler/ResponseHandler.h

@@ -0,0 +1,56 @@
+//===-- ResponseHandler.h -------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_TOOLS_LLDB_DAP_HANDLER_RESPONSEHANDLER_H
+#define LLDB_TOOLS_LLDB_DAP_HANDLER_RESPONSEHANDLER_H
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include <cstdint>
+
+namespace lldb_dap {
+struct DAP;
+
+/// Handler for responses to reverse requests.
+class ResponseHandler {
+public:
+  ResponseHandler(llvm::StringRef command, int64_t id)
+      : m_command(command), m_id(id) {}
+
+  /// ResponseHandlers are not copyable.
+  /// @{
+  ResponseHandler(const ResponseHandler &) = delete;
+  ResponseHandler &operator=(const ResponseHandler &) = delete;
+  /// @}
+
+  virtual ~ResponseHandler() = default;
+
+  virtual void operator()(llvm::Expected<llvm::json::Value> value) const = 0;
+
+protected:
+  llvm::StringRef m_command;
+  int64_t m_id;
+};
+
+/// Response handler used for unknown responses.
+class UnknownResponseHandler : public ResponseHandler {
+public:
+  using ResponseHandler::ResponseHandler;
+  void operator()(llvm::Expected<llvm::json::Value> value) const override;
+};
+
+/// Response handler which logs to stderr in case of a failure.
+class LogFailureResponseHandler : public ResponseHandler {
+public:
+  using ResponseHandler::ResponseHandler;
+  void operator()(llvm::Expected<llvm::json::Value> value) const override;
+};
+
+} // namespace lldb_dap
+
+#endif