[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 27f7d03cfda9c6eea67973b9d8c3089abde8b732 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 | 17 ++++++++++++++---
 lldb/source/Target/Process.cpp                  | 11 +++++++----
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 50d1b563f469cf..9499d441ebe7ac 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,25 @@ 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::SystemRuntime);
+      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..84a0a65b0e4ebf 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -3857,8 +3857,8 @@ thread_result_t Process::RunPrivateStateThread(bool is_secondary_thread) {
         // case we should tell it to stop doing that.  Normally, we don't NEED
         // to do that because we will next close the communication to the stub
         // and that will get it to shut down.  But there are remote debugging
-        // cases where relying on that side-effect causes the shutdown to be 
-        // flakey, so we should send a positive signal to interrupt the wait. 
+        // cases where relying on that side-effect causes the shutdown to be
+        // flakey, so we should send a positive signal to interrupt the wait.
         Status error = HaltPrivate();
         BroadcastEvent(eBroadcastBitInterrupt, nullptr);
       } else if (StateIsRunningState(m_last_broadcast_state)) {
@@ -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;