jarin updated this revision to Diff 256207.
jarin marked 10 inline comments as done.
jarin added a comment.
Addressed some of the reviewer comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77765/new/
https://reviews.llvm.org/D77765
Files:
lldb/include/lldb/Target/Memory.h
lldb/include/lldb/Target/Process.h
lldb/source/Target/Memory.cpp
lldb/source/Target/Process.cpp
lldb/unittests/Target/CMakeLists.txt
lldb/unittests/Target/MemoryCacheTest.cpp
Index: lldb/unittests/Target/MemoryCacheTest.cpp
===================================================================
--- /dev/null
+++ lldb/unittests/Target/MemoryCacheTest.cpp
@@ -0,0 +1,128 @@
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Target/Memory.h"
+#include "lldb/Utility/Status.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+namespace {
+
+class MemoryCacheTest : public MemoryFromInferiorReader, public testing::Test {
+ static const size_t k_memory_size = 256;
+ static const uint64_t k_cache_line_size = 16;
+
+ size_t ReadMemoryFromInferior(lldb::addr_t vm_addr, void *buf, size_t size,
+ Status &error) override {
+ m_inferior_read_count++;
+
+ if (vm_addr >= m_memory.size())
+ return 0;
+
+ size = std::min(size, m_memory.size() - vm_addr);
+ memcpy(buf, m_memory.data() + vm_addr, size);
+ return size;
+ }
+
+public:
+ MemoryCacheTest()
+ : m_cache(*this, k_cache_line_size), m_inferior_read_count(0) {
+ // Fill memory from [0x0 - 0x256) with byte values that match the index. We
+ // will use this memory in each test and each test will start with a fresh
+ // copy.
+ for (size_t i = 0; i < k_memory_size; i++)
+ m_memory.push_back(static_cast<uint8_t>(i));
+ }
+
+ void AddRangeToL1Cache(lldb::addr_t addr, size_t len) {
+ m_cache.AddL1CacheData(addr, m_memory.data() + addr, len);
+ }
+
+ void IncrementAndFlushRange(lldb::addr_t addr, size_t len) {
+ for (size_t i = 0; i < len; i++)
+ m_memory[addr + i]++;
+ m_cache.Flush(addr, len);
+ }
+
+ uint8_t ReadByte(lldb::addr_t addr) {
+ uint8_t result;
+ Status error;
+ m_cache.Read(addr, &result, 1, error);
+ EXPECT_TRUE(error.Success());
+ return result;
+ }
+
+protected:
+ MemoryCache m_cache;
+ std::vector<uint8_t> m_memory;
+ size_t m_inferior_read_count;
+};
+
+TEST_F(MemoryCacheTest, L1FlushSimple) {
+ // Add a byte to the cache.
+ AddRangeToL1Cache(10, 1);
+
+ // Sanity check - memory should agree with what ReadByte returns.
+ EXPECT_EQ(10, m_memory[10]);
+ EXPECT_EQ(m_memory[10], ReadByte(10));
+ // Check that we have hit the cache and not the inferior's memory.
+ EXPECT_EQ(0u, m_inferior_read_count);
+
+ IncrementAndFlushRange(10, 1);
+
+ // Check the memory is incremented and the real memory agrees with ReadByte's
+ // result.
+ EXPECT_EQ(11, m_memory[11]);
+ EXPECT_EQ(m_memory[10], ReadByte(10));
+}
+
+// Let us do a parametrized test that caches two ranges. In the test, we then
+// try to modify (and flush) various regions of memory and check that the cache
+// still returns the correct values from reads.
+using AddrRange = Range<lldb::addr_t, lldb::addr_t>;
+class MemoryCacheParametrizedTest
+ : public MemoryCacheTest,
+ public testing::WithParamInterface<AddrRange> {};
+
+TEST_P(MemoryCacheParametrizedTest, L1FlushWithTwoRegions) {
+ // Add two ranges to the cache.
+ std::vector<AddrRange> cached = {{10, 10}, {30, 10}};
+ for (AddrRange cached_range : cached)
+ AddRangeToL1Cache(cached_range.base, cached_range.size);
+
+ // Flush the range given by the parameter.
+ AddrRange flush_range = GetParam();
+ IncrementAndFlushRange(flush_range.base, flush_range.size);
+
+ // Check that reads from the cached ranges read the inferior's memory
+ // if and only if the flush range intersects with one of the cached ranges.
+ bool has_intersection = false;
+ for (AddrRange cached_range : cached) {
+ if (flush_range.DoesIntersect(cached_range))
+ has_intersection = true;
+
+ for (size_t i = 0; i < cached_range.size; i++)
+ ReadByte(cached_range.base + i);
+ }
+ EXPECT_EQ(has_intersection, m_inferior_read_count > 0);
+
+ // Sanity check: check the memory contents.
+ for (size_t i = 0; i < 50; i++) {
+ EXPECT_EQ(m_memory[i], ReadByte(i)) << " (element at addr=" << i << ")";
+ }
+}
+
+INSTANTIATE_TEST_CASE_P(
+ AllMemoryCacheParametrizedTest, MemoryCacheParametrizedTest,
+ ::testing::Values(AddrRange(5, 6), AddrRange(5, 10), AddrRange(5, 20),
+ AddrRange(5, 30), AddrRange(5, 40), AddrRange(10, 1),
+ AddrRange(10, 21), AddrRange(19, 1), AddrRange(19, 11),
+ AddrRange(19, 12), AddrRange(20, 11), AddrRange(20, 25),
+ AddrRange(29, 2), AddrRange(29, 12), AddrRange(30, 1),
+ AddrRange(39, 1), AddrRange(39, 5), AddrRange(5, 1),
+ AddrRange(5, 5), AddrRange(9, 1), AddrRange(20, 1),
+ AddrRange(20, 10), AddrRange(29, 1), AddrRange(40, 1),
+ AddrRange(40, 10)));
+
+} // namespace
Index: lldb/unittests/Target/CMakeLists.txt
===================================================================
--- lldb/unittests/Target/CMakeLists.txt
+++ lldb/unittests/Target/CMakeLists.txt
@@ -1,6 +1,7 @@
add_lldb_unittest(TargetTests
ABITest.cpp
ExecutionContextTest.cpp
+ MemoryCacheTest.cpp
MemoryRegionInfoTest.cpp
ModuleCacheTest.cpp
PathMappingListTest.cpp
Index: lldb/source/Target/Process.cpp
===================================================================
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -485,9 +485,10 @@
m_stdio_communication("process.stdio"), m_stdio_communication_mutex(),
m_stdin_forward(false), m_stdout_data(), m_stderr_data(),
m_profile_data_comm_mutex(), m_profile_data(), m_iohandler_sync(0),
- m_memory_cache(*this), m_allocated_memory_cache(*this),
- m_should_detach(false), m_next_event_action_up(), m_public_run_lock(),
- m_private_run_lock(), m_finalizing(false), m_finalize_called(false),
+ m_memory_cache(*this, GetMemoryCacheLineSize()),
+ m_allocated_memory_cache(*this), m_should_detach(false),
+ m_next_event_action_up(), m_public_run_lock(), m_private_run_lock(),
+ m_finalizing(false), m_finalize_called(false),
m_clear_thread_plans_on_stop(false), m_force_next_event_delivery(false),
m_last_broadcast_state(eStateInvalid), m_destroy_in_process(false),
m_can_interpret_function_calls(false), m_warnings_issued(),
@@ -608,7 +609,7 @@
std::vector<Notifications> empty_notifications;
m_notifications.swap(empty_notifications);
m_image_tokens.clear();
- m_memory_cache.Clear();
+ m_memory_cache.Clear(GetMemoryCacheLineSize());
m_allocated_memory_cache.Clear();
{
std::lock_guard<std::recursive_mutex> guard(m_language_runtimes_mutex);
@@ -1452,7 +1453,7 @@
m_mod_id.BumpStopID();
if (!m_mod_id.IsLastResumeForUserExpression())
m_mod_id.SetStopEventForLastNaturalStopID(event_sp);
- m_memory_cache.Clear();
+ m_memory_cache.Clear(GetMemoryCacheLineSize());
LLDB_LOGF(log, "Process::SetPrivateState (%s) stop_id = %u",
StateAsCString(new_state), m_mod_id.GetStopID());
}
@@ -5571,7 +5572,7 @@
}
m_instrumentation_runtimes.clear();
m_thread_list.DiscardThreadPlans();
- m_memory_cache.Clear(true);
+ m_memory_cache.Clear(GetMemoryCacheLineSize(), true);
DoDidExec();
CompleteAttach();
// Flush the process (threads and all stack frames) after running
Index: lldb/source/Target/Memory.cpp
===================================================================
--- lldb/source/Target/Memory.cpp
+++ lldb/source/Target/Memory.cpp
@@ -20,21 +20,21 @@
using namespace lldb_private;
// MemoryCache constructor
-MemoryCache::MemoryCache(Process &process)
+MemoryCache::MemoryCache(MemoryFromInferiorReader &reader,
+ uint64_t cache_line_size)
: m_mutex(), m_L1_cache(), m_L2_cache(), m_invalid_ranges(),
- m_process(process),
- m_L2_cache_line_byte_size(process.GetMemoryCacheLineSize()) {}
+ m_reader(reader), m_L2_cache_line_byte_size(cache_line_size) {}
// Destructor
MemoryCache::~MemoryCache() {}
-void MemoryCache::Clear(bool clear_invalid_ranges) {
+void MemoryCache::Clear(uint64_t cache_line_size, bool clear_invalid_ranges) {
std::lock_guard<std::recursive_mutex> guard(m_mutex);
m_L1_cache.clear();
m_L2_cache.clear();
if (clear_invalid_ranges)
m_invalid_ranges.Clear();
- m_L2_cache_line_byte_size = m_process.GetMemoryCacheLineSize();
+ m_L2_cache_line_byte_size = cache_line_size;
}
void MemoryCache::AddL1CacheData(lldb::addr_t addr, const void *src,
@@ -60,7 +60,13 @@
AddrRange flush_range(addr, size);
BlockMap::iterator pos = m_L1_cache.upper_bound(addr);
if (pos != m_L1_cache.begin()) {
- --pos;
+ // If we are not in the beginning, the previous range might be
+ // intersecting.
+ BlockMap::iterator previous = pos;
+ previous--;
+ AddrRange chunk_range(previous->first, previous->second->GetByteSize());
+ if (chunk_range.DoesIntersect(flush_range))
+ m_L1_cache.erase(previous);
}
while (pos != m_L1_cache.end()) {
AddrRange chunk_range(pos->first, pos->second->GetByteSize());
@@ -157,7 +163,7 @@
// it in the cache.
if (dst && dst_len > m_L2_cache_line_byte_size) {
size_t bytes_read =
- m_process.ReadMemoryFromInferior(addr, dst, dst_len, error);
+ m_reader.ReadMemoryFromInferior(addr, dst, dst_len, error);
// Add this non block sized range to the L1 cache if we actually read
// anything
if (bytes_read > 0)
@@ -220,24 +226,24 @@
}
}
- // We need to read from the process
+ // We need to read from the inferior
if (bytes_left > 0) {
assert((curr_addr % cache_line_byte_size) == 0);
std::unique_ptr<DataBufferHeap> data_buffer_heap_up(
new DataBufferHeap(cache_line_byte_size, 0));
- size_t process_bytes_read = m_process.ReadMemoryFromInferior(
+ size_t inferior_bytes_read = m_reader.ReadMemoryFromInferior(
curr_addr, data_buffer_heap_up->GetBytes(),
data_buffer_heap_up->GetByteSize(), error);
- if (process_bytes_read == 0)
+ if (inferior_bytes_read == 0)
return dst_len - bytes_left;
- if (process_bytes_read != cache_line_byte_size) {
- if (process_bytes_read < data_buffer_heap_up->GetByteSize()) {
- dst_len -= data_buffer_heap_up->GetByteSize() - process_bytes_read;
- bytes_left = process_bytes_read;
+ if (inferior_bytes_read != cache_line_byte_size) {
+ if (inferior_bytes_read < data_buffer_heap_up->GetByteSize()) {
+ dst_len -= data_buffer_heap_up->GetByteSize() - inferior_bytes_read;
+ bytes_left = inferior_bytes_read;
}
- data_buffer_heap_up->SetByteSize(process_bytes_read);
+ data_buffer_heap_up->SetByteSize(inferior_bytes_read);
}
m_L2_cache[curr_addr] = DataBufferSP(data_buffer_heap_up.release());
// We have read data and put it into the cache, continue through the
Index: lldb/include/lldb/Target/Process.h
===================================================================
--- lldb/include/lldb/Target/Process.h
+++ lldb/include/lldb/Target/Process.h
@@ -355,7 +355,8 @@
public UserID,
public Broadcaster,
public ExecutionContextScope,
- public PluginInterface {
+ public PluginInterface,
+ public MemoryFromInferiorReader {
friend class FunctionCaller; // For WaitForStateChangeEventsPrivate
friend class Debugger; // For PopProcessIOHandler and ProcessIOHandlerIsActive
friend class DynamicLoader; // For LoadOperatingSystemPlugin
@@ -1480,7 +1481,7 @@
/// vm_addr, \a buf, and \a size updated appropriately. Zero is
/// returned in the case of an error.
size_t ReadMemoryFromInferior(lldb::addr_t vm_addr, void *buf, size_t size,
- Status &error);
+ Status &error) override;
/// Read a NULL terminated string from memory
///
Index: lldb/include/lldb/Target/Memory.h
===================================================================
--- lldb/include/lldb/Target/Memory.h
+++ lldb/include/lldb/Target/Memory.h
@@ -16,16 +16,52 @@
#include <vector>
namespace lldb_private {
+
+// Abstraction of a reader from inferior's memory.
+class MemoryFromInferiorReader {
+public:
+ /// Read of memory from a process.
+ ///
+ /// This function has the same semantics of ReadMemory except that it
+ /// bypasses caching.
+ ///
+ /// \param[in] vm_addr
+ /// A virtual load address that indicates where to start reading
+ /// memory from.
+ ///
+ /// \param[out] buf
+ /// A byte buffer that is at least \a size bytes long that
+ /// will receive the memory bytes.
+ ///
+ /// \param[in] size
+ /// The number of bytes to read.
+ ///
+ /// \param[out] error
+ /// An error that indicates the success or failure of this
+ /// operation. If error indicates success (error.Success()),
+ /// then the value returned can be trusted, otherwise zero
+ /// will be returned.
+ ///
+ /// \return
+ /// The number of bytes that were actually read into \a buf. If
+ /// the returned number is greater than zero, yet less than \a
+ /// size, then this function will get called again with \a
+ /// vm_addr, \a buf, and \a size updated appropriately. Zero is
+ /// returned in the case of an error.
+ virtual size_t ReadMemoryFromInferior(lldb::addr_t vm_addr, void *buf,
+ size_t size, Status &error) = 0;
+};
+
// A class to track memory that was read from a live process between
// runs.
class MemoryCache {
public:
// Constructors and Destructors
- MemoryCache(Process &process);
+ MemoryCache(MemoryFromInferiorReader &reader, uint64_t cache_line_size);
~MemoryCache();
- void Clear(bool clear_invalid_ranges = false);
+ void Clear(uint64_t cache_line_size, bool clear_invalid_ranges = false);
void Flush(lldb::addr_t addr, size_t size);
@@ -52,10 +88,10 @@
BlockMap m_L1_cache; // A first level memory cache whose chunk sizes vary that
// will be used only if the memory read fits entirely in
// a chunk
- BlockMap m_L2_cache; // A memory cache of fixed size chinks
+ BlockMap m_L2_cache; // A memory cache of fixed size chunks
// (m_L2_cache_line_byte_size bytes in size each)
InvalidRanges m_invalid_ranges;
- Process &m_process;
+ MemoryFromInferiorReader &m_reader;
uint32_t m_L2_cache_line_byte_size;
private:
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
