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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

There was a fix that was never submitted for Google Stadia for the memory cache here:

https://github.com/googlestadia/vsi-lldb/tree/master/patches/llvm-project

Might be worth checking what they did to ensure we have all of the same abilities.



================
Comment at: lldb/source/Target/Memory.cpp:181-183
+    // FIXME: We should be able to wrap this into the check near the beginning
+    // of this function but we don't check for overlapping ranges so we will
+    // additionally check each cache line we read.
----------------
Not on the FIXME: We can't really check this near the beginning, because this happens for each cache line we as we advance the "curr_cache_line_base_addr" right? 

One thing to note about this code is that we might need to read at most 2 cache lines for any requests that make it to this code since we check above for "if (dst_len > m_L2_cache_line_byte_size)..." and use the L1 cache if that is true. So we know that we will read at most 2 cache lines depending on the offset. Might be nice to read the 2 cache lines in one memory access below if possible, and then make two cache entries with the result, but it will be either one cache line read, or two


================
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.
----------------
JDevlieghere wrote:
> Why can't we read from the process? Same question below.
Because if we did a memory request before from a valid "curr_cache_line_base_addr", and we got back fewer bytes that requested, then the bytes won't be available later right?


================
Comment at: lldb/source/Target/Memory.cpp:245
+      assert((curr_cache_line_base_addr % cache_line_byte_size) == 0);
+      std::unique_ptr<DataBufferHeap> data_buffer_heap_up(
+          new DataBufferHeap(cache_line_byte_size, 0));
----------------
Should we just create a DataBufferSP right away here instead of creating a unique pointer and releasing it later?


================
Comment at: lldb/source/Target/Memory.cpp:255-256
 
-      // We need to read from the process
+      if (process_bytes_read < cache_line_byte_size)
+        data_buffer_heap_up->SetByteSize(process_bytes_read);
 
----------------
If we don't read an entire cache line, should we populate this into the L1 cache instead? It might make the logic for accessing data in the L2 cache a bit simpler?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145624