[Lldb-commits] [lldb] 38b252a - Disable ThreadPlanSingleThreadTimeout during step over breakpoint (#104532)

[via lldb-commits]

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 <jeffreytan at 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;
 }