From 98f1b5e8a37987e185038380ccc5203a8fae8063 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Wed, 25 Sep 2024 11:04:56 -0700 Subject: [PATCH] [lldb] Introduce an always-on system log category/channel Add an "always on" log category and channel. Unlike other, existing log channels, it is not exposed to users. The channel is meant to be used sparsely and deliberately for logging high-value information to the system log. We have a similar concept in the downstream Swift fork and this has proven to be extremely valuable. This is especially true on macOS where system log messages are automatically captured as part of a sysdiagnose. This patch revives https://reviews.llvm.org/D135621 although the purpose is slightly different (logging to the system log rather than to a ring buffer dumped as part of the diagnostics). This patch addresses all the remaining outstanding comments. --- lldb/include/lldb/Host/Host.h | 19 +++++++++++ lldb/include/lldb/Utility/Log.h | 11 +++--- lldb/source/API/SystemInitializerFull.cpp | 3 ++ lldb/source/Host/common/Host.cpp | 16 +++++++++ lldb/source/Host/common/HostInfoBase.cpp | 2 ++ lldb/source/Utility/Log.cpp | 34 ++++++++++++------- lldb/test/Shell/Host/TestSytemLogChannel.test | 3 ++ 7 files changed, 70 insertions(+), 18 deletions(-) create mode 100644 lldb/test/Shell/Host/TestSytemLogChannel.test diff --git a/lldb/include/lldb/Host/Host.h b/lldb/include/lldb/Host/Host.h index 9d0994978402f..d8113a5fceead 100644 --- a/lldb/include/lldb/Host/Host.h +++ b/lldb/include/lldb/Host/Host.h @@ -31,6 +31,25 @@ class ProcessInstanceInfo; class ProcessInstanceInfoMatch; typedef std::vector ProcessInstanceInfoList; +// System log category and channel. This log channel is always enabled and +// therefore is supposed to be used sparsely. Use this log channel to log +// critical information that is expected to be relevant to the majority of bug +// reports. +enum class SystemLog : Log::MaskType { + System = Log::ChannelFlag<0>, + LLVM_MARK_AS_BITMASK_ENUM(System) +}; + +LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE(); + +class LogChannelSystem { +public: + static void Initialize(); + static void Terminate(); +}; + +template <> Log::Channel &LogChannelFor(); + // Exit Type for inferior processes struct WaitStatus { enum Type : uint8_t { diff --git a/lldb/include/lldb/Utility/Log.h b/lldb/include/lldb/Utility/Log.h index 27707c17f9b82..ac6347153a101 100644 --- a/lldb/include/lldb/Utility/Log.h +++ b/lldb/include/lldb/Utility/Log.h @@ -272,6 +272,12 @@ class Log final { void VAFormatf(llvm::StringRef file, llvm::StringRef function, const char *format, va_list args); + void Enable(const std::shared_ptr &handler_sp, + std::optional flags = std::nullopt, + uint32_t options = 0); + + void Disable(std::optional flags = std::nullopt); + private: Channel &m_channel; @@ -297,11 +303,6 @@ class Log final { return m_handler; } - void Enable(const std::shared_ptr &handler_sp, uint32_t options, - MaskType flags); - - void Disable(MaskType flags); - bool Dump(llvm::raw_ostream &stream); typedef llvm::StringMap ChannelMap; diff --git a/lldb/source/API/SystemInitializerFull.cpp b/lldb/source/API/SystemInitializerFull.cpp index 995d14f7c1fa1..8a992a6889a91 100644 --- a/lldb/source/API/SystemInitializerFull.cpp +++ b/lldb/source/API/SystemInitializerFull.cpp @@ -17,6 +17,7 @@ #include "lldb/Interpreter/CommandInterpreter.h" #include "lldb/Target/ProcessTrace.h" #include "lldb/Utility/Timer.h" +#include "lldb/Version/Version.h" #include "llvm/Support/CommandLine.h" #include "llvm/Support/TargetSelect.h" @@ -83,6 +84,8 @@ llvm::Error SystemInitializerFull::Initialize() { // Use the Debugger's LLDBAssert callback. SetLLDBAssertCallback(Debugger::AssertCallback); + LLDB_LOG(GetLog(SystemLog::System), "{0}", GetVersion()); + return llvm::Error::success(); } diff --git a/lldb/source/Host/common/Host.cpp b/lldb/source/Host/common/Host.cpp index f08adea6546ae..abca6068d3604 100644 --- a/lldb/source/Host/common/Host.cpp +++ b/lldb/source/Host/common/Host.cpp @@ -125,6 +125,22 @@ void Host::SystemLog(Severity severity, llvm::StringRef message) { #endif #endif +static constexpr Log::Category g_categories[] = { + {{"system"}, {"system log"}, SystemLog::System}}; + +static Log::Channel g_system_channel(g_categories, SystemLog::System); +static Log g_system_log(g_system_channel); + +template <> Log::Channel &lldb_private::LogChannelFor() { + return g_system_channel; +} + +void LogChannelSystem::Initialize() { + g_system_log.Enable(std::make_shared()); +} + +void LogChannelSystem::Terminate() { g_system_log.Disable(); } + #if !defined(__APPLE__) && !defined(_WIN32) static thread_result_t MonitorChildProcessThreadFunction(::pid_t pid, diff --git a/lldb/source/Host/common/HostInfoBase.cpp b/lldb/source/Host/common/HostInfoBase.cpp index 5c44c2f38b287..89dfe4a9e9baa 100644 --- a/lldb/source/Host/common/HostInfoBase.cpp +++ b/lldb/source/Host/common/HostInfoBase.cpp @@ -76,9 +76,11 @@ static HostInfoBase::SharedLibraryDirectoryHelper *g_shlib_dir_helper = nullptr; void HostInfoBase::Initialize(SharedLibraryDirectoryHelper *helper) { g_shlib_dir_helper = helper; g_fields = new HostInfoBaseFields(); + LogChannelSystem::Initialize(); } void HostInfoBase::Terminate() { + LogChannelSystem::Terminate(); g_shlib_dir_helper = nullptr; delete g_fields; g_fields = nullptr; diff --git a/lldb/source/Utility/Log.cpp b/lldb/source/Utility/Log.cpp index f6b1381f63ad1..3798f40647637 100644 --- a/lldb/source/Utility/Log.cpp +++ b/lldb/source/Utility/Log.cpp @@ -93,22 +93,28 @@ Log::MaskType Log::GetFlags(llvm::raw_ostream &stream, } void Log::Enable(const std::shared_ptr &handler_sp, - uint32_t options, Log::MaskType flags) { + std::optional flags, uint32_t options) { llvm::sys::ScopedWriter lock(m_mutex); - MaskType mask = m_mask.fetch_or(flags, std::memory_order_relaxed); - if (mask | flags) { + if (!flags) + flags = m_channel.default_flags; + + MaskType mask = m_mask.fetch_or(*flags, std::memory_order_relaxed); + if (mask | *flags) { m_options.store(options, std::memory_order_relaxed); m_handler = handler_sp; m_channel.log_ptr.store(this, std::memory_order_relaxed); } } -void Log::Disable(Log::MaskType flags) { +void Log::Disable(std::optional flags) { llvm::sys::ScopedWriter lock(m_mutex); - MaskType mask = m_mask.fetch_and(~flags, std::memory_order_relaxed); - if (!(mask & ~flags)) { + if (!flags) + flags = std::numeric_limits::max(); + + MaskType mask = m_mask.fetch_and(~(*flags), std::memory_order_relaxed); + if (!(mask & ~(*flags))) { m_handler.reset(); m_channel.log_ptr.store(nullptr, std::memory_order_relaxed); } @@ -230,10 +236,11 @@ bool Log::EnableLogChannel(const std::shared_ptr &log_handler_sp, error_stream << llvm::formatv("Invalid log channel '{0}'.\n", channel); return false; } - MaskType flags = categories.empty() - ? iter->second.m_channel.default_flags - : GetFlags(error_stream, *iter, categories); - iter->second.Enable(log_handler_sp, log_options, flags); + + auto flags = categories.empty() ? std::optional{} + : GetFlags(error_stream, *iter, categories); + + iter->second.Enable(log_handler_sp, flags, log_options); return true; } @@ -245,9 +252,10 @@ bool Log::DisableLogChannel(llvm::StringRef channel, error_stream << llvm::formatv("Invalid log channel '{0}'.\n", channel); return false; } - MaskType flags = categories.empty() - ? std::numeric_limits::max() - : GetFlags(error_stream, *iter, categories); + + auto flags = categories.empty() ? std::optional{} + : GetFlags(error_stream, *iter, categories); + iter->second.Disable(flags); return true; } diff --git a/lldb/test/Shell/Host/TestSytemLogChannel.test b/lldb/test/Shell/Host/TestSytemLogChannel.test new file mode 100644 index 0000000000000..4de699f0e09a4 --- /dev/null +++ b/lldb/test/Shell/Host/TestSytemLogChannel.test @@ -0,0 +1,3 @@ +RUN: %lldb -o 'log list' -o 'log disable system' 2>&1 | FileCheck %s +CHECK-NOT: Logging categories for 'system' +CHECK: Invalid log channel 'system'