[Lldb-commits] [lldb] [lldb-dap] Updating the logging of lldb-dap to use existing LLDBLog. (PR #129294)

[John Harrison via lldb-commits]

ashgti wrote:

> The more I think about it, the less I like the idea of using `LLDBLog` in `lldb-dap`. Initially, I liked the idea of reusing our existing logging implementation, although I have expressed my concern about relying on `liblldb` in other PRs. But the issues and workarounds we're talking about here sound to me like signs that we're taking it too far.

I think I also ran into this issue when using some of the Host types in lldb-dap where we had two different 'global' instances of a value due to the static linking. Specifically this was the in the `lldb_private::FileSystem` API and its when I was trying to handling some defaults correctly across different platforms. I ended up re-writing the patch to not use that API specifically.

> 
> I think if we want to do this right, we have two options, and they're both pretty drastic:
> 
> 1. We expose `LLDBLog` from the SB API. This is something that has come up in the past in the context of people writing LLDB scripts. Similar to exposing the ability to generate progress events from the SB API, I think we need to be careful about this and clearly distinguish between unvetted logs, coming from scripts and vetted logs, coming from LLDB itself.
> 2. We give up on trying to build lldb-dap on top the SB API and move the DAP functionality into liblldb, allowing us to use `lldb_private` and exposing a simple interface that gets set up in the `lldb-dap` binary, similar to the command line driver. On the one hand I think it'd be sad to give up on building on top of the SB API. I always liked the idea that in theory, you could use `lldb-dap` with a different version of `liblldb`. In practice, I don't know if anyone does. We've already been cheating in that regard but we've gotten away with it because of static linking and I have a feeling this is the direction we'll likely keep on going. It's just too hard to pass up on reusing existing functionality.

At Google, we do actually do this to support building lldb-dap at head with the LLDB.framework linked from Xcode. This is a pretty specific use case because we are doing that so we can support mobile devices. And every once in a while when lldb-dap uses a new SB API that isn't in the internally supported Xcode release I end up writing either a back port or a fallback implementation.

> There is of course the third option which is designing a dedicated logging framework for `lldb-dap`. Although `LLDBLog` has is upsides, it also has its drawbacks, most notably that everything is global. If you change something in one `(SB)Debugger` instance, it is reflected in all the other ones. We'd have a similar problem for `lldb-dap` where someone changing the logging settings in one instance will affect all instances in the same process, which will be especially problematic in server mode.

I think there are some improvements we could make to the logging incrementally, instead of just passing a `std::ofstream *` around.

Thinking about these issues, I agree that we may not want to use the LLDBLog api and instead maybe we can make some incremental improvements to the existing system instead. I may revisit this after I get my other patch in to refactor the IOStream into a Transport abstraction and then I had two other follow ups for better type handling and cancel request support. For now I think I'll just close this PR.

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