Redo callback prototype and refactor to not pass expected size and the read chunk size. Also don't log if a truncated read is the final read

Author: Jlalond

lldb/include/lldb/Target/Process.h

@@ -1592,14 +1592,12 @@ class Process : public std::enable_shared_from_this<Process>,
   // Callback definition for read Memory in chunks
   //
   // Status, the status returned from ReadMemoryFromInferior
-  // uint8_t*, pointer to the bytes read
-  // 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 &, const void *,
-                                        lldb::addr_t, uint64_t, uint64_t)>
+  // void*, pointer to the bytes read
+  // addr_t, the bytes_addr, start + bytes read so far.
+  // bytes_size, the count of bytes read for this chunk
+  typedef std::function<IterationAction(
+      lldb_private::Status &error, lldb::addr_t bytes_addr, const void *bytes,
+      lldb::offset_t bytes_size)>
       ReadMemoryChunkCallback;
 
   /// Read of memory from a process in discrete chunks, terminating
@@ -1611,15 +1609,16 @@ class Process : public std::enable_shared_from_this<Process>,
   ///     memory from.
   ///
   /// \param[in] buf
-  ///     A byte buffer that is at least \a chunk_size bytes long that
-  ///     will receive the memory bytes.
+  ///    If NULL, a buffer of \a chunk_size will be created and used for the
+  ///    callback. If non NULL, this buffer must be at least \a chunk_size bytes
+  ///    and will be used for storing chunked memory reads.
   ///
   /// \param[in] chunk_size
   ///     The minimum size of the byte buffer, and the chunk size of memory
   ///     to read.
   ///
-  /// \param[in] size
-  ///     The number of bytes to read.
+  /// \param[in] total_size
+  ///     The total number of bytes to read.
   ///
   /// \param[in] callback
   ///     The callback to invoke when a chunk is read from memory.
@@ -1631,8 +1630,8 @@ class Process : public std::enable_shared_from_this<Process>,
   ///     size, then this function will get called again with \a
   ///     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, void *buf,
-                            lldb::addr_t chunk_size, size_t size,
+  lldb::offset_t ReadMemoryInChunks(lldb::addr_t vm_addr, void *buf,
+                            lldb::addr_t chunk_size, lldb::offset_t total_size,
                             ReadMemoryChunkCallback callback);
 
   /// Read a NULL terminated C string from memory

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

@@ -973,17 +973,18 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
     lldb_private::DataBufferHeap &data_buffer,
     const lldb_private::CoreFileMemoryRange &range, uint64_t &bytes_read) {
 
+  const lldb::addr_t addr = range.range.start();
+  const lldb::addr_t size = range.range.size();
   Log *log = GetLog(LLDBLog::Object);
   Status addDataError;
   Process::ReadMemoryChunkCallback callback =
-      [&](Status &error, const void *buf, 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) {
+      [&](Status &error, lldb::addr_t current_addr, const void *buf,
+          uint64_t bytes_read) -> lldb_private::IterationAction {
+    if (error.Fail() || bytes_read == 0) {
       LLDB_LOGF(log,
-                "Failed to read memory region at: %" PRIx64
-                ". Bytes read: %zu, error: %s",
-                current_addr, bytes_read_for_chunk, error.AsCString());
+                "Failed to read memory region at: 0x%" PRIx64
+                ". Bytes read: %" PRIx64 ", error: %s",
+                current_addr, bytes_read, 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
@@ -1000,16 +1001,19 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
     // 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(buf, bytes_read_for_chunk);
+    addDataError = AddData(buf, bytes_read);
     if (addDataError.Fail())
       return lldb_private::IterationAction::Stop;
 
-    if (bytes_read_for_chunk != bytes_to_read) {
+    // If we have a partial read, report it, but only if the partial read
+    // didn't finish reading the entire region.
+    if (bytes_read != data_buffer.GetByteSize() &&
+        current_addr + bytes_read != size) {
       LLDB_LOGF(log,
-                "Memory region at: %" PRIx64 " partiall read %" PRIx64
+                "Memory region at: %" PRIx64 " partiall read 0x%" PRIx64
                 " bytes out of %" PRIx64 " bytes.",
-                current_addr, bytes_read_for_chunk,
-                bytes_to_read - bytes_read_for_chunk);
+                current_addr, bytes_read,
+                data_buffer.GetByteSize() - bytes_read);
 
       // If we've read some bytes, we stop trying to read more and return
       // this best effort attempt
@@ -1020,8 +1024,6 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
     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.GetBytes(), data_buffer.GetByteSize(), size, callback);
   return addDataError;

lldb/source/Target/Process.cpp

@@ -2233,36 +2233,44 @@ size_t Process::ReadMemoryFromInferior(addr_t addr, void *buf, size_t size,
   return bytes_read;
 }
 
-size_t Process::ReadMemoryInChunks(lldb::addr_t vm_addr, void *buf,
-                                   lldb::addr_t chunk_size, size_t size,
+lldb::offset_t Process::ReadMemoryInChunks(lldb::addr_t vm_addr, void *buf,
+                                   lldb::addr_t chunk_size, lldb::offset_t size,
                                    ReadMemoryChunkCallback callback) {
   // Safety check to prevent an infinite loop.
   if (chunk_size == 0)
     return 0;
 
-  // Create a data buffer heap of the specified size, initialized to 0.
+  // Buffer for when a NULL buf is provided, initialized
+  // to 0 bytes, we set it to chunk_size and then replace buf
+  // with the new buffer.
+  DataBufferHeap data_buffer;
+  if (!buf) {
+    data_buffer.SetByteSize(chunk_size);
+    buf = data_buffer.GetBytes();
+  }
+
   uint64_t bytes_remaining = size;
   uint64_t 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, chunk_size);
+    const lldb::addr_t bytes_to_read = std::min(bytes_remaining, chunk_size);
     const lldb::addr_t current_addr = vm_addr + bytes_read;
-    const size_t bytes_read_for_chunk =
+    const lldb::addr_t bytes_read_for_chunk =
         ReadMemoryFromInferior(current_addr, buf, bytes_to_read, error);
 
-    if (callback(error, buf, 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;
+
+    if (callback(error, current_addr, buf, bytes_read_for_chunk) ==
+      IterationAction::Stop)
+      break;
   }
 
   return bytes_read;