[Lldb-commits] [lldb] [LLDB][Minidump]Update MinidumpFileBuilder to read and write in chunks (PR #129307)

[Jacob Lalonde via lldb-commits]

================
@@ -969,6 +969,83 @@ Status MinidumpFileBuilder::DumpDirectories() const {
   return error;
 }
 
+Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
+    const std::unique_ptr<lldb_private::DataBufferHeap> &data_up,
+    const lldb_private::CoreFileMemoryRange &range, uint64_t *bytes_read) {
+  if (!data_up)
+    return Status::FromErrorString("No buffer supplied to read memory.");
+
+  if (!bytes_read)
+    return Status::FromErrorString("Bytes read pointer cannot be null.");
+  Log *log = GetLog(LLDBLog::Object);
+  const lldb::addr_t addr = range.range.start();
+  const lldb::addr_t size = range.range.size();
+  // First we set the byte tally to 0, so if we do exit gracefully
+  // the caller doesn't think the random garbage on the stack is a
+  // success.
+  *bytes_read = 0;
+
+  uint64_t bytes_remaining = size;
+  Status error;
+  while (bytes_remaining > 0) {
+    // Get the next read chunk size as the minimum of the remaining bytes and
+    // the write chunk max size.
+    const size_t bytes_to_read =
+        std::min(bytes_remaining, data_up->GetByteSize());
+    const size_t bytes_read_for_chunk =
+        m_process_sp->ReadMemory(range.range.start() + *bytes_read,
+                                 data_up->GetBytes(), bytes_to_read, error);
+    if (error.Fail() || bytes_read_for_chunk == 0) {
+      LLDB_LOGF(log,
+                "Failed to read memory region at: %" PRIx64
+                ". Bytes read: %zu, error: %s",
+                addr, bytes_read_for_chunk, error.AsCString());
+      // If we've read nothing, and get an error or fail to read
+      // we can just give up early.
+      if (*bytes_read == 0)
+        return Status();
+
+      // If we've read some bytes, we stop trying to read more and return
+      // this best effort attempt
+      bytes_remaining = 0;
----------------
Jlalond wrote:

I'm okay with another exit statement, the `bytes_remaining == 0` does feel quite brittle. But my primary concern here is do we include or exclude partial reads. In the existing system we would just skip them. So there is a precedence for it, I think by trying to include the extra bytes we can improve the user experience.

I will update the check that if `bytes_read_for_chunk == 0` we also just quick return.

https://github.com/llvm/llvm-project/pull/129307