[lldb] Implement Process::{ReadPointers,ReadUnsignedIntegers}FromMemory #11686
Conversation
lldb/source/Target/Process.cpp
Outdated
| continue; | ||
| } | ||
|
|
||
| DataExtractor data(range.data(), integer_byte_size, GetByteOrder(), |
There was a problem hiding this comment.
You put addr_size in a const int earlier to avoid fetching it for each element, but fetch the byte order for each element. Pls do both the same way.
lldb/source/Target/Process.cpp
Outdated
|
|
||
| DataExtractor data(range.data(), integer_byte_size, GetByteOrder(), | ||
| addr_size); | ||
| lldb::offset_t offset = 0; |
There was a problem hiding this comment.
super unimportant nit, but you don't need to prefix lldb types with lldb:: in our source files. There were some lldb::addr_t's in the PR as well.
There was a problem hiding this comment.
Ahhh, that's true. In the headers we do though :(
I'll remove them from the cpp file
lldb/unittests/Target/MemoryTest.cpp
Outdated
| llvm::SmallVector<std::optional<addr_t>> read_results = | ||
| process->ReadPointersFromMemory(ptr_locs); | ||
|
|
||
| for (auto [maybe_ptr, ptr_loc] : llvm::zip(read_results, ptr_locs)) { |
lldb/unittests/Target/MemoryTest.cpp
Outdated
| llvm::SmallVector<std::optional<addr_t>> read_results = | ||
| process->ReadPointersFromMemory(ptr_locs); | ||
|
|
||
| for (auto [maybe_ptr, ptr_loc] : llvm::zip(read_results, ptr_locs)) { |
There was a problem hiding this comment.
I don't understand these unit tests. Where are the expected_result bytes passed to the DummyProcess, so that the ReadMemory calls work correctly? Is this byte sequence already in DummyProcess so reads from any address will return it? I couldn't find anything like that in the DummyProcess class but maybe I missed it.
There was a problem hiding this comment.
I think you looked at the wrong one, there is a "DummyProcess" and there is a "DummyReaderProcess", the tests in this PR are using the latter:
/// A process class that, when asked to read memory from some address X, returns
/// the least significant byte of X.
class DummyReaderProcess : public Process {
There was a problem hiding this comment.
which makes me realize: there is a wrong static cast in this test!!
These are methods that use the MultiMemRead packet to speed up reading unsigned numbers / pointers from memory. As the comments say, they should go upstream once we find a use for them there (and the timing constraints are a bit more favourable).
|
Addressed review comments |
felipepiovezan
force-pushed
the
felipe/proc_read_poiters_from_mem
branch
from
e27c23b to
7ef61a7
Compare
|
@swift-ci test |
| // Read pointers at arbitrary addresses. | ||
| llvm::SmallVector<addr_t> ptr_locs = {0x0, 0x100, 0x2000, 0x123400}; | ||
| // Because of how DummyReaderProcess works, each byte of a memory read result | ||
| // is its address modulo 256: |
There was a problem hiding this comment.
This comment (and the DummyReaderProcess impl) describe what is done in TestReadUnsignedIntgersFromMemory, but in this test we're reading from a series of addresses that have a low byte of 0, so the returned result will be 64-bits of 0x0 in each case. How does this expected_result get returned?
There was a problem hiding this comment.
so the returned result will be 64-bits of 0x0 in each case
I don't think that's true. DummyReaderProcess reads byte by byte. So if the pointer's address starts at 0, the pointer will have 8 bytes, the first byte being zero, the last byte being 7
There was a problem hiding this comment.
for (size_t addr = vm_addr; addr < vm_addr + size; addr++)
buffer[addr - vm_addr] = static_cast<uint8_t>(addr); // LSB of addr.
?
There was a problem hiding this comment.
Your comment above describes exactly this - reading 4 bytes from addr 0x5 will get you a UInt32 of 0x05050505. That's what TestReadUnsignedIntegersFromMemory is doing. But this method is expecting a UInt64 with different values in each byte pos.
There was a problem hiding this comment.
reading 4 bytes from addr 0x5 will get you a UInt32 of 0x05050505
No, reading 4 bytes from addr 0x5 will get you 0x05060708 (or the other way around, endianness is hard)
There was a problem hiding this comment.
ah my bad, i see you're incrementing addr in your writer loop. I thought it would not mutate that.
There was a problem hiding this comment.
fwiw each byte of a memory read result is its address modulo 256 -- I'd probably say "the byte's address" instead of "its address" because it's easy for me to read this as "the address of the byte range % 256"
There was a problem hiding this comment.
ah my bad, i see you're incrementing
addrin your writer loop. I thought it would not mutate that.
addr is the loop variable; the function argument is vm_addr, which is not mutated.
I will try to update that code on a separate PR to change the comment as you described and rename some variables.
|
@swift-ci test macos platform |
|
13:00:11 lldb-api :: tools/lldb-dap/attach/TestDAP_attach.py failed for a second time in a row... |
|
@swift-ci test macos platform |
|
@swift-ci test windows platform |
|
@swift-ci test macOS platform |
2 participants
These are methods that use the MultiMemRead packet to speed up reading unsigned numbers / pointers from memory.
As the comments say, they should go upstream once we find a use for them there (and the timing constraints are a bit more favourable).