[Lldb-commits] [PATCH] D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread

Thu Mar 5 15:53:48 PST 2020

jingham created this revision.
jingham added reviewers: friss, clayborg.
Herald added subscribers: lldb-commits, danielkiss, kristof.beyls.
Herald added a project: LLDB.

This is the first part of a multi-part commit.  This change makes the ThreadPlans use the process & TID to specify which thread they belong to, rather than holding onto the Thread * directly.  It doesn't change any behavior, it just means ThreadPlans have to get their thread from the ThreadPlan::GetThread() API, and OTOH can access their process and TID directly.

This point of this multi-part commit will be to make it possible to preserve the stepping intentions for a thread in the target, even if the target doesn't report all threads on every stop.  This is particularly a problem for targets that provide their threads using an OSPlugin thread provider.  The Darwin kernel uses this extensively, and they found that if they reported all the current threads on every stop, it made stepping unusably slow.  So they only report the "on core" threads when they stop.

That made stepping perform acceptably, but caused stepping to behave badly.   For instance, suppose you were next-ing Thread A, and the code had stepped into a function and so needed to step out.  So lldb would set a breakpoint at the return address of the function, then continue.  If you are unlucky, Thread B might hit a breakpoint before thread A completed its step out, and furthermore when Thread B stopped, Thread A didn't happen to be on another core.  Then lldb would think Thread A has exited, and would remove the step out breakpoint.  That meant when you continued again, your step-over would have turned into a continue.

To fix this problem, I am going to move the ThreadPlan stacks into a separate class, and then have the Process hold onto the map of TID -> ThreadPlanStacks.  Then if a thread goes away, we can leave its entry in the map, and if the thread comes back to life, we will know what we were doing with it.

Couple of other random observations:

- I tried preserving the whole Thread object for a while, but the logic for merging the old & new thread lists is pretty tricky and intervening at that point made this tricky code even harder to understand.  Plus this was preserving a lot of unneeded information.  Just keeping the ThreadPlanStack off to one side was much cleaner.

- Leaving actually obsolete thread plan stacks around does have a cost.  For instance, if the thread in the example above had really gone away, then we would leave a step-out breakpoint stranded.  It wouldn't do any harm, since it is thread specific for a non-existent thread.  But it would slow down execution.

In the patches in this sequence, I will only preserve vanished thread's ThreadPlanStacks if there is an OperatingSystem plugin.  So it won't affect the current ProcessGDBRemote clients, since ProcessGDBRemote always gets all "real" threads on every stop at present.

In the future, we could change ProcessGDBRemote to NOT gather all threads on stop, but just read the threads that stopped "for a reason".  We would have to encode that in the stop packet somehow.

That could make stepping more efficient.  If lldb was going to continue immediately (it was in the middle of stepping or whatever) it would be great to only have to reconstruct one or two threads.  In the case of ProcessGDBRemote, we could even refine this by asking about the existence of all the threads that had non-trivial thread plans in flight, along with the stopped-for-a-reason threads.  That would allow us to reap any of the ThreadPlanStacks that had any effect on the process (setting breakpoints, etc...) without having to enumerate all threads.

Future:

The sequence of patches should be:

1. This patch deals with how ThreadPlans know what thread they belong to.
2. Do the same change to ThreadPlanTracers
3. Introduce the ThreadPlanStacks, and move the plans there, but keep them in the Thread
4. Move the ThreadPlanStacks into the Process, and manage detaching & reattaching them from Threads as they come and go.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75711

Files:
  lldb/include/lldb/Target/ThreadPlan.h
  lldb/include/lldb/Target/ThreadPlanPython.h
  lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp
  lldb/source/Target/ThreadPlan.cpp
  lldb/source/Target/ThreadPlanBase.cpp
  lldb/source/Target/ThreadPlanCallFunction.cpp
  lldb/source/Target/ThreadPlanCallFunctionUsingABI.cpp
  lldb/source/Target/ThreadPlanCallUserExpression.cpp
  lldb/source/Target/ThreadPlanPython.cpp
  lldb/source/Target/ThreadPlanRunToAddress.cpp
  lldb/source/Target/ThreadPlanStepInRange.cpp
  lldb/source/Target/ThreadPlanStepInstruction.cpp
  lldb/source/Target/ThreadPlanStepOut.cpp
  lldb/source/Target/ThreadPlanStepOverBreakpoint.cpp
  lldb/source/Target/ThreadPlanStepOverRange.cpp
  lldb/source/Target/ThreadPlanStepRange.cpp
  lldb/source/Target/ThreadPlanStepThrough.cpp
  lldb/source/Target/ThreadPlanStepUntil.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D75711.248613.patch
Type: text/x-patch
Size: 74451 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20200305/86edab7f/attachment-0001.bin>