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

Author: adrian-prantl

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() {