[Lldb-commits] [lldb] [lldb] Fix circular dependency and deadlock in scripted frame providers (PR #187411)

[Jonas Devlieghere via lldb-commits]

JDevlieghere wrote:

I understand how this PR addresses the issue being described, but I still have the feeling this is working around a fundamental limitation of the current stack frame provider design. I would like to understand why we can't design this problem away and instead must work around it.

> The problem here is that you often don't have a choice of calling GetStackFrameList when you are doing work in the stack frame provider. Any call that you make that ends up trying to fill out the current execution context will do that for you. The difficulty here would trying to limit the lldb API's you use to ones that would not themselves call GetStackFrameList somewhere along the line, and I don't think that's a difficulty that we want to impose on people writing the code to implement a stack frame provider.

That answers my original question. It's not the implementation of the stack frame provider calling GetStackFrameList directly. because they could use the stack frame list that's passed in. The problem is using any API that indirectly calls GetStackFrameList. 

> If you were directly in the code implementing the stack frame list provider, you should know that you want to ask question of the stack frame list that you were given when constructed - the one you are basing your modifications on. But there's no way for code that you call that indirectly asks for a stack frame list to get this right unless we have some way of influencing which StackFrameList gets provided by API's the code you are calling calls.

Can you explain a bit more why this is not an option? I'd like to understand what alternative designs were considered and why they were discarded. 

I'll pitch two straw man ideas to illustrate the kind of things I'm thinking about:

1. One way to look at this problem is that you don't know whether you should operate on the real or the synthetic frames. Have you considering threading that information through explicitly? That would achieve something similar to what this patch is doing, without the magic of checking the calling thread we're on and instead forcing you to think about it. As the synthetic frames are primarily a tool for presentation, maybe we can limit the places where it's used to the "boundary" of LLDB where it gets presented to users.
2. Another way to look at this problem is that the stack frame providers are synchronous, and we are doing work that may trigger changing the stack frame list form underneath us. Have you considered something where the providers build the stack frame list (without influencing the stack frame list as seen by LLDB) and then commit it when they're done with the work? 

FWIW, I totally recognize that compared to other scripted affordances, this one is especially tricky. For scripted processes, you generally manipulate another process under the hood, but here, you are essentially manipulating yourself. That's why I think it's so important to (re)evaluate how this feature works with that in mind.

As for the current implementation, I guarantee that this is going to lead to hard-to-diagnose bugs down the line. Even if this accounts for everything LLDB does today, someone will come along and make a change and forget to account for it. The reason I'm so confident is because we see the same thing with the private and public state thread which does something similar and Jim keeps needing to come back to either fix or retrofit something to make it work with that.

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