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

This reverts commit 40d8888f13.
One last Windows failure remaining.

lldb/include/lldb/Utility/Status.h

@@ -28,67 +28,6 @@ namespace lldb_private {
 
 const char *ExpressionResultAsCString(lldb::ExpressionResults result);
 
-/// Going a bit against the spirit of llvm::Error,
-/// lldb_private::Status need to store errors long-term and sometimes
-/// copy them. This base class defines an interface for this
-/// operation.
-class CloneableError
-    : public llvm::ErrorInfo<CloneableError, llvm::ErrorInfoBase> {
-public:
-  using llvm::ErrorInfo<CloneableError, llvm::ErrorInfoBase>::ErrorInfo;
-  CloneableError() : ErrorInfo() {}
-  virtual std::unique_ptr<CloneableError> Clone() const = 0;
-  static char ID;
-};
-
-/// Common base class for all error-code errors.
-class CloneableECError
-    : public llvm::ErrorInfo<CloneableECError, CloneableError> {
-public:
-  using llvm::ErrorInfo<CloneableECError, CloneableError>::ErrorInfo;
-  std::error_code convertToErrorCode() const override { return EC; }
-  void log(llvm::raw_ostream &OS) const override { OS << EC.message(); }
-  static char ID;
-
-protected:
-  CloneableECError() = delete;
-  CloneableECError(std::error_code ec) : ErrorInfo(), EC(ec) {}
-  std::error_code EC;
-};
-/// FIXME: Move these declarations closer to where they're used.
-class MachKernelError
-    : public llvm::ErrorInfo<MachKernelError, CloneableECError> {
-public:
-  using llvm::ErrorInfo<MachKernelError, CloneableECError>::ErrorInfo;
-  MachKernelError(std::error_code ec) : ErrorInfo(ec) {}
-  std::string message() const override;
-  std::unique_ptr<CloneableError> Clone() const override;
-  static char ID;
-};
-
-class Win32Error : public llvm::ErrorInfo<Win32Error, CloneableECError> {
-public:
-  using llvm::ErrorInfo<Win32Error, CloneableECError>::ErrorInfo;
-  Win32Error(std::error_code ec, const llvm::Twine &msg = {}) : ErrorInfo(ec) {}
-  std::string message() const override;
-  std::unique_ptr<CloneableError> Clone() const override;
-  static char ID;
-};
-
-class ExpressionError
-    : public llvm::ErrorInfo<ExpressionError, CloneableECError> {
-public:
-  using llvm::ErrorInfo<ExpressionError, CloneableECError>::ErrorInfo;
-  ExpressionError(std::error_code ec, std::string msg = {})
-      : ErrorInfo(ec), m_string(msg) {}
-  std::unique_ptr<CloneableError> Clone() const override;
-  std::string message() const override { return m_string; }
-  static char ID;
-
-protected:
-  std::string m_string;
-};
-
 /// \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
@@ -161,7 +100,9 @@ public:
   }
 
   static Status FromExpressionError(lldb::ExpressionResults result,
-                                    std::string msg);
+                                    std::string msg) {
+    return Status(result, lldb::eErrorTypeExpression, msg);
+  }
 
   /// Set the current error to errno.
   ///
@@ -174,7 +115,6 @@ public:
   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.
   llvm::Error ToError() const;
   /// Don't call this function in new code. Instead, redesign the API
@@ -209,20 +149,12 @@ public:
 
   /// Access the error value.
   ///
-  /// If the internally stored \ref llvm::Error is an \ref
-  /// llvm::ErrorList then this returns the error value of the first
-  /// error.
-  ///
   /// \return
   ///     The error value.
   ValueType GetError() const;
 
   /// Access the error type.
   ///
-  /// If the internally stored \ref llvm::Error is an \ref
-  /// llvm::ErrorList then this returns the error value of the first
-  /// error.
-  ///
   /// \return
   ///     The error type enumeration value.
   lldb::ErrorType GetType() const;
@@ -238,9 +170,12 @@ public:
   bool Success() const;
 
 protected:
-  Status(llvm::Error error) : m_error(std::move(error)) {}
-  llvm::Error m_error;
-  /// TODO: Replace this with just calling toString(m_error).
+  Status(llvm::Error error);
+  /// 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.
   mutable std::string m_string;
 };
 

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp

@@ -1108,10 +1108,9 @@ public:
         py_error = Status::FromError(r.takeError());
     }
     base_error = Base::Close();
-    // Cloning since the wrapped exception may still reference the PyThread.
     if (py_error.Fail())
-      return py_error.Clone();
-    return base_error.Clone();
+      return py_error;
+    return base_error;
   };
 
   PyObject *GetPythonObject() const {
@@ -1197,8 +1196,7 @@ public:
       return Flush();
     auto r = m_py_obj.CallMethod("close");
     if (!r)
-      // Cloning since the wrapped exception may still reference the PyThread.
-      return Status::FromError(r.takeError()).Clone();
+      return Status::FromError(r.takeError());
     return Status();
   }
 
@@ -1206,8 +1204,7 @@ public:
     GIL takeGIL;
     auto r = m_py_obj.CallMethod("flush");
     if (!r)
-      // Cloning since the wrapped exception may still reference the PyThread.
-      return Status::FromError(r.takeError()).Clone();
+      return Status::FromError(r.takeError());
     return Status();
   }
 
@@ -1243,8 +1240,7 @@ public:
     PyObject *pybuffer_p = PyMemoryView_FromMemory(
         const_cast<char *>((const char *)buf), num_bytes, PyBUF_READ);
     if (!pybuffer_p)
-      // Cloning since the wrapped exception may still reference the PyThread.
-      return Status::FromError(llvm::make_error<PythonException>()).Clone();
+      return Status::FromError(llvm::make_error<PythonException>());
     auto pybuffer = Take<PythonObject>(pybuffer_p);
     num_bytes = 0;
     auto bytes_written = As<long long>(m_py_obj.CallMethod("write", pybuffer));
@@ -1264,8 +1260,7 @@ public:
     auto pybuffer_obj =
         m_py_obj.CallMethod("read", (unsigned long long)num_bytes);
     if (!pybuffer_obj)
-      // Cloning since the wrapped exception may still reference the PyThread.
-      return Status::FromError(pybuffer_obj.takeError()).Clone();
+      return Status::FromError(pybuffer_obj.takeError());
     num_bytes = 0;
     if (pybuffer_obj.get().IsNone()) {
       // EOF
@@ -1274,8 +1269,7 @@ public:
     }
     auto pybuffer = PythonBuffer::Create(pybuffer_obj.get());
     if (!pybuffer)
-      // Cloning since the wrapped exception may still reference the PyThread.
-      return Status::FromError(pybuffer.takeError()).Clone();
+      return Status::FromError(pybuffer.takeError());
     memcpy(buf, pybuffer.get().get().buf, pybuffer.get().get().len);
     num_bytes = pybuffer.get().get().len;
     return Status();
@@ -1306,8 +1300,7 @@ public:
     auto bytes_written =
         As<long long>(m_py_obj.CallMethod("write", pystring.get()));
     if (!bytes_written)
-      // Cloning since the wrapped exception may still reference the PyThread.
-      return Status::FromError(bytes_written.takeError()).Clone();
+      return Status::FromError(bytes_written.takeError());
     if (bytes_written.get() < 0)
       return Status::FromErrorString(
           ".write() method returned a negative number!");
@@ -1328,16 +1321,14 @@ public:
     auto pystring = As<PythonString>(
         m_py_obj.CallMethod("read", (unsigned long long)num_chars));
     if (!pystring)
-      // Cloning since the wrapped exception may still reference the PyThread.
-      return Status::FromError(pystring.takeError()).Clone();
+      return Status::FromError(pystring.takeError());
     if (pystring.get().IsNone()) {
       // EOF
       return Status();
     }
     auto stringref = pystring.get().AsUTF8();
     if (!stringref)
-      // Cloning since the wrapped exception may still reference the PyThread.
-      return Status::FromError(stringref.takeError()).Clone();
+      return Status::FromError(stringref.takeError());
     num_bytes = stringref.get().size();
     memcpy(buf, stringref.get().begin(), num_bytes);
     return Status();

lldb/source/Utility/Status.cpp

@@ -8,8 +8,6 @@
 
 #include "lldb/Utility/Status.h"
 
-#include "lldb/Utility/LLDBLog.h"
-#include "lldb/Utility/Log.h"
 #include "lldb/Utility/VASPrintf.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
@@ -39,84 +37,48 @@ class raw_ostream;
 using namespace lldb;
 using namespace lldb_private;
 
-char CloneableError::ID;
-char CloneableECError::ID;
-char MachKernelError::ID;
-char Win32Error::ID;
-char ExpressionError::ID;
-
-namespace {
-/// A std::error_code category for eErrorTypeGeneric.
-class LLDBGenericCategory : public std::error_category {
-  const char *name() const noexcept override { return "LLDBGenericCategory"; }
-  std::string message(int __ev) const override { return "generic LLDB error"; };
-};
-LLDBGenericCategory &lldb_generic_category() {
-  static LLDBGenericCategory g_generic_category;
-  return g_generic_category;
-}
-
-/// A std::error_code category for eErrorTypeExpression.
-class ExpressionCategory : public std::error_category {
-  const char *name() const noexcept override {
-    return "LLDBExpressionCategory";
-  }
-  std::string message(int __ev) const override {
-    return ExpressionResultAsCString(
-        static_cast<lldb::ExpressionResults>(__ev));
-  };
-};
-ExpressionCategory &expression_category() {
-  static ExpressionCategory g_expression_category;
-  return g_expression_category;
-}
-} // namespace
-
-Status::Status() : m_error(llvm::Error::success()) {}
-
-static llvm::Error ErrorFromEnums(Status::ValueType err, ErrorType type,
-                                  std::string msg) {
-  switch (type) {
-  case eErrorTypeMachKernel:
-    return llvm::make_error<MachKernelError>(
-        std::error_code(err, std::system_category()));
-  case eErrorTypeWin32:
-#ifdef _WIN32
-    if (err == NO_ERROR)
-      return llvm::Error::success();
-#endif
-    return llvm::make_error<Win32Error>(
-        std::error_code(err, std::system_category()));
-  case eErrorTypePOSIX:
-    if (msg.empty())
-      return llvm::errorCodeToError(
-          std::error_code(err, std::generic_category()));
-    return llvm::createStringError(
-        std::move(msg), std::error_code(err, std::generic_category()));
-  default:
-    return llvm::createStringError(
-        std::move(msg), std::error_code(err, lldb_generic_category()));
-  }
-}
+Status::Status() {}
 
 Status::Status(ValueType err, ErrorType type, std::string msg)
-    : m_error(ErrorFromEnums(err, type, msg)) {}
+    : m_code(err), m_type(type), m_string(std::move(msg)) {}
 
-// This logic is confusing because C++ calls the traditional (posix) errno codes
+// This logic is confusing because c++ calls the traditional (posix) errno codes
 // "generic errors", while we use the term "generic" to mean completely
 // arbitrary (text-based) errors.
 Status::Status(std::error_code EC)
-    : m_error(!EC ? llvm::Error::success() : llvm::errorCodeToError(EC)) {}
+    : m_code(EC.value()),
+      m_type(EC.category() == std::generic_category() ? eErrorTypePOSIX
+                                                      : eErrorTypeGeneric),
+      m_string(EC.message()) {}
 
 Status::Status(std::string err_str)
-    : m_error(
-          llvm::createStringError(llvm::inconvertibleErrorCode(), err_str)) {}
+    : m_code(LLDB_GENERIC_ERROR), m_type(eErrorTypeGeneric),
+      m_string(std::move(err_str)) {}
 
-const Status &Status::operator=(Status &&other) {
-  Clear();
-  llvm::consumeError(std::move(m_error));
-  m_error = std::move(other.m_error);
-  return *this;
+Status::Status(llvm::Error error) {
+  if (!error) {
+    Clear();
+    return;
+  }
+
+  // if the error happens to be a errno error, preserve the error code
+  error = llvm::handleErrors(
+      std::move(error), [&](std::unique_ptr<llvm::ECError> e) -> llvm::Error {
+        std::error_code ec = e->convertToErrorCode();
+        if (ec.category() == std::generic_category()) {
+          m_code = ec.value();
+          m_type = ErrorType::eErrorTypePOSIX;
+          return llvm::Error::success();
+        }
+        return llvm::Error(std::move(e));
+      });
+
+  // Otherwise, just preserve the message
+  if (error) {
+    m_code = LLDB_GENERIC_ERROR;
+    m_type = eErrorTypeGeneric;
+    m_string = llvm::toString(std::move(error));
+  }
 }
 
 Status Status::FromErrorStringWithFormat(const char *format, ...) {
@@ -132,35 +94,25 @@ Status Status::FromErrorStringWithFormat(const char *format, ...) {
   return Status(string);
 }
 
-Status Status::FromExpressionError(lldb::ExpressionResults result,
-                                   std::string msg) {
-  return Status(llvm::make_error<ExpressionError>(
-      std::error_code(result, expression_category()), msg));
-}
-
-/// Creates a deep copy of all known errors and converts all other
-/// errors to a new llvm::StringError.
-static llvm::Error CloneError(const llvm::Error &error) {
-  llvm::Error result = llvm::Error::success();
-  auto clone = [](const llvm::ErrorInfoBase &e) {
-    if (e.isA<CloneableError>())
-      return llvm::Error(static_cast<const CloneableError &>(e).Clone());
-    if (e.isA<llvm::ECError>())
-      return llvm::errorCodeToError(e.convertToErrorCode());
-    return llvm::make_error<llvm::StringError>(e.message(),
-                                               e.convertToErrorCode(), true);
-  };
-  llvm::visitErrors(error, [&](const llvm::ErrorInfoBase &e) {
-    result = joinErrors(std::move(result), clone(e));
-  });
-  return result;
-}
-
 Status Status::FromError(llvm::Error error) { return Status(std::move(error)); }
 
-llvm::Error Status::ToError() const { return CloneError(m_error); }
+llvm::Error Status::ToError() const {
+  if (Success())
+    return llvm::Error::success();
+  if (m_type == ErrorType::eErrorTypePOSIX)
+    return llvm::errorCodeToError(
+        std::error_code(m_code, std::generic_category()));
+  return llvm::createStringError(AsCString());
+}
 
-Status::~Status() { llvm::consumeError(std::move(m_error)); }
+Status::~Status() = default;
+
+const Status &Status::operator=(Status &&other) {
+  m_code = other.m_code;
+  m_type = other.m_type;
+  m_string = std::move(other.m_string);
+  return *this;
+}
 
 #ifdef _WIN32
 static std::string RetrieveWin32ErrorString(uint32_t error_code) {
@@ -188,33 +140,6 @@ static std::string RetrieveWin32ErrorString(uint32_t error_code) {
 }
 #endif
 
-std::string MachKernelError::message() const {
-#if defined(__APPLE__)
-  if (const char *s = ::mach_error_string(convertToErrorCode().value()))
-    return s;
-#endif
-  return "MachKernelError";
-}
-
-std::string Win32Error::message() const {
-#if defined(_WIN32)
-  return RetrieveWin32ErrorString(convertToErrorCode().value());
-#endif
-  return "Win32Error";
-}
-
-std::unique_ptr<CloneableError> MachKernelError::Clone() const {
-  return std::make_unique<MachKernelError>(convertToErrorCode());
-}
-
-std::unique_ptr<CloneableError> Win32Error::Clone() const {
-  return std::make_unique<Win32Error>(convertToErrorCode());
-}
-
-std::unique_ptr<CloneableError> ExpressionError::Clone() const {
-  return std::make_unique<ExpressionError>(convertToErrorCode(), message());
-}
-
 // Get the error value as a NULL C string. The error string will be fetched and
 // cached on demand. The cached error string value will remain until the error
 // value is changed or cleared.
@@ -222,12 +147,29 @@ const char *Status::AsCString(const char *default_error_str) const {
   if (Success())
     return nullptr;
 
-  m_string = llvm::toStringWithoutConsuming(m_error);
-  // Backwards compatibility with older implementations of Status.
-  if (m_error.isA<llvm::ECError>())
-    if (!m_string.empty() && m_string[m_string.size() - 1] == '\n')
-      m_string.pop_back();
+  if (m_string.empty()) {
+    switch (m_type) {
+    case eErrorTypeMachKernel:
+#if defined(__APPLE__)
+      if (const char *s = ::mach_error_string(m_code))
+        m_string.assign(s);
+#endif
+      break;
 
+    case eErrorTypePOSIX:
+      m_string = llvm::sys::StrError(m_code);
+      break;
+
+    case eErrorTypeWin32:
+#if defined(_WIN32)
+      m_string = RetrieveWin32ErrorString(m_code);
+#endif
+      break;
+
+    default:
+      break;
+    }
+  }
   if (m_string.empty()) {
     if (default_error_str)
       m_string.assign(default_error_str);
@@ -239,59 +181,29 @@ const char *Status::AsCString(const char *default_error_str) const {
 
 // Clear the error and any cached error string that it might contain.
 void Status::Clear() {
-  if (m_error)
-    LLDB_LOG_ERRORV(GetLog(LLDBLog::API), std::move(m_error),
-                    "dropping error {0}");
-  m_error = llvm::Error::success();
+  m_code = 0;
+  m_type = eErrorTypeInvalid;
+  m_string.clear();
 }
 
-Status::ValueType Status::GetError() const {
-  Status::ValueType result = 0;
-  llvm::visitErrors(m_error, [&](const llvm::ErrorInfoBase &error) {
-    // Return the first only.
-    if (result)
-      return;
-    std::error_code ec = error.convertToErrorCode();
-    result = ec.value();
-  });
-  return result;
-}
+// Access the error value.
+Status::ValueType Status::GetError() const { return m_code; }
 
 // Access the error type.
-ErrorType Status::GetType() const {
-  ErrorType result = eErrorTypeInvalid;
-  llvm::visitErrors(m_error, [&](const llvm::ErrorInfoBase &error) {
-    // Return the first only.
-    if (result != eErrorTypeInvalid)
-      return;
-    if (error.isA<MachKernelError>())
-      result = eErrorTypeMachKernel;
-    else if (error.isA<Win32Error>())
-      result = eErrorTypeWin32;
-    else if (error.isA<ExpressionError>())
-      result = eErrorTypeExpression;
-    else if (error.convertToErrorCode().category() == std::generic_category())
-      result = eErrorTypePOSIX;
-    else if (error.convertToErrorCode().category() == lldb_generic_category() ||
-             error.convertToErrorCode() == llvm::inconvertibleErrorCode())
-      result = eErrorTypeGeneric;
-    else
-      result = eErrorTypeInvalid;
-  });
-  return result;
-}
+ErrorType Status::GetType() const { return m_type; }
 
-bool Status::Fail() const {
-  // Note that this does not clear the checked flag in
-  // m_error. Otherwise we'd need to make this thread-safe.
-  return m_error.isA<llvm::ErrorInfoBase>();
-}
+// Returns true if this object contains a value that describes an error or
+// otherwise non-success result.
+bool Status::Fail() const { return m_code != 0; }
 
-Status Status::FromErrno() { return Status(llvm::errnoAsErrorCode()); }
+Status Status::FromErrno() {
+  // Update the error value to be "errno" and update the type to be "POSIX".
+  return Status(errno, eErrorTypePOSIX);
+}
 
 // Returns true if the error code in this object is considered a successful
 // return value.
-bool Status::Success() const { return !Fail(); }
+bool Status::Success() const { return m_code == 0; }
 
 void llvm::format_provider<lldb_private::Status>::format(
     const lldb_private::Status &error, llvm::raw_ostream &OS,

lldb/unittests/Utility/StatusTest.cpp

@@ -70,14 +70,6 @@ TEST(StatusTest, ErrorConversion) {
   llvm::Error foo = Status::FromErrorString("foo").ToError();
   EXPECT_TRUE(bool(foo));
   EXPECT_EQ("foo", llvm::toString(std::move(foo)));
-
-  llvm::Error eperm = llvm::errorCodeToError({EPERM, std::generic_category()});
-  llvm::Error eintr = llvm::errorCodeToError({EINTR, std::generic_category()});
-  llvm::Error elist = llvm::joinErrors(std::move(eperm), std::move(eintr));
-  elist = llvm::joinErrors(std::move(elist), llvm::createStringError("foo"));
-  Status list = Status::FromError(std::move(elist));
-  EXPECT_EQ((int)list.GetError(), EPERM);
-  EXPECT_EQ(list.GetType(), eErrorTypePOSIX);
 }
 
 #ifdef _WIN32