https://github.com/DavidSpickett updated https://github.com/llvm/llvm-project/pull/205561
>From 7dd73d1b8536a74e6cee4046c74ab844368992b1 Mon Sep 17 00:00:00 2001 From: David Spickett <[email protected]> Date: Wed, 24 Jun 2026 10:38:17 +0000 Subject: [PATCH 1/2] [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 73b9764508cbf..8b6c3e777e8c6 100644 --- a/lldb/include/lldb/Utility/Log.h +++ b/lldb/include/lldb/Utility/Log.h @@ -317,9 +317,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 945dc6210bfea..b9e2a51bcd4bc 100644 --- a/lldb/source/Commands/CommandObjectLog.cpp +++ b/lldb/source/Commands/CommandObjectLog.cpp @@ -207,12 +207,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 09b4eb05f2edd..ccb71beb58339 100644 --- a/lldb/source/Utility/Log.cpp +++ b/lldb/source/Utility/Log.cpp @@ -70,11 +70,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(); @@ -92,12 +92,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; } @@ -224,8 +232,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; @@ -240,8 +253,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 eacf8d83390f7..8051cafe49519 100644 --- a/lldb/test/API/commands/log/basic/TestLogging.py +++ b/lldb/test/API/commands/log/basic/TestLogging.py @@ -124,3 +124,28 @@ def test_json_output(self): self.assertIn("tid", obj) self.assertIn("file", obj) self.assertIn("function", obj) + + 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 b1f03b36c578f..1f417d6dfed73 100644 --- a/lldb/tools/lldb-server/lldb-platform.cpp +++ b/lldb/tools/lldb-server/lldb-platform.cpp @@ -491,7 +491,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 fed0a5274b948..1090b113433ed 100644 --- a/lldb/unittests/Utility/LogTest.cpp +++ b/lldb/unittests/Utility/LogTest.cpp @@ -228,9 +228,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) { @@ -257,7 +263,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 e307a0834ae5ec3a64e84f221750bc603bc152e5 Mon Sep 17 00:00:00 2001 From: David Spickett <[email protected]> Date: Thu, 25 Jun 2026 09:23:32 +0000 Subject: [PATCH 2/2] 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 ccb71beb58339..180afa17302bf 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" _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
