[lldb] Remove CommandReturnObject's SetError(StringRef)

[DavidSpickett]

Summary

Replacing existing uses with AppendError.

SetError is also part of the SBI API. This remains
but instead of calling the underlying SetError it
will call AppendError.

[DavidSpickett]

I'm not totally sure about adding AppendError to the API.

Should we (and where should I) document that AppendError expects a non empty message? On the other hand if you're using a release build the assert to check that isn't a factor, you'll just set eReturnStatusFailed and not add any error text. So it's not going to come up for a lot of users.

Or I could just leave the API as is, with SetError calling AppendError internally.

[teemperor]

We should never assert unless we have to within SB API calls as that takes down the whole debugging session and all this is directly available to users via the script interpreter. I didn't realize we exposed that function via the SB API when I looked at the callers of those functions in D104525, so that's a regression we should fix. I can make a patch for that.

Anyway, this LGTM but please drop the addition of AppendError from the SB API. Adding a permanent new function to the SB API requires a bit more review than NFC refactoring. So that's the way to go:

Or I could just leave the API as is, with SetError calling AppendError internally.

[DavidSpickett]

I was going to say we only would need to remove the assert for this one:

void CommandReturnObject::SetError(const Status &error,
                                   const char *fallback_error_cstr) {

Since SetError checks that the char * is not null in the API layer before calling SetError proper. However, the assert is that the string is not empty. You could pass in a non null ptr, to an empty string. So you're right, those need to be removed.

(I'll update this patch shortly)

[DavidSpickett]

Don't change the API, just have SetError call underlying
AppendError.

[teemperor]

LGTM, thanks!

lldb/include/lldb/Interpreter/CommandReturnObject.h

public:
 
   template <typename... Args>
   void AppendErrorWithFormatv(const char *format, Args &&... args) {
     AppendError(llvm::formatv(format, std::forward<Args>(args)...).str());
   }
 
   void SetError(const Status &error, const char *fallback_error_cstr = nullptr);
 
-  void SetError(llvm::StringRef error_cstr);
-
   lldb::ReturnStatus GetStatus();
 
   void SetStatus(lldb::ReturnStatus status);
 
   bool Succeeded();
 
   bool HasResult();
 

lldb/source/API/SBCommandReturnObject.cpp

else if (fallback_error_cstr)
     ref().SetError(Status(), fallback_error_cstr);
 }
 
 void SBCommandReturnObject::SetError(const char *error_cstr) {
   LLDB_RECORD_METHOD(void, SBCommandReturnObject, SetError, (const char *),
                      error_cstr);
 
   if (error_cstr)
-    ref().SetError(error_cstr);
+    ref().AppendError(error_cstr);
 }
 
 namespace lldb_private {
 namespace repro {
 
 template <>
 void RegisterMethods<SBCommandReturnObject>(Registry &R) {
   LLDB_REGISTER_CONSTRUCTOR(SBCommandReturnObject, ());

lldb/source/Commands/CommandObjectBreakpoint.cpp

CommandCompletions::InvokeCommonCompletionCallbacks(
         request, nullptr);
   }
 
   Options *GetOptions() override { return &m_option_group; }
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     if (!m_name_options.m_name.OptionWasSet()) {
-      result.SetError("No name option provided.");
+      result.AppendError("No name option provided.");
       return false;
     }
 
     Target &target =
         GetSelectedOrDummyTarget(m_name_options.m_use_dummy.GetCurrentValue());
 
     std::unique_lock<std::recursive_mutex> lock;
     target.GetBreakpointList().GetListMutex(lock);
 
     const BreakpointList &breakpoints = target.GetBreakpointList();
 
     size_t num_breakpoints = breakpoints.GetSize();
     if (num_breakpoints == 0) {
-      result.SetError("No breakpoints, cannot add names.");
+      result.AppendError("No breakpoints, cannot add names.");
       return false;
     }
 
     // Particular breakpoint selected; disable that breakpoint.
     BreakpointIDList valid_bp_ids;
     CommandObjectMultiwordBreakpoint::VerifyBreakpointIDs(
         command, &target, result, &valid_bp_ids,
         BreakpointName::Permissions::PermissionKinds::listPerm);
 
     if (result.Succeeded()) {
       if (valid_bp_ids.GetSize() == 0) {
-        result.SetError("No breakpoints specified, cannot add names.");
+        result.AppendError("No breakpoints specified, cannot add names.");
         return false;
       }
       size_t num_valid_ids = valid_bp_ids.GetSize();
       const char *bp_name = m_name_options.m_name.GetCurrentValue();
       Status error; // This error reports illegal names, but we've already
                     // checked that, so we don't need to check it again here.
       for (size_t index = 0; index < num_valid_ids; index++) {
         lldb::break_id_t bp_id =
CommandCompletions::InvokeCommonCompletionCallbacks(
         request, nullptr);
   }
 
   Options *GetOptions() override { return &m_option_group; }
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     if (!m_name_options.m_name.OptionWasSet()) {
-      result.SetError("No name option provided.");
+      result.AppendError("No name option provided.");
       return false;
     }
 
     Target &target =
         GetSelectedOrDummyTarget(m_name_options.m_use_dummy.GetCurrentValue());
 
     std::unique_lock<std::recursive_mutex> lock;
     target.GetBreakpointList().GetListMutex(lock);
 
     const BreakpointList &breakpoints = target.GetBreakpointList();
 
     size_t num_breakpoints = breakpoints.GetSize();
     if (num_breakpoints == 0) {
-      result.SetError("No breakpoints, cannot delete names.");
+      result.AppendError("No breakpoints, cannot delete names.");
       return false;
     }
 
     // Particular breakpoint selected; disable that breakpoint.
     BreakpointIDList valid_bp_ids;
     CommandObjectMultiwordBreakpoint::VerifyBreakpointIDs(
         command, &target, result, &valid_bp_ids,
         BreakpointName::Permissions::PermissionKinds::deletePerm);
 
     if (result.Succeeded()) {
       if (valid_bp_ids.GetSize() == 0) {
-        result.SetError("No breakpoints specified, cannot delete names.");
+        result.AppendError("No breakpoints specified, cannot delete names.");
         return false;
       }
       ConstString bp_name(m_name_options.m_name.GetCurrentValue());
       size_t num_valid_ids = valid_bp_ids.GetSize();
       for (size_t index = 0; index < num_valid_ids; index++) {
         lldb::break_id_t bp_id =
             valid_bp_ids.GetBreakpointIDAtIndex(index).GetBreakpointID();
         BreakpointSP bp_sp = breakpoints.FindBreakpointByID(bp_id);

lldb/source/Commands/CommandObjectMultiword.cpp

llvm::StringRef CommandObjectProxy::GetUnsupportedError() {
   return "command is not implemented";
 }
 
 bool CommandObjectProxy::Execute(const char *args_string,
                                  CommandReturnObject &result) {
   CommandObject *proxy_command = GetProxyCommandObject();
   if (proxy_command)
     return proxy_command->Execute(args_string, result);
-  result.SetError(GetUnsupportedError());
+  result.AppendError(GetUnsupportedError());
   return false;
 }

lldb/source/Commands/CommandObjectProcess.cpp

public:
   ~CommandObjectProcessTraceStop() override = default;
 
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     ProcessSP process_sp = m_exe_ctx.GetProcessSP();
 
     TraceSP trace_sp = process_sp->GetTarget().GetTrace();
 
     if (llvm::Error err = trace_sp->Stop())
-      result.SetError(toString(std::move(err)));
+      result.AppendError(toString(std::move(err)));
     else
       result.SetStatus(eReturnStatusSuccessFinishResult);
 
     return result.Succeeded();
   }
 };
 
 // CommandObjectMultiwordProcessTrace

lldb/source/Commands/CommandObjectReproducer.cpp

if (reproducer_path) {
     }
     return loader;
   }
 
   if (Loader *loader = Reproducer::Instance().GetLoader())
     return loader;
 
   // This is a soft error because this is expected to fail during capture.
-  result.SetError("Not specifying a reproducer is only support during replay.");
+  result.AppendError(
+      "Not specifying a reproducer is only support during replay.");
   result.SetStatus(eReturnStatusSuccessFinishNoResult);
   return nullptr;
 }
 
 class CommandObjectReproducerGenerate : public CommandObjectParsed {
 public:
   CommandObjectReproducerGenerate(CommandInterpreter &interpreter)
       : CommandObjectParsed(
if (!command.empty()) {
       result.AppendErrorWithFormat("'%s' takes no arguments",
                                    m_cmd_name.c_str());
       return false;
     }
 
     auto &r = Reproducer::Instance();
 
     if (!r.IsCapturing() && !r.IsReplaying()) {
-      result.SetError(
+      result.AppendError(
           "forcing a crash is only supported when capturing a reproducer.");
       result.SetStatus(eReturnStatusSuccessFinishNoResult);
       return false;
     }
 
     switch (m_options.signal) {
     case eReproducerCrashSigill:
       std::raise(SIGILL);
case eReproducerProviderProcessInfo: {
           return false;
         }
       }
 
       result.SetStatus(eReturnStatusSuccessFinishResult);
       return true;
     }
     case eReproducerProviderNone:
-      result.SetError("No valid provider specified.");
+      result.AppendError("No valid provider specified.");
       return false;
     }
 
     result.SetStatus(eReturnStatusSuccessFinishNoResult);
     return result.Succeeded();
   }
 
 private:

lldb/source/Commands/CommandObjectThread.cpp

if (command.GetArgumentCount() == 0 && m_options.m_tids.empty()) {
         Process *process = m_exe_ctx.GetProcessPtr();
         StreamString tmp_strm;
         for (lldb::tid_t tid : m_options.m_tids) {
           bool success = process->DumpThreadPlansForTID(
               tmp_strm, tid, eDescriptionLevelFull, m_options.m_internal,
               true /* condense_trivial */, m_options.m_unreported);
           // If we didn't find a TID, stop here and return an error.
           if (!success) {
-            result.SetError("Error dumping plans:");
+            result.AppendError("Error dumping plans:");
             result.AppendError(tmp_strm.GetString());
             return false;
           }
           // Otherwise, add our data to the output:
           result.GetOutputStream() << tmp_strm.GetString();
         }
       }
       return CommandObjectIterateOverThreads::DoExecute(command, result);
public:
 
   bool DoExecuteOnThreads(Args &command, CommandReturnObject &result,
                           llvm::ArrayRef<lldb::tid_t> tids) override {
     ProcessSP process_sp = m_exe_ctx.GetProcessSP();
 
     TraceSP trace_sp = process_sp->GetTarget().GetTrace();
 
     if (llvm::Error err = trace_sp->Stop(tids))
-      result.SetError(toString(std::move(err)));
+      result.AppendError(toString(std::move(err)));
     else
       result.SetStatus(eReturnStatusSuccessFinishResult);
 
     return result.Succeeded();
   }
 };
 
 // CommandObjectTraceDumpInstructions
bool HandleOneThread(lldb::tid_t tid, CommandReturnObject &result) override {
     ThreadSP thread_sp =
         m_exe_ctx.GetProcessPtr()->GetThreadList().FindThreadByID(tid);
 
     size_t count = m_options.m_count;
     ssize_t position = m_options.m_position.getValueOr(
                            trace_sp->GetCursorPosition(*thread_sp)) -
                        m_consecutive_repetitions * count;
     if (position < 0)
-      result.SetError("error: no more data");
+      result.AppendError("error: no more data");
     else
       trace_sp->DumpTraceInstructions(*thread_sp, result.GetOutputStream(),
                                       count, position, m_options.m_raw);
     return true;
   }
 
   CommandOptions m_options;
 

lldb/source/Commands/CommandObjectTrace.cpp

public:
   ~CommandObjectTraceSchema() override = default;
 
   Options *GetOptions() override { return &m_options; }
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     Status error;
     if (command.empty()) {
-      result.SetError(
+      result.AppendError(
           "trace schema cannot be invoked without a plug-in as argument");
       return false;
     }
 
     StringRef plugin_name(command[0].c_str());
     if (plugin_name == "all") {
       size_t index = 0;
       while (true) {

lldb/source/Commands/CommandObjectType.cpp

llvm::StringRef GetHelpLong() override {
 
     m_cmd_help_long = std::string(stream.GetString());
     return m_cmd_help_long;
   }
 
   bool DoExecute(llvm::StringRef raw_command_line,
                  CommandReturnObject &result) override {
     if (raw_command_line.empty()) {
-      result.SetError(
+      result.AppendError(
           "type lookup cannot be invoked without a type name as argument");
       return false;
     }
 
     auto exe_ctx = GetCommandInterpreter().GetExecutionContext();
     m_option_group.NotifyOptionParsingStarting(&exe_ctx);
 
     OptionsWithRaw args(raw_command_line);

lldb/source/Interpreter/CommandObject.cpp

if (process == nullptr) {
         }
       }
     }
   }
 
   if (GetFlags().Test(eCommandProcessMustBeTraced)) {
     Target *target = m_exe_ctx.GetTargetPtr();
     if (target && !target->GetTrace()) {
-      result.SetError("Process is not being traced.");
+      result.AppendError("Process is not being traced.");
       return false;
     }
   }
 
   return true;
 }
 
 void CommandObject::Cleanup() {

lldb/source/Interpreter/CommandReturnObject.cpp

void CommandReturnObject::AppendError(llvm::StringRef in_string) {
   SetStatus(eReturnStatusFailed);
   assert(!in_string.empty() && "Expected a non-empty error message");
   error(GetErrorStream()) << in_string.rtrim() << '\n';
 }
 
 void CommandReturnObject::SetError(const Status &error,
                                    const char *fallback_error_cstr) {
   assert(error.Fail() && "Expected a failed Status");
-  SetError(error.AsCString(fallback_error_cstr));
-}
-
-void CommandReturnObject::SetError(llvm::StringRef error_str) {
-  SetStatus(eReturnStatusFailed);
-  AppendError(error_str);
+  AppendError(error.AsCString(fallback_error_cstr));
 }
 
 // Similar to AppendError, but do not prepend 'Status: ' to message, and don't
 // append "\n" to the end of it.
 
 void CommandReturnObject::AppendRawError(llvm::StringRef in_string) {
   SetStatus(eReturnStatusFailed);
   assert(!in_string.empty() && "Expected a non-empty error message");