Revert "[LLDB][SBSaveCore] Implement a selectable threadlist for Core… (#102018)

… Options.  (#100443)"

This reverts commit 3e4af61.

@adrian-prantl FYI

Reverts #100443

Author: Jlalond

lldb/include/lldb/API/SBProcess.h

@@ -586,7 +586,6 @@ class LLDB_API SBProcess {
   friend class SBBreakpointCallbackBaton;
   friend class SBBreakpointLocation;
   friend class SBCommandInterpreter;
-  friend class SBSaveCoreOptions;
   friend class SBDebugger;
   friend class SBExecutionContext;
   friend class SBFunction;

lldb/include/lldb/API/SBSaveCoreOptions.h

@@ -10,10 +10,6 @@
 #define LLDB_API_SBSAVECOREOPTIONS_H
 
 #include "lldb/API/SBDefines.h"
-#include "lldb/API/SBError.h"
-#include "lldb/API/SBFileSpec.h"
-#include "lldb/API/SBProcess.h"
-#include "lldb/API/SBThread.h"
 
 namespace lldb {
 
@@ -57,29 +53,6 @@ class LLDB_API SBSaveCoreOptions {
   /// \return The output file spec.
   SBFileSpec GetOutputFile() const;
 
-  /// Set the process to save, or unset if supplied with a default constructed
-  /// process.
-  ///
-  /// \param process The process to save.
-  /// \return Success if process was set, otherwise an error
-  /// \note This will clear all process specific options if a different process
-  /// is specified than the current set process, either explicitly from this
-  /// api, or implicitly from any function that requires a process.
-  SBError SetProcess(lldb::SBProcess process);
-
-  /// Add a thread to save in the core file.
-  ///
-  /// \param thread The thread to save.
-  /// \note This will set the process if it is not already set, or return
-  /// and error if the SBThread is not from the set process.
-  SBError AddThread(lldb::SBThread thread);
-
-  /// Remove a thread from the list of threads to save.
-  ///
-  /// \param thread The thread to remove.
-  /// \return True if the thread was removed, false if it was not in the list.
-  bool RemoveThread(lldb::SBThread thread);
-
   /// Reset all options.
   void Clear();
 

lldb/include/lldb/API/SBThread.h

@@ -233,7 +233,6 @@ class LLDB_API SBThread {
   friend class SBBreakpoint;
   friend class SBBreakpointLocation;
   friend class SBBreakpointCallbackBaton;
-  friend class SBSaveCoreOptions;
   friend class SBExecutionContext;
   friend class SBFrame;
   friend class SBProcess;
@@ -254,8 +253,6 @@ class LLDB_API SBThread {
   SBError ResumeNewPlan(lldb_private::ExecutionContext &exe_ctx,
                         lldb_private::ThreadPlan *new_plan);
 
-  lldb::ThreadSP GetSP() const;
-
   lldb::ExecutionContextRefSP m_opaque_sp;
 
   lldb_private::Thread *operator->();

lldb/include/lldb/Symbol/SaveCoreOptions.h

@@ -15,7 +15,6 @@
 
 #include <optional>
 #include <string>
-#include <unordered_set>
 
 namespace lldb_private {
 
@@ -33,25 +32,13 @@ class SaveCoreOptions {
   void SetOutputFile(lldb_private::FileSpec file);
   const std::optional<lldb_private::FileSpec> GetOutputFile() const;
 
-  Status SetProcess(lldb::ProcessSP process_sp);
-
-  Status AddThread(lldb::ThreadSP thread_sp);
-  bool RemoveThread(lldb::ThreadSP thread_sp);
-  bool ShouldThreadBeSaved(lldb::tid_t tid) const;
-
-  Status EnsureValidConfiguration(lldb::ProcessSP process_sp) const;
-
   void Clear();
 
 private:
-  void ClearProcessSpecificData();
-
   std::optional<std::string> m_plugin_name;
   std::optional<lldb_private::FileSpec> m_file;
   std::optional<lldb::SaveCoreStyle> m_style;
-  lldb::ProcessSP m_process_sp;
-  std::unordered_set<lldb::tid_t> m_threads_to_save;
 };
 } // namespace lldb_private
 
-#endif // LLDB_SOURCE_PLUGINS_OBJECTFILE_SAVECOREOPTIONS_H
+#endif // LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H

lldb/include/lldb/Target/Process.h

@@ -738,15 +738,9 @@ class Process : public std::enable_shared_from_this<Process>,
   /// 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(const SaveCoreOptions &core_options,
+  Status CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
                                      CoreFileMemoryRanges &ranges);
 
-  /// Helper function for Process::SaveCore(...) that calculates the thread list
-  /// based upon options set within a given \a core_options object.
-  /// \note If there is no thread list defined, all threads will be saved.
-  std::vector<lldb::ThreadSP>
-  CalculateCoreFileThreadList(const SaveCoreOptions &core_options);
-
 protected:
   virtual JITLoaderList &GetJITLoaders();
 

lldb/source/API/SBSaveCoreOptions.cpp

@@ -7,6 +7,8 @@
 //===----------------------------------------------------------------------===//
 
 #include "lldb/API/SBSaveCoreOptions.h"
+#include "lldb/API/SBError.h"
+#include "lldb/API/SBFileSpec.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Symbol/SaveCoreOptions.h"
 #include "lldb/Utility/Instrumentation.h"
@@ -73,21 +75,6 @@ lldb::SaveCoreStyle SBSaveCoreOptions::GetStyle() const {
   return m_opaque_up->GetStyle();
 }
 
-SBError SBSaveCoreOptions::SetProcess(lldb::SBProcess process) {
-  LLDB_INSTRUMENT_VA(this, process);
-  return m_opaque_up->SetProcess(process.GetSP());
-}
-
-SBError SBSaveCoreOptions::AddThread(lldb::SBThread thread) {
-  LLDB_INSTRUMENT_VA(this, thread);
-  return m_opaque_up->AddThread(thread.GetSP());
-}
-
-bool SBSaveCoreOptions::RemoveThread(lldb::SBThread thread) {
-  LLDB_INSTRUMENT_VA(this, thread);
-  return m_opaque_up->RemoveThread(thread.GetSP());
-}
-
 void SBSaveCoreOptions::Clear() {
   LLDB_INSTRUMENT_VA(this);
   m_opaque_up->Clear();

lldb/source/API/SBThread.cpp

@@ -1331,8 +1331,6 @@ bool SBThread::SafeToCallFunctions() {
   return true;
 }
 
-lldb::ThreadSP SBThread::GetSP() const { return m_opaque_sp->GetThreadSP(); }
-
 lldb_private::Thread *SBThread::operator->() {
   return get();
 }

lldb/source/Core/PluginManager.cpp

@@ -714,10 +714,6 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp,
     return error;
   }
 
-  error = options.EnsureValidConfiguration(process_sp);
-  if (error.Fail())
-    return error;
-
   if (!options.GetPluginName().has_value()) {
     // Try saving core directly from the process plugin first.
     llvm::Expected<bool> ret =

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

@@ -6347,24 +6347,22 @@ struct segment_vmaddr {
 // are some multiple passes over the image list while calculating
 // everything.
 
-static offset_t
-CreateAllImageInfosPayload(const lldb::ProcessSP &process_sp,
-                           offset_t initial_file_offset,
-                           StreamString &all_image_infos_payload,
-                           const lldb_private::SaveCoreOptions &options) {
+static offset_t CreateAllImageInfosPayload(
+    const lldb::ProcessSP &process_sp, offset_t initial_file_offset,
+    StreamString &all_image_infos_payload, SaveCoreStyle core_style) {
   Target &target = process_sp->GetTarget();
   ModuleList modules = target.GetImages();
 
   // stack-only corefiles have no reason to include binaries that
   // are not executing; we're trying to make the smallest corefile
   // we can, so leave the rest out.
-  if (options.GetStyle() == SaveCoreStyle::eSaveCoreStackOnly)
+  if (core_style == SaveCoreStyle::eSaveCoreStackOnly)
     modules.Clear();
 
   std::set<std::string> executing_uuids;
-  std::vector<ThreadSP> thread_list =
-      process_sp->CalculateCoreFileThreadList(options);
-  for (const ThreadSP &thread_sp : thread_list) {
+  ThreadList &thread_list(process_sp->GetThreadList());
+  for (uint32_t i = 0; i < thread_list.GetSize(); i++) {
+    ThreadSP thread_sp = thread_list.GetThreadAtIndex(i);
     uint32_t stack_frame_count = thread_sp->GetStackFrameCount();
     for (uint32_t j = 0; j < stack_frame_count; j++) {
       StackFrameSP stack_frame_sp = thread_sp->GetStackFrameAtIndex(j);
@@ -6561,7 +6559,7 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
 
     if (make_core) {
       Process::CoreFileMemoryRanges core_ranges;
-      error = process_sp->CalculateCoreFileSaveRanges(options, 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();
@@ -6732,8 +6730,8 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
             std::make_shared<StructuredData::Dictionary>());
         StructuredData::ArraySP threads(
             std::make_shared<StructuredData::Array>());
-        for (const ThreadSP &thread_sp :
-             process_sp->CalculateCoreFileThreadList(options)) {
+        for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
+          ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
           StructuredData::DictionarySP thread(
               std::make_shared<StructuredData::Dictionary>());
           thread->AddIntegerItem("thread_id", thread_sp->GetID());
@@ -6756,7 +6754,7 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
         all_image_infos_lcnote_up->payload_file_offset = file_offset;
         file_offset = CreateAllImageInfosPayload(
             process_sp, file_offset, all_image_infos_lcnote_up->payload,
-            options);
+            core_style);
         lc_notes.push_back(std::move(all_image_infos_lcnote_up));
 
         // Add LC_NOTE load commands

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

@@ -69,9 +69,10 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
     m_expected_directories += 9;
 
   // Go through all of the threads and check for exceptions.
-  std::vector<lldb::ThreadSP> threads =
-      m_process_sp->CalculateCoreFileThreadList(m_save_core_options);
-  for (const ThreadSP &thread_sp : threads) {
+  lldb_private::ThreadList thread_list = m_process_sp->GetThreadList();
+  const uint32_t num_threads = thread_list.GetSize();
+  for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
+    ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
     StopInfoSP stop_info_sp = thread_sp->GetStopInfo();
     if (stop_info_sp) {
       const StopReason &stop_reason = stop_info_sp->GetStopReason();
@@ -587,13 +588,12 @@ Status MinidumpFileBuilder::FixThreadStacks() {
 
 Status MinidumpFileBuilder::AddThreadList() {
   constexpr size_t minidump_thread_size = sizeof(llvm::minidump::Thread);
-  std::vector<ThreadSP> thread_list =
-      m_process_sp->CalculateCoreFileThreadList(m_save_core_options);
+  lldb_private::ThreadList thread_list = m_process_sp->GetThreadList();
 
   // size of the entire thread stream consists of:
   // number of threads and threads array
   size_t thread_stream_size = sizeof(llvm::support::ulittle32_t) +
-                              thread_list.size() * minidump_thread_size;
+                              thread_list.GetSize() * minidump_thread_size;
   // save for the ability to set up RVA
   size_t size_before = GetCurrentDataEndOffset();
   Status error;
@@ -602,15 +602,17 @@ Status MinidumpFileBuilder::AddThreadList() {
     return error;
 
   llvm::support::ulittle32_t thread_count =
-      static_cast<llvm::support::ulittle32_t>(thread_list.size());
+      static_cast<llvm::support::ulittle32_t>(thread_list.GetSize());
   m_data.AppendData(&thread_count, sizeof(llvm::support::ulittle32_t));
 
   // Take the offset after the thread count.
   m_thread_list_start = GetCurrentDataEndOffset();
   DataBufferHeap helper_data;
 
+  const uint32_t num_threads = thread_list.GetSize();
   Log *log = GetLog(LLDBLog::Object);
-  for (const ThreadSP &thread_sp : thread_list) {
+  for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
+    ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
     RegisterContextSP reg_ctx_sp(thread_sp->GetRegisterContext());
 
     if (!reg_ctx_sp) {
@@ -648,7 +650,7 @@ Status MinidumpFileBuilder::AddThreadList() {
     m_tid_to_reg_ctx[thread_sp->GetID()] = thread_context_memory_locator;
 
     LLDB_LOGF(log, "AddThreadList for thread %d: thread_context %zu bytes",
-              thread_sp->GetIndexID(), thread_context.size());
+              thread_idx, thread_context.size());
     helper_data.AppendData(thread_context.data(), thread_context.size());
 
     llvm::minidump::Thread t;
@@ -672,10 +674,11 @@ Status MinidumpFileBuilder::AddThreadList() {
 }
 
 Status MinidumpFileBuilder::AddExceptions() {
-  std::vector<ThreadSP> thread_list =
-      m_process_sp->CalculateCoreFileThreadList(m_save_core_options);
+  lldb_private::ThreadList thread_list = m_process_sp->GetThreadList();
   Status error;
-  for (const ThreadSP &thread_sp : thread_list) {
+  const uint32_t num_threads = thread_list.GetSize();
+  for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
+    ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
     StopInfoSP stop_info_sp = thread_sp->GetStopInfo();
     bool add_exception = false;
     if (stop_info_sp) {
@@ -816,7 +819,7 @@ Status MinidumpFileBuilder::AddLinuxFileStreams() {
   return error;
 }
 
-Status MinidumpFileBuilder::AddMemoryList() {
+Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
   Status error;
 
   // We first save the thread stacks to ensure they fit in the first UINT32_MAX
@@ -825,26 +828,18 @@ Status MinidumpFileBuilder::AddMemoryList() {
   // in accessible with a 32 bit offset.
   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);
+  error = m_process_sp->CalculateCoreFileSaveRanges(
+      SaveCoreStyle::eSaveCoreStackOnly, ranges_32);
   if (error.Fail())
     return error;
 
-  // Start by saving all of the stacks and ensuring they fit under the 32b
-  // limit.
+  // Calculate totalsize including the current offset.
   uint64_t total_size = GetCurrentDataEndOffset();
-  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_ranges.erase(iterator);
-    } else {
-      iterator++;
-    }
+  total_size += ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor);
+  std::unordered_set<addr_t> stack_start_addresses;
+  for (const auto &core_range : ranges_32) {
+    stack_start_addresses.insert(core_range.range.start());
+    total_size += core_range.range.size();
   }
 
   if (total_size >= UINT32_MAX) {
@@ -854,20 +849,31 @@ Status MinidumpFileBuilder::AddMemoryList() {
     return error;
   }
 
+  Process::CoreFileMemoryRanges all_core_memory_ranges;
+  if (core_style != SaveCoreStyle::eSaveCoreStackOnly) {
+    error = m_process_sp->CalculateCoreFileSaveRanges(core_style,
+                                                      all_core_memory_ranges);
+    if (error.Fail())
+      return error;
+  }
+
   // After saving the stacks, we start packing as much as we can into 32b.
   // We apply a generous padding here so that the Directory, MemoryList and
   // Memory64List sections all begin in 32b addressable space.
   // 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() *
-                         sizeof(llvm::minidump::MemoryDescriptor_64));
+    total_size +=
+        256 + (all_core_memory_ranges.size() - stack_start_addresses.size()) *
+                  sizeof(llvm::minidump::MemoryDescriptor_64);
 
   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.
+    if (stack_start_addresses.count(core_range.range.start()) > 0)
+      // Don't double save stacks.
+      continue;
+
     if (total_size + range_size < UINT32_MAX) {
       ranges_32.push_back(core_range);
       total_size += range_size;