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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.


================
Comment at: lldb/source/Target/Memory.cpp:175
 
-  if (dst && bytes_left > 0) {
-    const uint32_t cache_line_byte_size = m_L2_cache_line_byte_size;
-    uint8_t *dst_buf = (uint8_t *)dst;
-    addr_t curr_addr = addr - (addr % cache_line_byte_size);
-    addr_t cache_offset = addr - curr_addr;
-
-    while (bytes_left > 0) {
-      if (m_invalid_ranges.FindEntryThatContains(curr_addr)) {
-        error.SetErrorStringWithFormat("memory read failed for 0x%" PRIx64,
-                                       curr_addr);
-        return dst_len - bytes_left;
-      }
+  const uint32_t cache_line_byte_size = m_L2_cache_line_byte_size;
+  uint8_t *dst_buf = (uint8_t *)dst;
----------------
nit: Is this necessary ?


================
Comment at: lldb/source/Target/Memory.cpp:177-178
+  uint8_t *dst_buf = (uint8_t *)dst;
+  addr_t cache_offset = addr % cache_line_byte_size;
+  addr_t curr_cache_line_base_addr = addr - cache_offset;
+
----------------
Could use a comment explaining what we're doing here.


================
Comment at: lldb/source/Target/Memory.cpp:200
+      if (cache_offset >= cached_line_size)
+        return dst_len - bytes_left;
 
----------------
Shouldn't this be the size of the cached line ?


================
Comment at: lldb/source/Target/Memory.cpp:210
+      bytes_left -= curr_read_size;
+      curr_cache_line_base_addr += cache_line_byte_size;
+      cache_offset = 0;
----------------
IIUC, now that the read succeeded, you're shifting the current cache line base address to point to the next cache line base address, so you can continue reading if necessary. I had troubles understanding the point of this line before reading the rest of the code so either this should move closer to where it's used or at least it should have a comment explaining what it's doing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145624