https://github.com/felipepiovezan updated https://github.com/llvm/llvm-project/pull/169799
>From 17b475b7cc48ff1fd9eceffdaa13e09043cbeec5 Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan <[email protected]> Date: Thu, 27 Nov 2025 12:41:18 +0000 Subject: [PATCH 1/3] [lldb] Fix ThreadPlanStepOut::DoPlanExplainsStop inspection of BreakpointSite 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. --- lldb/source/Target/ThreadPlanStepOut.cpp | 27 ++++++++++++++++++------ 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp index d49a01bdbcef7..7970e731aa972 100644 --- a/lldb/source/Target/ThreadPlanStepOut.cpp +++ b/lldb/source/Target/ThreadPlanStepOut.cpp @@ -8,6 +8,7 @@ #include "lldb/Target/ThreadPlanStepOut.h" #include "lldb/Breakpoint/Breakpoint.h" +#include "lldb/Breakpoint/BreakpointLocation.h" #include "lldb/Core/Value.h" #include "lldb/Symbol/Block.h" #include "lldb/Symbol/Function.h" @@ -306,6 +307,21 @@ bool ThreadPlanStepOut::ValidatePlan(Stream *error) { return true; } +/// Returns true if : +/// 1. All breakpoints in this site are internal, and +/// 2. All breakpoint locations in this site are NOT valid for `thread`. +static bool NoUserBreakpointsHere(BreakpointSite &site, Thread &thread) { + for (unsigned bp_idx = 0; bp_idx < site.GetNumberOfConstituents(); bp_idx++) { + BreakpointLocation &bp_loc = *site.GetConstituentAtIndex(bp_idx); + const Breakpoint &bp = bp_loc.GetBreakpoint(); + if (bp.IsInternal()) + continue; + if (bp_loc.ValidForThisThread(thread)) + return false; + } + return true; +} + bool ThreadPlanStepOut::DoPlanExplainsStop(Event *event_ptr) { // If the step out plan is done, then we just need to step through the // inlined frame. @@ -356,13 +372,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 (NoUserBreakpointsHere(*site_sp, GetThread())) return true; } return false; >From 940bc91278c68a436223fe19340c3dbe473e059c Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan <[email protected]> Date: Tue, 2 Dec 2025 12:46:22 +0000 Subject: [PATCH 2/3] fixup! address review comments --- lldb/include/lldb/Breakpoint/BreakpointSite.h | 4 ++++ lldb/source/Breakpoint/BreakpointSite.cpp | 16 ++++++++++++++++ lldb/source/Target/ThreadPlanStepOut.cpp | 17 +---------------- 3 files changed, 21 insertions(+), 16 deletions(-) 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 7970e731aa972..08a62ed9f1346 100644 --- a/lldb/source/Target/ThreadPlanStepOut.cpp +++ b/lldb/source/Target/ThreadPlanStepOut.cpp @@ -307,21 +307,6 @@ bool ThreadPlanStepOut::ValidatePlan(Stream *error) { return true; } -/// Returns true if : -/// 1. All breakpoints in this site are internal, and -/// 2. All breakpoint locations in this site are NOT valid for `thread`. -static bool NoUserBreakpointsHere(BreakpointSite &site, Thread &thread) { - for (unsigned bp_idx = 0; bp_idx < site.GetNumberOfConstituents(); bp_idx++) { - BreakpointLocation &bp_loc = *site.GetConstituentAtIndex(bp_idx); - const Breakpoint &bp = bp_loc.GetBreakpoint(); - if (bp.IsInternal()) - continue; - if (bp_loc.ValidForThisThread(thread)) - return false; - } - return true; -} - bool ThreadPlanStepOut::DoPlanExplainsStop(Event *event_ptr) { // If the step out plan is done, then we just need to step through the // inlined frame. @@ -375,7 +360,7 @@ bool ThreadPlanStepOut::DoPlanExplainsStop(Event *event_ptr) { // 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 (NoUserBreakpointsHere(*site_sp, GetThread())) + if (!site_sp->ContainsUserBreakpointForThread(GetThread())) return true; } return false; >From 69fa2d5a7dbce612b737cedb6468f0149bd2d993 Mon Sep 17 00:00:00 2001 From: Felipe de Azevedo Piovezan <[email protected]> Date: Wed, 3 Dec 2025 10:46:02 +0000 Subject: [PATCH 3/3] fixup! address review comments --- lldb/source/Target/ThreadPlanStepOut.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp index 08a62ed9f1346..0307b38d7d94b 100644 --- a/lldb/source/Target/ThreadPlanStepOut.cpp +++ b/lldb/source/Target/ThreadPlanStepOut.cpp @@ -8,7 +8,6 @@ #include "lldb/Target/ThreadPlanStepOut.h" #include "lldb/Breakpoint/Breakpoint.h" -#include "lldb/Breakpoint/BreakpointLocation.h" #include "lldb/Core/Value.h" #include "lldb/Symbol/Block.h" #include "lldb/Symbol/Function.h" _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
