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

[Adrian Prantl 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));
----------------
adrian-prantl wrote:

> I'm still confused by this. Why isn't this just `return Status(std::move(error))` ?

The scripting bridge hands out llvm::Errors that contain pointers to Python state that is accessed when converting the error to a string. If we delay the string conversion by just moving the error, this causes use-after-frees in some testcases. The cloneError function converts all unknown errors to StringErrors which are safe to store.

There is definitely room for a better solution here, I this as a transitional state that we should revisit in a follow-up.

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