fixup! Address review comments

Author: felipepiovezan

lldb/include/lldb/Target/Process.h

@@ -1572,6 +1572,9 @@ class Process : public std::enable_shared_from_this<Process>,
                             Status &error);
 
   /// Read from multiple memory ranges and write the results into buffer.
+  /// This calls ReadMemoryFromInferior multiple times, once per range,
+  /// bypassing the read cache. Process implementations that can perform this
+  /// operation more efficiently should override this.
   ///
   /// \param[in] ranges
   ///     A collection of ranges (base address + size) to read from.

lldb/source/Target/Process.cpp

@@ -1974,25 +1974,36 @@ size_t Process::ReadMemory(addr_t addr, void *buf, size_t size, Status &error) {
 llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>
 Process::ReadMemoryRanges(llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges,
                           llvm::MutableArrayRef<uint8_t> buffer) {
-  llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> results;
+  auto total_ranges_len = llvm::sum_of(
+      llvm::map_range(ranges, [](auto range) { return range.size; }));
+  // If the buffer is not large enough, this is a programmer error.
+  // In production builds, gracefully fail by returning empty chunks.
+  assert(buffer.size() >= total_ranges_len);
+  if (buffer.size() < total_ranges_len)
+    return llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>(ranges.size());
 
-  for (auto [addr, len] : ranges) {
-    // This is either a programmer error, or a protocol violation.
-    // In production builds, gracefully fail.
-    assert(buffer.size() >= len);
-    if (buffer.size() < len) {
-      results.push_back(buffer.take_front(0));
-      continue;
-    }
+  llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> results;
 
+  // While `buffer` has space, take the next requested range and read
+  // memory into a `buffer` chunk, then slice it to remove the used chunk.
+  for (auto [addr, range_len] : ranges) {
     Status status;
     size_t num_bytes_read =
-        ReadMemoryFromInferior(addr, buffer.data(), len, status);
+        ReadMemoryFromInferior(addr, buffer.data(), range_len, status);
     // FIXME: ReadMemoryFromInferior promises to return 0 in case of errors, but
     // it doesn't; it never checks for errors.
     if (status.Fail())
       num_bytes_read = 0;
+
+    assert(num_bytes_read <= range_len && "read more than requested bytes");
+    if (num_bytes_read > range_len) {
+      // In production builds, gracefully fail by returning an empty chunk.
+      results.emplace_back();
+      continue;
+    }
+
     results.push_back(buffer.take_front(num_bytes_read));
+    // Slice buffer to remove the used chunk.
     buffer = buffer.drop_front(num_bytes_read);
   }
 

lldb/unittests/Target/MemoryTest.cpp

@@ -18,7 +18,6 @@
 #include "lldb/Utility/DataBufferHeap.h"
 #include "gtest/gtest.h"
 #include <cstdint>
-#include <random>
 
 using namespace lldb_private;
 using namespace lldb;
@@ -228,15 +227,15 @@ TEST_F(MemoryTest, TesetMemoryCacheRead) {
                                                        // old cache
 }
 
-/// A process class that reads `lower_byte(address)` for each `address` it
-/// reads.
+/// A process class that, when asked to read memory from some address X, returns
+/// the least significant byte of X.
 class DummyReaderProcess : public Process {
 public:
   size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
                       Status &error) override {
     uint8_t *buffer = static_cast<uint8_t *>(buf);
     for (size_t addr = vm_addr; addr < vm_addr + size; addr++)
-      buffer[addr - vm_addr] = addr;
+      buffer[addr - vm_addr] = static_cast<uint8_t>(addr); // LSB of addr.
     return size;
   }
   // Boilerplate, nothing interesting below.
@@ -270,12 +269,11 @@ TEST_F(MemoryTest, TestReadMemoryRanges) {
 
   llvm::SmallVector<uint8_t, 0> buffer(1024, 0);
 
-  // Read 8 ranges of 128 bytes, starting at random addresses
-  std::mt19937 rng(42);
-  std::uniform_int_distribution<addr_t> distribution(1, 100000);
-  llvm::SmallVector<Range<addr_t, size_t>> ranges;
-  for (unsigned i = 0; i < 1024; i += 128)
-    ranges.emplace_back(distribution(rng), 128);
+  // Read 8 ranges of 128 bytes with arbitrary base addresses.
+  llvm::SmallVector<Range<addr_t, size_t>> ranges = {
+      {0x12345, 128},      {0x11112222, 128}, {0x77777777, 128},
+      {0xffaabbccdd, 128}, {0x0, 128},        {0x4242424242, 128},
+      {0x17171717, 128},   {0x99999, 128}};
 
   llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> read_results =
       process->ReadMemoryRanges(ranges, buffer);