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

Thu Apr 9 01:03:46 PDT 2020

jarin added a comment.

I rewrote parts of the test, hopefully making it a bit clearer. Please let me know if this made it better.

================
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(),
----------------
clayborg wrote:
> 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.
That's what I thought originally, but it does not logically belong there - it does not feel like inferior's memory should control cache line sizes. I also like that the extra parameter makes it clear when we reflect changes in the cache line size setting in the cache itself rather than having to guess when the cache might do a callback. I had trouble understanding that part before. 

Also, note that Process::GetMemoryCacheLineSize would have to be some ugly forwarder method because the existing GetMemoryCacheLineSize method is in ProcessProperties.

I am happy to put the method there if you insist, but I do not think it is a good idea. I would like to hear Pavel's opinion, too.

================
Comment at: lldb/source/Target/Memory.cpp:65-66
+      // intersecting.
+      BlockMap::iterator previous = pos;
+      previous--;
+      AddrRange previous_range(previous->first,
----------------
clayborg wrote:
> This can just be:
> 
> ```
> BlockMap::iterator previous = pos - 1;
> ```
It cannot because `map::iterator` does not have `operator-`.

================
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),
----------------
clayborg wrote:
> remove GetMemoryCacheLineSize() if we add virtual method to MemoryFromInferiorReader
Is not calling virtual functions in constructors dangerous? Note that this is going to be called during the construction of process.

================
Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:75-76
+
+  m_cache.Read(10, result.data(), result.size(), error);
+  ASSERT_EQ(10, result[0]);
+
----------------
clayborg wrote:
> 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]);
> ...
> ```
> 
Ouch, this test does not make sense because we do not put anything into L1. Fixed now.

Changed this to test just one byte.

================
Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:81
+  m_cache.Read(10, result.data(), result.size(), error);
+  ASSERT_EQ(m_memory[10], result[0]);
+}
----------------
clayborg wrote:
> 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]);
> ...
> ```
> 
What we really care about is that the value that Read returns is the same as the real one in memory, no? I actually do not really care how the IncrementAndFlushRange method modifies the memory as long as Read returns the correct value.

Nevertheless, I rewrote the code to read just one byte and to assert both the value and the invariant.

================
Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:92
+  for (std::pair<lldb::addr_t, size_t> p : cached)
+    AddRangeToL1Cache(p.first, p.second);
+
----------------
clayborg wrote:
> 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);
> ```
> 
I am quite confused by this comment. What the test does is precisely what we do in [the real world](https://github.com/llvm/llvm-project/blob/1d3b7370c466eba4bf22dce4a51f885f76698053/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp#L2108).

As far as I can see, reading from memory does not update L1 cache (only L2). The only way to update L1 is what is done here.

================
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));
----------------
clayborg wrote:
> I can't tell what these three lines are doing. Very hard to follow
I agree, this was a mess. Changed to check in a loop.

================
Comment at: lldb/unittests/Target/MemoryCacheTest.cpp:121
+  for (std::pair<lldb::addr_t, size_t> p : cached)
+    AddRangeToL1Cache(p.first, p.second);
+
----------------
clayborg wrote:
> Same comment as last test about just reading from memory to populate the cache?
See above.

================
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);
----------------
clayborg wrote:
> 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?
Because this test is only called with range that won't flush L1. I merged the test with the test above, hopefully making it clearer.

================
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));
----------------
clayborg wrote:
> What is this checking?
This just checks that the read return the real contents of the memory. Changed that to a check in a loop.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77765/new/

https://reviews.llvm.org/D77765