Author: Felipe de Azevedo Piovezan Date: 2025-12-03T10:51:27Z New Revision: c0f0936f5a47270d47486f6d5860b5f8e30e0e32
URL: https://github.com/llvm/llvm-project/commit/c0f0936f5a47270d47486f6d5860b5f8e30e0e32 DIFF: https://github.com/llvm/llvm-project/commit/c0f0936f5a47270d47486f6d5860b5f8e30e0e32.diff LOG: [lldb] Fix ThreadPlanStepOut::DoPlanExplainsStop inspection of BreakpointSite (#169799) Suppose two threads are performing the exact same step out plan. They will both have an internal breakpoint set at their parent frame. Now supposed both of those breakpoints are in the same address (i.e. the same BreakpointSite). At the end of `ThreadPlanStepOut::DoPlanExplainsStop`, we see this: ``` // If there was only one owner, then we're done. But if we also hit // some user breakpoint on our way out, we should mark ourselves as // done, but also not claim to explain the stop, since it is more // important to report the user breakpoint than the step out // completion. if (site_sp->GetNumberOfConstituents() == 1) return true; ``` In other words, the plan looks at the name number of constituents of the site to decide whether it explains the stop, the logic being that a _user_ might have put a breakpoint there. However, the implementation is not correct; in particular, it will fail in the situation described above. We should only care about non-internal breakpoints that would stop for the current thread. It is tricky to test this, as it depends on the timing of threads, but I was able to consistently reproduce the issue with a swift program using concurrency. rdar://165481473 Added: Modified: lldb/include/lldb/Breakpoint/BreakpointSite.h lldb/source/Breakpoint/BreakpointSite.cpp lldb/source/Target/ThreadPlanStepOut.cpp Removed: ################################################################################ diff --git a/lldb/include/lldb/Breakpoint/BreakpointSite.h b/lldb/include/lldb/Breakpoint/BreakpointSite.h index a935b2441c02a..e189ed77e261b 100644 --- a/lldb/include/lldb/Breakpoint/BreakpointSite.h +++ b/lldb/include/lldb/Breakpoint/BreakpointSite.h @@ -156,6 +156,10 @@ class BreakpointSite : public std::enable_shared_from_this<BreakpointSite>, /// would be valid for this thread, false otherwise. bool ValidForThisThread(Thread &thread); + /// Returns true if at least one constituent is both public and valid for + /// `thread`. + bool ContainsUserBreakpointForThread(Thread &thread); + /// Print a description of this breakpoint site to the stream \a s. /// GetDescription tells you about the breakpoint site's constituents. Use /// BreakpointSite::Dump(Stream *) to get information about the breakpoint diff --git a/lldb/source/Breakpoint/BreakpointSite.cpp b/lldb/source/Breakpoint/BreakpointSite.cpp index fd7666be6b1bf..8639379afe1df 100644 --- a/lldb/source/Breakpoint/BreakpointSite.cpp +++ b/lldb/source/Breakpoint/BreakpointSite.cpp @@ -168,6 +168,22 @@ bool BreakpointSite::ValidForThisThread(Thread &thread) { return m_constituents.ValidForThisThread(thread); } +bool BreakpointSite::ContainsUserBreakpointForThread(Thread &thread) { + if (ThreadSP backed_thread = thread.GetBackedThread()) + return ContainsUserBreakpointForThread(*backed_thread); + + std::lock_guard<std::recursive_mutex> guard(m_constituents_mutex); + for (const BreakpointLocationSP &bp_loc : + m_constituents.BreakpointLocations()) { + const Breakpoint &bp = bp_loc->GetBreakpoint(); + if (bp.IsInternal()) + continue; + if (bp_loc->ValidForThisThread(thread)) + return true; + } + return false; +} + void BreakpointSite::BumpHitCounts() { std::lock_guard<std::recursive_mutex> guard(m_constituents_mutex); for (BreakpointLocationSP loc_sp : m_constituents.BreakpointLocations()) { diff --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp index d49a01bdbcef7..0307b38d7d94b 100644 --- a/lldb/source/Target/ThreadPlanStepOut.cpp +++ b/lldb/source/Target/ThreadPlanStepOut.cpp @@ -356,13 +356,10 @@ bool ThreadPlanStepOut::DoPlanExplainsStop(Event *event_ptr) { } } - // If there was only one owner, then we're done. But if we also hit - // some user breakpoint on our way out, we should mark ourselves as - // done, but also not claim to explain the stop, since it is more - // important to report the user breakpoint than the step out - // completion. - - if (site_sp->GetNumberOfConstituents() == 1) + // If the thread also hit a user breakpoint on its way out, the plan is + // done but should not claim to explain the stop. It is more important + // to report the user breakpoint than the step out completion. + if (!site_sp->ContainsUserBreakpointForThread(GetThread())) return true; } return false; _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
