[LLDB][SBSaveCore] Implement a selectable threadlist for Core Options.

[Jlalond]

In #98403 I enabled the SBSaveCoreOptions object, which allows users via the scripting API to define what they want saved into their core file. As the first option I've added a threadlist, so users can scan and identify which threads and corresponding stacks they want to save.

In order to support this, I had to add a new method to Process.h on how we identify which threads are to be saved, and I had to change the book keeping in minidump to ensure we don't double save the stacks.

Important to @jasonmolenda I also changed the MachO coredump to accept these new APIs.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jacob Lalonde (Jlalond)

Changes

In #98403 I enabled the SBSaveCoreOptions object, which allows users via the scripting API to define what they want saved into their core file. As the first option I've added a threadlist, so users can scan and identify which threads and corresponding stacks they want to save.

In order to support this, I had to add a new method to Process.h on how we identify which threads are to be saved, and I had to change the book keeping in minidump to ensure we don't double save the stacks.

Important to @jasonmolenda I also changed the MachO coredump to accept these new APIs.

---

Patch is 20.56 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/100443.diff

11 Files Affected:

• (modified) lldb/include/lldb/API/SBSaveCoreOptions.h (+24)
• (modified) lldb/include/lldb/Symbol/SaveCoreOptions.h (+10)
• (modified) lldb/include/lldb/Target/Process.h (+6-1)
• (modified) lldb/source/API/SBSaveCoreOptions.cpp (+20)
• (modified) lldb/source/Core/PluginManager.cpp (+4)
• (modified) lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp (+6-5)
• (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp (+27-29)
• (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h (+7-3)
• (modified) lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp (+3-2)
• (modified) lldb/source/Symbol/SaveCoreOptions.cpp (+51)
• (modified) lldb/source/Target/Process.cpp (+25-5)

diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h
index e77496bd3a4a0..b485371ce8f55 100644
--- a/lldb/include/lldb/API/SBSaveCoreOptions.h
+++ b/lldb/include/lldb/API/SBSaveCoreOptions.h
@@ -53,6 +53,30 @@ class LLDB_API SBSaveCoreOptions {
   /// \return The output file spec.
   SBFileSpec GetOutputFile() const;
 
+  /// Add a thread to save in the core file.
+  ///
+  /// \param thread_id The thread ID to save.
+  void AddThread(lldb::tid_t thread_id);
+
+  /// Remove a thread from the list of threads to save.
+  ///
+  /// \param thread_id The thread ID to remove.
+  /// \return True if the thread was removed, false if it was not in the list.
+  bool RemoveThread(lldb::tid_t thread_id);
+
+  /// Get the number of threads to save. If this list is empty all threads will
+  /// be saved.
+  ///
+  /// \return The number of threads to save.
+  uint32_t GetNumThreads() const;
+
+  /// Get the thread ID at the given index.
+  ///
+  /// \param[in] index The index of the thread ID to get.
+  /// \return The thread ID at the given index, or an error
+  /// if there is no thread at the index.
+  lldb::tid_t GetThreadAtIndex(uint32_t index, SBError &error) const;
+
   /// Reset all options.
   void Clear();
 
diff --git a/lldb/include/lldb/Symbol/SaveCoreOptions.h b/lldb/include/lldb/Symbol/SaveCoreOptions.h
index 583bc1f483d04..d583b32b29508 100644
--- a/lldb/include/lldb/Symbol/SaveCoreOptions.h
+++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h
@@ -14,6 +14,7 @@
 #include "lldb/lldb-types.h"
 
 #include <optional>
+#include <set>
 #include <string>
 
 namespace lldb_private {
@@ -32,12 +33,21 @@ class SaveCoreOptions {
   void SetOutputFile(lldb_private::FileSpec file);
   const std::optional<lldb_private::FileSpec> GetOutputFile() const;
 
+  void AddThread(lldb::tid_t tid);
+  bool RemoveThread(lldb::tid_t tid);
+  size_t GetNumThreads() const;
+  int64_t GetThreadAtIndex(size_t index) const;
+  bool ShouldSaveThread(lldb::tid_t tid) const;
+
+  Status EnsureValidConfiguration() const;
+
   void Clear();
 
 private:
   std::optional<std::string> m_plugin_name;
   std::optional<lldb_private::FileSpec> m_file;
   std::optional<lldb::SaveCoreStyle> m_style;
+  std::set<lldb::tid_t> m_threads_to_save;
 };
 } // namespace lldb_private
 
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index c8475db8ae160..ef3907154c20f 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -738,9 +738,14 @@ 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(lldb::SaveCoreStyle core_style,
+  Status CalculateCoreFileSaveRanges(const SaveCoreOptions &core_options,
                                      CoreFileMemoryRanges &ranges);
 
+  /// Helper function for Process::SaveCore(...) that calculates the thread list
+  /// based upon options set within a given \a core_options object.
+  std::vector<lldb::ThreadSP>
+  CalculateCoreFileThreadList(const SaveCoreOptions &core_options);
+
 protected:
   virtual JITLoaderList &GetJITLoaders();
 
diff --git a/lldb/source/API/SBSaveCoreOptions.cpp b/lldb/source/API/SBSaveCoreOptions.cpp
index 6c3f74596203d..1d45313d2426a 100644
--- a/lldb/source/API/SBSaveCoreOptions.cpp
+++ b/lldb/source/API/SBSaveCoreOptions.cpp
@@ -75,6 +75,26 @@ lldb::SaveCoreStyle SBSaveCoreOptions::GetStyle() const {
   return m_opaque_up->GetStyle();
 }
 
+void SBSaveCoreOptions::AddThread(lldb::tid_t tid) {
+  m_opaque_up->AddThread(tid);
+}
+
+bool SBSaveCoreOptions::RemoveThread(lldb::tid_t tid) {
+  return m_opaque_up->RemoveThread(tid);
+}
+
+uint32_t SBSaveCoreOptions::GetNumThreads() const {
+  return m_opaque_up->GetNumThreads();
+}
+
+lldb::tid_t SBSaveCoreOptions::GetThreadAtIndex(uint32_t idx,
+                                                SBError &error) const {
+  int64_t tid = m_opaque_up->GetThreadAtIndex(idx);
+  if (tid == -1)
+    error.SetErrorString("Invalid index");
+  return 0;
+}
+
 void SBSaveCoreOptions::Clear() {
   LLDB_INSTRUMENT_VA(this);
   m_opaque_up->Clear();
diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp
index 759ef3a8afe02..94e3cb85f31b9 100644
--- a/lldb/source/Core/PluginManager.cpp
+++ b/lldb/source/Core/PluginManager.cpp
@@ -714,6 +714,10 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp,
     return error;
   }
 
+  error = options.EnsureValidConfiguration();
+  if (error.Fail())
+    return error;
+
   if (!options.GetPluginName().has_value()) {
     // Try saving core directly from the process plugin first.
     llvm::Expected<bool> ret =
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 2c7005449f9d7..f6a9a5dd50d99 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6558,7 +6558,7 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
 
     if (make_core) {
       Process::CoreFileMemoryRanges core_ranges;
-      error = process_sp->CalculateCoreFileSaveRanges(core_style, 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();
@@ -6608,8 +6608,9 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
         mach_header.ncmds = segment_load_commands.size();
         mach_header.flags = 0;
         mach_header.reserved = 0;
-        ThreadList &thread_list = process_sp->GetThreadList();
-        const uint32_t num_threads = thread_list.GetSize();
+        std::vector<ThreadSP> thread_list =
+            process_sp->CalculateCoreFileThreadList(options);
+        const uint32_t num_threads = thread_list.size();
 
         // Make an array of LC_THREAD data items. Each one contains the
         // contents of the LC_THREAD load command. The data doesn't contain
@@ -6622,7 +6623,7 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
           LC_THREAD_data.SetByteOrder(byte_order);
         }
         for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
-          ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
+          ThreadSP thread_sp = thread_list.at(thread_idx);
           if (thread_sp) {
             switch (mach_header.cputype) {
             case llvm::MachO::CPU_TYPE_ARM64:
@@ -6730,7 +6731,7 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
         StructuredData::ArraySP threads(
             std::make_shared<StructuredData::Array>());
         for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
-          ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
+          ThreadSP thread_sp = thread_list.at(thread_idx);
           StructuredData::DictionarySP thread(
               std::make_shared<StructuredData::Dictionary>());
           thread->AddIntegerItem("thread_id", thread_sp->GetID());
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index de212c6b20da7..3d65596c93522 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -588,12 +588,13 @@ Status MinidumpFileBuilder::FixThreadStacks() {
 
 Status MinidumpFileBuilder::AddThreadList() {
   constexpr size_t minidump_thread_size = sizeof(llvm::minidump::Thread);
-  lldb_private::ThreadList thread_list = m_process_sp->GetThreadList();
+  std::vector<ThreadSP> thread_list =
+      m_process_sp->CalculateCoreFileThreadList(m_save_core_options);
 
   // 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.GetSize() * minidump_thread_size;
+                              thread_list.size() * minidump_thread_size;
   // save for the ability to set up RVA
   size_t size_before = GetCurrentDataEndOffset();
   Status error;
@@ -602,17 +603,17 @@ Status MinidumpFileBuilder::AddThreadList() {
     return error;
 
   llvm::support::ulittle32_t thread_count =
-      static_cast<llvm::support::ulittle32_t>(thread_list.GetSize());
+      static_cast<llvm::support::ulittle32_t>(thread_list.size());
   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();
+  const uint32_t num_threads = thread_list.size();
   Log *log = GetLog(LLDBLog::Object);
   for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
-    ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
+    ThreadSP thread_sp = thread_list.at(thread_idx);
     RegisterContextSP reg_ctx_sp(thread_sp->GetRegisterContext());
 
     if (!reg_ctx_sp) {
@@ -819,7 +820,7 @@ Status MinidumpFileBuilder::AddLinuxFileStreams() {
   return error;
 }
 
-Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
+Status MinidumpFileBuilder::AddMemoryList() {
   Status error;
 
   // We first save the thread stacks to ensure they fit in the first UINT32_MAX
@@ -828,18 +829,26 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
   // in accessible with a 32 bit offset.
   Process::CoreFileMemoryRanges ranges_32;
   Process::CoreFileMemoryRanges ranges_64;
-  error = m_process_sp->CalculateCoreFileSaveRanges(
-      SaveCoreStyle::eSaveCoreStackOnly, ranges_32);
+  Process::CoreFileMemoryRanges all_core_memory_ranges;
+  error = m_process_sp->CalculateCoreFileSaveRanges(m_save_core_options,
+                                                    all_core_memory_ranges);
   if (error.Fail())
     return error;
 
-  // Calculate totalsize including the current offset.
+  // Start by saving all of the stacks and ensuring they fit under the 32b
+  // limit.
   uint64_t total_size = GetCurrentDataEndOffset();
-  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();
+  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++;
+    }
   }
 
   if (total_size >= UINT32_MAX) {
@@ -849,14 +858,6 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
     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.
@@ -864,16 +865,13 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
   // 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() - stack_start_addresses.size()) *
-                  sizeof(llvm::minidump::MemoryDescriptor_64);
+    total_size += 256 + (all_core_memory_ranges.size() *
+                         sizeof(llvm::minidump::MemoryDescriptor_64));
 
   for (const auto &core_range : all_core_memory_ranges) {
     const addr_t range_size = core_range.range.size();
-    if (stack_start_addresses.count(core_range.range.start()) > 0)
-      // Don't double save stacks.
-      continue;
-
+    // We don't need to check for stacks here because we already removed them
+    // from all_core_memory_ranges.
     if (total_size + range_size < UINT32_MAX) {
       ranges_32.push_back(core_range);
       total_size += range_size;
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index 20564e0661f2a..c039492aa5c5a 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -75,8 +75,10 @@ lldb_private::Status WriteString(const std::string &to_write,
 class MinidumpFileBuilder {
 public:
   MinidumpFileBuilder(lldb::FileUP &&core_file,
-                      const lldb::ProcessSP &process_sp)
-      : m_process_sp(process_sp), m_core_file(std::move(core_file)){};
+                      const lldb::ProcessSP &process_sp,
+                      const lldb_private::SaveCoreOptions &save_core_options)
+      : m_process_sp(process_sp), m_core_file(std::move(core_file)),
+        m_save_core_options(save_core_options){};
 
   MinidumpFileBuilder(const MinidumpFileBuilder &) = delete;
   MinidumpFileBuilder &operator=(const MinidumpFileBuilder &) = delete;
@@ -103,7 +105,7 @@ class MinidumpFileBuilder {
   // Add Exception streams for any threads that stopped with exceptions.
   lldb_private::Status AddExceptions();
   // Add MemoryList stream, containing dumps of important memory segments
-  lldb_private::Status AddMemoryList(lldb::SaveCoreStyle core_style);
+  lldb_private::Status AddMemoryList();
   // Add MiscInfo stream, mainly providing ProcessId
   lldb_private::Status AddMiscInfo();
   // Add informative files about a Linux process
@@ -163,7 +165,9 @@ class MinidumpFileBuilder {
   // to duplicate it in the exception data.
   std::unordered_map<lldb::tid_t, llvm::minidump::LocationDescriptor>
       m_tid_to_reg_ctx;
+  std::unordered_set<lldb::addr_t> m_saved_stack_ranges;
   lldb::FileUP m_core_file;
+  lldb_private::SaveCoreOptions m_save_core_options;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_OBJECTFILE_MINIDUMP_MINIDUMPFILEBUILDER_H
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index faa144bfb5f6a..2380ff4c00ca9 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -74,7 +74,8 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
     error = maybe_core_file.takeError();
     return false;
   }
-  MinidumpFileBuilder builder(std::move(maybe_core_file.get()), process_sp);
+  MinidumpFileBuilder builder(std::move(maybe_core_file.get()), process_sp,
+                              options);
 
   Log *log = GetLog(LLDBLog::Object);
   error = builder.AddHeaderAndCalculateDirectories();
@@ -121,7 +122,7 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
 
   // Note: add memory HAS to be the last thing we do. It can overflow into 64b
   // land and many RVA's only support 32b
-  error = builder.AddMemoryList(core_style);
+  error = builder.AddMemoryList();
   if (error.Fail()) {
     LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString());
     return false;
diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp b/lldb/source/Symbol/SaveCoreOptions.cpp
index 0f6fdac1ce22e..7cb87374b6495 100644
--- a/lldb/source/Symbol/SaveCoreOptions.cpp
+++ b/lldb/source/Symbol/SaveCoreOptions.cpp
@@ -46,8 +46,59 @@ SaveCoreOptions::GetOutputFile() const {
   return m_file;
 }
 
+void SaveCoreOptions::AddThread(lldb::tid_t tid) {
+  if (m_threads_to_save.count(tid) == 0)
+    m_threads_to_save.emplace(tid);
+}
+
+bool SaveCoreOptions::RemoveThread(lldb::tid_t tid) {
+  if (m_threads_to_save.count(tid) == 0) {
+    m_threads_to_save.erase(tid);
+    return true;
+  }
+
+  return false;
+}
+
+size_t SaveCoreOptions::GetNumThreads() const {
+  return m_threads_to_save.size();
+}
+
+int64_t SaveCoreOptions::GetThreadAtIndex(size_t index) const {
+  auto iter = m_threads_to_save.begin();
+  while (index >= 0 && iter != m_threads_to_save.end()) {
+    if (index == 0)
+      return *iter;
+    index--;
+    iter++;
+  }
+
+  return -1;
+}
+
+bool SaveCoreOptions::ShouldSaveThread(lldb::tid_t tid) const {
+  // If the user specified no threads to save, then we save all threads.
+  if (m_threads_to_save.empty())
+    return true;
+  return m_threads_to_save.count(tid) > 0;
+}
+
+Status SaveCoreOptions::EnsureValidConfiguration() const {
+  Status error;
+  std::string error_str;
+  if (!m_threads_to_save.empty() && GetStyle() == lldb::eSaveCoreFull) {
+    error_str += "Cannot save a full core with a subset of threads\n";
+  }
+
+  if (!error_str.empty())
+    error.SetErrorString(error_str);
+
+  return error;
+}
+
 void SaveCoreOptions::Clear() {
   m_file = std::nullopt;
   m_plugin_name = std::nullopt;
   m_style = std::nullopt;
+  m_threads_to_save.clear();
 }
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index d5a639d9beacd..5c4a0f470670e 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6532,8 +6532,9 @@ static void AddRegion(const MemoryRegionInfo &region, bool try_dirty_pages,
 }
 
 static void SaveOffRegionsWithStackPointers(
-    Process &process, const MemoryRegionInfos &regions,
-    Process::CoreFileMemoryRanges &ranges, std::set<addr_t> &stack_ends) {
+    Process &process, const SaveCoreOptions &core_options,
+    const MemoryRegionInfos &regions, Process::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
@@ -6555,10 +6556,16 @@ static void SaveOffRegionsWithStackPointers(
     if (process.GetMemoryRegionInfo(sp, sp_region).Success()) {
       const size_t stack_head = (sp - red_zone);
       const size_t stack_size = sp_region.GetRange().GetRangeEnd() - stack_head;
+      // Even if the SaveCoreOption doesn't want us to save the stack
+      // we still need to populate the stack_ends set so it doesn't get saved
+      // off in other calls
       sp_region.GetRange().SetRangeBase(stack_head);
       sp_region.GetRange().SetByteSize(stack_size);
       stack_ends.insert(sp_region.GetRange().GetRangeEnd());
-      AddRegion(sp_region, try_dirty_pages, ranges);
+      // This will return true if the threadlist the user specified is empty,
+      // or contains the thread id from thread_sp.
+      if (core_options.ShouldSaveThread(thread_sp->GetID()))
+        AddRegion(sp_region, try_dirty_pages, ranges);
     }
   }
 }
@@ -6627,10 +6634,11 @@ static void GetCoreFileSaveRangesStackOnly(
   }
 }
 
-Status Process::CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
+Status Process::CalculateCoreFileSaveRanges(const SaveCoreOptions &options,
                                             CoreFileMemoryRanges &ranges) {
   lldb_private::MemoryRegionInfos regions;
   Status err = GetMemoryRegions(regions);
+  SaveCoreStyle core_style = options.GetStyle();
   if (err.Fail())
     return err;
   if (regions.empty())
@@ -6640,7 +6648,7 @@ Status Process::CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
                   "eSaveCoreUnspecified");
 
   std::set<addr_t> stack_ends;
-  SaveOffRegionsWithStackPointers(*this, regions, ranges, stack_ends);
+  SaveOffRegionsWithStackPointers(*this, options, regions, ranges, stack_ends);
 
   switch (core_style) {
   case eSaveCoreUnspecified:
@@ -6668,6 +6676,18 @@ Status Process::CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
   return Sta...
[truncated]

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 0891ccc0c68c35e17562c752955788f08054bcdb 1aaa1e29e1350d50cdabb0d06bc027ca070103ea --extensions cpp,h -- lldb/include/lldb/API/SBProcess.h lldb/include/lldb/API/SBSaveCoreOptions.h lldb/include/lldb/API/SBThread.h lldb/include/lldb/Symbol/SaveCoreOptions.h lldb/include/lldb/Target/Process.h lldb/source/API/SBSaveCoreOptions.cpp lldb/source/API/SBThread.cpp lldb/source/Core/PluginManager.cpp 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/Symbol/SaveCoreOptions.cpp lldb/source/Target/Process.cpp

View the diff from clang-format here.

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index c039492aa5..2e97f3e2fd 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -78,7 +78,7 @@ public:
                       const lldb::ProcessSP &process_sp,
                       const lldb_private::SaveCoreOptions &save_core_options)
       : m_process_sp(process_sp), m_core_file(std::move(core_file)),
-        m_save_core_options(save_core_options){};
+        m_save_core_options(save_core_options) {};
 
   MinidumpFileBuilder(const MinidumpFileBuilder &) = delete;
   MinidumpFileBuilder &operator=(const MinidumpFileBuilder &) = delete;
diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp b/lldb/source/Symbol/SaveCoreOptions.cpp
index 3c4ca2d852..658dd470e5 100644
--- a/lldb/source/Symbol/SaveCoreOptions.cpp
+++ b/lldb/source/Symbol/SaveCoreOptions.cpp
@@ -101,8 +101,8 @@ bool SaveCoreOptions::ShouldThreadBeSaved(lldb::tid_t tid) const {
   return m_threads_to_save.count(tid) > 0;
 }
 
-Status SaveCoreOptions::EnsureValidConfiguration(
-    lldb::ProcessSP process_sp) const {
+Status
+SaveCoreOptions::EnsureValidConfiguration(lldb::ProcessSP process_sp) const {
   Status error;
   std::string error_str;
   if (!m_threads_to_save.empty() && GetStyle() == lldb::eSaveCoreFull)
@@ -118,10 +118,10 @@ Status SaveCoreOptions::EnsureValidConfiguration(
   return error;
 }
 
-void SaveCoreOptions::ClearProcessSpecificData() { 
+void SaveCoreOptions::ClearProcessSpecificData() {
   // Deliberately not following the formatter style here to indicate that
   // this method will be expanded in the future.
-  m_threads_to_save.clear(); 
+  m_threads_to_save.clear();
 }
 
 void SaveCoreOptions::Clear() {

[clayborg]

Update the public API to user SBThread instead of the thread IDs and then add some tests.

[clayborg]

Do we want to do things by lldb::tid_t or do we want to pass in a lldb::SBThread? The reason I mention this is there are two integer identifiers from a SBThread:

  lldb::tid_t lldb::SBThread::GetThreadID() const;
  uint32_t lldb::SBThread::GetIndexID() const;

This API could and probably should be:

void AddThread(lldb::SBThread thread);

Because in order to get a lldb::tid_t users would need to have a SBThread already anyway. Then the user can't mess up by specifying the wrong ID (index ID instead of lldb::tid_t)

[Jlalond]

I think SBThread, because it solves my issue where I'm returning a thread at an index but need to support an error case

[clayborg]

returning a lldb::SBThread

[clayborg]

FYI: Totally fine to save things internally by lldb::tid_t since we know and control this code.

[clayborg]

No need to check the count, and no need to use emplace when we have a std::set<lldb::tid_t> and the type T is an integer:

m_threads_to_save.insert(tid);

[Jlalond]

Ah, good catch. Originally I was using a vector but then moved to a set.

[clayborg]

erase returns a bool if something was erased, lines 55-60 can just be:

  return m_threads_to_save.erase(tid) > 0;

[clayborg]

Remove {} on single line if statement per llvm coding guidelines

[clayborg]

Should we allow "full" core files to be emitted without some thread stacks? We could allow "full" to mean save all memory regions except the thread stacks for any threads that were not in the list. This would allow core files to be a bit smaller, but still contain all mapped memory except the thread stacks we didn't want. We can emit a warning when saving a core file saying something to this effect like we do for "stacks" and "modified-memory"

The reason I say this is for core files for and Apple systems. If you save a full style, all mach-o binaries are fully mapped into memory and you would have everything you need to load the core file as all system libraries are mapped into memory, and it would be nice to be able to not save all thread stacks if you don't need them. So maybe turn this into a warning we can expose to the user

[Jlalond]

I think you're right, but I don't actually know how to return a warning to the error that isn't a log. If you can point me to an example I'll make sure to implement it!

[clayborg]

We might need to pass down a lldb_private::Stream to the SaveCore functions. Just let this happen for now and we can do a follow up patch to provide feedback to the client.

[clayborg]

Use lldb::SBThread instead of lldb::tid_t

[clayborg]

If this list is large, then this function can be quite inefficient. But probably ok for now.

[Jlalond]

Should we support returning at an index then? To be clear, I do not like this code but due to my limited C++ knowledge didn't know a better way to return the Nth item of a set

[clayborg]

FYI this would fail to remove the tid because you have m_threads_to_save.count(tid) == 0 instead of m_threads_to_save.count(tid) != 0, but see the one line replacement code that will work below.

[jasonmolenda]

The MachO changes look fine to me. I had a few other small pieces of feedback, I think they're mostly matters of opinion so just take them as such, not something that must be changed.

[jasonmolenda]

Just take this as one possible opinion: I find the naming of this std::optional<ProcessSP> m_process_sp a little confusing. In this method we have a ProcessSP process_sp and m_process_sp which is an optional. Above we see code doing if (!process_sp) - cool. Then I come to if (m_process_sp.has_value()) and I'm trying to figure out what that method does in a std::shared_ptr and why it's different than the above bool.
I'm not sure m_process_sp is the best name, but we have no convention for this kind of thing today.

[jasonmolenda]

personal preference, but you could m_threads_to_save.insert({thread->GetID(), thread->GetBackingThread()}) here. Fine this way too.

[Jlalond]

As someone new to C++, sometimes the STL containers can get me with the clunkiness. I changed this to Greg's suggestion but I agree this is much better than defining a pair.

[jasonmolenda]

ShouldSaveThread makes this method sound like a caller can use this to request a thread is included. This is more like ThreadWillBeSaved maybe?

[clayborg]

Good call Jason. Switch to ThreadShouldBeSaved

[Jlalond]

I didn't call it ThreadShouldBeSaved I worded it more like a predicate ShouldThreadBeSaved. I like this more personally, but I'm not an expert in the codebase convention so please let me know if ThreadShouldBeSaved is more fitting

[jasonmolenda]

Stepping back a bit, do we gain anything in SaveCoreOptions by having m_process_sp be optional? Is there a distinction between an empty ProcessSP and no ProcessSP object?

[clayborg]

And mention that it will return an error if the process is already set and the SBThread doesn't match the current process

[clayborg]

This is a shared pointer, it being NULL is enough to say that there is no process set. Switch this to just a ProcessSP m_process_sp;

[clayborg]

We might want to add a ThreadSP SBThread::get_sp() function to avoid people possibly putting a raw "Thread *" into another shared pointer that isn't the same as the one that constructed it.

[bulbazord]

+1. Thread also has enable_shared_from_this, so that is an option as well.

[Jlalond]

Added this, and explicitly kept it lower case for the convention. Should we open an issue to look for casts to pointer and then GetSP() and clean them up?

[clayborg]

remove this as before we needed to use the ThreadList class which didn't have an iterator. Now we have a std::vector<ThreadSP> so we can use the build in iteration. See below.

[clayborg]

early return like Jason suggested and this will now be:

if (!thread_sp) {

[clayborg]

if (m_process_sp) {
  if (m_process_sp != thread->GetProcess()) {
    error.SetErrorString("Cannot add thread from a different process.");
    return error;
  }
} else {
  m_process_sp = thread->GetProcess();
}

[clayborg]

We should switch the parameter to a ThreadSP. But FYI: lldb_private::Thread inherits from std::enable_shared_from_this<Thread> which means you can call thread->shared_from_this() to get a shared pointer. All of our objects that are held in shared pointers inherit from std::enable_shared_from_this<T> to allow them to do this.

you can change this code to:

m_threads_to_save[thread_sp->GetID()] = thread_sp;

and remove the insert call below.

[clayborg]

Switch to using a ThreadSP

[clayborg]

Good call Jason. Switch to ThreadShouldBeSaved

[Jlalond]

Stepping back a bit, do we gain anything in SaveCoreOptions by having m_process_sp be optional? Is there a distinction between an empty ProcessSP and no ProcessSP object?

No, this was mostly a mistake on my part. I wanted to describe process as optional but required for advanced functions. Making it optional gave us no value over nullability.

[clayborg]

We might need to pass down a lldb_private::Stream to the SaveCore functions. Just let this happen for now and we can do a follow up patch to provide feedback to the client.

[clayborg]

These lines should go up to column 79, these lines are stopping at column 60. Re-wrap.

Maybe reword a bit:

  /// \note This will clear all process specific options if a different process 
  /// is specified from a previous call to this function or to any other 
  /// functions that set the process.

[clayborg]

I don't think we need this line as we don't need the num_threads variable anymore?

[clayborg]

We can't use thread_sp->GetIndexID() here, as before this was the index of the thread from thread_list, not the thread's index ID. Why? Because thread index IDs are unique for a process. If you have a process which starts with one thread, its index ID is 1. If this process creates a thread and we stop when this thread is created, we will create a new thread for it and its index ID will be 2. Then this thread exits and a new thread is created and we stop. We will now have two threads whose index IDs are 1 and 3. So we can't use this index as a zero based index into the LC_THREAD_datas array.

So we might need to still use a thread_idx in this for loop. See above.

[clayborg]

we need to revert this change back as we need a zero based thread index into LC_THREAD_datas:

for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
  ThreadSP thread_sp = thread_list[thread_idx];

We don't want to use for (const ThreadSP &thread_sp : thread_list) { anymore because we need the index for the thread_sp within thread_list

[clayborg]

This won't work anymore right? We now require a SBThread. We need to do this test with a live process now. We also need to test a few more things:

• call SBSaveCoreOptions::AddThread() with a default constructed SBThread and verify error
• call SBSaveCoreOptions::AddThread() with a valid SBThread and then one from another process and verify the error. If you are loading a core file, you can use the live process and the core file process as your second process to verify these errors after you load the core file
• verify changing the process resets the thread specific options both with another valid SBProcess or an default constructed SBProcess.

[clayborg]

change null process to a default constructed SBProcess... though Maybe we should require a valid process here and add a void ClearProcess() API to have this make more sense.

[github-actions]

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:

darker --check --diff -r 0891ccc0c68c35e17562c752955788f08054bcdb...1aaa1e29e1350d50cdabb0d06bc027ca070103ea lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py

View the diff from darker here.

--- python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py	2024-08-02 01:35:51.000000 +0000
+++ python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py	2024-08-02 19:20:39.788053 +0000
@@ -8,13 +8,15 @@
     basic_minidump = "basic_minidump.yaml"
     basic_minidump_different_pid = "basic_minidump_different_pid.yaml"
 
     def get_process_from_yaml(self, yaml_file):
         minidump_path = self.getBuildArtifact(os.path.basename(yaml_file) + ".dmp")
-        print ("minidump_path: " + minidump_path)
+        print("minidump_path: " + minidump_path)
         self.yaml2obj(yaml_file, minidump_path)
-        self.assertTrue(os.path.exists(minidump_path), "yaml2obj did not emit a minidump file")
+        self.assertTrue(
+            os.path.exists(minidump_path), "yaml2obj did not emit a minidump file"
+        )
         target = self.dbg.CreateTarget(None)
         process = target.LoadCore(minidump_path)
         self.assertTrue(process.IsValid(), "Process is not valid")
         return process
 
@@ -57,11 +59,10 @@
         removed_success = options.RemoveThread(thread)
         self.assertTrue(removed_success)
         removed_success = options.RemoveThread(thread)
         self.assertFalse(removed_success)
 
-
     def test_adding_thread_different_process(self):
         """Test adding and removing a thread from save core options."""
         options = lldb.SBSaveCoreOptions()
         process = self.get_basic_process()
         process_2 = self.get_basic_process_different_pid()

[clayborg]

Do the variable rename and all good

[clayborg]

rename to "process_sp"

[Michael137]

Looks like this broke the macOS buildbots: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/8988/execution/node/97/log/

FAIL: LLDB (/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/bin/clang-arm64) :: test_lc_note_dsym (TestSkinnyCorefile.TestSkinnyCorefile)
UNSUPPORTED: LLDB (/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/bin/clang-arm64) :: test_lc_note_dwarf (TestSkinnyCorefile.TestSkinnyCorefile) (skipping due to the following parameter(s): debug info format [This test is looking explicitly for a dSYM]) 
UNSUPPORTED: LLDB (/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/bin/clang-arm64) :: test_lc_note_dwo (TestSkinnyCorefile.TestSkinnyCorefile) (test case does not fall in any category of interest for this run) 
Restore dir to: /Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/tools/lldb/test
======================================================================
FAIL: test_lc_note_dsym (TestSkinnyCorefile.TestSkinnyCorefile)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1758, in test_method
    return attrvalue(self)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 148, in wrapper
    return func(*args, **kwargs)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/test/API/macosx/skinny-corefile/TestSkinnyCorefile.py", line 111, in test_lc_note
    self.runCmd("process save-core " + self.corefile)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1003, in runCmd
    self.assertTrue(self.res.Succeeded(), msg if (msg) else CMD_MSG(cmd))
AssertionError: False is not true : Command 'process save-core /Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/lldb-test-build.noindex/macosx/skinny-corefile/TestSkinnyCorefile.test_lc_note_dsym/process.core
Error output:
error: Failed to save core file for process: callers must set the core_style to something other than eSaveCoreUnspecified
' did not return successfully
Config=arm64-/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/bin/clang
----------------------------------------------------------------------
Ran 3 tests in 1.680s

Could you take a look?

[Jlalond]

@Michael137 ack, I'll make a fix

[Jlalond]

@Michael137 Has Green Dragon been down for awhile? I did break this test, but it looks like it was about a month ago. I'll make a patch to drop this as we let each respective flavor set their own default.

Edit: Disregard, the issue here is the new API takes the savecoreoptions, and we are incorrectly setting the machO defaults in the subsequent methods where we pass down the reference to the specified options. Working on a patch now.