Update Minidump file builder to use new API

Author: Jlalond

lldb/include/lldb/Target/Process.h

@@ -1589,10 +1589,21 @@ class Process : public std::enable_shared_from_this<Process>,
   size_t ReadMemoryFromInferior(lldb::addr_t vm_addr, void *buf, size_t size,
                                 Status &error);
 
-  typedef lldb::IterationAction(ReadMemoryChunkCallback)(lldb::Status &,
-                                                         DataBufferHeap &, lldb
-                                                         : addr_t,
-                                                           lldb::offset_t);
+  // Callback definition for read Memory in chunks
+  //
+  // Status, the status returned from ReadMemoryFromInferior
+  // DataBufferHeap, buffer with bytes potentially written to it
+  // addr_t, the current_addr, start + bytes read so far.
+  // uint64_t bytes_to_read, the expected bytes read, this 
+  // is important if it's a partial read
+  // uint64_t bytes_read_for_chunk, the actual count of bytes read for this 
+  // chunk
+  typedef std::function<IterationAction(lldb_private::Status&,
+                                        lldb_private::DataBufferHeap&,
+                                        lldb::addr_t,
+                                        uint64_t,
+                                        uint64_t)> ReadMemoryChunkCallback;
+
   /// Read of memory from a process in discrete chunks, terminating
   /// either when all bytes are read, or the supplied callback returns
   /// IterationAction::Stop
@@ -1610,9 +1621,6 @@ class Process : public std::enable_shared_from_this<Process>,
   ///
   /// \param[in] callback
   ///     The callback to invoke when a chunk is read from memory.
-  /// \param[in] cacheReads
-  ///     Whether to use the ReadMemory instead of ReadMemory inferior, defaults
-  ///     to false.
   ///
   /// \return
   ///     The number of bytes that were actually read into \a buf and
@@ -1622,8 +1630,7 @@ class Process : public std::enable_shared_from_this<Process>,
   ///     vm_addr, \a buf, and \a size updated appropriately. Zero is
   ///     returned in the case of an error.
   size_t ReadMemoryInChunks(lldb::addr_t vm_addr, DataBufferHeap &data,
-                            size_t size, ReadMemoryChunkCallback callback,
-                            bool cacheReads = false);
+                            size_t size, ReadMemoryChunkCallback callback);
 
   /// Read a NULL terminated C string from memory
   ///

lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp

@@ -974,68 +974,57 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
     const lldb_private::CoreFileMemoryRange &range, uint64_t &bytes_read) {
 
   Log *log = GetLog(LLDBLog::Object);
-  const lldb::addr_t addr = range.range.start();
-  const lldb::addr_t size = range.range.size();
-
-  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_buffer.GetByteSize());
-    const size_t bytes_read_for_chunk =
-        m_process_sp->ReadMemory(range.range.start() + bytes_read,
-                                 data_buffer.GetBytes(), bytes_to_read, error);
+  Status addDataError;
+  Process::ReadMemoryChunkCallback callback =
+      [&](Status &error, DataBufferHeap &data, lldb::addr_t current_addr,
+          uint64_t bytes_to_read,
+          uint64_t bytes_read_for_chunk) -> lldb_private::IterationAction {
     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());
+                current_addr, bytes_read_for_chunk, error.AsCString());
 
       // If we failed in a memory read, we would normally want to skip
       // this entire region, if we had already written to the minidump
-      // file, we can't easily rewind the state.
+      // file, we can't easily rewind that state.
       //
       // So if we do encounter an error while reading, we just return
       // immediately, any prior bytes read will still be included but
       // any bytes partially read before the error are ignored.
-      return Status();
+      return lldb_private::IterationAction::Stop;
     }
 
-    // Write to the minidump file with the chunk potentially flushing to disk.
-    // this is the only place we want to return a true error, so that we can
-    // fail. If we get an error writing to disk we can't easily gaurauntee
-    // that we won't corrupt the minidump.
-    error = AddData(data_buffer.GetBytes(), bytes_read_for_chunk);
-    if (error.Fail())
-      return error;
-
-    // If the bytes read in this chunk would cause us to overflow, something
-    // went wrong and we should fail out of creating the Minidump.
-    if (bytes_read_for_chunk > bytes_remaining)
-      return Status::FromErrorString("Unexpected number of bytes read.");
-    else
-      bytes_remaining -= bytes_read_for_chunk;
-
-    // Update the caller with the number of bytes read, but also written to the
-    // underlying buffer.
-    bytes_read += bytes_read_for_chunk;
+    // Write to the minidump file with the chunk potentially flushing to
+    // disk.
+    // This error will be captured by the outer scope and is considered fatal.
+    // If we get an error writing to disk we can't easily guarauntee that we
+    // won't corrupt the minidump.
+    addDataError = AddData(data_buffer.GetBytes(), bytes_read_for_chunk);
+    if (addDataError.Fail())
+      return lldb_private::IterationAction::Stop;
 
     if (bytes_read_for_chunk != bytes_to_read) {
       LLDB_LOGF(log,
                 "Memory region at: %" PRIx64 " partiall read %" PRIx64
                 " bytes out of %" PRIx64 " bytes.",
-                addr, bytes_read_for_chunk,
+                current_addr, bytes_read_for_chunk,
                 bytes_to_read - bytes_read_for_chunk);
 
       // If we've read some bytes, we stop trying to read more and return
       // this best effort attempt
-      break;
+      return lldb_private::IterationAction::Stop;
     }
-  }
 
-  return error;
+    // No problems, keep going!
+    return lldb_private::IterationAction::Continue;
+  };
+
+  const lldb::addr_t addr = range.range.start();
+  const lldb::addr_t size = range.range.size();
+  bytes_read =
+      m_process_sp->ReadMemoryInChunks(addr, data_buffer, size, callback);
+  return addDataError;
 }
 
 static uint64_t

lldb/source/Target/Process.cpp

@@ -2235,6 +2235,10 @@ size_t Process::ReadMemoryFromInferior(addr_t addr, void *buf, size_t size,
 
 size_t Process::ReadMemoryInChunks(lldb::addr_t vm_addr, DataBufferHeap &data,
                                    size_t size, ReadMemoryChunkCallback callback) {
+  // Safety check to prevent an infinite loop.
+  if (data.GetByteSize() == 0)
+    return 0;
+
   uint64_t bytes_remaining = size;
   uint64_t bytes_read = 0;
   Status error;
@@ -2243,12 +2247,21 @@ size_t Process::ReadMemoryInChunks(lldb::addr_t vm_addr, DataBufferHeap &data,
     // the write chunk max size.
     const size_t bytes_to_read =
         std::min(bytes_remaining, data.GetByteSize());
-    const lldb::addr_t current_addr = vm_addr + bytes_read
+    const lldb::addr_t current_addr = vm_addr + bytes_read;
     const size_t bytes_read_for_chunk =
-        m_process_sp->ReadMemory(current_addr,
+        ReadMemoryFromInferior(current_addr,
                                  data.GetBytes(), bytes_to_read, error);
-    if (callback(error, data, current_addr, bytes_read_for_chunk) == lldb::IterationAction::Stop)
+
+    if (callback(error, data, current_addr, bytes_to_read, bytes_read_for_chunk) == IterationAction::Stop)
       break;
+
+    bytes_read += bytes_read_for_chunk;
+    // If the bytes read in this chunk would cause us to overflow, something
+    // went wrong and we should fail fast.
+    if (bytes_read_for_chunk > bytes_remaining)
+      return 0;
+    else
+      bytes_remaining -= bytes_read_for_chunk;
   }
 
   return bytes_read;