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

[via lldb-commits]

================
@@ -969,6 +969,84 @@ 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.");
+  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.
+  if (bytes_read)
+    *bytes_read = 0;
+
+  uint64_t bytes_remaining = size;
+  uint64_t total_bytes_read = 0;
+  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, MAX_WRITE_CHUNK_SIZE);
----------------
jeffreytan81 wrote:

I am not a big fan of this -- you are trying to ensure the reading won't exceed `data_up` buffer size, then you should use `data_up->GetSize()` here instead of `MAX_WRITE_CHUNK_SIZE`. What is the `data_up` is smaller than `MAX_WRITE_CHUNK_SIZE`? Then you are relying on the fact that `size` from the range ensures `data_up` size. You can see there are two much coupling logic between caller and callee here. 

I would change to `std::min(bytes_remaining, data_up->GetSize());`. That's the purpose of passing `data_up` into this function anyway, otherwise, we should pass `data_up->GetBytes()` into this function.

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