https://github.com/DavidSpickett created https://github.com/llvm/llvm-project/pull/206479
Instead of a boolean plus writing errors to a stream. llvm::Error is either success or an error message, unlike before where we could (and did sometimes) succeed but also have an error message. We can do this for DisableLogChannel too but I'll do that in a follow up. >From 09997ffdc07bb7bb9fca429564ca9adb6bdee780 Mon Sep 17 00:00:00 2001 From: David Spickett <[email protected]> Date: Wed, 24 Jun 2026 10:38:17 +0000 Subject: [PATCH 1/3] [lldb] Fix surprising return values from Log::Enable/DisableChannel Fixes #202615, and the fact that the command "log enable" was not being marked as a failure in some situations where it printed errors. The root of this is some unexpected behaviour from Log::EnableChannel and Log::DisableChannel. They only returned false if the channel was invalid. If any of the categories were invalid, error messages would be generated, but EnableChannel would return true. This is what caused lldb-server not to exit or print any error when this happened. The lldb command "log enable" was printing the error stream regardless of result, but the command object was never set to failed. (the same applies to DisableChannel) To fix this I have made them do what you'd expect. Return false if any part of the log channel or categories was not recognised. To do that, I had to allow for GetFlags failing. So that has changed return type. The existing behaviour of Log::Enable and Log::Disable is also not obvious at first. I have not changed it in this patch but I have had to preserve it, so I'll explain it: * If given an empty optional for flags, use the default flags. * If given a flags value of 0, use it, which results in disabling all categories. * If given a non-zero flags value, use it, enabling whatever categories had flags set. There is also an argument for changing all of this into llvm::expected, so that you either return a thing, or have error messasges, not both. However, doing that through the whole call stack is going to obscure the fix I'm doing here. --- lldb/include/lldb/Utility/Log.h | 11 +++-- lldb/source/Commands/CommandObjectLog.cpp | 5 ++- lldb/source/Utility/Log.cpp | 42 +++++++++++++------ .../API/commands/log/basic/TestLogging.py | 25 +++++++++++ .../TestGdbserverErrorMessages.test | 10 +++++ .../TestPlatformErrorMessages.test | 7 ++++ lldb/tools/lldb-server/lldb-gdbserver.cpp | 2 +- lldb/tools/lldb-server/lldb-platform.cpp | 2 +- lldb/unittests/Utility/LogTest.cpp | 10 ++++- 9 files changed, 93 insertions(+), 21 deletions(-) diff --git a/lldb/include/lldb/Utility/Log.h b/lldb/include/lldb/Utility/Log.h index 9da33f5f39562..2c027ad07c616 100644 --- a/lldb/include/lldb/Utility/Log.h +++ b/lldb/include/lldb/Utility/Log.h @@ -309,9 +309,14 @@ class Log final { static void ListCategories(llvm::raw_ostream &stream, const ChannelMap::value_type &entry); - static Log::MaskType GetFlags(llvm::raw_ostream &stream, - const ChannelMap::value_type &entry, - llvm::ArrayRef<const char *> categories); + + /// Convert an array of category names into a set of category flags. + /// Returns a bitmask of categories, or an empty optional in the case that + /// at least one category name was not recognised. All unknown category names + /// are reported as errors in the provided stream. + static std::optional<Log::MaskType> + GetFlags(llvm::raw_ostream &stream, const ChannelMap::value_type &entry, + llvm::ArrayRef<const char *> categories); Log(const Log &) = delete; void operator=(const Log &) = delete; diff --git a/lldb/source/Commands/CommandObjectLog.cpp b/lldb/source/Commands/CommandObjectLog.cpp index e51a85e9f0308..3995247394925 100644 --- a/lldb/source/Commands/CommandObjectLog.cpp +++ b/lldb/source/Commands/CommandObjectLog.cpp @@ -204,12 +204,13 @@ class CommandObjectLogEnable : public CommandObjectParsed { channel, args.GetArgumentArrayRef(), log_file, m_options.log_options, m_options.buffer_size.GetCurrentValue(), m_options.handler, error_stream); - result.GetErrorStream() << error; if (success) result.SetStatus(eReturnStatusSuccessFinishNoResult); - else + else { + result.GetErrorStream() << error; result.SetStatus(eReturnStatusFailed); + } } CommandOptions m_options; diff --git a/lldb/source/Utility/Log.cpp b/lldb/source/Utility/Log.cpp index 6b077a3766e9f..c93b4c544b318 100644 --- a/lldb/source/Utility/Log.cpp +++ b/lldb/source/Utility/Log.cpp @@ -65,11 +65,11 @@ void Log::ListCategories(llvm::raw_ostream &stream, }); } -Log::MaskType Log::GetFlags(llvm::raw_ostream &stream, - const ChannelMap::value_type &entry, - llvm::ArrayRef<const char *> categories) { - bool list_categories = false; +std::optional<Log::MaskType> +Log::GetFlags(llvm::raw_ostream &stream, const ChannelMap::value_type &entry, + llvm::ArrayRef<const char *> categories) { Log::MaskType flags = 0; + llvm::SmallVector<std::string> unrecognized_categories; for (const char *category : categories) { if (llvm::StringRef("all").equals_insensitive(category)) { flags |= std::numeric_limits<Log::MaskType>::max(); @@ -87,12 +87,20 @@ Log::MaskType Log::GetFlags(llvm::raw_ostream &stream, flags |= cat->flag; continue; } - stream << llvm::formatv("error: unrecognized log category '{0}'\n", - category); - list_categories = true; + unrecognized_categories.push_back(llvm::formatv("'{}'", category)); } - if (list_categories) + + if (unrecognized_categories.size()) { + stream << "error: unrecognized log " + << ((unrecognized_categories.size() == 1) ? "category " + : "categories ") + << llvm::join(unrecognized_categories.begin(), + unrecognized_categories.end(), ", ") + << "\n"; ListCategories(stream, entry); + return {}; + } + return flags; } @@ -215,8 +223,13 @@ bool Log::EnableLogChannel(const std::shared_ptr<LogHandler> &log_handler_sp, return false; } - auto flags = categories.empty() ? std::optional<MaskType>{} - : GetFlags(error_stream, *iter, categories); + std::optional<MaskType> flags; + + if (!categories.empty()) { + flags = GetFlags(error_stream, *iter, categories); + if (!flags) + return false; + } iter->second.Enable(log_handler_sp, flags, log_options); return true; @@ -231,8 +244,13 @@ bool Log::DisableLogChannel(llvm::StringRef channel, return false; } - auto flags = categories.empty() ? std::optional<MaskType>{} - : GetFlags(error_stream, *iter, categories); + std::optional<MaskType> flags; + + if (!categories.empty()) { + flags = GetFlags(error_stream, *iter, categories); + if (!flags) + return false; + } iter->second.Disable(flags); return true; diff --git a/lldb/test/API/commands/log/basic/TestLogging.py b/lldb/test/API/commands/log/basic/TestLogging.py index daa32dc85753d..e8dce0e2250ac 100644 --- a/lldb/test/API/commands/log/basic/TestLogging.py +++ b/lldb/test/API/commands/log/basic/TestLogging.py @@ -94,3 +94,28 @@ def test_all_log_options(self): self.runCmd("log disable lldb") self.assertTrue(os.path.isfile(self.log_file)) + + def test_log_invalid(self): + self.expect( + "log enable not_a_channel not_a_category", + error=True, + substrs=["Invalid log channel 'not_a_channel'"], + ) + + self.expect( + "log enable lldb not_a_category", + error=True, + substrs=[ + "unrecognized log category 'not_a_category'", + "Logging categories for 'lldb':", + ], + ) + + self.expect( + "log enable lldb not_a_category api not_a_category_either", + error=True, + substrs=[ + "unrecognized log categories 'not_a_category', 'not_a_category_either'", + "Logging categories for 'lldb':", + ], + ) diff --git a/lldb/test/Shell/lldb-server/TestGdbserverErrorMessages.test b/lldb/test/Shell/lldb-server/TestGdbserverErrorMessages.test index d0bcbcfb2eb37..bcf3c780a3c79 100644 --- a/lldb/test/Shell/lldb-server/TestGdbserverErrorMessages.test +++ b/lldb/test/Shell/lldb-server/TestGdbserverErrorMessages.test @@ -13,5 +13,15 @@ BOGUS: error: unknown argument '--bogus' RUN: not %lldb-server gdbserver 2>&1 | FileCheck --check-prefixes=CONN,ALL %s CONN: error: no connection arguments +RUN: not %lldb-server gdbserver --log-channels 2>&1 | FileCheck --check-prefixes=LOGCHANNELS_MISSING,ALL %s +LOGCHANNELS_MISSING: error: --log-channels: missing argument + +RUN: not %lldb-server gdbserver --log-channels not_a_channel 2>&1 | FileCheck --check-prefixes=INVALID_LOG_CHANNEL %s +INVALID_LOG_CHANNEL: Unable to setup logging for channel "not_a_channel": Invalid log channel 'not_a_channel'. + +RUN: not %lldb-server gdbserver --log-channels "lldb not_a_category" 2>&1 | FileCheck --check-prefixes=INVALID_LOG_CATEGORY %s +INVALID_LOG_CATEGORY: Unable to setup logging for channel "lldb": error: unrecognized log category 'not_a_category' +INVALID_LOG_CATEGORY-NEXT: Logging categories for 'lldb': + ALL: Use '{{.*}} g[dbserver] --help' for a complete list of options. diff --git a/lldb/test/Shell/lldb-server/TestPlatformErrorMessages.test b/lldb/test/Shell/lldb-server/TestPlatformErrorMessages.test index 94d8d0f0bbcee..2cc03b4f4d785 100644 --- a/lldb/test/Shell/lldb-server/TestPlatformErrorMessages.test +++ b/lldb/test/Shell/lldb-server/TestPlatformErrorMessages.test @@ -22,4 +22,11 @@ LOGFILE_MISSING: error: --log-file: missing argument RUN: not %platformserver --log-channels 2>&1 | FileCheck --check-prefixes=LOGCHANNELS_MISSING,ALL %s LOGCHANNELS_MISSING: error: --log-channels: missing argument +RUN: not %platformserver --log-channels not_a_channel 2>&1 | FileCheck --check-prefixes=INVALID_LOG_CHANNEL %s +INVALID_LOG_CHANNEL: Unable to setup logging for channel "not_a_channel": Invalid log channel 'not_a_channel'. + +RUN: not %platformserver --log-channels "lldb not_a_category" 2>&1 | FileCheck --check-prefixes=INVALID_LOG_CATEGORY %s +INVALID_LOG_CATEGORY: Unable to setup logging for channel "lldb": error: unrecognized log category 'not_a_category' +INVALID_LOG_CATEGORY-NEXT: Logging categories for 'lldb': + ALL: Use 'lldb-server{{(\.exe)?}} {{p|platform}} --help' for a complete list of options. diff --git a/lldb/tools/lldb-server/lldb-gdbserver.cpp b/lldb/tools/lldb-server/lldb-gdbserver.cpp index 2e00c66eaa4dd..dc3c45358563d 100644 --- a/lldb/tools/lldb-server/lldb-gdbserver.cpp +++ b/lldb/tools/lldb-server/lldb-gdbserver.cpp @@ -437,7 +437,7 @@ int main_gdbserver(int argc, char *argv[]) { log_file, log_channels, LLDB_LOG_OPTION_PREPEND_TIMESTAMP | LLDB_LOG_OPTION_PREPEND_FILE_FUNCTION)) - return -1; + return EXIT_FAILURE; std::vector<llvm::StringRef> Inputs; for (opt::Arg *Arg : Args.filtered(OPT_INPUT)) diff --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp index ec2ef053241ca..d4ad0ca4511b8 100644 --- a/lldb/tools/lldb-server/lldb-platform.cpp +++ b/lldb/tools/lldb-server/lldb-platform.cpp @@ -493,7 +493,7 @@ int main_platform(int argc, char *argv[]) { } if (!LLDBServerUtilities::SetupLogging(log_file, log_channels, 0)) - return -1; + return EXIT_FAILURE; // Print usage and exit if no listening port is specified. if (listen_host_port.empty() && fd == SharedSocket::kInvalidFD) { diff --git a/lldb/unittests/Utility/LogTest.cpp b/lldb/unittests/Utility/LogTest.cpp index 602d019adac54..bc283971a7d99 100644 --- a/lldb/unittests/Utility/LogTest.cpp +++ b/lldb/unittests/Utility/LogTest.cpp @@ -227,9 +227,15 @@ TEST_F(LogChannelTest, Enable) { EXPECT_NE(nullptr, GetLog(TestChannel::FOO)); EXPECT_NE(nullptr, GetLog(TestChannel::BAR)); - EXPECT_TRUE(EnableChannel(log_handler_sp, 0, "chan", {"baz"}, error)); + EXPECT_FALSE(EnableChannel(log_handler_sp, 0, "chan", {"baz"}, error)); EXPECT_NE(std::string::npos, error.find("unrecognized log category 'baz'")) << "error: " << error; + + EXPECT_FALSE( + EnableChannel(log_handler_sp, 0, "chan", {"baz", "bar", "abc"}, error)); + EXPECT_NE(std::string::npos, + error.find("unrecognized log categories 'baz', 'abc'")) + << "error: " << error; } TEST_F(LogChannelTest, EnableOptions) { @@ -256,7 +262,7 @@ TEST_F(LogChannelTest, Disable) { EXPECT_NE(nullptr, GetLog(TestChannel::FOO)); EXPECT_EQ(nullptr, GetLog(TestChannel::BAR)); - EXPECT_TRUE(DisableChannel("chan", {"baz"}, error)); + EXPECT_FALSE(DisableChannel("chan", {"baz"}, error)); EXPECT_NE(std::string::npos, error.find("unrecognized log category 'baz'")) << "error: " << error; EXPECT_NE(nullptr, GetLog(TestChannel::FOO)); >From 2131aac0779f00703681fd59da6623c1c855a668 Mon Sep 17 00:00:00 2001 From: David Spickett <[email protected]> Date: Thu, 25 Jun 2026 09:23:32 +0000 Subject: [PATCH 2/3] missing include --- lldb/source/Utility/Log.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/lldb/source/Utility/Log.cpp b/lldb/source/Utility/Log.cpp index c93b4c544b318..8018316e77dbc 100644 --- a/lldb/source/Utility/Log.cpp +++ b/lldb/source/Utility/Log.cpp @@ -10,6 +10,7 @@ #include "lldb/Utility/VASPrintf.h" #include "llvm/ADT/SmallString.h" +#include "llvm/ADT/StringExtras.h" #include "llvm/ADT/Twine.h" #include "llvm/ADT/iterator.h" >From e4faec27bf2c7a7cc838ad529a65d623f9e8c68a Mon Sep 17 00:00:00 2001 From: David Spickett <[email protected]> Date: Fri, 26 Jun 2026 15:16:58 +0000 Subject: [PATCH 3/3] [lldb] Return llvm::Error from EnableLogChannel Instead of a boolean plus writing errors to a stream. llvm::Error is either success or an error message, unlike before where we could (and did sometimes) succeed but also have an error message. We can do this for DisableLogChannel too but I'll do that in a follow up. --- lldb/include/lldb/Core/Debugger.h | 9 +- lldb/include/lldb/Utility/Log.h | 13 +-- lldb/source/API/SBDebugger.cpp | 9 +- lldb/source/Commands/CommandObjectLog.cpp | 19 ++- lldb/source/Core/Debugger.cpp | 24 ++-- lldb/source/Utility/Log.cpp | 69 ++++++----- .../log/invalid-args/TestInvalidArgsLog.py | 2 +- .../tools/lldb-server/LLDBServerUtilities.cpp | 13 +-- lldb/tools/lldb-test/lldb-test.cpp | 5 +- lldb/unittests/Utility/LogTest.cpp | 109 ++++++++++-------- 10 files changed, 142 insertions(+), 130 deletions(-) diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h index 179ef0f38d940..8c6a7158a5262 100644 --- a/lldb/include/lldb/Core/Debugger.h +++ b/lldb/include/lldb/Core/Debugger.h @@ -258,11 +258,10 @@ class Debugger : public std::enable_shared_from_this<Debugger>, void ClearIOHandlers(); - bool EnableLog(llvm::StringRef channel, - llvm::ArrayRef<const char *> categories, - llvm::StringRef log_file, uint32_t log_options, - size_t buffer_size, LogHandlerKind log_handler_kind, - llvm::raw_ostream &error_stream); + llvm::Error EnableLog(llvm::StringRef channel, + llvm::ArrayRef<const char *> categories, + llvm::StringRef log_file, uint32_t log_options, + size_t buffer_size, LogHandlerKind log_handler_kind); void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton); diff --git a/lldb/include/lldb/Utility/Log.h b/lldb/include/lldb/Utility/Log.h index 2c027ad07c616..899d0584eb001 100644 --- a/lldb/include/lldb/Utility/Log.h +++ b/lldb/include/lldb/Utility/Log.h @@ -194,11 +194,10 @@ class Log final { static void Register(llvm::StringRef name, Channel &channel); static void Unregister(llvm::StringRef name); - static bool + static llvm::Error EnableLogChannel(const std::shared_ptr<LogHandler> &log_handler_sp, uint32_t log_options, llvm::StringRef channel, - llvm::ArrayRef<const char *> categories, - llvm::raw_ostream &error_stream); + llvm::ArrayRef<const char *> categories); static bool DisableLogChannel(llvm::StringRef channel, llvm::ArrayRef<const char *> categories, @@ -311,11 +310,11 @@ class Log final { const ChannelMap::value_type &entry); /// Convert an array of category names into a set of category flags. - /// Returns a bitmask of categories, or an empty optional in the case that + /// Returns a bitmask of categories, or an error message in the case that /// at least one category name was not recognised. All unknown category names - /// are reported as errors in the provided stream. - static std::optional<Log::MaskType> - GetFlags(llvm::raw_ostream &stream, const ChannelMap::value_type &entry, + /// are included in the error. + static llvm::Expected<Log::MaskType> + GetFlags(const ChannelMap::value_type &entry, llvm::ArrayRef<const char *> categories); Log(const Log &) = delete; diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp index fa2b2e465359b..4f340342df66b 100644 --- a/lldb/source/API/SBDebugger.cpp +++ b/lldb/source/API/SBDebugger.cpp @@ -1642,11 +1642,10 @@ bool SBDebugger::EnableLog(const char *channel, const char **categories) { if (m_opaque_sp) { uint32_t log_options = LLDB_LOG_OPTION_PREPEND_TIMESTAMP | LLDB_LOG_OPTION_PREPEND_THREAD_NAME; - std::string error; - llvm::raw_string_ostream error_stream(error); - return m_opaque_sp->EnableLog(channel, GetCategoryArray(categories), "", - log_options, /*buffer_size=*/0, - eLogHandlerStream, error_stream); + llvm::Error err = m_opaque_sp->EnableLog( + channel, GetCategoryArray(categories), "", log_options, + /*buffer_size=*/0, eLogHandlerStream); + return !bool(err); } else return false; } diff --git a/lldb/source/Commands/CommandObjectLog.cpp b/lldb/source/Commands/CommandObjectLog.cpp index 3995247394925..0abf31a3d8902 100644 --- a/lldb/source/Commands/CommandObjectLog.cpp +++ b/lldb/source/Commands/CommandObjectLog.cpp @@ -21,6 +21,8 @@ #include "lldb/Utility/Stream.h" #include "lldb/Utility/Timer.h" +#include "llvm/Support/FormatAdapters.h" + using namespace lldb; using namespace lldb_private; @@ -198,19 +200,16 @@ class CommandObjectLogEnable : public CommandObjectParsed { else log_file[0] = '\0'; - std::string error; - llvm::raw_string_ostream error_stream(error); - bool success = GetDebugger().EnableLog( + llvm::Error error = GetDebugger().EnableLog( channel, args.GetArgumentArrayRef(), log_file, m_options.log_options, - m_options.buffer_size.GetCurrentValue(), m_options.handler, - error_stream); + m_options.buffer_size.GetCurrentValue(), m_options.handler); - if (success) - result.SetStatus(eReturnStatusSuccessFinishNoResult); - else { - result.GetErrorStream() << error; + if (error) { + result.GetErrorStream() + << llvm::formatv("{}", llvm::fmt_consume(std::move(error))); result.SetStatus(eReturnStatusFailed); - } + } else + result.SetStatus(eReturnStatusSuccessFinishNoResult); } CommandOptions m_options; diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp index 9a75c5eb407c5..f57e9c98cf41c 100644 --- a/lldb/source/Core/Debugger.cpp +++ b/lldb/source/Core/Debugger.cpp @@ -71,6 +71,7 @@ #include "llvm/Config/llvm-config.h" #include "llvm/Support/DynamicLibrary.h" #include "llvm/Support/FileSystem.h" +#include "llvm/Support/FormatAdapters.h" #include "llvm/Support/Process.h" #include "llvm/Support/ThreadPool.h" #include "llvm/Support/Threading.h" @@ -1884,11 +1885,11 @@ CreateLogHandler(LogHandlerKind log_handler_kind, int fd, bool should_close, return {}; } -bool Debugger::EnableLog(llvm::StringRef channel, - llvm::ArrayRef<const char *> categories, - llvm::StringRef log_file, uint32_t log_options, - size_t buffer_size, LogHandlerKind log_handler_kind, - llvm::raw_ostream &error_stream) { +llvm::Error Debugger::EnableLog(llvm::StringRef channel, + llvm::ArrayRef<const char *> categories, + llvm::StringRef log_file, uint32_t log_options, + size_t buffer_size, + LogHandlerKind log_handler_kind) { std::shared_ptr<LogHandler> log_handler_sp; if (m_callback_handler_sp) { @@ -1913,11 +1914,10 @@ bool Debugger::EnableLog(llvm::StringRef channel, flags |= File::eOpenOptionTruncate; llvm::Expected<FileUP> file = FileSystem::Instance().Open( FileSpec(log_file), flags, lldb::eFilePermissionsFileDefault, false); - if (!file) { - error_stream << "Unable to open log file '" << log_file - << "': " << llvm::toString(file.takeError()) << "\n"; - return false; - } + if (!file) + return llvm::createStringErrorV("Unable to open log file '{}': {}", + log_file, + llvm::fmt_consume(file.takeError())); log_handler_sp = CreateLogHandler(log_handler_kind, (*file)->GetDescriptor(), @@ -1930,8 +1930,8 @@ bool Debugger::EnableLog(llvm::StringRef channel, if (log_options == 0) log_options = LLDB_LOG_OPTION_PREPEND_THREAD_NAME; - return Log::EnableLogChannel(log_handler_sp, log_options, channel, categories, - error_stream); + return Log::EnableLogChannel(log_handler_sp, log_options, channel, + categories); } ScriptInterpreter * diff --git a/lldb/source/Utility/Log.cpp b/lldb/source/Utility/Log.cpp index 8018316e77dbc..47697d6a5f44a 100644 --- a/lldb/source/Utility/Log.cpp +++ b/lldb/source/Utility/Log.cpp @@ -16,6 +16,7 @@ #include "llvm/Support/Casting.h" #include "llvm/Support/Chrono.h" +#include "llvm/Support/ErrorExtras.h" #include "llvm/Support/ManagedStatic.h" #include "llvm/Support/Path.h" #include "llvm/Support/Signals.h" @@ -66,8 +67,8 @@ void Log::ListCategories(llvm::raw_ostream &stream, }); } -std::optional<Log::MaskType> -Log::GetFlags(llvm::raw_ostream &stream, const ChannelMap::value_type &entry, +llvm::Expected<Log::MaskType> +Log::GetFlags(const ChannelMap::value_type &entry, llvm::ArrayRef<const char *> categories) { Log::MaskType flags = 0; llvm::SmallVector<std::string> unrecognized_categories; @@ -92,14 +93,16 @@ Log::GetFlags(llvm::raw_ostream &stream, const ChannelMap::value_type &entry, } if (unrecognized_categories.size()) { - stream << "error: unrecognized log " - << ((unrecognized_categories.size() == 1) ? "category " - : "categories ") - << llvm::join(unrecognized_categories.begin(), - unrecognized_categories.end(), ", ") - << "\n"; - ListCategories(stream, entry); - return {}; + std::string error_str; + llvm::raw_string_ostream error_stream(error_str); + error_stream << "error: unrecognized log " + << ((unrecognized_categories.size() == 1) ? "category " + : "categories ") + << llvm::join(unrecognized_categories.begin(), + unrecognized_categories.end(), ", ") + << "\n"; + ListCategories(error_stream, entry); + return llvm::createStringError(error_str); } return flags; @@ -214,26 +217,25 @@ void Log::Unregister(llvm::StringRef name) { g_channel_map->erase(iter); } -bool Log::EnableLogChannel(const std::shared_ptr<LogHandler> &log_handler_sp, - uint32_t log_options, llvm::StringRef channel, - llvm::ArrayRef<const char *> categories, - llvm::raw_ostream &error_stream) { +llvm::Error +Log::EnableLogChannel(const std::shared_ptr<LogHandler> &log_handler_sp, + uint32_t log_options, llvm::StringRef channel, + llvm::ArrayRef<const char *> categories) { auto iter = g_channel_map->find(channel); - if (iter == g_channel_map->end()) { - error_stream << llvm::formatv("Invalid log channel '{0}'.\n", channel); - return false; - } - - std::optional<MaskType> flags; + if (iter == g_channel_map->end()) + return llvm::createStringErrorV("Invalid log channel '{0}'.\n", channel); - if (!categories.empty()) { - flags = GetFlags(error_stream, *iter, categories); - if (!flags) - return false; + if (categories.empty()) { + iter->second.Enable(log_handler_sp, std::nullopt, log_options); + return llvm::Error::success(); } - iter->second.Enable(log_handler_sp, flags, log_options); - return true; + llvm::Expected<MaskType> flags = GetFlags(*iter, categories); + if (!flags) + return flags.takeError(); + + iter->second.Enable(log_handler_sp, *flags, log_options); + return llvm::Error::success(); } bool Log::DisableLogChannel(llvm::StringRef channel, @@ -245,15 +247,18 @@ bool Log::DisableLogChannel(llvm::StringRef channel, return false; } - std::optional<MaskType> flags; + if (categories.empty()) { + iter->second.Disable(std::nullopt); + return true; + } - if (!categories.empty()) { - flags = GetFlags(error_stream, *iter, categories); - if (!flags) - return false; + llvm::Expected<MaskType> flags = GetFlags(*iter, categories); + if (!flags) { + error_stream << toString(flags.takeError()) << "\n"; + return false; } - iter->second.Disable(flags); + iter->second.Disable(*flags); return true; } diff --git a/lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py b/lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py index 96089bd04ab85..dec2d9eedf7ea 100644 --- a/lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py +++ b/lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py @@ -28,5 +28,5 @@ def test_enable_invalid_path(self): self.expect( "log enable lldb all -f " + invalid_path, error=True, - substrs=["Unable to open log file '" + invalid_path + "': ", "\n"], + substrs=["Unable to open log file '" + invalid_path + "': "], ) diff --git a/lldb/tools/lldb-server/LLDBServerUtilities.cpp b/lldb/tools/lldb-server/LLDBServerUtilities.cpp index 0891f0eb6a976..27c2ee058fd48 100644 --- a/lldb/tools/lldb-server/LLDBServerUtilities.cpp +++ b/lldb/tools/lldb-server/LLDBServerUtilities.cpp @@ -15,6 +15,7 @@ #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/FileSystem.h" +#include "llvm/Support/FormatAdapters.h" using namespace lldb; using namespace lldb_private::lldb_server; @@ -62,18 +63,16 @@ bool LLDBServerUtilities::SetupLogging(const std::string &log_file, SmallVector<StringRef, 32> channel_array; log_channels.split(channel_array, ":", /*MaxSplit*/ -1, /*KeepEmpty*/ false); for (auto channel_with_categories : channel_array) { - std::string error; - llvm::raw_string_ostream error_stream(error); Args channel_then_categories(channel_with_categories); std::string channel(channel_then_categories.GetArgumentAtIndex(0)); channel_then_categories.Shift(); // Shift off the channel - bool success = Log::EnableLogChannel( - log_stream_sp, log_options, channel, - channel_then_categories.GetArgumentArrayRef(), error_stream); - if (!success) { + llvm::Error error = + Log::EnableLogChannel(log_stream_sp, log_options, channel, + channel_then_categories.GetArgumentArrayRef()); + if (error) { errs() << formatv("Unable to setup logging for channel \"{0}\": {1}", - channel, error); + channel, fmt_consume(std::move(error))); return false; } } diff --git a/lldb/tools/lldb-test/lldb-test.cpp b/lldb/tools/lldb-test/lldb-test.cpp index c43f6a88edb51..69624dea95f6a 100644 --- a/lldb/tools/lldb-test/lldb-test.cpp +++ b/lldb/tools/lldb-test/lldb-test.cpp @@ -1269,7 +1269,10 @@ int main(int argc, const char *argv[]) { /*add_to_history*/ eLazyBoolNo, Result); if (!opts::Log.empty()) - Dbg->EnableLog("lldb", {"all"}, opts::Log, 0, 0, eLogHandlerStream, errs()); + if (llvm::Error e = + Dbg->EnableLog("lldb", {"all"}, opts::Log, 0, 0, eLogHandlerStream)) + WithColor::error() << "failed to enable logs: " << toString(std::move(e)) + << '\n'; if (opts::BreakpointSubcommand) return opts::breakpoint::evaluateBreakpoints(*Dbg); diff --git a/lldb/unittests/Utility/LogTest.cpp b/lldb/unittests/Utility/LogTest.cpp index bc283971a7d99..3569327ebd5b7 100644 --- a/lldb/unittests/Utility/LogTest.cpp +++ b/lldb/unittests/Utility/LogTest.cpp @@ -14,6 +14,7 @@ #include "llvm/ADT/BitmaskEnum.h" #include "llvm/Support/ManagedStatic.h" #include "llvm/Support/Threading.h" +#include "llvm/Testing/Support/Error.h" #include <thread> using namespace lldb; @@ -38,17 +39,7 @@ namespace lldb_private { template <> Log::Channel &LogChannelFor<TestChannel>() { return test_channel; } } // namespace lldb_private -// Wrap enable, disable and list functions to make them easier to test. -static bool EnableChannel(std::shared_ptr<LogHandler> log_handler_sp, - uint32_t log_options, llvm::StringRef channel, - llvm::ArrayRef<const char *> categories, - std::string &error) { - error.clear(); - llvm::raw_string_ostream error_stream(error); - return Log::EnableLogChannel(log_handler_sp, log_options, channel, categories, - error_stream); -} - +// Wrap disable and list functions to make them easier to test. static bool DisableChannel(llvm::StringRef channel, llvm::ArrayRef<const char *> categories, std::string &error) { @@ -117,8 +108,8 @@ static std::string GetDumpAsString(const RotatingLogHandler &handler) { void LogChannelEnabledTest::SetUp() { LogChannelTest::SetUp(); - std::string error; - ASSERT_TRUE(EnableChannel(m_log_handler_sp, 0, "chan", {}, error)); + ASSERT_THAT_ERROR(Log::EnableLogChannel(m_log_handler_sp, 0, "chan", {}), + llvm::Succeeded()); m_log = GetLog(TestChannel::FOO); ASSERT_NE(nullptr, m_log); @@ -161,8 +152,8 @@ TEST(LogTest, Unregister) { Log::Register("chan", test_channel); EXPECT_EQ(nullptr, GetLog(TestChannel::FOO)); auto log_handler_sp = std::make_shared<TestLogHandler>(); - EXPECT_TRUE( - Log::EnableLogChannel(log_handler_sp, 0, "chan", {"foo"}, llvm::nulls())); + EXPECT_THAT_ERROR(Log::EnableLogChannel(log_handler_sp, 0, "chan", {"foo"}), + llvm::Succeeded()); EXPECT_NE(nullptr, GetLog(TestChannel::FOO)); Log::Unregister("chan"); EXPECT_EQ(nullptr, GetLog(TestChannel::FOO)); @@ -214,36 +205,47 @@ TEST(LogHandlerTest, TeeLogHandler) { TEST_F(LogChannelTest, Enable) { EXPECT_EQ(nullptr, GetLog(TestChannel::FOO)); auto log_handler_sp = std::make_shared<TestLogHandler>(); - std::string error; - ASSERT_FALSE(EnableChannel(log_handler_sp, 0, "chanchan", {}, error)); - EXPECT_EQ("Invalid log channel 'chanchan'.\n", error); + ASSERT_THAT_ERROR( + Log::EnableLogChannel(log_handler_sp, 0, "chanchan", {}), + llvm::FailedWithMessage("Invalid log channel 'chanchan'.\n")); - EXPECT_TRUE(EnableChannel(log_handler_sp, 0, "chan", {}, error)); + EXPECT_THAT_ERROR(Log::EnableLogChannel(log_handler_sp, 0, "chan", {}), + llvm::Succeeded()); EXPECT_NE(nullptr, GetLog(TestChannel::FOO)); EXPECT_EQ(nullptr, GetLog(TestChannel::BAR)); EXPECT_NE(nullptr, GetLog(TestChannel::FOO | TestChannel::BAR)); - EXPECT_TRUE(EnableChannel(log_handler_sp, 0, "chan", {"bar"}, error)); + EXPECT_THAT_ERROR(Log::EnableLogChannel(log_handler_sp, 0, "chan", {"bar"}), + llvm::Succeeded()); EXPECT_NE(nullptr, GetLog(TestChannel::FOO)); EXPECT_NE(nullptr, GetLog(TestChannel::BAR)); - EXPECT_FALSE(EnableChannel(log_handler_sp, 0, "chan", {"baz"}, error)); - EXPECT_NE(std::string::npos, error.find("unrecognized log category 'baz'")) - << "error: " << error; - - EXPECT_FALSE( - EnableChannel(log_handler_sp, 0, "chan", {"baz", "bar", "abc"}, error)); - EXPECT_NE(std::string::npos, - error.find("unrecognized log categories 'baz', 'abc'")) - << "error: " << error; + EXPECT_THAT_ERROR( + Log::EnableLogChannel(log_handler_sp, 0, "chan", {"baz"}), + llvm::FailedWithMessage("error: unrecognized log category 'baz'\n" + "Logging categories for 'chan':\n" + " all - all available logging categories\n" + " default - default set of logging categories\n" + " foo - log foo\n" + " bar - log bar\n")); + + EXPECT_THAT_ERROR( + Log::EnableLogChannel(log_handler_sp, 0, "chan", {"baz", "bar", "abc"}), + llvm::FailedWithMessage( + "error: unrecognized log categories 'baz', 'abc'\n" + "Logging categories for 'chan':\n" + " all - all available logging categories\n" + " default - default set of logging categories\n" + " foo - log foo\n" + " bar - log bar\n")); } TEST_F(LogChannelTest, EnableOptions) { EXPECT_EQ(nullptr, GetLog(TestChannel::FOO)); auto log_handler_sp = std::make_shared<TestLogHandler>(); - std::string error; - EXPECT_TRUE(EnableChannel(log_handler_sp, LLDB_LOG_OPTION_VERBOSE, "chan", {}, - error)); + EXPECT_THAT_ERROR(Log::EnableLogChannel(log_handler_sp, + LLDB_LOG_OPTION_VERBOSE, "chan", {}), + llvm::Succeeded()); Log *log = GetLog(TestChannel::FOO); ASSERT_NE(nullptr, log); @@ -253,11 +255,13 @@ TEST_F(LogChannelTest, EnableOptions) { TEST_F(LogChannelTest, Disable) { EXPECT_EQ(nullptr, GetLog(TestChannel::FOO)); auto log_handler_sp = std::make_shared<TestLogHandler>(); - std::string error; - EXPECT_TRUE(EnableChannel(log_handler_sp, 0, "chan", {"foo", "bar"}, error)); + EXPECT_THAT_ERROR( + Log::EnableLogChannel(log_handler_sp, 0, "chan", {"foo", "bar"}), + llvm::Succeeded()); EXPECT_NE(nullptr, GetLog(TestChannel::FOO)); EXPECT_NE(nullptr, GetLog(TestChannel::BAR)); + std::string error; EXPECT_TRUE(DisableChannel("chan", {"bar"}, error)); EXPECT_NE(nullptr, GetLog(TestChannel::FOO)); EXPECT_EQ(nullptr, GetLog(TestChannel::BAR)); @@ -289,23 +293,26 @@ TEST_F(LogChannelTest, List) { } TEST_F(LogChannelEnabledTest, log_options) { - std::string Err; EXPECT_EQ("Hello World\n", logAndTakeOutput("Hello World")); - EXPECT_TRUE(EnableChannel(getLogHandler(), 0, "chan", {}, Err)); + EXPECT_THAT_ERROR(Log::EnableLogChannel(getLogHandler(), 0, "chan", {}), + llvm::Succeeded()); EXPECT_EQ("Hello World\n", logAndTakeOutput("Hello World")); { - EXPECT_TRUE(EnableChannel(getLogHandler(), LLDB_LOG_OPTION_PREPEND_SEQUENCE, - "chan", {}, Err)); + EXPECT_THAT_ERROR(Log::EnableLogChannel(getLogHandler(), + LLDB_LOG_OPTION_PREPEND_SEQUENCE, + "chan", {}), + llvm::Succeeded()); llvm::StringRef Msg = logAndTakeOutput("Hello World"); int seq_no; EXPECT_EQ(1, sscanf(Msg.str().c_str(), "%d Hello World", &seq_no)); } { - EXPECT_TRUE(EnableChannel(getLogHandler(), - LLDB_LOG_OPTION_PREPEND_FILE_FUNCTION, "chan", {}, - Err)); + EXPECT_THAT_ERROR( + Log::EnableLogChannel( + getLogHandler(), LLDB_LOG_OPTION_PREPEND_FILE_FUNCTION, "chan", {}), + llvm::Succeeded()); llvm::StringRef Msg = logAndTakeOutput("Hello World"); char File[12]; char Function[17]; @@ -318,9 +325,10 @@ TEST_F(LogChannelEnabledTest, log_options) { } { - EXPECT_TRUE(EnableChannel(getLogHandler(), - LLDB_LOG_OPTION_PREPEND_FILE_FUNCTION, "chan", {}, - Err)); + EXPECT_THAT_ERROR( + Log::EnableLogChannel( + getLogHandler(), LLDB_LOG_OPTION_PREPEND_FILE_FUNCTION, "chan", {}), + llvm::Succeeded()); llvm::StringRef Msg = logAndTakeOutputf("Hello World"); char File[12]; char Function[18]; @@ -332,9 +340,10 @@ TEST_F(LogChannelEnabledTest, log_options) { EXPECT_STREQ("logAndTakeOutputf", Function); } - EXPECT_TRUE(EnableChannel(getLogHandler(), - LLDB_LOG_OPTION_PREPEND_PROC_AND_THREAD, "chan", {}, - Err)); + EXPECT_THAT_ERROR( + Log::EnableLogChannel( + getLogHandler(), LLDB_LOG_OPTION_PREPEND_PROC_AND_THREAD, "chan", {}), + llvm::Succeeded()); EXPECT_EQ(llvm::formatv("[{0,0+4}/{1,0+4}] Hello World\n", ::getpid(), llvm::get_threadid()) .str(), @@ -373,13 +382,13 @@ TEST_F(LogChannelEnabledTest, LogThread) { TEST_F(LogChannelEnabledTest, LogVerboseThread) { // Test that we are able to concurrently check the verbose flag of a log // channel and enable it. - std::string err; // Start logging on one thread. Concurrently, try enabling the log channel // (with different log options). std::thread log_thread([this] { LLDB_LOG_VERBOSE(getLog(), "Hello World"); }); - EXPECT_TRUE( - EnableChannel(getLogHandler(), LLDB_LOG_OPTION_VERBOSE, "chan", {}, err)); + EXPECT_THAT_ERROR(Log::EnableLogChannel(getLogHandler(), + LLDB_LOG_OPTION_VERBOSE, "chan", {}), + llvm::Succeeded()); log_thread.join(); // The log thread either managed to write to the log, or it didn't. In either _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
