[lldb] Implement Process::ReadMemoryRanges (llvm#163651)

This commit introduces a base-class implementation for a method that
reads memory from multiple ranges at once. This implementation simply
calls the underlying `ReadMemoryFromInferior` method on each requested
range, intentionally bypassing the memory caching mechanism (though this
may be easily changed in the future).

`Process` implementations that can be perform this operation more
efficiently - e.g. with the MultiMemPacket described in [1] - are
expected to override this method.

As an example, this commit changes AppleObjCClassDescriptorV2 to use the
new API.

Note about the API
------------------

In the RFC, we discussed having the API return some kind of class
`ReadMemoryRangesResult`. However, while writing such a class, it became
clear that it was merely wrapping a vector, without providing anything
useful. For example, this class:

```
struct ReadMemoryRangesResult {
  ReadMemoryRangesResult(
      llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> ranges)
      : ranges(std::move(ranges)) {}

  llvm::ArrayRef<llvm::MutableArrayRef<uint8_t>> getRanges() const {
    return ranges;
  }

private:
  llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> ranges;
};
```

As can be seen in the added test and in the added use-case
(AppleObjCClassDescriptorV2), users of this API will just iterate over
the vector of memory buffers. So they want a return type that can be
iterated over, and the vector seems more natural than creating a new
class and defining iterators for it.

Likewise, in the RFC, we discussed wrapping the result into an
`Expected`. Upon experimenting with the code, this feels like it limits
what the API is able to do as the base class implementation never needs
to fail the entire result, it's the individual reads that may fail and
this is expressed through a zero-length result. Any derived classes
overriding `ReadMemoryRanges` should also never produce a top level
failure: if they did, they can just fall back to the base class
implementation, which would produce a better result.

The choice of having the caller allocate a buffer and pass it to
`Process::ReadMemoryRanges` is done mostly to follow conventions already
done in the Process class.

[1]:
https://discourse.llvm.org/t/rfc-a-new-vectorized-memory-read-packet/

(cherry picked from commit f2cb997)
(cherry picked from commit bccb899)

Author: felipepiovezan

lldb/include/lldb/Target/Process.h

@@ -1574,6 +1574,28 @@ class Process : public std::enable_shared_from_this<Process>,
   virtual size_t ReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
                             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.
+  ///
+  /// \param[out] buffer
+  ///     A buffer where the read memory will be written to. It must be at least
+  ///     as long as the sum of the sizes of each range.
+  ///
+  /// \return
+  ///     A vector of MutableArrayRef, where each MutableArrayRef is a slice of
+  ///     the input buffer into which the memory contents were copied. The size
+  ///     of the slice indicates how many bytes were read successfully. Partial
+  ///     reads are always performed from the start of the requested range,
+  ///     never from the middle or end.
+  virtual llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>
+  ReadMemoryRanges(llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges,
+                   llvm::MutableArrayRef<uint8_t> buffer);
+
   /// Read of memory from a process.
   ///
   /// This function has the same semantics of ReadMemory except that it

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCClassDescriptorV2.cpp

@@ -279,22 +279,23 @@ ClassDescriptorV2::ReadMethods(llvm::ArrayRef<lldb::addr_t> addresses,
   const size_t num_methods = addresses.size();
 
   llvm::SmallVector<uint8_t, 0> buffer(num_methods * size, 0);
-  llvm::DenseSet<uint32_t> failed_indices;
 
-  for (auto [idx, addr] : llvm::enumerate(addresses)) {
-    Status error;
-    process->ReadMemory(addr, buffer.data() + idx * size, size, error);
-    if (error.Fail())
-      failed_indices.insert(idx);
-  }
+  llvm::SmallVector<Range<addr_t, size_t>> mem_ranges =
+      llvm::to_vector(llvm::map_range(llvm::seq(num_methods), [&](size_t idx) {
+        return Range<addr_t, size_t>(addresses[idx], size);
+      }));
+
+  llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> read_results =
+      process->ReadMemoryRanges(mem_ranges, buffer);
 
   llvm::SmallVector<method_t, 0> methods;
   methods.reserve(num_methods);
-  for (auto [idx, addr] : llvm::enumerate(addresses)) {
-    if (failed_indices.contains(idx))
+  for (auto [addr, memory] : llvm::zip(addresses, read_results)) {
+    // Ignore partial reads.
+    if (memory.size() != size)
       continue;
-    DataExtractor extractor(buffer.data() + idx * size, size,
-                            process->GetByteOrder(),
+
+    DataExtractor extractor(memory.data(), size, process->GetByteOrder(),
                             process->GetAddressByteSize());
     methods.push_back(method_t());
     methods.back().Read(extractor, process, addr, relative_selector_base_addr,

lldb/source/Target/Process.cpp

@@ -2119,6 +2119,50 @@ 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) {
+  uint64_t total_ranges_len = 0;
+  for (auto range : ranges)
+    total_ranges_len += range.size;
+  // If the buffer is not large enough, this is a programmer error.
+  // In production builds, gracefully fail by returning a length of 0 for all
+  // ranges.
+  assert(buffer.size() >= total_ranges_len && "provided buffer is too short");
+  if (buffer.size() < total_ranges_len) {
+    llvm::MutableArrayRef<uint8_t> empty;
+    return {ranges.size(), empty};
+  }
+
+  llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> results;
+
+  // While `buffer` has space, take the next requested range and read
+  // memory into a `buffer` piece, then slice it to remove the used memory.
+  for (auto [addr, range_len] : ranges) {
+    Status status;
+    size_t num_bytes_read =
+        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 length zero for this
+      // range.
+      results.emplace_back();
+      continue;
+    }
+
+    results.push_back(buffer.take_front(num_bytes_read));
+    // Slice buffer to remove the used memory.
+    buffer = buffer.drop_front(num_bytes_read);
+  }
+
+  return results;
+}
+
 void Process::DoFindInMemory(lldb::addr_t start_addr, lldb::addr_t end_addr,
                              const uint8_t *buf, size_t size,
                              AddressRanges &matches, size_t alignment,

lldb/unittests/Target/MemoryTest.cpp

@@ -17,6 +17,7 @@
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "gtest/gtest.h"
+#include <cstdint>
 
 using namespace lldb_private;
 using namespace lldb_private::repro;
@@ -226,3 +227,144 @@ TEST_F(MemoryTest, TesetMemoryCacheRead) {
                                                        // instead of using an
                                                        // old cache
 }
+
+/// 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:
+  // If true, `DoReadMemory` will not return all requested bytes.
+  // It's not possible to control exactly how many bytes will be read, because
+  // Process::ReadMemoryFromInferior tries to fulfill the entire request by
+  // reading smaller chunks until it gets nothing back.
+  bool read_less_than_requested = false;
+  bool read_more_than_requested = false;
+
+  size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
+                      Status &error) override {
+    if (read_less_than_requested && size > 0)
+      size--;
+    if (read_more_than_requested)
+      size *= 2;
+    uint8_t *buffer = static_cast<uint8_t *>(buf);
+    for (size_t addr = vm_addr; addr < vm_addr + size; addr++)
+      buffer[addr - vm_addr] = static_cast<uint8_t>(addr); // LSB of addr.
+    return size;
+  }
+  // Boilerplate, nothing interesting below.
+  DummyReaderProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+      : Process(target_sp, listener_sp) {}
+  bool CanDebug(lldb::TargetSP, bool) override { return true; }
+  Status DoDestroy() override { return {}; }
+  void RefreshStateAfterStop() override {}
+  bool DoUpdateThreadList(ThreadList &, ThreadList &) override { return false; }
+  llvm::StringRef GetPluginName() override { return "Dummy"; }
+};
+
+TEST_F(MemoryTest, TestReadMemoryRanges) {
+  ArchSpec arch("x86_64-apple-macosx-");
+
+  Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch));
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  TargetSP target_sp = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp =
+      std::make_shared<DummyReaderProcess>(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+
+  {
+    llvm::SmallVector<uint8_t, 0> buffer(1024, 0);
+    // 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_sp->ReadMemoryRanges(ranges, buffer);
+
+    for (auto [range, memory] : llvm::zip(ranges, read_results)) {
+      ASSERT_EQ(memory.size(), 128u);
+      addr_t range_base = range.GetRangeBase();
+      for (auto [idx, byte] : llvm::enumerate(memory))
+        ASSERT_EQ(byte, static_cast<uint8_t>(range_base + idx));
+    }
+  }
+
+  auto &dummy_process = static_cast<DummyReaderProcess &>(*process_sp);
+  dummy_process.read_less_than_requested = true;
+  {
+    llvm::SmallVector<uint8_t, 0> buffer(1024, 0);
+    llvm::SmallVector<Range<addr_t, size_t>> ranges = {
+        {0x12345, 128}, {0x11112222, 128}, {0x77777777, 128}};
+    llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> read_results =
+        dummy_process.ReadMemoryRanges(ranges, buffer);
+    for (auto [range, memory] : llvm::zip(ranges, read_results)) {
+      ASSERT_LT(memory.size(), 128u);
+      addr_t range_base = range.GetRangeBase();
+      for (auto [idx, byte] : llvm::enumerate(memory))
+        ASSERT_EQ(byte, static_cast<uint8_t>(range_base + idx));
+    }
+  }
+}
+
+using MemoryDeathTest = MemoryTest;
+
+TEST_F(MemoryDeathTest, TestReadMemoryRangesReturnsTooMuch) {
+  ArchSpec arch("x86_64-apple-macosx-");
+  Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch));
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+  TargetSP target_sp = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp =
+      std::make_shared<DummyReaderProcess>(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+
+  auto &dummy_process = static_cast<DummyReaderProcess &>(*process_sp);
+  dummy_process.read_more_than_requested = true;
+  llvm::SmallVector<uint8_t, 0> buffer(1024, 0);
+  llvm::SmallVector<Range<addr_t, size_t>> ranges = {{0x12345, 128}};
+
+  llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> read_results;
+  ASSERT_DEBUG_DEATH(
+      { read_results = process_sp->ReadMemoryRanges(ranges, buffer); },
+      "read more than requested bytes");
+#ifdef NDEBUG
+  // With asserts off, the read should return empty ranges.
+  ASSERT_EQ(read_results.size(), 1u);
+  ASSERT_TRUE(read_results[0].empty());
+#endif
+}
+
+TEST_F(MemoryDeathTest, TestReadMemoryRangesWithShortBuffer) {
+  ArchSpec arch("x86_64-apple-macosx-");
+  Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch));
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+  TargetSP target_sp = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp =
+      std::make_shared<DummyReaderProcess>(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+
+  llvm::SmallVector<uint8_t, 0> short_buffer(10, 0);
+  llvm::SmallVector<Range<addr_t, size_t>> ranges = {{0x12345, 128},
+                                                     {0x11, 128}};
+  llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> read_results;
+  ASSERT_DEBUG_DEATH(
+      { read_results = process_sp->ReadMemoryRanges(ranges, short_buffer); },
+      "provided buffer is too short");
+#ifdef NDEBUG
+  // With asserts off, the read should return empty ranges.
+  ASSERT_EQ(read_results.size(), ranges.size());
+  for (llvm::MutableArrayRef<uint8_t> result : read_results)
+    ASSERT_TRUE(result.empty());
+#endif
+}