[lldb] Make MemoryCache::Read more resilient

[bulbazord]

Summary

MemoryCache::Read is not resilient to partial reads when reading memory
chunks less than or equal in size to L2 cache lines. There have been
attempts in the past to fix this but nothing really solved the root of
the issue.

I first created a test exercising MemoryCache's implementation and
documenting how I believe MemoryCache::Read should behave. I then
rewrote the implementation of MemoryCache::Read as needed to make sure
that the different scenarios behaved correctly.

rdar://105407095

[JDevlieghere]

lldb/source/Target/Memory.cpp

155–156

This isn't used until line 180. I'd move it down, closer to where it is being used.

155–157

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?

161–163

What would a more thorough check look like? Or phrased differently: what is the current check missing?

165

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?

202–204

Why can't we read from the process? Same question below.

[mib]

lldb/source/Target/Memory.cpp

199–200

nit: Is this necessary ?

200

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.

201–202

Could use a comment explaining what we're doing here.

206

Shouldn't this be the size of the cached line ?

[clayborg]

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.

lldb/source/Target/Memory.cpp

202–204

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?

205–207

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

260–261

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?

263

Should we just create a DataBufferSP right away here instead of creating a unique pointer and releasing it later?

[bulbazord]

lldb/source/Target/Memory.cpp

155–157

Good point!

165

I don't think the concept of "invalid range" is meaningful to the user right now. I'm pretty sure we only use it to prevent us from reading __PAGEZERO on apple platforms.

200

I should have paid closer attention to this line. We should only be moving the cache line base address to the next cache line if we're going to continue reading. Will refactor.

202–204

If we've hit this code path then we have previously read from the process and got back fewer bytes than a cache line fits. For example, maybe a cache line is 512 bytes and when we performed the read we got back 502 bytes for some reason. If we're trying to read the last 10 bytes of that line, that's just not available, so we bail out. We could try to protect against this in a number of ways, like if we get back fewer bytes than we wanted initially then maybe we can retry the read before caching the line, or if the line isn't filled out maybe can try to read the inferior one more time or something.

Ultimately, I want MemoryCache to be prepared for reads to be incomplete and guard against touching memory that we don't have.

205–207

If I'm understanding what you mean correctly, I think we can check this near the beginning. We have all the information we need to do safety checks before we even start reading anything, I believe...?

Also, because we read at most 2 cache lines, I can probably get rid of the loop and just do 2 sequential reads...

206

To be honest, I'm somewhat sure that this line actually could be return 0;. I'm pretty sure that we only will hit this on the first cache line read. If we read a second cache line, we should always be starting at the beginning of the cache line... I'll probably refactor this.

260–261

That might not be a bad idea actually. I'll try it and see how much it simplifies the logic.

263

That sounds like a smarter move.

[clayborg]

One other optimization we can do is if we read from the process memory and it returns that is read zero bytes, right now we add the range we were trying to read into the m_invalid_ranges member variable. So lets say we were trying to read the range [0x1000-0x2000) on a mac. We will fail to read this due to __PAGEZERO, but I believe we currently add this range to the m_invalid_ranges. But we could ask about this memory region from the process and realize we can actually add [0x0-0x100000000) to the m_invalid_ranges. That might help avoid multiple bad reads from a large area that isn't mapped.

[clayborg]

I would suggest checking the google stadia patch for the L1 and L2 caches:

https://github.com/googlestadia/vsi-lldb/blob/master/patches/llvm-project/0019-lldb-Fix-incorrect-L1-inferior-memory-cache-flushing.patch

Just to see how they did things.

[bulbazord]

In D145624#4182424, @clayborg wrote:

I would suggest checking the google stadia patch for the L1 and L2 caches:

https://github.com/googlestadia/vsi-lldb/blob/master/patches/llvm-project/0019-lldb-Fix-incorrect-L1-inferior-memory-cache-flushing.patch

Just to see how they did things.

I looked at this patch earlier. They're modifying code that I'm not touching and these 2 patches can be applied separately without conflict. It may be worth trying to apply this fix in a follow-up commit.

[bulbazord]

• Addressed reviewer feedback
• Simplified the case where we use L2 cache lines

[bulbazord]

lldb/source/Target/Memory.cpp

260–261

It didn't make things any simpler so I didn't change it. Good idea though.

[clayborg]

lldb/source/Target/Memory.cpp

133–135

remove braces for single line if statement per llvm coding guidelines

203–204

move these two lines below the 2 if statements below that return early?

232–233

Is this an error here? We already got something from the first read and we are just returning partial data, do we need an error? If we fail the first read, then this is an error.

[bulbazord]

lldb/source/Target/Memory.cpp

232–233

If the second cache line you read is in an invalid range, maybe the user would want some feedback about why it was a partial read. It's a detectable condition. Maybe we shouldn't set an error string though, idk.

[bulbazord]

Address small nits from Greg

[JDevlieghere]

LGTM if @clayborg is happy

[clayborg]

LGTM

lldb/include/lldb/Target/Memory.h

BlockMap m_L2_cache; // A memory cache of fixed size chinks
                        // (m_L2_cache_line_byte_size bytes in size each)
   InvalidRanges m_invalid_ranges;
   Process &m_process;
   uint32_t m_L2_cache_line_byte_size;
 
 private:
   MemoryCache(const MemoryCache &) = delete;
   const MemoryCache &operator=(const MemoryCache &) = delete;
+
+  lldb::DataBufferSP GetL2CacheLine(lldb::addr_t addr, Status &error);
 };
 
     
 
 class AllocatedBlock {
 public:
   AllocatedBlock(lldb::addr_t addr, uint32_t byte_size, uint32_t permissions,
                  uint32_t chunk_size);

lldb/source/Target/Memory.cpp

if (idx != UINT32_MAX) {
       if (entry->GetRangeBase() == base_addr &&
           entry->GetByteSize() == byte_size)
         return m_invalid_ranges.RemoveEntryAtIndex(idx);
     }
   }
   return false;
 }
 
-size_t MemoryCache::Read(addr_t addr, void *dst, size_t dst_len,
+lldb::DataBufferSP MemoryCache::GetL2CacheLine(lldb::addr_t line_base_addr,
                                                Status &error) {
-  size_t bytes_left = dst_len;
+  // This function assumes that the address given is aligned correctly.
+  assert((line_base_addr % m_L2_cache_line_byte_size) == 0);
+
+  std::lock_guard<std::recursive_mutex> guard(m_mutex);
+  auto pos = m_L2_cache.find(line_base_addr);
+  if (pos != m_L2_cache.end())
+    return pos->second;
+
+  auto data_buffer_heap_sp =
+      std::make_shared<DataBufferHeap>(m_L2_cache_line_byte_size, 0);
+  size_t process_bytes_read = m_process.ReadMemoryFromInferior(
+      line_base_addr, data_buffer_heap_sp->GetBytes(),
+      data_buffer_heap_sp->GetByteSize(), error);
+
+  // If we failed a read, not much we can do.
+  if (process_bytes_read == 0)
+    return lldb::DataBufferSP();
+
+  // If we didn't get a complete read, we can still cache what we did get.
+  if (process_bytes_read < m_L2_cache_line_byte_size)
+    data_buffer_heap_sp->SetByteSize(process_bytes_read);
+
+  m_L2_cache[line_base_addr] = data_buffer_heap_sp;
+  return data_buffer_heap_sp;
+}
 
-  // 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
-  // m_L2_cache_line_byte_size bytes in size, so we don't try anything tricky
-  // when reading from them (no partial reads from the L1 cache).
+size_t MemoryCache::Read(addr_t addr, void *dst, size_t dst_len,
+                         Status &error) {
+  if (!dst || dst_len == 0)
+    return 0;
 
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
+  // 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). `FindEntryThatContains` has an implementation
+  // that takes a range, but it only checks to see if the argument is contained
+  // by an existing invalid range. It cannot check if the argument contains
+  // invalid ranges and cannot check for overlaps.
+  if (m_invalid_ranges.FindEntryThatContains(addr)) {
+    error.SetErrorStringWithFormat("memory read failed for 0x%" PRIx64, addr);
+    return 0;
+  }
+
+  // Check the L1 cache for a range that contains the entire memory read.
+  // L1 cache contains chunks of memory that are not required to be the size of
+  // an L2 cache line. We avoid trying to do partial reads from the L1 cache to
+  // simplify the implementation.
   if (!m_L1_cache.empty()) {
     AddrRange read_range(addr, dst_len);
     BlockMap::iterator pos = m_L1_cache.upper_bound(addr);
     if (pos != m_L1_cache.begin()) {
       --pos;
     }
     AddrRange chunk_range(pos->first, pos->second->GetByteSize());
     if (chunk_range.Contains(read_range)) {
       memcpy(dst, pos->second->GetBytes() + (addr - chunk_range.GetRangeBase()),
              dst_len);
       return dst_len;
     }
   }
 
-  // If this memory read request is larger than the cache line size, then we
-  // (1) try to read as much of it at once as possible, and (2) don't add the
-  // data to the memory cache.  We don't want to split a big read up into more
-  // separate reads than necessary, and with a large memory read request, it is
-  // unlikely that the caller function will ask for the next
-  // 4 bytes after the large memory read - so there's little benefit to saving
-  // it in the cache.
-  if (dst && dst_len > m_L2_cache_line_byte_size) {
+  // If the size of the read is greater than the size of an L2 cache line, we'll
+  // just read from the inferior. If that read is successful, we'll cache what
+  // we read in the L1 cache for future use.
+  if (dst_len > m_L2_cache_line_byte_size) {
     size_t bytes_read =
         m_process.ReadMemoryFromInferior(addr, dst, dst_len, error);
-    // Add this non block sized range to the L1 cache if we actually read
-    // anything
     if (bytes_read > 0)
       AddL1CacheData(addr, dst, bytes_read);
     return bytes_read;
   }
 
-  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;
-      }
+  // If the size of the read fits inside one L2 cache line, we'll try reading
+  // from the L2 cache. Note that if the range of memory we're reading sits
+  // between two contiguous cache lines, we'll touch two cache lines instead of
+  // just one.
+
+  // We're going to have all of our loads and reads be cache line aligned.
+  addr_t cache_line_offset = addr % m_L2_cache_line_byte_size;
+  addr_t cache_line_base_addr = addr - cache_line_offset;
+  DataBufferSP first_cache_line = GetL2CacheLine(cache_line_base_addr, error);
+  // If we get nothing, then the read to the inferior likely failed. Nothing to
+  // do here.
+  if (!first_cache_line)
+    return 0;
+
+  // If the cache line was not filled out completely and the offset is greater
+  // than what we have available, we can't do anything further here.
+  if (cache_line_offset >= first_cache_line->GetByteSize())
+    return 0;
 
-      BlockMap::const_iterator pos = m_L2_cache.find(curr_addr);
-      BlockMap::const_iterator end = m_L2_cache.end();
-
-      if (pos != end) {
-        size_t curr_read_size = cache_line_byte_size - cache_offset;
-        if (curr_read_size > bytes_left)
-          curr_read_size = bytes_left;
+  uint8_t *dst_buf = (uint8_t *)dst;
+  size_t bytes_left = dst_len;
+  size_t read_size = first_cache_line->GetByteSize() - cache_line_offset;
+  if (read_size > bytes_left)
+    read_size = bytes_left;
 
-        memcpy(dst_buf + dst_len - bytes_left,
-               pos->second->GetBytes() + cache_offset, curr_read_size);
+         first_cache_line->GetBytes() + cache_line_offset, read_size);
+  bytes_left -= read_size;
 
-        bytes_left -= curr_read_size;
-        curr_addr += curr_read_size + cache_offset;
-        cache_offset = 0;
+  // If the cache line was not filled out completely and we still have data to
+  // read, we can't do anything further.
+  if (first_cache_line->GetByteSize() < m_L2_cache_line_byte_size &&
+      bytes_left > 0)
+    return dst_len - bytes_left;
 
-        if (bytes_left > 0) {
-          // Get sequential cache page hits
-          for (++pos; (pos != end) && (bytes_left > 0); ++pos) {
-            assert((curr_addr % cache_line_byte_size) == 0);
-
-            if (pos->first != curr_addr)
-              break;
+  // We'll hit this scenario if our read straddles two cache lines.
+    cache_line_base_addr += m_L2_cache_line_byte_size;
 
-            curr_read_size = pos->second->GetByteSize();
-            if (curr_read_size > bytes_left)
-              curr_read_size = bytes_left;
-
-            memcpy(dst_buf + dst_len - bytes_left, pos->second->GetBytes(),
-                   curr_read_size);
-
-            bytes_left -= curr_read_size;
-            curr_addr += curr_read_size;
-
-            // We have a cache page that succeeded to read some bytes but not
-            // an entire page. If this happens, we must cap off how much data
-            // we are able to read...
-            if (pos->second->GetByteSize() != cache_line_byte_size)
-              return dst_len - bytes_left;
-          }
-        }
-      }
+    // FIXME: Until we are able to more thoroughly check for invalid ranges, we
+    // will have to check the second line to see if it is in an invalid range as
+    // well. See the check near the beginning of the function for more details.
+    if (m_invalid_ranges.FindEntryThatContains(cache_line_base_addr)) {
+      error.SetErrorStringWithFormat("memory read failed for 0x%" PRIx64,
+                                     cache_line_base_addr);
 
-      // We need to read from the process
-
-      if (bytes_left > 0) {
-        assert((curr_addr % cache_line_byte_size) == 0);
-        std::unique_ptr<DataBufferHeap> data_buffer_heap_up(
-            new DataBufferHeap(cache_line_byte_size, 0));
-        size_t process_bytes_read = m_process.ReadMemoryFromInferior(
-            curr_addr, data_buffer_heap_up->GetBytes(),
-            data_buffer_heap_up->GetByteSize(), error);
-        if (process_bytes_read == 0)
-          return dst_len - bytes_left;
+    DataBufferSP second_cache_line =
+        GetL2CacheLine(cache_line_base_addr, error);
+    if (!second_cache_line)
 
-        if (process_bytes_read != cache_line_byte_size) {
-          data_buffer_heap_up->SetByteSize(process_bytes_read);
-          if (process_bytes_read < data_buffer_heap_up->GetByteSize()) {
-            dst_len -= data_buffer_heap_up->GetByteSize() - process_bytes_read;
-            bytes_left = process_bytes_read;
-          }
-        }
-        m_L2_cache[curr_addr] = DataBufferSP(data_buffer_heap_up.release());
-        // We have read data and put it into the cache, continue through the
-        // loop again to get the data out of the cache...
-      }
-    }
-  }
+    read_size = bytes_left;
+    if (read_size > second_cache_line->GetByteSize())
+      read_size = second_cache_line->GetByteSize();
+
+    memcpy(dst_buf + dst_len - bytes_left, second_cache_line->GetBytes(),
+           read_size);
+    bytes_left -= read_size;
 
     return dst_len - bytes_left;
   }
 
+  return dst_len;
+}
+
 AllocatedBlock::AllocatedBlock(lldb::addr_t addr, uint32_t byte_size,
                                uint32_t permissions, uint32_t chunk_size)
     : m_range(addr, byte_size), m_permissions(permissions),
       m_chunk_size(chunk_size)
 {
   // The entire address range is free to start with.
   m_free_blocks.Append(m_range);
   assert(byte_size > chunk_size);

lldb/unittests/Target/CMakeLists.txt

 add_lldb_unittest(TargetTests
   ABITest.cpp
   DynamicRegisterInfoTest.cpp
   ExecutionContextTest.cpp
   MemoryRegionInfoTest.cpp
+  MemoryTest.cpp
   MemoryTagMapTest.cpp
   ModuleCacheTest.cpp
   PathMappingListTest.cpp
   RemoteAwarePlatformTest.cpp
   StackFrameRecognizerTest.cpp
   FindFileTest.cpp
 
   LINK_LIBS
       lldbCore
       lldbHost
       lldbPluginObjectFileELF
       lldbPluginPlatformLinux
+      lldbPluginPlatformMacOSX
       lldbPluginSymbolFileSymtab
       lldbTarget
       lldbSymbol
       lldbUtility
       lldbUtilityHelpers
     LINK_COMPONENTS
       Support
   )
 
 add_unittest_inputs(TargetTests TestModule.so)

lldb/unittests/Target/MemoryTest.cpp

+//===-- MemoryTest.cpp ----------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Target/Memory.h"
+#include "Plugins/Platform/MacOSX/PlatformMacOSX.h"
+#include "Plugins/Platform/MacOSX/PlatformRemoteMacOSX.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Target.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/DataBufferHeap.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+using namespace lldb;
+
+namespace {
+class MemoryTest : public ::testing::Test {
+public:
+  void SetUp() override {
+    FileSystem::Initialize();
+    HostInfo::Initialize();
+    PlatformMacOSX::Initialize();
+  }
+  void TearDown() override {
+    PlatformMacOSX::Terminate();
+    HostInfo::Terminate();
+    FileSystem::Terminate();
+  }
+};
+
+class DummyProcess : public Process {
+public:
+  DummyProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+      : Process(target_sp, listener_sp), m_bytes_left(0) {}
+
+  // Required overrides
+  bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) override {
+    return true;
+  }
+  Status DoDestroy() override { return {}; }
+  void RefreshStateAfterStop() override {}
+  size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
+                      Status &error) override {
+    if (m_bytes_left == 0)
+      return 0;
+
+    size_t num_bytes_to_write = size;
+    if (m_bytes_left < size) {
+      num_bytes_to_write = m_bytes_left;
+      m_bytes_left = 0;
+    } else {
+      m_bytes_left -= size;
+    }
+
+    memset(buf, 'B', num_bytes_to_write);
+    return num_bytes_to_write;
+  }
+  bool DoUpdateThreadList(ThreadList &old_thread_list,
+                          ThreadList &new_thread_list) override {
+    return false;
+  }
+  llvm::StringRef GetPluginName() override { return "Dummy"; }
+
+  // Test-specific additions
+  size_t m_bytes_left;
+  MemoryCache &GetMemoryCache() { return m_memory_cache; }
+  void SetMaxReadSize(size_t size) { m_bytes_left = size; }
+};
+} // namespace
+
+TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) {
+  PlatformSP platform_sp;
+  TargetSP target_sp;
+  debugger_sp->GetTargetList().CreateTarget(
+      *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
+  return target_sp;
+}
+
+TEST_F(MemoryTest, TesetMemoryCacheRead) {
+  ArchSpec arch("x86_64-apple-macosx-");
+
+  Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch));
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  TargetSP target_sp = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp = std::make_shared<DummyProcess>(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+
+  DummyProcess *process = static_cast<DummyProcess *>(process_sp.get());
+  MemoryCache &mem_cache = process->GetMemoryCache();
+  const uint64_t l2_cache_size = process->GetMemoryCacheLineSize();
+  Status error;
+  auto data_sp = std::make_shared<DataBufferHeap>(l2_cache_size * 2, '\0');
+  size_t bytes_read = 0;
+
+  // Cache empty, memory read fails, size > l2 cache size
+  process->SetMaxReadSize(0);
+  bytes_read = mem_cache.Read(0x1000, data_sp->GetBytes(),
+                              data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == 0);
+
+  // Cache empty, memory read fails, size <= l2 cache size
+  data_sp->SetByteSize(l2_cache_size);
+  bytes_read = mem_cache.Read(0x1000, data_sp->GetBytes(),
+                              data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == 0);
+
+  // Cache empty, memory read succeeds, size > l2 cache size
+  process->SetMaxReadSize(l2_cache_size * 4);
+  data_sp->SetByteSize(l2_cache_size * 2);
+  bytes_read = mem_cache.Read(0x1000, data_sp->GetBytes(),
+                              data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == data_sp->GetByteSize());
+  ASSERT_TRUE(process->m_bytes_left == l2_cache_size * 2);
+
+  // Reading data previously cached (not in L2 cache).
+  data_sp->SetByteSize(l2_cache_size + 1);
+  bytes_read = mem_cache.Read(0x1000, data_sp->GetBytes(),
+                              data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == data_sp->GetByteSize());
+  ASSERT_TRUE(process->m_bytes_left == l2_cache_size * 2); // Verify we didn't
+                                                           // read from the
+                                                           // inferior.
+
+  // Read from a different address, but make the size == l2 cache size.
+  // This should fill in a the L2 cache.
+  data_sp->SetByteSize(l2_cache_size);
+  bytes_read = mem_cache.Read(0x2000, data_sp->GetBytes(),
+                              data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == data_sp->GetByteSize());
+  ASSERT_TRUE(process->m_bytes_left == l2_cache_size);
+
+  // Read from that L2 cache entry but read less than size of the cache line.
+  // Additionally, read from an offset.
+  data_sp->SetByteSize(l2_cache_size - 5);
+  bytes_read = mem_cache.Read(0x2001, data_sp->GetBytes(),
+                              data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == data_sp->GetByteSize());
+  ASSERT_TRUE(process->m_bytes_left == l2_cache_size); // Verify we didn't read
+                                                       // from the inferior.
+
+  // What happens if we try to populate an L2 cache line but the read gives less
+  // than the size of a cache line?
+  process->SetMaxReadSize(l2_cache_size - 10);
+  data_sp->SetByteSize(l2_cache_size - 5);
+  bytes_read = mem_cache.Read(0x3000, data_sp->GetBytes(),
+                              data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == l2_cache_size - 10);
+  ASSERT_TRUE(process->m_bytes_left == 0);
+
+  // What happens if we have a partial L2 cache line filled in and we try to
+  // read the part that isn't filled in?
+  data_sp->SetByteSize(10);
+  bytes_read = mem_cache.Read(0x3000 + l2_cache_size - 10, data_sp->GetBytes(),
+                              data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == 0); // The last 10 bytes from this line are
+                                // missing and we should be reading nothing
+                                // here.
+
+  // What happens when we try to straddle 2 cache lines?
+  process->SetMaxReadSize(l2_cache_size * 2);
+  data_sp->SetByteSize(l2_cache_size);
+  bytes_read = mem_cache.Read(0x4001, data_sp->GetBytes(),
+                              data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == l2_cache_size);
+  ASSERT_TRUE(process->m_bytes_left == 0);
+
+  // What happens when we try to straddle 2 cache lines where the first one is
+  // only partially filled?
+  process->SetMaxReadSize(l2_cache_size - 1);
+  data_sp->SetByteSize(l2_cache_size);
+  bytes_read = mem_cache.Read(0x5005, data_sp->GetBytes(),
+                              data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == l2_cache_size - 6); // Ignoring the first 5 bytes,
+                                                // missing the last byte
+  ASSERT_TRUE(process->m_bytes_left == 0);
+
+  // What happens if we add an invalid range and try to do a read larger than
+  // a cache line?
+  mem_cache.AddInvalidRange(0x6000, l2_cache_size * 2);
+  process->SetMaxReadSize(l2_cache_size * 2);
+  data_sp->SetByteSize(l2_cache_size * 2);
+  bytes_read = mem_cache.Read(0x6000, data_sp->GetBytes(),
+                              data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == 0);
+  ASSERT_TRUE(process->m_bytes_left == l2_cache_size * 2);
+
+  // What happens if we add an invalid range and try to do a read lt/eq a
+  // cache line?
+  mem_cache.AddInvalidRange(0x7000, l2_cache_size);
+  process->SetMaxReadSize(l2_cache_size);
+  data_sp->SetByteSize(l2_cache_size);
+  bytes_read = mem_cache.Read(0x7000, data_sp->GetBytes(),
+                              data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == 0);
+  ASSERT_TRUE(process->m_bytes_left == l2_cache_size);
+
+  // What happens if we remove the invalid range and read again?
+  mem_cache.RemoveInvalidRange(0x7000, l2_cache_size);
+  bytes_read = mem_cache.Read(0x7000, data_sp->GetBytes(),
+                              data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == l2_cache_size);
+  ASSERT_TRUE(process->m_bytes_left == 0);
+
+  // What happens if we flush and read again?
+  process->SetMaxReadSize(l2_cache_size * 2);
+  mem_cache.Flush(0x7000, l2_cache_size);
+  bytes_read = mem_cache.Read(0x7000, data_sp->GetBytes(),
+                              data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == l2_cache_size);
+  ASSERT_TRUE(process->m_bytes_left == l2_cache_size); // Verify that we re-read
+                                                       // instead of using an
+                                                       // old cache
+}