[lldb] Respect max packet size limits for MultiMemRead in ProcessGDBRemote

[felipepiovezan]

(ignore the first commit, it is the dependency of this PR)

Servers advertise what their maximum packet size is, MultiMemRead needs
to respect that.

Depends on:

• [lldb][nfc] Change ProcessGDBRemote::ParseMultiMemReadPacket signature #172020

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

(ignore the first commit, it is the dependency of this PR)

Servers advertise what their maximum packet size is, MultiMemRead needs
to respect that.

Depends on #172020

---

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

2 Files Affected:

• (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+55-25)
• (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h (+4-4)

diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index b976b53c035b0..45d3b111ed194 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -2790,6 +2790,23 @@ size_t ProcessGDBRemote::DoReadMemory(addr_t addr, void *buf, size_t size,
   return 0;
 }
 
+/// Returns the number of ranges that is safe to request using MultiMemRead
+/// while respecting max_packet_size.
+static uint64_t ComputeNumRangesMultiMemRead(
+    uint64_t max_packet_size,
+    llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges) {
+  // Each range is specified by two numbers (~16 ASCII characters) and one
+  // comma.
+  constexpr uint64_t range_overhead = 33;
+  uint64_t current_size = 0;
+  for (auto [idx, range] : llvm::enumerate(ranges)) {
+    uint64_t potential_size = current_size + range.size + range_overhead;
+    if (potential_size > max_packet_size)
+      return idx;
+  }
+  return ranges.size();
+}
+
 llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>
 ProcessGDBRemote::ReadMemoryRanges(
     llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges,
@@ -2797,25 +2814,40 @@ ProcessGDBRemote::ReadMemoryRanges(
   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);
-  }
+  const llvm::ArrayRef<Range<lldb::addr_t, size_t>> original_ranges = ranges;
+  llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> memory_regions;
 
-  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);
+  while (!ranges.empty()) {
+    uint64_t num_ranges =
+        ComputeNumRangesMultiMemRead(m_max_memory_size, ranges);
+    if (num_ranges == 0) {
+      LLDB_LOG(
+          GetLog(GDBRLog::Process),
+          "MultiMemRead has a range bigger than maximum allowed by remote");
+      return Process::ReadMemoryRanges(original_ranges, buffer);
+    }
+
+    auto ranges_for_request = ranges.take_front(num_ranges);
+    ranges = ranges.drop_front(num_ranges);
+
+    llvm::Expected<StringExtractorGDBRemote> response =
+        SendMultiMemReadPacket(ranges_for_request);
+    if (!response) {
+      LLDB_LOG_ERROR(GetLog(GDBRLog::Process), response.takeError(),
+                     "MultiMemRead error response: {0}");
+      return Process::ReadMemoryRanges(original_ranges, buffer);
+    }
+
+    llvm::StringRef response_str = response->GetStringRef();
+    const unsigned expected_num_ranges = ranges_for_request.size();
+    if (llvm::Error error = ParseMultiMemReadPacket(
+            response_str, buffer, expected_num_ranges, memory_regions)) {
+      LLDB_LOG_ERROR(GetLog(GDBRLog::Process), std::move(error),
+                     "MultiMemRead error parsing response: {0}");
+      return Process::ReadMemoryRanges(original_ranges, buffer);
+    }
   }
-  return std::move(*parsed_response);
+  return memory_regions;
 }
 
 llvm::Expected<StringExtractorGDBRemote>
@@ -2851,10 +2883,10 @@ ProcessGDBRemote::SendMultiMemReadPacket(
   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) {
+llvm::Error ProcessGDBRemote::ParseMultiMemReadPacket(
+    llvm::StringRef response_str, llvm::MutableArrayRef<uint8_t> buffer,
+    unsigned expected_num_ranges,
+    llvm::SmallVectorImpl<llvm::MutableArrayRef<uint8_t>> &memory_regions) {
   // The sizes and the data are separated by a `;`.
   auto [sizes_str, memory_data] = response_str.split(';');
   if (sizes_str.size() == response_str.size())
@@ -2862,8 +2894,6 @@ ProcessGDBRemote::ParseMultiMemReadPacket(llvm::StringRef response_str,
         "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;
@@ -2886,10 +2916,10 @@ ProcessGDBRemote::ParseMultiMemReadPacket(llvm::StringRef response_str,
     buffer = buffer.drop_front(read_size);
 
     memcpy(region_to_write.data(), region_to_read.data(), read_size);
-    read_results.push_back(region_to_write);
+    memory_regions.push_back(region_to_write);
   }
 
-  return read_results;
+  return llvm::Error::success();
 }
 
 bool ProcessGDBRemote::SupportsMemoryTagging() {
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
index b7e8777c9e12e..bfe24926c2bb8 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -147,10 +147,10 @@ class ProcessGDBRemote : public Process,
   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);
+  llvm::Error ParseMultiMemReadPacket(
+      llvm::StringRef response_str, llvm::MutableArrayRef<uint8_t> buffer,
+      unsigned expected_num_ranges,
+      llvm::SmallVectorImpl<llvm::MutableArrayRef<uint8_t>> &parsed_ranges);
 
 public:
   Status

[DavidSpickett]

By ~you mean roughly and by roughly you mean "up to"? (given that the address size is at maximum 64 bits)

[DavidSpickett]

Worst case overhead - in other words.

[felipepiovezan]

yup, I'll update the wording

[DavidSpickett]

If you could log the range itself, that would be great. I think it's easiest to move the log call into the helper.

[jasonmolenda]

looks good to me, David's suggestions seem good.

[felipepiovezan]

Rebased, addressed review comments

[DavidSpickett]

LGTM