[Lldb-commits] [lldb] [lldb] Keep the unexpected b/p state for suspended threads (PR #174264)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Igor Kudrin (igorkudrin)

<details>
<summary>Changes</summary>

This fixes stepping out for a case when two threads reach the stepping-out target breakpoint simultaneously, but a concurrent thread executes the breakpoint first. The issue affects platforms with software breakpoints. The scenario is as follows:

 * The `step-out` command is executed for thread `A`.
 * `ThreadPlanStepOut` creates a breakpoint at the target location.
 * All threads are resumed, because the `step-out` command does not suspend other threads.
 * Threads `A` and `B` reach the stepping-out address at the same time, but `B` executes the breakpoint instruction first.
 * `SetThreadStoppedAtUnexecutedBP()` is called for thread `A`, and `SetThreadHitBreakpointSite()` is called for thread `B`.
 * Thread `B` has no plans to stop at this location, so `ThreadPlanStepOverBreakpoint` is scheduled.
 * The plan disables the breakpoint and resumes thread `B` with `eStateStepping`; for thread `A`, `ShouldResume(eStateSuspended)` is called, which clears `m_stopped_at_unexecuted_bp`.
 * After the stepping, `ThreadPlanStepOverBreakpoint` finishes, the breakpoint is re-enabled, and all threads are resumed.
 * At this moment, thread `A` is still at the location of the breakpoint, but `m_stopped_at_unexecuted_bp` is cleared, so `Thread::SetupToStepOverBreakpointIfNeeded()` schedules `ThreadPlanStepOverBreakpoint` for it.
 * `ThreadPlanStepOverBreakpoint` steps over the target breakpoint, so `ThreadPlanStepOut` can't catch the execution there.

---
Full diff: https://github.com/llvm/llvm-project/pull/174264.diff


2 Files Affected:

- (modified) lldb/source/Target/Thread.cpp (+1-1) 
- (modified) lldb/unittests/Thread/ThreadTest.cpp (+54) 


``````````diff
diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp
index b833918c27818..8856d3505c5f5 100644
--- a/lldb/source/Target/Thread.cpp
+++ b/lldb/source/Target/Thread.cpp
@@ -739,12 +739,12 @@ bool Thread::ShouldResume(StateType resume_state) {
 
     if (need_to_resume && resume_state != eStateSuspended) {
       m_stop_info_sp.reset();
+      m_stopped_at_unexecuted_bp = LLDB_INVALID_ADDRESS;
     }
   }
 
   if (need_to_resume) {
     ClearStackFrames();
-    m_stopped_at_unexecuted_bp = LLDB_INVALID_ADDRESS;
     // Let Thread subclasses do any special work they need to prior to resuming
     WillResume(resume_state);
   }
diff --git a/lldb/unittests/Thread/ThreadTest.cpp b/lldb/unittests/Thread/ThreadTest.cpp
index ef3a73cd3de87..4f4a5558db8df 100644
--- a/lldb/unittests/Thread/ThreadTest.cpp
+++ b/lldb/unittests/Thread/ThreadTest.cpp
@@ -24,6 +24,7 @@
 #include "lldb/Host/HostThread.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StopInfo.h"
+#include "lldb/Target/ThreadPlanBase.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "gtest/gtest.h"
 
@@ -105,6 +106,18 @@ class DummyThread : public Thread {
   bool CalculateStopInfo() override { return false; }
 
   bool IsStillAtLastBreakpointHit() override { return true; }
+
+  using Thread::PushPlan;
+
+  lldb::addr_t GetStoppedAtUnexpectedBP() const {
+    return m_stopped_at_unexecuted_bp;
+  }
+};
+
+class DummyThreadPlan : public ThreadPlanBase {
+public:
+  DummyThreadPlan(Thread &thread) : ThreadPlanBase(thread) {}
+  ~DummyThreadPlan() override = default;
 };
 } // namespace
 
@@ -228,3 +241,44 @@ TEST_F(ThreadTest, GetPrivateStopInfo) {
   StopInfoSP new_stopinfo_sp = thread_sp->GetPrivateStopInfo();
   ASSERT_TRUE(new_stopinfo_sp && stopinfo_sp->IsValid() == true);
 }
+
+TEST_F(ThreadTest, ShouldResume) {
+  ArchSpec arch("x86_64-pc-linux");
+
+  Platform::SetHostPlatform(
+      platform_linux::PlatformLinux::CreateInstance(true, &arch));
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  TargetSP target_sp = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp = std::make_shared<DummyProcess>(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+
+  auto thread_sp = std::make_shared<DummyThread>(*process_sp.get(), 0);
+  ASSERT_TRUE(thread_sp);
+
+  thread_sp->PushPlan(std::make_shared<DummyThreadPlan>(*thread_sp));
+  ASSERT_TRUE(thread_sp->GetCurrentPlan());
+
+  const lldb::addr_t unexpected_bp_addr = 0x1234;
+
+  // If a thread is resumed with 'eStateRunning' or 'eStateStepping', it should
+  // clear the address of an unexpected breakpoint.
+  thread_sp->SetThreadStoppedAtUnexecutedBP(unexpected_bp_addr);
+  EXPECT_TRUE(thread_sp->ShouldResume(eStateRunning));
+  EXPECT_EQ(LLDB_INVALID_ADDRESS, thread_sp->GetStoppedAtUnexpectedBP());
+
+  thread_sp->SetThreadStoppedAtUnexecutedBP(unexpected_bp_addr);
+  EXPECT_TRUE(thread_sp->ShouldResume(eStateStepping));
+  EXPECT_EQ(LLDB_INVALID_ADDRESS, thread_sp->GetStoppedAtUnexpectedBP());
+
+  // If a thread is resumed with 'eStateSuspended', the address of an unexpected
+  // breakpoint should be preserved.
+  thread_sp->SetThreadStoppedAtUnexecutedBP(unexpected_bp_addr);
+  EXPECT_TRUE(thread_sp->ShouldResume(eStateSuspended));
+  EXPECT_EQ(unexpected_bp_addr, thread_sp->GetStoppedAtUnexpectedBP());
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/174264