Revert "[LLDB] Reappply SBSaveCore AddMemoryList" (#107731)

JDevlieghere · web-flow · commit bb343468ffa8 · 2024-09-07T17:10:20.000-07:00
Reverts #107159 as this is still causing `TestSkinnyCorefile.py` to time out. https://ci.swift.org/view/all/job/llvm.org/view/LLDB/job/as-lldb-cmake/11099/ https://ci.swift.org/view/all/job/llvm.org/view/LLDB/job/lldb-cmake/5544/
diff --git a/lldb/include/lldb/API/SBMemoryRegionInfo.h b/lldb/include/lldb/API/SBMemoryRegionInfo.h
@@ -120,7 +120,7 @@ class LLDB_API SBMemoryRegionInfo {
 private:
   friend class SBProcess;
   friend class SBMemoryRegionInfoList;
-  friend class SBSaveCoreOptions;
+
   friend class lldb_private::ScriptInterpreter;
 
   lldb_private::MemoryRegionInfo &ref();
diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h
@@ -80,17 +80,6 @@ class LLDB_API SBSaveCoreOptions {
   /// \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();
 
diff --git a/lldb/include/lldb/Symbol/SaveCoreOptions.h b/lldb/include/lldb/Symbol/SaveCoreOptions.h
@@ -10,15 +10,13 @@
 #define LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H
 
 #include "lldb/Utility/FileSpec.h"
-#include "lldb/Utility/RangeMap.h"
+#include "lldb/lldb-forward.h"
+#include "lldb/lldb-types.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 {
@@ -40,12 +38,8 @@ class SaveCoreOptions {
   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();
 
@@ -57,7 +51,6 @@ class SaveCoreOptions {
   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
 
diff --git a/lldb/include/lldb/Target/CoreFileMemoryRanges.h b/lldb/include/lldb/Target/CoreFileMemoryRanges.h
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
@@ -35,8 +35,6 @@
 #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"
@@ -712,6 +710,29 @@ 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.
diff --git a/lldb/include/lldb/Utility/RangeMap.h b/lldb/include/lldb/Utility/RangeMap.h
@@ -450,8 +450,6 @@ class RangeDataVector {
 
   void Append(const Entry &entry) { m_entries.emplace_back(entry); }
 
-  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;
diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
@@ -1222,7 +1222,6 @@ enum SaveCoreStyle {
   eSaveCoreFull = 1,
   eSaveCoreDirtyOnly = 2,
   eSaveCoreStackOnly = 3,
-  eSaveCoreCustomOnly = 4,
 };
 
 /// Events that might happen during a trace session.
diff --git a/lldb/include/lldb/lldb-forward.h b/lldb/include/lldb/lldb-forward.h
@@ -207,7 +207,6 @@ class StackFrameRecognizer;
 class StackFrameRecognizerManager;
 class StackID;
 class Status;
-class SaveCoreOptions;
 class StopInfo;
 class Stoppoint;
 class StoppointCallbackContext;
diff --git a/lldb/include/lldb/lldb-private-interfaces.h b/lldb/include/lldb/lldb-private-interfaces.h
@@ -9,6 +9,7 @@
 #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"
diff --git a/lldb/source/API/SBSaveCoreOptions.cpp b/lldb/source/API/SBSaveCoreOptions.cpp
@@ -7,7 +7,6 @@
 //===----------------------------------------------------------------------===//
 
 #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"
@@ -90,16 +89,6 @@ 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();
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -25,7 +25,6 @@
 #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"
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6562,15 +6562,13 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
     }
 
     if (make_core) {
-      CoreFileMemoryRanges core_ranges;
+      Process::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_info : core_ranges) {
-          // TODO: Refactor RangeDataVector to have a data iterator.
-          const auto &core_range = core_range_info.data;
+        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) {
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
@@ -12,7 +12,6 @@
 #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"
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -831,32 +831,25 @@ 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.
-  std::vector<CoreFileMemoryRange> ranges_32;
-  std::vector<CoreFileMemoryRange> ranges_64;
-  CoreFileMemoryRanges all_core_memory_ranges;
+  Process::CoreFileMemoryRanges ranges_32;
+  Process::CoreFileMemoryRanges ranges_64;
+  Process::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_vec.begin();
-  while (iterator != all_core_memory_vec.end()) {
+  auto iterator = all_core_memory_ranges.begin();
+  while (iterator != all_core_memory_ranges.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_vec.erase(iterator);
+      iterator = all_core_memory_ranges.erase(iterator);
     } else {
       iterator++;
     }
@@ -876,11 +869,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_vec.empty())
-    total_size += 256 + (all_core_memory_vec.size() *
+  if (!all_core_memory_ranges.empty())
+    total_size += 256 + (all_core_memory_ranges.size() *
                          sizeof(llvm::minidump::MemoryDescriptor_64));
 
-  for (const auto &core_range : all_core_memory_vec) {
+  for (const auto &core_range : all_core_memory_ranges) {
     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.
@@ -965,15 +958,15 @@ Status MinidumpFileBuilder::DumpDirectories() const {
 }
 
 static uint64_t
-GetLargestRangeSize(const std::vector<CoreFileMemoryRange> &ranges) {
+GetLargestRangeSize(const Process::CoreFileMemoryRanges &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(
-    std::vector<CoreFileMemoryRange> &ranges) {
+Status
+MinidumpFileBuilder::AddMemoryList_32(Process::CoreFileMemoryRanges &ranges) {
   std::vector<MemoryDescriptor> descriptors;
   Status error;
   if (ranges.size() == 0)
@@ -1049,8 +1042,8 @@ Status MinidumpFileBuilder::AddMemoryList_32(
   return error;
 }
 
-Status MinidumpFileBuilder::AddMemoryList_64(
-    std::vector<CoreFileMemoryRange> &ranges) {
+Status
+MinidumpFileBuilder::AddMemoryList_64(Process::CoreFileMemoryRanges &ranges) {
   Status error;
   if (ranges.empty())
     return error;
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -23,7 +23,6 @@
 #include <utility>
 #include <variant>
 
-#include "lldb/Symbol/SaveCoreOptions.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataBufferHeap.h"
@@ -121,9 +120,9 @@ class MinidumpFileBuilder {
   lldb_private::Status AddData(const void *data, uint64_t size);
   // Add MemoryList stream, containing dumps of important memory segments
   lldb_private::Status
-  AddMemoryList_64(std::vector<lldb_private::CoreFileMemoryRange> &ranges);
+  AddMemoryList_64(lldb_private::Process::CoreFileMemoryRanges &ranges);
   lldb_private::Status
-  AddMemoryList_32(std::vector<lldb_private::CoreFileMemoryRange> &ranges);
+  AddMemoryList_32(lldb_private::Process::CoreFileMemoryRanges &ranges);
   // Update the thread list on disk with the newly emitted stack RVAs.
   lldb_private::Status FixThreadStacks();
   lldb_private::Status FlushBufferToDisk();
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h
@@ -21,7 +21,6 @@
 #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 {
diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -17,7 +17,6 @@
 #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"
diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
@@ -13,7 +13,6 @@
 #include <vector>
 
 #include "lldb/Symbol/ObjectFile.h"
-#include "lldb/Symbol/SaveCoreOptions.h"
 #include "llvm/Object/COFF.h"
 
 class ObjectFilePECOFF : public lldb_private::ObjectFile {
diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp b/lldb/source/Symbol/SaveCoreOptions.cpp
diff --git a/lldb/source/Target/CMakeLists.txt b/lldb/source/Target/CMakeLists.txt
diff --git a/lldb/source/Target/CoreFileMemoryRanges.cpp b/lldb/source/Target/CoreFileMemoryRanges.cpp
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
diff --git a/lldb/unittests/Process/Utility/CMakeLists.txt b/lldb/unittests/Process/Utility/CMakeLists.txt
diff --git a/lldb/unittests/Process/Utility/CoreFileMemoryRangesTest.cpp b/lldb/unittests/Process/Utility/CoreFileMemoryRangesTest.cpp
