llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: David Spickett (DavidSpickett) <details> <summary>Changes</summary> 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. --- Patch is 28.52 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/206479.diff 15 Files Affected: - (modified) lldb/include/lldb/Core/Debugger.h (+4-5) - (modified) lldb/include/lldb/Utility/Log.h (+10-6) - (modified) lldb/source/API/SBDebugger.cpp (+4-5) - (modified) lldb/source/Commands/CommandObjectLog.cpp (+9-9) - (modified) lldb/source/Core/Debugger.cpp (+12-12) - (modified) lldb/source/Utility/Log.cpp (+47-23) - (modified) lldb/test/API/commands/log/basic/TestLogging.py (+25) - (modified) lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py (+1-1) - (modified) lldb/test/Shell/lldb-server/TestGdbserverErrorMessages.test (+10) - (modified) lldb/test/Shell/lldb-server/TestPlatformErrorMessages.test (+7) - (modified) lldb/tools/lldb-server/LLDBServerUtilities.cpp (+6-7) - (modified) lldb/tools/lldb-server/lldb-gdbserver.cpp (+1-1) - (modified) lldb/tools/lldb-server/lldb-platform.cpp (+1-1) - (modified) lldb/tools/lldb-test/lldb-test.cpp (+4-1) - (modified) lldb/unittests/Utility/LogTest.cpp (+60-45) ``````````diff 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 9da33f5f39562..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, @@ -309,9 +308,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 error message in the case that + /// at least one category name was not recognised. All unknown category names + /// 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; void operator=(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 e51a85e9f0308..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,18 +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); - result.GetErrorStream() << error; + m_options.buffer_size.GetCurrentValue(), m_options.handler); - if (success) - result.SetStatus(eReturnStatusSuccessFinishNoResult); - else + 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 6b077a3766e9f..47697d6a5f44a 100644 --- a/lldb/source/Utility/Log.cpp +++ b/lldb/source/Utility/Log.cpp @@ -10,11 +10,13 @@ #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" #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" @@ -65,11 +67,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; +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; for (const char *category : categories) { if (llvm::StringRef("all").equals_insensitive(category)) { flags |= std::numeric_limits<Log::MaskType>::max(); @@ -87,12 +89,22 @@ 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) - ListCategories(stream, entry); + + if (unrecognized_categories.size()) { + 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; } @@ -205,21 +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; + if (iter == g_channel_map->end()) + return llvm::createStringErrorV("Invalid log channel '{0}'.\n", channel); + + if (categories.empty()) { + iter->second.Enable(log_handler_sp, std::nullopt, log_options); + return llvm::Error::success(); } - auto flags = categories.empty() ? std::optional<MaskType>{} - : GetFlags(error_stream, *iter, categories); + llvm::Expected<MaskType> flags = GetFlags(*iter, categories); + if (!flags) + return flags.takeError(); - iter->second.Enable(log_handler_sp, flags, log_options); - return true; + iter->second.Enable(log_handler_sp, *flags, log_options); + return llvm::Error::success(); } bool Log::DisableLogChannel(llvm::StringRef channel, @@ -231,10 +247,18 @@ bool Log::DisableLogChannel(llvm::StringRef channel, return false; } - auto flags = categories.empty() ? std::optional<MaskType>{} - : GetFlags(error_stream, *iter, categories); + if (categories.empty()) { + iter->second.Disable(std::nullopt); + return true; + } + + 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/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/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/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/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-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/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 602d019adac54..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_opti... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/206479 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
