labath added a comment.
Given that you're improving the linux implementation (which is the only one
that benefits from chunked reads) of ReadMemory in
https://reviews.llvm.org/D62715, does it make sense to do the chunking here?
After all, if an implementation (like NetBSD) has an efficient ptrace interface
for reading memory, then the chunked reads might actually be pessimizing it.
Theoretically, the linux implementation could be improved further to use the
chunking ("If I am using ptrace, and have just crossed a page boundary, try
process_vm_readv again in case the new page happens to be readable"), but I'm
not sure if that is really necessary.
================
Comment at: lldb/source/Host/common/NativeProcessProtocol.cpp:686
+
+ auto str_end = std::memchr(curr_buffer, '\0', bytes_read);
+ if (str_end != NULL) {
----------------
s/auto/void */
================
Comment at: lldb/source/Host/common/NativeProcessProtocol.cpp:687
+ auto str_end = std::memchr(curr_buffer, '\0', bytes_read);
+ if (str_end != NULL) {
+ total_bytes_read =
----------------
s/NULL/nullptr/
================
Comment at: lldb/source/Host/common/NativeProcessProtocol.cpp:696-697
+ curr_buffer += bytes_read;
+ curr_addr = reinterpret_cast<addr_t>(reinterpret_cast<char *>(curr_addr) +
+ bytes_read);
+ bytes_left -= bytes_read;
----------------
curr_addr+=bytes_read
================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2093
+ error = ReadCStringFromMemory(link_map.l_name,
+ reinterpret_cast<char *>(&name_buffer),
+ sizeof(name_buffer), bytes_read);
----------------
It doesn't look like the reinterpret_cast should be needed here.
================
Comment at: lldb/unittests/Host/NativeProcessProtocolTest.cpp:128-133
+ EXPECT_THAT_ERROR(
+ Process.ReadCStringFromMemory(0x0, &string[0], sizeof(string),
bytes_read)
+ .ToError(),
+ llvm::Succeeded());
+ EXPECT_STREQ(string, "hel");
+ EXPECT_EQ(bytes_read, 3UL);
----------------
I'm wondering how will the caller know that the read has been truncated. Given
that ReadMemory already returns an error in case of partial reads, maybe we
could do the same here ? (return an error, but still set bytes_read and the
string as they are now). What do you think ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62503/new/
https://reviews.llvm.org/D62503
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
