[lldb] Make deep copies of Status explicit (NFC)

lldb/bindings/python/python-swigsafecast.swig

@@ -33,8 +33,8 @@ PythonObject SWIGBridge::ToSWIGWrapper(lldb::BreakpointSP breakpoint_sp) {
                       SWIGTYPE_p_lldb__SBBreakpoint);
 }
 
-PythonObject SWIGBridge::ToSWIGWrapper(const Status& status) {
-  return ToSWIGHelper(new lldb::SBError(status), SWIGTYPE_p_lldb__SBError);
+PythonObject SWIGBridge::ToSWIGWrapper(Status status) {
+  return ToSWIGHelper(new lldb::SBError(std::move(status)), SWIGTYPE_p_lldb__SBError);
 }
 
 PythonObject SWIGBridge::ToSWIGWrapper(std::unique_ptr<lldb::SBStructuredData> data_sb) {

lldb/include/lldb/API/SBError.h

@@ -97,7 +97,7 @@ class LLDB_API SBError {
   friend class lldb_private::ScriptInterpreter;
   friend class lldb_private::python::SWIGBridge;
 
-  SBError(const lldb_private::Status &error);
+  SBError(lldb_private::Status &&error);
 
   lldb_private::Status *get();
 
@@ -107,7 +107,7 @@ class LLDB_API SBError {
 
   lldb_private::Status &ref();
 
-  void SetError(const lldb_private::Status &lldb_error);
+  void SetError(lldb_private::Status &&lldb_error);
 
 private:
   std::unique_ptr<lldb_private::Status> m_opaque_up;

lldb/include/lldb/API/SBValueList.h

@@ -96,7 +96,7 @@ class LLDB_API SBValueList {
 
   std::unique_ptr<ValueListImpl> m_opaque_up;
 
-  void SetError(const lldb_private::Status &status);
+  void SetError(lldb_private::Status &&status);
 };
 
 } // namespace lldb

lldb/include/lldb/Core/ValueObjectConstResult.h

@@ -61,7 +61,7 @@ class ValueObjectConstResult : public ValueObject {
 
   // When an expression fails to evaluate, we return an error
   static lldb::ValueObjectSP Create(ExecutionContextScope *exe_scope,
-                                    const Status &error);
+                                    Status &&error);
 
   std::optional<uint64_t> GetByteSize() override;
 
@@ -146,7 +146,7 @@ class ValueObjectConstResult : public ValueObject {
                          ConstString name, Module *module = nullptr);
 
   ValueObjectConstResult(ExecutionContextScope *exe_scope,
-                         ValueObjectManager &manager, const Status &error);
+                         ValueObjectManager &manager, Status &&error);
 
   ValueObject *CreateChildAtIndex(size_t idx) override {
     return m_impl.CreateChildAtIndex(idx);

lldb/include/lldb/Utility/Status.h

@@ -43,13 +43,32 @@ const char *ExpressionResultAsCString(lldb::ExpressionResults result);
 /// of themselves for printing results and error codes. The string value will
 /// be fetched on demand and its string value will be cached until the error
 /// is cleared of the value of the error changes.
+///
+/// API design notes:
+///
+/// Most APIs that currently vend a Status would be better served by
+/// returning llvm::Expected<> instead. If possibles APIs should be
+/// refactored to avoid Status. The only legitimate long-term uses of
+/// Status are objects that need to store an error for a long time
+/// (which should be questioned as a design decision, too).
+///
+/// Implementation notes:
+///
+/// Internally, Status stores an llvm::Error.
+///   eErrorTypeInvalid
+///   eErrorTypeGeneric      llvm::StringError
+///   eErrorTypePOSIX        llvm::ECError
+///   eErrorTypeMachKernel   MachKernelError
+///   eErrorTypeExpression   llvm::ErrorList<ExpressionError>
+///   eErrorTypeWin32        Win32Error
+
 class Status {
 public:
-  /// Every error value that this object can contain needs to be able to fit
   /// into ValueType.
   typedef uint32_t ValueType;
 
   Status();
+  Status(Status &&other) = default;
 
   /// Initialize the error object with a generic success value.
   ///
@@ -93,10 +112,14 @@ class Status {
 
   ~Status();
 
+  const Status &operator=(Status &&);
   /// Avoid using this in new code. Migrate APIs to llvm::Expected instead.
   static Status FromError(llvm::Error error);
-  /// FIXME: Replace this with a takeError method.
+  /// FIXME: Replace this with a takeError() method.
   llvm::Error ToError() const;
+  /// Don't call this function in new code. Instead, redesign the API
+  /// to use llvm::Expected instead of Status.
+  Status Clone() const { return Status(ToError()); }
 
   /// Get the error string associated with the current error.
   //

lldb/source/API/SBBreakpoint.cpp

@@ -622,7 +622,7 @@ SBError SBBreakpoint::SetScriptCallbackFunction(
                                                callback_function_name,
                                                extra_args.m_impl_up
                                                    ->GetObjectSP());
-    sb_error.SetError(error);
+    sb_error.SetError(std::move(error));
   } else
     sb_error = Status::FromErrorString("invalid breakpoint");
 
@@ -645,7 +645,7 @@ SBError SBBreakpoint::SetScriptCallbackBody(const char *callback_body_text) {
             .GetScriptInterpreter()
             ->SetBreakpointCommandCallback(bp_options, callback_body_text,
                                            /*is_callback=*/false);
-    sb_error.SetError(error);
+    sb_error.SetError(std::move(error));
   } else
     sb_error = Status::FromErrorString("invalid breakpoint");
 
@@ -670,7 +670,7 @@ SBError SBBreakpoint::AddNameWithErrorHandling(const char *new_name) {
         bkpt_sp->GetTarget().GetAPIMutex());
     Status error;
     bkpt_sp->GetTarget().AddNameToBreakpoint(bkpt_sp, new_name, error);
-    status.SetError(error);
+    status.SetError(std::move(error));
   } else {
     status = Status::FromErrorString("invalid breakpoint");
   }

lldb/source/API/SBBreakpointLocation.cpp

@@ -239,7 +239,7 @@ SBError SBBreakpointLocation::SetScriptCallbackFunction(
                                                callback_function_name,
                                                extra_args.m_impl_up
                                                    ->GetObjectSP());
-      sb_error.SetError(error);
+    sb_error.SetError(std::move(error));
     } else
       sb_error = Status::FromErrorString("invalid breakpoint");
 
@@ -264,7 +264,7 @@ SBBreakpointLocation::SetScriptCallbackBody(const char *callback_body_text) {
             .GetScriptInterpreter()
             ->SetBreakpointCommandCallback(bp_options, callback_body_text,
                                            /*is_callback=*/false);
-    sb_error.SetError(error);
+    sb_error.SetError(std::move(error));
   } else
     sb_error = Status::FromErrorString("invalid breakpoint");
 

lldb/source/API/SBBreakpointName.cpp

@@ -570,14 +570,13 @@ SBError SBBreakpointName::SetScriptCallbackFunction(
         m_impl_up->GetTarget()->GetAPIMutex());
 
   BreakpointOptions &bp_options = bp_name->GetOptions();
-  Status error;
-  error = m_impl_up->GetTarget()
-              ->GetDebugger()
-              .GetScriptInterpreter()
-              ->SetBreakpointCommandCallbackFunction(
-                  bp_options, callback_function_name,
-                  extra_args.m_impl_up->GetObjectSP());
-  sb_error.SetError(error);
+  Status error = m_impl_up->GetTarget()
+                     ->GetDebugger()
+                     .GetScriptInterpreter()
+                     ->SetBreakpointCommandCallbackFunction(
+                         bp_options, callback_function_name,
+                         extra_args.m_impl_up->GetObjectSP());
+  sb_error.SetError(std::move(error));
   UpdateName(*bp_name);
   return sb_error;
 }
@@ -600,7 +599,7 @@ SBBreakpointName::SetScriptCallbackBody(const char *callback_body_text) {
                      .GetScriptInterpreter()
                      ->SetBreakpointCommandCallback(
                          bp_options, callback_body_text, /*is_callback=*/false);
-  sb_error.SetError(error);
+  sb_error.SetError(std::move(error));
   if (!sb_error.Fail())
     UpdateName(*bp_name);
 

lldb/source/API/SBDebugger.cpp

@@ -1360,7 +1360,7 @@ SBError SBDebugger::SetInternalVariable(const char *var_name, const char *value,
         "invalid debugger instance name '%s'", debugger_instance_name);
   }
   if (error.Fail())
-    sb_error.SetError(error);
+    sb_error.SetError(std::move(error));
   return sb_error;
 }
 

lldb/source/API/SBError.cpp

@@ -23,7 +23,8 @@ SBError::SBError() { LLDB_INSTRUMENT_VA(this); }
 SBError::SBError(const SBError &rhs) {
   LLDB_INSTRUMENT_VA(this, rhs);
 
-  m_opaque_up = clone(rhs.m_opaque_up);
+  if (rhs.m_opaque_up)
+    m_opaque_up = std::make_unique<Status>(rhs.m_opaque_up->Clone());
 }
 
 SBError::SBError(const char *message) {
@@ -32,8 +33,8 @@ SBError::SBError(const char *message) {
   SetErrorString(message);
 }
 
-SBError::SBError(const lldb_private::Status &status)
-    : m_opaque_up(new Status(status)) {
+SBError::SBError(lldb_private::Status &&status)
+    : m_opaque_up(new Status(std::move(status))) {
   LLDB_INSTRUMENT_VA(this, status);
 }
 
@@ -43,7 +44,9 @@ const SBError &SBError::operator=(const SBError &rhs) {
   LLDB_INSTRUMENT_VA(this, rhs);
 
   if (this != &rhs)
-    m_opaque_up = clone(rhs.m_opaque_up);
+    if (rhs.m_opaque_up)
+      m_opaque_up = std::make_unique<Status>(rhs.m_opaque_up->Clone());
+
   return *this;
 }
 
@@ -111,9 +114,9 @@ void SBError::SetError(uint32_t err, ErrorType type) {
   *m_opaque_up = Status(err, type);
 }
 
-void SBError::SetError(const Status &lldb_error) {
+void SBError::SetError(Status &&lldb_error) {
   CreateIfNeeded();
-  *m_opaque_up = lldb_error;
+  *m_opaque_up = std::move(lldb_error);
 }
 
 void SBError::SetErrorToErrno() {

lldb/source/API/SBFile.cpp

@@ -62,8 +62,7 @@ SBError SBFile::Read(uint8_t *buf, size_t num_bytes, size_t *bytes_read) {
     error = Status::FromErrorString("invalid SBFile");
     *bytes_read = 0;
   } else {
-    Status status = m_opaque_sp->Read(buf, num_bytes);
-    error.SetError(status);
+    error.SetError(m_opaque_sp->Read(buf, num_bytes));
     *bytes_read = num_bytes;
   }
   return error;
@@ -78,8 +77,7 @@ SBError SBFile::Write(const uint8_t *buf, size_t num_bytes,
     error = Status::FromErrorString("invalid SBFile");
     *bytes_written = 0;
   } else {
-    Status status = m_opaque_sp->Write(buf, num_bytes);
-    error.SetError(status);
+    error.SetError(m_opaque_sp->Write(buf, num_bytes));
     *bytes_written = num_bytes;
   }
   return error;
@@ -92,8 +90,7 @@ SBError SBFile::Flush() {
   if (!m_opaque_sp) {
     error = Status::FromErrorString("invalid SBFile");
   } else {
-    Status status = m_opaque_sp->Flush();
-    error.SetError(status);
+    error.SetError(m_opaque_sp->Flush());
   }
   return error;
 }
@@ -106,10 +103,8 @@ bool SBFile::IsValid() const {
 SBError SBFile::Close() {
   LLDB_INSTRUMENT_VA(this);
   SBError error;
-  if (m_opaque_sp) {
-    Status status = m_opaque_sp->Close();
-    error.SetError(status);
-  }
+  if (m_opaque_sp)
+    error.SetError(m_opaque_sp->Close());
   return error;
 }
 

lldb/source/API/SBFormat.cpp

@@ -36,7 +36,7 @@ SBFormat::SBFormat(const char *format, lldb::SBError &error) {
   FormatEntrySP format_entry_sp = std::make_shared<FormatEntity::Entry>();
   Status status = FormatEntity::Parse(format, *format_entry_sp);
 
-  error.SetError(status);
+  error.SetError(std::move(status));
   if (error.Success())
     m_opaque_sp = format_entry_sp;
 }

lldb/source/API/SBFrame.cpp

@@ -809,7 +809,7 @@ SBValueList SBFrame::GetVariables(const lldb::SBVariablesOptions &options) {
         Status var_error;
         variable_list = frame->GetVariableList(true, &var_error);
         if (var_error.Fail())
-          value_list.SetError(var_error);
+          value_list.SetError(std::move(var_error));
         if (variable_list) {
           const size_t num_variables = variable_list->GetSize();
           if (num_variables) {
@@ -1033,7 +1033,8 @@ SBValue SBFrame::EvaluateExpression(const char *expr) {
     Status error;
     error = Status::FromErrorString("can't evaluate expressions when the "
                                     "process is running.");
-    ValueObjectSP error_val_sp = ValueObjectConstResult::Create(nullptr, error);
+    ValueObjectSP error_val_sp =
+        ValueObjectConstResult::Create(nullptr, std::move(error));
     result.SetSP(error_val_sp, false);
   }
   return result;
@@ -1129,13 +1130,13 @@ lldb::SBValue SBFrame::EvaluateExpression(const char *expr,
       Status error;
       error = Status::FromErrorString("can't evaluate expressions when the "
                                       "process is running.");
-      expr_value_sp = ValueObjectConstResult::Create(nullptr, error);
+      expr_value_sp = ValueObjectConstResult::Create(nullptr, std::move(error));
       expr_result.SetSP(expr_value_sp, false);
     }
   } else {
       Status error;
       error = Status::FromErrorString("sbframe object is not valid.");
-      expr_value_sp = ValueObjectConstResult::Create(nullptr, error);
+      expr_value_sp = ValueObjectConstResult::Create(nullptr, std::move(error));
       expr_result.SetSP(expr_value_sp, false);
   }
 

lldb/source/API/SBPlatform.cpp

@@ -586,7 +586,7 @@ SBProcess SBPlatform::Attach(SBAttachInfo &attach_info,
       Status status;
       ProcessSP process_sp = platform_sp->Attach(info, debugger.ref(),
                                                  target.GetSP().get(), status);
-      error.SetError(status);
+      error.SetError(std::move(status));
       return SBProcess(process_sp);
     }
 
@@ -728,7 +728,7 @@ SBError SBPlatform::SetLocateModuleCallback(
           symbol_file_spec = symbol_file_spec_sb.ref();
         }
 
-        return error.ref();
+        return error.ref().Clone();
       });
   return SBError();
 }

lldb/source/API/SBProcess.cpp

@@ -1450,7 +1450,7 @@ lldb::SBError SBProcess::DeallocateMemory(lldb::addr_t ptr) {
       std::lock_guard<std::recursive_mutex> guard(
           process_sp->GetTarget().GetAPIMutex());
       Status error = process_sp->DeallocateMemory(ptr);
-      sb_error.SetError(error);
+      sb_error.SetError(std::move(error));
     } else {
       sb_error = Status::FromErrorString("process is running");
     }

lldb/source/API/SBSaveCoreOptions.cpp

@@ -40,8 +40,7 @@ SBSaveCoreOptions::operator=(const SBSaveCoreOptions &rhs) {
 
 SBError SBSaveCoreOptions::SetPluginName(const char *name) {
   LLDB_INSTRUMENT_VA(this, name);
-  lldb_private::Status error = m_opaque_up->SetPluginName(name);
-  return SBError(error);
+  return SBError(m_opaque_up->SetPluginName(name));
 }
 
 void SBSaveCoreOptions::SetStyle(lldb::SaveCoreStyle style) {

lldb/source/API/SBStructuredData.cpp

@@ -133,7 +133,7 @@ lldb::SBError SBStructuredData::GetDescription(lldb::SBStream &stream) const {
 
   Status error = m_impl_up->GetDescription(stream.ref());
   SBError sb_error;
-  sb_error.SetError(error);
+  sb_error.SetError(std::move(error));
   return sb_error;
 }
 

lldb/source/API/SBTarget.cpp

@@ -1369,7 +1369,7 @@ SBTarget::WatchpointCreateByAddress(lldb::addr_t addr, size_t size,
     CompilerType *type = nullptr;
     watchpoint_sp =
         target_sp->CreateWatchpoint(addr, size, type, watch_type, cw_error);
-    error.SetError(cw_error);
+    error.SetError(std::move(cw_error));
     sb_watchpoint.SetSP(watchpoint_sp);
   }
 
@@ -2326,7 +2326,8 @@ lldb::SBValue SBTarget::EvaluateExpression(const char *expr,
           Status error;
           error = Status::FromErrorString("can't evaluate expressions when the "
                                           "process is running.");
-          expr_value_sp = ValueObjectConstResult::Create(nullptr, error);
+          expr_value_sp =
+              ValueObjectConstResult::Create(nullptr, std::move(error));
         }
       } else {
         target->EvaluateExpression(expr, frame, expr_value_sp, options.ref());

lldb/source/API/SBThread.cpp

@@ -958,7 +958,7 @@ SBError SBThread::JumpToLine(lldb::SBFileSpec &file_spec, uint32_t line) {
   Thread *thread = exe_ctx.GetThreadPtr();
 
   Status err = thread->JumpToLine(file_spec.ref(), line, true);
-  sb_error.SetError(err);
+  sb_error.SetError(std::move(err));
   return sb_error;
 }