[lldb-dap] Use protocol types for ReadMemory request

lldb/test/API/tools/lldb-dap/memory/TestDAP_memory.py

@@ -111,8 +111,17 @@ def test_readMemory(self):
         # 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"], "")
+        self.assertNotIn(
+            "data", mem["body"], f"expects no data key in response: {mem!r}"
+        )
+
+        # Reads at offset 0x0 return unreadable bytes
+        bytes_to_read = 6
+        mem = self.dap_server.request_readMemory("0x0", 0, bytes_to_read)
+        self.assertEqual(mem["body"]["unreadableBytes"], bytes_to_read)
+
+        # Reads with invalid address fails.
+        mem = self.dap_server.request_readMemory("-3204", 0, 10)
+        self.assertFalse(mem["success"], "expect fail on reading memory.")
 
-        # Reads at offset 0x0 fail
-        mem = self.dap_server.request_readMemory("0x0", 0, 6)
-        self.assertEqual(mem["success"], False)
+        self.continue_to_exit()

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

@@ -7,136 +7,57 @@
 //===----------------------------------------------------------------------===//
 
 #include "DAP.h"
-#include "EventHelper.h"
 #include "JSONUtils.h"
 #include "RequestHandler.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Base64.h"
 
 namespace lldb_dap {
 
-// "ReadMemoryRequest": {
-//   "allOf": [ { "$ref": "#/definitions/Request" }, {
-//     "type": "object",
-//     "description": "Reads bytes from memory at the provided location. Clients
-//                     should only call this request if the corresponding
-//                     capability `supportsReadMemoryRequest` is true.",
-//     "properties": {
-//       "command": {
-//         "type": "string",
-//         "enum": [ "readMemory" ]
-//       },
-//       "arguments": {
-//         "$ref": "#/definitions/ReadMemoryArguments"
-//       }
-//     },
-//     "required": [ "command", "arguments" ]
-//   }]
-// },
-// "ReadMemoryArguments": {
-//   "type": "object",
-//   "description": "Arguments for `readMemory` request.",
-//   "properties": {
-//     "memoryReference": {
-//       "type": "string",
-//       "description": "Memory reference to the base location from which data
-//                       should be read."
-//     },
-//     "offset": {
-//       "type": "integer",
-//       "description": "Offset (in bytes) to be applied to the reference
-//                       location before reading data. Can be negative."
-//     },
-//     "count": {
-//       "type": "integer",
-//       "description": "Number of bytes to read at the specified location and
-//                       offset."
-//     }
-//   },
-//   "required": [ "memoryReference", "count" ]
-// },
-// "ReadMemoryResponse": {
-//   "allOf": [ { "$ref": "#/definitions/Response" }, {
-//     "type": "object",
-//     "description": "Response to `readMemory` request.",
-//     "properties": {
-//       "body": {
-//         "type": "object",
-//         "properties": {
-//           "address": {
-//             "type": "string",
-//             "description": "The address of the first byte of data returned.
-//                             Treated as a hex value if prefixed with `0x`, or
-//                             as a decimal value otherwise."
-//           },
-//           "unreadableBytes": {
-//             "type": "integer",
-//             "description": "The number of unreadable bytes encountered after
-//                             the last successfully read byte.\nThis can be
-//                             used to determine the number of bytes that should
-//                             be skipped before a subsequent
-//             `readMemory` request succeeds."
-//           },
-//           "data": {
-//             "type": "string",
-//             "description": "The bytes read from memory, encoded using base64.
-//                             If the decoded length of `data` is less than the
-//                             requested `count` in the original `readMemory`
-//                             request, and `unreadableBytes` is zero or
-//                             omitted, then the client should assume it's
-//                             reached the end of readable memory."
-//           }
-//         },
-//         "required": [ "address" ]
-//       }
-//     }
-//   }]
-// },
-void ReadMemoryRequestHandler::operator()(
-    const llvm::json::Object &request) const {
-  llvm::json::Object response;
-  FillResponse(request, response);
-  auto *arguments = request.getObject("arguments");
+// Reads bytes from memory at the provided location.
+//
+// Clients should only call this request if the corresponding capability
+// `supportsReadMemoryRequest` is true
+llvm::Expected<protocol::ReadMemoryResponse>
+ReadMemoryRequestHandler::Run(const protocol::ReadMemoryArguments &args) const {
 
-  llvm::StringRef memoryReference =
-      GetString(arguments, "memoryReference").value_or("");
-  auto addr_opt = DecodeMemoryReference(memoryReference);
-  if (!addr_opt.has_value()) {
-    response["success"] = false;
-    response["message"] =
-        "Malformed memory reference: " + memoryReference.str();
-    dap.SendJSON(llvm::json::Value(std::move(response)));
-    return;
-  }
-  lldb::addr_t addr_int = *addr_opt;
-  addr_int += GetInteger<uint64_t>(arguments, "offset").value_or(0);
-  const uint64_t count_requested =
-      GetInteger<uint64_t>(arguments, "count").value_or(0);
+  const std::optional<lldb::addr_t> addr_opt =
+      DecodeMemoryReference(args.memoryReference);
+  if (!addr_opt)
+    return llvm::make_error<DAPError>(
+        llvm::formatv("Malformed memory reference: {}", args.memoryReference));
+
+  const lldb::addr_t raw_address = *addr_opt + args.offset.value_or(0);
+
+  lldb::SBProcess process = dap.target.GetProcess();
+  if (!lldb::SBDebugger::StateIsStoppedState(process.GetState()))
+    return llvm::make_error<NotStoppedError>();
 
+  const uint64_t count_read = std::max<uint64_t>(args.count, 1);
   // 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);
+  auto buffer = std::vector<uint8_t>(count_read);
+
   lldb::SBError error;
-  lldb::SBAddress addr{addr_int, dap.target};
-  size_t count_result =
-      dap.target.ReadMemory(addr, buf.data(), count_read, error);
-  if (count_result == 0) {
-    response["success"] = false;
-    EmplaceSafeString(response, "message", error.GetCString());
-    dap.SendJSON(llvm::json::Value(std::move(response)));
-    return;
+  const size_t memory_count = dap.target.GetProcess().ReadMemory(
+      raw_address, buffer.data(), buffer.size(), error);
+
+  protocol::ReadMemoryResponse response;
+  response.address = "0x" + llvm::utohexstr(raw_address);
+
+  // reading memory may fail for multiple reasons. memory not readable,
+  // reading out of memory range and gaps in memory. return from
+  // the last readable byte.
+  if (error.Fail() && (memory_count < count_read)) {
+    response.unreadableBytes = count_read - memory_count;
   }
-  buf.resize(std::min<size_t>(count_result, count_requested));
 
-  llvm::json::Object body;
-  std::string formatted_addr = "0x" + llvm::utohexstr(addr_int);
-  body.try_emplace("address", formatted_addr);
-  body.try_emplace("data", llvm::encodeBase64(buf));
-  response.try_emplace("body", std::move(body));
-  dap.SendJSON(llvm::json::Value(std::move(response)));
+  buffer.resize(std::min<size_t>(memory_count, args.count));
+  if (!buffer.empty())
+    response.data = llvm::encodeBase64(buffer);
+
+  return response;
 }
 
 } // namespace lldb_dap

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

@@ -564,14 +564,17 @@ class DisassembleRequestHandler final
   Run(const protocol::DisassembleArguments &args) const override;
 };
 
-class ReadMemoryRequestHandler : public LegacyRequestHandler {
+class ReadMemoryRequestHandler final
+    : public RequestHandler<protocol::ReadMemoryArguments,
+                            llvm::Expected<protocol::ReadMemoryResponse>> {
 public:
-  using LegacyRequestHandler::LegacyRequestHandler;
+  using RequestHandler::RequestHandler;
   static llvm::StringLiteral GetCommand() { return "readMemory"; }
   FeatureSet GetSupportedFeatures() const override {
     return {protocol::eAdapterFeatureReadMemoryRequest};
   }
-  void operator()(const llvm::json::Object &request) const override;
+  llvm::Expected<protocol::ReadMemoryResponse>
+  Run(const protocol::ReadMemoryArguments &args) const override;
 };
 
 class CancelRequestHandler : public RequestHandler<protocol::CancelArguments,

lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp

@@ -926,4 +926,22 @@ llvm::json::Value toJSON(const DisassembledInstruction &DI) {
   return result;
 }
 
+bool fromJSON(const json::Value &Params, ReadMemoryArguments &RMA,
+              json::Path P) {
+  json::ObjectMapper O(Params, P);
+  return O && O.map("memoryReference", RMA.memoryReference) &&
+         O.map("count", RMA.count) && O.mapOptional("offset", RMA.offset);
+}
+
+json::Value toJSON(const ReadMemoryResponse &RMR) {
+  json::Object result{{"address", RMR.address}};
+
+  if (RMR.unreadableBytes)
+    result.insert({"unreadableBytes", RMR.unreadableBytes});
+  if (RMR.data)
+    result.insert({"data", RMR.data});
+
+  return result;
+}
+
 } // namespace lldb_dap::protocol

lldb/tools/lldb-dap/Protocol/ProtocolTypes.h

@@ -742,6 +742,42 @@ bool fromJSON(const llvm::json::Value &, DisassembledInstruction &,
               llvm::json::Path);
 llvm::json::Value toJSON(const DisassembledInstruction &);
 
+/// Arguments for `readMemory` request.
+struct ReadMemoryArguments {
+  /// Memory reference to the base location from which data should be read.
+  std::string memoryReference;
+
+  /// Offset (in bytes) to be applied to the reference location before reading
+  /// data. Can be negative.
+  std::optional<int64_t> offset;
+
+  /// Number of bytes to read at the specified location and offset.
+  uint64_t count;
+};
+bool fromJSON(const llvm::json::Value &, ReadMemoryArguments &,
+              llvm::json::Path);
+
+/// Response to `readMemory` request.
+struct ReadMemoryResponse {
+  /// The address of the first byte of data returned.
+  /// Treated as a hex value if prefixed with `0x`, or as a decimal value
+  /// otherwise.
+  std::string address;
+
+  /// The number of unreadable bytes encountered after the last successfully
+  /// read byte.
+  /// This can be used to determine the number of bytes that should be skipped
+  /// before a subsequent `readMemory` request succeeds.
+  std::optional<uint64_t> unreadableBytes;
+
+  /// The bytes read from memory, encoded using base64. If the decoded length
+  /// of `data` is less than the requested `count` in the original `readMemory`
+  /// request, and `unreadableBytes` is zero or omitted, then the client should
+  /// assume it's reached the end of readable memory.
+  std::optional<std::string> data;
+};
+llvm::json::Value toJSON(const ReadMemoryResponse &);
+
 } // namespace lldb_dap::protocol
 
 #endif

lldb/unittests/DAP/ProtocolTypesTest.cpp

@@ -765,3 +765,30 @@ TEST(ProtocolTypesTest, StepInTarget) {
   EXPECT_EQ(target.endLine, deserialized_target->endLine);
   EXPECT_EQ(target.endColumn, deserialized_target->endColumn);
 }
+
+TEST(ProtocolTypesTest, ReadMemoryArguments) {
+  ReadMemoryArguments args;
+  args.count = 20;
+  args.memoryReference = "0xabba";
+  args.offset = std::nullopt;
+
+  llvm::Expected<ReadMemoryArguments> expected = parse<ReadMemoryArguments>(
+      R"({"memoryReference":"0xabba", "count": 20})");
+  ASSERT_THAT_EXPECTED(expected, llvm::Succeeded());
+
+  EXPECT_EQ(args.count, expected->count);
+  EXPECT_EQ(args.memoryReference, expected->memoryReference);
+  EXPECT_EQ(args.offset, expected->offset);
+}
+
+TEST(ProtocolTypesTest, ReadMemoryResponse) {
+  ReadMemoryResponse response;
+  response.address = "0xdeadbeef";
+  response.data = "aGVsbG8gd29ybGQhCg==";
+  response.unreadableBytes = 0;
+
+  Expected<Value> expected = json::parse(
+      R"({ "address": "0xdeadbeef", "data": "aGVsbG8gd29ybGQhCg==", "unreadableBytes": 0})");
+  ASSERT_THAT_EXPECTED(expected, llvm::Succeeded());
+  EXPECT_EQ(pp(*expected), pp(response));
+}