[Lldb-commits] [lldb] [lldb-dap] Simplify `readMemory` (PR #109485)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Adrian Vogelsgesang (vogelsgesang)

<details>
<summary>Changes</summary>

The `readMemory` request used the `MemoryRegionInfo` so it could also support short reads. Since #<!-- -->106532, this is no longer necessary, as mentioned by @<!-- -->labath in a comment on #<!-- -->104317.

We no longer set the `unreadableBytes` in the result. But this is optional, anyway, and afaik the VS Code UI would not yet make good use of `unreadableBytes`, anyway.

---
Full diff: https://github.com/llvm/llvm-project/pull/109485.diff


2 Files Affected:

- (modified) lldb/test/API/tools/lldb-dap/memory/TestDAP_memory.py (+2-3) 
- (modified) lldb/tools/lldb-dap/lldb-dap.cpp (+19-50) 


``````````diff
diff --git a/lldb/test/API/tools/lldb-dap/memory/TestDAP_memory.py b/lldb/test/API/tools/lldb-dap/memory/TestDAP_memory.py
index 1082541aebcf7c..9c4bb794dccb61 100644
--- a/lldb/test/API/tools/lldb-dap/memory/TestDAP_memory.py
+++ b/lldb/test/API/tools/lldb-dap/memory/TestDAP_memory.py
@@ -93,7 +93,6 @@ def test_readMemory(self):
 
         # We can read the complete string
         mem = self.dap_server.request_readMemory(memref, 0, 5)["body"]
-        self.assertEqual(mem["unreadableBytes"], 0)
         self.assertEqual(b64decode(mem["data"]), b"dead\0")
 
         # Use an offset
@@ -101,7 +100,7 @@ def test_readMemory(self):
         self.assertEqual(b64decode(mem["data"]), b"ad\0")
 
         # Reads of size 0 are successful
-        # VS-Code sends those in order to check if a `memoryReference` can actually be dereferenced.
+        # VS Code sends those in order to check if a `memoryReference` can actually be dereferenced.
         mem = self.dap_server.request_readMemory(memref, 0, 0)
         self.assertEqual(mem["success"], True)
         self.assertEqual(mem["body"]["data"], "")
@@ -109,4 +108,4 @@ def test_readMemory(self):
         # Reads at offset 0x0 fail
         mem = self.dap_server.request_readMemory("0x0", 0, 6)
         self.assertEqual(mem["success"], False)
-        self.assertEqual(mem["message"], "Memory region is not readable")
+        self.assertEqual(mem["message"], "memory read failed for 0x0")
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index 2fb86f675b4516..1ee04f0089179e 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -4405,14 +4405,6 @@ void request_readMemory(const llvm::json::Object &request) {
   FillResponse(request, response);
   auto *arguments = request.getObject("arguments");
 
-  lldb::SBProcess process = g_dap.target.GetProcess();
-  if (!process.IsValid()) {
-    response["success"] = false;
-    response["message"] = "No process running";
-    g_dap.SendJSON(llvm::json::Value(std::move(response)));
-    return;
-  }
-
   llvm::StringRef memoryReference = GetString(arguments, "memoryReference");
   auto addr_opt = DecodeMemoryReference(memoryReference);
   if (!addr_opt.has_value()) {
@@ -4422,57 +4414,34 @@ void request_readMemory(const llvm::json::Object &request) {
     g_dap.SendJSON(llvm::json::Value(std::move(response)));
     return;
   }
-  lldb::addr_t addr = *addr_opt;
+  lldb::addr_t addr_int = *addr_opt;
+  addr_int += GetSigned(arguments, "offset", 0);
+  const uint64_t count_requested = GetUnsigned(arguments, "count", 0);
 
-  addr += GetSigned(arguments, "offset", 0);
-  const uint64_t requested_count = GetUnsigned(arguments, "count", 0);
-  lldb::SBMemoryRegionInfo region_info;
-  lldb::SBError memreg_error = process.GetMemoryRegionInfo(addr, region_info);
-  if (memreg_error.Fail()) {
-    response["success"] = false;
-    EmplaceSafeString(response, "message",
-                      "Unable to find memory region: " +
-                          std::string(memreg_error.GetCString()));
-    g_dap.SendJSON(llvm::json::Value(std::move(response)));
-    return;
-  }
-  if (!region_info.IsReadable()) {
+  lldb::SBAddress addr =
+      g_dap.target.ResolveLoadAddress(addr_int);
+
+  // We also need support reading 0 bytes
+  // VS Code sends those requests to check if a `memoryReference`
+  // can be dereferenced.
+  const uint64_t count_read = std::max<uint64_t>(count_requested, 1);
+  std::vector<uint8_t> buf;
+  buf.resize(count_read);
+  lldb::SBError error;
+  size_t count_result =
+      g_dap.target.ReadMemory(addr, buf.data(), count_read, error);
+  if (error.Fail()) {
     response["success"] = false;
-    response.try_emplace("message", "Memory region is not readable");
+    EmplaceSafeString(response, "message", error.GetCString());
     g_dap.SendJSON(llvm::json::Value(std::move(response)));
     return;
   }
-  const uint64_t available_count =
-      std::min(requested_count, region_info.GetRegionEnd() - addr);
-  const uint64_t unavailable_count = requested_count - available_count;
-
-  std::vector<uint8_t> buf;
-  buf.resize(available_count);
-  if (available_count > 0) {
-    lldb::SBError memread_error;
-    uint64_t bytes_read =
-        process.ReadMemory(addr, buf.data(), available_count, memread_error);
-    if (memread_error.Fail()) {
-      response["success"] = false;
-      EmplaceSafeString(response, "message",
-                        "Unable to read memory: " +
-                            std::string(memread_error.GetCString()));
-      g_dap.SendJSON(llvm::json::Value(std::move(response)));
-      return;
-    }
-    if (bytes_read != available_count) {
-      response["success"] = false;
-      EmplaceSafeString(response, "message", "Unexpected, short read");
-      g_dap.SendJSON(llvm::json::Value(std::move(response)));
-      return;
-    }
-  }
+  buf.resize(std::min(count_result, count_requested));
 
   llvm::json::Object body;
-  std::string formatted_addr = "0x" + llvm::utohexstr(addr);
+  std::string formatted_addr = "0x" + llvm::utohexstr(addr_int);
   body.try_emplace("address", formatted_addr);
   body.try_emplace("data", llvm::encodeBase64(buf));
-  body.try_emplace("unreadableBytes", unavailable_count);
   response.try_emplace("body", std::move(body));
   g_dap.SendJSON(llvm::json::Value(std::move(response)));
 }

``````````

</details>


https://github.com/llvm/llvm-project/pull/109485