llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: David Spickett (DavidSpickett) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/205561.diff 9 Files Affected: - (modified) lldb/include/lldb/Utility/Log.h (+8-3) - (modified) lldb/source/Commands/CommandObjectLog.cpp (+3-2) - (modified) lldb/source/Utility/Log.cpp (+30-12) - (modified) lldb/test/API/commands/log/basic/TestLogging.py (+25) - (modified) lldb/test/Shell/lldb-server/TestGdbserverErrorMessages.test (+10) - (modified) lldb/test/Shell/lldb-server/TestPlatformErrorMessages.test (+7) - (modified) lldb/tools/lldb-server/lldb-gdbserver.cpp (+1-1) - (modified) lldb/tools/lldb-server/lldb-platform.cpp (+1-1) - (modified) lldb/unittests/Utility/LogTest.cpp (+8-2) ``````````diff 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 1921573996196..6ab1d8ca563aa 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)); `````````` </details> https://github.com/llvm/llvm-project/pull/205561 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
