[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

[Pavel Labath via Phabricator via lldb-commits]

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