[LLDB] Reapply SBSaveCore Add Memory List (#107937)

Recently in #107731 this change was revereted due to excess memory size
in `TestSkinnyCore`. This was due to a bug where a range's end was being
passed as size. Creating massive memory ranges.

Additionally, and requiring additional review, I added more unit tests
and more verbose logic to the merging of save core memory regions.

@jasonmolenda as an FYI.

lldb/include/lldb/API/SBMemoryRegionInfo.h

@@ -120,7 +120,7 @@ public:
 private:
   friend class SBProcess;
   friend class SBMemoryRegionInfoList;
-
+  friend class SBSaveCoreOptions;
   friend class lldb_private::ScriptInterpreter;
 
   lldb_private::MemoryRegionInfo &ref();

lldb/include/lldb/API/SBSaveCoreOptions.h

@@ -80,6 +80,17 @@ public:
   /// \return True if the thread was removed, false if it was not in the list.
   bool RemoveThread(lldb::SBThread thread);
 
+  /// Add a memory region to save in the core file.
+  ///
+  /// \param region The memory region to save.
+  /// \returns An empty SBError upon success, or an error if the region is
+  /// invalid.
+  /// \note Ranges that overlapped will be unioned into a single region, this
+  /// also supercedes stack minification. Specifying full regions and a
+  /// non-custom core style will include the specified regions and union them
+  /// with all style specific regions.
+  SBError AddMemoryRegionToSave(const SBMemoryRegionInfo &region);
+
   /// Reset all options.
   void Clear();
 

lldb/include/lldb/Symbol/SaveCoreOptions.h

@@ -10,13 +10,15 @@
 #define LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H
 
 #include "lldb/Utility/FileSpec.h"
-#include "lldb/lldb-forward.h"
-#include "lldb/lldb-types.h"
+#include "lldb/Utility/RangeMap.h"
 
 #include <optional>
+#include <set>
 #include <string>
 #include <unordered_set>
 
+using MemoryRanges = lldb_private::RangeVector<lldb::addr_t, lldb::addr_t>;
+
 namespace lldb_private {
 
 class SaveCoreOptions {
@@ -38,8 +40,12 @@ public:
   Status AddThread(lldb::ThreadSP thread_sp);
   bool RemoveThread(lldb::ThreadSP thread_sp);
   bool ShouldThreadBeSaved(lldb::tid_t tid) const;
+  bool HasSpecifiedThreads() const;
 
   Status EnsureValidConfiguration(lldb::ProcessSP process_sp) const;
+  const MemoryRanges &GetCoreFileMemoryRanges() const;
+
+  void AddMemoryRegionToSave(const lldb_private::MemoryRegionInfo &region);
 
   void Clear();
 
@@ -51,6 +57,7 @@ private:
   std::optional<lldb::SaveCoreStyle> m_style;
   lldb::ProcessSP m_process_sp;
   std::unordered_set<lldb::tid_t> m_threads_to_save;
+  MemoryRanges m_regions_to_save;
 };
 } // namespace lldb_private
 

lldb/include/lldb/Target/CoreFileMemoryRanges.h

@@ -0,0 +1,50 @@
+//===-- CoreFileMemoryRanges.h ----------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Utility/RangeMap.h"
+#include "lldb/Utility/Status.h"
+
+#include "llvm/ADT/AddressRanges.h"
+
+#ifndef LLDB_TARGET_COREFILEMEMORYRANGES_H
+#define LLDB_TARGET_COREFILEMEMORYRANGES_H
+
+namespace lldb_private {
+
+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;
+  }
+};
+
+class CoreFileMemoryRanges
+    : public lldb_private::RangeDataVector<lldb::addr_t, lldb::addr_t,
+                                           CoreFileMemoryRange> {
+public:
+  /// Finalize and merge all overlapping ranges in this collection. Ranges
+  /// will be seperated based on permissions.
+  Status FinalizeCoreFileSaveRanges();
+};
+} // namespace lldb_private
+
+#endif // LLDB_TARGET_COREFILEMEMORYRANGES_H

lldb/include/lldb/Target/Process.h

@@ -35,6 +35,8 @@
 #include "lldb/Host/ProcessLaunchInfo.h"
 #include "lldb/Host/ProcessRunLock.h"
 #include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
+#include "lldb/Target/CoreFileMemoryRanges.h"
 #include "lldb/Target/ExecutionContextScope.h"
 #include "lldb/Target/InstrumentationRuntime.h"
 #include "lldb/Target/Memory.h"
@@ -710,29 +712,6 @@ public:
   ///     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.

lldb/include/lldb/Utility/RangeMap.h

@@ -450,6 +450,12 @@ public:
 
   void Append(const Entry &entry) { m_entries.emplace_back(entry); }
 
+  /// Append a range with data to the vector
+  /// \param B The base of the memory range
+  /// \param S The size of the memory range
+  /// \param T The data associated with the memory range
+  void Append(B &&b, S &&s, T &&t) { m_entries.emplace_back(Entry(b, s, t)); }
+
   bool Erase(uint32_t start, uint32_t end) {
     if (start >= end || end > m_entries.size())
       return false;

lldb/include/lldb/lldb-enumerations.h

@@ -1222,6 +1222,7 @@ enum SaveCoreStyle {
   eSaveCoreFull = 1,
   eSaveCoreDirtyOnly = 2,
   eSaveCoreStackOnly = 3,
+  eSaveCoreCustomOnly = 4,
 };
 
 /// Events that might happen during a trace session.

lldb/include/lldb/lldb-forward.h

@@ -207,6 +207,7 @@ class StackFrameRecognizer;
 class StackFrameRecognizerManager;
 class StackID;
 class Status;
+class SaveCoreOptions;
 class StopInfo;
 class Stoppoint;
 class StoppointCallbackContext;

lldb/include/lldb/lldb-private-interfaces.h

@@ -9,7 +9,6 @@
 #ifndef LLDB_LLDB_PRIVATE_INTERFACES_H
 #define LLDB_LLDB_PRIVATE_INTERFACES_H
 
-#include "lldb/Symbol/SaveCoreOptions.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-private-enumerations.h"

lldb/source/API/SBSaveCoreOptions.cpp

@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "lldb/API/SBSaveCoreOptions.h"
+#include "lldb/API/SBMemoryRegionInfo.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Symbol/SaveCoreOptions.h"
 #include "lldb/Utility/Instrumentation.h"
@@ -89,6 +90,16 @@ bool SBSaveCoreOptions::RemoveThread(lldb::SBThread thread) {
   return m_opaque_up->RemoveThread(thread.GetSP());
 }
 
+lldb::SBError
+SBSaveCoreOptions::AddMemoryRegionToSave(const SBMemoryRegionInfo &region) {
+  LLDB_INSTRUMENT_VA(this, region);
+  // Currently add memory region can't fail, so we always return a success
+  // SBerror, but because these API's live forever, this is the most future
+  // proof thing to do.
+  m_opaque_up->AddMemoryRegionToSave(region.ref());
+  return SBError();
+}
+
 void SBSaveCoreOptions::Clear() {
   LLDB_INSTRUMENT_VA(this);
   m_opaque_up->Clear();

lldb/source/Commands/CommandObjectProcess.cpp

@@ -25,6 +25,7 @@
 #include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/Interpreter/OptionGroupPythonClassWithDict.h"
 #include "lldb/Interpreter/Options.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
 #include "lldb/Target/Platform.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StopInfo.h"

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

@@ -6562,13 +6562,15 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
     }
 
     if (make_core) {
-      Process::CoreFileMemoryRanges core_ranges;
+      CoreFileMemoryRanges core_ranges;
       error = process_sp->CalculateCoreFileSaveRanges(options, 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) {
+        for (const auto &core_range_info : core_ranges) {
+          // TODO: Refactor RangeDataVector to have a data iterator.
+          const auto &core_range = core_range_info.data;
           uint32_t cmd_type = LC_SEGMENT_64;
           uint32_t segment_size = sizeof(llvm::MachO::segment_command_64);
           if (addr_byte_size == 4) {

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h

@@ -12,6 +12,7 @@
 #include "lldb/Core/Address.h"
 #include "lldb/Host/SafeMachO.h"
 #include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/FileSpecList.h"
 #include "lldb/Utility/RangeMap.h"

lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp

@@ -831,25 +831,32 @@ Status MinidumpFileBuilder::AddMemoryList() {
   // bytes of the core file. Thread structures in minidump files can only use
   // 32 bit memory descriptiors, so we emit them first to ensure the memory is
   // in accessible with a 32 bit offset.
-  Process::CoreFileMemoryRanges ranges_32;
-  Process::CoreFileMemoryRanges ranges_64;
-  Process::CoreFileMemoryRanges all_core_memory_ranges;
+  std::vector<CoreFileMemoryRange> ranges_32;
+  std::vector<CoreFileMemoryRange> ranges_64;
+  CoreFileMemoryRanges all_core_memory_ranges;
   error = m_process_sp->CalculateCoreFileSaveRanges(m_save_core_options,
                                                     all_core_memory_ranges);
+
+  std::vector<CoreFileMemoryRange> all_core_memory_vec;
+  // Extract all the data into just a vector of data. So we can mutate this in
+  // place.
+  for (const auto &core_range : all_core_memory_ranges)
+    all_core_memory_vec.push_back(core_range.data);
+
   if (error.Fail())
     return error;
 
   // Start by saving all of the stacks and ensuring they fit under the 32b
   // limit.
   uint64_t total_size = GetCurrentDataEndOffset();
-  auto iterator = all_core_memory_ranges.begin();
-  while (iterator != all_core_memory_ranges.end()) {
+  auto iterator = all_core_memory_vec.begin();
+  while (iterator != all_core_memory_vec.end()) {
     if (m_saved_stack_ranges.count(iterator->range.start()) > 0) {
       // We don't save stacks twice.
       ranges_32.push_back(*iterator);
       total_size +=
           iterator->range.size() + sizeof(llvm::minidump::MemoryDescriptor);
-      iterator = all_core_memory_ranges.erase(iterator);
+      iterator = all_core_memory_vec.erase(iterator);
     } else {
       iterator++;
     }
@@ -869,11 +876,11 @@ Status MinidumpFileBuilder::AddMemoryList() {
   // Then anything overflow extends into 64b addressable space.
   // All core memeroy ranges will either container nothing on stacks only
   // or all the memory ranges including stacks
-  if (!all_core_memory_ranges.empty())
-    total_size += 256 + (all_core_memory_ranges.size() *
+  if (!all_core_memory_vec.empty())
+    total_size += 256 + (all_core_memory_vec.size() *
                          sizeof(llvm::minidump::MemoryDescriptor_64));
 
-  for (const auto &core_range : all_core_memory_ranges) {
+  for (const auto &core_range : all_core_memory_vec) {
     const addr_t range_size = core_range.range.size();
     // We don't need to check for stacks here because we already removed them
     // from all_core_memory_ranges.
@@ -958,15 +965,15 @@ Status MinidumpFileBuilder::DumpDirectories() const {
 }
 
 static uint64_t
-GetLargestRangeSize(const Process::CoreFileMemoryRanges &ranges) {
+GetLargestRangeSize(const std::vector<CoreFileMemoryRange> &ranges) {
   uint64_t max_size = 0;
   for (const auto &core_range : ranges)
     max_size = std::max(max_size, core_range.range.size());
   return max_size;
 }
 
-Status
-MinidumpFileBuilder::AddMemoryList_32(Process::CoreFileMemoryRanges &ranges) {
+Status MinidumpFileBuilder::AddMemoryList_32(
+    std::vector<CoreFileMemoryRange> &ranges) {
   std::vector<MemoryDescriptor> descriptors;
   Status error;
   if (ranges.size() == 0)
@@ -1042,8 +1049,8 @@ MinidumpFileBuilder::AddMemoryList_32(Process::CoreFileMemoryRanges &ranges) {
   return error;
 }
 
-Status
-MinidumpFileBuilder::AddMemoryList_64(Process::CoreFileMemoryRanges &ranges) {
+Status MinidumpFileBuilder::AddMemoryList_64(
+    std::vector<CoreFileMemoryRange> &ranges) {
   Status error;
   if (ranges.empty())
     return error;

lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h

@@ -23,6 +23,7 @@
 #include <utility>
 #include <variant>
 
+#include "lldb/Symbol/SaveCoreOptions.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataBufferHeap.h"
@@ -120,9 +121,9 @@ private:
   lldb_private::Status AddData(const void *data, uint64_t size);
   // Add MemoryList stream, containing dumps of important memory segments
   lldb_private::Status
-  AddMemoryList_64(lldb_private::Process::CoreFileMemoryRanges &ranges);
+  AddMemoryList_64(std::vector<lldb_private::CoreFileMemoryRange> &ranges);
   lldb_private::Status
-  AddMemoryList_32(lldb_private::Process::CoreFileMemoryRanges &ranges);
+  AddMemoryList_32(std::vector<lldb_private::CoreFileMemoryRange> &ranges);
   // Update the thread list on disk with the newly emitted stack RVAs.
   lldb_private::Status FixThreadStacks();
   lldb_private::Status FlushBufferToDisk();

lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h

@@ -21,6 +21,7 @@
 #define LLDB_SOURCE_PLUGINS_OBJECTFILE_MINIDUMP_OBJECTFILEMINIDUMP_H
 
 #include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
 #include "lldb/Utility/ArchSpec.h"
 
 class ObjectFileMinidump : public lldb_private::PluginInterface {

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

@@ -17,6 +17,7 @@
 #include "lldb/Interpreter/OptionValueDictionary.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
 #include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h

@@ -13,6 +13,7 @@
 #include <vector>
 
 #include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Symbol/SaveCoreOptions.h"
 #include "llvm/Object/COFF.h"
 
 class ObjectFilePECOFF : public lldb_private::ObjectFile {

lldb/source/Symbol/SaveCoreOptions.cpp

@@ -102,6 +102,19 @@ bool SaveCoreOptions::ShouldThreadBeSaved(lldb::tid_t tid) const {
   return m_threads_to_save.count(tid) > 0;
 }
 
+bool SaveCoreOptions::HasSpecifiedThreads() const {
+  return !m_threads_to_save.empty();
+}
+
+void SaveCoreOptions::AddMemoryRegionToSave(
+    const lldb_private::MemoryRegionInfo &region) {
+  m_regions_to_save.Insert(region.GetRange(), /*combine=*/true);
+}
+
+const MemoryRanges &SaveCoreOptions::GetCoreFileMemoryRanges() const {
+  return m_regions_to_save;
+}
+
 Status SaveCoreOptions::EnsureValidConfiguration(
     lldb::ProcessSP process_sp) const {
   Status error;
@@ -131,4 +144,5 @@ void SaveCoreOptions::Clear() {
   m_style = std::nullopt;
   m_threads_to_save.clear();
   m_process_sp.reset();
+  m_regions_to_save.Clear();
 }

lldb/source/Target/CMakeLists.txt

@@ -11,6 +11,7 @@ add_lldb_library(lldbTarget
   ABI.cpp
   AssertFrameRecognizer.cpp
   DynamicRegisterInfo.cpp
+  CoreFileMemoryRanges.cpp
   ExecutionContext.cpp
   InstrumentationRuntime.cpp
   InstrumentationRuntimeStopInfo.cpp

lldb/source/Target/CoreFileMemoryRanges.cpp

@@ -0,0 +1,86 @@
+//===-- CoreFileMemoryRanges.cpp --------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Target/CoreFileMemoryRanges.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+using Entry = CoreFileMemoryRanges::Entry;
+
+static bool Overlaps(const Entry *region_one, const Entry *region_two) {
+  return !(region_one->GetRangeEnd() < region_two->GetRangeBase() ||
+           region_two->GetRangeEnd() < region_one->GetRangeBase());
+}
+
+static bool IntersectHelper(const Entry *region_one, const Entry *region_two) {
+  return region_one->GetRangeBase() == region_two->GetRangeEnd() ||
+         region_one->GetRangeEnd() == region_two->GetRangeBase();
+}
+
+static bool OnlyIntersects(const Entry *region_one, const Entry *region_two) {
+  return IntersectHelper(region_one, region_two) ||
+         IntersectHelper(region_two, region_one);
+}
+
+static bool PermissionsMatch(const Entry *region_one, const Entry *region_two) {
+  return region_one->data.lldb_permissions == region_two->data.lldb_permissions;
+}
+
+// This assumes any overlapping ranges will share the same permissions
+// and that adjacent ranges could have different permissions.
+Status CoreFileMemoryRanges::FinalizeCoreFileSaveRanges() {
+  Status error;
+  this->Sort();
+  for (size_t i = this->GetSize() - 1; i > 0; i--) {
+    auto region_one = this->GetMutableEntryAtIndex(i);
+    auto region_two = this->GetMutableEntryAtIndex(i - 1);
+    if (Overlaps(region_one, region_two)) {
+      // It's okay for interesecting regions to have different permissions but
+      // if they overlap we fail because we don't know what to do with them.
+      if (!PermissionsMatch(region_one, region_two)) {
+        // Permissions mismatch and it's not a simple intersection.
+        if (!OnlyIntersects(region_one, region_two)) {
+          error = Status::FromErrorStringWithFormatv(
+              "Memory region at {0}::{1} has different permssions than "
+              "overlapping region at {2}::{3}",
+              region_one->GetRangeBase(), region_one->GetRangeEnd(),
+              region_two->GetRangeBase(), region_two->GetRangeEnd());
+          return error;
+        }
+        // Simple intersection, we can just not merge these.
+        else
+          continue;
+      }
+      const addr_t base =
+          std::min(region_one->GetRangeBase(), region_two->GetRangeBase());
+      const addr_t byte_size =
+          std::max(region_one->GetRangeEnd(), region_two->GetRangeEnd()) - base;
+
+      region_two->SetRangeBase(base);
+      region_two->SetByteSize(byte_size);
+
+      // Because this is a range data vector, the entry has a base as well
+      // as the data contained in the entry. So we have to update both.
+      // And llvm::AddressRange isn't mutable so we have to create a new one.
+      llvm::AddressRange range(base, base + byte_size);
+      const CoreFileMemoryRange core_range = {
+          range, region_two->data.lldb_permissions};
+      region_two->data = core_range;
+      // Erase is delete from [Inclusive, exclusive index).
+      if (!this->Erase(i, i + 1)) {
+        error = Status::FromErrorStringWithFormat(
+            "Core file memory ranges mutated outside of "
+            "CalculateCoreFileSaveRanges");
+        return error;
+      }
+    }
+  }
+
+  return error;
+}

lldb/source/Target/Process.cpp

@@ -6463,7 +6463,7 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, size_t len,
 }
 
 // Create a CoreFileMemoryRange from a MemoryRegionInfo
-static Process::CoreFileMemoryRange
+static CoreFileMemoryRange
 CreateCoreFileMemoryRange(const MemoryRegionInfo &region) {
   const addr_t addr = region.GetRange().GetRangeBase();
   llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
@@ -6474,7 +6474,7 @@ CreateCoreFileMemoryRange(const MemoryRegionInfo &region) {
 // 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) {
+                          CoreFileMemoryRanges &ranges) {
   const auto &dirty_page_list = region.GetDirtyPageList();
   if (!dirty_page_list)
     return false;
@@ -6494,14 +6494,14 @@ static bool AddDirtyPages(const MemoryRegionInfo &region,
       } else {
         // Add previous contiguous range and init the new range with the
         // current dirty page.
-        ranges.push_back({range, lldb_permissions});
+        ranges.Append(range.start(), range.size(), {range, lldb_permissions});
         range = llvm::AddressRange(page_addr, page_addr + page_size);
       }
     }
   }
   // The last range
   if (!range.empty())
-    ranges.push_back({range, lldb_permissions});
+    ranges.Append(range.start(), range.size(), {range, lldb_permissions});
   return true;
 }
 
@@ -6513,7 +6513,7 @@ static bool AddDirtyPages(const MemoryRegionInfo &region,
 // 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) {
+                      CoreFileMemoryRanges &ranges) {
   // Don't add empty ranges.
   if (region.GetRange().GetByteSize() == 0)
     return;
@@ -6522,13 +6522,17 @@ static void AddRegion(const MemoryRegionInfo &region, bool try_dirty_pages,
     return;
   if (try_dirty_pages && AddDirtyPages(region, ranges))
     return;
-  ranges.push_back(CreateCoreFileMemoryRange(region));
+
+  ranges.Append(region.GetRange().GetRangeBase(),
+                region.GetRange().GetByteSize(),
+                CreateCoreFileMemoryRange(region));
 }
 
-static void SaveOffRegionsWithStackPointers(
-    Process &process, const SaveCoreOptions &core_options,
-    const MemoryRegionInfos &regions, Process::CoreFileMemoryRanges &ranges,
-    std::set<addr_t> &stack_ends) {
+static void SaveOffRegionsWithStackPointers(Process &process,
+                                            const SaveCoreOptions &core_options,
+                                            const MemoryRegionInfos &regions,
+                                            CoreFileMemoryRanges &ranges,
+                                            std::set<addr_t> &stack_ends) {
   const bool try_dirty_pages = true;
 
   // Before we take any dump, we want to save off the used portions of the
@@ -6568,11 +6572,11 @@ static void SaveOffRegionsWithStackPointers(
 // for a full core file style.
 static void GetCoreFileSaveRangesFull(Process &process,
                                       const MemoryRegionInfos &regions,
-                                      Process::CoreFileMemoryRanges &ranges,
+                                      CoreFileMemoryRanges &ranges,
                                       std::set<addr_t> &stack_ends) {
 
   // Don't add only dirty pages, add full regions.
-const bool try_dirty_pages = false;
+  const bool try_dirty_pages = false;
   for (const auto &region : regions)
     if (stack_ends.count(region.GetRange().GetRangeEnd()) == 0)
       AddRegion(region, try_dirty_pages, ranges);
@@ -6582,9 +6586,10 @@ const bool try_dirty_pages = false;
 // 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, std::set<addr_t> &stack_ends) {
+static void GetCoreFileSaveRangesDirtyOnly(Process &process,
+                                           const MemoryRegionInfos &regions,
+                                           CoreFileMemoryRanges &ranges,
+                                           std::set<addr_t> &stack_ends) {
 
   // Iterate over the regions and find all dirty pages.
   bool have_dirty_page_info = false;
@@ -6613,9 +6618,10 @@ static void GetCoreFileSaveRangesDirtyOnly(
 // 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, std::set<addr_t> &stack_ends) {
+static void GetCoreFileSaveRangesStackOnly(Process &process,
+                                           const MemoryRegionInfos &regions,
+                                           CoreFileMemoryRanges &ranges,
+                                           std::set<addr_t> &stack_ends) {
   const bool try_dirty_pages = true;
   // Some platforms support annotating the region information that tell us that
   // it comes from a thread stack. So look for those regions first.
@@ -6628,6 +6634,24 @@ static void GetCoreFileSaveRangesStackOnly(
   }
 }
 
+static void GetUserSpecifiedCoreFileSaveRanges(Process &process,
+                                               const MemoryRegionInfos &regions,
+                                               const SaveCoreOptions &options,
+                                               CoreFileMemoryRanges &ranges) {
+  const auto &option_ranges = options.GetCoreFileMemoryRanges();
+  if (option_ranges.IsEmpty())
+    return;
+
+  for (const auto &range : regions) {
+    auto entry = option_ranges.FindEntryThatContains(range.GetRange());
+    if (entry) {
+      ranges.Append(range.GetRange().GetRangeBase(),
+                    range.GetRange().GetByteSize(),
+                    CreateCoreFileMemoryRange(range));
+    }
+  }
+}
+
 Status Process::CalculateCoreFileSaveRanges(const SaveCoreOptions &options,
                                             CoreFileMemoryRanges &ranges) {
   lldb_private::MemoryRegionInfos regions;
@@ -6643,11 +6667,18 @@ Status Process::CalculateCoreFileSaveRanges(const SaveCoreOptions &options,
         "callers must set the core_style to something other than "
         "eSaveCoreUnspecified");
 
+  GetUserSpecifiedCoreFileSaveRanges(*this, regions, options, ranges);
+
   std::set<addr_t> stack_ends;
-  SaveOffRegionsWithStackPointers(*this, options, regions, ranges, stack_ends);
+  // For fully custom set ups, we don't want to even look at threads if there
+  // are no threads specified.
+  if (core_style != lldb::eSaveCoreCustomOnly || options.HasSpecifiedThreads())
+    SaveOffRegionsWithStackPointers(*this, options, regions, ranges,
+                                    stack_ends);
 
   switch (core_style) {
   case eSaveCoreUnspecified:
+  case eSaveCoreCustomOnly:
     break;
 
   case eSaveCoreFull:
@@ -6666,10 +6697,11 @@ Status Process::CalculateCoreFileSaveRanges(const SaveCoreOptions &options,
   if (err.Fail())
     return err;
 
-  if (ranges.empty())
-    return Status("no valid address ranges found for core style");
+  if (ranges.IsEmpty())
+    return Status::FromErrorStringWithFormat(
+        "no valid address ranges found for core style");
 
-  return Status(); // Success!
+  return ranges.FinalizeCoreFileSaveRanges();
 }
 
 std::vector<ThreadSP>

lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py

@@ -344,3 +344,152 @@ class ProcessSaveCoreMinidumpTestCase(TestBase):
             self.assertTrue(self.dbg.DeleteTarget(target))
             if os.path.isfile(default_value_file):
                 os.unlink(default_value_file)
+
+    @skipUnlessArch("x86_64")
+    @skipUnlessPlatform(["linux"])
+    def test_save_linux_minidump_one_region(self):
+        """Test that we can save a Linux mini dump with one region in sbsavecore regions"""
+
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+        one_region_file = self.getBuildArtifact("core.one_region.dmp")
+        try:
+            target = self.dbg.CreateTarget(exe)
+            process = target.LaunchSimple(
+                None, None, self.get_process_working_directory()
+            )
+            self.assertState(process.GetState(), lldb.eStateStopped)
+
+            memory_region = lldb.SBMemoryRegionInfo()
+            memory_list = process.GetMemoryRegions()
+            memory_list.GetMemoryRegionAtIndex(0, memory_region)
+
+            # This is almost identical to the single thread test case because
+            # minidump defaults to stacks only, so we want to see if the
+            # default options work as expected.
+            options = lldb.SBSaveCoreOptions()
+            file_spec = lldb.SBFileSpec(one_region_file)
+            options.SetOutputFile(file_spec)
+            options.SetPluginName("minidump")
+            options.AddMemoryRegionToSave(memory_region)
+            options.SetStyle(lldb.eSaveCoreCustomOnly)
+            error = process.SaveCore(options)
+            print(f"Error: {error.GetCString()}")
+            self.assertTrue(error.Success(), error.GetCString())
+
+            core_target = self.dbg.CreateTarget(None)
+            core_proc = core_target.LoadCore(one_region_file)
+            core_memory_list = core_proc.GetMemoryRegions()
+            # Note because the /proc/pid maps are included on linux, we can't
+            # depend on size for validation, so we'll ensure the first region
+            # is present and then assert we fail on the second.
+            core_memory_region = lldb.SBMemoryRegionInfo()
+            core_memory_list.GetMemoryRegionAtIndex(0, core_memory_region)
+            self.assertEqual(
+                core_memory_region.GetRegionBase(), memory_region.GetRegionBase()
+            )
+            self.assertEqual(
+                core_memory_region.GetRegionEnd(), memory_region.GetRegionEnd()
+            )
+
+            region_two = lldb.SBMemoryRegionInfo()
+            core_memory_list.GetMemoryRegionAtIndex(1, region_two)
+            err = lldb.SBError()
+            content = core_proc.ReadMemory(region_two.GetRegionBase(), 1, err)
+            self.assertTrue(err.Fail(), "Should fail to read memory")
+
+        finally:
+            self.assertTrue(self.dbg.DeleteTarget(target))
+            if os.path.isfile(one_region_file):
+                os.unlink(one_region_file)
+
+    @skipUnlessArch("x86_64")
+    @skipUnlessPlatform(["linux"])
+    def test_save_minidump_custom_save_style(self):
+        """Test that verifies a custom and unspecified save style fails for
+        containing no data to save"""
+
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+        custom_file = self.getBuildArtifact("core.custom.dmp")
+        try:
+            target = self.dbg.CreateTarget(exe)
+            process = target.LaunchSimple(
+                None, None, self.get_process_working_directory()
+            )
+            self.assertState(process.GetState(), lldb.eStateStopped)
+
+            options = lldb.SBSaveCoreOptions()
+            options.SetOutputFile(lldb.SBFileSpec(custom_file))
+            options.SetPluginName("minidump")
+            options.SetStyle(lldb.eSaveCoreCustomOnly)
+
+            error = process.SaveCore(options)
+            self.assertTrue(error.Fail())
+            self.assertEqual(
+                error.GetCString(), "no valid address ranges found for core style"
+            )
+
+        finally:
+            self.assertTrue(self.dbg.DeleteTarget(target))
+            if os.path.isfile(custom_file):
+                os.unlink(custom_file)
+
+    def save_core_with_region(self, process, region_index):
+        try:
+            custom_file = self.getBuildArtifact("core.custom.dmp")
+            memory_region = lldb.SBMemoryRegionInfo()
+            memory_list = process.GetMemoryRegions()
+            memory_list.GetMemoryRegionAtIndex(0, memory_region)
+            options = lldb.SBSaveCoreOptions()
+            options.SetOutputFile(lldb.SBFileSpec(custom_file))
+            options.SetPluginName("minidump")
+            options.SetStyle(lldb.eSaveCoreFull)
+
+            error = process.SaveCore(options)
+            self.assertTrue(error.Success())
+            core_target = self.dbg.CreateTarget(None)
+            core_proc = core_target.LoadCore(custom_file)
+            core_memory_list = core_proc.GetMemoryRegions()
+            # proc/pid/ maps are included on linux, so we can't depend on size
+            # for validation, we make a set of all the ranges,
+            # and ensure no duplicates!
+            range_set = set()
+            for x in range(core_memory_list.GetSize()):
+                core_memory_region = lldb.SBMemoryRegionInfo()
+                core_memory_list.GetMemoryRegionAtIndex(x, core_memory_region)
+                mem_tuple = (
+                    core_memory_region.GetRegionBase(),
+                    core_memory_region.GetRegionEnd(),
+                )
+                self.assertTrue(
+                    mem_tuple not in range_set, "Duplicate memory region found"
+                )
+                range_set.add(mem_tuple)
+        finally:
+            if os.path.isfile(custom_file):
+                os.unlink(custom_file)
+
+    @skipUnlessArch("x86_64")
+    @skipUnlessPlatform(["linux"])
+    def test_save_minidump_custom_save_style_duplicated_regions(self):
+        """Test that verifies a custom and unspecified save style fails for
+        containing no data to save"""
+
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+        try:
+            target = self.dbg.CreateTarget(exe)
+            process = target.LaunchSimple(
+                None, None, self.get_process_working_directory()
+            )
+            self.assertState(process.GetState(), lldb.eStateStopped)
+
+            memory_list = process.GetMemoryRegions()
+            # Test that we don't duplicate regions, by duplicating regions
+            # at various indices.
+            self.save_core_with_region(process, 0)
+            self.save_core_with_region(process, len(memory_list) - 1)
+
+        finally:
+            self.assertTrue(self.dbg.DeleteTarget(target))

lldb/unittests/Process/Utility/CMakeLists.txt

@@ -18,6 +18,7 @@ add_lldb_unittest(ProcessUtilityTests
   LinuxProcMapsTest.cpp
   MemoryTagManagerAArch64MTETest.cpp
   RegisterContextTest.cpp
+  CoreFileMemoryRangesTest.cpp
   ${PLATFORM_SOURCES}
 
   LINK_LIBS

lldb/unittests/Process/Utility/CoreFileMemoryRangesTest.cpp

@@ -0,0 +1,205 @@
+//===-- CoreFileMemoryRangesTests.cpp
+//---------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Target/CoreFileMemoryRanges.h"
+#include "lldb/lldb-types.h"
+
+using namespace lldb_private;
+
+TEST(CoreFileMemoryRangesTest, MapOverlappingRanges) {
+  lldb_private::CoreFileMemoryRanges ranges;
+  const lldb::addr_t start_addr = 0x1000;
+  const lldb::addr_t increment_addr = 0x1000;
+  const size_t iterations = 10;
+  for (size_t i = 0; i < iterations; i++) {
+    const lldb::addr_t start = start_addr + (i * increment_addr);
+    const lldb::addr_t end = start + increment_addr;
+    // Arbitrary value
+    const uint32_t permissions = 0x3;
+    llvm::AddressRange range(start, end);
+    const CoreFileMemoryRange core_range = {range, permissions};
+    // The range data is Start, Size, While the range is start-end.
+    CoreFileMemoryRanges::Entry entry = {start, end - start, core_range};
+    ranges.Append(entry);
+  }
+
+  Status error = ranges.FinalizeCoreFileSaveRanges();
+  EXPECT_TRUE(error.Success());
+  ASSERT_THAT(1, ranges.GetSize());
+  const auto range = ranges.GetEntryAtIndex(0);
+  ASSERT_TRUE(range);
+  ASSERT_THAT(start_addr, range->GetRangeBase());
+  ASSERT_THAT(start_addr + (iterations * increment_addr), range->GetRangeEnd());
+}
+
+TEST(CoreFileMemoryRangesTest, RangesSplitByPermissions) {
+  lldb_private::CoreFileMemoryRanges ranges;
+  const lldb::addr_t start_addr = 0x1000;
+  const lldb::addr_t increment_addr = 0x1000;
+  const size_t iterations = 10;
+  for (size_t i = 0; i < iterations; i++) {
+    const lldb::addr_t start = start_addr + (i * increment_addr);
+    const lldb::addr_t end = start + increment_addr;
+    const uint32_t permissions = i;
+    llvm::AddressRange range(start, end);
+    const CoreFileMemoryRange core_range = {range, permissions};
+    // The range data is Start, Size, While the range is start-end.
+    CoreFileMemoryRanges::Entry entry = {start, end - start, core_range};
+    ranges.Append(entry);
+  }
+
+  Status error = ranges.FinalizeCoreFileSaveRanges();
+  EXPECT_TRUE(error.Success());
+  ASSERT_THAT(10, ranges.GetSize());
+  const auto range = ranges.GetEntryAtIndex(0);
+  ASSERT_TRUE(range);
+  ASSERT_THAT(start_addr, range->GetRangeBase());
+  ASSERT_THAT(start_addr + increment_addr, range->GetRangeEnd());
+}
+
+TEST(CoreFileMemoryRangesTest, MapPartialOverlappingRanges) {
+  lldb_private::CoreFileMemoryRanges ranges;
+  const lldb::addr_t start_addr = 0x1000;
+  const lldb::addr_t increment_addr = 0x1000;
+  const size_t iterations = 10;
+  for (size_t i = 0; i < iterations; i++) {
+    const lldb::addr_t start = start_addr + (i * increment_addr);
+    const lldb::addr_t end = start + increment_addr;
+    // Arbitrary value
+    const uint32_t permissions = 0x3;
+    llvm::AddressRange range(start, end);
+    const CoreFileMemoryRange core_range = {range, permissions};
+    // The range data is Start, Size, While the range is start-end.
+    CoreFileMemoryRanges::Entry entry = {start, end - start, core_range};
+    ranges.Append(entry);
+  }
+
+  const lldb::addr_t unique_start = 0x7fff0000;
+  const lldb::addr_t unique_end = unique_start + increment_addr;
+  llvm::AddressRange range(unique_start, unique_end);
+  const uint32_t permissions = 0x3;
+  const CoreFileMemoryRange core_range = {range, permissions};
+  // The range data is Start, Size, While the range is start-end.
+  CoreFileMemoryRanges::Entry entry = {unique_start, unique_end - unique_start,
+                                       core_range};
+  ranges.Append(entry);
+
+  Status error = ranges.FinalizeCoreFileSaveRanges();
+  EXPECT_TRUE(error.Success());
+  ASSERT_THAT(2, ranges.GetSize());
+  const auto merged_range = ranges.GetEntryAtIndex(0);
+  ASSERT_TRUE(merged_range);
+  ASSERT_THAT(start_addr, merged_range->GetRangeBase());
+  ASSERT_THAT(start_addr + (iterations * increment_addr),
+              merged_range->GetRangeEnd());
+  const auto unique_range = ranges.GetEntryAtIndex(1);
+  ASSERT_TRUE(unique_range);
+  ASSERT_THAT(unique_start, unique_range->GetRangeBase());
+  ASSERT_THAT(unique_end, unique_range->GetRangeEnd());
+}
+
+TEST(CoreFileMemoryRangesTest, SuperiorAndInferiorRanges_SamePermissions) {
+  lldb_private::CoreFileMemoryRanges ranges;
+  const lldb::addr_t start_addr = 0x1000;
+  const lldb::addr_t increment_addr = 0x1000;
+  const lldb::addr_t superior_region_end = start_addr + increment_addr * 10;
+  llvm::AddressRange range(start_addr, superior_region_end);
+  const CoreFileMemoryRange core_range = {range, 0x3};
+  CoreFileMemoryRanges::Entry entry = {
+      start_addr, superior_region_end - start_addr, core_range};
+  ranges.Append(entry);
+  const lldb::addr_t inferior_region_end = start_addr + increment_addr;
+  llvm::AddressRange inferior_range(start_addr, inferior_region_end);
+  const CoreFileMemoryRange inferior_core_range = {inferior_range, 0x3};
+  CoreFileMemoryRanges::Entry inferior_entry = {
+      start_addr, inferior_region_end - start_addr, inferior_core_range};
+  ranges.Append(inferior_entry);
+
+  Status error = ranges.FinalizeCoreFileSaveRanges();
+  EXPECT_TRUE(error.Success());
+  ASSERT_THAT(1, ranges.GetSize());
+  const auto searched_range = ranges.GetEntryAtIndex(0);
+  ASSERT_TRUE(searched_range);
+  ASSERT_THAT(start_addr, searched_range->GetRangeBase());
+  ASSERT_THAT(superior_region_end, searched_range->GetRangeEnd());
+}
+
+TEST(CoreFileMemoryRangesTest, SuperiorAndInferiorRanges_DifferentPermissions) {
+  lldb_private::CoreFileMemoryRanges ranges;
+  const lldb::addr_t start_addr = 0x1000;
+  const lldb::addr_t increment_addr = 0x1000;
+  const lldb::addr_t superior_region_end = start_addr + increment_addr * 10;
+  llvm::AddressRange range(start_addr, superior_region_end);
+  const CoreFileMemoryRange core_range = {range, 0x3};
+  CoreFileMemoryRanges::Entry entry = {
+      start_addr, superior_region_end - start_addr, core_range};
+  ranges.Append(entry);
+  const lldb::addr_t inferior_region_end = start_addr + increment_addr;
+  llvm::AddressRange inferior_range(start_addr, inferior_region_end);
+  const CoreFileMemoryRange inferior_core_range = {inferior_range, 0x4};
+  CoreFileMemoryRanges::Entry inferior_entry = {
+      start_addr, inferior_region_end - start_addr, inferior_core_range};
+  ranges.Append(inferior_entry);
+
+  Status error = ranges.FinalizeCoreFileSaveRanges();
+  EXPECT_TRUE(error.Fail());
+}
+
+TEST(CoreFileMemoryRangesTest, NonIntersectingRangesSamePermissions) {
+  const int permissions = 0x7;
+  lldb_private::CoreFileMemoryRanges ranges;
+  const lldb::addr_t region_one_start = 0x1000;
+  const lldb::addr_t region_one_end = 0x2000;
+  llvm::AddressRange range_one(region_one_start, region_one_end);
+  const CoreFileMemoryRange core_range_one = {range_one, permissions};
+  CoreFileMemoryRanges::Entry entry_one = {
+      region_one_start, region_one_end - region_one_start, core_range_one};
+  ranges.Append(entry_one);
+  const lldb::addr_t region_two_start = 0xb000;
+  const lldb::addr_t region_two_end = 0xc000;
+  llvm::AddressRange range_two(region_two_start, region_two_end);
+  const CoreFileMemoryRange core_range_two = {range_two, permissions};
+  CoreFileMemoryRanges::Entry entry_two = {
+      region_two_start, region_two_end - region_two_start, core_range_two};
+  ranges.Append(entry_two);
+
+  Status error = ranges.FinalizeCoreFileSaveRanges();
+  EXPECT_TRUE(error.Success());
+  ASSERT_THAT(2UL, ranges.GetSize());
+  ASSERT_THAT(region_one_start, ranges.GetEntryAtIndex(0)->GetRangeBase());
+  ASSERT_THAT(region_two_start, ranges.GetEntryAtIndex(1)->GetRangeBase());
+}
+
+TEST(CoreFileMemoryRangesTest, PartialOverlapping) {
+  const int permissions = 0x3;
+  lldb_private::CoreFileMemoryRanges ranges;
+  const lldb::addr_t start_addr = 0x1000;
+  const lldb::addr_t end_addr = 0x2000;
+  llvm::AddressRange range_one(start_addr, end_addr);
+  const CoreFileMemoryRange core_range_one = {range_one, permissions};
+  CoreFileMemoryRanges::Entry entry_one = {start_addr, end_addr - start_addr,
+                                           core_range_one};
+  llvm::AddressRange range_two(start_addr / 2, end_addr / 2);
+  const CoreFileMemoryRange core_range_two = {range_two, permissions};
+  CoreFileMemoryRanges::Entry entry_two = {
+      start_addr / 2, end_addr / 2 - start_addr / 2, core_range_two};
+  ranges.Append(entry_one);
+  ranges.Append(entry_two);
+
+  Status error = ranges.FinalizeCoreFileSaveRanges();
+  EXPECT_TRUE(error.Success());
+  ASSERT_THAT(1, ranges.GetSize());
+  const auto searched_range = ranges.GetEntryAtIndex(0);
+  ASSERT_TRUE(searched_range);
+  ASSERT_THAT(start_addr / 2, searched_range->GetRangeBase());
+  ASSERT_THAT(end_addr, searched_range->GetRangeEnd());
+}