[lldb-dap] Use protocol types for ReadMemory request #144552
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) ChangesRead memory from process instead of target. Full diff: https://github.com/llvm/llvm-project/pull/144552.diff 6 Files Affected:
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 74062f3ab2164..55fb4a961e783 100644
--- a/lldb/test/API/tools/lldb-dap/memory/TestDAP_memory.py
+++ b/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()
diff --git a/lldb/tools/lldb-dap/Handler/ReadMemoryRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/ReadMemoryRequestHandler.cpp
index 891c2af4f2f28..a44243b187709 100644
--- a/lldb/tools/lldb-dap/Handler/ReadMemoryRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/ReadMemoryRequestHandler.cpp
@@ -7,7 +7,6 @@
//===----------------------------------------------------------------------===//
#include "DAP.h"
-#include "EventHelper.h"
#include "JSONUtils.h"
#include "RequestHandler.h"
#include "llvm/ADT/StringExtras.h"
@@ -15,128 +14,50 @@
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
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index 054cc7a321316..6008fa43bd8c6 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/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,
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
index 7f96c07faae10..3762aec7c4f4a 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
+++ b/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
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
index 7fe7454113994..92d180cdef7e8 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
+++ b/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
diff --git a/lldb/unittests/DAP/ProtocolTypesTest.cpp b/lldb/unittests/DAP/ProtocolTypesTest.cpp
index 46a09f090fea2..bf3956273aba1 100644
--- a/lldb/unittests/DAP/ProtocolTypesTest.cpp
+++ b/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));
+}
\ No newline at end of file
|
ashgti
reviewed
Jun 17, 2025
|
|
||
| /// Offset (in bytes) to be applied to the reference location before reading | ||
| /// data. Can be negative. | ||
| std::optional<int64_t> offset; |
There was a problem hiding this comment.
For simple values, like this, could we default to 0 and remove the optional? That makes it a little more straight forward to reason about when working with the type and we can not include the value if its 0 in the toJSON.
ashgti
approved these changes
Jun 17, 2025
| /// 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; |
There was a problem hiding this comment.
Sorry for making you go back and forth on this. Reading more of the helpers in llvm/ADT/StringExtras.h and the like, it looks like in llvm raw data bytes are are often either represented as llvm::ArrayRef<uint8_t> (for non-owning cases) or a std::string (for cases where you'd want to own the underlying data buffer).
I'm just used to swift where there is a separate Data type (from Foundation or Array<Uint8> used in the swift stdlib) for this and I was trying to figure out the c++ equivalent. It seems that std::string is the common way to handle this.
I guess we can go back to std::string data; here and then still convert the value to hex in from/toJSON or if you want to keep using the std::vector<std::byte> thats also fine.
In the SB API there is a lldb::SBData type for this use case and in lldb_private its built on top of lldb_private::DataExtractor but for the protocol representation, I am not sure either of those types make sense.
I'm open to suggestions for this, I just was worried about mishandling the data since it looked like a string.
There was a problem hiding this comment.
I am happy with std::vector<std::byte> since it feels more representative of the actual data.
JDevlieghere
approved these changes
Jun 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Read memory from process instead of target.
If cannot read the entire range, report the unread location.