Feedback, move over to a process specific world and operate on SBThread instead of tids. Run Formatter.

Author: Jlalond

lldb/include/lldb/API/SBProcess.h

@@ -586,6 +586,7 @@ 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,6 +10,7 @@
 #define LLDB_API_SBSAVECOREOPTIONS_H
 
 #include "lldb/API/SBDefines.h"
+#include "lldb/API/SBThread.h"
 
 namespace lldb {
 
@@ -53,29 +54,25 @@ class LLDB_API SBSaveCoreOptions {
   /// \return The output file spec.
   SBFileSpec GetOutputFile() const;
 
+  /// Set the process to save, or unset if supplied with a null 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
+  /// an exisiting process is overriden.
+  SBError SetProcess(lldb::SBProcess process);
+
   /// Add a thread to save in the core file.
   ///
-  /// \param thread_id The thread ID to save.
-  void AddThread(lldb::tid_t thread_id);
+  /// \param thread The thread to save.
+  /// \note This will set the process if it is not already set.
+  SBError AddThread(lldb::SBThread thread);
 
   /// Remove a thread from the list of threads to save.
   ///
-  /// \param thread_id The thread ID to remove.
+  /// \param thread The thread 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;
+  bool RemoveThread(lldb::SBThread thread);
 
   /// Reset all options.
   void Clear();

lldb/include/lldb/API/SBThread.h

@@ -233,6 +233,7 @@ 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;

lldb/include/lldb/Symbol/SaveCoreOptions.h

@@ -13,8 +13,8 @@
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-types.h"
 
+#include <map>
 #include <optional>
-#include <set>
 #include <string>
 
 namespace lldb_private {
@@ -33,21 +33,24 @@ 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;
+  Status SetProcess(lldb::ProcessSP process_sp);
+
+  Status AddThread(lldb_private::Thread *thread);
+  bool RemoveThread(lldb_private::Thread *thread);
   bool ShouldSaveThread(lldb::tid_t tid) const;
 
-  Status EnsureValidConfiguration() const;
+  Status EnsureValidConfiguration(lldb::ProcessSP process_to_save) 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;
-  std::set<lldb::tid_t> m_threads_to_save;
+  std::optional<lldb::ProcessSP> m_process_sp;
+  std::map<lldb::tid_t, lldb::ThreadSP> m_threads_to_save;
 };
 } // namespace lldb_private
 

lldb/source/API/SBSaveCoreOptions.cpp

@@ -9,6 +9,8 @@
 #include "lldb/API/SBSaveCoreOptions.h"
 #include "lldb/API/SBError.h"
 #include "lldb/API/SBFileSpec.h"
+#include "lldb/API/SBProcess.h"
+#include "lldb/API/SBThread.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Symbol/SaveCoreOptions.h"
 #include "lldb/Utility/Instrumentation.h"
@@ -75,24 +77,16 @@ lldb::SaveCoreStyle SBSaveCoreOptions::GetStyle() const {
   return m_opaque_up->GetStyle();
 }
 
-void SBSaveCoreOptions::AddThread(lldb::tid_t tid) {
-  m_opaque_up->AddThread(tid);
+SBError SBSaveCoreOptions::SetProcess(lldb::SBProcess process) {
+  return m_opaque_up->SetProcess(process.GetSP());
 }
 
-bool SBSaveCoreOptions::RemoveThread(lldb::tid_t tid) {
-  return m_opaque_up->RemoveThread(tid);
+SBError SBSaveCoreOptions::AddThread(lldb::SBThread thread) {
+  return m_opaque_up->AddThread(thread.get());
 }
 
-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;
+bool SBSaveCoreOptions::RemoveThread(lldb::SBThread thread) {
+  return m_opaque_up->RemoveThread(thread.get());
 }
 
 void SBSaveCoreOptions::Clear() {

lldb/source/Core/PluginManager.cpp

@@ -714,7 +714,7 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp,
     return error;
   }
 
-  error = options.EnsureValidConfiguration();
+  error = options.EnsureValidConfiguration(process_sp);
   if (error.Fail())
     return error;
 

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

@@ -78,7 +78,7 @@ class MinidumpFileBuilder {
                       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;

lldb/source/Symbol/SaveCoreOptions.cpp

@@ -8,6 +8,8 @@
 
 #include "lldb/Symbol/SaveCoreOptions.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Thread.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -46,34 +48,48 @@ 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);
-}
+Status SaveCoreOptions::SetProcess(lldb::ProcessSP process_sp) {
+  Status error;
+  if (!process_sp) {
+    ClearProcessSpecificData();
+    m_process_sp = std::nullopt;
+    return error;
+  }
 
-bool SaveCoreOptions::RemoveThread(lldb::tid_t tid) {
-  if (m_threads_to_save.count(tid) == 0) {
-    m_threads_to_save.erase(tid);
-    return true;
+  if (!process_sp->IsValid()) {
+    error.SetErrorString("Cannot assign an invalid process.");
+    return error;
   }
 
-  return false;
-}
+  if (m_process_sp.has_value())
+    ClearProcessSpecificData();
 
-size_t SaveCoreOptions::GetNumThreads() const {
-  return m_threads_to_save.size();
+  m_process_sp = process_sp;
+  return error;
 }
 
-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++;
+Status SaveCoreOptions::AddThread(lldb_private::Thread *thread) {
+  Status error;
+  if (!thread) {
+    error.SetErrorString("Thread is null");
   }
 
-  return -1;
+  if (!m_process_sp.has_value())
+    m_process_sp = thread->GetProcess();
+
+  if (m_process_sp.value() != thread->GetProcess()) {
+    error.SetErrorString("Cannot add thread from a different process.");
+    return error;
+  }
+
+  std::pair<lldb::tid_t, lldb::ThreadSP> tid_pair(thread->GetID(),
+                                                  thread->GetBackingThread());
+  m_threads_to_save.insert(tid_pair);
+  return error;
+}
+
+bool SaveCoreOptions::RemoveThread(lldb_private::Thread *thread) {
+  return thread && m_threads_to_save.erase(thread->GetID()) > 0;
 }
 
 bool SaveCoreOptions::ShouldSaveThread(lldb::tid_t tid) const {
@@ -83,19 +99,25 @@ bool SaveCoreOptions::ShouldSaveThread(lldb::tid_t tid) const {
   return m_threads_to_save.count(tid) > 0;
 }
 
-Status SaveCoreOptions::EnsureValidConfiguration() const {
+Status SaveCoreOptions::EnsureValidConfiguration(
+    lldb::ProcessSP process_to_save) const {
   Status error;
   std::string error_str;
-  if (!m_threads_to_save.empty() && GetStyle() == lldb::eSaveCoreFull) {
+  if (!m_threads_to_save.empty() && GetStyle() == lldb::eSaveCoreFull)
     error_str += "Cannot save a full core with a subset of threads\n";
-  }
+
+  if (m_process_sp.has_value() && m_process_sp != process_to_save)
+    error_str += "Cannot save core for process using supplied core options. "
+                 "Options were constructed targeting a different process. \n";
 
   if (!error_str.empty())
     error.SetErrorString(error_str);
 
   return error;
 }
 
+void SaveCoreOptions::ClearProcessSpecificData() { m_threads_to_save.clear(); }
+
 void SaveCoreOptions::Clear() {
   m_file = std::nullopt;
   m_plugin_name = std::nullopt;

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

@@ -216,7 +216,7 @@ def test_save_linux_mini_dump_thread_options(self):
             )
             self.assertState(process.GetState(), lldb.eStateStopped)
 
-            thread_to_include = process.GetThreadAtIndex(0).GetThreadID()
+            thread_to_include = process.GetThreadAtIndex(0)
             options = lldb.SBSaveCoreOptions()
             thread_subset_spec = lldb.SBFileSpec(thread_subset_dmp)
             options.AddThread(thread_to_include)