Author: jeffreytan81 Date: 2024-08-28T13:34:35-07:00 New Revision: 38b252aa45abad53d7c07c666569b174a215d94d
URL: https://github.com/llvm/llvm-project/commit/38b252aa45abad53d7c07c666569b174a215d94d DIFF: https://github.com/llvm/llvm-project/commit/38b252aa45abad53d7c07c666569b174a215d94d.diff LOG: Disable ThreadPlanSingleThreadTimeout during step over breakpoint (#104532) This PR fixes another race condition in https://github.com/llvm/llvm-project/pull/90930. The failure was found by @labath with this log: https://paste.debian.net/hidden/30235a5c/: ``` dotest_wrapper. < 15> send packet: $z0,224505,1#65 ... b-remote.async> < 22> send packet: $vCont;s:p1dcf.1dcf#4c intern-state GDBRemoteClientBase::Lock::Lock sent packet: \x03 b-remote.async> < 818> read packet: $T13thread:p1dcf.1dcf;name:a.out;threads:1dcf,1dd2;jstopinfo:5b7b226e616d65223a22612e6f7574222c22726561736f6e223a227369676e616c222c227369676e616c223a31392c22746964223a373633317d2c7b226e616d65223a22612e6f7574222c22746964223a373633347d5d;thread-pcs:0000000000224505,00007f4e4302119a;00:0000000000000000;01:0000000000000000;02:0100000000000000;03:0000000000000000;04:9084997dfc7f0000;05:a8742a0000000000;06:b084997dfc7f0000;07:6084997dfc7f0000;08:0000000000000000;09:00d7e5424e7f0000;0a:d0d9e5424e7f0000;0b:0202000000000000;0c:80cc290000000000;0d:d8cc1c434e7f0000;0e:2886997dfc7f0000;0f:0100000000000000;10:0545220000000000;11:0602000000000000;12:3300000000000000;13:0000000000000000;14:0000000000000000;15:2b00000000000000;16:80fbe5424e7f0000;17:0000000000000000;18:0000000000000000;19:0000000000000000;reason:signal;#b9 ``` It shows an async interrupt "\x03" was sent immediately after `vCont;s` single step over breakpoint at address `0x224505` (which was disabled before vCont). And the later stop was still at the original PC (0x224505) not moving forward. The investigation shows the failure happens when timeout is short and async interrupt is sent to lldb-server immediately after vCont so ptrace() resumes and then async interrupts debuggee immediately so debuggee does not get a chance to execute and move PC. So it enters stop mode immediately at original PC. `ThreadPlanStepOverBreakpoint` does not expect PC not moving and reports stop at the original place. To fix this, the PR prevents `ThreadPlanSingleThreadTimeout` from being created during `ThreadPlanStepOverBreakpoint` by introduces a new `SupportsResumeOthers()` method and `ThreadPlanStepOverBreakpoint` returns false for it. This makes sense because we should never resume threads during step over breakpoint anyway otherwise it might cause other threads to miss breakpoint. --------- Co-authored-by: jeffreytan81 <jeffrey...@fb.com> Added: Modified: lldb/include/lldb/Target/ThreadPlan.h lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h lldb/source/Target/ThreadPlanSingleThreadTimeout.cpp lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp Removed: ################################################################################ diff --git a/lldb/include/lldb/Target/ThreadPlan.h b/lldb/include/lldb/Target/ThreadPlan.h index c336b6bb37df1b..d6da484f4fc137 100644 --- a/lldb/include/lldb/Target/ThreadPlan.h +++ b/lldb/include/lldb/Target/ThreadPlan.h @@ -385,7 +385,13 @@ class ThreadPlan : public std::enable_shared_from_this<ThreadPlan>, virtual void SetStopOthers(bool new_value); virtual bool StopOthers(); - + + // Returns true if the thread plan supports ThreadPlanSingleThreadTimeout to + // resume other threads after timeout. If the thread plan returns false it + // will prevent ThreadPlanSingleThreadTimeout from being created when this + // thread plan is alive. + virtual bool SupportsResumeOthers() { return true; } + virtual bool ShouldRunBeforePublicStop() { return false; } // This is the wrapper for DoWillResume that does generic ThreadPlan logic, diff --git a/lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h b/lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h index 1f3aff45c49abe..0da8dbf44ffd8a 100644 --- a/lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h +++ b/lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h @@ -23,6 +23,7 @@ class ThreadPlanStepOverBreakpoint : public ThreadPlan { void GetDescription(Stream *s, lldb::DescriptionLevel level) override; bool ValidatePlan(Stream *error) override; bool ShouldStop(Event *event_ptr) override; + bool SupportsResumeOthers() override; bool StopOthers() override; lldb::StateType GetPlanRunState() override; bool WillStop() override; diff --git a/lldb/source/Target/ThreadPlanSingleThreadTimeout.cpp b/lldb/source/Target/ThreadPlanSingleThreadTimeout.cpp index 806ba95c508b7c..71be81365a2668 100644 --- a/lldb/source/Target/ThreadPlanSingleThreadTimeout.cpp +++ b/lldb/source/Target/ThreadPlanSingleThreadTimeout.cpp @@ -76,6 +76,9 @@ void ThreadPlanSingleThreadTimeout::PushNewWithTimeout(Thread &thread, if (!thread.GetCurrentPlan()->StopOthers()) return; + if (!thread.GetCurrentPlan()->SupportsResumeOthers()) + return; + auto timeout_plan = new ThreadPlanSingleThreadTimeout(thread, info); ThreadPlanSP thread_plan_sp(timeout_plan); auto status = thread.QueueThreadPlan(thread_plan_sp, @@ -102,6 +105,9 @@ void ThreadPlanSingleThreadTimeout::ResumeFromPrevState(Thread &thread, if (!thread.GetCurrentPlan()->StopOthers()) return; + if (!thread.GetCurrentPlan()->SupportsResumeOthers()) + return; + auto timeout_plan = new ThreadPlanSingleThreadTimeout(thread, info); ThreadPlanSP thread_plan_sp(timeout_plan); auto status = thread.QueueThreadPlan(thread_plan_sp, diff --git a/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp b/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp index f88a2b895931cd..3602527a9231b2 100644 --- a/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp +++ b/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp @@ -103,6 +103,13 @@ bool ThreadPlanStepOverBreakpoint::ShouldStop(Event *event_ptr) { bool ThreadPlanStepOverBreakpoint::StopOthers() { return true; } +// This thread plan does a single instruction step over a breakpoint instruction +// and needs to not resume other threads, so return false to stop the +// ThreadPlanSingleThreadTimeout from timing out and trying to resume all +// threads. If all threads gets resumed before we disable, single step and +// re-enable the breakpoint, we can miss breakpoints on other threads. +bool ThreadPlanStepOverBreakpoint::SupportsResumeOthers() { return false; } + StateType ThreadPlanStepOverBreakpoint::GetPlanRunState() { return eStateStepping; } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
