jimingham wrote:

The ThreadPlans themselves don't lock access to their state as it stands now.  
That's not really a problem because ThreadPlans only mutate their state in the 
course of the ThreadList::ShouldStop process, which happens before the new 
ThreadPlanStack is accessible to other parts of lldb - since we aren't formally 
stopped yet.  By the time general code is accessing them, they are just 
reporting already computed data.

Note if concurrent access to ThreadPlans were a problem, this change wouldn't 
substantially affect that.  We don't delegate the tasks that do the majority of 
the work - like `ThreadPlan::ShouldStop` - to the ThreadPlanStack, we find 
elements of the stack, and query them directly.  

I'm not sure how to express this more formally.  We'd have to have a couple of 
domains for handling ThreadPlans, one that happens serially before anything but 
the ShouldStop mechanism sees the newly stopped Thread, and one that happens 
when generic code is asking questions.  The former domain is a "writer domain" 
and the latter a "reader domain".  

This is currently enforced informally because the "writer domain" functions 
aren't useful once you've stopped, and the things the thread plans store don't 
get computed lazily.

Also, all the changes where I substituted the recursive_mutex with the Writer 
lock are protected the same way they were before, so we only need to look at 
the ones that have the Reader lock. The only ones of those that might do work 
in the ThreadPlans are:

ThreadPlanStack::GetReturnValueObject
ThreadPlanStack::GetExpressionVariable
ThreadPlanStack::ClearThreadCache

In practice those three shouldn't be a problem.  In all the extant uses, the 
first two aren't actually modifying, they always read out a value that was 
cached previously.  So those are really reader operations.  ClearThreadCache 
should only happen as part of preparing for a stop, before we've made any 
Threads (and so any ThreadPlanStack's) available to general-purpose code.  
That's an example of a call that really only makes sense as part of ShouldStop.

https://github.com/llvm/llvm-project/pull/116438
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to