[lldb-dap] Simplify readMemory
#109485
[lldb-dap] Simplify readMemory
#109485
Conversation
|
@llvm/pr-subscribers-lldb Author: Adrian Vogelsgesang (vogelsgesang) ChangesThe We no longer set the Full diff: https://github.com/llvm/llvm-project/pull/109485.diff 2 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 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)));
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Thanks for doing this. I'll respond to your [https://github.com//pull/104317#issuecomment-2364684837] for the other thread here:
The method I'd recommend is to call ReadMemory
I assume I should prefer Target::ReadMemory over Process::readMemory, right?
That would not be a correct assumption. However, I'm also not saying you should not use Target::ReadMemory. :P
The way I understand it, the main difference between the two functions is that the Target function also works before starting a process, and it may returned cached data from disk for running processes (which means it will be faster, but potentially return incorrect data).
You probably don't care about the first point. You may care about the second one, but I don't know how you should come to a decision there. If I was doing this, I'd use SBProcess because it's more convenient (it takes an integer instead of SBAddress), and matches what you've been doing already. However, I'm leaving that up to you..
vogelsgesang
force-pushed
the
lldb-dap-readmem-2
branch
from
74fbd99 to
d039e71
Compare
I think the 2nd point shouldn't be an issue because
I think I found a good criterion: Do whatever the |
I see. SG then.
No, this sounds pretty convincing to me. This looks good to me, except for the test case failure (it fails to fail to read memory). I looked into this (for way longer than it should have been necessary), and I tracked it down to a somewhat embarrassing bug. I've created #109764 to fix that. |
Thanks for looking into this. It would certainly have taken me much longer. I will rebase on top of your commit, after your fix is merged, and then merge this pull request as soon as the test cases are green on Github |
The `readMemory` request used the `MemoryRegionInfo` so it could also support short reads. Since llvm#106532, this is no longer necessary. 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.
vogelsgesang
force-pushed
the
lldb-dap-readmem-2
branch
from
d039e71 to
13acd21
Compare
#109485 tried to std::min between size_t and uint64_t. size_t on 32 bit is 32 bits. https://lab.llvm.org/buildbot/#/builders/18/builds/4430/steps/4/logs/stdio Explicitly select the size_t template to fix this. This will truncate one of the arguments but that's the count_requested. If you're debugging from a 32 bit host and you asked it to read > 32 bit range of memory from a 64 bit target, you weren't going to have any success anyway. The final result needs to be size_t to resize the vector with.
The `readMemory` request used the `MemoryRegionInfo` so it could also support short reads. Since llvm#106532, this is no longer necessary, as mentioned by @labath in a comment on llvm#104317. With this commit, we no longer set the `unreadableBytes` in the result. But this is optional, anyway, according to the spec, and afaik the VS Code UI does not make good use of `unreadableBytes`, anyway. We prefer `SBTarget::ReadMemory` over `SBProcess::ReadMemory`, because the `memory read` command also reads memory through the target instead of the process, and because users would expect the UI view and the results from memory read to be in-sync. (cherry picked from commit 786dc5a)
llvm#109485 tried to std::min between size_t and uint64_t. size_t on 32 bit is 32 bits. https://lab.llvm.org/buildbot/#/builders/18/builds/4430/steps/4/logs/stdio Explicitly select the size_t template to fix this. This will truncate one of the arguments but that's the count_requested. If you're debugging from a 32 bit host and you asked it to read > 32 bit range of memory from a 64 bit target, you weren't going to have any success anyway. The final result needs to be size_t to resize the vector with. (cherry picked from commit 26e0b50)
The
readMemoryrequest used theMemoryRegionInfoso it could also support short reads. Since #106532, this is no longer necessary, as mentioned by @labath in a comment on #104317.With this commit, we no longer set the
unreadableBytesin the result. But this is optional, anyway, and afaik the VS Code UI would not yet make good use ofunreadableBytes, anyway.We prefer
SBTarget::ReadMemoryoverSBProcess::ReadMemory, because thememory readcommand also reads memory through the target instead of the process, and because users would expect the UI view and the results from memory read to be in-sync.