[Lldb-commits] [lldb] 6deee0d - [lldb] Use llvm::Error instead of CommandReturnObject for error reporting (#125125)
via lldb-commits
lldb-commits at lists.llvm.org
Fri Jan 31 13:23:29 PST 2025
Author: Jonas Devlieghere
Date: 2025-01-31T13:23:26-08:00
New Revision: 6deee0d5b36c8b4b83209759df8d4933e4922bc8
URL: https://github.com/llvm/llvm-project/commit/6deee0d5b36c8b4b83209759df8d4933e4922bc8
DIFF: https://github.com/llvm/llvm-project/commit/6deee0d5b36c8b4b83209759df8d4933e4922bc8.diff
LOG: [lldb] Use llvm::Error instead of CommandReturnObject for error reporting (#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.
Added:
Modified:
lldb/include/lldb/Interpreter/Options.h
lldb/source/Interpreter/CommandAlias.cpp
lldb/source/Interpreter/CommandObject.cpp
lldb/source/Interpreter/Options.cpp
lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
lldb/test/API/functionalities/abbreviation/TestAbbreviations.py
Removed:
################################################################################
diff --git a/lldb/include/lldb/Interpreter/Options.h b/lldb/include/lldb/Interpreter/Options.h
index 9a6a17c2793fa4c..864bda6f24c8cc5 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 c5971b52f837faf..b45fcca358a5ee2 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);
}
diff --git a/lldb/source/Interpreter/CommandObject.cpp b/lldb/source/Interpreter/CommandObject.cpp
index 559e2e7a76dd992..7008253e32bca19 100644
--- a/lldb/source/Interpreter/CommandObject.cpp
+++ b/lldb/source/Interpreter/CommandObject.cpp
@@ -120,17 +120,24 @@ bool CommandObject::ParseOptions(Args &args, CommandReturnObject &result) {
if (args_or) {
args = std::move(*args_or);
error = options->NotifyOptionParsingFinished(&exe_ctx);
- } else
+ } else {
error = Status::FromError(args_or.takeError());
+ }
- if (error.Success()) {
- if (options->VerifyOptions(result))
- return true;
- } else {
+ if (error.Fail()) {
result.SetError(error.takeError());
+ result.SetStatus(eReturnStatusFailed);
+ return false;
}
- result.SetStatus(eReturnStatusFailed);
- return false;
+
+ if (llvm::Error error = options->VerifyOptions()) {
+ result.SetError(std::move(error));
+ result.SetStatus(eReturnStatusFailed);
+ return false;
+ }
+
+ result.SetStatus(eReturnStatusSuccessFinishNoResult);
+ return true;
}
return true;
}
diff --git a/lldb/source/Interpreter/Options.cpp b/lldb/source/Interpreter/Options.cpp
index 893a3b71604ba8f..fdadba62987d362 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
diff erence: 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
diff erence: 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 4ca8bd2f9085df8..82f18c5fe37a216 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;
diff --git a/lldb/test/API/functionalities/abbreviation/TestAbbreviations.py b/lldb/test/API/functionalities/abbreviation/TestAbbreviations.py
index b4e770af0e392c7..cc767edaaa619eb 100644
--- a/lldb/test/API/functionalities/abbreviation/TestAbbreviations.py
+++ b/lldb/test/API/functionalities/abbreviation/TestAbbreviations.py
@@ -94,6 +94,12 @@ def test_command_abbreviations_and_aliases(self):
self.assertTrue(result.Succeeded())
self.assertEqual("scripting run 1+1", result.GetOutput())
+ # Name and line are incompatible options.
+ command_interpreter.HandleCommand(
+ "alias zzyx breakpoint set -n %1 -l %2", result
+ )
+ self.assertFalse(result.Succeeded())
+
# Prompt changing stuff should be tested, but this doesn't seem like the
# right test to do it in. It has nothing to do with aliases or abbreviations.
# self.runCmd("com sou ./change_prompt.lldb")
More information about the lldb-commits
mailing list