Automerge: [lldb] Implement ProcessGDBRemote support for ReadMemoryRanges (#164311)

This commit makes use of the newly created MultiMemRead packet to
provide an efficient implementation of MultiMemRead inside
ProcessGDBRemote.

Testing is tricky, but it is accomplished two ways:

1. Some Objective-C tests would fail if this were implemented incorrectly,
as there is already an in-tree use of the base class implementation of
MultiMemRead, which is now getting replaced by the derived class.

2. One Objective-C test is modified so that we ensure the packet is
being sent by looking at the packet logs. While not the most elegant
solution, it is a strategy adopted in other tests as well. This gets
around the fact that we cannot instantiate / unittest a mock
ProcessGDBRemote.

Depends on llvm/llvm-project#163651

Author: felipepiovezan

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

@@ -86,6 +86,7 @@
 #include "lldb/Host/Host.h"
 #include "lldb/Utility/StringExtractorGDBRemote.h"
 
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringSwitch.h"
@@ -2762,6 +2763,108 @@ size_t ProcessGDBRemote::DoReadMemory(addr_t addr, void *buf, size_t size,
   return 0;
 }
 
+llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>
+ProcessGDBRemote::ReadMemoryRanges(
+    llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges,
+    llvm::MutableArrayRef<uint8_t> buffer) {
+  if (!m_gdb_comm.GetMultiMemReadSupported())
+    return Process::ReadMemoryRanges(ranges, buffer);
+
+  llvm::Expected<StringExtractorGDBRemote> response =
+      SendMultiMemReadPacket(ranges);
+  if (!response) {
+    LLDB_LOG_ERROR(GetLog(GDBRLog::Process), response.takeError(),
+                   "MultiMemRead error response: {0}");
+    return Process::ReadMemoryRanges(ranges, buffer);
+  }
+
+  llvm::StringRef response_str = response->GetStringRef();
+  const unsigned expected_num_ranges = ranges.size();
+  llvm::Expected<llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>>
+      parsed_response =
+          ParseMultiMemReadPacket(response_str, buffer, expected_num_ranges);
+  if (!parsed_response) {
+    LLDB_LOG_ERROR(GetLog(GDBRLog::Process), parsed_response.takeError(),
+                   "MultiMemRead error parsing response: {0}");
+    return Process::ReadMemoryRanges(ranges, buffer);
+  }
+  return std::move(*parsed_response);
+}
+
+llvm::Expected<StringExtractorGDBRemote>
+ProcessGDBRemote::SendMultiMemReadPacket(
+    llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges) {
+  std::string packet_str;
+  llvm::raw_string_ostream stream(packet_str);
+  stream << "MultiMemRead:ranges:";
+
+  auto range_to_stream = [&](auto range) {
+    // the "-" marker omits the '0x' prefix.
+    stream << llvm::formatv("{0:x-},{1:x-}", range.base, range.size);
+  };
+  llvm::interleave(ranges, stream, range_to_stream, ",");
+  stream << ";";
+
+  StringExtractorGDBRemote response;
+  GDBRemoteCommunication::PacketResult packet_result =
+      m_gdb_comm.SendPacketAndWaitForResponse(packet_str.data(), response,
+                                              GetInterruptTimeout());
+  if (packet_result != GDBRemoteCommunication::PacketResult::Success)
+    return llvm::createStringError(
+        llvm::formatv("MultiMemRead failed to send packet: '{0}'", packet_str));
+
+  if (response.IsErrorResponse())
+    return llvm::createStringError(
+        llvm::formatv("MultiMemRead failed: '{0}'", response.GetStringRef()));
+
+  if (!response.IsNormalResponse())
+    return llvm::createStringError(llvm::formatv(
+        "MultiMemRead unexpected response: '{0}'", response.GetStringRef()));
+
+  return response;
+}
+
+llvm::Expected<llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>>
+ProcessGDBRemote::ParseMultiMemReadPacket(llvm::StringRef response_str,
+                                          llvm::MutableArrayRef<uint8_t> buffer,
+                                          unsigned expected_num_ranges) {
+  // The sizes and the data are separated by a `;`.
+  auto [sizes_str, memory_data] = response_str.split(';');
+  if (sizes_str.size() == response_str.size())
+    return llvm::createStringError(llvm::formatv(
+        "MultiMemRead response missing field separator ';' in: '{0}'",
+        response_str));
+
+  llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> read_results;
+
+  // Sizes are separated by a `,`.
+  for (llvm::StringRef size_str : llvm::split(sizes_str, ',')) {
+    uint64_t read_size;
+    if (size_str.getAsInteger(16, read_size))
+      return llvm::createStringError(llvm::formatv(
+          "MultiMemRead response has invalid size string: {0}", size_str));
+
+    if (memory_data.size() < read_size)
+      return llvm::createStringError(
+          llvm::formatv("MultiMemRead response did not have enough data, "
+                        "requested sizes: {0}",
+                        sizes_str));
+
+    llvm::StringRef region_to_read = memory_data.take_front(read_size);
+    memory_data = memory_data.drop_front(read_size);
+
+    assert(buffer.size() >= read_size);
+    llvm::MutableArrayRef<uint8_t> region_to_write =
+        buffer.take_front(read_size);
+    buffer = buffer.drop_front(read_size);
+
+    memcpy(region_to_write.data(), region_to_read.data(), read_size);
+    read_results.push_back(region_to_write);
+  }
+
+  return read_results;
+}
+
 bool ProcessGDBRemote::SupportsMemoryTagging() {
   return m_gdb_comm.GetMemoryTaggingSupported();
 }

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

@@ -137,6 +137,22 @@ class ProcessGDBRemote : public Process,
   size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
                       Status &error) override;
 
+  /// Override of ReadMemoryRanges that uses MultiMemRead to optimize this
+  /// operation.
+  llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>
+  ReadMemoryRanges(llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges,
+                   llvm::MutableArrayRef<uint8_t> buf) override;
+
+private:
+  llvm::Expected<StringExtractorGDBRemote>
+  SendMultiMemReadPacket(llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges);
+
+  llvm::Expected<llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>>
+  ParseMultiMemReadPacket(llvm::StringRef response_str,
+                          llvm::MutableArrayRef<uint8_t> buffer,
+                          unsigned expected_num_ranges);
+
+public:
   Status
   WriteObjectFile(std::vector<ObjectFile::LoadableData> entries) override;
 

lldb/test/API/lang/objc/foundation/TestObjCMethodsNSError.py

@@ -45,3 +45,30 @@ def test_NSError_p(self):
             ],
         )
         self.runCmd("process continue")
+
+    @skipIfOutOfTreeDebugserver
+    def test_runtime_types_efficient_memreads(self):
+        # Test that we use an efficient reading of memory when reading
+        # Objective-C method descriptions.
+        logfile = os.path.join(self.getBuildDir(), "log.txt")
+        self.runCmd(f"log enable -f {logfile} gdb-remote packets process")
+        self.addTearDownHook(lambda: self.runCmd("log disable gdb-remote packets"))
+
+        self.build()
+        self.target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(
+            self, "// Break here for NSString tests", lldb.SBFileSpec("main.m", False)
+        )
+
+        self.runCmd(f"proc plugin packet send StartTesting", check=False)
+        self.expect('expression str = [NSString stringWithCString: "new"]')
+        self.runCmd(f"proc plugin packet send EndTesting", check=False)
+
+        self.assertTrue(os.path.exists(logfile))
+        log_text = open(logfile).read()
+        log_text = log_text.split("StartTesting", 1)[-1].split("EndTesting", 1)[0]
+
+        # This test is only checking that the packet it used at all (and that
+        # no errors are produced). It doesn't check that the packet is being
+        # used to solve a problem in an optimal way.
+        self.assertIn("MultiMemRead:", log_text)
+        self.assertNotIn("MultiMemRead error", log_text)