Fix ASAN failure in TestSingleThreadStepTimeout.py

[jeffreytan81]

This PR fixes the ASAN failure in #90930.

The original PR made the assumption that parent ThreadPlanStepOverRange's lifetime will always be longer than ThreadPlanSingleThreadTimeout leaf plan so it passes the m_timeout_info as reference to it.
From the ASAN failure, it seems that this assumption may not be true (likely the thread stack is holding a strong reference to the leaf plan).

This PR fixes this lifetime issue by using shared pointer instead of passing by reference.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (jeffreytan81)

Changes

This PR fixes the ASAN failure in #90930.

The original PR made the assumption that parent ThreadPlanStepOverRange's lifetime will always be longer than ThreadPlanSingleThreadTimeout leaf plan so it passes the m_timeout_info as reference to it.
From the ASAN failure, it seems that this assumption may not be true (likely the thread stack is holding a strong reference to the leaf plan).

This PR fixes this lifetime issue by using shared pointer instead of passing by reference.

---

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

3 Files Affected:

• (modified) lldb/include/lldb/Target/ThreadPlanSingleThreadTimeout.h (+11-4)
• (modified) lldb/include/lldb/Target/TimeoutResumeAll.h (+5-2)
• (modified) lldb/source/Target/ThreadPlanSingleThreadTimeout.cpp (+18-15)

diff --git a/lldb/include/lldb/Target/ThreadPlanSingleThreadTimeout.h b/lldb/include/lldb/Target/ThreadPlanSingleThreadTimeout.h
index 1d88d6a51260b1..87e6ba6d766955 100644
--- a/lldb/include/lldb/Target/ThreadPlanSingleThreadTimeout.h
+++ b/lldb/include/lldb/Target/ThreadPlanSingleThreadTimeout.h
@@ -54,11 +54,15 @@ class ThreadPlanSingleThreadTimeout : public ThreadPlan {
   // state. The reference of \param info is passed in so that when
   // ThreadPlanSingleThreadTimeout got popped its last state can be stored
   // in it for future resume.
-  static void PushNewWithTimeout(Thread &thread, TimeoutInfo &info);
+  static void PushNewWithTimeout(
+      Thread &thread,
+      std::shared_ptr<ThreadPlanSingleThreadTimeout::TimeoutInfo> &info);
 
   // Push a new ThreadPlanSingleThreadTimeout by restoring state from
   // input \param info and resume execution.
-  static void ResumeFromPrevState(Thread &thread, TimeoutInfo &info);
+  static void ResumeFromPrevState(
+      Thread &thread,
+      std::shared_ptr<ThreadPlanSingleThreadTimeout::TimeoutInfo> &info);
 
   void GetDescription(Stream *s, lldb::DescriptionLevel level) override;
   bool ValidatePlan(Stream *error) override { return true; }
@@ -78,7 +82,9 @@ class ThreadPlanSingleThreadTimeout : public ThreadPlan {
   bool StopOthers() override;
 
 private:
-  ThreadPlanSingleThreadTimeout(Thread &thread, TimeoutInfo &info);
+  ThreadPlanSingleThreadTimeout(
+      Thread &thread,
+      std::shared_ptr<ThreadPlanSingleThreadTimeout::TimeoutInfo> &info);
 
   bool IsTimeoutAsyncInterrupt(Event *event_ptr);
   bool HandleEvent(Event *event_ptr);
@@ -91,7 +97,8 @@ class ThreadPlanSingleThreadTimeout : public ThreadPlan {
   const ThreadPlanSingleThreadTimeout &
   operator=(const ThreadPlanSingleThreadTimeout &) = delete;
 
-  TimeoutInfo &m_info; // Reference to controlling ThreadPlan's TimeoutInfo.
+  std::shared_ptr<ThreadPlanSingleThreadTimeout::TimeoutInfo>
+      m_info; // Reference to controlling ThreadPlan's TimeoutInfo.
   State m_state;
 
   // Lock for m_wakeup_cv and m_exit_flag between thread plan thread and timer
diff --git a/lldb/include/lldb/Target/TimeoutResumeAll.h b/lldb/include/lldb/Target/TimeoutResumeAll.h
index d484ea02bcce94..30e923f30602a3 100644
--- a/lldb/include/lldb/Target/TimeoutResumeAll.h
+++ b/lldb/include/lldb/Target/TimeoutResumeAll.h
@@ -19,7 +19,10 @@ namespace lldb_private {
 // ResumeWithTimeout() during DoWillResume().
 class TimeoutResumeAll {
 public:
-  TimeoutResumeAll(Thread &thread) : m_thread(thread) {}
+  TimeoutResumeAll(Thread &thread)
+      : m_thread(thread),
+        m_timeout_info(
+            std::make_shared<ThreadPlanSingleThreadTimeout::TimeoutInfo>()) {}
 
   void PushNewTimeout() {
     ThreadPlanSingleThreadTimeout::PushNewWithTimeout(m_thread, m_timeout_info);
@@ -32,7 +35,7 @@ class TimeoutResumeAll {
 
 private:
   Thread &m_thread;
-  ThreadPlanSingleThreadTimeout::TimeoutInfo m_timeout_info;
+  std::shared_ptr<ThreadPlanSingleThreadTimeout::TimeoutInfo> m_timeout_info;
 };
 
 } // namespace lldb_private
diff --git a/lldb/source/Target/ThreadPlanSingleThreadTimeout.cpp b/lldb/source/Target/ThreadPlanSingleThreadTimeout.cpp
index 9cc21bfbb1952d..17e6befb7d388c 100644
--- a/lldb/source/Target/ThreadPlanSingleThreadTimeout.cpp
+++ b/lldb/source/Target/ThreadPlanSingleThreadTimeout.cpp
@@ -24,19 +24,20 @@
 using namespace lldb_private;
 using namespace lldb;
 
-ThreadPlanSingleThreadTimeout::ThreadPlanSingleThreadTimeout(Thread &thread,
-                                                             TimeoutInfo &info)
+ThreadPlanSingleThreadTimeout::ThreadPlanSingleThreadTimeout(
+    Thread &thread,
+    std::shared_ptr<ThreadPlanSingleThreadTimeout::TimeoutInfo> &info)
     : ThreadPlan(ThreadPlan::eKindSingleThreadTimeout, "Single thread timeout",
                  thread, eVoteNo, eVoteNoOpinion),
       m_info(info), m_state(State::WaitTimeout) {
   // TODO: reuse m_timer_thread without recreation.
   m_timer_thread = std::thread(TimeoutThreadFunc, this);
-  m_info.m_isAlive = true;
-  m_state = m_info.m_last_state;
+  m_info->m_isAlive = true;
+  m_state = m_info->m_last_state;
 }
 
 ThreadPlanSingleThreadTimeout::~ThreadPlanSingleThreadTimeout() {
-  m_info.m_isAlive = false;
+  m_info->m_isAlive = false;
 }
 
 uint64_t ThreadPlanSingleThreadTimeout::GetRemainingTimeoutMilliSeconds() {
@@ -65,8 +66,9 @@ std::string ThreadPlanSingleThreadTimeout::StateToString(State state) {
   }
 }
 
-void ThreadPlanSingleThreadTimeout::PushNewWithTimeout(Thread &thread,
-                                                       TimeoutInfo &info) {
+void ThreadPlanSingleThreadTimeout::PushNewWithTimeout(
+    Thread &thread,
+    std::shared_ptr<ThreadPlanSingleThreadTimeout::TimeoutInfo> &info) {
   uint64_t timeout_in_ms = thread.GetSingleThreadPlanTimeout();
   if (timeout_in_ms == 0)
     return;
@@ -87,14 +89,15 @@ void ThreadPlanSingleThreadTimeout::PushNewWithTimeout(Thread &thread,
       timeout_in_ms);
 }
 
-void ThreadPlanSingleThreadTimeout::ResumeFromPrevState(Thread &thread,
-                                                        TimeoutInfo &info) {
+void ThreadPlanSingleThreadTimeout::ResumeFromPrevState(
+    Thread &thread,
+    std::shared_ptr<ThreadPlanSingleThreadTimeout::TimeoutInfo> &info) {
   uint64_t timeout_in_ms = thread.GetSingleThreadPlanTimeout();
   if (timeout_in_ms == 0)
     return;
 
   // There is already an instance alive.
-  if (info.m_isAlive)
+  if (info->m_isAlive)
     return;
 
   // Do not create timeout if we are not stopping other threads.
@@ -118,7 +121,7 @@ bool ThreadPlanSingleThreadTimeout::WillStop() {
   LLDB_LOGF(log, "ThreadPlanSingleThreadTimeout::WillStop().");
 
   // Reset the state during stop.
-  m_info.m_last_state = State::WaitTimeout;
+  m_info->m_last_state = State::WaitTimeout;
   return true;
 }
 
@@ -128,7 +131,7 @@ void ThreadPlanSingleThreadTimeout::DidPop() {
     std::lock_guard<std::mutex> lock(m_mutex);
     LLDB_LOGF(log, "ThreadPlanSingleThreadTimeout::DidPop().");
     // Tell timer thread to exit.
-    m_info.m_isAlive = false;
+    m_info->m_isAlive = false;
   }
   m_wakeup_cv.notify_one();
   // Wait for timer thread to exit.
@@ -163,12 +166,12 @@ void ThreadPlanSingleThreadTimeout::TimeoutThreadFunc(
       " ms",
       timeout_in_ms);
   self->m_wakeup_cv.wait_for(lock, std::chrono::milliseconds(timeout_in_ms),
-                             [self] { return !self->m_info.m_isAlive; });
+                             [self] { return !self->m_info->m_isAlive; });
   LLDB_LOGF(log,
             "ThreadPlanSingleThreadTimeout::TimeoutThreadFunc() wake up with "
             "m_isAlive(%d).",
-            self->m_info.m_isAlive);
-  if (!self->m_info.m_isAlive)
+            self->m_info->m_isAlive);
+  if (!self->m_info->m_isAlive)
     return;
 
   self->HandleTimeout();

[jimingham]

This looks fine. I'd add a

using TimeoutInfoSP = std::shared_ptr<...>

and refer to it that way. The construct appears enough times to warrant that; it's a lot easier to read and we use the SP suffix consistently so it will be understood. But that's a matter of taste.

[jeffreytan81]

I am still waiting for all bots to green before landing it. Let me know if I should skip and land immediately to unblock anyone.