[Lldb-commits] [PATCH] D128321: [lldb] Add OSLog log handler

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D128321#3603947 <https://reviews.llvm.org/D128321#3603947>, @labath wrote:

> In D128321#3603007 <https://reviews.llvm.org/D128321#3603007>, @JDevlieghere wrote:
>
>> In D128321#3602129 <https://reviews.llvm.org/D128321#3602129>, @clayborg wrote:
>>
>>> I think we should allow this class to always exist and not conditionally compile it in or out. Then we add a new Host.h method to emit a log message in "lldb/Host/Host.h" and allow each host OS to emit a message. Maybe there is a default implementation that emits the message to stderr, and then we override this for in HostInfoMacOSX.h? Then each OS can use their OS logging API of choice.
>>
>> Yeah, I considered that, but that would make Utility depend on Host which would (re)introduce the circular dependency between the two. The alternative would be to still make the class available unconditionally, but use target ifdefs to have different implementations.
>
> We already have `Host::SystemLog`. Could we use that? Or change it so that it can be used?

Not in its current state, no. The first problem is that it writes the message to the host log channel if it's enabled and in verbose mode. The second problem is that on macOS, it writes both to the system log and to stderr (all other platforms just write to stderr). If you look at its callers, it's clear that this function is really used for error reporting (the fact that it takes two log level "warning" and "error") is already a good indication of that. When I was looking at it yesterday I was thinking about ripping it out and replacing it with the diagnostics I added a little while ago.

> As for the dependencies, I think we could structure things such that the SystemLogHandler lives in the Host module, and the general logging infrastructure is not aware of it. (The only one who would know about it would be the `log enable` command, which would pass it down as a generic reference.)

I also considered that, but that just delays the problem until I want to enable this handler for the always-on logging (as outlined in https://discourse.llvm.org/c/subprojects/lldb/8). I haven't really looked into where that logic would live, but the reproducers lived in Utility so I was expecting all of this to be there as well.

So both things combined, I still think the current approach makes the most sense. But if we really want this to live in Host (and deal with the dependency issues later) I can implement a new `Host::SystemLog` variant that essentially does what the SystemLogHander is doing and implement the log handler in Host based on that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128321/new/

https://reviews.llvm.org/D128321