Change Target::ReadMemory to ensure the amount of memory read from the file-cache is the amount requested.

[augusto2112]

Summary

This change ensures that if for whatever reason we read less bytes than expected (for example, when trying to read memory that spans multiple sections), we try reading from the live process as well.

[shafik]

lldb/source/Target/Target.cpp

1775

Is there a reason why we need to use malloc and free?

[augusto2112]

lldb/source/Target/Target.cpp

1775

Since the type is void I thought that was the correct way. What should I use instead?

[shafik]

lldb/source/Target/Target.cpp

1775

I looked at the places we are calling Target::ReadMemory and they are using some form of a uint8_t buffer. So a uint8_t array would be fine:

std::unique_ptr<uint8_t[]> file_cache_read_buffer;
//....
p = std::make_unique<uint8_t[]>(file_cache_bytes_read);

[augusto2112]

Change buffer unique pointer from void to uint8_t, eliminating the need to call malloc/free.

[augusto2112]

lldb/source/Target/Target.cpp

1775

Thanks Shakif, it does look a bit simpler now.

[shafik]

lldb/source/Target/Target.cpp

1775

std::vector for a buffer is also possible some callers do that but I don't see a reason to prefer that over this solution.

[JDevlieghere]

lldb/source/Target/Target.cpp

1818–1825

The unique_ptr provides an operator bool, so you don't need to check the underlying pointer yourself.

1825

[augusto2112]

Rely on unique_ptr's bool operator, instead of checking the underlying pointer.

[augusto2112]

lldb/source/Target/Target.cpp

1818–1825

Thanks Jonas!

[jasonmolenda]

LGTM!

lldb/source/Target/Target.cpp

 //===-- Target.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/Target.h"
 #include "Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h"
 #include "lldb/Breakpoint/BreakpointIDList.h"
 #include "lldb/Breakpoint/BreakpointPrecondition.h"
 #include "lldb/Breakpoint/BreakpointResolver.h"
 #include "lldb/Breakpoint/BreakpointResolverAddress.h"
 #include "lldb/Breakpoint/BreakpointResolverFileLine.h"
 #include "lldb/Breakpoint/BreakpointResolverFileRegex.h"
 #include "lldb/Breakpoint/BreakpointResolverName.h"
if (section_load_list.IsEmpty()) {
       load_addr = addr.GetOffset(); // "addr" doesn't have a section, so its
                                     // offset is the load address
       section_load_list.ResolveLoadAddress(load_addr, resolved_addr);
     }
   }
   if (!resolved_addr.IsValid())
     resolved_addr = addr;
 
-  bool is_readonly = false;
+  // If we read from the file cache but can't get as many bytes as requested,
+  // we keep the result around in this buffer, in case this result is the
+  // best we can do.
+  std::unique_ptr<uint8_t[]> file_cache_read_buffer;
+  size_t file_cache_bytes_read = 0;
+
   // Read from file cache if read-only section.
   if (!force_live_memory && resolved_addr.IsSectionOffset()) {
     SectionSP section_sp(resolved_addr.GetSection());
     if (section_sp) {
       auto permissions = Flags(section_sp->GetPermissions());
-      is_readonly = !permissions.Test(ePermissionsWritable) &&
+      bool is_readonly = !permissions.Test(ePermissionsWritable) &&
                          permissions.Test(ePermissionsReadable);
-    }
       if (is_readonly) {
-      bytes_read = ReadMemoryFromFileCache(resolved_addr, dst, dst_len, error);
-      if (bytes_read > 0)
-        return bytes_read;
+        file_cache_bytes_read =
+            ReadMemoryFromFileCache(resolved_addr, dst, dst_len, error);
+        if (file_cache_bytes_read == dst_len)
+          return file_cache_bytes_read;
+        else if (file_cache_bytes_read > 0) {
+          file_cache_read_buffer =
+              std::make_unique<uint8_t[]>(file_cache_bytes_read);
+          std::memcpy(file_cache_read_buffer.get(), dst, file_cache_bytes_read);
+        }
+      }
     }
   }
 
   if (ProcessIsValid()) {
     if (load_addr == LLDB_INVALID_ADDRESS)
       load_addr = resolved_addr.GetLoadAddress(this);
 
     if (load_addr == LLDB_INVALID_ADDRESS) {
if (load_addr == LLDB_INVALID_ADDRESS) {
       if (bytes_read) {
         if (load_addr_ptr)
           *load_addr_ptr = load_addr;
         return bytes_read;
       }
     }
   }
 
-  if (!is_readonly && resolved_addr.IsSectionOffset()) {
+  if (file_cache_read_buffer && file_cache_bytes_read > 0) {
+    // Reading from the process failed. If we've previously succeeded in reading
+    // something from the file cache, then copy that over and return that.
+    std::memcpy(dst, file_cache_read_buffer.get(), file_cache_bytes_read);
+    return file_cache_bytes_read;
+  }
+
+  if (!file_cache_read_buffer && resolved_addr.IsSectionOffset()) {
     // If we didn't already try and read from the object file cache, then try
     // it after failing to read from the process.
     return ReadMemoryFromFileCache(resolved_addr, dst, dst_len, error);
   }
   return 0;
 }
 
 size_t Target::ReadCStringFromMemory(const Address &addr, std::string &out_str,