Merge pull request #11015 from augusto2112/fix-read-remote-address

[lldb] Add an extra optional did_read_live_memory to Target::ReadMemory  and [lldb] Implement LLDBMemoryReader::readRemoteAddressImpl

Author: augusto2112

lldb/include/lldb/Target/Target.h

@@ -1193,11 +1193,14 @@ class Target : public std::enable_shared_from_this<Target>,
   // 2 - if there is a process, then read from memory
   // 3 - if there is no process, then read from the file cache
   //
+  // The optional did_read_live_memory parameter will be set true if live memory
+  // was read successfully.
+  //
   // The method is virtual for mocking in the unit tests.
   virtual size_t ReadMemory(const Address &addr, void *dst, size_t dst_len,
                             Status &error, bool force_live_memory = false,
-                            lldb::addr_t *load_addr_ptr = nullptr);
-
+                            lldb::addr_t *load_addr_ptr = nullptr,
+                            bool *did_read_live_memory = nullptr);
   size_t ReadCStringFromMemory(const Address &addr, std::string &out_str,
                                Status &error, bool force_live_memory = false);
 

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

@@ -225,7 +225,8 @@ LLDBMemoryReader::resolvePointer(swift::remote::RemoteAddress address,
   // to a tagged address so further memory reads originating from it benefit
   // from the file-cache optimization.
   swift::remote::RemoteAbsolutePointer process_pointer{
-      swift::remote::RemoteAddress{readValue, address.getAddressSpace()}};
+      swift::remote::RemoteAddress{
+          readValue, swift::remote::RemoteAddress::DefaultAddressSpace}};
 
   if (!readMetadataFromFileCacheEnabled()) {
     assert(address.getAddressSpace() ==
@@ -314,6 +315,13 @@ LLDBMemoryReader::resolvePointer(swift::remote::RemoteAddress address,
 
 bool LLDBMemoryReader::readBytes(swift::remote::RemoteAddress address,
                                  uint8_t *dest, uint64_t size) {
+  auto [success, _] = readBytesImpl(address, dest, size);
+  return success;
+}
+
+std::pair<bool, bool>
+LLDBMemoryReader::readBytesImpl(swift::remote::RemoteAddress address,
+                                uint8_t *dest, uint64_t size) {
   Log *log = GetLog(LLDBLog::Types);
   if (m_local_buffer) {
     bool overflow = false;
@@ -322,15 +330,15 @@ bool LLDBMemoryReader::readBytes(swift::remote::RemoteAddress address,
     if (overflow) {
       LLDB_LOGV(log, "[MemoryReader] address {0:x} + size {1} overflows", addr,
                 size);
-      return false;
+      return {false, false};
     }
     if (addr >= *m_local_buffer &&
         end <= *m_local_buffer + m_local_buffer_size) {
       // If this crashes, the assumptions stated in
       // GetDynamicTypeAndAddress_Protocol() most likely no longer
       // hold.
       memcpy(dest, (void *)addr, size);
-      return true;
+      return {true, false};
     }
   }
 
@@ -345,7 +353,7 @@ bool LLDBMemoryReader::readBytes(swift::remote::RemoteAddress address,
   if (!maybeAddr) {
     LLDB_LOGV(log, "[MemoryReader] could not resolve address {0:x}",
               address.getRawAddress());
-    return false;
+    return {false, false};
   }
   auto addr = *maybeAddr;
   if (addr.IsSectionOffset()) {
@@ -354,30 +362,34 @@ bool LLDBMemoryReader::readBytes(swift::remote::RemoteAddress address,
     if (object_file->GetType() == ObjectFile::Type::eTypeDebugInfo) {
       LLDB_LOGV(log, "[MemoryReader] Reading memory from symbol rich binary");
 
-      return object_file->ReadSectionData(section.get(), addr.GetOffset(), dest,
-                                          size);
+      bool success = object_file->ReadSectionData(section.get(),
+                                                  addr.GetOffset(), dest, size);
+      return {success, false};
     }
   }
 
   if (size > m_max_read_amount) {
     LLDB_LOGV(log, "[MemoryReader] memory read exceeds maximum allowed size");
-    return false;
+    return {false, false};
   }
   Target &target(m_process.GetTarget());
   Status error;
   // We only want to allow the file-cache optimization if we resolved the
   // address to section + offset.
   const bool force_live_memory =
       !readMetadataFromFileCacheEnabled() || !addr.IsSectionOffset();
-  if (size > target.ReadMemory(addr, dest, size, error, force_live_memory)) {
+  bool did_read_live_memory = false;
+  if (size > target.ReadMemory(addr, dest, size, error, force_live_memory,
+                               /*load_addr_ptr=*/nullptr,
+                               &did_read_live_memory)) {
     LLDB_LOGV(log,
               "[MemoryReader] memory read returned fewer bytes than asked for");
-    return false;
+    return {false, did_read_live_memory};
   }
   if (error.Fail()) {
     LLDB_LOGV(log, "[MemoryReader] memory read returned error: {0}",
               error.AsCString());
-    return false;
+    return {false, did_read_live_memory};
   }
 
   auto format_data = [](auto dest, auto size) {
@@ -391,7 +403,7 @@ bool LLDBMemoryReader::readBytes(swift::remote::RemoteAddress address,
   LLDB_LOGV(log, "[MemoryReader] memory read returned data: {0}",
             format_data(dest, size));
 
-  return true;
+  return {true, did_read_live_memory};
 }
 
 bool LLDBMemoryReader::readString(swift::remote::RemoteAddress address,
@@ -596,6 +608,32 @@ LLDBMemoryReader::getFileAddressAndModuleForTaggedAddress(
   return {{file_address, module}};
 }
 
+bool LLDBMemoryReader::readRemoteAddressImpl(
+    swift::remote::RemoteAddress address, swift::remote::RemoteAddress &out,
+    std::size_t size) {
+  assert((size == 4 || size == 8) &&
+         "Only 32 or 64 bit architectures are supported!");
+  auto *dest = (uint8_t *)std::malloc(size);
+  auto defer = llvm::make_scope_exit([&] { free(dest); });
+
+  auto [success, did_read_live_memory] = readBytesImpl(address, dest, size);
+  if (!success)
+    return false;
+
+  uint8_t addressSpace = did_read_live_memory
+                             ? swift::remote::RemoteAddress::DefaultAddressSpace
+                             : LLDBAddressSpace;
+  if (size == 4)
+    out = swift::remote::RemoteAddress(*reinterpret_cast<uint32_t *>(dest),
+                                       addressSpace);
+  else if (size == 8)
+    out = swift::remote::RemoteAddress(*reinterpret_cast<uint64_t *>(dest),
+                                       addressSpace);
+  else
+    return false;
+  return true;
+}
+
 std::optional<swift::reflection::RemoteAddress>
 LLDBMemoryReader::resolveRemoteAddress(
     swift::reflection::RemoteAddress address) const {
@@ -730,11 +768,6 @@ LLDBMemoryReader::resolveRemoteAddressFromSymbolObjectFile(
 }
 
 bool LLDBMemoryReader::readMetadataFromFileCacheEnabled() const {
-  auto &triple = m_process.GetTarget().GetArchitecture().GetTriple();
-
-  // 32 doesn't have a flag bit we can reliably use, so reading from filecache
-  // is disabled on it.
-  return m_process.GetTarget().GetSwiftReadMetadataFromFileCache() &&
-         triple.isArch64Bit();
+  return m_process.GetTarget().GetSwiftReadMetadataFromFileCache();
 }
 } // namespace lldb_private

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.h

@@ -98,6 +98,11 @@ class LLDBMemoryReader : public swift::remote::MemoryReader {
   /// Returns whether the filecache optimization is enabled or not.
   bool readMetadataFromFileCacheEnabled() const;
 
+protected:
+  bool readRemoteAddressImpl(swift::remote::RemoteAddress address,
+                             swift::remote::RemoteAddress &out,
+                             std::size_t integerSize) override;
+
 private:
   friend MemoryReaderLocalBufferHolder;
 
@@ -119,6 +124,12 @@ class LLDBMemoryReader : public swift::remote::MemoryReader {
   std::optional<Address>
   remoteAddressToLLDBAddress(swift::remote::RemoteAddress address) const;
 
+  /// Implementation detail of readBytes. Returns a pair where the first element
+  /// indicates whether the memory was read successfully, the second element
+  /// indicates whether live memory was read.
+  std::pair<bool, bool> readBytesImpl(swift::remote::RemoteAddress address,
+                                      uint8_t *dest, uint64_t size);
+
   /// Reads memory from the symbol rich binary from the address into dest.
   /// \return true if it was able to successfully read memory.
   std::optional<Address> resolveRemoteAddressFromSymbolObjectFile(

lldb/source/Target/Target.cpp

@@ -1921,7 +1921,8 @@ size_t Target::ReadMemoryFromFileCache(const Address &addr, void *dst,
 
 size_t Target::ReadMemory(const Address &addr, void *dst, size_t dst_len,
                           Status &error, bool force_live_memory,
-                          lldb::addr_t *load_addr_ptr) {
+                          lldb::addr_t *load_addr_ptr,
+                          bool *did_read_live_memory) {
   error.Clear();
 
   Address fixed_addr = addr;
@@ -2020,6 +2021,8 @@ size_t Target::ReadMemory(const Address &addr, void *dst, size_t dst_len,
       if (bytes_read) {
         if (load_addr_ptr)
           *load_addr_ptr = load_addr;
+        if (did_read_live_memory)
+          *did_read_live_memory = true;
         return bytes_read;
       }
     }

lldb/unittests/Expression/DWARFExpressionTest.cpp

@@ -111,7 +111,8 @@ class MockTarget : public Target {
 
   size_t ReadMemory(const Address &addr, void *dst, size_t dst_len,
                     Status &error, bool force_live_memory = false,
-                    lldb::addr_t *load_addr_ptr = nullptr) /*override*/ {
+                    lldb::addr_t *load_addr_ptr = nullptr,
+                    bool *did_read_live_memory = nullptr) /*override*/ {
     auto expected_memory = this->ReadMemory(addr.GetOffset(), dst_len);
     if (!expected_memory) {
       llvm::consumeError(expected_memory.takeError());