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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

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.



================
Comment at: lldb/include/lldb/Utility/Log.h:99
 
+#if defined(__APPLE__)
+class OSLogLogHandler : public LogHandler {
----------------
remove #if so everyone can use this and we can enable this in any log.


================
Comment at: lldb/source/Utility/Log.cpp:35-39
+#if defined(__APPLE__)
+#include <os/log.h>
+static os_log_t g_os_log;
+static std::once_flag g_os_log_once;
+#endif
----------------
move this to HostInfoMacOSX.h in the Host::EmitLogMessage() as mentioned in the main feedback comment


================
Comment at: lldb/source/Utility/Log.cpp:399
+
+#if defined(__APPLE__)
+OSLogLogHandler::OSLogLogHandler() {
----------------
remove #if


================
Comment at: lldb/source/Utility/Log.cpp:400-404
+OSLogLogHandler::OSLogLogHandler() {
+  std::call_once(g_os_log_once, []() {
+    g_os_log = os_log_create("com.apple.dt.lldb", "lldb");
+  });
+}
----------------
move this to HostInfoMacOSX.h to be used in HostInfoMacOSX::EmitLogMessage(StringRef)


================
Comment at: lldb/source/Utility/Log.cpp:407
+void OSLogLogHandler::Emit(llvm::StringRef message) {
+  os_log_with_type(g_os_log, OS_LOG_TYPE_DEFAULT, "%{public}s", message.data());
+}
----------------
Be careful as "message.data()" might cause more data to be emitted. We will need to use "message.str().c_str()" to ensure it is NULL terminated. Though we could require that the "message" come in as NULL terminated already, so maybe we don't need to, but we should document it, and possibly add a:

```
assert(message.data()[message.size()] == '\0');
```


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

https://reviews.llvm.org/D128321