kastiglione created this revision.
kastiglione added a reviewer: jingham.
kastiglione requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
While learning the semantics of `IsMasterPlan` and `OkayToDiscard`, and guided
by the
FIXME comment in ThreadPlan.h, this change decouples the two functions.
These two functions have been called in pairs, for example:
SetIsMasterPlan(true);
SetOkayToDiscard(false);
With this change, almost all cases turn into a single `SetIsMasterPlan(true)`
call. Note
that `m_okay_to_discard` now defaults to false.
Most of the decoupling happens in
`ThreadPlanStack::DiscardConsultingMasterPlans`, which
handles `OkayToDiscard` and `IsMasterPlan` individually. This function could be
renamed,
but I haven't thought of a good alternative.
Other notes:
- `OkayToDiscard` is no longer virtual
- `ThreadPlanPython` was incorrectly calling `SetOkayToDiscard(true)`
- Reworded some comments
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D96715
Files:
lldb/include/lldb/Target/ThreadPlan.h
lldb/include/lldb/Target/ThreadPlanBase.h
lldb/source/API/SBThread.cpp
lldb/source/Commands/CommandObjectThread.cpp
lldb/source/Expression/FunctionCaller.cpp
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp
lldb/source/Target/Process.cpp
lldb/source/Target/StopInfo.cpp
lldb/source/Target/Thread.cpp
lldb/source/Target/ThreadPlan.cpp
lldb/source/Target/ThreadPlanCallFunction.cpp
lldb/source/Target/ThreadPlanCallUserExpression.cpp
lldb/source/Target/ThreadPlanPython.cpp
lldb/source/Target/ThreadPlanStack.cpp
lldb/source/Target/ThreadPlanStepOut.cpp
Index: lldb/source/Target/ThreadPlanStepOut.cpp
===================================================================
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -474,7 +474,6 @@
m_step_through_inline_plan_sp.get());
m_step_through_inline_plan_sp->SetPrivate(true);
- step_through_inline_plan_ptr->SetOkayToDiscard(true);
StreamString errors;
if (!step_through_inline_plan_ptr->ValidatePlan(&errors)) {
// FIXME: Log this failure.
Index: lldb/source/Target/ThreadPlanStack.cpp
===================================================================
--- lldb/source/Target/ThreadPlanStack.cpp
+++ lldb/source/Target/ThreadPlanStack.cpp
@@ -198,32 +198,30 @@
void ThreadPlanStack::DiscardConsultingMasterPlans() {
while (true) {
- int master_plan_idx;
- bool discard = true;
+ int discard_plan_idx;
// Find the first master plan, see if it wants discarding, and if yes
// discard up to it.
- for (master_plan_idx = m_plans.size() - 1; master_plan_idx >= 0;
- master_plan_idx--) {
- if (m_plans[master_plan_idx]->IsMasterPlan()) {
- discard = m_plans[master_plan_idx]->OkayToDiscard();
+ for (discard_plan_idx = m_plans.size() - 1; discard_plan_idx >= 0;
+ discard_plan_idx--) {
+ auto plan_sp = m_plans[discard_plan_idx];
+ if (plan_sp->OkayToDiscard()) {
break;
}
+ if (plan_sp->IsMasterPlan()) {
+ return;
+ }
}
- // If the master plan doesn't want to get discarded, then we're done.
- if (!discard)
- return;
-
// First pop all the dependent plans:
- for (int i = m_plans.size() - 1; i > master_plan_idx; i--) {
+ for (int i = m_plans.size() - 1; i > discard_plan_idx; i--) {
DiscardPlan();
}
// Now discard the master plan itself.
// The bottom-most plan never gets discarded. "OkayToDiscard" for it
// means discard it's dependent plans, but not it...
- if (master_plan_idx > 0) {
+ if (discard_plan_idx > 0) {
DiscardPlan();
}
}
Index: lldb/source/Target/ThreadPlanPython.cpp
===================================================================
--- lldb/source/Target/ThreadPlanPython.cpp
+++ lldb/source/Target/ThreadPlanPython.cpp
@@ -32,7 +32,6 @@
m_class_name(class_name), m_args_data(args_data), m_did_push(false),
m_stop_others(false) {
SetIsMasterPlan(true);
- SetOkayToDiscard(true);
SetPrivate(false);
}
Index: lldb/source/Target/ThreadPlanCallUserExpression.cpp
===================================================================
--- lldb/source/Target/ThreadPlanCallUserExpression.cpp
+++ lldb/source/Target/ThreadPlanCallUserExpression.cpp
@@ -40,7 +40,6 @@
// User expressions are generally "User generated" so we should set them up
// to stop when done.
SetIsMasterPlan(true);
- SetOkayToDiscard(false);
}
ThreadPlanCallUserExpression::~ThreadPlanCallUserExpression() {}
Index: lldb/source/Target/ThreadPlanCallFunction.cpp
===================================================================
--- lldb/source/Target/ThreadPlanCallFunction.cpp
+++ lldb/source/Target/ThreadPlanCallFunction.cpp
@@ -34,7 +34,6 @@
Thread &thread, ABI *&abi, lldb::addr_t &start_load_addr,
lldb::addr_t &function_load_addr) {
SetIsMasterPlan(true);
- SetOkayToDiscard(false);
SetPrivate(true);
ProcessSP process_sp(thread.GetProcess());
Index: lldb/source/Target/ThreadPlan.cpp
===================================================================
--- lldb/source/Target/ThreadPlan.cpp
+++ lldb/source/Target/ThreadPlan.cpp
@@ -26,7 +26,7 @@
m_takes_iteration_count(false), m_could_not_resolve_hw_bp(false),
m_thread(&thread), m_kind(kind), m_name(name), m_plan_complete_mutex(),
m_cached_plan_explains_stop(eLazyBoolCalculate), m_plan_complete(false),
- m_plan_private(false), m_okay_to_discard(true), m_is_master_plan(false),
+ m_plan_private(false), m_okay_to_discard(false), m_is_master_plan(false),
m_plan_succeeded(true) {
SetID(GetNextID());
}
@@ -153,9 +153,7 @@
void ThreadPlan::WillPop() {}
-bool ThreadPlan::OkayToDiscard() {
- return IsMasterPlan() ? m_okay_to_discard : true;
-}
+bool ThreadPlan::OkayToDiscard() { return m_okay_to_discard; }
lldb::StateType ThreadPlan::RunState() {
if (m_tracer_sp && m_tracer_sp->TracingEnabled())
Index: lldb/source/Target/Thread.cpp
===================================================================
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -845,11 +845,9 @@
current_plan->WillStop();
PopPlan();
} while ((current_plan = GetCurrentPlan()) != prev_plan_ptr);
- // 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->IsMasterPlan() && !plan_ptr->OkayToDiscard());
+ // Now, if the responsible plan was a master plan then we're done,
+ // otherwise we forward this to the next plan in the stack below.
+ done_processing_current_plan = plan_ptr->IsMasterPlan();
} else
done_processing_current_plan = true;
@@ -881,11 +879,10 @@
if (should_stop)
current_plan->WillStop();
- // If a Master Plan wants to stop, and wants to stick on the stack,
- // we let it. Otherwise, see if the plan's parent wants to stop.
+ // If a Master Plan wants to stop, we let it. Otherwise, see if the
+ // plan's parent wants to stop.
- if (should_stop && current_plan->IsMasterPlan() &&
- !current_plan->OkayToDiscard()) {
+ if (should_stop && current_plan->IsMasterPlan()) {
PopPlan();
break;
} else {
@@ -1903,7 +1900,6 @@
}
new_plan_sp->SetIsMasterPlan(true);
- new_plan_sp->SetOkayToDiscard(false);
// Why do we need to set the current thread by ID here???
process->GetThreadList().SetSelectedThreadByID(GetID());
@@ -1936,7 +1932,6 @@
}
new_plan_sp->SetIsMasterPlan(true);
- new_plan_sp->SetOkayToDiscard(false);
// Why do we need to set the current thread by ID here???
process->GetThreadList().SetSelectedThreadByID(GetID());
@@ -1960,7 +1955,6 @@
eVoteYes, eVoteNoOpinion, 0, error));
new_plan_sp->SetIsMasterPlan(true);
- new_plan_sp->SetOkayToDiscard(false);
// Why do we need to set the current thread by ID here???
process->GetThreadList().SetSelectedThreadByID(GetID());
Index: lldb/source/Target/StopInfo.cpp
===================================================================
--- lldb/source/Target/StopInfo.cpp
+++ lldb/source/Target/StopInfo.cpp
@@ -728,7 +728,6 @@
new_plan_status));
if (new_plan_sp && new_plan_status.Success()) {
new_plan_sp->SetIsMasterPlan(true);
- new_plan_sp->SetOkayToDiscard(false);
new_plan_sp->SetPrivate(true);
}
process_sp->GetThreadList().SetSelectedThreadByID(
Index: lldb/source/Target/Process.cpp
===================================================================
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -4641,7 +4641,6 @@
// should stop, which may give the wrong answer.
thread_plan_sp->SetIsMasterPlan(true);
- thread_plan_sp->SetOkayToDiscard(false);
// If we are running some utility expression for LLDB, we now have to mark
// this in the ProcesModID of this process. This RAII takes care of marking
Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp
===================================================================
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp
@@ -68,6 +68,7 @@
GetThread().CalculateExecutionContext(exc_ctx);
m_func_sp = m_impl_function->GetThreadPlanToCallFunction(
exc_ctx, m_args_addr, options, diagnostics);
+ m_func_sp->SetIsMasterPlan(false);
m_func_sp->SetOkayToDiscard(true);
PushPlan(m_func_sp);
}
Index: lldb/source/Expression/FunctionCaller.cpp
===================================================================
--- lldb/source/Expression/FunctionCaller.cpp
+++ lldb/source/Expression/FunctionCaller.cpp
@@ -255,7 +255,6 @@
lldb::ThreadPlanSP new_plan_sp(new ThreadPlanCallFunction(
*thread, wrapper_address, CompilerType(), args, options));
new_plan_sp->SetIsMasterPlan(true);
- new_plan_sp->SetOkayToDiscard(false);
return new_plan_sp;
}
Index: lldb/source/Commands/CommandObjectThread.cpp
===================================================================
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -547,7 +547,6 @@
if (new_plan_sp) {
new_plan_sp->SetIsMasterPlan(true);
- new_plan_sp->SetOkayToDiscard(false);
if (m_options.m_step_count > 1) {
if (!new_plan_sp->SetIterationCount(m_options.m_step_count)) {
@@ -1059,7 +1058,6 @@
// user (stepping around the breakpoint) and then a "continue" will
// resume the original plan.
new_plan_sp->SetIsMasterPlan(true);
- new_plan_sp->SetOkayToDiscard(false);
} else {
result.SetError(new_plan_status);
result.SetStatus(eReturnStatusFailed);
Index: lldb/source/API/SBThread.cpp
===================================================================
--- lldb/source/API/SBThread.cpp
+++ lldb/source/API/SBThread.cpp
@@ -501,7 +501,6 @@
// plans executed, and then a "continue" will resume the plan.
if (new_plan != nullptr) {
new_plan->SetIsMasterPlan(true);
- new_plan->SetOkayToDiscard(false);
}
// Why do we need to set the current thread by ID here???
Index: lldb/include/lldb/Target/ThreadPlanBase.h
===================================================================
--- lldb/include/lldb/Target/ThreadPlanBase.h
+++ lldb/include/lldb/Target/ThreadPlanBase.h
@@ -33,8 +33,6 @@
lldb::StateType GetPlanRunState() override;
bool MischiefManaged() override;
- bool OkayToDiscard() override { return false; }
-
bool IsBasePlan() override { return true; }
protected:
Index: lldb/include/lldb/Target/ThreadPlan.h
===================================================================
--- lldb/include/lldb/Target/ThreadPlan.h
+++ lldb/include/lldb/Target/ThreadPlan.h
@@ -150,16 +150,16 @@
// stack up to the point of the plan that understood the stop reason.
// However, if a plan wishes to stay on the stack after an event it didn't
// directly handle it can designate itself a "Master" plan by responding true
-// to IsMasterPlan, and then if it wants not to be discarded, it can return
-// false to OkayToDiscard, and it and all its dependent plans will be
-// preserved when we resume execution.
+// to IsMasterPlan, and it and all its dependent plans will be preserved when
+// we resume execution. If instead a plan wants to be discarded, it can
+// return true from OkayToDiscard.
//
// The other effect of being a master plan is that when the Master plan is
-// done , if it has set "OkayToDiscard" to false, then it will be popped &
-// execution will stop and return to the user. Remember that if OkayToDiscard
-// is false, the plan will be popped and control will be given to the next
-// plan above it on the stack So setting OkayToDiscard to false means the
-// user will regain control when the MasterPlan is completed.
+// done, then it will be popped & execution will stop and return to the user.
+// Remember that if IsMasterPlan is false, the plan will be popped and control
+// will be given to the next plan above it on the stack. So setting
+// IsMasterPlan to true means the user will regain control when the MasterPlan
+// is completed.
//
// Between these two controls this allows things like: a
// MasterPlan/DontDiscard Step Over to hit a breakpoint, stop and return
@@ -168,16 +168,6 @@
// continue to step in/step over/etc, and finally when they continue, they
// will finish up the Step Over.
//
-// FIXME: MasterPlan & OkayToDiscard aren't really orthogonal. MasterPlan
-// designation means that this plan controls it's fate and the fate of plans
-// below it. OkayToDiscard tells whether the MasterPlan wants to stay on the
-// stack. I originally thought "MasterPlan-ness" would need to be a fixed
-// characteristic of a ThreadPlan, in which case you needed the extra control.
-// But that doesn't seem to be true. So we should be able to convert to only
-// MasterPlan status to mean the current "MasterPlan/DontDiscard". Then no
-// plans would be MasterPlans by default, and you would set the ones you
-// wanted to be "user level" in this way.
-//
//
// Actually Stopping:
//
@@ -385,7 +375,7 @@
return old_value;
}
- virtual bool OkayToDiscard();
+ bool OkayToDiscard();
void SetOkayToDiscard(bool value) { m_okay_to_discard = value; }
@@ -569,8 +559,6 @@
bool IsBasePlan() override { return true; }
- bool OkayToDiscard() override { return false; }
-
const Status &GetStatus() { return m_status; }
protected:
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
