From 5a756db04b1e5124b99fa44c162439fbf8385aee Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Fri, 10 Jan 2025 14:26:10 -0800 Subject: [PATCH 1/8] Make the thread list for SBSaveCoreOptions iterable --- lldb/include/lldb/API/SBSaveCoreOptions.h | 15 +++++++ lldb/include/lldb/Symbol/SaveCoreOptions.h | 8 +++- lldb/source/API/SBSaveCoreOptions.cpp | 13 ++++++ lldb/source/Symbol/SaveCoreOptions.cpp | 34 +++++++++++--- .../TestSBSaveCoreOptions.py | 45 +++++++++++++++++-- .../sbsavecoreoptions/basic_minidump.yaml | 10 +++++ 6 files changed, 114 insertions(+), 11 deletions(-) diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h index 74aa2fe5bd5f92..d480b6272749e6 100644 --- a/lldb/include/lldb/API/SBSaveCoreOptions.h +++ b/lldb/include/lldb/API/SBSaveCoreOptions.h @@ -111,6 +111,21 @@ class LLDB_API SBSaveCoreOptions { /// style specific regions. SBError AddMemoryRegionToSave(const SBMemoryRegionInfo ®ion); + /// Get the number of Threads to be saved + /// + /// \returns + /// The count of Threads to be saved. + uint32_t GetNumThreads() const; + + /// Get the Thread at the specified index. + /// + /// \param [in] idx + /// The index of the thread to return. + /// \returns + /// The thread at the specified index, or an empty thread if the index is + /// greater than or equal to the number of threads. + lldb::SBThread GetThreadAtIndex(uint32_t idx) const; + /// Reset all options. void Clear(); diff --git a/lldb/include/lldb/Symbol/SaveCoreOptions.h b/lldb/include/lldb/Symbol/SaveCoreOptions.h index d90d08026016dc..b33e02b2d72922 100644 --- a/lldb/include/lldb/Symbol/SaveCoreOptions.h +++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h @@ -13,7 +13,6 @@ #include "lldb/Utility/RangeMap.h" #include -#include #include #include @@ -47,6 +46,9 @@ class SaveCoreOptions { void AddMemoryRegionToSave(const lldb_private::MemoryRegionInfo ®ion); + std::optional GetThreadAtIndex(uint32_t idx) const; + uint32_t GetNumThreads() const; + void Clear(); private: @@ -56,8 +58,10 @@ class SaveCoreOptions { std::optional m_file; std::optional m_style; lldb::ProcessSP m_process_sp; - std::unordered_set m_threads_to_save; + std::unordered_map m_threads_to_save; MemoryRanges m_regions_to_save; + + std::vector m_thread_indexes; }; } // namespace lldb_private diff --git a/lldb/source/API/SBSaveCoreOptions.cpp b/lldb/source/API/SBSaveCoreOptions.cpp index c79b57fa62c2be..6e50145add822e 100644 --- a/lldb/source/API/SBSaveCoreOptions.cpp +++ b/lldb/source/API/SBSaveCoreOptions.cpp @@ -100,6 +100,19 @@ SBSaveCoreOptions::AddMemoryRegionToSave(const SBMemoryRegionInfo ®ion) { return SBError(); } +uint32_t lldb::SBSaveCoreOptions::GetNumThreads() const { + LLDB_INSTRUMENT_VA(this); + return m_opaque_up->GetNumThreads(); +} + +SBThread SBSaveCoreOptions::GetThreadAtIndex(uint32_t idx) const { + LLDB_INSTRUMENT_VA(this, idx); + std::optional thread_sp = m_opaque_up->GetThreadAtIndex(idx); + if (thread_sp) + return SBThread(thread_sp.value()); + return SBThread(); +} + void SBSaveCoreOptions::Clear() { LLDB_INSTRUMENT_VA(this); m_opaque_up->Clear(); diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp b/lldb/source/Symbol/SaveCoreOptions.cpp index 8d9aadece2152d..84c4a76f09453e 100644 --- a/lldb/source/Symbol/SaveCoreOptions.cpp +++ b/lldb/source/Symbol/SaveCoreOptions.cpp @@ -87,12 +87,33 @@ Status SaveCoreOptions::AddThread(lldb::ThreadSP thread_sp) { m_process_sp = thread_sp->GetProcess(); } - m_threads_to_save.insert(thread_sp->GetID()); + m_threads_to_save.insert({thread_sp->GetID(), thread_sp}); + m_thread_indexes.push_back(thread_sp->GetID()); return error; } bool SaveCoreOptions::RemoveThread(lldb::ThreadSP thread_sp) { - return thread_sp && m_threads_to_save.erase(thread_sp->GetID()) > 0; + if (!thread_sp) + return false; + if (m_threads_to_save.erase(thread_sp->GetID()) == 0) + return false; + + auto it = std::find(m_thread_indexes.begin(), m_thread_indexes.end(), + thread_sp->GetID()); + m_thread_indexes.erase(it); + return true; +} + +uint32_t SaveCoreOptions::GetNumThreads() const { + return m_threads_to_save.size(); +} + +std::optional +SaveCoreOptions::GetThreadAtIndex(uint32_t idx) const { + if (idx >= m_thread_indexes.size()) + return std::nullopt; + lldb::tid_t tid = m_thread_indexes[idx]; + return m_threads_to_save.find(tid)->second; } bool SaveCoreOptions::ShouldThreadBeSaved(lldb::tid_t tid) const { @@ -115,8 +136,8 @@ const MemoryRanges &SaveCoreOptions::GetCoreFileMemoryRanges() const { return m_regions_to_save; } -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) @@ -132,10 +153,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() { @@ -145,4 +166,5 @@ void SaveCoreOptions::Clear() { m_threads_to_save.clear(); m_process_sp.reset(); m_regions_to_save.Clear(); + m_thread_indexes.clear(); } diff --git a/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py b/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py index 40d0cc7e96ff48..00fddbde345154 100644 --- a/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py +++ b/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py @@ -4,15 +4,18 @@ from lldbsuite.test.decorators import * from lldbsuite.test.lldbtest import * + class SBSaveCoreOptionsAPICase(TestBase): 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") @@ -59,7 +62,6 @@ def test_adding_and_removing_thread(self): 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() @@ -79,3 +81,40 @@ def test_adding_thread_different_process(self): self.assertTrue(error.Fail()) error = options.AddThread(thread) self.assertTrue(error.Success()) + + def test_removing_and_adding_insertion_order(self): + """Test insertion order is maintained when removing and adding threads.""" + options = lldb.SBSaveCoreOptions() + process = self.get_basic_process() + threads = [] + for x in range(0, 3): + thread = process.GetThreadAtIndex(x) + threads.append(thread) + error = options.AddThread(thread) + self.assertTrue(error.Success()) + + # Get the middle thread, remove it, and insert it at the end. + middle_thread = threads[1] + self.assertTrue(options.RemoveThread(middle_thread)) + num_threads = options.GetNumThreads() + self.assertEqual(num_threads, 2) + error = options.AddThread(middle_thread) + self.assertTrue(error.Success()) + num_threads = options.GetNumThreads() + self.assertEqual(num_threads, 3) + thread_at_last_index = options.GetThreadAtIndex(2) + self.assertEqual(thread_at_last_index.id, middle_thread.id) + thread_at_middle_index = options.GetThreadAtIndex(1) + self.assertEqual(thread_at_middle_index.id, threads[2].id) + + # Pop the front thread, remove it, and insert it at the end. + front_thread = threads[0] + self.assertTrue(options.RemoveThread(front_thread)) + num_threads = options.GetNumThreads() + self.assertEqual(num_threads, 2) + error = options.AddThread(front_thread) + self.assertTrue(error.Success()) + num_threads = options.GetNumThreads() + self.assertEqual(num_threads, 3) + thread_at_last_index = options.GetThreadAtIndex(2) + self.assertEqual(thread_at_last_index.id, front_thread.id) diff --git a/lldb/test/API/python_api/sbsavecoreoptions/basic_minidump.yaml b/lldb/test/API/python_api/sbsavecoreoptions/basic_minidump.yaml index 993c7da21225a9..96302fbfb6b5c2 100644 --- a/lldb/test/API/python_api/sbsavecoreoptions/basic_minidump.yaml +++ b/lldb/test/API/python_api/sbsavecoreoptions/basic_minidump.yaml @@ -24,3 +24,13 @@ Streams: Stack: Start of Memory Range: 0x00007FFFC8D0E000 Content: 'DEADBEEF' + - Thread Id: 0x000074DE + Context: 0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000B0010000000000033000000000000000000000002020100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000040109600000000000100000000000000000000000000000068E7D0C8FF7F000068E7D0C8FF7F000097E6D0C8FF7F000010109600000000000000000000000000020000000000000088E4D0C8FF7F0000603FFF85C77F0000F00340000000000080E7D0C8FF7F000000000000000000000000000000000000E0034000000000007F0300000000000000000000000000000000000000000000801F0000FFFF00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF252525252525252525252525252525250000000000000000000000000000000000000000000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000FF00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 + Stack: + Start of Memory Range: 0x00007FFFC8D0A000 + Content: 'BEEFDEAD' + - Thread Id: 0x000074DF + Context: 0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000B0010000000000033000000000000000000000002020100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000040109600000000000100000000000000000000000000000068E7D0C8FF7F000068E7D0C8FF7F000097E6D0C8FF7F000010109600000000000000000000000000020000000000000088E4D0C8FF7F0000603FFF85C77F0000F00340000000000080E7D0C8FF7F000000000000000000000000000000000000E0034000000000007F0300000000000000000000000000000000000000000000801F0000FFFF00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF252525252525252525252525252525250000000000000000000000000000000000000000000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000FF00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 + Stack: + Start of Memory Range: 0x00007FFFC8DFF000 + Content: 'BAADBEEF' From 7638844607c544c13ee4ef4327d31a8dff470e09 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Fri, 10 Jan 2025 15:10:58 -0800 Subject: [PATCH 2/8] Comment cleanup --- lldb/include/lldb/API/SBSaveCoreOptions.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h index d480b6272749e6..c102b63df287b0 100644 --- a/lldb/include/lldb/API/SBSaveCoreOptions.h +++ b/lldb/include/lldb/API/SBSaveCoreOptions.h @@ -120,10 +120,10 @@ class LLDB_API SBSaveCoreOptions { /// Get the Thread at the specified index. /// /// \param [in] idx - /// The index of the thread to return. + /// The zero based index of the thread to return. /// \returns - /// The thread at the specified index, or an empty thread if the index is - /// greater than or equal to the number of threads. + /// The thread at the specified index, or an invalid SBThread if the index + /// is greater than or equal to the number of threads. lldb::SBThread GetThreadAtIndex(uint32_t idx) const; /// Reset all options. From a1c2d9d3db19b4cd101daf5317928747f7250c79 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Fri, 10 Jan 2025 16:12:57 -0800 Subject: [PATCH 3/8] Refactor to a thread collection SP --- lldb/include/lldb/API/SBSaveCoreOptions.h | 17 +++------- lldb/include/lldb/API/SBThreadCollection.h | 2 +- lldb/include/lldb/Symbol/SaveCoreOptions.h | 5 +-- lldb/source/API/SBSaveCoreOptions.cpp | 12 ++----- lldb/source/Symbol/SaveCoreOptions.cpp | 33 ++++++------------- .../TestSBSaveCoreOptions.py | 32 +++++------------- 6 files changed, 28 insertions(+), 73 deletions(-) diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h index c102b63df287b0..3c332534c7b9e4 100644 --- a/lldb/include/lldb/API/SBSaveCoreOptions.h +++ b/lldb/include/lldb/API/SBSaveCoreOptions.h @@ -14,6 +14,7 @@ #include "lldb/API/SBFileSpec.h" #include "lldb/API/SBProcess.h" #include "lldb/API/SBThread.h" +#include "lldb/API/SBThreadCollection.h" namespace lldb { @@ -111,26 +112,18 @@ class LLDB_API SBSaveCoreOptions { /// style specific regions. SBError AddMemoryRegionToSave(const SBMemoryRegionInfo ®ion); - /// Get the number of Threads to be saved + /// Get an unsorted collection of the threads to save /// /// \returns - /// The count of Threads to be saved. - uint32_t GetNumThreads() const; - - /// Get the Thread at the specified index. - /// - /// \param [in] idx - /// The zero based index of the thread to return. - /// \returns - /// The thread at the specified index, or an invalid SBThread if the index - /// is greater than or equal to the number of threads. - lldb::SBThread GetThreadAtIndex(uint32_t idx) const; + /// An unsorted collection with zero or more threads. + SBThreadCollection GetThreadsToSave() const; /// Reset all options. void Clear(); protected: friend class SBProcess; + friend class SBThreadCollection; lldb_private::SaveCoreOptions &ref() const; private: diff --git a/lldb/include/lldb/API/SBThreadCollection.h b/lldb/include/lldb/API/SBThreadCollection.h index fe57a6b95d909c..5a052e62460260 100644 --- a/lldb/include/lldb/API/SBThreadCollection.h +++ b/lldb/include/lldb/API/SBThreadCollection.h @@ -48,7 +48,7 @@ class LLDB_API SBThreadCollection { private: friend class SBProcess; friend class SBThread; - + friend class SBSaveCoreOptions; lldb::ThreadCollectionSP m_opaque_sp; }; diff --git a/lldb/include/lldb/Symbol/SaveCoreOptions.h b/lldb/include/lldb/Symbol/SaveCoreOptions.h index b33e02b2d72922..f50ba018c6ca7f 100644 --- a/lldb/include/lldb/Symbol/SaveCoreOptions.h +++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h @@ -46,8 +46,7 @@ class SaveCoreOptions { void AddMemoryRegionToSave(const lldb_private::MemoryRegionInfo ®ion); - std::optional GetThreadAtIndex(uint32_t idx) const; - uint32_t GetNumThreads() const; + lldb::ThreadCollectionSP GetThreadsToSave() const; void Clear(); @@ -60,8 +59,6 @@ class SaveCoreOptions { lldb::ProcessSP m_process_sp; std::unordered_map m_threads_to_save; MemoryRanges m_regions_to_save; - - std::vector m_thread_indexes; }; } // namespace lldb_private diff --git a/lldb/source/API/SBSaveCoreOptions.cpp b/lldb/source/API/SBSaveCoreOptions.cpp index 6e50145add822e..b24bd8ba68cd82 100644 --- a/lldb/source/API/SBSaveCoreOptions.cpp +++ b/lldb/source/API/SBSaveCoreOptions.cpp @@ -100,17 +100,9 @@ SBSaveCoreOptions::AddMemoryRegionToSave(const SBMemoryRegionInfo ®ion) { return SBError(); } -uint32_t lldb::SBSaveCoreOptions::GetNumThreads() const { +lldb::SBThreadCollection SBSaveCoreOptions::GetThreadsToSave() const { LLDB_INSTRUMENT_VA(this); - return m_opaque_up->GetNumThreads(); -} - -SBThread SBSaveCoreOptions::GetThreadAtIndex(uint32_t idx) const { - LLDB_INSTRUMENT_VA(this, idx); - std::optional thread_sp = m_opaque_up->GetThreadAtIndex(idx); - if (thread_sp) - return SBThread(thread_sp.value()); - return SBThread(); + return SBThreadCollection(m_opaque_up->GetThreadsToSave()); } void SBSaveCoreOptions::Clear() { diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp b/lldb/source/Symbol/SaveCoreOptions.cpp index 84c4a76f09453e..84afc585a83164 100644 --- a/lldb/source/Symbol/SaveCoreOptions.cpp +++ b/lldb/source/Symbol/SaveCoreOptions.cpp @@ -88,32 +88,11 @@ Status SaveCoreOptions::AddThread(lldb::ThreadSP thread_sp) { } m_threads_to_save.insert({thread_sp->GetID(), thread_sp}); - m_thread_indexes.push_back(thread_sp->GetID()); return error; } bool SaveCoreOptions::RemoveThread(lldb::ThreadSP thread_sp) { - if (!thread_sp) - return false; - if (m_threads_to_save.erase(thread_sp->GetID()) == 0) - return false; - - auto it = std::find(m_thread_indexes.begin(), m_thread_indexes.end(), - thread_sp->GetID()); - m_thread_indexes.erase(it); - return true; -} - -uint32_t SaveCoreOptions::GetNumThreads() const { - return m_threads_to_save.size(); -} - -std::optional -SaveCoreOptions::GetThreadAtIndex(uint32_t idx) const { - if (idx >= m_thread_indexes.size()) - return std::nullopt; - lldb::tid_t tid = m_thread_indexes[idx]; - return m_threads_to_save.find(tid)->second; + return thread_sp && m_threads_to_save.erase(thread_sp->GetID()) > 0; } bool SaveCoreOptions::ShouldThreadBeSaved(lldb::tid_t tid) const { @@ -136,6 +115,15 @@ const MemoryRanges &SaveCoreOptions::GetCoreFileMemoryRanges() const { return m_regions_to_save; } +lldb::ThreadCollectionSP SaveCoreOptions::GetThreadsToSave() const { + lldb::ThreadCollectionSP threadcollection_sp = + std::make_shared(); + for (const auto &thread : m_threads_to_save) { + threadcollection_sp->AddThread(thread.second); + } + return threadcollection_sp; +} + Status SaveCoreOptions::EnsureValidConfiguration(lldb::ProcessSP process_sp) const { Status error; @@ -166,5 +154,4 @@ void SaveCoreOptions::Clear() { m_threads_to_save.clear(); m_process_sp.reset(); m_regions_to_save.Clear(); - m_thread_indexes.clear(); } diff --git a/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py b/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py index 00fddbde345154..5a754c7be2a4d3 100644 --- a/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py +++ b/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py @@ -93,28 +93,14 @@ def test_removing_and_adding_insertion_order(self): error = options.AddThread(thread) self.assertTrue(error.Success()) - # Get the middle thread, remove it, and insert it at the end. + # Get the middle thread, remove it, and insert it back. middle_thread = threads[1] self.assertTrue(options.RemoveThread(middle_thread)) - num_threads = options.GetNumThreads() - self.assertEqual(num_threads, 2) - error = options.AddThread(middle_thread) - self.assertTrue(error.Success()) - num_threads = options.GetNumThreads() - self.assertEqual(num_threads, 3) - thread_at_last_index = options.GetThreadAtIndex(2) - self.assertEqual(thread_at_last_index.id, middle_thread.id) - thread_at_middle_index = options.GetThreadAtIndex(1) - self.assertEqual(thread_at_middle_index.id, threads[2].id) - - # Pop the front thread, remove it, and insert it at the end. - front_thread = threads[0] - self.assertTrue(options.RemoveThread(front_thread)) - num_threads = options.GetNumThreads() - self.assertEqual(num_threads, 2) - error = options.AddThread(front_thread) - self.assertTrue(error.Success()) - num_threads = options.GetNumThreads() - self.assertEqual(num_threads, 3) - thread_at_last_index = options.GetThreadAtIndex(2) - self.assertEqual(thread_at_last_index.id, front_thread.id) + thread_collection = options.GetThreadsToSave() + self.assertTrue(thread_collection is not None) + self.assertEqual(thread_collection.GetSize(), 2) + # error = options.AddThread(middle_thread) + # self.assertTrue(error.Success()) + # thread_collection = options.GetThreadsToSave() + # self.assertEqual(thread_collection.GetSize(), 3) + # self.assertIn(middle_thread, thread_collection) From a93b5153d838cf12acc1bd42bc5576faf8c4623f Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Fri, 10 Jan 2025 16:19:37 -0800 Subject: [PATCH 4/8] Rephrase from collection to copy --- lldb/include/lldb/API/SBSaveCoreOptions.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h index 3c332534c7b9e4..8fbeda7ed80af2 100644 --- a/lldb/include/lldb/API/SBSaveCoreOptions.h +++ b/lldb/include/lldb/API/SBSaveCoreOptions.h @@ -112,10 +112,10 @@ class LLDB_API SBSaveCoreOptions { /// style specific regions. SBError AddMemoryRegionToSave(const SBMemoryRegionInfo ®ion); - /// Get an unsorted collection of the threads to save + /// Get an unsorted copy of all threads to save /// /// \returns - /// An unsorted collection with zero or more threads. + /// An unsorted copy of all threads to save. SBThreadCollection GetThreadsToSave() const; /// Reset all options. From 47c0c505e035e42043580d3ea887bc5e8fd8181c Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Fri, 10 Jan 2025 16:20:37 -0800 Subject: [PATCH 5/8] Uncomment tests --- .../sbsavecoreoptions/TestSBSaveCoreOptions.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py b/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py index 5a754c7be2a4d3..ace84e8497a596 100644 --- a/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py +++ b/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py @@ -99,8 +99,8 @@ def test_removing_and_adding_insertion_order(self): thread_collection = options.GetThreadsToSave() self.assertTrue(thread_collection is not None) self.assertEqual(thread_collection.GetSize(), 2) - # error = options.AddThread(middle_thread) - # self.assertTrue(error.Success()) - # thread_collection = options.GetThreadsToSave() - # self.assertEqual(thread_collection.GetSize(), 3) - # self.assertIn(middle_thread, thread_collection) + error = options.AddThread(middle_thread) + self.assertTrue(error.Success()) + thread_collection = options.GetThreadsToSave() + self.assertEqual(thread_collection.GetSize(), 3) + self.assertIn(middle_thread, thread_collection) From d6d45adf57c6a1e3472d1a2a1d500da4f2cbe32f Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Mon, 13 Jan 2025 09:26:44 -0800 Subject: [PATCH 6/8] Save core doesn't store thread info, instead we get the threadlist from process --- lldb/include/lldb/Symbol/SaveCoreOptions.h | 2 +- lldb/source/Symbol/SaveCoreOptions.cpp | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/lldb/include/lldb/Symbol/SaveCoreOptions.h b/lldb/include/lldb/Symbol/SaveCoreOptions.h index f50ba018c6ca7f..766c396b6debaf 100644 --- a/lldb/include/lldb/Symbol/SaveCoreOptions.h +++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h @@ -57,7 +57,7 @@ class SaveCoreOptions { std::optional m_file; std::optional m_style; lldb::ProcessSP m_process_sp; - std::unordered_map m_threads_to_save; + std::unordered_set m_threads_to_save; MemoryRanges m_regions_to_save; }; } // namespace lldb_private diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp b/lldb/source/Symbol/SaveCoreOptions.cpp index 84afc585a83164..e7a394286ea959 100644 --- a/lldb/source/Symbol/SaveCoreOptions.cpp +++ b/lldb/source/Symbol/SaveCoreOptions.cpp @@ -87,7 +87,7 @@ Status SaveCoreOptions::AddThread(lldb::ThreadSP thread_sp) { m_process_sp = thread_sp->GetProcess(); } - m_threads_to_save.insert({thread_sp->GetID(), thread_sp}); + m_threads_to_save.insert(thread_sp->GetID()); return error; } @@ -118,9 +118,13 @@ const MemoryRanges &SaveCoreOptions::GetCoreFileMemoryRanges() const { lldb::ThreadCollectionSP SaveCoreOptions::GetThreadsToSave() const { lldb::ThreadCollectionSP threadcollection_sp = std::make_shared(); - for (const auto &thread : m_threads_to_save) { - threadcollection_sp->AddThread(thread.second); - } + if (!m_process_sp) + return threadcollection_sp; + + ThreadList &thread_list = m_process_sp->GetThreadList(); + for (const auto &tid : m_threads_to_save) + threadcollection_sp->AddThread(thread_list.FindThreadByID(tid)); + return threadcollection_sp; } From 642f63cddc72a144002de7f98adeef492963acf5 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Tue, 14 Jan 2025 14:56:28 -0800 Subject: [PATCH 7/8] Reorganize code per suggestion --- lldb/include/lldb/API/SBSaveCoreOptions.h | 3 ++- lldb/source/Symbol/SaveCoreOptions.cpp | 29 ++++++++++++----------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h index 8fbeda7ed80af2..7852858f8ade9d 100644 --- a/lldb/include/lldb/API/SBSaveCoreOptions.h +++ b/lldb/include/lldb/API/SBSaveCoreOptions.h @@ -115,7 +115,8 @@ class LLDB_API SBSaveCoreOptions { /// Get an unsorted copy of all threads to save /// /// \returns - /// An unsorted copy of all threads to save. + /// An unsorted copy of all threads to save. If no process is specified + /// an empty collection will be returned. SBThreadCollection GetThreadsToSave() const; /// Reset all options. diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp b/lldb/source/Symbol/SaveCoreOptions.cpp index e7a394286ea959..ef382c1b73c877 100644 --- a/lldb/source/Symbol/SaveCoreOptions.cpp +++ b/lldb/source/Symbol/SaveCoreOptions.cpp @@ -114,20 +114,6 @@ void SaveCoreOptions::AddMemoryRegionToSave( const MemoryRanges &SaveCoreOptions::GetCoreFileMemoryRanges() const { return m_regions_to_save; } - -lldb::ThreadCollectionSP SaveCoreOptions::GetThreadsToSave() const { - lldb::ThreadCollectionSP threadcollection_sp = - std::make_shared(); - if (!m_process_sp) - return threadcollection_sp; - - ThreadList &thread_list = m_process_sp->GetThreadList(); - for (const auto &tid : m_threads_to_save) - threadcollection_sp->AddThread(thread_list.FindThreadByID(tid)); - - return threadcollection_sp; -} - Status SaveCoreOptions::EnsureValidConfiguration(lldb::ProcessSP process_sp) const { Status error; @@ -145,6 +131,21 @@ SaveCoreOptions::EnsureValidConfiguration(lldb::ProcessSP process_sp) const { return error; } +lldb::ThreadCollectionSP SaveCoreOptions::GetThreadsToSave() const { + lldb::ThreadCollectionSP threadcollection_sp = + std::make_shared(); + + // In cases where no process is set, such as when no threads are specified. + if (!m_process_sp) + return threadcollection_sp; + + ThreadList &thread_list = m_process_sp->GetThreadList(); + for (const auto &tid : m_threads_to_save) + threadcollection_sp->AddThread(thread_list.FindThreadByID(tid)); + + return threadcollection_sp; +} + void SaveCoreOptions::ClearProcessSpecificData() { // Deliberately not following the formatter style here to indicate that // this method will be expanded in the future. From b7653081093784431cd28d920498ad26fc354c27 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde Date: Wed, 15 Jan 2025 09:33:24 -0800 Subject: [PATCH 8/8] Pass back vector of thread sp and construct ThreadCollectionSP at the SB layer --- lldb/include/lldb/Symbol/SaveCoreOptions.h | 3 ++- lldb/source/API/SBSaveCoreOptions.cpp | 6 +++++- lldb/source/Symbol/SaveCoreOptions.cpp | 13 ++++++------- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/lldb/include/lldb/Symbol/SaveCoreOptions.h b/lldb/include/lldb/Symbol/SaveCoreOptions.h index 766c396b6debaf..bcf0087fbea5c4 100644 --- a/lldb/include/lldb/Symbol/SaveCoreOptions.h +++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h @@ -9,6 +9,7 @@ #ifndef LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H #define LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H +#include "lldb/Target/ThreadCollection.h" #include "lldb/Utility/FileSpec.h" #include "lldb/Utility/RangeMap.h" @@ -46,7 +47,7 @@ class SaveCoreOptions { void AddMemoryRegionToSave(const lldb_private::MemoryRegionInfo ®ion); - lldb::ThreadCollectionSP GetThreadsToSave() const; + lldb_private::ThreadCollection::collection GetThreadsToSave() const; void Clear(); diff --git a/lldb/source/API/SBSaveCoreOptions.cpp b/lldb/source/API/SBSaveCoreOptions.cpp index b24bd8ba68cd82..35b9da569dfa15 100644 --- a/lldb/source/API/SBSaveCoreOptions.cpp +++ b/lldb/source/API/SBSaveCoreOptions.cpp @@ -10,6 +10,7 @@ #include "lldb/API/SBMemoryRegionInfo.h" #include "lldb/Host/FileSystem.h" #include "lldb/Symbol/SaveCoreOptions.h" +#include "lldb/Target/ThreadCollection.h" #include "lldb/Utility/Instrumentation.h" #include "Utils.h" @@ -102,7 +103,10 @@ SBSaveCoreOptions::AddMemoryRegionToSave(const SBMemoryRegionInfo ®ion) { lldb::SBThreadCollection SBSaveCoreOptions::GetThreadsToSave() const { LLDB_INSTRUMENT_VA(this); - return SBThreadCollection(m_opaque_up->GetThreadsToSave()); + lldb::ThreadCollectionSP threadcollection_sp = + std::make_shared( + m_opaque_up->GetThreadsToSave()); + return SBThreadCollection(threadcollection_sp); } void SBSaveCoreOptions::Clear() { diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp b/lldb/source/Symbol/SaveCoreOptions.cpp index ef382c1b73c877..c9f6efeb25d22d 100644 --- a/lldb/source/Symbol/SaveCoreOptions.cpp +++ b/lldb/source/Symbol/SaveCoreOptions.cpp @@ -131,19 +131,18 @@ SaveCoreOptions::EnsureValidConfiguration(lldb::ProcessSP process_sp) const { return error; } -lldb::ThreadCollectionSP SaveCoreOptions::GetThreadsToSave() const { - lldb::ThreadCollectionSP threadcollection_sp = - std::make_shared(); - +lldb_private::ThreadCollection::collection +SaveCoreOptions::GetThreadsToSave() const { + lldb_private::ThreadCollection::collection thread_collection; // In cases where no process is set, such as when no threads are specified. if (!m_process_sp) - return threadcollection_sp; + return thread_collection; ThreadList &thread_list = m_process_sp->GetThreadList(); for (const auto &tid : m_threads_to_save) - threadcollection_sp->AddThread(thread_list.FindThreadByID(tid)); + thread_collection.push_back(thread_list.FindThreadByID(tid)); - return threadcollection_sp; + return thread_collection; } void SaveCoreOptions::ClearProcessSpecificData() {