[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 8877be23ad4d342e7f9b61896581707005f76d4e 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  | 16 +++++++++++++---
 lldb/source/Target/Process.cpp                   |  7 +++++--
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 50d1b563f469cf..25a57c6a81b091 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -20,6 +20,8 @@
 #include "lldb/Target/StopInfo.h"
 #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"
@@ -649,16 +651,24 @@ 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 (bytes_read == 0)
+    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.
     LocationDescriptor memory_dump;
     memory_dump.DataSize = static_cast<llvm::support::ulittle32_t>(bytes_read);
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;