[Lldb-commits] [lldb] 92631a4 - [lldb][MinidumpFileBuilder] Fix addition of MemoryList steam (#88564)

[via lldb-commits]

Author: Miro Bucko
Date: 2024-04-22T10:40:06-07:00
New Revision: 92631a4824a91f3268a5716dd3459df8dc6bfb63

URL: https://github.com/llvm/llvm-project/commit/92631a4824a91f3268a5716dd3459df8dc6bfb63
DIFF: https://github.com/llvm/llvm-project/commit/92631a4824a91f3268a5716dd3459df8dc6bfb63.diff

LOG: [lldb][MinidumpFileBuilder] Fix addition of MemoryList steam (#88564)

Summary:
AddMemoryList() was returning the last error status returned by
ReadMemory(). So if an invalid memory region was read last, the function
would return an error.

Test Plan:
./bin/llvm-lit -sv
~/src/llvm-project/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py

Reviewers:
kevinfrei,clayborg 

Subscribers:

Tasks:

Tags:

Added: 
    

Modified: 
    lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
    lldb/source/Target/Process.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index cefd4cb22b6bae..601f11d51d4282 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -21,6 +21,7 @@
 #include "lldb/Target/ThreadList.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/LLDBLog.h"
+#include "lldb/Utility/Log.h"
 #include "lldb/Utility/RegisterValue.h"
 
 #include "llvm/ADT/StringRef.h"
@@ -663,14 +664,20 @@ MinidumpFileBuilder::AddMemoryList(const lldb::ProcessSP &process_sp,
   DataBufferHeap helper_data;
   std::vector<MemoryDescriptor> mem_descriptors;
   for (const auto &core_range : core_ranges) {
-    // Skip empty memory regions or any regions with no permissions.
-    if (core_range.range.empty() || core_range.lldb_permissions == 0)
+    // Skip empty memory regions.
+    if (core_range.range.empty())
       continue;
     const addr_t addr = core_range.range.start();
     const addr_t size = core_range.range.size();
     auto data_up = std::make_unique<DataBufferHeap>(size, 0);
     const size_t bytes_read =
         process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
+    if (error.Fail()) {
+      Log *log = GetLog(LLDBLog::Object);
+      LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s",
+                bytes_read, error.AsCString());
+      error.Clear();
+    }
     if (bytes_read == 0)
       continue;
     // We have a good memory region with valid bytes to store.

diff  --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index f02ec37cb0f08f..606518ca541267 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6325,8 +6325,11 @@ static bool AddDirtyPages(const MemoryRegionInfo &region,
 // ranges.
 static void AddRegion(const MemoryRegionInfo &region, bool try_dirty_pages,
                       Process::CoreFileMemoryRanges &ranges) {
-  // Don't add empty ranges or ranges with no permissions.
-  if (region.GetRange().GetByteSize() == 0 || region.GetLLDBPermissions() == 0)
+  // Don't add empty ranges.
+  if (region.GetRange().GetByteSize() == 0)
+    return;
+  // Don't add ranges with no read permissions.
+  if ((region.GetLLDBPermissions() & lldb::ePermissionsReadable) == 0)
     return;
   if (try_dirty_pages && AddDirtyPages(region, ranges))
     return;