[lldb] Implement ProcessGDBRemote support for ReadMemoryRanges

[felipepiovezan]

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 is 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 #163651

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

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 is implemented correctly, 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 #163651

---

Full diff: https://github.com/llvm/llvm-project/pull/164311.diff

3 Files Affected:

• (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+102)
• (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h (+14)
• (modified) lldb/test/API/lang/objc/foundation/TestObjCMethodsNSError.py (+23)

diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 91f3a6ce383b1..d488ef4444d42 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/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,107 @@ 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: '{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();
 }
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
index 7c3dfb179a4b3..342edbbbbbb4a 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -137,6 +137,20 @@ class ProcessGDBRemote : public Process,
   size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
                       Status &error) override;
 
+  llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>
+  ReadMemoryRanges(llvm::ArrayRef<Range<lldb::addr_t, size_t>> vm_addrs,
+                   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;
 
diff --git a/lldb/test/API/lang/objc/foundation/TestObjCMethodsNSError.py b/lldb/test/API/lang/objc/foundation/TestObjCMethodsNSError.py
index a14035db5e057..5ebbfba02e74f 100644
--- a/lldb/test/API/lang/objc/foundation/TestObjCMethodsNSError.py
+++ b/lldb/test/API/lang/objc/foundation/TestObjCMethodsNSError.py
@@ -45,3 +45,26 @@ 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]
+        self.assertIn("MultiMemRead:", log_text)
+        self.assertNotIn("MultiMemRead error", log_text)

[jasonmolenda]

Does that text appear if there's an error? Won't it just be an Exx reply code from debugserver?

[felipepiovezan]

Ohh i should add a comment about this, the sneaky thing is that I also enabled the gdbremote process log

[DavidSpickett]

Some Objective-C tests would fail if this is implemented correctly

I assume you mean incorrectly.

[DavidSpickett]

Also assert that it is in there one or N times whatever is correct.

[felipepiovezan]

Sadly I don't think we can do this, as it is very dependent on debug info for core objective C libraries. I wish we had a more direct way of testing things, Jason and I could not come up with anything better

[jasonmolenda]

If we had a command that exclusively could be completed with only a MultiMemRead, we might be able to assert that no x/m packets were sent. But this core operation will still do memory reads to get c-strings for the methods; we didn't tackle Process::MultiCStringRead, but even if we had there would be a handful of simple memory reads issued while evaluating the expression. I think confirming the presence of at least one MultiMemRead: packet may be the best we could do here.

[DavidSpickett]

very dependent on debug info for core objective C libraries

Understood. Then add a comment to that effect.

This makes it clear the test is not checking whether the underlying layers batch reads in some optimal way. Just that it uses the new thing at all.

I want to have a look around lldb for a non-Darwin use for this packet, might be a case where we can expect only one. I'm fine with these tests as they are though.

[DavidSpickett]

LGTM

I'm gonna have a look for other uses for this packet, but none of that blocks this work.