From 5b143ddaad4a8369bd31b78b0a5fbf41140f0288 Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan Date: Tue, 14 Oct 2025 08:07:51 -0700 Subject: [PATCH 1/9] [lldb] Implement Process::ReadMemoryRanges 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> ranges) : ranges(std::move(ranges)) {} llvm::ArrayRef> getRanges() const { return ranges; } private: llvm::SmallVector> 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/ --- lldb/include/lldb/Target/Process.h | 19 ++++++ .../AppleObjCClassDescriptorV2.cpp | 23 +++---- lldb/source/Target/Process.cpp | 28 +++++++++ lldb/unittests/Target/MemoryTest.cpp | 62 +++++++++++++++++++ 4 files changed, 121 insertions(+), 11 deletions(-) diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h index dc75d98acea70..7a260323b5a3d 100644 --- a/lldb/include/lldb/Target/Process.h +++ b/lldb/include/lldb/Target/Process.h @@ -1571,6 +1571,25 @@ class Process : public std::enable_shared_from_this, 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. + /// + /// \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> + ReadMemoryRanges(llvm::ArrayRef> ranges, + llvm::MutableArrayRef buffer); + /// Read of memory from a process. /// /// This function has the same semantics of ReadMemory except that it diff --git a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCClassDescriptorV2.cpp b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCClassDescriptorV2.cpp index 6d8f41aef1ffc..7ce4cbf4c61a4 100644 --- a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCClassDescriptorV2.cpp +++ b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCClassDescriptorV2.cpp @@ -279,22 +279,23 @@ ClassDescriptorV2::ReadMethods(llvm::ArrayRef addresses, const size_t num_methods = addresses.size(); llvm::SmallVector buffer(num_methods * size, 0); - llvm::DenseSet 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> mem_ranges = + llvm::to_vector(llvm::map_range(llvm::seq(num_methods), [&](size_t idx) { + return Range(addresses[idx], size); + })); + + llvm::SmallVector> read_results = + process->ReadMemoryRanges(mem_ranges, buffer); llvm::SmallVector 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, diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index 3176852f0b724..9692a7fece7e5 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -1971,6 +1971,34 @@ size_t Process::ReadMemory(addr_t addr, void *buf, size_t size, Status &error) { } } +llvm::SmallVector> +Process::ReadMemoryRanges(llvm::ArrayRef> ranges, + llvm::MutableArrayRef buffer) { + llvm::SmallVector> results; + + for (auto [addr, len] : ranges) { + // This is either a programmer error, or a protocol violation. + // In production builds, gracefully fail. + assert(buffer.size() >= len); + if (buffer.size() < len) { + results.push_back(buffer.take_front(0)); + continue; + } + + Status status; + size_t num_bytes_read = + ReadMemoryFromInferior(addr, buffer.data(), 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; + results.push_back(buffer.take_front(num_bytes_read)); + 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, diff --git a/lldb/unittests/Target/MemoryTest.cpp b/lldb/unittests/Target/MemoryTest.cpp index 4a96730e00464..3bf7b75728c65 100644 --- a/lldb/unittests/Target/MemoryTest.cpp +++ b/lldb/unittests/Target/MemoryTest.cpp @@ -17,6 +17,8 @@ #include "lldb/Utility/ArchSpec.h" #include "lldb/Utility/DataBufferHeap.h" #include "gtest/gtest.h" +#include +#include using namespace lldb_private; using namespace lldb; @@ -225,3 +227,63 @@ TEST_F(MemoryTest, TesetMemoryCacheRead) { // instead of using an // old cache } + +/// A process class that reads `lower_byte(address)` for each `address` it +/// reads. +class DummyReaderProcess : public Process { +public: + size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size, + Status &error) override { + uint8_t *buffer = static_cast(buf); + for (size_t addr = vm_addr; addr < vm_addr + size; addr++) + buffer[addr - vm_addr] = 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(target_sp, listener_sp); + ASSERT_TRUE(process_sp); + + DummyProcess *process = static_cast(process_sp.get()); + process->SetMaxReadSize(1024); + + llvm::SmallVector buffer(1024, 0); + + // Read 8 ranges of 128 bytes, starting at random addresses + std::mt19937 rng(42); + std::uniform_int_distribution distribution(1, 100000); + llvm::SmallVector> ranges; + for (unsigned i = 0; i < 1024; i += 128) + ranges.emplace_back(distribution(rng), 128); + + llvm::SmallVector> read_results = + process->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(range_base + idx)); + } +} From 58d7eda862c0219e4798a00f4834ad9ce6e5eb2c Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan Date: Mon, 20 Oct 2025 09:52:06 -0700 Subject: [PATCH 2/9] fixup! Address review comments --- lldb/include/lldb/Target/Process.h | 3 +++ lldb/source/Target/Process.cpp | 31 +++++++++++++++++++--------- lldb/unittests/Target/MemoryTest.cpp | 18 +++++++--------- 3 files changed, 32 insertions(+), 20 deletions(-) diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h index 7a260323b5a3d..8f5892e16cedf 100644 --- a/lldb/include/lldb/Target/Process.h +++ b/lldb/include/lldb/Target/Process.h @@ -1572,6 +1572,9 @@ class Process : public std::enable_shared_from_this, 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. diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index 9692a7fece7e5..e4375d4d2b50e 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -1974,25 +1974,36 @@ size_t Process::ReadMemory(addr_t addr, void *buf, size_t size, Status &error) { llvm::SmallVector> Process::ReadMemoryRanges(llvm::ArrayRef> ranges, llvm::MutableArrayRef buffer) { - llvm::SmallVector> results; + auto total_ranges_len = llvm::sum_of( + llvm::map_range(ranges, [](auto range) { return range.size; })); + // If the buffer is not large enough, this is a programmer error. + // In production builds, gracefully fail by returning empty chunks. + assert(buffer.size() >= total_ranges_len); + if (buffer.size() < total_ranges_len) + return llvm::SmallVector>(ranges.size()); - for (auto [addr, len] : ranges) { - // This is either a programmer error, or a protocol violation. - // In production builds, gracefully fail. - assert(buffer.size() >= len); - if (buffer.size() < len) { - results.push_back(buffer.take_front(0)); - continue; - } + llvm::SmallVector> results; + // While `buffer` has space, take the next requested range and read + // memory into a `buffer` chunk, then slice it to remove the used chunk. + for (auto [addr, range_len] : ranges) { Status status; size_t num_bytes_read = - ReadMemoryFromInferior(addr, buffer.data(), len, status); + 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 an empty chunk. + results.emplace_back(); + continue; + } + results.push_back(buffer.take_front(num_bytes_read)); + // Slice buffer to remove the used chunk. buffer = buffer.drop_front(num_bytes_read); } diff --git a/lldb/unittests/Target/MemoryTest.cpp b/lldb/unittests/Target/MemoryTest.cpp index 3bf7b75728c65..2543376897fc4 100644 --- a/lldb/unittests/Target/MemoryTest.cpp +++ b/lldb/unittests/Target/MemoryTest.cpp @@ -18,7 +18,6 @@ #include "lldb/Utility/DataBufferHeap.h" #include "gtest/gtest.h" #include -#include using namespace lldb_private; using namespace lldb; @@ -228,15 +227,15 @@ TEST_F(MemoryTest, TesetMemoryCacheRead) { // old cache } -/// A process class that reads `lower_byte(address)` for each `address` it -/// reads. +/// 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: size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size, Status &error) override { uint8_t *buffer = static_cast(buf); for (size_t addr = vm_addr; addr < vm_addr + size; addr++) - buffer[addr - vm_addr] = addr; + buffer[addr - vm_addr] = static_cast(addr); // LSB of addr. return size; } // Boilerplate, nothing interesting below. @@ -270,12 +269,11 @@ TEST_F(MemoryTest, TestReadMemoryRanges) { llvm::SmallVector buffer(1024, 0); - // Read 8 ranges of 128 bytes, starting at random addresses - std::mt19937 rng(42); - std::uniform_int_distribution distribution(1, 100000); - llvm::SmallVector> ranges; - for (unsigned i = 0; i < 1024; i += 128) - ranges.emplace_back(distribution(rng), 128); + // Read 8 ranges of 128 bytes with arbitrary base addresses. + llvm::SmallVector> ranges = { + {0x12345, 128}, {0x11112222, 128}, {0x77777777, 128}, + {0xffaabbccdd, 128}, {0x0, 128}, {0x4242424242, 128}, + {0x17171717, 128}, {0x99999, 128}}; llvm::SmallVector> read_results = process->ReadMemoryRanges(ranges, buffer); From 1bcac3440f8f31f1e9dd4e0d6801dc485204e535 Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan Date: Tue, 21 Oct 2025 07:19:16 -0700 Subject: [PATCH 3/9] fixup! Remove dead code from test --- lldb/unittests/Target/MemoryTest.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lldb/unittests/Target/MemoryTest.cpp b/lldb/unittests/Target/MemoryTest.cpp index 2543376897fc4..390b07d718fab 100644 --- a/lldb/unittests/Target/MemoryTest.cpp +++ b/lldb/unittests/Target/MemoryTest.cpp @@ -264,9 +264,6 @@ TEST_F(MemoryTest, TestReadMemoryRanges) { std::make_shared(target_sp, listener_sp); ASSERT_TRUE(process_sp); - DummyProcess *process = static_cast(process_sp.get()); - process->SetMaxReadSize(1024); - llvm::SmallVector buffer(1024, 0); // Read 8 ranges of 128 bytes with arbitrary base addresses. @@ -276,7 +273,7 @@ TEST_F(MemoryTest, TestReadMemoryRanges) { {0x17171717, 128}, {0x99999, 128}}; llvm::SmallVector> read_results = - process->ReadMemoryRanges(ranges, buffer); + process_sp->ReadMemoryRanges(ranges, buffer); for (auto [range, memory] : llvm::zip(ranges, read_results)) { ASSERT_EQ(memory.size(), 128u); From 52aae17338cd97d094cb7885b0ba92dc102737ee Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan Date: Tue, 21 Oct 2025 07:41:48 -0700 Subject: [PATCH 4/9] fixup! Add test for reading fewer bytes than requested --- lldb/unittests/Target/MemoryTest.cpp | 52 +++++++++++++++++++++------- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/lldb/unittests/Target/MemoryTest.cpp b/lldb/unittests/Target/MemoryTest.cpp index 390b07d718fab..f4d132dba2978 100644 --- a/lldb/unittests/Target/MemoryTest.cpp +++ b/lldb/unittests/Target/MemoryTest.cpp @@ -231,8 +231,16 @@ TEST_F(MemoryTest, TesetMemoryCacheRead) { /// 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; + size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size, Status &error) override { + if (read_less_than_requested && size > 0) + size--; uint8_t *buffer = static_cast(buf); for (size_t addr = vm_addr; addr < vm_addr + size; addr++) buffer[addr - vm_addr] = static_cast(addr); // LSB of addr. @@ -264,21 +272,39 @@ TEST_F(MemoryTest, TestReadMemoryRanges) { std::make_shared(target_sp, listener_sp); ASSERT_TRUE(process_sp); - llvm::SmallVector buffer(1024, 0); - // Read 8 ranges of 128 bytes with arbitrary base addresses. - llvm::SmallVector> ranges = { - {0x12345, 128}, {0x11112222, 128}, {0x77777777, 128}, - {0xffaabbccdd, 128}, {0x0, 128}, {0x4242424242, 128}, - {0x17171717, 128}, {0x99999, 128}}; + { + llvm::SmallVector buffer(1024, 0); + // Read 8 ranges of 128 bytes with arbitrary base addresses. + llvm::SmallVector> ranges = { + {0x12345, 128}, {0x11112222, 128}, {0x77777777, 128}, + {0xffaabbccdd, 128}, {0x0, 128}, {0x4242424242, 128}, + {0x17171717, 128}, {0x99999, 128}}; - llvm::SmallVector> read_results = - process_sp->ReadMemoryRanges(ranges, buffer); + llvm::SmallVector> 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(range_base + idx)); + 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(range_base + idx)); + } + } + + auto &dummy_process = static_cast(*process_sp); + dummy_process.read_less_than_requested = true; + { + llvm::SmallVector buffer(1024, 0); + llvm::SmallVector> ranges = { + {0x12345, 128}, {0x11112222, 128}, {0x77777777, 128}}; + llvm::SmallVector> 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(range_base + idx)); + } } } From d498717af6913812e30051f9b6eebf47b9330782 Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan Date: Tue, 21 Oct 2025 07:53:30 -0700 Subject: [PATCH 5/9] fixup! rewrite empty response for clarity --- lldb/source/Target/Process.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index e4375d4d2b50e..bb0b553a9d898 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -1979,8 +1979,10 @@ Process::ReadMemoryRanges(llvm::ArrayRef> ranges, // If the buffer is not large enough, this is a programmer error. // In production builds, gracefully fail by returning empty chunks. assert(buffer.size() >= total_ranges_len); - if (buffer.size() < total_ranges_len) - return llvm::SmallVector>(ranges.size()); + if (buffer.size() < total_ranges_len) { + llvm::MutableArrayRef empty; + return {ranges.size(), empty}; + } llvm::SmallVector> results; From 1b470fbba124383337b33d007e20cfe25e67e35d Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan Date: Tue, 21 Oct 2025 08:09:12 -0700 Subject: [PATCH 6/9] fixup! add death tests --- lldb/source/Target/Process.cpp | 2 +- lldb/unittests/Target/MemoryTest.cpp | 45 ++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index bb0b553a9d898..b749601552cc2 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -1978,7 +1978,7 @@ Process::ReadMemoryRanges(llvm::ArrayRef> ranges, llvm::map_range(ranges, [](auto range) { return range.size; })); // If the buffer is not large enough, this is a programmer error. // In production builds, gracefully fail by returning empty chunks. - assert(buffer.size() >= total_ranges_len); + assert(buffer.size() >= total_ranges_len && "provided buffer is too short"); if (buffer.size() < total_ranges_len) { llvm::MutableArrayRef empty; return {ranges.size(), empty}; diff --git a/lldb/unittests/Target/MemoryTest.cpp b/lldb/unittests/Target/MemoryTest.cpp index f4d132dba2978..1380503362a5d 100644 --- a/lldb/unittests/Target/MemoryTest.cpp +++ b/lldb/unittests/Target/MemoryTest.cpp @@ -236,11 +236,14 @@ class DummyReaderProcess : public Process { // 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(buf); for (size_t addr = vm_addr; addr < vm_addr + size; addr++) buffer[addr - vm_addr] = static_cast(addr); // LSB of addr. @@ -308,3 +311,45 @@ TEST_F(MemoryTest, TestReadMemoryRanges) { } } } + +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(target_sp, listener_sp); + ASSERT_TRUE(process_sp); + + auto &dummy_process = static_cast(*process_sp); + dummy_process.read_more_than_requested = true; + llvm::SmallVector buffer(1024, 0); + llvm::SmallVector> ranges = {{0x12345, 128}}; + ASSERT_DEATH( + { process_sp->ReadMemoryRanges(ranges, buffer); }, + "read more than requested bytes"); +} + +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(target_sp, listener_sp); + ASSERT_TRUE(process_sp); + + llvm::SmallVector too_short_buffer(10, 0); + llvm::SmallVector> ranges = {{0x12345, 128}}; + ASSERT_DEATH( + { process_sp->ReadMemoryRanges(ranges, too_short_buffer); }, + "provided buffer is too short"); +} From c1434e468ca13b32084371fba00ceca51e906ff3 Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan Date: Tue, 21 Oct 2025 08:12:05 -0700 Subject: [PATCH 7/9] fixup! remove chunk references --- lldb/source/Target/Process.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index b749601552cc2..fb9e7eb5ed1bd 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -1977,7 +1977,8 @@ Process::ReadMemoryRanges(llvm::ArrayRef> ranges, auto total_ranges_len = llvm::sum_of( llvm::map_range(ranges, [](auto range) { return range.size; })); // If the buffer is not large enough, this is a programmer error. - // In production builds, gracefully fail by returning empty chunks. + // 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 empty; @@ -1987,7 +1988,7 @@ Process::ReadMemoryRanges(llvm::ArrayRef> ranges, llvm::SmallVector> results; // While `buffer` has space, take the next requested range and read - // memory into a `buffer` chunk, then slice it to remove the used chunk. + // 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 = @@ -1999,13 +2000,14 @@ Process::ReadMemoryRanges(llvm::ArrayRef> ranges, assert(num_bytes_read <= range_len && "read more than requested bytes"); if (num_bytes_read > range_len) { - // In production builds, gracefully fail by returning an empty chunk. + // 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 chunk. + // Slice buffer to remove the used memory. buffer = buffer.drop_front(num_bytes_read); } From 6e13b809feebb3794f5324ade75c344b8fb302ba Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan Date: Tue, 21 Oct 2025 08:15:32 -0700 Subject: [PATCH 8/9] fixup! clang-format --- lldb/unittests/Target/MemoryTest.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lldb/unittests/Target/MemoryTest.cpp b/lldb/unittests/Target/MemoryTest.cpp index 1380503362a5d..a236afe0df6a5 100644 --- a/lldb/unittests/Target/MemoryTest.cpp +++ b/lldb/unittests/Target/MemoryTest.cpp @@ -275,7 +275,6 @@ TEST_F(MemoryTest, TestReadMemoryRanges) { std::make_shared(target_sp, listener_sp); ASSERT_TRUE(process_sp); - { llvm::SmallVector buffer(1024, 0); // Read 8 ranges of 128 bytes with arbitrary base addresses. @@ -295,7 +294,7 @@ TEST_F(MemoryTest, TestReadMemoryRanges) { } } - auto &dummy_process = static_cast(*process_sp); + auto &dummy_process = static_cast(*process_sp); dummy_process.read_less_than_requested = true; { llvm::SmallVector buffer(1024, 0); From f5e435877db61909744999b811fa95d94b3a3d3f Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan Date: Tue, 21 Oct 2025 10:43:19 -0700 Subject: [PATCH 9/9] fixup! use ASSERT_DEBUG_DEATH --- lldb/unittests/Target/MemoryTest.cpp | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/lldb/unittests/Target/MemoryTest.cpp b/lldb/unittests/Target/MemoryTest.cpp index a236afe0df6a5..f7b4e97b1f64a 100644 --- a/lldb/unittests/Target/MemoryTest.cpp +++ b/lldb/unittests/Target/MemoryTest.cpp @@ -329,9 +329,16 @@ TEST_F(MemoryDeathTest, TestReadMemoryRangesReturnsTooMuch) { dummy_process.read_more_than_requested = true; llvm::SmallVector buffer(1024, 0); llvm::SmallVector> ranges = {{0x12345, 128}}; - ASSERT_DEATH( - { process_sp->ReadMemoryRanges(ranges, buffer); }, + + llvm::SmallVector> 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) { @@ -346,9 +353,17 @@ TEST_F(MemoryDeathTest, TestReadMemoryRangesWithShortBuffer) { std::make_shared(target_sp, listener_sp); ASSERT_TRUE(process_sp); - llvm::SmallVector too_short_buffer(10, 0); - llvm::SmallVector> ranges = {{0x12345, 128}}; - ASSERT_DEATH( - { process_sp->ReadMemoryRanges(ranges, too_short_buffer); }, + llvm::SmallVector short_buffer(10, 0); + llvm::SmallVector> ranges = {{0x12345, 128}, + {0x11, 128}}; + llvm::SmallVector> 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 result : read_results) + ASSERT_TRUE(result.empty()); +#endif }