[Lldb-commits] [lldb] 41d0b20 - [lldb] Avoid moving ThreadPlanSP from plans vector
Dave Lee via lldb-commits
lldb-commits at lists.llvm.org
Sun Aug 1 11:43:57 PDT 2021
Author: Dave Lee
Date: 2021-08-01T10:40:04-07:00
New Revision: 41d0b20cc90f2aea25b4306f6c8f6c258ca3d377
URL: https://github.com/llvm/llvm-project/commit/41d0b20cc90f2aea25b4306f6c8f6c258ca3d377
DIFF: https://github.com/llvm/llvm-project/commit/41d0b20cc90f2aea25b4306f6c8f6c258ca3d377.diff
LOG: [lldb] Avoid moving ThreadPlanSP from plans vector
Change `ThreadPlanStack::PopPlan` and `::DiscardPlan` to not do the following:
1. Move the last plan, leaving a moved `ThreadPlanSP` in the plans vector
2. Operate on the last plan
3. Pop the last plan off the plans vector
This leaves a period of time where the last element in the plans vector has been moved. I am not sure what, if any, guarantees there are when doing this, but it seems like it would/could leave a null `ThreadPlanSP` in the container. There are asserts in place to prevent empty/null `ThreadPlanSP` instances from being pushed on to the stack, and so this could break that invariant during multithreaded access to the thread plan stack.
An open question is whether this use of `std::move` was the result of a measure performance problem.
Differential Revision: https://reviews.llvm.org/D106171
Added:
Modified:
lldb/include/lldb/Target/ThreadPlan.h
lldb/include/lldb/Target/ThreadPlanCallFunction.h
lldb/include/lldb/Target/ThreadPlanCallUserExpression.h
lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h
lldb/source/Target/ThreadPlan.cpp
lldb/source/Target/ThreadPlanCallFunction.cpp
lldb/source/Target/ThreadPlanCallUserExpression.cpp
lldb/source/Target/ThreadPlanStack.cpp
lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Target/ThreadPlan.h b/lldb/include/lldb/Target/ThreadPlan.h
index 5e14a1fd6577d..58d6ef5fef8ba 100644
--- a/lldb/include/lldb/Target/ThreadPlan.h
+++ b/lldb/include/lldb/Target/ThreadPlan.h
@@ -81,7 +81,7 @@ namespace lldb_private {
//
// Cleaning up after your plans:
//
-// When the plan is moved from the plan stack its WillPop method is always
+// When the plan is moved from the plan stack its DidPop method is always
// called, no matter why. Once it is moved off the plan stack it is done, and
// won't get a chance to run again. So you should undo anything that affects
// target state in this method. But be sure to leave the plan able to
@@ -413,7 +413,7 @@ class ThreadPlan : public std::enable_shared_from_this<ThreadPlan>,
virtual void DidPush();
- virtual void WillPop();
+ virtual void DidPop();
ThreadPlanKind GetKind() const { return m_kind; }
diff --git a/lldb/include/lldb/Target/ThreadPlanCallFunction.h b/lldb/include/lldb/Target/ThreadPlanCallFunction.h
index 24c5736f44c3f..cb6e7caebb4ad 100644
--- a/lldb/include/lldb/Target/ThreadPlanCallFunction.h
+++ b/lldb/include/lldb/Target/ThreadPlanCallFunction.h
@@ -68,10 +68,10 @@ class ThreadPlanCallFunction : public ThreadPlan {
// been cleaned up.
lldb::addr_t GetFunctionStackPointer() { return m_function_sp; }
- // Classes that derive from FunctionCaller, and implement their own WillPop
+ // Classes that derive from FunctionCaller, and implement their own DidPop
// methods should call this so that the thread state gets restored if the
// plan gets discarded.
- void WillPop() override;
+ void DidPop() override;
// If the thread plan stops mid-course, this will be the stop reason that
// interrupted us. Once DoTakedown is called, this will be the real stop
diff --git a/lldb/include/lldb/Target/ThreadPlanCallUserExpression.h b/lldb/include/lldb/Target/ThreadPlanCallUserExpression.h
index adaea6c7056fe..11e126a2da9cb 100644
--- a/lldb/include/lldb/Target/ThreadPlanCallUserExpression.h
+++ b/lldb/include/lldb/Target/ThreadPlanCallUserExpression.h
@@ -32,7 +32,7 @@ class ThreadPlanCallUserExpression : public ThreadPlanCallFunction {
void DidPush() override;
- void WillPop() override;
+ void DidPop() override;
lldb::StopInfoSP GetRealStopInfo() override;
diff --git a/lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h b/lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h
index 86f7798487c30..1f3aff45c49ab 100644
--- a/lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h
+++ b/lldb/include/lldb/Target/ThreadPlanStepOverBreakpoint.h
@@ -26,7 +26,7 @@ class ThreadPlanStepOverBreakpoint : public ThreadPlan {
bool StopOthers() override;
lldb::StateType GetPlanRunState() override;
bool WillStop() override;
- void WillPop() override;
+ void DidPop() override;
bool MischiefManaged() override;
void ThreadDestroyed() override;
void SetAutoContinue(bool do_it);
diff --git a/lldb/source/Target/ThreadPlan.cpp b/lldb/source/Target/ThreadPlan.cpp
index 6b55f3912d11b..b996cb1d432a2 100644
--- a/lldb/source/Target/ThreadPlan.cpp
+++ b/lldb/source/Target/ThreadPlan.cpp
@@ -149,7 +149,7 @@ lldb::user_id_t ThreadPlan::GetNextID() {
void ThreadPlan::DidPush() {}
-void ThreadPlan::WillPop() {}
+void ThreadPlan::DidPop() {}
bool ThreadPlan::OkayToDiscard() {
return IsMasterPlan() ? m_okay_to_discard : true;
diff --git a/lldb/source/Target/ThreadPlanCallFunction.cpp b/lldb/source/Target/ThreadPlanCallFunction.cpp
index 3699a507d0589..08efc43032ae8 100644
--- a/lldb/source/Target/ThreadPlanCallFunction.cpp
+++ b/lldb/source/Target/ThreadPlanCallFunction.cpp
@@ -209,7 +209,7 @@ void ThreadPlanCallFunction::DoTakedown(bool success) {
}
}
-void ThreadPlanCallFunction::WillPop() { DoTakedown(PlanSucceeded()); }
+void ThreadPlanCallFunction::DidPop() { DoTakedown(PlanSucceeded()); }
void ThreadPlanCallFunction::GetDescription(Stream *s, DescriptionLevel level) {
if (level == eDescriptionLevelBrief) {
diff --git a/lldb/source/Target/ThreadPlanCallUserExpression.cpp b/lldb/source/Target/ThreadPlanCallUserExpression.cpp
index 9dddd850b6ab4..513d599b9c4ad 100644
--- a/lldb/source/Target/ThreadPlanCallUserExpression.cpp
+++ b/lldb/source/Target/ThreadPlanCallUserExpression.cpp
@@ -59,8 +59,8 @@ void ThreadPlanCallUserExpression::DidPush() {
m_user_expression_sp->WillStartExecuting();
}
-void ThreadPlanCallUserExpression::WillPop() {
- ThreadPlanCallFunction::WillPop();
+void ThreadPlanCallUserExpression::DidPop() {
+ ThreadPlanCallFunction::DidPop();
if (m_user_expression_sp)
m_user_expression_sp.reset();
}
diff --git a/lldb/source/Target/ThreadPlanStack.cpp b/lldb/source/Target/ThreadPlanStack.cpp
index d25602d25b91f..c5ee08341b914 100644
--- a/lldb/source/Target/ThreadPlanStack.cpp
+++ b/lldb/source/Target/ThreadPlanStack.cpp
@@ -150,10 +150,13 @@ lldb::ThreadPlanSP ThreadPlanStack::PopPlan() {
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
assert(m_plans.size() > 1 && "Can't pop the base thread plan");
- lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
- m_completed_plans.push_back(plan_sp);
- plan_sp->WillPop();
+ // Note that moving the top element of the vector would leave it in an
+ // undefined state, and break the guarantee that the stack's thread plans are
+ // all valid.
+ lldb::ThreadPlanSP plan_sp = m_plans.back();
m_plans.pop_back();
+ m_completed_plans.push_back(plan_sp);
+ plan_sp->DidPop();
return plan_sp;
}
@@ -161,10 +164,13 @@ lldb::ThreadPlanSP ThreadPlanStack::DiscardPlan() {
std::lock_guard<std::recursive_mutex> guard(m_stack_mutex);
assert(m_plans.size() > 1 && "Can't discard the base thread plan");
- lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
- m_discarded_plans.push_back(plan_sp);
- plan_sp->WillPop();
+ // Note that moving the top element of the vector would leave it in an
+ // undefined state, and break the guarantee that the stack's thread plans are
+ // all valid.
+ lldb::ThreadPlanSP plan_sp = m_plans.back();
m_plans.pop_back();
+ m_discarded_plans.push_back(plan_sp);
+ plan_sp->DidPop();
return plan_sp;
}
diff --git a/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp b/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp
index 965a7b3a99604..f007b0fa9371b 100644
--- a/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp
+++ b/lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp
@@ -124,9 +124,7 @@ bool ThreadPlanStepOverBreakpoint::WillStop() {
return true;
}
-void ThreadPlanStepOverBreakpoint::WillPop() {
- ReenableBreakpointSite();
-}
+void ThreadPlanStepOverBreakpoint::DidPop() { ReenableBreakpointSite(); }
bool ThreadPlanStepOverBreakpoint::MischiefManaged() {
lldb::addr_t pc_addr = GetThread().GetRegisterContext()->GetPC();
More information about the lldb-commits
mailing list