jarin created this revision.
jarin added reviewers: labath, clayborg.
jarin added a project: LLDB.
Herald added subscribers: lldb-commits, mgorny.
jarin retitled this revision from "Fix incorrect L1 cache flushing" to "Fix
incorrect L1 inferior memory cache flushing".
As discussed in https://reviews.llvm.org/D74398, the L1
<https://reviews.llvm.org/L1> memory cache flushing is incorrect.
For instance, if the L1 <https://reviews.llvm.org/L1> cache contains two chunks
(10, 10) and (30, 10) and we call MemoryCache::Flush(25, 10), the current code
does not flush anything (because it just tries to flush the previous range (10,
10) and if that is not intersecting, it will bail out).
With this patch, if the previous chunk is not overlapping, we still try the
next chunk, and only if that one is not overlapping, we bail out.
This also adds some unit tests for the cache (some of the tests fail with the
current code). The unit tests required some changes for testability.
Repository:
rG LLVM Github Monorepo
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,144 @@
+#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;
+
+public:
+ MemoryCacheTest();
+
+ void AddRangeToL1Cache(lldb::addr_t addr, size_t len);
+ void IncrementAndFlushRange(lldb::addr_t addr, size_t len);
+ std::vector<uint8_t> ReadByBytes(lldb::addr_t addr, size_t len);
+
+protected:
+ MemoryCache m_cache;
+ std::vector<uint8_t> m_memory;
+ size_t m_inferior_read_count;
+};
+
+MemoryCacheTest::MemoryCacheTest()
+ : m_cache(*this, k_cache_line_size), m_inferior_read_count(0) {
+ for (size_t i = 0; i < k_memory_size; i++)
+ m_memory.push_back(static_cast<uint8_t>(i));
+}
+
+void MemoryCacheTest::AddRangeToL1Cache(lldb::addr_t addr, size_t len) {
+ m_cache.AddL1CacheData(addr, m_memory.data() + addr, len);
+}
+
+size_t MemoryCacheTest::ReadMemoryFromInferior(lldb::addr_t vm_addr, void *buf,
+ size_t size, Status &error) {
+ 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;
+}
+
+void MemoryCacheTest::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);
+}
+
+std::vector<uint8_t> MemoryCacheTest::ReadByBytes(lldb::addr_t addr,
+ size_t len) {
+ std::vector<uint8_t> result(len, 0);
+ for (size_t i = 0; i < len; i++) {
+ Status error;
+ m_cache.Read(addr + i, &(result[i]), 1, error);
+ EXPECT_TRUE(error.Success());
+ }
+ return result;
+}
+
+TEST_F(MemoryCacheTest, L1FlushSimple) {
+ Status error;
+ std::vector<uint8_t> result(10);
+
+ m_cache.Read(10, result.data(), result.size(), error);
+ ASSERT_EQ(10, result[0]);
+
+ IncrementAndFlushRange(10, 1);
+
+ m_cache.Read(10, result.data(), result.size(), error);
+ ASSERT_EQ(m_memory[10], result[0]);
+}
+
+class MemoryCacheOverlapTest
+ : public MemoryCacheTest,
+ public testing::WithParamInterface<std::pair<lldb::addr_t, size_t>> {};
+
+TEST_P(MemoryCacheOverlapTest, L1FlushFromTwoWithOverlap) {
+ // Add two ranges to the cache.
+ std::vector<std::pair<lldb::addr_t, size_t>> cached = {{10, 10}, {30, 10}};
+ for (std::pair<lldb::addr_t, size_t> p : cached)
+ AddRangeToL1Cache(p.first, p.second);
+
+ // Flush the range given by the parameter.
+ IncrementAndFlushRange(GetParam().first, GetParam().second);
+
+ // Check the memory.
+ size_t check_size = 50;
+ std::vector<uint8_t> result = ReadByBytes(0, check_size);
+ EXPECT_THAT(result, ::testing::ElementsAreArray(m_memory.data(), check_size));
+}
+
+INSTANTIATE_TEST_CASE_P(
+ AllMemoryCacheOverlapTest, MemoryCacheOverlapTest,
+ ::testing::Values(
+ std::make_pair(5, 6), std::make_pair(5, 10), std::make_pair(5, 20),
+ std::make_pair(5, 30), std::make_pair(5, 40), std::make_pair(10, 1),
+ std::make_pair(10, 21), std::make_pair(19, 1), std::make_pair(19, 11),
+ std::make_pair(19, 12), std::make_pair(20, 11), std::make_pair(20, 25),
+ std::make_pair(29, 2), std::make_pair(29, 12), std::make_pair(30, 1),
+ std::make_pair(39, 1), std::make_pair(39, 5)));
+
+class MemoryCacheNoOverlapTest
+ : public MemoryCacheTest,
+ public testing::WithParamInterface<std::pair<lldb::addr_t, size_t>> {};
+
+TEST_P(MemoryCacheNoOverlapTest, L1FlushFromTwoWithoutOverlap) {
+ // Add two ranges to the cache.
+ std::vector<std::pair<lldb::addr_t, size_t>> cached = {{10, 10}, {30, 10}};
+ for (std::pair<lldb::addr_t, size_t> p : cached)
+ AddRangeToL1Cache(p.first, p.second);
+
+ // Flush the range given by the parameter.
+ IncrementAndFlushRange(GetParam().first, GetParam().second);
+
+ // Check that we can read from the cache without going to the inferior.
+ for (std::pair<lldb::addr_t, size_t> p : cached)
+ ReadByBytes(p.first, p.second);
+ EXPECT_EQ(0u, m_inferior_read_count);
+
+ // Check the memory.
+ size_t check_size = 50;
+ std::vector<uint8_t> result = ReadByBytes(0, check_size);
+ EXPECT_THAT(result, ::testing::ElementsAreArray(m_memory.data(), check_size));
+}
+
+INSTANTIATE_TEST_CASE_P(
+ AllMemoryCacheNoOverlapTest, MemoryCacheNoOverlapTest,
+ ::testing::Values(std::make_pair(5, 1), std::make_pair(5, 5),
+ std::make_pair(9, 1), std::make_pair(20, 1),
+ std::make_pair(20, 10), std::make_pair(29, 1),
+ std::make_pair(40, 1), std::make_pair(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,14 @@
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 previous_range(previous->first,
+ previous->second->GetByteSize());
+ if (previous_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 +164,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 +227,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
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
