[lldb-dap] Use protocol types for modules request and events. (#146966)

Update tests to fix silently failing test and handle when a module is
removed.

Author: da-viper

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

@@ -199,8 +199,8 @@ def _read_packet_thread(self):
         finally:
             dump_dap_log(self.log_file)
 
-    def get_modules(self):
-        module_list = self.request_modules()["body"]["modules"]
+    def get_modules(self, startModule: int = 0, moduleCount: int = 0):
+        module_list = self.request_modules(startModule, moduleCount)["body"]["modules"]
         modules = {}
         for module in module_list:
             modules[module["name"]] = module
@@ -1143,8 +1143,14 @@ def request_completions(self, text, frameId=None):
         }
         return self.send_recv(command_dict)
 
-    def request_modules(self):
-        return self.send_recv({"command": "modules", "type": "request"})
+    def request_modules(self, startModule: int, moduleCount: int):
+        return self.send_recv(
+            {
+                "command": "modules",
+                "type": "request",
+                "arguments": {"startModule": startModule, "moduleCount": moduleCount},
+            }
+        )
 
     def request_stackTrace(
         self, threadId=None, startFrame=None, levels=None, format=None, dump=False

lldb/test/API/tools/lldb-dap/module-event/TestDAP_module_event.py

@@ -32,7 +32,7 @@ def test_module_event(self):
 
         # Make sure we got a module event for libother.
         event = self.dap_server.wait_for_event("module", 5)
-        self.assertTrue(event, "didn't get a module event")
+        self.assertIsNotNone(event, "didn't get a module event")
         module_name = event["body"]["module"]["name"]
         module_id = event["body"]["module"]["id"]
         self.assertEqual(event["body"]["reason"], "new")
@@ -43,13 +43,20 @@ def test_module_event(self):
 
         # Make sure we got a module event for libother.
         event = self.dap_server.wait_for_event("module", 5)
-        self.assertTrue(event, "didn't get a module event")
+        self.assertIsNotNone(event, "didn't get a module event")
         reason = event["body"]["reason"]
-        self.assertEqual(event["body"]["reason"], "removed")
+        self.assertEqual(reason, "removed")
         self.assertEqual(event["body"]["module"]["id"], module_id)
 
-        # The removed module event should omit everything but the module id.
-        # Check that there's no module name in the event.
-        self.assertNotIn("name", event["body"]["module"])
+        # The removed module event should omit everything but the module id and name
+        # as they are required fields.
+        module_data = event["body"]["module"]
+        required_keys = ["id", "name"]
+        self.assertListEqual(list(module_data.keys()), required_keys)
+        self.assertEqual(module_data["name"], "", "expects empty name.")
+
+        # Make sure we do not send another event
+        event = self.dap_server.wait_for_event("module", 3)
+        self.assertIsNone(event, "expects no events.")
 
         self.continue_to_exit()

lldb/test/API/tools/lldb-dap/module/TestDAP_module.py

@@ -45,16 +45,20 @@ def run_test(self, symbol_basename, expect_debug_info_size):
             context="repl",
         )
 
-        def checkSymbolsLoadedWithSize():
+        def check_symbols_loaded_with_size():
             active_modules = self.dap_server.get_modules()
             program_module = active_modules[program_basename]
             self.assertIn("symbolFilePath", program_module)
             self.assertIn(symbols_path, program_module["symbolFilePath"])
-            symbol_regex = re.compile(r"[0-9]+(\.[0-9]*)?[KMG]?B")
-            return symbol_regex.match(program_module["symbolStatus"])
+            size_regex = re.compile(r"[0-9]+(\.[0-9]*)?[KMG]?B")
+            return size_regex.match(program_module["debugInfoSize"])
 
         if expect_debug_info_size:
-            self.waitUntil(checkSymbolsLoadedWithSize)
+            self.assertTrue(
+                self.waitUntil(check_symbols_loaded_with_size),
+                "expect has debug info size",
+            )
+
         active_modules = self.dap_server.get_modules()
         program_module = active_modules[program_basename]
         self.assertEqual(program_basename, program_module["name"])
@@ -83,6 +87,7 @@ def checkSymbolsLoadedWithSize():
         # symbols got added.
         self.assertNotEqual(len(module_changed_names), 0)
         self.assertIn(program_module["name"], module_changed_names)
+        self.continue_to_exit()
 
     @skipIfWindows
     def test_modules(self):
@@ -124,3 +129,5 @@ def test_compile_units(self):
         self.assertTrue(response["body"])
         cu_paths = [cu["compileUnitPath"] for cu in response["body"]["compileUnits"]]
         self.assertIn(main_source_path, cu_paths, "Real path to main.cpp matches")
+
+        self.continue_to_exit()

lldb/tools/lldb-dap/DAP.cpp

@@ -16,6 +16,7 @@
 #include "LLDBUtils.h"
 #include "OutputRedirector.h"
 #include "Protocol/ProtocolBase.h"
+#include "Protocol/ProtocolEvents.h"
 #include "Protocol/ProtocolRequests.h"
 #include "Protocol/ProtocolTypes.h"
 #include "ProtocolUtils.h"
@@ -1353,37 +1354,37 @@ void DAP::EventThread() {
             event_mask & lldb::SBTarget::eBroadcastBitSymbolsChanged) {
           const uint32_t num_modules =
               lldb::SBTarget::GetNumModulesFromEvent(event);
+          const bool remove_module =
+              event_mask & lldb::SBTarget::eBroadcastBitModulesUnloaded;
+
           std::lock_guard<std::mutex> guard(modules_mutex);
           for (uint32_t i = 0; i < num_modules; ++i) {
             lldb::SBModule module =
                 lldb::SBTarget::GetModuleAtIndexFromEvent(i, event);
-            if (!module.IsValid())
-              continue;
-            llvm::StringRef module_id = module.GetUUIDString();
-            if (module_id.empty())
+
+            std::optional<protocol::Module> p_module =
+                CreateModule(target, module, remove_module);
+            if (!p_module)
               continue;
 
-            llvm::StringRef reason;
-            bool id_only = false;
-            if (modules.contains(module_id)) {
-              if (event_mask & lldb::SBTarget::eBroadcastBitModulesUnloaded) {
-                modules.erase(module_id);
-                reason = "removed";
-                id_only = true;
-              } else {
-                reason = "changed";
-              }
-            } else {
+            llvm::StringRef module_id = p_module->id;
+
+            const bool module_exists = modules.contains(module_id);
+            if (remove_module && module_exists) {
+              modules.erase(module_id);
+              Send(protocol::Event{
+                  "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}});
+            } else if (!remove_module) {
               modules.insert(module_id);
-              reason = "new";
+              Send(protocol::Event{
+                  "module", ModuleEventBody{std::move(p_module).value(),
+                                            ModuleEventBody::eReasonNew}});
             }
-
-            llvm::json::Object body;
-            body.try_emplace("reason", reason);
-            body.try_emplace("module", CreateModule(target, module, id_only));
-            llvm::json::Object module_event = CreateEventObject("module");
-            module_event.try_emplace("body", std::move(body));
-            SendJSON(llvm::json::Value(std::move(module_event)));
           }
         }
       } else if (lldb::SBBreakpoint::EventIsBreakpointEvent(event)) {

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

@@ -7,64 +7,38 @@
 //===----------------------------------------------------------------------===//
 
 #include "DAP.h"
-#include "EventHelper.h"
-#include "JSONUtils.h"
+#include "ProtocolUtils.h"
 #include "RequestHandler.h"
 
+using namespace lldb_dap::protocol;
 namespace lldb_dap {
 
-// "modulesRequest": {
-//   "allOf": [ { "$ref": "#/definitions/Request" }, {
-//     "type": "object",
-//     "description": "Modules request; value of command field is
-//                     'modules'.",
-//     "properties": {
-//       "command": {
-//         "type": "string",
-//         "enum": [ "modules" ]
-//       },
-//     },
-//     "required": [ "command" ]
-//   }]
-// },
-// "modulesResponse": {
-//   "allOf": [ { "$ref": "#/definitions/Response" }, {
-//     "type": "object",
-//     "description": "Response to 'modules' request.",
-//     "properties": {
-//       "body": {
-//         "description": "Response to 'modules' request. Array of
-//                         module objects."
-//       }
-//     }
-//   }]
-// }
-void ModulesRequestHandler::operator()(
-    const llvm::json::Object &request) const {
-  llvm::json::Object response;
-  FillResponse(request, response);
-
-  llvm::json::Array modules;
-
-  {
-    std::lock_guard<std::mutex> guard(dap.modules_mutex);
-    for (size_t i = 0; i < dap.target.GetNumModules(); i++) {
-      lldb::SBModule module = dap.target.GetModuleAtIndex(i);
-      if (!module.IsValid())
-        continue;
-
-      llvm::StringRef module_id = module.GetUUIDString();
-      if (!module_id.empty())
-        dap.modules.insert(module_id);
-
-      modules.emplace_back(CreateModule(dap.target, module));
+/// Modules can be retrieved from the debug adapter with this request which can
+/// either return all modules or a range of modules to support paging.
+///
+/// Clients should only call this request if the corresponding capability
+/// `supportsModulesRequest` is true.
+llvm::Expected<ModulesResponseBody>
+ModulesRequestHandler::Run(const std::optional<ModulesArguments> &args) const {
+  ModulesResponseBody response;
+
+  std::vector<Module> &modules = response.modules;
+  std::lock_guard<std::mutex> guard(dap.modules_mutex);
+  const uint32_t total_modules = dap.target.GetNumModules();
+  response.totalModules = total_modules;
+
+  modules.reserve(total_modules);
+  for (uint32_t i = 0; i < total_modules; i++) {
+    lldb::SBModule module = dap.target.GetModuleAtIndex(i);
+
+    std::optional<Module> result = CreateModule(dap.target, module);
+    if (result && !result->id.empty()) {
+      dap.modules.insert(result->id);
+      modules.emplace_back(std::move(result).value());
     }
   }
 
-  llvm::json::Object body;
-  body.try_emplace("modules", std::move(modules));
-  response.try_emplace("body", std::move(body));
-  dap.SendJSON(llvm::json::Value(std::move(response)));
+  return response;
 }
 
 } // namespace lldb_dap

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

@@ -466,14 +466,17 @@ class CompileUnitsRequestHandler : public LegacyRequestHandler {
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class ModulesRequestHandler : public LegacyRequestHandler {
+class ModulesRequestHandler final
+    : public RequestHandler<std::optional<protocol::ModulesArguments>,
+                            llvm::Expected<protocol::ModulesResponseBody>> {
 public:
-  using LegacyRequestHandler::LegacyRequestHandler;
+  using RequestHandler::RequestHandler;
   static llvm::StringLiteral GetCommand() { return "modules"; }
   FeatureSet GetSupportedFeatures() const override {
     return {protocol::eAdapterFeatureModulesRequest};
   }
-  void operator()(const llvm::json::Object &request) const override;
+  llvm::Expected<protocol::ModulesResponseBody>
+  Run(const std::optional<protocol::ModulesArguments> &args) const override;
 };
 
 class PauseRequestHandler : public LegacyRequestHandler {

lldb/tools/lldb-dap/JSONUtils.cpp

@@ -341,101 +341,6 @@ llvm::json::Value CreateScope(const llvm::StringRef name,
   return llvm::json::Value(std::move(object));
 }
 
-static uint64_t GetDebugInfoSizeInSection(lldb::SBSection section) {
-  uint64_t debug_info_size = 0;
-  llvm::StringRef section_name(section.GetName());
-  if (section_name.starts_with(".debug") ||
-      section_name.starts_with("__debug") ||
-      section_name.starts_with(".apple") || section_name.starts_with("__apple"))
-    debug_info_size += section.GetFileByteSize();
-  size_t num_sub_sections = section.GetNumSubSections();
-  for (size_t i = 0; i < num_sub_sections; i++) {
-    debug_info_size +=
-        GetDebugInfoSizeInSection(section.GetSubSectionAtIndex(i));
-  }
-  return debug_info_size;
-}
-
-static uint64_t GetDebugInfoSize(lldb::SBModule module) {
-  uint64_t debug_info_size = 0;
-  size_t num_sections = module.GetNumSections();
-  for (size_t i = 0; i < num_sections; i++) {
-    debug_info_size += GetDebugInfoSizeInSection(module.GetSectionAtIndex(i));
-  }
-  return debug_info_size;
-}
-
-static std::string ConvertDebugInfoSizeToString(uint64_t debug_info) {
-  std::ostringstream oss;
-  oss << std::fixed << std::setprecision(1);
-  if (debug_info < 1024) {
-    oss << debug_info << "B";
-  } else if (debug_info < 1024 * 1024) {
-    double kb = double(debug_info) / 1024.0;
-    oss << kb << "KB";
-  } else if (debug_info < 1024 * 1024 * 1024) {
-    double mb = double(debug_info) / (1024.0 * 1024.0);
-    oss << mb << "MB";
-  } else {
-    double gb = double(debug_info) / (1024.0 * 1024.0 * 1024.0);
-    oss << gb << "GB";
-  }
-  return oss.str();
-}
-
-llvm::json::Value CreateModule(lldb::SBTarget &target, lldb::SBModule &module,
-                               bool id_only) {
-  llvm::json::Object object;
-  if (!target.IsValid() || !module.IsValid())
-    return llvm::json::Value(std::move(object));
-
-  const char *uuid = module.GetUUIDString();
-  object.try_emplace("id", uuid ? std::string(uuid) : std::string(""));
-
-  if (id_only)
-    return llvm::json::Value(std::move(object));
-
-  object.try_emplace("name", std::string(module.GetFileSpec().GetFilename()));
-  char module_path_arr[PATH_MAX];
-  module.GetFileSpec().GetPath(module_path_arr, sizeof(module_path_arr));
-  std::string module_path(module_path_arr);
-  object.try_emplace("path", module_path);
-  if (module.GetNumCompileUnits() > 0) {
-    std::string symbol_str = "Symbols loaded.";
-    std::string debug_info_size;
-    uint64_t debug_info = GetDebugInfoSize(module);
-    if (debug_info > 0) {
-      debug_info_size = ConvertDebugInfoSizeToString(debug_info);
-    }
-    object.try_emplace("symbolStatus", symbol_str);
-    object.try_emplace("debugInfoSize", debug_info_size);
-    char symbol_path_arr[PATH_MAX];
-    module.GetSymbolFileSpec().GetPath(symbol_path_arr,
-                                       sizeof(symbol_path_arr));
-    std::string symbol_path(symbol_path_arr);
-    object.try_emplace("symbolFilePath", symbol_path);
-  } else {
-    object.try_emplace("symbolStatus", "Symbols not found.");
-  }
-  std::string load_address =
-      llvm::formatv("{0:x}",
-                    module.GetObjectFileHeaderAddress().GetLoadAddress(target))
-          .str();
-  object.try_emplace("addressRange", load_address);
-  std::string version_str;
-  uint32_t version_nums[3];
-  uint32_t num_versions =
-      module.GetVersion(version_nums, sizeof(version_nums) / sizeof(uint32_t));
-  for (uint32_t i = 0; i < num_versions; ++i) {
-    if (!version_str.empty())
-      version_str += ".";
-    version_str += std::to_string(version_nums[i]);
-  }
-  if (!version_str.empty())
-    object.try_emplace("version", version_str);
-  return llvm::json::Value(std::move(object));
-}
-
 // "Event": {
 //   "allOf": [ { "$ref": "#/definitions/ProtocolMessage" }, {
 //     "type": "object",