[lldb] Use ThreadPlanSP instead of raw ThreadPlan* (NFC)

I'm investigating a lifetime issue and I noticed raw ThreadPlan pointers
being passed around. Although this may be safe, it's really hard to
prove and more fundamentally, defeats the purpose of using smart
pointers in the first place.

Author: JDevlieghere

lldb/include/lldb/Target/Thread.h

@@ -1017,7 +1017,7 @@ class Thread : public std::enable_shared_from_this<Thread>,
   ///
   /// \return
   ///     A pointer to the next executed plan.
-  ThreadPlan *GetCurrentPlan() const;
+  lldb::ThreadPlanSP GetCurrentPlan() const;
 
   /// Unwinds the thread stack for the innermost expression plan currently
   /// on the thread plan stack.
@@ -1311,7 +1311,7 @@ class Thread : public std::enable_shared_from_this<Thread>,
 
   void DiscardPlan();
 
-  ThreadPlan *GetPreviousPlan(ThreadPlan *plan) const;
+  lldb::ThreadPlanSP GetPreviousPlan(ThreadPlan *plan) const;
 
   virtual Unwind &GetUnwinder();
 

lldb/include/lldb/Target/ThreadPlan.h

@@ -536,7 +536,9 @@ class ThreadPlan : public std::enable_shared_from_this<ThreadPlan>,
   // This is mostly a formal requirement, it allows us to make the Thread's
   // GetPreviousPlan protected, but only friend ThreadPlan to thread.
 
-  ThreadPlan *GetPreviousPlan() { return GetThread().GetPreviousPlan(this); }
+  lldb::ThreadPlanSP GetPreviousPlan() {
+    return GetThread().GetPreviousPlan(this);
+  }
 
   // This forwards the private Thread::GetPrivateStopInfo which is generally
   // what ThreadPlan's need to know.

lldb/include/lldb/Target/ThreadPlanStack.h

@@ -85,7 +85,7 @@ class ThreadPlanStack {
 
   bool WasPlanDiscarded(ThreadPlan *plan) const;
 
-  ThreadPlan *GetPreviousPlan(ThreadPlan *current_plan) const;
+  lldb::ThreadPlanSP GetPreviousPlan(ThreadPlan *current_plan) const;
 
   ThreadPlan *GetInnermostExpression() const;
 

lldb/source/Target/Thread.cpp

@@ -609,15 +609,10 @@ std::string Thread::GetStopDescriptionRaw() {
 }
 
 void Thread::WillStop() {
-  ThreadPlan *current_plan = GetCurrentPlan();
-
   // FIXME: I may decide to disallow threads with no plans.  In which
   // case this should go to an assert.
-
-  if (!current_plan)
-    return;
-
-  current_plan->WillStop();
+  if (ThreadPlanSP current_plan = GetCurrentPlan())
+    current_plan->WillStop();
 }
 
 bool Thread::SetupToStepOverBreakpointIfNeeded(RunDirection direction) {
@@ -652,12 +647,12 @@ bool Thread::SetupToStepOverBreakpointIfNeeded(RunDirection direction) {
         // Note, don't assume there's a ThreadPlanStepOverBreakpoint, the
         // target may not require anything special to step over a breakpoint.
 
-        ThreadPlan *cur_plan = GetCurrentPlan();
+        ThreadPlanSP cur_plan_sp = GetCurrentPlan();
 
         bool push_step_over_bp_plan = false;
-        if (cur_plan->GetKind() == ThreadPlan::eKindStepOverBreakpoint) {
+        if (cur_plan_sp->GetKind() == ThreadPlan::eKindStepOverBreakpoint) {
           ThreadPlanStepOverBreakpoint *bp_plan =
-              (ThreadPlanStepOverBreakpoint *)cur_plan;
+              (ThreadPlanStepOverBreakpoint *)cur_plan_sp.get();
           if (bp_plan->GetBreakpointLoadAddress() != thread_pc)
             push_step_over_bp_plan = true;
         } else
@@ -720,12 +715,12 @@ bool Thread::ShouldResume(StateType resume_state) {
   // runs.
 
   bool need_to_resume = false;
-  ThreadPlan *plan_ptr = GetCurrentPlan();
-  if (plan_ptr) {
-    need_to_resume = plan_ptr->WillResume(resume_state, true);
+  ThreadPlanSP plan_sp = GetCurrentPlan();
+  if (plan_sp) {
+    need_to_resume = plan_sp->WillResume(resume_state, true);
 
-    while ((plan_ptr = GetPreviousPlan(plan_ptr)) != nullptr) {
-      plan_ptr->WillResume(resume_state, false);
+    while ((plan_sp = GetPreviousPlan(plan_sp.get()))) {
+      plan_sp->WillResume(resume_state, false);
     }
 
     // If the WillResume for the plan says we are faking a resume, then it will
@@ -755,7 +750,7 @@ void Thread::DidResume() {
 void Thread::DidStop() { SetState(eStateStopped); }
 
 bool Thread::ShouldStop(Event *event_ptr) {
-  ThreadPlan *current_plan = GetCurrentPlan();
+  ThreadPlanSP current_plan = GetCurrentPlan();
 
   bool should_stop = true;
 
@@ -854,34 +849,34 @@ bool Thread::ShouldStop(Event *event_ptr) {
 
       // If the current plan doesn't explain the stop, then find one that does
       // and let it handle the situation.
-      ThreadPlan *plan_ptr = current_plan;
-      while ((plan_ptr = GetPreviousPlan(plan_ptr)) != nullptr) {
-        if (plan_ptr->PlanExplainsStop(event_ptr)) {
-          LLDB_LOGF(log, "Plan %s explains stop.", plan_ptr->GetName());
+      ThreadPlanSP plan_sp = current_plan;
+      while ((plan_sp = GetPreviousPlan(plan_sp.get())) != nullptr) {
+        if (plan_sp->PlanExplainsStop(event_ptr)) {
+          LLDB_LOGF(log, "Plan %s explains stop.", plan_sp->GetName());
 
-          should_stop = plan_ptr->ShouldStop(event_ptr);
+          should_stop = plan_sp->ShouldStop(event_ptr);
 
           // plan_ptr explains the stop, next check whether plan_ptr is done,
           // if so, then we should take it and all the plans below it off the
           // stack.
 
-          if (plan_ptr->MischiefManaged()) {
+          if (plan_sp->MischiefManaged()) {
             // We're going to pop the plans up to and including the plan that
             // explains the stop.
-            ThreadPlan *prev_plan_ptr = GetPreviousPlan(plan_ptr);
+            ThreadPlanSP prev_plan_sp = GetPreviousPlan(plan_sp.get());
 
             do {
               if (should_stop)
                 current_plan->WillStop();
               PopPlan();
-            } while ((current_plan = GetCurrentPlan()) != prev_plan_ptr);
+            } while ((current_plan = GetCurrentPlan()) != prev_plan_sp);
             // Now, if the responsible plan was not "Okay to discard" then
             // we're done, otherwise we forward this to the next plan in the
             // stack below.
             done_processing_current_plan =
-                (plan_ptr->IsControllingPlan() && !plan_ptr->OkayToDiscard());
+                (plan_sp->IsControllingPlan() && !plan_sp->OkayToDiscard());
           } else {
-            bool should_force_run = plan_ptr->ShouldRunBeforePublicStop();
+            bool should_force_run = plan_sp->ShouldRunBeforePublicStop();
             if (should_force_run) {
               SetShouldRunBeforePublicStop(true);
               should_stop = false;
@@ -952,14 +947,14 @@ bool Thread::ShouldStop(Event *event_ptr) {
   // original plan on the stack, This code clears stale plans off the stack.
 
   if (should_stop) {
-    ThreadPlan *plan_ptr = GetCurrentPlan();
+    ThreadPlanSP plan_sp = GetCurrentPlan();
 
     // Discard the stale plans and all plans below them in the stack, plus move
     // the completed plans to the completed plan stack
-    while (!plan_ptr->IsBasePlan()) {
-      bool stale = plan_ptr->IsPlanStale();
-      ThreadPlan *examined_plan = plan_ptr;
-      plan_ptr = GetPreviousPlan(examined_plan);
+    while (!plan_sp->IsBasePlan()) {
+      bool stale = plan_sp->IsPlanStale();
+      ThreadPlanSP examined_plan = plan_sp;
+      plan_sp = GetPreviousPlan(examined_plan.get());
 
       if (stale) {
         LLDB_LOGF(
@@ -1034,16 +1029,16 @@ Vote Thread::ShouldReportStop(Event *event_ptr) {
     return GetPlans().GetCompletedPlan(false)->ShouldReportStop(event_ptr);
   } else {
     Vote thread_vote = eVoteNoOpinion;
-    ThreadPlan *plan_ptr = GetCurrentPlan();
+    ThreadPlanSP plan_sp = GetCurrentPlan();
     while (true) {
-      if (plan_ptr->PlanExplainsStop(event_ptr)) {
-        thread_vote = plan_ptr->ShouldReportStop(event_ptr);
+      if (plan_sp->PlanExplainsStop(event_ptr)) {
+        thread_vote = plan_sp->ShouldReportStop(event_ptr);
         break;
       }
-      if (plan_ptr->IsBasePlan())
+      if (plan_sp->IsBasePlan())
         break;
       else
-        plan_ptr = GetPreviousPlan(plan_ptr);
+        plan_sp = GetPreviousPlan(plan_sp.get());
     }
     LLDB_LOGF(log,
               "Thread::ShouldReportStop() tid = 0x%4.4" PRIx64
@@ -1154,8 +1149,8 @@ void Thread::AutoCompleteThreadPlans(CompletionRequest &request) const {
   }
 }
 
-ThreadPlan *Thread::GetCurrentPlan() const {
-  return GetPlans().GetCurrentPlan().get();
+ThreadPlanSP Thread::GetCurrentPlan() const {
+  return GetPlans().GetCurrentPlan();
 }
 
 ThreadPlanSP Thread::GetCompletedPlan() const {
@@ -1182,7 +1177,7 @@ bool Thread::CompletedPlanOverridesBreakpoint() const {
   return GetPlans().AnyCompletedPlans();
 }
 
-ThreadPlan *Thread::GetPreviousPlan(ThreadPlan *current_plan) const{
+lldb::ThreadPlanSP Thread::GetPreviousPlan(ThreadPlan *current_plan) const {
   return GetPlans().GetPreviousPlan(current_plan);
 }
 

lldb/source/Target/ThreadPlan.cpp

@@ -80,8 +80,7 @@ Vote ThreadPlan::ShouldReportStop(Event *event_ptr) {
   Log *log = GetLog(LLDBLog::Step);
 
   if (m_report_stop_vote == eVoteNoOpinion) {
-    ThreadPlan *prev_plan = GetPreviousPlan();
-    if (prev_plan) {
+    if (ThreadPlanSP prev_plan = GetPreviousPlan()) {
       Vote prev_vote = prev_plan->ShouldReportStop(event_ptr);
       LLDB_LOG(log, "returning previous thread plan vote: {0}", prev_vote);
       return prev_vote;
@@ -93,8 +92,7 @@ Vote ThreadPlan::ShouldReportStop(Event *event_ptr) {
 
 Vote ThreadPlan::ShouldReportRun(Event *event_ptr) {
   if (m_report_run_vote == eVoteNoOpinion) {
-    ThreadPlan *prev_plan = GetPreviousPlan();
-    if (prev_plan)
+    if (ThreadPlanSP prev_plan = GetPreviousPlan())
       return prev_plan->ShouldReportRun(event_ptr);
   }
   return m_report_run_vote;
@@ -103,9 +101,9 @@ Vote ThreadPlan::ShouldReportRun(Event *event_ptr) {
 void ThreadPlan::ClearThreadCache() { m_thread = nullptr; }
 
 bool ThreadPlan::StopOthers() {
-  ThreadPlan *prev_plan;
-  prev_plan = GetPreviousPlan();
-  return (prev_plan == nullptr) ? false : prev_plan->StopOthers();
+  if (ThreadPlanSP prev_plan = GetPreviousPlan())
+    return prev_plan->StopOthers();
+  return false;
 }
 
 void ThreadPlan::SetStopOthers(bool new_value) {

lldb/source/Target/ThreadPlanStack.cpp

@@ -355,7 +355,7 @@ bool ThreadPlanStack::WasPlanDiscarded(ThreadPlan *in_plan) const {
   return false;
 }
 
-ThreadPlan *ThreadPlanStack::GetPreviousPlan(ThreadPlan *current_plan) const {
+ThreadPlanSP ThreadPlanStack::GetPreviousPlan(ThreadPlan *current_plan) const {
   llvm::sys::ScopedReader guard(m_stack_mutex);
   if (current_plan == nullptr)
     return nullptr;
@@ -365,20 +365,20 @@ ThreadPlan *ThreadPlanStack::GetPreviousPlan(ThreadPlan *current_plan) const {
   int stack_size = m_completed_plans.size();
   for (int i = stack_size - 1; i > 0; i--) {
     if (current_plan == m_completed_plans[i].get())
-      return m_completed_plans[i - 1].get();
+      return m_completed_plans[i - 1];
   }
 
   // If this is the first completed plan, the previous one is the
   // bottom of the regular plan stack.
   if (stack_size > 0 && m_completed_plans[0].get() == current_plan) {
-    return GetCurrentPlanNoLock().get();
+    return GetCurrentPlanNoLock();
   }
 
   // Otherwise look for it in the regular plans.
   stack_size = m_plans.size();
   for (int i = stack_size - 1; i > 0; i--) {
     if (current_plan == m_plans[i].get())
-      return m_plans[i - 1].get();
+      return m_plans[i - 1];
   }
   return nullptr;
 }