https://github.com/felipepiovezan updated https://github.com/llvm/llvm-project/pull/172022
>From 66bd7c47587dde697fdb905c54763c355ba79cc3 Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan <[email protected]> Date: Tue, 9 Dec 2025 11:37:47 +0000 Subject: [PATCH 1/3] [lldb][nfc] Change ProcessGDBRemote::ParseMultiMemReadPacket signature Instead of returning an `Expected<vector<...>>` it now returns an Error, and receives a vector argument to fill in. This will be useful to support a change were ParseMultiMemReadPacket will be called multiple times in a loop with the same vector; without this change, we would have to concatenate vectors and copy memory around. --- .../Process/gdb-remote/ProcessGDBRemote.cpp | 25 ++++++++----------- .../Process/gdb-remote/ProcessGDBRemote.h | 8 +++--- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index b976b53c035b0..d1b0ead74701b 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -2807,15 +2807,14 @@ ProcessGDBRemote::ReadMemoryRanges( 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(), + llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> memory_regions; + 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(ranges, buffer); } - return std::move(*parsed_response); + return memory_regions; } llvm::Expected<StringExtractorGDBRemote> @@ -2851,10 +2850,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 +2861,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 +2883,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 >From 67e0b9b0d1789a3dec67e56832c043d3e5c6e483 Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan <[email protected]> Date: Tue, 9 Dec 2025 14:34:26 +0000 Subject: [PATCH 2/3] [lldb] Respect max packet size limits for MultiMemRead in ProcessGDBRemote Servers advertise what their maximum packet size is, MultiMemRead needs to respect that. Depends on https://github.com/llvm/llvm-project/pull/172020 --- .../Process/gdb-remote/ProcessGDBRemote.cpp | 63 ++++++++++++++----- 1 file changed, 48 insertions(+), 15 deletions(-) diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index d1b0ead74701b..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,22 +2814,38 @@ 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); - } - - llvm::StringRef response_str = response->GetStringRef(); - const unsigned expected_num_ranges = ranges.size(); + const llvm::ArrayRef<Range<lldb::addr_t, size_t>> original_ranges = ranges; llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> memory_regions; - 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(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 memory_regions; } >From 837956bd837555a4f60435d3373c3749c3ce9e87 Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan <[email protected]> Date: Sun, 14 Dec 2025 10:39:10 +0000 Subject: [PATCH 3/3] fixup! address review comments --- .../Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index 45d3b111ed194..5ad4add135d6f 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -2795,14 +2795,20 @@ size_t ProcessGDBRemote::DoReadMemory(addr_t addr, void *buf, size_t 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 + // Each range is specified by two numbers (up to 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) + if (potential_size > max_packet_size) { + if (idx == 0) + LLDB_LOG(GetLog(GDBRLog::Process), + "MultiMemRead input has a range (base = {0:x}, size = {1}) " + "bigger than the maximum allowed by remote", + range.base, range.size); return idx; + } } return ranges.size(); } @@ -2821,9 +2827,6 @@ ProcessGDBRemote::ReadMemoryRanges( 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); } _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
