[lldb-dap] Migrating DAP 'initialize' to new typed RequestHandler. (#133007)

This adds new types and helpers to support the 'initialize' request with
the new typed RequestHandler. While working on this I found there were a
few cases where we incorrectly treated initialize arguments as
capabilities. The new `lldb_dap::protocol::InitializeRequestArguments`
and `lldb_dap::protocol::Capabilities` uncovered the inconsistencies.

---------

Co-authored-by: Jonas Devlieghere <[email protected]>

Author: ashgti

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py

@@ -776,7 +776,8 @@ def request_initialize(self, sourceInitFile):
                 "supportsVariablePaging": True,
                 "supportsVariableType": True,
                 "supportsStartDebuggingRequest": True,
-                "sourceInitFile": sourceInitFile,
+                "supportsProgressReporting": True,
+                "$__lldb_sourceInitFile": sourceInitFile,
             },
         }
         response = self.send_recv(command_dict)
@@ -1261,7 +1262,7 @@ def launch(cls, /, executable, env=None, log_file=None, connection=None):
         expected_prefix = "Listening for: "
         out = process.stdout.readline().decode()
         if not out.startswith(expected_prefix):
-            self.process.kill()
+            process.kill()
             raise ValueError(
                 "lldb-dap failed to print listening address, expected '{}', got '{}'".format(
                     expected_prefix, out

lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py

@@ -524,8 +524,7 @@ def test_version(self):
 
         # The first line is the prompt line like "(lldb) version", so we skip it.
         version_eval_output_without_prompt_line = version_eval_output.splitlines()[1:]
-        lldb_json = self.dap_server.get_initialize_value("__lldb")
-        version_string = lldb_json["version"]
+        version_string = self.dap_server.get_initialize_value("$__lldb_version")
         self.assertEqual(
             version_eval_output_without_prompt_line,
             version_string.splitlines(),

lldb/tools/lldb-dap/DAP.cpp

@@ -14,6 +14,7 @@
 #include "LLDBUtils.h"
 #include "OutputRedirector.h"
 #include "Protocol/ProtocolBase.h"
+#include "Protocol/ProtocolTypes.h"
 #include "Transport.h"
 #include "lldb/API/SBBreakpoint.h"
 #include "lldb/API/SBCommandInterpreter.h"
@@ -1144,32 +1145,41 @@ lldb::SBValue Variables::FindVariable(uint64_t variablesReference,
   return variable;
 }
 
-llvm::StringMap<bool> DAP::GetCapabilities() {
-  llvm::StringMap<bool> capabilities;
-
-  // Supported capabilities.
-  capabilities["supportTerminateDebuggee"] = true;
-  capabilities["supportsDataBreakpoints"] = true;
-  capabilities["supportsDelayedStackTraceLoading"] = true;
-  capabilities["supportsEvaluateForHovers"] = true;
-  capabilities["supportsExceptionOptions"] = true;
-  capabilities["supportsLogPoints"] = true;
-  capabilities["supportsProgressReporting"] = true;
-  capabilities["supportsSteppingGranularity"] = true;
-  capabilities["supportsValueFormattingOptions"] = true;
-
-  // Unsupported capabilities.
-  capabilities["supportsGotoTargetsRequest"] = false;
-  capabilities["supportsLoadedSourcesRequest"] = false;
-  capabilities["supportsRestartFrame"] = false;
-  capabilities["supportsStepBack"] = false;
+protocol::Capabilities DAP::GetCapabilities() {
+  protocol::Capabilities capabilities;
+
+  // Supported capabilities that are not specific to a single request.
+  capabilities.supportedFeatures = {
+      protocol::eAdapterFeatureLogPoints,
+      protocol::eAdapterFeatureSteppingGranularity,
+      protocol::eAdapterFeatureValueFormattingOptions,
+  };
 
   // Capabilities associated with specific requests.
   for (auto &kv : request_handlers) {
-    for (auto &request_kv : kv.second->GetCapabilities())
-      capabilities[request_kv.getKey()] = request_kv.getValue();
+    llvm::SmallDenseSet<AdapterFeature, 1> features =
+        kv.second->GetSupportedFeatures();
+    capabilities.supportedFeatures.insert(features.begin(), features.end());
   }
 
+  // Available filters or options for the setExceptionBreakpoints request.
+  std::vector<protocol::ExceptionBreakpointsFilter> filters;
+  for (const auto &exc_bp : *exception_breakpoints)
+    filters.emplace_back(CreateExceptionBreakpointFilter(exc_bp));
+  capabilities.exceptionBreakpointFilters = std::move(filters);
+
+  // FIXME: This should be registered based on the supported languages?
+  std::vector<std::string> completion_characters;
+  completion_characters.emplace_back(".");
+  // FIXME: I wonder if we should remove this key... its very aggressive
+  // triggering and accepting completions.
+  completion_characters.emplace_back(" ");
+  completion_characters.emplace_back("\t");
+  capabilities.completionTriggerCharacters = std::move(completion_characters);
+
+  // Put in non-DAP specification lldb specific information.
+  capabilities.lldbExtVersion = debugger.GetVersionString();
+
   return capabilities;
 }
 

lldb/tools/lldb-dap/DAP.h

@@ -16,6 +16,8 @@
 #include "OutputRedirector.h"
 #include "ProgressEvent.h"
 #include "Protocol/ProtocolBase.h"
+#include "Protocol/ProtocolRequests.h"
+#include "Protocol/ProtocolTypes.h"
 #include "SourceBreakpoint.h"
 #include "Transport.h"
 #include "lldb/API/SBBroadcaster.h"
@@ -58,6 +60,9 @@ typedef llvm::StringMap<FunctionBreakpoint> FunctionBreakpointMap;
 typedef llvm::DenseMap<lldb::addr_t, InstructionBreakpoint>
     InstructionBreakpointMap;
 
+using AdapterFeature = protocol::AdapterFeature;
+using ClientFeature = protocol::ClientFeature;
+
 enum class OutputType { Console, Stdout, Stderr, Telemetry };
 
 /// Buffer size for handling output events.
@@ -205,6 +210,8 @@ struct DAP {
   // empty; if the previous expression was a variable expression, this string
   // will contain that expression.
   std::string last_nonempty_var_expression;
+  /// The set of features supported by the connected client.
+  llvm::DenseSet<ClientFeature> clientFeatures;
 
   /// Creates a new DAP sessions.
   ///
@@ -363,8 +370,8 @@ struct DAP {
     request_handlers[Handler::GetCommand()] = std::make_unique<Handler>(*this);
   }
 
-  /// Return a key-value list of capabilities.
-  llvm::StringMap<bool> GetCapabilities();
+  /// The set of capablities supported by this adapter.
+  protocol::Capabilities GetCapabilities();
 
   /// Debuggee will continue from stopped state.
   void WillContinue() { variables.Clear(); }

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

@@ -9,12 +9,14 @@
 #include "DAP.h"
 #include "EventHelper.h"
 #include "JSONUtils.h"
+#include "Protocol/ProtocolRequests.h"
 #include "RequestHandler.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBListener.h"
 #include "lldb/API/SBStream.h"
 
 using namespace lldb;
+using namespace lldb_dap::protocol;
 
 namespace lldb_dap {
 
@@ -229,136 +231,46 @@ static void EventThreadFunction(DAP &dap) {
   }
 }
 
-// "InitializeRequest": {
-//   "allOf": [ { "$ref": "#/definitions/Request" }, {
-//     "type": "object",
-//     "description": "Initialize request; value of command field is
-//                     'initialize'.",
-//     "properties": {
-//       "command": {
-//         "type": "string",
-//         "enum": [ "initialize" ]
-//       },
-//       "arguments": {
-//         "$ref": "#/definitions/InitializeRequestArguments"
-//       }
-//     },
-//     "required": [ "command", "arguments" ]
-//   }]
-// },
-// "InitializeRequestArguments": {
-//   "type": "object",
-//   "description": "Arguments for 'initialize' request.",
-//   "properties": {
-//     "clientID": {
-//       "type": "string",
-//       "description": "The ID of the (frontend) client using this adapter."
-//     },
-//     "adapterID": {
-//       "type": "string",
-//       "description": "The ID of the debug adapter."
-//     },
-//     "locale": {
-//       "type": "string",
-//       "description": "The ISO-639 locale of the (frontend) client using
-//                       this adapter, e.g. en-US or de-CH."
-//     },
-//     "linesStartAt1": {
-//       "type": "boolean",
-//       "description": "If true all line numbers are 1-based (default)."
-//     },
-//     "columnsStartAt1": {
-//       "type": "boolean",
-//       "description": "If true all column numbers are 1-based (default)."
-//     },
-//     "pathFormat": {
-//       "type": "string",
-//       "_enum": [ "path", "uri" ],
-//       "description": "Determines in what format paths are specified. The
-//                       default is 'path', which is the native format."
-//     },
-//     "supportsVariableType": {
-//       "type": "boolean",
-//       "description": "Client supports the optional type attribute for
-//                       variables."
-//     },
-//     "supportsVariablePaging": {
-//       "type": "boolean",
-//       "description": "Client supports the paging of variables."
-//     },
-//     "supportsRunInTerminalRequest": {
-//       "type": "boolean",
-//       "description": "Client supports the runInTerminal request."
-//     }
-//   },
-//   "required": [ "adapterID" ]
-// },
-// "InitializeResponse": {
-//   "allOf": [ { "$ref": "#/definitions/Response" }, {
-//     "type": "object",
-//     "description": "Response to 'initialize' request.",
-//     "properties": {
-//       "body": {
-//         "$ref": "#/definitions/Capabilities",
-//         "description": "The capabilities of this debug adapter."
-//       }
-//     }
-//   }]
-// }
-void InitializeRequestHandler::operator()(
-    const llvm::json::Object &request) const {
-  llvm::json::Object response;
-  FillResponse(request, response);
-  llvm::json::Object body;
-
-  const auto *arguments = request.getObject("arguments");
-  // sourceInitFile option is not from formal DAP specification. It is only
-  // used by unit tests to prevent sourcing .lldbinit files from environment
-  // which may affect the outcome of tests.
-  bool source_init_file =
-      GetBoolean(arguments, "sourceInitFile").value_or(true);
+/// Initialize request; value of command field is 'initialize'.
+llvm::Expected<InitializeResponseBody> InitializeRequestHandler::Run(
+    const InitializeRequestArguments &arguments) const {
+  dap.clientFeatures = arguments.supportedFeatures;
 
   // Do not source init files until in/out/err are configured.
   dap.debugger = lldb::SBDebugger::Create(false);
   dap.debugger.SetInputFile(dap.in);
-  auto out_fd = dap.out.GetWriteFileDescriptor();
-  if (llvm::Error err = out_fd.takeError()) {
-    response["success"] = false;
-    EmplaceSafeString(response, "message", llvm::toString(std::move(err)));
-    dap.SendJSON(llvm::json::Value(std::move(response)));
-    return;
-  }
+
+  llvm::Expected<int> out_fd = dap.out.GetWriteFileDescriptor();
+  if (!out_fd)
+    return out_fd.takeError();
   dap.debugger.SetOutputFile(lldb::SBFile(*out_fd, "w", false));
-  auto err_fd = dap.err.GetWriteFileDescriptor();
-  if (llvm::Error err = err_fd.takeError()) {
-    response["success"] = false;
-    EmplaceSafeString(response, "message", llvm::toString(std::move(err)));
-    dap.SendJSON(llvm::json::Value(std::move(response)));
-    return;
-  }
+
+  llvm::Expected<int> err_fd = dap.err.GetWriteFileDescriptor();
+  if (!err_fd)
+    return err_fd.takeError();
   dap.debugger.SetErrorFile(lldb::SBFile(*err_fd, "w", false));
 
   auto interp = dap.debugger.GetCommandInterpreter();
 
-  if (source_init_file) {
+  // The sourceInitFile option is not part of the DAP specification. It is an
+  // extension used by the test suite to prevent sourcing `.lldbinit` and
+  // changing its behavior.
+  if (arguments.lldbExtSourceInitFile.value_or(true)) {
     dap.debugger.SkipLLDBInitFiles(false);
     dap.debugger.SkipAppInitFiles(false);
     lldb::SBCommandReturnObject init;
     interp.SourceInitFileInGlobalDirectory(init);
     interp.SourceInitFileInHomeDirectory(init);
   }
 
-  if (llvm::Error err = dap.RunPreInitCommands()) {
-    response["success"] = false;
-    EmplaceSafeString(response, "message", llvm::toString(std::move(err)));
-    dap.SendJSON(llvm::json::Value(std::move(response)));
-    return;
-  }
+  if (llvm::Error err = dap.RunPreInitCommands())
+    return err;
 
   dap.PopulateExceptionBreakpoints();
   auto cmd = dap.debugger.GetCommandInterpreter().AddMultiwordCommand(
       "lldb-dap", "Commands for managing lldb-dap.");
-  if (GetBoolean(arguments, "supportsStartDebuggingRequest").value_or(false)) {
+  if (arguments.supportedFeatures.contains(
+          eClientFeatureStartDebuggingRequest)) {
     cmd.AddCommand(
         "start-debugging", new StartDebuggingRequestHandler(dap),
         "Sends a startDebugging request from the debug adapter to the client "
@@ -370,37 +282,15 @@ void InitializeRequestHandler::operator()(
   cmd.AddCommand("send-event", new SendEventRequestHandler(dap),
                  "Sends an DAP event to the client.");
 
-  dap.progress_event_thread =
-      std::thread(ProgressEventThreadFunction, std::ref(dap));
+  if (arguments.supportedFeatures.contains(eClientFeatureProgressReporting))
+    dap.progress_event_thread =
+        std::thread(ProgressEventThreadFunction, std::ref(dap));
 
   // Start our event thread so we can receive events from the debugger, target,
   // process and more.
   dap.event_thread = std::thread(EventThreadFunction, std::ref(dap));
 
-  llvm::StringMap<bool> capabilities = dap.GetCapabilities();
-  for (auto &kv : capabilities)
-    body.try_emplace(kv.getKey(), kv.getValue());
-
-  // Available filters or options for the setExceptionBreakpoints request.
-  llvm::json::Array filters;
-  for (const auto &exc_bp : *dap.exception_breakpoints)
-    filters.emplace_back(CreateExceptionBreakpointFilter(exc_bp));
-  body.try_emplace("exceptionBreakpointFilters", std::move(filters));
-
-  llvm::json::Array completion_characters;
-  completion_characters.emplace_back(".");
-  completion_characters.emplace_back(" ");
-  completion_characters.emplace_back("\t");
-  body.try_emplace("completionTriggerCharacters",
-                   std::move(completion_characters));
-
-  // Put in non-DAP specification lldb specific information.
-  llvm::json::Object lldb_json;
-  lldb_json.try_emplace("version", dap.debugger.GetVersionString());
-  body.try_emplace("__lldb", std::move(lldb_json));
-
-  response.try_emplace("body", std::move(body));
-  dap.SendJSON(llvm::json::Value(std::move(response)));
+  return dap.GetCapabilities();
 }
 
 } // namespace lldb_dap

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

@@ -12,6 +12,7 @@
 #include "JSONUtils.h"
 #include "LLDBUtils.h"
 #include "RunInTerminal.h"
+#include "llvm/Support/Error.h"
 
 #if !defined(_WIN32)
 #include <unistd.h>
@@ -97,6 +98,11 @@ void BaseRequestHandler::SetSourceMapFromArguments(
 static llvm::Error RunInTerminal(DAP &dap,
                                  const llvm::json::Object &launch_request,
                                  const uint64_t timeout_seconds) {
+  if (!dap.clientFeatures.contains(
+          protocol::eClientFeatureRunInTerminalRequest))
+    return llvm::make_error<DAPError>("Cannot use runInTerminal, feature is "
+                                      "not supported by the connected client");
+
   dap.is_attach = true;
   lldb::SBAttachInfo attach_info;