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

[Pavel Labath via lldb-commits]

================
@@ -96,16 +124,44 @@ Status Status::FromErrorStringWithFormat(const char *format, ...) {
   return Status(string);
 }
 
-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::FromExpressionError(lldb::ExpressionResults result,
+                                   std::string msg) {
+  return Status(llvm::make_error<ExpressionError>(
+      std::error_code(result, expression_category()), msg));
 }
 
-Status::~Status() = default;
+/// Creates a deep copy of all known errors and converts all other
+/// errors to a new llvm::StringError.
+static llvm::Error cloneError(llvm::Error &error) {
+  llvm::Error result = llvm::Error::success();
+  llvm::consumeError(std::move(result));
+  llvm::visitErrors(error, [&](const llvm::ErrorInfoBase &error) {
+    if (error.isA<MachKernelError>())
+      result = llvm::make_error<MachKernelError>(error.convertToErrorCode());
+    else if (error.isA<Win32Error>())
+      result = llvm::make_error<Win32Error>(error.convertToErrorCode());
+    else if (error.isA<ExpressionError>())
+      result = llvm::make_error<ExpressionError>(error.convertToErrorCode(),
+                                                 error.message());
+    else
+      result =
+          llvm::createStringError(error.convertToErrorCode(), error.message());
+  });
+  return result;
+}
+
+Status Status::FromError(llvm::Error &&error) {
+  // Existing clients assume that the conversion to Status consumes
+  // and destroys the error. Use cloneError to convert all unnown
+  // errors to strings.
+  llvm::Error clone = cloneError(error);
+  llvm::consumeError(std::move(error));
----------------
labath wrote:

Interesting. I think we should look into this more closely as it may impact the overall design choice. I think it should be possible to store python exception objects for ~arbitrarily long (at least as long as the python interpreter exists, which I think we never destroy). And `PythonException` object seems to handle reference counting correctly -- as long as you don't copy it: it has a (implicit) copy constructor, that that just does a bytewise copy without incrementing the python reference count. I'm not sure how you implemented copying originally, but I think the solution could be as simple as adding some Py_INCREFs to its copy constructor.

https://github.com/llvm/llvm-project/pull/106774