[Lldb-commits] [lldb] [lldb] Use llvm::Error instead of CommandReturnObject for error reporting (PR #125125)
Jonas Devlieghere via lldb-commits
lldb-commits at lists.llvm.org
Thu Jan 30 14:23:56 PST 2025
https://github.com/JDevlieghere created https://github.com/llvm/llvm-project/pull/125125
Use `llvm::Error` instead of `CommandReturnObject` for error reporting. The command return objects were populated with errors but never displayed. With this patch they're at least logged.
>From 218db812d58dcc34d841fc812b2dbd02bb3c123a Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Thu, 30 Jan 2025 14:22:21 -0800
Subject: [PATCH] [lldb] Use llvm::Error instead of CommandReturnObject for
error reporting
Use `llvm::Error` instead of `CommandReturnObject` for error reporting.
The command return objects were populated with errors but never
displayed. With this patch they're at least logged.
---
lldb/include/lldb/Interpreter/Options.h | 4 +-
lldb/source/Interpreter/CommandAlias.cpp | 59 +++++++------
lldb/source/Interpreter/CommandObject.cpp | 30 +++----
lldb/source/Interpreter/Options.cpp | 87 ++++++++++---------
.../DarwinLog/StructuredDataDarwinLog.cpp | 9 +-
5 files changed, 96 insertions(+), 93 deletions(-)
diff --git a/lldb/include/lldb/Interpreter/Options.h b/lldb/include/lldb/Interpreter/Options.h
index 9a6a17c2793fa4..864bda6f24c8cc 100644
--- a/lldb/include/lldb/Interpreter/Options.h
+++ b/lldb/include/lldb/Interpreter/Options.h
@@ -76,12 +76,12 @@ class Options {
// This gets passed the short option as an integer...
void OptionSeen(int short_option);
- bool VerifyOptions(CommandReturnObject &result);
+ llvm::Error VerifyOptions();
// Verify that the options given are in the options table and can be used
// together, but there may be some required options that are missing (used to
// verify options that get folded into command aliases).
- bool VerifyPartialOptions(CommandReturnObject &result);
+ llvm::Error VerifyPartialOptions();
void OutputFormattedUsageText(Stream &strm,
const OptionDefinition &option_def,
diff --git a/lldb/source/Interpreter/CommandAlias.cpp b/lldb/source/Interpreter/CommandAlias.cpp
index c5971b52f837fa..b29b06c48c84f5 100644
--- a/lldb/source/Interpreter/CommandAlias.cpp
+++ b/lldb/source/Interpreter/CommandAlias.cpp
@@ -10,6 +10,7 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FormatAdapters.h"
#include "lldb/Interpreter/CommandInterpreter.h"
#include "lldb/Interpreter/CommandObject.h"
@@ -20,20 +21,17 @@
using namespace lldb;
using namespace lldb_private;
-static bool ProcessAliasOptionsArgs(lldb::CommandObjectSP &cmd_obj_sp,
- llvm::StringRef options_args,
- OptionArgVectorSP &option_arg_vector_sp) {
- bool success = true;
+static llvm::Error
+ProcessAliasOptionsArgs(lldb::CommandObjectSP &cmd_obj_sp,
+ llvm::StringRef options_args,
+ OptionArgVectorSP &option_arg_vector_sp) {
OptionArgVector *option_arg_vector = option_arg_vector_sp.get();
if (options_args.size() < 1)
- return true;
+ return llvm::Error::success();
Args args(options_args);
std::string options_string(options_args);
- // TODO: Find a way to propagate errors in this CommandReturnObject up the
- // stack.
- CommandReturnObject result(false);
// Check to see if the command being aliased can take any command options.
Options *options = cmd_obj_sp->GetOptions();
if (options) {
@@ -45,34 +43,30 @@ static bool ProcessAliasOptionsArgs(lldb::CommandObjectSP &cmd_obj_sp,
llvm::Expected<Args> args_or =
options->ParseAlias(args, option_arg_vector, options_string);
- if (!args_or) {
- result.AppendError(toString(args_or.takeError()));
- result.AppendError("Unable to create requested alias.\n");
- return false;
- }
+ if (!args_or)
+ return llvm::createStringError(
+ llvm::formatv("unable to create alias: {0}",
+ llvm::fmt_consume(args_or.takeError())));
args = std::move(*args_or);
- options->VerifyPartialOptions(result);
- if (!result.Succeeded() &&
- result.GetStatus() != lldb::eReturnStatusStarted) {
- result.AppendError("Unable to create requested alias.\n");
- return false;
- }
+ if (llvm::Error error = options->VerifyPartialOptions())
+ return error;
}
if (!options_string.empty()) {
- if (cmd_obj_sp->WantsRawCommandString())
- option_arg_vector->emplace_back(CommandInterpreter::g_argument,
- -1, options_string);
- else {
+ if (cmd_obj_sp->WantsRawCommandString()) {
+ option_arg_vector->emplace_back(CommandInterpreter::g_argument, -1,
+ options_string);
+ } else {
for (auto &entry : args.entries()) {
if (!entry.ref().empty())
- option_arg_vector->emplace_back(std::string(CommandInterpreter::g_argument), -1,
- std::string(entry.ref()));
+ option_arg_vector->emplace_back(
+ std::string(CommandInterpreter::g_argument), -1,
+ std::string(entry.ref()));
}
}
}
- return success;
+ return llvm::Error::success();
}
CommandAlias::CommandAlias(CommandInterpreter &interpreter,
@@ -85,10 +79,15 @@ CommandAlias::CommandAlias(CommandInterpreter &interpreter,
m_option_args_sp(new OptionArgVector),
m_is_dashdash_alias(eLazyBoolCalculate), m_did_set_help(false),
m_did_set_help_long(false) {
- if (ProcessAliasOptionsArgs(cmd_sp, options_args, m_option_args_sp)) {
+ if (llvm::Error error =
+ ProcessAliasOptionsArgs(cmd_sp, options_args, m_option_args_sp)) {
+ // FIXME: Find a way to percolate this error up.
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Host), std::move(error),
+ "ProcessAliasOptionsArgs failed: {0}");
+ } else {
m_underlying_command_sp = cmd_sp;
for (int i = 0;
- auto cmd_entry = m_underlying_command_sp->GetArgumentEntryAtIndex(i);
+ auto *cmd_entry = m_underlying_command_sp->GetArgumentEntryAtIndex(i);
i++) {
m_arguments.push_back(*cmd_entry);
}
@@ -159,8 +158,8 @@ void CommandAlias::GetAliasExpansion(StreamString &help_string) const {
help_string.Printf(" %s", value.c_str());
} else {
help_string.Printf(" %s", opt.c_str());
- if ((value != CommandInterpreter::g_no_argument)
- && (value != CommandInterpreter::g_need_argument)) {
+ if ((value != CommandInterpreter::g_no_argument) &&
+ (value != CommandInterpreter::g_need_argument)) {
help_string.Printf(" %s", value.c_str());
}
}
diff --git a/lldb/source/Interpreter/CommandObject.cpp b/lldb/source/Interpreter/CommandObject.cpp
index 559e2e7a76dd99..bd0a792b450ca7 100644
--- a/lldb/source/Interpreter/CommandObject.cpp
+++ b/lldb/source/Interpreter/CommandObject.cpp
@@ -124,7 +124,7 @@ bool CommandObject::ParseOptions(Args &args, CommandReturnObject &result) {
error = Status::FromError(args_or.takeError());
if (error.Success()) {
- if (options->VerifyOptions(result))
+ if (options->VerifyOptions())
return true;
} else {
result.SetError(error.takeError());
@@ -306,7 +306,7 @@ void CommandObject::HandleArgumentCompletion(
assert(entry_ptr && "We said there was one entry, but there wasn't.");
return; // Not worth crashing if asserts are off...
}
-
+
CommandArgumentEntry &entry = *entry_ptr;
// For now, we only handle the simple case of one homogenous argument type.
if (entry.size() != 1)
@@ -492,21 +492,21 @@ bool CommandObject::IsPairType(ArgumentRepetitionType arg_repeat_type) {
(arg_repeat_type == eArgRepeatPairRangeOptional);
}
-std::optional<ArgumentRepetitionType>
+std::optional<ArgumentRepetitionType>
CommandObject::ArgRepetitionFromString(llvm::StringRef string) {
return llvm::StringSwitch<ArgumentRepetitionType>(string)
- .Case("plain", eArgRepeatPlain)
- .Case("optional", eArgRepeatOptional)
- .Case("plus", eArgRepeatPlus)
- .Case("star", eArgRepeatStar)
- .Case("range", eArgRepeatRange)
- .Case("pair-plain", eArgRepeatPairPlain)
- .Case("pair-optional", eArgRepeatPairOptional)
- .Case("pair-plus", eArgRepeatPairPlus)
- .Case("pair-star", eArgRepeatPairStar)
- .Case("pair-range", eArgRepeatPairRange)
- .Case("pair-range-optional", eArgRepeatPairRangeOptional)
- .Default({});
+ .Case("plain", eArgRepeatPlain)
+ .Case("optional", eArgRepeatOptional)
+ .Case("plus", eArgRepeatPlus)
+ .Case("star", eArgRepeatStar)
+ .Case("range", eArgRepeatRange)
+ .Case("pair-plain", eArgRepeatPairPlain)
+ .Case("pair-optional", eArgRepeatPairOptional)
+ .Case("pair-plus", eArgRepeatPairPlus)
+ .Case("pair-star", eArgRepeatPairStar)
+ .Case("pair-range", eArgRepeatPairRange)
+ .Case("pair-range-optional", eArgRepeatPairRangeOptional)
+ .Default({});
}
static CommandObject::CommandArgumentEntry
diff --git a/lldb/source/Interpreter/Options.cpp b/lldb/source/Interpreter/Options.cpp
index 893a3b71604ba8..fdadba62987d36 100644
--- a/lldb/source/Interpreter/Options.cpp
+++ b/lldb/source/Interpreter/Options.cpp
@@ -138,46 +138,6 @@ void Options::OptionsSetUnion(const OptionSet &set_a, const OptionSet &set_b,
}
}
-bool Options::VerifyOptions(CommandReturnObject &result) {
- bool options_are_valid = false;
-
- int num_levels = GetRequiredOptions().size();
- if (num_levels) {
- for (int i = 0; i < num_levels && !options_are_valid; ++i) {
- // This is the correct set of options if: 1). m_seen_options contains
- // all of m_required_options[i] (i.e. all the required options at this
- // level are a subset of m_seen_options); AND 2). { m_seen_options -
- // m_required_options[i] is a subset of m_options_options[i] (i.e. all
- // the rest of m_seen_options are in the set of optional options at this
- // level.
-
- // Check to see if all of m_required_options[i] are a subset of
- // m_seen_options
- if (IsASubset(GetRequiredOptions()[i], m_seen_options)) {
- // Construct the set difference: remaining_options = {m_seen_options} -
- // {m_required_options[i]}
- OptionSet remaining_options;
- OptionsSetDiff(m_seen_options, GetRequiredOptions()[i],
- remaining_options);
- // Check to see if remaining_options is a subset of
- // m_optional_options[i]
- if (IsASubset(remaining_options, GetOptionalOptions()[i]))
- options_are_valid = true;
- }
- }
- } else {
- options_are_valid = true;
- }
-
- if (options_are_valid) {
- result.SetStatus(eReturnStatusSuccessFinishNoResult);
- } else {
- result.AppendError("invalid combination of options for the given command");
- }
-
- return options_are_valid;
-}
-
// This is called in the Options constructor, though we could call it lazily if
// that ends up being a performance problem.
@@ -590,13 +550,50 @@ void Options::GenerateOptionUsage(Stream &strm, CommandObject &cmd,
strm.SetIndentLevel(save_indent_level);
}
+llvm::Error Options::VerifyOptions() {
+ bool options_are_valid = false;
+
+ int num_levels = GetRequiredOptions().size();
+ if (num_levels) {
+ for (int i = 0; i < num_levels && !options_are_valid; ++i) {
+ // This is the correct set of options if: 1). m_seen_options contains
+ // all of m_required_options[i] (i.e. all the required options at this
+ // level are a subset of m_seen_options); AND 2). { m_seen_options -
+ // m_required_options[i] is a subset of m_options_options[i] (i.e. all
+ // the rest of m_seen_options are in the set of optional options at this
+ // level.
+
+ // Check to see if all of m_required_options[i] are a subset of
+ // m_seen_options
+ if (IsASubset(GetRequiredOptions()[i], m_seen_options)) {
+ // Construct the set difference: remaining_options = {m_seen_options} -
+ // {m_required_options[i]}
+ OptionSet remaining_options;
+ OptionsSetDiff(m_seen_options, GetRequiredOptions()[i],
+ remaining_options);
+ // Check to see if remaining_options is a subset of
+ // m_optional_options[i]
+ if (IsASubset(remaining_options, GetOptionalOptions()[i]))
+ options_are_valid = true;
+ }
+ }
+ } else {
+ options_are_valid = true;
+ }
+
+ if (!options_are_valid)
+ return llvm::createStringError(
+ "invalid combination of options for the given command");
+
+ return llvm::Error::success();
+}
+
// This function is called when we have been given a potentially incomplete set
// of options, such as when an alias has been defined (more options might be
// added at at the time the alias is invoked). We need to verify that the
// options in the set m_seen_options are all part of a set that may be used
// together, but m_seen_options may be missing some of the "required" options.
-
-bool Options::VerifyPartialOptions(CommandReturnObject &result) {
+llvm::Error Options::VerifyPartialOptions() {
bool options_are_valid = false;
int num_levels = GetRequiredOptions().size();
@@ -613,7 +610,11 @@ bool Options::VerifyPartialOptions(CommandReturnObject &result) {
}
}
- return options_are_valid;
+ if (!options_are_valid)
+ return llvm::createStringError(
+ "invalid combination of options for the given command");
+
+ return llvm::Error::success();
}
bool Options::HandleOptionCompletion(CompletionRequest &request,
diff --git a/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp b/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
index 4ca8bd2f9085df..82f18c5fe37a21 100644
--- a/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
+++ b/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
@@ -975,8 +975,6 @@ EnableOptionsSP ParseAutoEnableOptions(Status &error, Debugger &debugger) {
EnableOptionsSP options_sp(new EnableOptions());
options_sp->NotifyOptionParsingStarting(&exe_ctx);
- CommandReturnObject result(debugger.GetUseColor());
-
// Parse the arguments.
auto options_property_sp =
debugger.GetPropertyValue(nullptr,
@@ -1013,8 +1011,13 @@ EnableOptionsSP ParseAutoEnableOptions(Status &error, Debugger &debugger) {
return EnableOptionsSP();
}
- if (!options_sp->VerifyOptions(result))
+ if (llvm::Error error = options_sp->VerifyOptions()) {
+ LLDB_LOG_ERROR(
+ log, std::move(error),
+ "Parsing plugin.structured-data.darwin-log.auto-enable-options value "
+ "failed: {0}");
return EnableOptionsSP();
+ }
// We successfully parsed and validated the options.
return options_sp;
More information about the lldb-commits
mailing list