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

1780

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

[augusto2112]

lldb/source/Target/Target.cpp

1780

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

[shafik]

lldb/source/Target/Target.cpp

1780

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

1780

Thanks Shakif, it does look a bit simpler now.

[shafik]

lldb/source/Target/Target.cpp

1780

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

1823–1830

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

1830

[augusto2112]

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

[augusto2112]

lldb/source/Target/Target.cpp

1823–1830

Thanks Jonas!

[jasonmolenda]

LGTM!

lldb/source/Target/Target.cpp

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,