This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Refine ThreadPlan::ShouldAutoContinue
ClosedPublic

Authored by kastiglione on Feb 19 2021, 12:09 PM.

Details

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.

Diff Detail

Event Timeline

kastiglione requested review of this revision.Feb 19 2021, 12:09 PM
kastiglione created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2021, 12:09 PM
kastiglione added inline comments.Feb 19 2021, 12:18 PM
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.

kastiglione edited the summary of this revision. (Show Details)Feb 19 2021, 12:20 PM

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".

jingham accepted this revision.Feb 19 2021, 4:28 PM
This revision is now accepted and ready to land.Feb 19 2021, 4:28 PM

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 added inline comments.Feb 19 2021, 5:06 PM
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.

This revision was landed with ongoing or failed builds.Feb 20 2021, 5:25 PM
This revision was automatically updated to reflect the committed changes.