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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to