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

(based on a conversation I had with @labath yesterday in
https://github.com/llvm/llvm-project/pull/106442)

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
```

Relanding with built-in cloning support for llvm::ECError, and support
for initializing a Windows error with a NO_ERROR error code.

2 files changed, 191 insertions, 94 deletions

diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
index 24cf3430..90ccd10 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
@@ -1108,9 +1108,10 @@ 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;
-    return base_error;
+      return py_error.Clone();
+    return base_error.Clone();
   };
 
   PyObject *GetPythonObject() const {
@@ -1196,7 +1197,8 @@ public:
       return Flush();
     auto r = m_py_obj.CallMethod("close");
     if (!r)
-      return Status::FromError(r.takeError());
+      // Cloning since the wrapped exception may still reference the PyThread.
+      return Status::FromError(r.takeError()).Clone();
     return Status();
   }
 
@@ -1204,7 +1206,8 @@ public:
     GIL takeGIL;
     auto r = m_py_obj.CallMethod("flush");
     if (!r)
-      return Status::FromError(r.takeError());
+      // Cloning since the wrapped exception may still reference the PyThread.
+      return Status::FromError(r.takeError()).Clone();
     return Status();
   }
 
@@ -1240,7 +1243,8 @@ public:
     PyObject *pybuffer_p = PyMemoryView_FromMemory(
         const_cast<char *>((const char *)buf), num_bytes, PyBUF_READ);
     if (!pybuffer_p)
-      return Status::FromError(llvm::make_error<PythonException>());
+      // Cloning since the wrapped exception may still reference the PyThread.
+      return Status::FromError(llvm::make_error<PythonException>()).Clone();
     auto pybuffer = Take<PythonObject>(pybuffer_p);
     num_bytes = 0;
     auto bytes_written = As<long long>(m_py_obj.CallMethod("write", pybuffer));
@@ -1260,7 +1264,8 @@ public:
     auto pybuffer_obj =
         m_py_obj.CallMethod("read", (unsigned long long)num_bytes);
     if (!pybuffer_obj)
-      return Status::FromError(pybuffer_obj.takeError());
+      // Cloning since the wrapped exception may still reference the PyThread.
+      return Status::FromError(pybuffer_obj.takeError()).Clone();
     num_bytes = 0;
     if (pybuffer_obj.get().IsNone()) {
       // EOF
@@ -1269,7 +1274,8 @@ public:
     }
     auto pybuffer = PythonBuffer::Create(pybuffer_obj.get());
     if (!pybuffer)
-      return Status::FromError(pybuffer.takeError());
+      // Cloning since the wrapped exception may still reference the PyThread.
+      return Status::FromError(pybuffer.takeError()).Clone();
     memcpy(buf, pybuffer.get().get().buf, pybuffer.get().get().len);
     num_bytes = pybuffer.get().get().len;
     return Status();
@@ -1300,7 +1306,8 @@ public:
     auto bytes_written =
         As<long long>(m_py_obj.CallMethod("write", pystring.get()));
     if (!bytes_written)
-      return Status::FromError(bytes_written.takeError());
+      // Cloning since the wrapped exception may still reference the PyThread.
+      return Status::FromError(bytes_written.takeError()).Clone();
     if (bytes_written.get() < 0)
       return Status::FromErrorString(
           ".write() method returned a negative number!");
@@ -1321,14 +1328,16 @@ public:
     auto pystring = As<PythonString>(
         m_py_obj.CallMethod("read", (unsigned long long)num_chars));
     if (!pystring)
-      return Status::FromError(pystring.takeError());
+      // Cloning since the wrapped exception may still reference the PyThread.
+      return Status::FromError(pystring.takeError()).Clone();
     if (pystring.get().IsNone()) {
       // EOF
       return Status();
     }
     auto stringref = pystring.get().AsUTF8();
     if (!stringref)
-      return Status::FromError(stringref.takeError());
+      // Cloning since the wrapped exception may still reference the PyThread.
+      return Status::FromError(stringref.takeError()).Clone();
     num_bytes = stringref.get().size();
     memcpy(buf, stringref.get().begin(), num_bytes);
     return Status();
diff --git a/lldb/source/Utility/Status.cpp b/lldb/source/Utility/Status.cpp
index 4af3af5..7985cc5 100644
--- a/lldb/source/Utility/Status.cpp
+++ b/lldb/source/Utility/Status.cpp
@@ -8,6 +8,8 @@
 
 #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"
@@ -37,48 +39,84 @@ class raw_ostream;
 using namespace lldb;
 using namespace lldb_private;
 
-Status::Status() {}
+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(ValueType err, ErrorType type, std::string msg)
-    : m_code(err), m_type(type), m_string(std::move(msg)) {}
+    : m_error(ErrorFromEnums(err, type, 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_code(EC.value()),
-      m_type(EC.category() == std::generic_category() ? eErrorTypePOSIX
-                                                      : eErrorTypeGeneric),
-      m_string(EC.message()) {}
+    : m_error(!EC ? llvm::Error::success() : llvm::errorCodeToError(EC)) {}
 
 Status::Status(std::string err_str)
-    : m_code(LLDB_GENERIC_ERROR), m_type(eErrorTypeGeneric),
-      m_string(std::move(err_str)) {}
+    : m_error(
+          llvm::createStringError(llvm::inconvertibleErrorCode(), err_str)) {}
 
-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));
-  }
+const Status &Status::operator=(Status &&other) {
+  Clear();
+  llvm::consumeError(std::move(m_error));
+  m_error = std::move(other.m_error);
+  return *this;
 }
 
 Status Status::FromErrorStringWithFormat(const char *format, ...) {
@@ -94,25 +132,35 @@ Status Status::FromErrorStringWithFormat(const char *format, ...) {
   return Status(string);
 }
 
-Status Status::FromError(llvm::Error error) { return Status(std::move(error)); }
+Status Status::FromExpressionError(lldb::ExpressionResults result,
+                                   std::string msg) {
+  return Status(llvm::make_error<ExpressionError>(
+      std::error_code(result, expression_category()), msg));
+}
 
-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());
+/// 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() = default;
+Status Status::FromError(llvm::Error error) { return Status(std::move(error)); }
+
+llvm::Error Status::ToError() const { return CloneError(m_error); }
 
-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;
-}
+Status::~Status() { llvm::consumeError(std::move(m_error)); }
 
 #ifdef _WIN32
 static std::string RetrieveWin32ErrorString(uint32_t error_code) {
@@ -140,6 +188,33 @@ 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.
@@ -147,29 +222,12 @@ const char *Status::AsCString(const char *default_error_str) const {
   if (Success())
     return nullptr;
 
-  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;
+  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();
 
-    default:
-      break;
-    }
-  }
   if (m_string.empty()) {
     if (default_error_str)
       m_string.assign(default_error_str);
@@ -181,29 +239,59 @@ 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() {
-  m_code = 0;
-  m_type = eErrorTypeInvalid;
-  m_string.clear();
+  if (m_error)
+    LLDB_LOG_ERRORV(GetLog(LLDBLog::API), std::move(m_error),
+                    "dropping error {0}");
+  m_error = llvm::Error::success();
 }
 
-// Access the error value.
-Status::ValueType Status::GetError() const { return m_code; }
+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 type.
-ErrorType Status::GetType() const { return m_type; }
-
-// 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; }
+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;
+}
 
-Status Status::FromErrno() {
-  // Update the error value to be "errno" and update the type to be "POSIX".
-  return Status(errno, eErrorTypePOSIX);
+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>();
 }
 
+Status Status::FromErrno() { return Status(llvm::errnoAsErrorCode()); }
+
 // Returns true if the error code in this object is considered a successful
 // return value.
-bool Status::Success() const { return m_code == 0; }
+bool Status::Success() const { return !Fail(); }
 
 void llvm::format_provider<lldb_private::Status>::format(
     const lldb_private::Status &error, llvm::raw_ostream &OS,