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

Thu Mar 12 17:23:27 PDT 2020

jingham added a comment.

In D75711#1920642 <https://reviews.llvm.org/D75711#1920642>, @clayborg wrote:

> Everything looks good, just a question in inlined comment about having a thread plan hold onto a pointer to a thread. Seems dangerous

The way the ThreadPlanStacks will get used, every time a process stops, lldb will build up a new thread list like it does already, then we update the collection of thread plan stacks, passing in the new thread list.  That will grab the TID for each new thread and look it up in the map of stacks (which is indexed by TID).  Each time we find a TID from the new thread list that's already in the map, we set the Thread * in the stack map to the one we got the TID from.  Then for all the TID's that aren't in the new thread list we either discard it (if we're not holding onto plans for vanished threads) or we set its Thread * to nullptr.  So the Thread * is just acting as a cache so you don't need to keep looking up the TID every time you need to get the Thread * for a ThreadPlan between one stop and another.

Unless there's some way that a thread that we saw when we stopped can go away before the next resume without the ThreadList being notified so it can re-sync, this mechanism is safe, and I can't think of any mechanism of that sort in lldb.

If you're still worried about this, I think the safe solution is to remove the Thread * and look up the TID in the Process every time you need the Thread *.  Using a ThreadWP doesn't really express what you want here, because the Thread may have been discarded but it's SP is still alive because it's in some list we haven't gotten rid of or something like that.  So WP.lock() working doesn't really mean the thread is still alive in the process, but TID in ThreadList does.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75711/new/

https://reviews.llvm.org/D75711