llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/149598.diff 6 Files Affected: - (modified) lldb/include/lldb/Target/Thread.h (+2-2) - (modified) lldb/include/lldb/Target/ThreadPlan.h (+3-1) - (modified) lldb/include/lldb/Target/ThreadPlanStack.h (+1-1) - (modified) lldb/source/Target/Thread.cpp (+34-39) - (modified) lldb/source/Target/ThreadPlan.cpp (+5-7) - (modified) lldb/source/Target/ThreadPlanStack.cpp (+4-4) ``````````diff diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h index 6ede7fa301a82..7ee87dfcaeecc 100644 --- a/lldb/include/lldb/Target/Thread.h +++ b/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(); diff --git a/lldb/include/lldb/Target/ThreadPlan.h b/lldb/include/lldb/Target/ThreadPlan.h index a7bac8cc5ecf6..d0567da749de2 100644 --- a/lldb/include/lldb/Target/ThreadPlan.h +++ b/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. diff --git a/lldb/include/lldb/Target/ThreadPlanStack.h b/lldb/include/lldb/Target/ThreadPlanStack.h index e0f8104de9a4d..423a259071f1e 100644 --- a/lldb/include/lldb/Target/ThreadPlanStack.h +++ b/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; diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp index 8c3e19725f8cb..4bad97c096e1f 100644 --- a/lldb/source/Target/Thread.cpp +++ b/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); } diff --git a/lldb/source/Target/ThreadPlan.cpp b/lldb/source/Target/ThreadPlan.cpp index f05e1faf343a6..77e380f95bb7a 100644 --- a/lldb/source/Target/ThreadPlan.cpp +++ b/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) { diff --git a/lldb/source/Target/ThreadPlanStack.cpp b/lldb/source/Target/ThreadPlanStack.cpp index d5d600dda47a3..5402be6b9c48a 100644 --- a/lldb/source/Target/ThreadPlanStack.cpp +++ b/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; } `````````` </details> https://github.com/llvm/llvm-project/pull/149598 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
