diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h b/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h index 1df4e074680f5..124cb55eaf723 100644 --- a/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h +++ b/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h @@ -9,6 +9,7 @@ #ifndef LLDB_BREAKPOINT_BREAKPOINTLOCATIONCOLLECTION_H #define LLDB_BREAKPOINT_BREAKPOINTLOCATIONCOLLECTION_H +#include #include #include @@ -19,7 +20,15 @@ namespace lldb_private { class BreakpointLocationCollection { public: - BreakpointLocationCollection(); + /// Breakpoint locations don't keep their breakpoint owners alive, so neither + /// will a collection of breakpoint locations. However, if you need to + /// use this collection in a context where some of the breakpoints whose + /// locations are in the collection might get deleted during its lifespan, + /// then you need to make sure the breakpoints don't get deleted out from + /// under you. To do that, pass true for preserving, and so long as there is + /// a location for a given breakpoint in the collection, the breakpoint will + /// not get destroyed. + BreakpointLocationCollection(bool preserving = false); ~BreakpointLocationCollection(); @@ -164,6 +173,10 @@ class BreakpointLocationCollection { collection m_break_loc_collection; mutable std::mutex m_collection_mutex; + /// These are used if we're preserving breakpoints in this list: + const bool m_preserving_bkpts = false; + std::map, lldb::BreakpointSP> + m_preserved_bps; public: typedef llvm::iterator_range @@ -172,7 +185,6 @@ class BreakpointLocationCollection { return BreakpointLocationCollectionIterable(m_break_loc_collection); } }; - } // namespace lldb_private #endif // LLDB_BREAKPOINT_BREAKPOINTLOCATIONCOLLECTION_H diff --git a/lldb/source/Breakpoint/BreakpointLocationCollection.cpp b/lldb/source/Breakpoint/BreakpointLocationCollection.cpp index 1d052c5fc9bb6..97715836ec104 100644 --- a/lldb/source/Breakpoint/BreakpointLocationCollection.cpp +++ b/lldb/source/Breakpoint/BreakpointLocationCollection.cpp @@ -17,7 +17,8 @@ using namespace lldb; using namespace lldb_private; // BreakpointLocationCollection constructor -BreakpointLocationCollection::BreakpointLocationCollection() = default; +BreakpointLocationCollection::BreakpointLocationCollection(bool preserving) + : m_preserving_bkpts(preserving) {} // Destructor BreakpointLocationCollection::~BreakpointLocationCollection() = default; @@ -26,8 +27,19 @@ void BreakpointLocationCollection::Add(const BreakpointLocationSP &bp_loc) { std::lock_guard guard(m_collection_mutex); BreakpointLocationSP old_bp_loc = FindByIDPair(bp_loc->GetBreakpoint().GetID(), bp_loc->GetID()); - if (!old_bp_loc.get()) + if (!old_bp_loc.get()) { m_break_loc_collection.push_back(bp_loc); + if (m_preserving_bkpts) { + lldb::break_id_t bp_loc_id = bp_loc->GetID(); + Breakpoint &bkpt = bp_loc->GetBreakpoint(); + lldb::break_id_t bp_id = bkpt.GetID(); + std::pair key = + std::make_pair(bp_id, bp_loc_id); + auto entry = m_preserved_bps.find(key); + if (entry == m_preserved_bps.end()) + m_preserved_bps.emplace(key, bkpt.shared_from_this()); + } + } } bool BreakpointLocationCollection::Remove(lldb::break_id_t bp_id, @@ -35,6 +47,15 @@ bool BreakpointLocationCollection::Remove(lldb::break_id_t bp_id, std::lock_guard guard(m_collection_mutex); collection::iterator pos = GetIDPairIterator(bp_id, bp_loc_id); // Predicate if (pos != m_break_loc_collection.end()) { + if (m_preserving_bkpts) { + std::pair key = + std::make_pair(bp_id, bp_loc_id); + auto entry = m_preserved_bps.find(key); + if (entry == m_preserved_bps.end()) + assert(0 && "Breakpoint added to collection but not preserving map."); + else + m_preserved_bps.erase(entry); + } m_break_loc_collection.erase(pos); return true; } diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index 7fa1fc5d71f13..e9e534a57973e 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -87,11 +87,15 @@ bool StopInfo::HasTargetRunSinceMe() { namespace lldb_private { class StopInfoBreakpoint : public StopInfo { public: + // We use a "breakpoint preserving BreakpointLocationCollection because we + // may need to hand out the "breakpoint hit" list as any point, potentially + // after the breakpoint has been deleted. But we still need to refer to them. StopInfoBreakpoint(Thread &thread, break_id_t break_id) : StopInfo(thread, break_id), m_should_stop(false), m_should_stop_is_valid(false), m_should_perform_action(true), m_address(LLDB_INVALID_ADDRESS), m_break_id(LLDB_INVALID_BREAK_ID), - m_was_all_internal(false), m_was_one_shot(false) { + m_was_all_internal(false), m_was_one_shot(false), + m_async_stopped_locs(true) { StoreBPInfo(); } @@ -99,7 +103,8 @@ class StopInfoBreakpoint : public StopInfo { : StopInfo(thread, break_id), m_should_stop(should_stop), m_should_stop_is_valid(true), m_should_perform_action(true), m_address(LLDB_INVALID_ADDRESS), m_break_id(LLDB_INVALID_BREAK_ID), - m_was_all_internal(false), m_was_one_shot(false) { + m_was_all_internal(false), m_was_one_shot(false), + m_async_stopped_locs(true) { StoreBPInfo(); } @@ -699,6 +704,10 @@ class StopInfoBreakpoint : public StopInfo { lldb::break_id_t m_break_id; bool m_was_all_internal; bool m_was_one_shot; + /// The StopInfoBreakpoint lives after the stop, and could get queried + /// at any time so we need to make sure that it keeps the breakpoints for + /// each of the locations it records alive while it is around. That's what + /// The BreakpointPreservingLocationCollection does. BreakpointLocationCollection m_async_stopped_locs; }; diff --git a/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/Makefile b/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/Makefile new file mode 100644 index 0000000000000..695335e068c0c --- /dev/null +++ b/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/Makefile @@ -0,0 +1,4 @@ +C_SOURCES := main.c +CFLAGS_EXTRAS := -std=c99 + +include Makefile.rules diff --git a/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/TestCallbackDeletesBreakpoints.py b/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/TestCallbackDeletesBreakpoints.py new file mode 100644 index 0000000000000..2b8fc662ad42e --- /dev/null +++ b/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/TestCallbackDeletesBreakpoints.py @@ -0,0 +1,67 @@ +""" +Make sure that deleting breakpoints in another breakpoint +callback doesn't cause problems. +""" + + +import lldb +import lldbsuite.test.lldbutil as lldbutil +from lldbsuite.test.lldbtest import * + + +class TestBreakpointDeletionInCallback(TestBase): + NO_DEBUG_INFO_TESTCASE = True + + def test_breakpoint_deletion_in_callback(self): + self.build() + self.main_source_file = lldb.SBFileSpec("main.c") + self.delete_others_test() + + def delete_others_test(self): + """You might use the test implementation in several ways, say so here.""" + + # This function starts a process, "a.out" by default, sets a source + # breakpoint, runs to it, and returns the thread, process & target. + # It optionally takes an SBLaunchOption argument if you want to pass + # arguments or environment variables. + (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint( + self, "Set a breakpoint here", self.main_source_file + ) + + # Now set a breakpoint on "I did something" several times + # + bkpt_numbers = [] + for idx in range(0, 5): + bkpt_numbers.append( + lldbutil.run_break_set_by_source_regexp(self, "// Deletable location") + ) + + # And add commands to the third one to delete two others: + deleter = target.FindBreakpointByID(bkpt_numbers[2]) + self.assertTrue(deleter.IsValid(), "Deleter is a good breakpoint") + commands = lldb.SBStringList() + deleted_ids = [bkpt_numbers[0], bkpt_numbers[3]] + for idx in deleted_ids: + commands.AppendString(f"break delete {idx}") + + deleter.SetCommandLineCommands(commands) + + thread_list = lldbutil.continue_to_breakpoint(process, deleter) + self.assertEqual(len(thread_list), 1) + stop_data = thread.stop_reason_data + # There are 5 breakpoints so 10 break_id, break_loc_id. + self.assertEqual(len(stop_data), 10) + # We should have been able to get break ID's and locations for all the + # breakpoints that we originally hit, but some won't be around anymore: + for idx in range(0, 5): + bkpt_id = stop_data[idx * 2] + print(f"{idx}: {bkpt_id}") + self.assertIn(bkpt_id, bkpt_numbers, "Found breakpoints are right") + loc_id = stop_data[idx * 2 + 1] + self.assertEqual(loc_id, 1, "All breakpoints have one location") + bkpt = target.FindBreakpointByID(bkpt_id) + if bkpt_id in deleted_ids: + # Looking these up should be an error: + self.assertFalse(bkpt.IsValid(), "Deleted breakpoints are deleted") + else: + self.assertTrue(bkpt.IsValid(), "The rest are still valid") diff --git a/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/main.c b/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/main.c new file mode 100644 index 0000000000000..2ffb897b2f92d --- /dev/null +++ b/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/main.c @@ -0,0 +1,12 @@ +#include + +int do_something(int input) { + return input % 5; // Deletable location +} + +int main() { + printf("Set a breakpoint here.\n"); + do_something(100); + do_something(200); + return 0; +}