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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Another point to make clear here.  You might wonder whether ThreadPlan::GetThread should return an Optional<Thread> or something like that, since there might not be a Thread associated with the ThreadPlan.

I tried that out but it made the code really ugly.  When I thought about it a little more, I realized that besides the destructor, and Description methods all the other methods are only callable when the ThreadPlanStack is attached to a thread, since they are all only called as part of the "ShouldStop" and "WillResume" negotiation, which always work on Thread objects in the ThreadList.  So nobody has any business calling any of these other methods when detached from a Thread.  And conversely, it's pointless to check whether you have a thread in these methods, you always will...

It doesn't cause practical problems that you can't assume anything about the state of your Thread in these two methods.  When you are being destroyed you can't assume anything about the state of the underlying thread, it's target side may very well have gone away, so the destructors don't actually use the thread.  And actually all the Description methods really wanted the TID, or were using the thread to get to the process.  So I didn't have to change either's behavior.

In the patch that detaches the ThreadPlanStacks from the Thread, I'll assert if you call GetThread and there isn't one.  But that won't 100% prevent the problem, since in most scenarios there will always be a thread in these situations.

I was planning on adding the assert and then documenting this restriction, and leave it at that.  That's in part because I don't know how to say ThreadPlan::GetThread can be called from anywhere in ThreadPlan BUT ThreadPlan::GetDescription and it's descendants, and ThreadPlan::~ThreadPlan or its descendants.  But if anyone knows a clever way to do this, I'd happily use that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75711