[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
Fri Jan 31 10:57:23 PST 2025
https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/125125
>From 24ddc550e3ee61b863cbaea05ff49981bc20f7ad Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Thu, 30 Jan 2025 14:25:16 -0800
Subject: [PATCH 1/4] [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 | 49 +++++------
lldb/source/Interpreter/CommandObject.cpp | 2 +-
lldb/source/Interpreter/Options.cpp | 87 ++++++++++---------
.../DarwinLog/StructuredDataDarwinLog.cpp | 9 +-
5 files changed, 77 insertions(+), 74 deletions(-)
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..ac0c7a3279e64b9 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,
+static llvm::Error
+ProcessAliasOptionsArgs(lldb::CommandObjectSP &cmd_obj_sp,
llvm::StringRef options_args,
OptionArgVectorSP &option_arg_vector_sp) {
- bool success = true;
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,
+ 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..4f21c75c2c136af 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());
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 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 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;
>From 29ebafd86dcbb8a22542cab3abc40174e021f65d Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Thu, 30 Jan 2025 14:33:58 -0800
Subject: [PATCH 2/4] Fix formatting
---
lldb/source/Interpreter/CommandAlias.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lldb/source/Interpreter/CommandAlias.cpp b/lldb/source/Interpreter/CommandAlias.cpp
index ac0c7a3279e64b9..b45fcca358a5ee2 100644
--- a/lldb/source/Interpreter/CommandAlias.cpp
+++ b/lldb/source/Interpreter/CommandAlias.cpp
@@ -23,8 +23,8 @@ using namespace lldb_private;
static llvm::Error
ProcessAliasOptionsArgs(lldb::CommandObjectSP &cmd_obj_sp,
- llvm::StringRef options_args,
- OptionArgVectorSP &option_arg_vector_sp) {
+ llvm::StringRef options_args,
+ OptionArgVectorSP &option_arg_vector_sp) {
OptionArgVector *option_arg_vector = option_arg_vector_sp.get();
if (options_args.size() < 1)
@@ -61,7 +61,7 @@ ProcessAliasOptionsArgs(lldb::CommandObjectSP &cmd_obj_sp,
if (!entry.ref().empty())
option_arg_vector->emplace_back(
std::string(CommandInterpreter::g_argument), -1,
- std::string(entry.ref()));
+ std::string(entry.ref()));
}
}
}
>From 5ede4321a6e901f7fe7d42ccd59cb470b84715c5 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Thu, 30 Jan 2025 14:42:04 -0800
Subject: [PATCH 3/4] Fix boolean inversion
---
lldb/source/Interpreter/CommandObject.cpp | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/lldb/source/Interpreter/CommandObject.cpp b/lldb/source/Interpreter/CommandObject.cpp
index 4f21c75c2c136af..9ffa5a8ba71188c 100644
--- a/lldb/source/Interpreter/CommandObject.cpp
+++ b/lldb/source/Interpreter/CommandObject.cpp
@@ -124,11 +124,14 @@ bool CommandObject::ParseOptions(Args &args, CommandReturnObject &result) {
error = Status::FromError(args_or.takeError());
if (error.Success()) {
- if (options->VerifyOptions())
- return true;
- } else {
- result.SetError(error.takeError());
+ if (llvm::Error error = options->VerifyOptions()) {
+ result.SetError(std::move(error));
+ return false;
+ }
+ return true;
}
+
+ result.SetError(error.takeError());
result.SetStatus(eReturnStatusFailed);
return false;
}
>From f3a12ba104134c42661bbafc4ad9211e8ce9dfb1 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Fri, 31 Jan 2025 10:47:53 -0800
Subject: [PATCH 4/4] Add missing eReturnStatusSuccessFinishNoResult
---
lldb/source/Interpreter/CommandObject.cpp | 24 +++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/lldb/source/Interpreter/CommandObject.cpp b/lldb/source/Interpreter/CommandObject.cpp
index 9ffa5a8ba71188c..7008253e32bca19 100644
--- a/lldb/source/Interpreter/CommandObject.cpp
+++ b/lldb/source/Interpreter/CommandObject.cpp
@@ -120,20 +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 (llvm::Error error = options->VerifyOptions()) {
- result.SetError(std::move(error));
- return false;
- }
- return true;
+ if (error.Fail()) {
+ result.SetError(error.takeError());
+ result.SetStatus(eReturnStatusFailed);
+ return false;
}
- result.SetError(error.takeError());
- 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;
}
More information about the lldb-commits
mailing list