[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Target/Memory.cpp:131
+
   size_t bytes_left = dst_len;
 
----------------
This isn't used until line 180. I'd move it down, closer to where it is being used.


================
Comment at: lldb/source/Target/Memory.cpp:133-141
   // Check the L1 cache for a range that contain the entire memory read. If we
-  // find a range in the L1 cache that does, we use it. Else we fall back to
-  // reading memory in m_L2_cache_line_byte_size byte sized chunks. The L1
-  // cache contains chunks of memory that are not required to be
+  // find a range in the L1 cache that does, we use it. If not:
+  //  - dst_len > L2 cache line size: Attempt to read the entire memory chunk
+  //    and insert whatever we get back into the L1 cache.
+  //  - dst_len <= L2 cache line size: Attempt to read it from the L2 cache, and
+  //    if its not there, attempt to populate the cache.
+  // The L1 cache contains chunks of memory that are not required to be
----------------
Instead of describing the algorithm here, would it make sense to break this up and put it above the relevant code below? It seems like it matches pretty well with the code structure. Looking at the signature of `FindEntryThatContains` and the fact that it doesn't take the size, I assume it's because we only check the start address? 


================
Comment at: lldb/source/Target/Memory.cpp:145-147
+  // FIXME: We should do a more thorough check to make sure that we're not
+  // overlapping with any invalid ranges (e.g. Read 0x100 - 0x200 but there's an
+  // invalid range 0x180 - 0x280).
----------------
What would a more thorough check look like? Or phrased differently: what is the current check missing?


================
Comment at: lldb/source/Target/Memory.cpp:149
+  if (m_invalid_ranges.FindEntryThatContains(addr)) {
+    error.SetErrorStringWithFormat("memory read failed for 0x%" PRIx64, addr);
+    return 0;
----------------
Should the error mention that if failed due to the address overlapping with an invalid range? Is an invalid range something that is meaningful to the user? 


================
Comment at: lldb/source/Target/Memory.cpp:196-198
+      // If we didn't fill out the cache line completely and the offset is
+      // bigger than what we have available, we can't do anything further
+      // here.
----------------
Why can't we read from the process? Same question below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145624