[lldb] Change the implementation of Status to store an llvm::Error (NFC)

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

This patch makes the transition to llvm::Error easier by making the
places that cannot switch to llvm::Error explicit: They are marked
with a call to Status::clone(). Every other API can and should be
refactored to use llvm::Expected. In the end Status should only be
used in very few places.

Whenever an unchecked Error is dropped by Status it logs this to the
verbose API channel.

Implementation notes:

This patch introduces two new kinds of error_category as well as new
llvm::Error types. Here is the mapping of lldb::ErrorType to
llvm::Errors:

   (eErrorTypeInvalid)
   eErrorTypeGeneric      llvm::StringError
   eErrorTypePOSIX        llvm::ECError
   eErrorTypeMachKernel   MachKernelError
   eErrorTypeExpression   llvm::ErrorList<ExpressionError>
   eErrorTypeWin32        Win32Error

Author: adrian-prantl

lldb/bindings/python/python-swigsafecast.swig

@@ -34,7 +34,7 @@ PythonObject SWIGBridge::ToSWIGWrapper(lldb::BreakpointSP breakpoint_sp) {
 }
 
 PythonObject SWIGBridge::ToSWIGWrapper(const Status& status) {
-  return ToSWIGHelper(new lldb::SBError(status), SWIGTYPE_p_lldb__SBError);
+  return ToSWIGHelper(new lldb::SBError(status.clone()), 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/Target/Process.h

@@ -1296,8 +1296,6 @@ class Process : public std::enable_shared_from_this<Process>,
                 const EvaluateExpressionOptions &options,
                 DiagnosticManager &diagnostic_manager);
 
-  static const char *ExecutionResultAsCString(lldb::ExpressionResults result);
-
   void GetStatus(Stream &ostrm);
 
   size_t GetThreadStatus(Stream &ostrm, bool only_threads_with_stop_reason,

lldb/include/lldb/Utility/Status.h

@@ -26,6 +26,36 @@ class raw_ostream;
 
 namespace lldb_private {
 
+class MachKernelError
+    : public llvm::ErrorInfo<MachKernelError, llvm::StringError> {
+public:
+  using llvm::ErrorInfo<MachKernelError, llvm::StringError>::ErrorInfo;
+  MachKernelError(std::error_code ec, const llvm::Twine &msg = {})
+      : ErrorInfo(msg, ec) {}
+  std::string message() const override;
+  static char ID;
+};
+
+class Win32Error : public llvm::ErrorInfo<Win32Error, llvm::StringError> {
+public:
+  using llvm::ErrorInfo<Win32Error, llvm::StringError>::ErrorInfo;
+  Win32Error(std::error_code ec, const llvm::Twine &msg = {})
+      : ErrorInfo(msg, ec) {}
+  std::string message() const override;
+  static char ID;
+};
+
+class ExpressionError
+    : public llvm::ErrorInfo<ExpressionError, llvm::StringError> {
+public:
+  using llvm::ErrorInfo<ExpressionError, llvm::StringError>::ErrorInfo;
+  ExpressionError(std::error_code ec, std::string msg = {})
+      : ErrorInfo(msg, ec) {}
+  static char ID;
+};
+
+const char *ExecutionResultAsCString(lldb::ExpressionResults result);
+
 /// \class Status Status.h "lldb/Utility/Status.h" An error handling class.
 ///
 /// This class is designed to be able to hold any error code that can be
@@ -41,13 +71,32 @@ namespace lldb_private {
 /// 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.
   ///
@@ -79,9 +128,7 @@ class Status {
   }
 
   static Status FromExpressionError(lldb::ExpressionResults result,
-                                    std::string msg) {
-    return Status(result, lldb::eErrorTypeExpression, msg);
-  }
+                                    std::string msg);
 
   /// Set the current error to errno.
   ///
@@ -91,10 +138,14 @@ class Status {
 
   ~Status();
 
-  // llvm::Error support
-  explicit Status(llvm::Error error) { *this = std::move(error); }
+  /// Avoid using this in new code. Migrate APIs to llvm::Expected instead.
+  static Status FromError(llvm::Error &&error);
   const Status &operator=(llvm::Error error);
+  Status &operator=(Status &&);
+  /// FIXME: Replace this with a takeError() method.
   llvm::Error ToError() const;
+  /// Don't call this function in new code. Redesign the API instead.
+  Status clone() const { return Status(ToError()); }
 
   /// Get the error string associated with the current error.
   //
@@ -145,11 +196,8 @@ class Status {
   bool Success() const;
 
 protected:
-  /// Status code as an integer value.
-  ValueType m_code = 0;
-  /// The type of the above error code.
-  lldb::ErrorType m_type = lldb::eErrorTypeInvalid;
-  /// A string representation of the error code.
+  Status(llvm::Error &&error) : m_error(std::move(error)) {}
+  mutable llvm::Error m_error;
   mutable std::string m_string;
 };
 

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

@@ -220,7 +220,7 @@ lldb::SBError SBDebugger::InitializeWithErrorHandling() {
   SBError error;
   if (auto e = g_debugger_lifetime->Initialize(
           std::make_unique<SystemInitializerFull>(), LoadPlugin)) {
-    error.SetError(Status(std::move(e)));
+    error.SetError(Status::FromError(std::move(e)));
   }
   return error;
 }
@@ -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;
 }