[lldb] Use a weak pointer to hold on to the underlying thread plan in SBThreadPlan

[JDevlieghere]

Summary

Use a weak pointer to hold on to the the underlying thread plan in SBThreadPlan. When the process continues, all the popped ThreadPlans get discarded, and you can’t reuse them, so you have to create them anew. Therefore the SBThreadPlan doesn’t need to keep the ThreadPlan alive. This fixes the cleanup error in TestThreadPlanCommands.py and TestStepScripted.py caused by the thread plans never being deleted.

[JDevlieghere]

s/thread_plan/thread_plan_sp/g

[jingham]

Yes, the only way you can get your hands on an SBThreadPlan is in the process of queueing one, so the ThreadPlan stack will always be the one to manage the lifecycle of a plan. And if an SBThreadPlan represents a ThreadPlan that has been discarded, it should convert back to an empty SBThreadPlan rather than keeping the underlying plan alive. So this is correct.

[jingham]

LGTM

lldb/include/lldb/API/SBThreadPlan.h

private:
   friend class SBBreakpointLocation;
   friend class SBFrame;
   friend class SBProcess;
   friend class SBDebugger;
   friend class SBValue;
   friend class lldb_private::QueueImpl;
   friend class SBQueueItem;
 
-  lldb_private::ThreadPlan *get();
+  lldb::ThreadPlanSP GetSP() const { return m_opaque_wp.lock(); }
+  lldb_private::ThreadPlan *get() const { return GetSP().get(); }
   void SetThreadPlan(const lldb::ThreadPlanSP &lldb_object_sp);
 
-  lldb::ThreadPlanSP m_opaque_sp;
+  lldb::ThreadPlanWP m_opaque_wp;
 };
 
 } // namespace lldb
 
 #endif // LLDB_API_SBTHREADPLAN_H

lldb/include/lldb/lldb-forward.h

typedef std::shared_ptr<lldb_private::SyntheticChildrenFrontEnd>
     SyntheticChildrenFrontEndSP;
 typedef std::shared_ptr<lldb_private::Target> TargetSP;
 typedef std::weak_ptr<lldb_private::Target> TargetWP;
 typedef std::shared_ptr<lldb_private::TargetProperties> TargetPropertiesSP;
 typedef std::shared_ptr<lldb_private::Thread> ThreadSP;
 typedef std::weak_ptr<lldb_private::Thread> ThreadWP;
 typedef std::shared_ptr<lldb_private::ThreadCollection> ThreadCollectionSP;
 typedef std::shared_ptr<lldb_private::ThreadPlan> ThreadPlanSP;
+typedef std::weak_ptr<lldb_private::ThreadPlan> ThreadPlanWP;
 typedef std::shared_ptr<lldb_private::ThreadPlanTracer> ThreadPlanTracerSP;
 typedef std::shared_ptr<lldb_private::TraceOptions> TraceOptionsSP;
 typedef std::shared_ptr<lldb_private::Type> TypeSP;
 typedef std::weak_ptr<lldb_private::Type> TypeWP;
 typedef std::shared_ptr<lldb_private::TypeCategoryImpl> TypeCategoryImplSP;
 typedef std::shared_ptr<lldb_private::TypeImpl> TypeImplSP;
 typedef std::shared_ptr<lldb_private::TypeMemberFunctionImpl>
     TypeMemberFunctionImplSP;

lldb/source/API/SBThreadPlan.cpp

 
 using namespace lldb;
 using namespace lldb_private;
 
 // Constructors
 SBThreadPlan::SBThreadPlan() { LLDB_RECORD_CONSTRUCTOR_NO_ARGS(SBThreadPlan); }
 
 SBThreadPlan::SBThreadPlan(const ThreadPlanSP &lldb_object_sp)
-    : m_opaque_sp(lldb_object_sp) {
+    : m_opaque_wp(lldb_object_sp) {
   LLDB_RECORD_CONSTRUCTOR(SBThreadPlan, (const lldb::ThreadPlanSP &),
                           lldb_object_sp);
 }
 
 SBThreadPlan::SBThreadPlan(const SBThreadPlan &rhs)
-    : m_opaque_sp(rhs.m_opaque_sp) {
+    : m_opaque_wp(rhs.m_opaque_wp) {
   LLDB_RECORD_CONSTRUCTOR(SBThreadPlan, (const lldb::SBThreadPlan &), rhs);
 }
 
 SBThreadPlan::SBThreadPlan(lldb::SBThread &sb_thread, const char *class_name) {
   LLDB_RECORD_CONSTRUCTOR(SBThreadPlan, (lldb::SBThread &, const char *),
                           sb_thread, class_name);
 
   Thread *thread = sb_thread.get();
   if (thread)
-    m_opaque_sp = std::make_shared<ThreadPlanPython>(*thread, class_name, 
-                                                     nullptr);
+    m_opaque_wp =
+        std::make_shared<ThreadPlanPython>(*thread, class_name, nullptr);
 }
 
 SBThreadPlan::SBThreadPlan(lldb::SBThread &sb_thread, const char *class_name,
                            lldb::SBStructuredData &args_data) {
   LLDB_RECORD_CONSTRUCTOR(SBThreadPlan, (lldb::SBThread &, const char *,
                                          SBStructuredData &),
                           sb_thread, class_name, args_data);
 
   Thread *thread = sb_thread.get();
   if (thread)
-    m_opaque_sp = std::make_shared<ThreadPlanPython>(*thread, class_name, 
+    m_opaque_wp = std::make_shared<ThreadPlanPython>(*thread, class_name,
                                                      args_data.m_impl_up.get());
 }
 
 // Assignment operator
 
 const lldb::SBThreadPlan &SBThreadPlan::operator=(const SBThreadPlan &rhs) {
   LLDB_RECORD_METHOD(const lldb::SBThreadPlan &,
                      SBThreadPlan, operator=,(const lldb::SBThreadPlan &), rhs);
 
   if (this != &rhs)
-    m_opaque_sp = rhs.m_opaque_sp;
+    m_opaque_wp = rhs.m_opaque_wp;
   return LLDB_RECORD_RESULT(*this);
 }
 // Destructor
 SBThreadPlan::~SBThreadPlan() = default;
 
-lldb_private::ThreadPlan *SBThreadPlan::get() { return m_opaque_sp.get(); }
-
 bool SBThreadPlan::IsValid() const {
   LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBThreadPlan, IsValid);
   return this->operator bool();
 }
 SBThreadPlan::operator bool() const {
   LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBThreadPlan, operator bool);
 
-  return m_opaque_sp.get() != nullptr;
+  return static_cast<bool>(GetSP());
 }
 
 void SBThreadPlan::Clear() {
   LLDB_RECORD_METHOD_NO_ARGS(void, SBThreadPlan, Clear);
 
-  m_opaque_sp.reset();
+  m_opaque_wp.reset();
 }
 
 lldb::StopReason SBThreadPlan::GetStopReason() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::StopReason, SBThreadPlan, GetStopReason);
 
   return eStopReasonNone;
 }
 
 size_t SBThreadPlan::GetStopReasonDataCount() {
   LLDB_RECORD_METHOD_NO_ARGS(size_t, SBThreadPlan, GetStopReasonDataCount);
 
   return 0;
 }
 
 uint64_t SBThreadPlan::GetStopReasonDataAtIndex(uint32_t idx) {
   LLDB_RECORD_METHOD(uint64_t, SBThreadPlan, GetStopReasonDataAtIndex,
                      (uint32_t), idx);
 
   return 0;
 }
 
 SBThread SBThreadPlan::GetThread() const {
   LLDB_RECORD_METHOD_CONST_NO_ARGS(lldb::SBThread, SBThreadPlan, GetThread);
 
-  if (m_opaque_sp) {
+  ThreadPlanSP thread_plan(GetSP());
+  if (thread_plan) {
     return LLDB_RECORD_RESULT(
-        SBThread(m_opaque_sp->GetThread().shared_from_this()));
+        SBThread(thread_plan->GetThread().shared_from_this()));
   } else
     return LLDB_RECORD_RESULT(SBThread());
 }
 
 bool SBThreadPlan::GetDescription(lldb::SBStream &description) const {
   LLDB_RECORD_METHOD_CONST(bool, SBThreadPlan, GetDescription,
                            (lldb::SBStream &), description);
 
-  if (m_opaque_sp) {
-    m_opaque_sp->GetDescription(description.get(), eDescriptionLevelFull);
+  ThreadPlanSP thread_plan(GetSP());
+  if (thread_plan) {
+    thread_plan->GetDescription(description.get(), eDescriptionLevelFull);
   } else {
     description.Printf("Empty SBThreadPlan");
   }
   return true;
 }
 
-void SBThreadPlan::SetThreadPlan(const ThreadPlanSP &lldb_object_sp) {
-  m_opaque_sp = lldb_object_sp;
+void SBThreadPlan::SetThreadPlan(const ThreadPlanSP &lldb_object_wp) {
+  m_opaque_wp = lldb_object_wp;
 }
 
 void SBThreadPlan::SetPlanComplete(bool success) {
   LLDB_RECORD_METHOD(void, SBThreadPlan, SetPlanComplete, (bool), success);
 
-  if (m_opaque_sp)
-    m_opaque_sp->SetPlanComplete(success);
+  ThreadPlanSP thread_plan(GetSP());
+  if (thread_plan)
+    thread_plan->SetPlanComplete(success);
 }
 
 bool SBThreadPlan::IsPlanComplete() {
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBThreadPlan, IsPlanComplete);
 
-  if (m_opaque_sp)
-    return m_opaque_sp->IsPlanComplete();
-  else
-    return true;
+  ThreadPlanSP thread_plan(GetSP());
+  if (thread_plan)
+    return thread_plan->IsPlanComplete();
 }
 
 bool SBThreadPlan::IsPlanStale() {
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBThreadPlan, IsPlanStale);
 
-  if (m_opaque_sp)
-    return m_opaque_sp->IsPlanStale();
-  else
-    return true;
+  ThreadPlanSP thread_plan(GetSP());
+  if (thread_plan)
+    return thread_plan->IsPlanStale();
 }
 
 bool SBThreadPlan::IsValid() {
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBThreadPlan, IsValid);
 
-  if (m_opaque_sp)
-    return m_opaque_sp->ValidatePlan(nullptr);
-  else
-    return false;
+  ThreadPlanSP thread_plan(GetSP());
+  if (thread_plan)
+    return thread_plan->ValidatePlan(nullptr);
 }
 
 // This section allows an SBThreadPlan to push another of the common types of
 // plans...
 //
 // FIXME, you should only be able to queue thread plans from inside the methods
 // of a Scripted Thread Plan.  Need a way to enforce that.
 

 
 SBThreadPlan SBThreadPlan::QueueThreadPlanForStepOverRange(
     SBAddress &sb_start_address, lldb::addr_t size, SBError &error) {
   LLDB_RECORD_METHOD(lldb::SBThreadPlan, SBThreadPlan,
                      QueueThreadPlanForStepOverRange,
                      (lldb::SBAddress &, lldb::addr_t, lldb::SBError &),
                      sb_start_address, size, error);
 
-  if (m_opaque_sp) {
+  ThreadPlanSP thread_plan(GetSP());
+  if (thread_plan) {
     Address *start_address = sb_start_address.get();
     if (!start_address) {
       return LLDB_RECORD_RESULT(SBThreadPlan());
     }
 
     AddressRange range(*start_address, size);
     SymbolContext sc;
     start_address->CalculateSymbolContext(&sc);
     Status plan_status;
 
     SBThreadPlan plan =
-        SBThreadPlan(m_opaque_sp->GetThread().QueueThreadPlanForStepOverRange(
+        SBThreadPlan(thread_plan->GetThread().QueueThreadPlanForStepOverRange(
             false, range, sc, eAllThreads, plan_status));
 
     if (plan_status.Fail())
       error.SetErrorString(plan_status.AsCString());
     else
-      plan.m_opaque_sp->SetPrivate(true);
-    
+      plan.GetSP()->SetPrivate(true);
     return LLDB_RECORD_RESULT(plan);
-  } else {
-    return LLDB_RECORD_RESULT(SBThreadPlan());
   }
+  return LLDB_RECORD_RESULT(SBThreadPlan());
 }
 
 SBThreadPlan
 SBThreadPlan::QueueThreadPlanForStepInRange(SBAddress &sb_start_address,
                                             lldb::addr_t size) {
   LLDB_RECORD_METHOD(lldb::SBThreadPlan, SBThreadPlan,
                      QueueThreadPlanForStepInRange,
                      (lldb::SBAddress &, lldb::addr_t), sb_start_address, size);
 
   SBError error;
   return LLDB_RECORD_RESULT(
       QueueThreadPlanForStepInRange(sb_start_address, size, error));
 }
 
 SBThreadPlan
 SBThreadPlan::QueueThreadPlanForStepInRange(SBAddress &sb_start_address,
                                             lldb::addr_t size, SBError &error) {
   LLDB_RECORD_METHOD(lldb::SBThreadPlan, SBThreadPlan,
                      QueueThreadPlanForStepInRange,
                      (lldb::SBAddress &, lldb::addr_t, lldb::SBError &),
                      sb_start_address, size, error);
 
-  if (m_opaque_sp) {
+  ThreadPlanSP thread_plan(GetSP());
+  if (thread_plan) {
     Address *start_address = sb_start_address.get();
     if (!start_address) {
       return LLDB_RECORD_RESULT(SBThreadPlan());
     }
 
     AddressRange range(*start_address, size);
     SymbolContext sc;
     start_address->CalculateSymbolContext(&sc);
 
     Status plan_status;
     SBThreadPlan plan =
-        SBThreadPlan(m_opaque_sp->GetThread().QueueThreadPlanForStepInRange(
+        SBThreadPlan(thread_plan->GetThread().QueueThreadPlanForStepInRange(
             false, range, sc, nullptr, eAllThreads, plan_status));
 
     if (plan_status.Fail())
       error.SetErrorString(plan_status.AsCString());
     else
-      plan.m_opaque_sp->SetPrivate(true);
+      plan.GetSP()->SetPrivate(true);
 
     return LLDB_RECORD_RESULT(plan);
-  } else {
-    return LLDB_RECORD_RESULT(SBThreadPlan());
   }
+  return LLDB_RECORD_RESULT(SBThreadPlan());
 }
 
 SBThreadPlan
 SBThreadPlan::QueueThreadPlanForStepOut(uint32_t frame_idx_to_step_to,
                                         bool first_insn) {
   LLDB_RECORD_METHOD(lldb::SBThreadPlan, SBThreadPlan,
                      QueueThreadPlanForStepOut, (uint32_t, bool),
                      frame_idx_to_step_to, first_insn);
 
   SBError error;
   return LLDB_RECORD_RESULT(
       QueueThreadPlanForStepOut(frame_idx_to_step_to, first_insn, error));
 }
 
 SBThreadPlan
 SBThreadPlan::QueueThreadPlanForStepOut(uint32_t frame_idx_to_step_to,
                                         bool first_insn, SBError &error) {
   LLDB_RECORD_METHOD(lldb::SBThreadPlan, SBThreadPlan,
                      QueueThreadPlanForStepOut,
                      (uint32_t, bool, lldb::SBError &), frame_idx_to_step_to,
                      first_insn, error);
 
-  if (m_opaque_sp) {
+  ThreadPlanSP thread_plan(GetSP());
+  if (thread_plan) {
     SymbolContext sc;
-    sc = m_opaque_sp->GetThread().GetStackFrameAtIndex(0)->GetSymbolContext(
+    sc = thread_plan->GetThread().GetStackFrameAtIndex(0)->GetSymbolContext(
         lldb::eSymbolContextEverything);
 
     Status plan_status;
     SBThreadPlan plan =
-        SBThreadPlan(m_opaque_sp->GetThread().QueueThreadPlanForStepOut(
+        SBThreadPlan(thread_plan->GetThread().QueueThreadPlanForStepOut(
             false, &sc, first_insn, false, eVoteYes, eVoteNoOpinion,
             frame_idx_to_step_to, plan_status));
 
     if (plan_status.Fail())
       error.SetErrorString(plan_status.AsCString());
     else
-      plan.m_opaque_sp->SetPrivate(true);
+      plan.GetSP()->SetPrivate(true);
 
     return LLDB_RECORD_RESULT(plan);
-  } else {
-    return LLDB_RECORD_RESULT(SBThreadPlan());
   }
+  return LLDB_RECORD_RESULT(SBThreadPlan());
 }
 
 SBThreadPlan
 SBThreadPlan::QueueThreadPlanForRunToAddress(SBAddress sb_address) {
   LLDB_RECORD_METHOD(lldb::SBThreadPlan, SBThreadPlan,
                      QueueThreadPlanForRunToAddress, (lldb::SBAddress),
                      sb_address);
 
   SBError error;
   return LLDB_RECORD_RESULT(QueueThreadPlanForRunToAddress(sb_address, error));
 }
 
 SBThreadPlan SBThreadPlan::QueueThreadPlanForRunToAddress(SBAddress sb_address,
                                                           SBError &error) {
   LLDB_RECORD_METHOD(lldb::SBThreadPlan, SBThreadPlan,
                      QueueThreadPlanForRunToAddress,
                      (lldb::SBAddress, lldb::SBError &), sb_address, error);
 
-  if (m_opaque_sp) {
+  ThreadPlanSP thread_plan(GetSP());
+  if (thread_plan) {
     Address *address = sb_address.get();
     if (!address)
       return LLDB_RECORD_RESULT(SBThreadPlan());
 
     Status plan_status;
     SBThreadPlan plan =
-        SBThreadPlan(m_opaque_sp->GetThread().QueueThreadPlanForRunToAddress(
+        SBThreadPlan(thread_plan->GetThread().QueueThreadPlanForRunToAddress(
             false, *address, false, plan_status));
 
     if (plan_status.Fail())
       error.SetErrorString(plan_status.AsCString());
     else
-      plan.m_opaque_sp->SetPrivate(true);
+      plan.GetSP()->SetPrivate(true);
 
     return LLDB_RECORD_RESULT(plan);
-  } else {
-    return LLDB_RECORD_RESULT(SBThreadPlan());
   }
+  return LLDB_RECORD_RESULT(SBThreadPlan());
 }
 
 SBThreadPlan
 SBThreadPlan::QueueThreadPlanForStepScripted(const char *script_class_name) {
   LLDB_RECORD_METHOD(lldb::SBThreadPlan, SBThreadPlan,
                      QueueThreadPlanForStepScripted, (const char *),
                      script_class_name);
 
   SBError error;
   return LLDB_RECORD_RESULT(
       QueueThreadPlanForStepScripted(script_class_name, error));
 }
 
 SBThreadPlan
 SBThreadPlan::QueueThreadPlanForStepScripted(const char *script_class_name,
                                              SBError &error) {
   LLDB_RECORD_METHOD(lldb::SBThreadPlan, SBThreadPlan,
                      QueueThreadPlanForStepScripted,
                      (const char *, lldb::SBError &), script_class_name, error);
 
-  if (m_opaque_sp) {
+  ThreadPlanSP thread_plan(GetSP());
+  if (thread_plan) {
     Status plan_status;
     StructuredData::ObjectSP empty_args;
     SBThreadPlan plan =
-        SBThreadPlan(m_opaque_sp->GetThread().QueueThreadPlanForStepScripted(
+        SBThreadPlan(thread_plan->GetThread().QueueThreadPlanForStepScripted(
             false, script_class_name, empty_args, false, plan_status));
 
     if (plan_status.Fail())
       error.SetErrorString(plan_status.AsCString());
     else
-      plan.m_opaque_sp->SetPrivate(true);
+      plan.GetSP()->SetPrivate(true);
 
     return LLDB_RECORD_RESULT(plan);
-  } else {
-    return LLDB_RECORD_RESULT(SBThreadPlan());
   }
+  return LLDB_RECORD_RESULT(SBThreadPlan());
 }
 
 SBThreadPlan
 SBThreadPlan::QueueThreadPlanForStepScripted(const char *script_class_name,
                                              lldb::SBStructuredData &args_data,
                                              SBError &error) {
   LLDB_RECORD_METHOD(lldb::SBThreadPlan, SBThreadPlan,
                      QueueThreadPlanForStepScripted,
                      (const char *, lldb::SBStructuredData &, lldb::SBError &), 
                      script_class_name, args_data, error);
 
-  if (m_opaque_sp) {
+  ThreadPlanSP thread_plan(GetSP());
+  if (thread_plan) {
     Status plan_status;
     StructuredData::ObjectSP args_obj = args_data.m_impl_up->GetObjectSP();
     SBThreadPlan plan =
-        SBThreadPlan(m_opaque_sp->GetThread().QueueThreadPlanForStepScripted(
+        SBThreadPlan(thread_plan->GetThread().QueueThreadPlanForStepScripted(
             false, script_class_name, args_obj, false, plan_status));
 
     if (plan_status.Fail())
       error.SetErrorString(plan_status.AsCString());
     else
-      plan.m_opaque_sp->SetPrivate(true);
+      plan.GetSP()->SetPrivate(true);
 
     return LLDB_RECORD_RESULT(plan);
   } else {
     return LLDB_RECORD_RESULT(SBThreadPlan());
   }
 }
 
 namespace lldb_private {