[lldb-dap] add review changes

Author: da-viper

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

@@ -10,24 +10,17 @@
 #include "JSONUtils.h"
 #include "RequestHandler.h"
 #include "llvm/ADT/StringExtras.h"
-#include "llvm/Support/Base64.h"
 
 namespace lldb_dap {
 
 // 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>
+llvm::Expected<protocol::ReadMemoryResponseBody>
 ReadMemoryRequestHandler::Run(const protocol::ReadMemoryArguments &args) const {
-
-  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);
+  const lldb::addr_t raw_address =
+      args.memoryReference + args.offset.value_or(0);
 
   lldb::SBProcess process = dap.target.GetProcess();
   if (!lldb::SBDebugger::StateIsStoppedState(process.GetState()))
@@ -37,13 +30,14 @@ ReadMemoryRequestHandler::Run(const protocol::ReadMemoryArguments &args) const {
   // We also need support reading 0 bytes
   // VS Code sends those requests to check if a `memoryReference`
   // can be dereferenced.
-  auto buffer = std::vector<uint8_t>(count_read);
+  protocol::ReadMemoryResponseBody response;
+  std::vector<std::byte> &buffer = response.data;
+  buffer.resize(count_read);
 
   lldb::SBError error;
   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,
@@ -54,9 +48,6 @@ ReadMemoryRequestHandler::Run(const protocol::ReadMemoryArguments &args) const {
   }
 
   buffer.resize(std::min<size_t>(memory_count, args.count));
-  if (!buffer.empty())
-    response.data = llvm::encodeBase64(buffer);
-
   return response;
 }
 

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

@@ -566,14 +566,14 @@ class DisassembleRequestHandler final
 
 class ReadMemoryRequestHandler final
     : public RequestHandler<protocol::ReadMemoryArguments,
-                            llvm::Expected<protocol::ReadMemoryResponse>> {
+                            llvm::Expected<protocol::ReadMemoryResponseBody>> {
 public:
   using RequestHandler::RequestHandler;
   static llvm::StringLiteral GetCommand() { return "readMemory"; }
   FeatureSet GetSupportedFeatures() const override {
     return {protocol::eAdapterFeatureReadMemoryRequest};
   }
-  llvm::Expected<protocol::ReadMemoryResponse>
+  llvm::Expected<protocol::ReadMemoryResponseBody>
   Run(const protocol::ReadMemoryArguments &args) const override;
 };
 

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

@@ -7,9 +7,11 @@
 //===----------------------------------------------------------------------===//
 
 #include "Protocol/ProtocolRequests.h"
+#include "JSONUtils.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Base64.h"
 #include "llvm/Support/JSON.h"
 #include <utility>
 
@@ -480,4 +482,39 @@ json::Value toJSON(const DisassembleResponseBody &DRB) {
   return json::Object{{"instructions", DRB.instructions}};
 }
 
+bool fromJSON(const json::Value &Params, ReadMemoryArguments &RMA,
+              json::Path P) {
+  json::ObjectMapper O(Params, P);
+
+  const json::Object *rma_obj = Params.getAsObject();
+  constexpr llvm::StringRef ref_key = "memoryReference";
+  const std::optional<llvm::StringRef> memory_ref = rma_obj->getString(ref_key);
+  if (!memory_ref) {
+    P.field(ref_key).report("missing value");
+    return false;
+  }
+
+  const std::optional<lldb::addr_t> addr_opt =
+      DecodeMemoryReference(*memory_ref);
+  if (!addr_opt) {
+    P.field(ref_key).report("Malformed memory reference");
+    return false;
+  }
+
+  RMA.memoryReference = *addr_opt;
+
+  return O && O.map("count", RMA.count) && O.mapOptional("offset", RMA.offset);
+}
+
+json::Value toJSON(const ReadMemoryResponseBody &RMR) {
+  json::Object result{{"address", RMR.address}};
+
+  if (RMR.unreadableBytes != 0)
+    result.insert({"unreadableBytes", RMR.unreadableBytes});
+  if (!RMR.data.empty())
+    result.insert({"data", llvm::encodeBase64(RMR.data)});
+
+  return result;
+}
+
 } // namespace lldb_dap::protocol

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

@@ -839,6 +839,43 @@ bool fromJSON(const llvm::json::Value &, DisassembleResponseBody &,
               llvm::json::Path);
 llvm::json::Value toJSON(const DisassembleResponseBody &);
 
+/// Arguments for `readMemory` request.
+struct ReadMemoryArguments {
+  /// Memory reference to the base location from which data should be read.
+  lldb::addr_t 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 ReadMemoryResponseBody {
+  /// 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.
+  uint64_t unreadableBytes = 0;
+
+  /// 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::vector<std::byte> data;
+  // std::optional<std::string> data;
+};
+llvm::json::Value toJSON(const ReadMemoryResponseBody &);
+
 } // namespace lldb_dap::protocol
 
 #endif

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

@@ -926,22 +926,4 @@ 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,42 +742,6 @@ 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

@@ -769,10 +769,13 @@ TEST(ProtocolTypesTest, StepInTarget) {
 TEST(ProtocolTypesTest, ReadMemoryArguments) {
   ReadMemoryArguments args;
   args.count = 20;
-  args.memoryReference = "0xabba";
+  args.memoryReference = 43962;
   args.offset = std::nullopt;
 
-  llvm::Expected<ReadMemoryArguments> expected = parse<ReadMemoryArguments>(
+  llvm::Expected<ReadMemoryArguments> expected =
+      parse<ReadMemoryArguments>(R"({"memoryReference":"-4000", "count": 20})");
+  ASSERT_THAT_EXPECTED(expected, llvm::Failed());
+  expected = parse<ReadMemoryArguments>(
       R"({"memoryReference":"0xabba", "count": 20})");
   ASSERT_THAT_EXPECTED(expected, llvm::Succeeded());
 
@@ -781,14 +784,17 @@ TEST(ProtocolTypesTest, ReadMemoryArguments) {
   EXPECT_EQ(args.offset, expected->offset);
 }
 
-TEST(ProtocolTypesTest, ReadMemoryResponse) {
-  ReadMemoryResponse response;
+TEST(ProtocolTypesTest, ReadMemoryResponseBody) {
+  ReadMemoryResponseBody response;
   response.address = "0xdeadbeef";
-  response.data = "aGVsbG8gd29ybGQhCg==";
-  response.unreadableBytes = 0;
+  const std::string data_str = "hello world!";
+  std::transform(data_str.begin(), data_str.end(),
+                 std::back_inserter(response.data),
+                 [](char letter) { return std::byte(letter); });
+  response.unreadableBytes = 1;
 
   Expected<Value> expected = json::parse(
-      R"({ "address": "0xdeadbeef", "data": "aGVsbG8gd29ybGQhCg==", "unreadableBytes": 0})");
+      R"({ "address": "0xdeadbeef", "data": "aGVsbG8gd29ybGQh", "unreadableBytes": 1})");
   ASSERT_THAT_EXPECTED(expected, llvm::Succeeded());
   EXPECT_EQ(pp(*expected), pp(response));
 }