llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: None (barsolo2000) <details> <summary>Changes</summary> Draft for now, following up from https://discourse.llvm.org/t/improving-performance-of-multiple-threads-stepping-over-the-same-breakpoint/89637 --- Patch is 31.16 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/180101.diff 6 Files Affected: - (modified) lldb/include/lldb/Target/ThreadList.h (+21) - (modified) lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h (+19) - (modified) lldb/source/Target/ThreadList.cpp (+169-1) - (modified) lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp (+14-4) - (added) lldb/test/API/functionalities/gdb_remote_client/TestBatchedBreakpointStepOver.py (+216) - (added) lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentBatchedBreakpointStepOver.py (+151) ``````````diff diff --git a/lldb/include/lldb/Target/ThreadList.h b/lldb/include/lldb/Target/ThreadList.h index c108962003598..97541ed83b200 100644 --- a/lldb/include/lldb/Target/ThreadList.h +++ b/lldb/include/lldb/Target/ThreadList.h @@ -9,7 +9,9 @@ #ifndef LLDB_TARGET_THREADLIST_H #define LLDB_TARGET_THREADLIST_H +#include <map> #include <mutex> +#include <set> #include <vector> #include "lldb/Target/Thread.h" @@ -141,6 +143,19 @@ class ThreadList : public ThreadCollection { /// Precondition: both thread lists must be belong to the same process. void Update(ThreadList &rhs); + /// Called by ThreadPlanStepOverBreakpoint when a thread finishes stepping + /// over a breakpoint. This tracks which threads are still stepping over + /// each breakpoint address, and only re-enables the breakpoint when ALL + /// threads have finished stepping over it. + void ThreadFinishedSteppingOverBreakpoint(lldb::addr_t breakpoint_addr, + lldb::tid_t tid); + + /// Register a thread that is about to step over a breakpoint. + /// The breakpoint will be re-enabled only after all registered threads + /// have called ThreadFinishedSteppingOverBreakpoint. + void RegisterThreadSteppingOverBreakpoint(lldb::addr_t breakpoint_addr, + lldb::tid_t tid); + protected: void SetShouldReportStop(Vote vote); @@ -154,6 +169,12 @@ class ThreadList : public ThreadCollection { m_selected_tid; ///< For targets that need the notion of a current thread. std::vector<lldb::tid_t> m_expression_tid_stack; + /// Tracks which threads are currently stepping over each breakpoint address. + /// Key: breakpoint address, Value: set of thread IDs stepping over it. + /// When a thread finishes stepping, it's removed from the set. When the set + /// becomes empty, the breakpoint is re-enabled. + std::map<lldb::addr_t, std::set<lldb::tid_t>> m_threads_stepping_over_bp; + private: ThreadList() = delete; }; diff --git a/lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h b/lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h index 0da8dbf44ffd8..0f0bbc0be4142 100644 --- a/lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h +++ b/lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h @@ -36,6 +36,24 @@ class ThreadPlanStepOverBreakpoint : public ThreadPlan { lldb::addr_t GetBreakpointLoadAddress() const { return m_breakpoint_addr; } + /// When set to true, the breakpoint site will NOT be re-enabled directly + /// by this plan. Instead, the plan will call + /// ThreadList::ThreadFinishedSteppingOverBreakpoint() when it completes, + /// allowing ThreadList to track all threads stepping over the same + /// breakpoint and only re-enable it when ALL threads have finished. + void SetDeferReenableBreakpointSite(bool defer) { + m_defer_reenable_breakpoint_site = defer; + } + + bool GetDeferReenableBreakpointSite() const { + return m_defer_reenable_breakpoint_site; + } + + /// Mark the breakpoint site as already re-enabled, suppressing any + /// re-enable in DidPop()/ThreadDestroyed(). Used when discarding plans + /// during WillResume cleanup to avoid spurious breakpoint toggles. + void SetReenabledBreakpointSite() { m_reenabled_breakpoint_site = true; } + protected: bool DoPlanExplainsStop(Event *event_ptr) override; bool DoWillResume(lldb::StateType resume_state, bool current_plan) override; @@ -47,6 +65,7 @@ class ThreadPlanStepOverBreakpoint : public ThreadPlan { lldb::user_id_t m_breakpoint_site_id; bool m_auto_continue; bool m_reenabled_breakpoint_site; + bool m_defer_reenable_breakpoint_site = false; ThreadPlanStepOverBreakpoint(const ThreadPlanStepOverBreakpoint &) = delete; const ThreadPlanStepOverBreakpoint & diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp index 77a1c40b95f70..fcfcf88e7fe15 100644 --- a/lldb/source/Target/ThreadList.cpp +++ b/lldb/source/Target/ThreadList.cpp @@ -15,6 +15,7 @@ #include "lldb/Target/Thread.h" #include "lldb/Target/ThreadList.h" #include "lldb/Target/ThreadPlan.h" +#include "lldb/Target/ThreadPlanStepOverBreakpoint.h" #include "lldb/Utility/LLDBAssert.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" @@ -504,9 +505,31 @@ bool ThreadList::WillResume(RunDirection &direction) { collection::iterator pos, end = m_threads.end(); + // Clear tracking state from the previous stop and pop any leftover + // StepOverBreakpoint plans. This gives us a clean slate: plans will be + // recreated fresh by SetupToStepOverBreakpointIfNeeded below, and the + // batching logic will recompute deferred state from scratch. + m_threads_stepping_over_bp.clear(); + for (pos = m_threads.begin(); pos != end; ++pos) { + ThreadSP thread_sp(*pos); + ThreadPlan *plan = thread_sp->GetCurrentPlan(); + if (plan && plan->GetKind() == ThreadPlan::eKindStepOverBreakpoint) { + // Suppress the re-enable side effect in DidPop() — the breakpoint + // may still be disabled from the previous batch, and we don't want + // to toggle it. The new plans will handle disable/re-enable correctly. + static_cast<ThreadPlanStepOverBreakpoint *>(plan) + ->SetReenabledBreakpointSite(); + thread_sp->DiscardPlan(); + } + } + // Go through the threads and see if any thread wants to run just itself. // if so then pick one and run it. + // Collect threads for batched vCont - multiple threads at the same breakpoint + // can be stepped together in a single vCont packet. + std::vector<ThreadSP> batched_step_threads; + ThreadList run_me_only_list(m_process); run_me_only_list.SetStopID(m_process.GetStopID()); @@ -576,6 +599,13 @@ bool ThreadList::WillResume(RunDirection &direction) { assert(thread_to_run->GetCurrentPlan()->GetDirection() == direction); } } else { + // Pre-scan to find all threads that need to step over a breakpoint, + // and group them by breakpoint address. This optimization allows us to + // step multiple threads over the same breakpoint with minimal breakpoint + // swaps - only the last thread in each group will re-enable the breakpoint. + std::map<lldb::addr_t, std::vector<ThreadSP>> breakpoint_groups; + bool found_run_before_public_stop = false; + for (pos = m_threads.begin(); pos != end; ++pos) { ThreadSP thread_sp(*pos); if (thread_sp->GetResumeState() != eStateSuspended) { @@ -589,14 +619,71 @@ bool ThreadList::WillResume(RunDirection &direction) { assert(thread_sp->GetCurrentPlan()->GetDirection() == direction); // You can't say "stop others" and also want yourself to be suspended. assert(thread_sp->GetCurrentPlan()->RunState() != eStateSuspended); + + // Get the breakpoint address from the step-over-breakpoint plan + ThreadPlan *current_plan = thread_sp->GetCurrentPlan(); + if (current_plan && + current_plan->GetKind() == ThreadPlan::eKindStepOverBreakpoint) { + ThreadPlanStepOverBreakpoint *bp_plan = + static_cast<ThreadPlanStepOverBreakpoint *>(current_plan); + lldb::addr_t bp_addr = bp_plan->GetBreakpointLoadAddress(); + breakpoint_groups[bp_addr].push_back(thread_sp); + } + thread_to_run = thread_sp; if (thread_sp->ShouldRunBeforePublicStop()) { // This takes precedence, so if we find one of these, service it: + found_run_before_public_stop = true; break; } } } } + + // Only apply batching optimization if we have a complete picture of + // breakpoint groups. If a ShouldRunBeforePublicStop thread caused the + // scan to exit early, the groups are incomplete and the priority thread + // must run solo. Deferred state will be cleaned up on next WillResume(). + if (!found_run_before_public_stop) { + // For each group of threads at the same breakpoint, register them with + // ThreadList and set them to use deferred re-enable. The breakpoint will + // only be re-enabled when ALL threads have finished stepping over it. + // Also collect threads for batched vCont if multiple threads at same BP. + for (auto &group : breakpoint_groups) { + lldb::addr_t bp_addr = group.first; + std::vector<ThreadSP> &threads = group.second; + + if (threads.size() > 1) { + // Multiple threads stepping over the same breakpoint - use tracking + for (ThreadSP &thread_sp : threads) { + // Register this thread as stepping over the breakpoint + RegisterThreadSteppingOverBreakpoint(bp_addr, thread_sp->GetID()); + + // Set the plan to defer re-enabling (use callback instead). + ThreadPlan *plan = thread_sp->GetCurrentPlan(); + // Verify the plan is actually a StepOverBreakpoint plan. + if (plan && + plan->GetKind() == ThreadPlan::eKindStepOverBreakpoint) { + ThreadPlanStepOverBreakpoint *bp_plan = + static_cast<ThreadPlanStepOverBreakpoint *>(plan); + bp_plan->SetDeferReenableBreakpointSite(true); + } + } + + // Collect for batched vCont - pick the largest group + if (threads.size() > batched_step_threads.size()) { + batched_step_threads = threads; + } + } + // Single thread at breakpoint - keeps default behavior (re-enable + // directly) + } + + // If we found a batch, use the first thread as thread_to_run + if (!batched_step_threads.empty()) { + thread_to_run = batched_step_threads[0]; + } + } } if (thread_to_run != nullptr) { @@ -615,7 +702,26 @@ bool ThreadList::WillResume(RunDirection &direction) { bool need_to_resume = true; - if (thread_to_run == nullptr) { + if (!batched_step_threads.empty()) { + // Batched stepping: all threads in the batch step together, + // all other threads stay suspended. + std::set<lldb::tid_t> batch_tids; + for (ThreadSP &thread_sp : batched_step_threads) { + batch_tids.insert(thread_sp->GetID()); + } + + for (pos = m_threads.begin(); pos != end; ++pos) { + ThreadSP thread_sp(*pos); + if (batch_tids.count(thread_sp->GetID()) > 0) { + // This thread is in the batch - let it step + if (!thread_sp->ShouldResume(thread_sp->GetCurrentPlan()->RunState())) + need_to_resume = false; + } else { + // Not in the batch - suspend it + thread_sp->ShouldResume(eStateSuspended); + } + } + } else if (thread_to_run == nullptr) { // Everybody runs as they wish: for (pos = m_threads.begin(); pos != end; ++pos) { ThreadSP thread_sp(*pos); @@ -801,3 +907,65 @@ ThreadList::ExpressionExecutionThreadPusher::ExpressionExecutionThreadPusher( m_thread_list->PushExpressionExecutionThread(m_tid); } } + +void ThreadList::RegisterThreadSteppingOverBreakpoint(addr_t breakpoint_addr, + tid_t tid) { + std::lock_guard<std::recursive_mutex> guard(GetMutex()); + m_threads_stepping_over_bp[breakpoint_addr].insert(tid); + + Log *log = GetLog(LLDBLog::Step); + LLDB_LOGF(log, + "ThreadList::%s: Registered thread 0x%" PRIx64 + " stepping over breakpoint at 0x%" PRIx64 " (now %zu threads)", + __FUNCTION__, tid, breakpoint_addr, + m_threads_stepping_over_bp[breakpoint_addr].size()); +} + +void ThreadList::ThreadFinishedSteppingOverBreakpoint(addr_t breakpoint_addr, + tid_t tid) { + std::lock_guard<std::recursive_mutex> guard(GetMutex()); + + Log *log = GetLog(LLDBLog::Step); + + auto it = m_threads_stepping_over_bp.find(breakpoint_addr); + if (it == m_threads_stepping_over_bp.end()) { + // No threads registered for this breakpoint - just re-enable it directly + LLDB_LOGF(log, + "ThreadList::%s: Thread 0x%" PRIx64 + " finished stepping over breakpoint at 0x%" PRIx64 + " but no threads were registered, re-enabling directly", + __FUNCTION__, tid, breakpoint_addr); + BreakpointSiteSP bp_site_sp( + m_process.GetBreakpointSiteList().FindByAddress(breakpoint_addr)); + if (bp_site_sp) { + m_process.EnableBreakpointSite(bp_site_sp.get()); + } + return; + } + + // Remove this thread from the set + it->second.erase(tid); + + LLDB_LOGF(log, + "ThreadList::%s: Thread 0x%" PRIx64 + " finished stepping over breakpoint at 0x%" PRIx64 + " (%zu threads remaining)", + __FUNCTION__, tid, breakpoint_addr, it->second.size()); + + // If no more threads are stepping over this breakpoint, re-enable it + if (it->second.empty()) { + LLDB_LOGF(log, + "ThreadList::%s: All threads finished stepping over breakpoint " + "at 0x%" PRIx64 ", re-enabling breakpoint", + __FUNCTION__, breakpoint_addr); + + BreakpointSiteSP bp_site_sp( + m_process.GetBreakpointSiteList().FindByAddress(breakpoint_addr)); + if (bp_site_sp) { + m_process.EnableBreakpointSite(bp_site_sp.get()); + } + + // Clean up the entry + m_threads_stepping_over_bp.erase(it); + } +} diff --git a/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp b/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp index 3602527a9231b..70538e21153b6 100644 --- a/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp +++ b/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp @@ -10,6 +10,7 @@ #include "lldb/Target/Process.h" #include "lldb/Target/RegisterContext.h" +#include "lldb/Target/ThreadList.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" #include "lldb/Utility/Stream.h" @@ -155,10 +156,19 @@ bool ThreadPlanStepOverBreakpoint::MischiefManaged() { void ThreadPlanStepOverBreakpoint::ReenableBreakpointSite() { if (!m_reenabled_breakpoint_site) { m_reenabled_breakpoint_site = true; - BreakpointSiteSP bp_site_sp( - m_process.GetBreakpointSiteList().FindByAddress(m_breakpoint_addr)); - if (bp_site_sp) { - m_process.EnableBreakpointSite(bp_site_sp.get()); + + if (m_defer_reenable_breakpoint_site) { + // Let ThreadList track all threads stepping over this breakpoint. + // It will re-enable the breakpoint only when ALL threads have finished. + m_process.GetThreadList().ThreadFinishedSteppingOverBreakpoint( + m_breakpoint_addr, GetThread().GetID()); + } else { + // Default behavior: re-enable the breakpoint directly + BreakpointSiteSP bp_site_sp( + m_process.GetBreakpointSiteList().FindByAddress(m_breakpoint_addr)); + if (bp_site_sp) { + m_process.EnableBreakpointSite(bp_site_sp.get()); + } } } } diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestBatchedBreakpointStepOver.py b/lldb/test/API/functionalities/gdb_remote_client/TestBatchedBreakpointStepOver.py new file mode 100644 index 0000000000000..4954037565c76 --- /dev/null +++ b/lldb/test/API/functionalities/gdb_remote_client/TestBatchedBreakpointStepOver.py @@ -0,0 +1,216 @@ +""" +Test that when multiple threads are stopped at the same breakpoint, LLDB sends +a batched vCont with multiple step actions and only one breakpoint disable/ +re-enable pair, rather than stepping each thread individually with repeated +breakpoint toggles. + +Uses a mock GDB server to directly verify the packets LLDB sends. +""" + +import re + +import lldb +from lldbsuite.test.lldbtest import * +from lldbsuite.test.decorators import * +from lldbsuite.test.gdbclientutils import * +from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase + + +class TestBatchedBreakpointStepOver(GDBRemoteTestBase): + @skipIfXmlSupportMissing + def test(self): + BP_ADDR = 0x0000000000401020 + # PC after stepping past the breakpoint instruction. + STEPPED_PC = BP_ADDR + 1 + NUM_THREADS = 10 + TIDS = [0x101 + i for i in range(NUM_THREADS)] + + class MyResponder(MockGDBServerResponder): + def __init__(self): + MockGDBServerResponder.__init__(self) + self.resume_count = 0 + # Track which threads have completed their step + self.stepped_threads = set() + + def qSupported(self, client_supported): + return ( + "PacketSize=3fff;QStartNoAckMode+;" + "qXfer:features:read+;swbreak+;hwbreak+" + ) + + def qfThreadInfo(self): + return "m" + ",".join("{:x}".format(t) for t in TIDS) + + def qsThreadInfo(self): + return "l" + + def haltReason(self): + # All threads stopped at the breakpoint address. + threads_str = ",".join("{:x}".format(t) for t in TIDS) + pcs_str = ",".join("{:x}".format(BP_ADDR) for _ in TIDS) + return "T05thread:{:x};threads:{};thread-pcs:{};" "swbreak:;".format( + TIDS[0], threads_str, pcs_str + ) + + def threadStopInfo(self, threadnum): + threads_str = ",".join("{:x}".format(t) for t in TIDS) + pcs_str = ",".join("{:x}".format(BP_ADDR) for _ in TIDS) + return "T05thread:{:x};threads:{};thread-pcs:{};" "swbreak:;".format( + threadnum, threads_str, pcs_str + ) + + def setBreakpoint(self, packet): + return "OK" + + def readRegisters(self): + return "00" * 160 + + def readRegister(self, regno): + return "00" * 8 + + def qXferRead(self, obj, annex, offset, length): + if annex == "target.xml": + return ( + """<?xml version="1.0"?> + <target version="1.0"> + <architecture>i386:x86-64</architecture> + <feature name="org.gnu.gdb.i386.core"> + <reg name="rax" bitsize="64" regnum="0" type="int" group="general"/> + <reg name="rbx" bitsize="64" regnum="1" type="int" group="general"/> + <reg name="rcx" bitsize="64" regnum="2" type="int" group="general"/> + <reg name="rdx" bitsize="64" regnum="3" type="int" group="general"/> + <reg name="rsi" bitsize="64" regnum="4" type="int" group="general"/> + <reg name="rdi" bitsize="64" regnum="5" type="int" group="general"/> + <reg name="rbp" bitsize="64" regnum="6" type="data_ptr" group="general"/> + <reg name="rsp" bitsize="64" regnum="7" type="data_ptr" group="general"/> + <reg name="r8" bitsize="64" regnum="8" type="int" group="general"/> + <reg name="r9" bitsize="64" regnum="9" type="int" group="general"/> + <reg name="r10" bitsize="64" regnum="10" type="int" group="general"/> + <reg name="r11" bitsize="64" regnum="11" type="int" group="general"/> + <reg name="r12" bitsize="64" regnum="12" type="int" group="general"/> + <reg name="r13" bitsize="64" regnum="13" type="int" group="general"/> + <reg name="r14" bitsize="64" regnum="14" type="int" group="general"/> + <reg name="r15" bitsize="64" regnum="15" type="int" group="general"/> + <reg name="rip" bitsize="64" regnum="16" type="code_ptr" group="general"/> + <reg name="eflags" bitsize="32" regnum="17" type="int" group="general"/> + <reg name="cs" bitsize="32" regnum="18" type="int" group="general"/> + <reg name="ss" bitsize="32" regnum="19" type="int" group="general"/> + </feature> + </target>""", + False, + ) + return None, False + + def other(self, packet): + if packet == "vCont?"... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/180101 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
