[Lldb-commits] [lldb] 215bacb - Centralize the code that figures out which memory ranges to save into core files (#71772)

via lldb-commits lldb-commits at lists.llvm.org
Sat Nov 11 11:21:36 PST 2023


Author: Greg Clayton
Date: 2023-11-11T11:21:32-08:00
New Revision: 215bacb5dc6e7027402434a14e1153e687a4a1cf

URL: https://github.com/llvm/llvm-project/commit/215bacb5dc6e7027402434a14e1153e687a4a1cf
DIFF: https://github.com/llvm/llvm-project/commit/215bacb5dc6e7027402434a14e1153e687a4a1cf.diff

LOG: Centralize the code that figures out which memory ranges to save into core files (#71772)

Prior to this patch, each core file plugin (ObjectFileMachO.cpp and
ObjectFileMinindump.cpp) would calculate the address ranges to save in
different ways. This patch adds a new function to Process.h/.cpp:

```
Status Process::CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style, CoreFileMemoryRanges &ranges);
```

The patch updates the ObjectFileMachO::SaveCore(...) and
ObjectFileMinindump::SaveCore(...) to use same code. This will allow
core files to be consistent with the lldb::SaveCoreStyle across
different core file creators and will allow us to add new core file
saving features that do more complex things in future patches.

Added: 
    

Modified: 
    lldb/include/lldb/Target/MemoryRegionInfo.h
    lldb/include/lldb/Target/Process.h
    lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
    lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
    lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
    lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
    lldb/source/Target/Process.cpp
    lldb/source/Target/TraceDumper.cpp
    lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py

Removed: 
    


################################################################################
diff  --git a/lldb/include/lldb/Target/MemoryRegionInfo.h b/lldb/include/lldb/Target/MemoryRegionInfo.h
index 47d4c9d6398728c..66a4b3ed1b42d5f 100644
--- a/lldb/include/lldb/Target/MemoryRegionInfo.h
+++ b/lldb/include/lldb/Target/MemoryRegionInfo.h
@@ -81,11 +81,11 @@ class MemoryRegionInfo {
   // lldb::Permissions
   uint32_t GetLLDBPermissions() const {
     uint32_t permissions = 0;
-    if (m_read)
+    if (m_read == eYes)
       permissions |= lldb::ePermissionsReadable;
-    if (m_write)
+    if (m_write == eYes)
       permissions |= lldb::ePermissionsWritable;
-    if (m_execute)
+    if (m_execute == eYes)
       permissions |= lldb::ePermissionsExecutable;
     return permissions;
   }
@@ -151,7 +151,7 @@ class MemoryRegionInfo {
   int m_pagesize = 0;
   std::optional<std::vector<lldb::addr_t>> m_dirty_pages;
 };
-  
+
 inline bool operator<(const MemoryRegionInfo &lhs,
                       const MemoryRegionInfo &rhs) {
   return lhs.GetRange() < rhs.GetRange();

diff  --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index a6d3e6c2d16926e..08e3c60f7c324e6 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -54,6 +54,7 @@
 #include "lldb/Utility/UserIDResolver.h"
 #include "lldb/lldb-private.h"
 
+#include "llvm/ADT/AddressRanges.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/VersionTuple.h"
@@ -354,12 +355,10 @@ class Process : public std::enable_shared_from_this<Process>,
   };
   // This is all the event bits the public process broadcaster broadcasts.
   // The process shadow listener signs up for all these bits...
-  static constexpr int g_all_event_bits = eBroadcastBitStateChanged 
-                                        | eBroadcastBitInterrupt
-                                        | eBroadcastBitSTDOUT 
-                                        | eBroadcastBitSTDERR
-                                        | eBroadcastBitProfileData 
-                                        | eBroadcastBitStructuredData;
+  static constexpr int g_all_event_bits =
+      eBroadcastBitStateChanged | eBroadcastBitInterrupt | eBroadcastBitSTDOUT |
+      eBroadcastBitSTDERR | eBroadcastBitProfileData |
+      eBroadcastBitStructuredData;
 
   enum {
     eBroadcastInternalStateControlStop = (1 << 0),
@@ -390,7 +389,7 @@ class Process : public std::enable_shared_from_this<Process>,
   ConstString &GetBroadcasterClass() const override {
     return GetStaticBroadcasterClass();
   }
-  
+
 /// A notification structure that can be used by clients to listen
 /// for changes in a process's lifetime.
 ///
@@ -579,7 +578,7 @@ class Process : public std::enable_shared_from_this<Process>,
   ///     of CommandObject like CommandObjectRaw, CommandObjectParsed,
   ///     or CommandObjectMultiword.
   virtual CommandObject *GetPluginCommandObject() { return nullptr; }
-  
+
   /// The underlying plugin might store the low-level communication history for
   /// this session.  Dump it into the provided stream.
   virtual void DumpPluginHistory(Stream &s) { return; }
@@ -614,7 +613,7 @@ class Process : public std::enable_shared_from_this<Process>,
     return error;
   }
 
-  /// The "ShadowListener" for a process is just an ordinary Listener that 
+  /// The "ShadowListener" for a process is just an ordinary Listener that
   /// listens for all the Process event bits.  It's convenient because you can
   /// specify it in the LaunchInfo or AttachInfo, so it will get events from
   /// the very start of the process.
@@ -704,6 +703,35 @@ class Process : public std::enable_shared_from_this<Process>,
   ///     is not supported by the plugin, error otherwise.
   virtual llvm::Expected<bool> SaveCore(llvm::StringRef outfile);
 
+  struct CoreFileMemoryRange {
+    llvm::AddressRange range;  /// The address range to save into the core file.
+    uint32_t lldb_permissions; /// A bit set of lldb::Permissions bits.
+
+    bool operator==(const CoreFileMemoryRange &rhs) const {
+      return range == rhs.range && lldb_permissions == rhs.lldb_permissions;
+    }
+
+    bool operator!=(const CoreFileMemoryRange &rhs) const {
+      return !(*this == rhs);
+    }
+
+    bool operator<(const CoreFileMemoryRange &rhs) const {
+      if (range < rhs.range)
+        return true;
+      if (range == rhs.range)
+        return lldb_permissions < rhs.lldb_permissions;
+      return false;
+    }
+  };
+
+  using CoreFileMemoryRanges = std::vector<CoreFileMemoryRange>;
+
+  /// Helper function for Process::SaveCore(...) that calculates the address
+  /// ranges that should be saved. This allows all core file plug-ins to save
+  /// consistent memory ranges given a \a core_style.
+  Status CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
+                                     CoreFileMemoryRanges &ranges);
+
 protected:
   virtual JITLoaderList &GetJITLoaders();
 

diff  --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 1ea4f649427727f..24f3939a8f2ba5a 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6476,9 +6476,8 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
     return false;
 
   // Default on macOS is to create a dirty-memory-only corefile.
-  if (core_style == SaveCoreStyle::eSaveCoreUnspecified) {
+  if (core_style == SaveCoreStyle::eSaveCoreUnspecified)
     core_style = SaveCoreStyle::eSaveCoreDirtyOnly;
-  }
 
   Target &target = process_sp->GetTarget();
   const ArchSpec target_arch = target.GetArchitecture();
@@ -6507,115 +6506,42 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
     }
 
     if (make_core) {
-      std::vector<llvm::MachO::segment_command_64> segment_load_commands;
-      //                uint32_t range_info_idx = 0;
-      MemoryRegionInfo range_info;
-      Status range_error = process_sp->GetMemoryRegionInfo(0, range_info);
-      const uint32_t addr_byte_size = target_arch.GetAddressByteSize();
-      const ByteOrder byte_order = target_arch.GetByteOrder();
-      std::vector<page_object> pages_to_copy;
-
-      if (range_error.Success()) {
-        while (range_info.GetRange().GetRangeBase() != LLDB_INVALID_ADDRESS) {
-          // Calculate correct protections
-          uint32_t prot = 0;
-          if (range_info.GetReadable() == MemoryRegionInfo::eYes)
-            prot |= VM_PROT_READ;
-          if (range_info.GetWritable() == MemoryRegionInfo::eYes)
-            prot |= VM_PROT_WRITE;
-          if (range_info.GetExecutable() == MemoryRegionInfo::eYes)
-            prot |= VM_PROT_EXECUTE;
-
-          const addr_t addr = range_info.GetRange().GetRangeBase();
-          const addr_t size = range_info.GetRange().GetByteSize();
-
-          if (size == 0)
-            break;
-
-          bool include_this_region = true;
-          bool dirty_pages_only = false;
-          if (core_style == SaveCoreStyle::eSaveCoreStackOnly) {
-            dirty_pages_only = true;
-            if (range_info.IsStackMemory() != MemoryRegionInfo::eYes) {
-              include_this_region = false;
-            }
-          }
-          if (core_style == SaveCoreStyle::eSaveCoreDirtyOnly) {
-            dirty_pages_only = true;
-          }
-
-          if (prot != 0 && include_this_region) {
-            addr_t pagesize = range_info.GetPageSize();
-            const std::optional<std::vector<addr_t>> &dirty_page_list =
-                range_info.GetDirtyPageList();
-            if (dirty_pages_only && dirty_page_list) {
-              for (addr_t dirtypage : *dirty_page_list) {
-                page_object obj;
-                obj.addr = dirtypage;
-                obj.size = pagesize;
-                obj.prot = prot;
-                pages_to_copy.push_back(obj);
-              }
-            } else {
-              page_object obj;
-              obj.addr = addr;
-              obj.size = size;
-              obj.prot = prot;
-              pages_to_copy.push_back(obj);
-            }
-          }
-
-          range_error = process_sp->GetMemoryRegionInfo(
-              range_info.GetRange().GetRangeEnd(), range_info);
-          if (range_error.Fail())
-            break;
-        }
-
-        // Combine contiguous entries that have the same
-        // protections so we don't have an excess of
-        // load commands.
-        std::vector<page_object> combined_page_objects;
-        page_object last_obj;
-        last_obj.addr = LLDB_INVALID_ADDRESS;
-        last_obj.size = 0;
-        for (page_object obj : pages_to_copy) {
-          if (last_obj.addr == LLDB_INVALID_ADDRESS) {
-            last_obj = obj;
-            continue;
-          }
-          if (last_obj.addr + last_obj.size == obj.addr &&
-              last_obj.prot == obj.prot) {
-            last_obj.size += obj.size;
-            continue;
-          }
-          combined_page_objects.push_back(last_obj);
-          last_obj = obj;
-        }
-        // Add the last entry we were looking to combine
-        // on to the array.
-        if (last_obj.addr != LLDB_INVALID_ADDRESS && last_obj.size != 0)
-          combined_page_objects.push_back(last_obj);
-
-        for (page_object obj : combined_page_objects) {
+      Process::CoreFileMemoryRanges core_ranges;
+      error = process_sp->CalculateCoreFileSaveRanges(core_style, core_ranges);
+      if (error.Success()) {
+        const uint32_t addr_byte_size = target_arch.GetAddressByteSize();
+        const ByteOrder byte_order = target_arch.GetByteOrder();
+        std::vector<llvm::MachO::segment_command_64> segment_load_commands;
+        for (const auto &core_range : core_ranges) {
           uint32_t cmd_type = LC_SEGMENT_64;
           uint32_t segment_size = sizeof(llvm::MachO::segment_command_64);
           if (addr_byte_size == 4) {
             cmd_type = LC_SEGMENT;
             segment_size = sizeof(llvm::MachO::segment_command);
           }
+          // Skip any ranges with no read/write/execute permissions and empty
+          // ranges.
+          if (core_range.lldb_permissions == 0 || core_range.range.size() == 0)
+            continue;
+          uint32_t vm_prot = 0;
+          if (core_range.lldb_permissions & ePermissionsReadable)
+            vm_prot |= VM_PROT_READ;
+          if (core_range.lldb_permissions & ePermissionsWritable)
+            vm_prot |= VM_PROT_WRITE;
+          if (core_range.lldb_permissions & ePermissionsExecutable)
+            vm_prot |= VM_PROT_EXECUTE;
+          const addr_t vm_addr = core_range.range.start();
+          const addr_t vm_size = core_range.range.size();
           llvm::MachO::segment_command_64 segment = {
               cmd_type,     // uint32_t cmd;
               segment_size, // uint32_t cmdsize;
               {0},          // char segname[16];
-              obj.addr,     // uint64_t vmaddr;    // uint32_t for 32-bit
-                            // Mach-O
-              obj.size,     // uint64_t vmsize;    // uint32_t for 32-bit
-                            // Mach-O
-              0,            // uint64_t fileoff;   // uint32_t for 32-bit Mach-O
-              obj.size,     // uint64_t filesize;  // uint32_t for 32-bit
-                            // Mach-O
-              obj.prot,     // uint32_t maxprot;
-              obj.prot,     // uint32_t initprot;
+              vm_addr,      // uint64_t vmaddr;   // uint32_t for 32-bit Mach-O
+              vm_size,      // uint64_t vmsize;   // uint32_t for 32-bit Mach-O
+              0,            // uint64_t fileoff;  // uint32_t for 32-bit Mach-O
+              vm_size,      // uint64_t filesize; // uint32_t for 32-bit Mach-O
+              vm_prot,      // uint32_t maxprot;
+              vm_prot,      // uint32_t initprot;
               0,            // uint32_t nsects;
               0};           // uint32_t flags;
           segment_load_commands.push_back(segment);
@@ -6624,11 +6550,7 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
         StreamString buffer(Stream::eBinary, addr_byte_size, byte_order);
 
         llvm::MachO::mach_header_64 mach_header;
-        if (addr_byte_size == 8) {
-          mach_header.magic = MH_MAGIC_64;
-        } else {
-          mach_header.magic = MH_MAGIC;
-        }
+        mach_header.magic = addr_byte_size == 8 ? MH_MAGIC_64 : MH_MAGIC;
         mach_header.cputype = target_arch.GetMachOCPUType();
         mach_header.cpusubtype = target_arch.GetMachOCPUSubType();
         mach_header.filetype = MH_CORE;
@@ -6913,9 +6835,6 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
             }
           }
         }
-      } else {
-        error.SetErrorString(
-            "process doesn't support getting memory region info");
       }
     }
     return true; // This is the right plug to handle saving core files for

diff  --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index c396cb061c01776..e8e0d09b5324d0f 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -557,43 +557,24 @@ Status MinidumpFileBuilder::AddException(const lldb::ProcessSP &process_sp) {
 }
 
 lldb_private::Status
-MinidumpFileBuilder::AddMemoryList(const lldb::ProcessSP &process_sp) {
+MinidumpFileBuilder::AddMemoryList(const lldb::ProcessSP &process_sp,
+                                   lldb::SaveCoreStyle core_style) {
   Status error;
-
+  Process::CoreFileMemoryRanges core_ranges;
+  error = process_sp->CalculateCoreFileSaveRanges(core_style, core_ranges);
   if (error.Fail()) {
     error.SetErrorString("Process doesn't support getting memory region info.");
     return error;
   }
 
-  // Get interesting addresses
-  std::vector<size_t> interesting_addresses;
-  auto thread_list = process_sp->GetThreadList();
-  for (size_t i = 0; i < thread_list.GetSize(); ++i) {
-    ThreadSP thread_sp(thread_list.GetThreadAtIndex(i));
-    RegisterContextSP reg_ctx_sp(thread_sp->GetRegisterContext());
-    RegisterContext *reg_ctx = reg_ctx_sp.get();
-
-    interesting_addresses.push_back(read_register_u64(reg_ctx, "rsp"));
-    interesting_addresses.push_back(read_register_u64(reg_ctx, "rip"));
-  }
-
   DataBufferHeap helper_data;
   std::vector<MemoryDescriptor> mem_descriptors;
-
-  std::set<addr_t> visited_region_base_addresses;
-  for (size_t interesting_address : interesting_addresses) {
-    MemoryRegionInfo range_info;
-    error = process_sp->GetMemoryRegionInfo(interesting_address, range_info);
-    // Skip failed memory region requests or any regions with no permissions.
-    if (error.Fail() || range_info.GetLLDBPermissions() == 0)
-      continue;
-    const addr_t addr = range_info.GetRange().GetRangeBase();
-    // Skip any regions we have already saved out.
-    if (visited_region_base_addresses.insert(addr).second == false)
-      continue;
-    const addr_t size = range_info.GetRange().GetByteSize();
-    if (size == 0)
+  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)
       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);

diff  --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index f4017fb663840ec..cae355799fa7247 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -64,7 +64,8 @@ class MinidumpFileBuilder {
   // failed status.
   lldb_private::Status AddException(const lldb::ProcessSP &process_sp);
   // Add MemoryList stream, containing dumps of important memory segments
-  lldb_private::Status AddMemoryList(const lldb::ProcessSP &process_sp);
+  lldb_private::Status AddMemoryList(const lldb::ProcessSP &process_sp,
+                                     lldb::SaveCoreStyle core_style);
   // Add MiscInfo stream, mainly providing ProcessId
   void AddMiscInfo(const lldb::ProcessSP &process_sp);
   // Add informative files about a Linux process

diff  --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index 17b37afe557d914..f5294b2f08c66e1 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -57,10 +57,9 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
                                   const lldb_private::FileSpec &outfile,
                                   lldb::SaveCoreStyle &core_style,
                                   lldb_private::Status &error) {
-  if (core_style != SaveCoreStyle::eSaveCoreStackOnly) {
-    error.SetErrorString("Only stack minidumps supported yet.");
-    return false;
-  }
+  // Set default core style if it isn't set.
+  if (core_style == SaveCoreStyle::eSaveCoreUnspecified)
+    core_style = SaveCoreStyle::eSaveCoreStackOnly;
 
   if (!process_sp)
     return false;
@@ -88,7 +87,7 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
     if (error.Fail())
       return false;
 
-    error = builder.AddMemoryList(process_sp);
+    error = builder.AddMemoryList(process_sp, core_style);
     if (error.Fail())
       return false;
   }

diff  --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index f82ab05362fbee9..21b80b8240ab64b 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -2404,16 +2404,7 @@ bool Process::GetLoadAddressPermissions(lldb::addr_t load_addr,
       range_info.GetExecutable() == MemoryRegionInfo::eDontKnow) {
     return false;
   }
-
-  if (range_info.GetReadable() == MemoryRegionInfo::eYes)
-    permissions |= lldb::ePermissionsReadable;
-
-  if (range_info.GetWritable() == MemoryRegionInfo::eYes)
-    permissions |= lldb::ePermissionsWritable;
-
-  if (range_info.GetExecutable() == MemoryRegionInfo::eYes)
-    permissions |= lldb::ePermissionsExecutable;
-
+  permissions = range_info.GetLLDBPermissions();
   return true;
 }
 
@@ -6252,3 +6243,185 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
                            *packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo &region) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo &region,
+                          Process::CoreFileMemoryRanges &ranges) {
+  const auto &dirty_page_list = region.GetDirtyPageList();
+  if (!dirty_page_list)
+    return false;
+  const uint32_t lldb_permissions = region.GetLLDBPermissions();
+  const addr_t page_size = region.GetPageSize();
+  if (page_size == 0)
+    return false;
+  llvm::AddressRange range(0, 0);
+  for (addr_t page_addr : *dirty_page_list) {
+    if (range.empty()) {
+      // No range yet, initialize the range with the current dirty page.
+      range = llvm::AddressRange(page_addr, page_addr + page_size);
+    } else {
+      if (range.end() == page_addr) {
+        // Combine consective ranges.
+        range = llvm::AddressRange(range.start(), page_addr + page_size);
+      } else {
+        // Add previous contiguous range and init the new range with the
+        // current dirty page.
+        ranges.push_back({range, lldb_permissions});
+        range = llvm::AddressRange(page_addr, page_addr + page_size);
+      }
+    }
+  }
+  // The last range
+  if (!range.empty())
+    ranges.push_back({range, lldb_permissions});
+  return true;
+}
+
+// Given a region, add the region to \a ranges.
+//
+// Only add the region if it isn't empty and if it has some permissions.
+// If \a try_dirty_pages is true, then try to add only the dirty pages for a
+// given region. If the region has dirty page information, only dirty pages
+// will be added to \a ranges, else the entire range will be added to \a
+// 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)
+    return;
+  if (try_dirty_pages && AddDirtyPages(region, ranges))
+    return;
+  ranges.push_back(CreateCoreFileMemoryRange(region));
+}
+
+// Save all memory regions that are not empty or have at least some permissions
+// for a full core file style.
+static void GetCoreFileSaveRangesFull(Process &process,
+                                      const MemoryRegionInfos &regions,
+                                      Process::CoreFileMemoryRanges &ranges) {
+
+  // Don't add only dirty pages, add full regions.
+const bool try_dirty_pages = false;
+  for (const auto &region : regions)
+    AddRegion(region, try_dirty_pages, ranges);
+}
+
+// Save only the dirty pages to the core file. Make sure the process has at
+// least some dirty pages, as some OS versions don't support reporting what
+// pages are dirty within an memory region. If no memory regions have dirty
+// page information fall back to saving out all ranges with write permissions.
+static void
+GetCoreFileSaveRangesDirtyOnly(Process &process,
+                               const MemoryRegionInfos &regions,
+                               Process::CoreFileMemoryRanges &ranges) {
+  // Iterate over the regions and find all dirty pages.
+  bool have_dirty_page_info = false;
+  for (const auto &region : regions) {
+    if (AddDirtyPages(region, ranges))
+      have_dirty_page_info = true;
+  }
+
+  if (!have_dirty_page_info) {
+    // We didn't find support for reporting dirty pages from the process
+    // plug-in so fall back to any region with write access permissions.
+    const bool try_dirty_pages = false;
+    for (const auto &region : regions)
+      if (region.GetWritable() == MemoryRegionInfo::eYes)
+        AddRegion(region, try_dirty_pages, ranges);
+  }
+}
+
+// Save all thread stacks to the core file. Some OS versions support reporting
+// when a memory region is stack related. We check on this information, but we
+// also use the stack pointers of each thread and add those in case the OS
+// doesn't support reporting stack memory. This function also attempts to only
+// emit dirty pages from the stack if the memory regions support reporting
+// dirty regions as this will make the core file smaller. If the process
+// doesn't support dirty regions, then it will fall back to adding the full
+// stack region.
+static void
+GetCoreFileSaveRangesStackOnly(Process &process,
+                               const MemoryRegionInfos &regions,
+                               Process::CoreFileMemoryRanges &ranges) {
+  // Some platforms support annotating the region information that tell us that
+  // it comes from a thread stack. So look for those regions first.
+
+  // Keep track of which stack regions we have added
+  std::set<addr_t> stack_bases;
+
+  const bool try_dirty_pages = true;
+  for (const auto &region : regions) {
+    if (region.IsStackMemory() == MemoryRegionInfo::eYes) {
+      stack_bases.insert(region.GetRange().GetRangeBase());
+      AddRegion(region, try_dirty_pages, ranges);
+    }
+  }
+
+  // Also check with our threads and get the regions for their stack pointers
+  // and add those regions if not already added above.
+  for (lldb::ThreadSP thread_sp : process.GetThreadList().Threads()) {
+    if (!thread_sp)
+      continue;
+    StackFrameSP frame_sp = thread_sp->GetStackFrameAtIndex(0);
+    if (!frame_sp)
+      continue;
+    RegisterContextSP reg_ctx_sp = frame_sp->GetRegisterContext();
+    if (!reg_ctx_sp)
+      continue;
+    const addr_t sp = reg_ctx_sp->GetSP();
+    lldb_private::MemoryRegionInfo sp_region;
+    if (process.GetMemoryRegionInfo(sp, sp_region).Success()) {
+      // Only add this region if not already added above. If our stack pointer
+      // is pointing off in the weeds, we will want this range.
+      if (stack_bases.count(sp_region.GetRange().GetRangeBase()) == 0)
+        AddRegion(sp_region, try_dirty_pages, ranges);
+    }
+  }
+}
+
+Status Process::CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
+                                            CoreFileMemoryRanges &ranges) {
+  lldb_private::MemoryRegionInfos regions;
+  Status err = GetMemoryRegions(regions);
+  if (err.Fail())
+    return err;
+  if (regions.empty())
+    return Status("failed to get any valid memory regions from the process");
+
+  switch (core_style) {
+  case eSaveCoreUnspecified:
+    err = Status("callers must set the core_style to something other than "
+                 "eSaveCoreUnspecified");
+    break;
+
+  case eSaveCoreFull:
+    GetCoreFileSaveRangesFull(*this, regions, ranges);
+    break;
+
+  case eSaveCoreDirtyOnly:
+    GetCoreFileSaveRangesDirtyOnly(*this, regions, ranges);
+    break;
+
+  case eSaveCoreStackOnly:
+    GetCoreFileSaveRangesStackOnly(*this, regions, ranges);
+    break;
+  }
+
+  if (err.Fail())
+    return err;
+
+  if (ranges.empty())
+    return Status("no valid address ranges found for core style");
+
+  return Status(); // Success!
+}

diff  --git a/lldb/source/Target/TraceDumper.cpp b/lldb/source/Target/TraceDumper.cpp
index d059d443805c5af..e92419e70b32baa 100644
--- a/lldb/source/Target/TraceDumper.cpp
+++ b/lldb/source/Target/TraceDumper.cpp
@@ -472,7 +472,7 @@ TraceDumper::TraceItem TraceDumper::CreatRawTraceItem() {
 static SymbolContext
 CalculateSymbolContext(const Address &address,
                        const SymbolContext &prev_symbol_context) {
-  AddressRange range;
+  lldb_private::AddressRange range;
   if (prev_symbol_context.GetAddressRange(eSymbolContextEverything, 0,
                                           /*inline_block_range*/ true, range) &&
       range.Contains(address))
@@ -508,7 +508,8 @@ CalculateDisass(const TraceDumper::SymbolInfo &symbol_info,
   // We fallback to a single instruction disassembler
   Target &target = exe_ctx.GetTargetRef();
   const ArchSpec arch = target.GetArchitecture();
-  AddressRange range(symbol_info.address, arch.GetMaximumOpcodeByteSize());
+  lldb_private::AddressRange range(symbol_info.address,
+                                   arch.GetMaximumOpcodeByteSize());
   DisassemblerSP disassembler =
       Disassembler::DisassembleRange(arch, /*plugin_name*/ nullptr,
                                      /*flavor*/ nullptr, target, range);
@@ -778,7 +779,7 @@ static TraceDumper::FunctionCall &AppendInstructionToFunctionCallForest(
     return *roots.back();
   }
 
-  AddressRange range;
+  lldb_private::AddressRange range;
   if (symbol_info.sc.GetAddressRange(
           eSymbolContextBlock | eSymbolContextFunction | eSymbolContextSymbol,
           0, /*inline_block_range*/ true, range)) {

diff  --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
index 04a395ade1bb4f8..7c04166b85fffe6 100644
--- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
+++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
@@ -11,14 +11,50 @@
 
 
 class ProcessSaveCoreMinidumpTestCase(TestBase):
+
+    def verify_core_file(self, core_path, expected_pid, expected_modules,
+                         expected_threads):
+        # To verify, we'll launch with the mini dump
+        target = self.dbg.CreateTarget(None)
+        process = target.LoadCore(core_path)
+
+        # check if the core is in desired state
+        self.assertTrue(process, PROCESS_IS_VALID)
+        self.assertTrue(process.GetProcessInfo().IsValid())
+        self.assertEqual(process.GetProcessInfo().GetProcessID(), expected_pid)
+        self.assertTrue(target.GetTriple().find("linux") != -1)
+        self.assertTrue(target.GetNumModules(), len(expected_modules))
+        self.assertEqual(process.GetNumThreads(), len(expected_threads))
+
+        for module, expected in zip(target.modules, expected_modules):
+            self.assertTrue(module.IsValid())
+            module_file_name = module.GetFileSpec().GetFilename()
+            expected_file_name = expected.GetFileSpec().GetFilename()
+            # skip kernel virtual dynamic shared objects
+            if "vdso" in expected_file_name:
+                continue
+            self.assertEqual(module_file_name, expected_file_name)
+            self.assertEqual(module.GetUUIDString(), expected.GetUUIDString())
+
+        for thread_idx in range(process.GetNumThreads()):
+            thread = process.GetThreadAtIndex(thread_idx)
+            self.assertTrue(thread.IsValid())
+            thread_id = thread.GetThreadID()
+            self.assertTrue(thread_id in expected_threads)
+        self.dbg.DeleteTarget(target)
+
     @skipUnlessArch("x86_64")
     @skipUnlessPlatform(["linux"])
     def test_save_linux_mini_dump(self):
         """Test that we can save a Linux mini dump."""
         self.build()
         exe = self.getBuildArtifact("a.out")
-        core = self.getBuildArtifact("core.dmp")
-        core_sb = self.getBuildArtifact("core_sb.dmp")
+        core_stack = self.getBuildArtifact("core.stack.dmp")
+        core_dirty = self.getBuildArtifact("core.dirty.dmp")
+        core_full = self.getBuildArtifact("core.full.dmp")
+        core_sb_stack = self.getBuildArtifact("core_sb.stack.dmp")
+        core_sb_dirty = self.getBuildArtifact("core_sb.dirty.dmp")
+        core_sb_full = self.getBuildArtifact("core_sb.full.dmp")
         try:
             target = self.dbg.CreateTarget(exe)
             process = target.LaunchSimple(
@@ -40,54 +76,65 @@ def test_save_linux_mini_dump(self):
                 expected_threads.append(thread_id)
 
             # save core and, kill process and verify corefile existence
+            base_command = "process save-core --plugin-name=minidump "
             self.runCmd(
-                "process save-core --plugin-name=minidump --style=stack " + core
+                base_command + " --style=stack '%s'" % (core_stack)
             )
-            self.assertTrue(os.path.isfile(core))
+            self.assertTrue(os.path.isfile(core_stack))
+            self.verify_core_file(core_stack, expected_pid, expected_modules,
+                                  expected_threads)
 
-            # validate savinig via SBProcess
-            error = process.SaveCore(core_sb, "minidump", lldb.eSaveCoreStackOnly)
-            self.assertTrue(error.Success())
-            self.assertTrue(os.path.isfile(core_sb))
+            self.runCmd(
+                base_command + " --style=modified-memory '%s'" % (core_dirty)
+            )
+            self.assertTrue(os.path.isfile(core_dirty))
+            self.verify_core_file(core_dirty, expected_pid, expected_modules,
+                                  expected_threads)
 
-            error = process.SaveCore(core_sb, "minidump", lldb.eSaveCoreFull)
-            self.assertTrue(error.Fail())
-            error = process.SaveCore(core_sb, "minidump", lldb.eSaveCoreDirtyOnly)
-            self.assertTrue(error.Fail())
+            self.runCmd(
+                base_command + " --style=full '%s'" % (core_full)
+            )
+            self.assertTrue(os.path.isfile(core_full))
+            self.verify_core_file(core_full, expected_pid, expected_modules,
+                                  expected_threads)
 
-            self.assertSuccess(process.Kill())
+            # validate saving via SBProcess
+            error = process.SaveCore(core_sb_stack, "minidump",
+                                     lldb.eSaveCoreStackOnly)
+            self.assertTrue(error.Success())
+            self.assertTrue(os.path.isfile(core_sb_stack))
+            self.verify_core_file(core_sb_stack, expected_pid,
+                                  expected_modules, expected_threads)
 
-            # To verify, we'll launch with the mini dump
-            target = self.dbg.CreateTarget(None)
-            process = target.LoadCore(core)
-
-            # check if the core is in desired state
-            self.assertTrue(process, PROCESS_IS_VALID)
-            self.assertTrue(process.GetProcessInfo().IsValid())
-            self.assertEqual(process.GetProcessInfo().GetProcessID(), expected_pid)
-            self.assertTrue(target.GetTriple().find("linux") != -1)
-            self.assertTrue(target.GetNumModules(), expected_number_of_modules)
-            self.assertEqual(process.GetNumThreads(), expected_number_of_threads)
-
-            for module, expected in zip(target.modules, expected_modules):
-                self.assertTrue(module.IsValid())
-                module_file_name = module.GetFileSpec().GetFilename()
-                expected_file_name = expected.GetFileSpec().GetFilename()
-                # skip kernel virtual dynamic shared objects
-                if "vdso" in expected_file_name:
-                    continue
-                self.assertEqual(module_file_name, expected_file_name)
-                self.assertEqual(module.GetUUIDString(), expected.GetUUIDString())
+            error = process.SaveCore(core_sb_dirty, "minidump",
+                                     lldb.eSaveCoreDirtyOnly)
+            self.assertTrue(error.Success())
+            self.assertTrue(os.path.isfile(core_sb_dirty))
+            self.verify_core_file(core_sb_dirty, expected_pid, expected_modules,
+                                  expected_threads)
 
-            for thread_idx in range(process.GetNumThreads()):
-                thread = process.GetThreadAtIndex(thread_idx)
-                self.assertTrue(thread.IsValid())
-                thread_id = thread.GetThreadID()
-                self.assertTrue(thread_id in expected_threads)
+            # Minidump can now save full core files, but they will be huge and
+            # they might cause this test to timeout.
+            error = process.SaveCore(core_sb_full, "minidump",
+                                     lldb.eSaveCoreFull)
+            self.assertTrue(error.Success())
+            self.assertTrue(os.path.isfile(core_sb_full))
+            self.verify_core_file(core_sb_full, expected_pid, expected_modules,
+                                  expected_threads)
+
+            self.assertSuccess(process.Kill())
         finally:
             # Clean up the mini dump file.
             self.assertTrue(self.dbg.DeleteTarget(target))
-            if os.path.isfile(core):
-                os.unlink(core)
-            if os.path.isfile(core_sb):
-                os.unlink(core_sb)
+            if os.path.isfile(core_stack):
+                os.unlink(core_stack)
+            if os.path.isfile(core_dirty):
+                os.unlink(core_dirty)
+            if os.path.isfile(core_full):
+                os.unlink(core_full)
+            if os.path.isfile(core_sb_stack):
+                os.unlink(core_sb_stack)
+            if os.path.isfile(core_sb_dirty):
+                os.unlink(core_sb_dirty)
+            if os.path.isfile(core_sb_full):
+                os.unlink(core_sb_full)


        


More information about the lldb-commits mailing list