[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.

Author: JDevlieghere

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,

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);
     }

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;
 }

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,

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;

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")