clayborg added a comment.
I am not a pro at the gtest stuff, but this was very hard to follow and figure
out what these tests are doing.
================
Comment at: lldb/source/Target/Memory.cpp:23-24
// 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(),
----------------
Might be cleaner to add getting the cache line size from the
MemoryFromInferiorReader as a second virtual function. This would avoid having
to add the extra parameter here and to the clear method.
================
Comment at: lldb/source/Target/Memory.cpp:31
-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);
----------------
remove "cache_line_size" if we add a new virtual function to
MemoryFromInferiorReader
================
Comment at: lldb/source/Target/Memory.cpp:37
m_invalid_ranges.Clear();
- m_L2_cache_line_byte_size = m_process.GetMemoryCacheLineSize();
+ m_L2_cache_line_byte_size = cache_line_size;
}
----------------
Change to:
```
m_L2_cache_line_byte_size = m_reader.GetCacheLineSize();
```
if we add virtual function to MemoryFromInferiorReader
================
Comment at: lldb/source/Target/Memory.cpp:65-66
+ // intersecting.
+ BlockMap::iterator previous = pos;
+ previous--;
+ AddrRange previous_range(previous->first,
----------------
This can just be:
```
BlockMap::iterator previous = pos - 1;
```
================
Comment at: lldb/source/Target/Memory.cpp:67-68
+ previous--;
+ AddrRange previous_range(previous->first,
+ previous->second->GetByteSize());
+ if (previous_range.DoesIntersect(flush_range))
----------------
name "chunk_range" like in the while loop below for consistency?
================
Comment at: lldb/source/Target/Process.cpp:488
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),
----------------
remove GetMemoryCacheLineSize() if we add virtual method to
MemoryFromInferiorReader
================
Comment at: lldb/source/Target/Process.cpp:612
m_image_tokens.clear();
- m_memory_cache.Clear();
+ m_memory_cache.Clear(GetMemoryCacheLineSize());
m_allocated_memory_cache.Clear();
----------------
remove GetMemoryCacheLineSize() if we add virtual method to
MemoryFromInferiorReader
================
Comment at: lldb/source/Target/Process.cpp:1456
m_mod_id.SetStopEventForLastNaturalStopID(event_sp);
- m_memory_cache.Clear();
+ m_memory_cache.Clear(GetMemoryCacheLineSize());
LLDB_LOGF(log, "Process::SetPrivateState (%s) stop_id = %u",
----------------
remove GetMemoryCacheLineSize() if we add virtual method to
MemoryFromInferiorReader
================
Comment at: lldb/source/Target/Process.cpp:5575
m_thread_list.DiscardThreadPlans();
- m_memory_cache.Clear(true);
+ m_memory_cache.Clear(GetMemoryCacheLineSize(), true);
DoDidExec();
----------------
remove GetMemoryCacheLineSize() if we add virtual method to
MemoryFromInferiorReader
================
Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:32-69
+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) {
----------------
Just inline these to avoid having to have a declaration inside the class and
then these functions outside of the class?
================
Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:34-35
+ : 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));
+}
----------------
Comment maybe saying something like:
```
// 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.
```
================
Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:75-76
+
+ m_cache.Read(10, result.data(), result.size(), error);
+ ASSERT_EQ(10, result[0]);
+
----------------
So we are reading 10 bytes, but only verifying the first byte in the 10 bytes
we read? Why not verify all of them to be sure it worked with a for loop?
```
for (int i=0; i<result.size(); ++i)
ASSERT_EQ(i+10, result[i]);
```
or just do manual asserts:
```
ASSERT_EQ(10, result[0]);
ASSERT_EQ(11, result[1]);
ASSERT_EQ(12, result[2]);
...
```
================
Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:81
+ m_cache.Read(10, result.data(), result.size(), error);
+ ASSERT_EQ(m_memory[10], result[0]);
+}
----------------
So with "IncrementAndFlushRange(10, 1);" we are modifying one byte at index 10
by incrementing it right? It isn't clear from the:
```
ASSERT_EQ(m_memory[10], result[0]);
```
What we are verifying here. Wouldn't it be simpler to do:
```
ASSERT_EQ(11, result[0]);
```
And also verify that all of the other 9 bytes we read were unchanged?
```
ASSERT_EQ(11, result[1]);
ASSERT_EQ(12, result[2]);
...
```
================
Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:92
+ for (std::pair<lldb::addr_t, size_t> p : cached)
+ AddRangeToL1Cache(p.first, p.second);
+
----------------
seems weird to use AddRangeToL1Cache instead of just reading memory from these
locations and it makes it less of a real world kind a test if we are mucking
with the L1 cache directly. Makes the test a bit harder to follow. Maybe just
reading from memory is easier to follow?:
```
// Read ranges from memory to populate the L1 cache
std::vector<uint8_t> result(10);
for (std::pair<lldb::addr_t, size_t> p : cached)
ReadByBytes(p.first, p.second);
```
================
Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:98-100
+ size_t check_size = 50;
+ std::vector<uint8_t> result = ReadByBytes(0, check_size);
+ EXPECT_THAT(result, ::testing::ElementsAreArray(m_memory.data(),
check_size));
----------------
I can't tell what these three lines are doing. Very hard to follow
================
Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:121
+ for (std::pair<lldb::addr_t, size_t> p : cached)
+ AddRangeToL1Cache(p.first, p.second);
+
----------------
Same comment as last test about just reading from memory to populate the cache?
================
Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:127-129
+ for (std::pair<lldb::addr_t, size_t> p : cached)
+ ReadByBytes(p.first, p.second);
+ EXPECT_EQ(0u, m_inferior_read_count);
----------------
How do we know this won't try and read any memory? Are we assured that
IncrementAndFlushRange won't touch any ranges we are trying to read here?
================
Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:132-134
+ size_t check_size = 50;
+ std::vector<uint8_t> result = ReadByBytes(0, check_size);
+ EXPECT_THAT(result, ::testing::ElementsAreArray(m_memory.data(),
check_size));
----------------
What is this checking?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77765/new/
https://reviews.llvm.org/D77765
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
