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

[Miro Bucko via lldb-commits]

https://github.com/mbucko updated https://github.com/llvm/llvm-project/pull/88564

>From 3a69226e9ca90bb7ae220b9c3a71a0c2371e52fc Mon Sep 17 00:00:00 2001
From: Miro Bucko <mbucko at meta.com>
Date: Fri, 12 Apr 2024 09:55:46 -0700
Subject: [PATCH] [lldb][MinidumpFileBuilder] Fix addition of MemoryList steam

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. Also, one of the reasons why the invalid memory region was read was because the check for reading permission in AddRegion() was incorrect.

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

Reviewers:

Subscribers:

Tasks:

Tags:
---
 .../ObjectFile/Minidump/MinidumpFileBuilder.cpp       | 11 +++++++++--
 lldb/source/Target/Process.cpp                        |  7 +++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

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;