Skip to content

[lldb] Use MemoryCache in Process::ReadRangesFromMemory #201166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
felipepiovezan merged 3 commits into from
Jun 4, 2026
Merged

[lldb] Use MemoryCache in Process::ReadRangesFromMemory #201166

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
59ab98f
[lldb] Use MemoryCache in Process::ReadRangesFromMemory
felipepiovezan Jun 2, 2026
44770a2
fixup! dont reuse address is tesst
felipepiovezan Jun 3, 2026
4028ab8
fixup! avoid find_if
felipepiovezan Jun 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions lldb/include/lldb/Target/Memory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

#include "lldb/Utility/RangeMap.h"
#include "lldb/lldb-private.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallVector.h"
#include <map>
#include <mutex>
#include <vector>
Expand All @@ -31,6 +33,13 @@ class MemoryCache {

size_t Read(lldb::addr_t addr, void *dst, size_t dst_len, Status &error);

/// Reads multiple memory ranges, serving cache hits from L1 and batching all
/// misses through Process::DoReadMemoryRanges. The semantics of the return
/// value match Process::ReadMemoryRanges.
llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>
ReadRanges(llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges,
llvm::MutableArrayRef<uint8_t> buffer);

uint32_t GetMemoryCacheLineSize() const { return m_L2_cache_line_byte_size; }

void AddInvalidRange(lldb::addr_t base_addr, lldb::addr_t byte_size);
Expand All @@ -40,6 +49,11 @@ class MemoryCache {
// Allow external sources to populate data into the L1 memory cache
void AddL1CacheData(lldb::addr_t addr, const void *src, size_t src_len);

void AddL1CacheData(lldb::addr_t addr, llvm::ArrayRef<uint8_t> src) {
if (!src.empty())
AddL1CacheData(addr, src.data(), src.size());
}

void AddL1CacheData(lldb::addr_t addr,
const lldb::DataBufferSP &data_buffer_sp);

Expand Down
1 change: 1 addition & 0 deletions lldb/include/lldb/Target/Process.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,7 @@ class Process : public std::enable_shared_from_this<Process>,
friend class StopInfo;
friend class Target;
friend class ThreadList;
friend class MemoryCache;

public:
/// Broadcaster event bits definitions.
Expand Down
53 changes: 53 additions & 0 deletions lldb/source/Target/Memory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
#include "lldb/Utility/RangeMap.h"
#include "lldb/Utility/State.h"

#include "llvm/ADT/STLExtras.h"

#include <cinttypes>
#include <memory>

Expand Down Expand Up @@ -270,6 +272,57 @@ size_t MemoryCache::Read(addr_t addr, void *dst, size_t dst_len,
return dst_len;
}

llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>
MemoryCache::ReadRanges(llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges,
llvm::MutableArrayRef<uint8_t> buffer) {
std::lock_guard<std::recursive_mutex> guard(m_mutex);

llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> results;
results.reserve(ranges.size());
llvm::SmallVector<Range<lldb::addr_t, size_t>> missed_ranges;

// Iterate once serving requests from L1.
for (auto range : ranges) {
const lldb::addr_t addr = range.GetRangeBase();
const size_t len = range.GetByteSize();

if (m_invalid_ranges.FindEntryThatContains(addr)) {
results.push_back(buffer.take_front(0));
continue;
}

if (const uint8_t *l1_data = FindL1CacheEntry(addr, len)) {
results.push_back(buffer.take_front(len));
buffer = buffer.drop_front(len);
memcpy(results.back().data(), l1_data, len);
continue;
}

// Use a nullptr to denote this needs fetching.
results.emplace_back(nullptr, nullptr);
missed_ranges.push_back(range);
}

if (missed_ranges.empty())
return results;

auto fetched_buffers = m_process.DoReadMemoryRanges(missed_ranges, buffer);

for (auto [missed_range, fetched] : llvm::zip(missed_ranges, fetched_buffers))
AddL1CacheData(missed_range.GetRangeBase(), fetched);

auto *results_it = results.begin();
auto *end = results.end();
for (auto fetched : fetched_buffers) {
results_it = std::find_if(

@bulbazord bulbazord Jun 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of iterating via find_if, would it be worth keeping a list of indices for missed ranges? Then you could fill them in directly instead of needing to walk the results.

@felipepiovezan felipepiovezan Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but felt this code was simple enough that we could avoid the extra allocation. I can give it a try if you feel the extra vector would make the code simpler

@bulbazord bulbazord Jun 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it would be simpler. It could look like:

for (auto [fetched_idx, missing_range_idx] : llvm::enumerate(missing_range_indices)) {
  results[missing_range_idx] = fetched_buffers[fetched_idx];
}

Ultimately I find both acceptable, so I'll leave this choice up to you.

@felipepiovezan felipepiovezan Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this a try, but I was not the biggest fan of how this makes the previous loop a bit more complicated (have to enumerate the array just to have an index that is used at the very end of the loop, plus the extra vector).

Instead, I think I found a middle ground that removes the find_if, and removes the need for an extra vector:

  for (auto &result : results)
    if (result.data() == nullptr)
      result = fetched_buffers.consume_front();

@felipepiovezan felipepiovezan Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at the top commit for the precise diff!

results_it, end, [](auto result) { return result.data() == nullptr; });
assert(results_it != end);
*results_it = fetched;
}

return results;
}

AllocatedBlock::AllocatedBlock(lldb::addr_t addr, uint32_t byte_size,
uint32_t permissions, uint32_t chunk_size)
: m_range(addr, byte_size), m_permissions(permissions),
Expand Down
2 changes: 2 additions & 0 deletions lldb/source/Target/Process.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2077,6 +2077,8 @@ Process::ReadMemoryRanges(llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges,
for (const Range<lldb::addr_t, size_t> &range : ranges)
fixed_ranges.emplace_back(FixAnyAddress(range.GetRangeBase()),
range.GetByteSize());
if (!GetDisableMemoryCache())
return m_memory_cache.ReadRanges(fixed_ranges, buffer);
return DoReadMemoryRanges(fixed_ranges, buffer);
}

Expand Down
Loading