[Lldb-commits] [lldb] [lldb] Guard SBFrame/SBThread methods against running processes (PR #152020)

[Felipe de Azevedo Piovezan via lldb-commits]

felipepiovezan wrote:

I think I've addressed the vast majority of comments here.

@jimingham I may have added "error messages" at a finer granularity than what you were expecting in the constructor. Were you thinking of limiting it to _only_ when we had a process and it was not stopped? If so, I can remove some of the messages I added.

> It looks like this PR is updating all call sites of this ctor. We're always passing a unique_lock and the StopLocker. I understand they do different things and we need both, but if we always need both, should we abstract this away a bit more. Like a UniqueStopLocker or something, which also holds onto the unique_lock RAII style?

@JDevlieghere I can definitely do that, but I think this is adding a new layer of abstraction without a lot of value -- it would be yet another thing for a reader to go "wait what is this UniqueStopLocker", whereas the current spelling makes it evident that we have an API lock and a process run lock.

It would also be nice to look at the design of this API, which I think aligns with your concern. The signature if the constructor  paints a picture of how complicated this object is: it takes an opaque pointer, it takes two locks, we _wait_ on one of them in some scenarios, we _try to wait_ on the other on some other scenarios, it looks like the constructor may fail, but in fact it is "ok" for it to be partially constructed. In contrast, many users of this class -- all the ones in SBThread/SBFrame -- don't care about those partial states, they need the full object. IMO all of these things indicate we should explore a broader redesign.

Also, as Jim mentioned, we will need to do a bigger revamp of the call sites, to clean up some redundant pointer checks. This is addressing a separate problem, so it would be a follow-up PR.

https://github.com/llvm/llvm-project/pull/152020