[lldb] Decouple IsMasterPlan and OkayToDiscard (NFC)

[kastiglione]

Summary

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

[kastiglione]

Restore a comment.

[kastiglione]

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp

72

This is the only remaining SetOkayToDiscard(true) call.

[kastiglione]

friendly ping @jingham

[jingham]

I don't understand how DiscardPlansConsultingMasterPlan in this patch works.

With your changes, every plan but the ObjC trampoline finder returns false from OkayToDiscard. So if you are trying to discard plans up to the innermost master plan, you start from the youngest plan asking it if is "OkayToDiscard". Since no plans return false, the iteration will go all the way to the Base thread plan regardless of whether there were intervening master plans, which isn't right. Why didn't you ask IsMasterPlan in that loop rather than OkayToDiscard?

Was there a problem with just saying "Everything but master plans can be discarded when we are discarding plans." I'm not sure why the ObjC Trampoline step through thread plan needs to be a Master Plan. It seems to me it's just another subsidiary plan to a step in plan or something like. Maybe it would be better to eliminate the OkayToDiscard altogether, and figure out a better way to handle the ObjC trampoline, if that actually needs doing.

lldb/include/lldb/Target/ThreadPlan.h

 //  per stop.
 //
 //  Master plans:
 //
 //  In the normal case, when we decide to stop, we will  collapse the plan
 //  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
 //  control to the user, but then when the user continues, the step out
 //  succeeds.  Even more tricky, when the breakpoint is hit, the user can
 //  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:
 //
 //  If a plan says responds "true" to ShouldStop, then it is asked if it's job
 //  is complete by calling MischiefManaged.  If that returns true, the plan is
 //  popped from the plan stack and added to the Completed Plan Stack.  Then the
 //  next plan in the stack is asked if it ShouldStop, and  it returns "true",
 //  it is asked if it is done, and if yes popped, and so on till we reach a
public:
   bool IsMasterPlan() { return m_is_master_plan; }
 
   bool SetIsMasterPlan(bool value) {
     bool old_value = m_is_master_plan;
     m_is_master_plan = value;
     return old_value;
   }
 
-  virtual bool OkayToDiscard();
+  bool OkayToDiscard();
 
   void SetOkayToDiscard(bool value) { m_okay_to_discard = value; }
 
   // The base class MischiefManaged does some cleanup - so you have to call it
   // in your MischiefManaged derived class.
   virtual bool MischiefManaged();
 
   virtual void ThreadDestroyed() {
public:
   bool ShouldStop(Event *event_ptr) override;
 
   bool MischiefManaged() override;
 
   void WillStop() override;
 
   bool IsBasePlan() override { return true; }
 
-  bool OkayToDiscard() override { return false; }
-
   const Status &GetStatus() { return m_status; }
 
 protected:
   bool DoPlanExplainsStop(Event *event_ptr) override;
 
   lldb::StateType GetPlanRunState() override;
 
   ThreadPlanNull(const ThreadPlanNull &) = delete;
   const ThreadPlanNull &operator=(const ThreadPlanNull &) = delete;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_TARGET_THREADPLAN_H

lldb/include/lldb/Target/ThreadPlanBase.h

public:
   void GetDescription(Stream *s, lldb::DescriptionLevel level) override;
   bool ValidatePlan(Stream *error) override;
   bool ShouldStop(Event *event_ptr) override;
   Vote ShouldReportStop(Event *event_ptr) override;
   bool StopOthers() override;
   lldb::StateType GetPlanRunState() override;
   bool MischiefManaged() override;
 
-  bool OkayToDiscard() override { return false; }
-
   bool IsBasePlan() override { return true; }
 
 protected:
   bool DoWillResume(lldb::StateType resume_state, bool current_plan) override;
   bool DoPlanExplainsStop(Event *event_ptr) override;
   ThreadPlanBase(Thread &thread);
 
 private:

lldb/source/API/SBThread.cpp

if (!thread) {
     sb_error.SetErrorString("No thread in SBThread::ResumeNewPlan");
     return sb_error;
   }
 
   // User level plans should be Master Plans so they can be interrupted, other
   // 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???
   process->GetThreadList().SetSelectedThreadByID(thread->GetID());
 
   if (process->GetTarget().GetDebugger().GetAsyncExecution())
     sb_error.ref() = process->Resume();
   else

lldb/source/Commands/CommandObjectThread.cpp

bool DoExecute(Args &command, CommandReturnObject &result) override {
     }
 
     // If we got a new plan, then set it to be a master plan (User level Plans
     // should be master plans so that they can be interruptible).  Then resume
     // the process.
 
     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)) {
           result.AppendWarning(
               "step operation does not support iteration count.");
         }
       }
 
if (process == nullptr) {
             abort_other_plans, &address_list.front(), address_list.size(),
             m_options.m_stop_others, m_options.m_frame_idx, new_plan_status);
         if (new_plan_sp) {
           // User level plans should be master plans so they can be interrupted
           // (e.g. by hitting a breakpoint) and other plans executed by the
           // 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);
           return false;
         }
       } else {
         result.AppendErrorWithFormat(
             "Frame index %u of thread %u has no debug information.\n",

lldb/source/Expression/FunctionCaller.cpp

lldb::ThreadPlanSP FunctionCaller::GetThreadPlanToCallFunction(
 
   Address wrapper_address(m_jit_start_addr);
 
   lldb::addr_t args = {args_addr};
 
   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;
 }
 
 bool FunctionCaller::FetchFunctionResults(ExecutionContext &exe_ctx,
                                           lldb::addr_t args_addr,
                                           Value &ret_value) {
   // Read the return value - it is the last field in the struct:
   // FIXME: How does clang tell us there's no return value?  We need to handle

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp

if (!m_func_sp) {
     ExecutionContext exc_ctx;
     EvaluateExpressionOptions options;
     options.SetUnwindOnError(true);
     options.SetIgnoreBreakpoints(true);
     options.SetStopOthers(false);
     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);
   }
   return true;
 }
 
 bool AppleThreadPlanStepThroughObjCTrampoline::
     PreResumeInitializeFunctionCaller(void *void_myself) {
   AppleThreadPlanStepThroughObjCTrampoline *myself =

lldb/source/Target/Process.cpp

Process::RunThreadPlan(ExecutionContext &exe_ctx,
 
   thread_plan_sp->SetPrivate(false);
 
   // The plans run with RunThreadPlan also need to be terminal master plans or
   // when they are done we will end up asking the plan above us whether we
   // 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
   // and reverting the mark it once we are done running the expression.
   UtilityFunctionScope util_scope(options.IsForUtilityExpr() ? this : nullptr);
 
   if (m_private_state.GetValue() != eStateStopped) {
     diagnostic_manager.PutString(

lldb/source/Target/StopInfo.cpp

if (thread_sp) {
                 ThreadPlanSP new_plan_sp(
                     thread_sp->QueueThreadPlanForStepSingleInstruction(
                         false, // step-over
                         false, // abort_other_plans
                         true,  // stop_other_threads
                         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(
                     thread_sp->GetID());
                 process_sp->ResumeSynchronous(nullptr);
                 process_sp->GetThreadList().SetSelectedThreadByID(
                     thread_sp->GetID());
                 thread_sp->SetStopInfo(stored_stop_info_sp);

lldb/source/Target/Thread.cpp

if (current_plan->TracerExplainsStop()) {
             // explains the stop.
             ThreadPlan *prev_plan_ptr = GetPreviousPlan(plan_ptr);
 
             do {
               if (should_stop)
                 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;
 
           break;
         }
       }
     }
   }
if (current_plan->IsBasePlan()) {
 
         should_stop = current_plan->ShouldStop(event_ptr);
         LLDB_LOGF(log, "Plan %s should stop: %d.", current_plan->GetName(),
                   should_stop);
         if (current_plan->MischiefManaged()) {
           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 {
             PopPlan();
 
             current_plan = GetCurrentPlan();
             if (current_plan == nullptr) {
               break;
if (source_step && frame_sp && frame_sp->HasDebugInformation()) {
           step_in_avoids_code_without_debug_info,
           step_out_avoids_code_without_debug_info);
     } else {
       new_plan_sp = QueueThreadPlanForStepSingleInstruction(
           false, abort_other_plans, run_mode, 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());
     error = process->Resume();
   } else {
     error.SetErrorString("process not stopped");
   }
   return error;
if (source_step && frame_sp && frame_sp->HasDebugInformation()) {
           abort_other_plans, sc.line_entry, sc, run_mode, error,
           step_out_avoids_code_without_debug_info);
     } else {
       new_plan_sp = QueueThreadPlanForStepSingleInstruction(
           true, abort_other_plans, run_mode, 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());
     error = process->Resume();
   } else {
     error.SetErrorString("process not stopped");
   }
   return error;
 }
 
 Status Thread::StepOut() {
   Status error;
   Process *process = GetProcess().get();
   if (StateIsStoppedState(process->GetState(), true)) {
     const bool first_instruction = false;
     const bool stop_other_threads = false;
     const bool abort_other_plans = false;
 
     ThreadPlanSP new_plan_sp(QueueThreadPlanForStepOut(
         abort_other_plans, nullptr, first_instruction, stop_other_threads,
         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());
     error = process->Resume();
   } else {
     error.SetErrorString("process not stopped");
   }
   return error;

lldb/source/Target/ThreadPlan.cpp

 // ThreadPlan constructor
 ThreadPlan::ThreadPlan(ThreadPlanKind kind, const char *name, Thread &thread,
                        Vote stop_vote, Vote run_vote)
     : m_process(*thread.GetProcess().get()), m_tid(thread.GetID()),
       m_stop_vote(stop_vote), m_run_vote(run_vote),
       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());
 }
 
 // Destructor
 ThreadPlan::~ThreadPlan() = default;
 
 Target &ThreadPlan::GetTarget() { return m_process.GetTarget(); }
lldb::user_id_t ThreadPlan::GetNextID() {
   static uint32_t g_nextPlanID = 0;
   return ++g_nextPlanID;
 }
 
 void ThreadPlan::DidPush() {}
 
 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())
     return eStateStepping;
   else
     return GetPlanRunState();
 }
 

lldb/source/Target/ThreadPlanCallFunction.cpp

 using namespace lldb;
 using namespace lldb_private;
 
 // ThreadPlanCallFunction: Plan to call a single function
 bool ThreadPlanCallFunction::ConstructorSetup(
     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());
   if (!process_sp)
     return false;
 
   abi = process_sp->GetABI().get();
 

lldb/source/Target/ThreadPlanCallUserExpression.cpp

ThreadPlanCallUserExpression::ThreadPlanCallUserExpression(
     Thread &thread, Address &function, llvm::ArrayRef<lldb::addr_t> args,
     const EvaluateExpressionOptions &options,
     lldb::UserExpressionSP &user_expression_sp)
     : ThreadPlanCallFunction(thread, function, CompilerType(), args, options),
       m_user_expression_sp(user_expression_sp) {
   // User expressions are generally "User generated" so we should set them up
   // to stop when done.
   SetIsMasterPlan(true);
-  SetOkayToDiscard(false);
 }
 
 ThreadPlanCallUserExpression::~ThreadPlanCallUserExpression() {}
 
 void ThreadPlanCallUserExpression::GetDescription(
     Stream *s, lldb::DescriptionLevel level) {
   if (level == eDescriptionLevelBrief)
     s->Printf("User Expression thread plan");

lldb/source/Target/ThreadPlanPython.cpp

 
 ThreadPlanPython::ThreadPlanPython(Thread &thread, const char *class_name,
                                    StructuredDataImpl *args_data)
     : ThreadPlan(ThreadPlan::eKindPython, "Python based Thread Plan", thread,
                  eVoteNoOpinion, eVoteNoOpinion),
       m_class_name(class_name), m_args_data(args_data), m_did_push(false),
       m_stop_others(false) {
   SetIsMasterPlan(true);
-  SetOkayToDiscard(true);
   SetPrivate(false);
 }
 
 ThreadPlanPython::~ThreadPlanPython() {
   // FIXME, do I need to decrement the ref count on this implementation object
   // to make it go away?
 }
 

lldb/source/Target/ThreadPlanStack.cpp

void ThreadPlanStack::DiscardAllPlans() {
   for (int i = stack_size - 1; i > 0; i--) {
     DiscardPlan();
   }
   return;
 }
 
 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 the master plan doesn't want to get discarded, then we're done.
-    if (!discard)
+      if (plan_sp->IsMasterPlan()) {
         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();
     }
   }
 }
 
 lldb::ThreadPlanSP ThreadPlanStack::GetCurrentPlan() const {
   assert(m_plans.size() != 0 && "There will always be a base plan.");
   return m_plans.back();

lldb/source/Target/ThreadPlanStepOut.cpp

if (inlined_block) {
         m_step_through_inline_plan_sp =
             std::make_shared<ThreadPlanStepOverRange>(
                 thread, inline_range, inlined_sc, run_mode, avoid_no_debug);
         ThreadPlanStepOverRange *step_through_inline_plan_ptr =
             static_cast<ThreadPlanStepOverRange *>(
                 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.
           delete step_through_inline_plan_ptr;
           return false;
         }
 
         for (size_t i = 1; i < num_ranges; i++) {