[Lldb-commits] [PATCH] D77765: Fix incorrect L1 inferior memory cache flushing

Wed Apr 8 21:02:56 PDT 2020

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