[lldb] Refine ThreadPlan::ShouldAutoContinue

[kastiglione]

Summary

Adjust ShouldAutoContinue to be available to any thread plan previous to the plan that
explains a stop, not limited to the parent to the plan that explains the stop.

Before this change, Thread::ShouldStop did the following:

1. find the plan that explains the stop
2. if it's not a master plan, continue processing previous (aka parent) plans
3. first, call ShouldAutoContinue on the immediate parent of the explaining plan
4. then loop over previous plans, calling ShouldStop and MischiefManaged

Of note, the iteration in step 4 does not call ShouldAutoContinue, so again only the
plan just prior to the explaining plan is given the opportunity to override whether to
continue or stop.

This commit changes the loop to also call ShouldAutoContinue, giving each plan the opportunity
to override ShouldStop of previous plans.

Why? This allows a plan to do the following:

1. mark itself done and be popped off the stack
2. allow parent plans to finish up, to also be popped off the stack
3. and finally, have the thread continue, not stop

This is useful for stepping into async functions. A plan will would step far enough
enough to set a breakpoint on the async target, and then use ShouldAutoContinue to
unwind the necessary stepping, and then have the calling thread continue.

[kastiglione]

lldb/include/lldb/Target/ThreadPlan.h

354–359

Happy to iterate on this. I wanted to take the opportunity to explain this, since by name it may seem unclear how exactly it relates to ShouldStop.

lldb/source/Target/Thread.cpp

832

This log was previously incorrect about which plan explains the stop. I moved it here and fixed it.

884

This is a drive by change. It was called from both branches of an if/else, so I lifted it out to be clear that it happens either way.

[jingham]

This seems like a fine improvement. One little nit, I would ask the current plan ShouldAutoContinue before popping it. Popping the plan does call WillPop, so the plan does have a chance to react to being popped, and you don't know what it will do. So to be on the safe side it would be better to ask questions of it as an active plan before you pop it.

lldb/include/lldb/Target/ThreadPlan.h

354–359

The use of "previous plans" is a little confusing, since it not clear "previous to what". To me the obvious meaning is plans previously dealt with in this negotiation, which is not what you mean, and then sounds really odd when you hit the "subsequently". I don't think you need the "previous" at all. You could just say "subsequently processed plans".

[kastiglione]

Good call on calling ShouldAutoContinue before Pop.

lldb/include/lldb/Target/ThreadPlan.h

354–359

Thanks I'll update it.

I agree that "previous" can be ambiguous, especially in the context of Thread::ShouldStop. What do you think of renaming GetPreviousPlan to GetParentPlan (and related variables and documentation too)? I can do this in a follow up if you agree.

[jingham]

lldb/include/lldb/Target/ThreadPlan.h

354–359

Changing away from previous would be great!

Parent isn't quite right anyway because there's no necessary relationship between plans on the stack. You could stop at a breakpoint while using one plan, and then do a "step" and the plan that was working before the breakpoint has no knowledge or relationship to the new plan.

I've been trying recently to be consistent about using Older and Younger for things pushed onto a stack. That seems clear to me since you are describing the process of pushing things onto a stack, so the order in the stack is governed by the time at which they were pushed. I've been trying to speak of stack frames this way as well.

lldb/include/lldb/Target/ThreadPlan.h

public:
   }
 
   lldb::StateType RunState();
 
   bool PlanExplainsStop(Event *event_ptr);
 
   virtual bool ShouldStop(Event *event_ptr) = 0;
 
+  /// Returns whether this thread plan overrides the `ShouldStop` of previous
+  /// plans.
+  ///
+  /// When processing the thread plan stack, this function gives plans the
+  /// ability to continue - even when previous plans subsequently return false
+  /// from `ShouldStop`. \see Thread::ShouldStop
   virtual bool ShouldAutoContinue(Event *event_ptr) { return false; }
 
   // Whether a "stop class" event should be reported to the "outside world".
   // In general if a thread plan is active, events should not be reported.
 
   virtual Vote ShouldReportStop(Event *event_ptr);
 
   Vote ShouldReportRun(Event *event_ptr);

lldb/source/Target/Thread.cpp

if (current_plan->TracerExplainsStop()) {
       done_processing_current_plan = true;
       should_stop = false;
     } else {
       // 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());
+
           should_stop = plan_ptr->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()) {
             // We're going to pop the plans up to and including the plan that
if (current_plan->TracerExplainsStop()) {
 
           break;
         }
       }
     }
   }
 
   if (!done_processing_current_plan) {
-    bool over_ride_stop = current_plan->ShouldAutoContinue(event_ptr);
-
-    LLDB_LOGF(log, "Plan %s explains stop, auto-continue %i.",
-              current_plan->GetName(), over_ride_stop);
+    bool override_stop = false;
 
     // We're starting from the base plan, so just let it decide;
     if (current_plan->IsBasePlan()) {
       should_stop = current_plan->ShouldStop(event_ptr);
       LLDB_LOGF(log, "Base plan says should stop: %i.", should_stop);
     } else {
       // Otherwise, don't let the base plan override what the other plans say
       // to do, since presumably if there were other plans they would know what
       // to do...
       while (true) {
         if (current_plan->IsBasePlan())
           break;
 
         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();
 
+          PopPlan();
+
+          if (current_plan->ShouldAutoContinue(event_ptr)) {
+            override_stop = true;
+            LLDB_LOGF(log, "Plan %s auto-continue: true.",
+                      current_plan->GetName());
+          }
+
           // 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()) {
-            PopPlan();
             break;
-          } else {
-            PopPlan();
+          }
 
-            current_plan = GetCurrentPlan();
-            if (current_plan == nullptr) {
-              break;
-            }
-          }
         } else {
           break;
         }
       }
     }
 
-    if (over_ride_stop)
+    if (override_stop)
       should_stop = false;
   }
 
   // One other potential problem is that we set up a master plan, then stop in
   // before it is complete - for instance by hitting a breakpoint during a
   // step-over - then do some step/finish/etc operations that wind up past the
   // end point condition of the initial plan.  We don't want to strand the
   // original plan on the stack, This code clears stale plans off the stack.