https://github.com/adrian-prantl updated
https://github.com/llvm/llvm-project/pull/106774
>From 31f3217635d937f5c3ff2733eb34905d63714085 Mon Sep 17 00:00:00 2001
From: Adrian Prantl <apra...@apple.com>
Date: Wed, 4 Sep 2024 12:50:37 -0700
Subject: [PATCH] [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. Where possible, 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
---
lldb/include/lldb/Utility/Status.h | 74 +++++++--
lldb/source/Utility/Status.cpp | 252 ++++++++++++++++++++---------
2 files changed, 238 insertions(+), 88 deletions(-)
diff --git a/lldb/include/lldb/Utility/Status.h
b/lldb/include/lldb/Utility/Status.h
index 795c830b965173..46b980e36700c7 100644
--- a/lldb/include/lldb/Utility/Status.h
+++ b/lldb/include/lldb/Utility/Status.h
@@ -28,6 +28,66 @@ 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;
+ CloneableECError(std::error_code ec) : ErrorInfo() {}
+ std::error_code convertToErrorCode() const override { return EC; }
+ void log(llvm::raw_ostream &OS) const override { OS << EC.message(); }
+ static char ID;
+
+protected:
+ 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;
+ 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
@@ -100,9 +160,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.
///
@@ -115,6 +173,7 @@ class Status {
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
@@ -170,12 +229,9 @@ class Status {
bool Success() const;
protected:
- 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.
+ Status(llvm::Error error) : m_error(std::move(error)) {}
+ llvm::Error m_error;
+ /// TODO: Replace this with just callling toString(m_error).
mutable std::string m_string;
};
diff --git a/lldb/source/Utility/Status.cpp b/lldb/source/Utility/Status.cpp
index 4af3af5fba0185..69d01513235407 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,77 @@ 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 GenericCategory : public std::error_category {
+ const char *name() const override { return "LLDBGenericCategory"; }
+ std::string message(int __ev) const override { return "generic LLDB error";
};
+};
+GenericCategory &generic_category() {
+ static GenericCategory g_generic_category;
+ return g_generic_category;
+}
+
+/// A std::error_code category for eErrorTypeExpression.
+class ExpressionCategory : public std::error_category {
+ const char *name() const 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 eErrorTypePOSIX:
+ return llvm::errorCodeToError(
+ std::error_code(err, std::generic_category()));
+ case eErrorTypeWin32:
+ return llvm::make_error<Win32Error>(
+ std::error_code(err, std::system_category()));
+ default:
+ return llvm::createStringError(std::move(msg),
+ std::error_code(err, 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()
+ : (EC.category() == std::generic_category()
+ ? llvm::errorCodeToError(EC)
+ : llvm::createStringError(EC, "generic error"))) {}
Status::Status(std::string err_str)
- : m_code(LLDB_GENERIC_ERROR), m_type(eErrorTypeGeneric),
- m_string(std::move(err_str)) {}
-
-Status::Status(llvm::Error error) {
- if (!error) {
- Clear();
- return;
- }
+ : m_error(
+ llvm::createStringError(llvm::inconvertibleErrorCode(), err_str)) {}
- // 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();
+ m_error = std::move(other.m_error);
+ return *this;
}
Status Status::FromErrorStringWithFormat(const char *format, ...) {
@@ -94,26 +125,49 @@ 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())
+/// 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) {
+ std::vector<std::unique_ptr<llvm::ErrorInfoBase>> info;
+ llvm::visitErrors(error, [&](const llvm::ErrorInfoBase &error) {
+ if (error.isA<CloneableError>())
+ info.push_back(static_cast<const CloneableError *>(&error)->Clone());
+ else
+ info.push_back(std::make_unique<llvm::StringError>(
+ error.message(), error.convertToErrorCode(), true));
+ });
+ if (info.size() == 0)
return llvm::Error::success();
- if (m_type == ErrorType::eErrorTypePOSIX)
- return llvm::errorCodeToError(
- std::error_code(m_code, std::generic_category()));
- return llvm::createStringError(AsCString());
+ llvm::Error e(std::move(info.front()));
+ for (auto it = std::next(info.begin()); it != info.end(); ++it)
+ e = llvm::joinErrors(std::move(e), llvm::Error(std::move(*it)));
+ return e;
}
-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;
+Status Status::FromError(llvm::Error error) {
+ // FIXME: Existing clients assume that the conversion to Status
+ // consumes and destroys the error. Particularly the scripting
+ // bridge errors refer to python state that may no longer be
+ // available when converting the error to a string later on in the
+ // Status object's lifetime.
+ //
+ // As a workaround, using CloneError() to convert all unknown errors
+ // to strings here.
+ llvm::Error clone = CloneError(error);
+ llvm::consumeError(std::move(error));
+ return Status(std::move(clone));
}
+llvm::Error Status::ToError() const { return CloneError(m_error); }
+
+Status::~Status() { llvm::consumeError(std::move(m_error)); }
+
#ifdef _WIN32
static std::string RetrieveWin32ErrorString(uint32_t error_code) {
char *buffer = nullptr;
@@ -140,6 +194,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 +228,8 @@ 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);
- default:
- break;
- }
- }
if (m_string.empty()) {
if (default_error_str)
m_string.assign(default_error_str);
@@ -181,29 +241,63 @@ 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();
+ llvm::consumeError(std::move(m_error));
}
// 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) {
+ std::error_code ec = error.convertToErrorCode();
+ if (ec.category() == std::generic_category() ||
+ ec.category() == generic_category() ||
+ ec.category() == expression_category())
+ result = ec.value();
+ else
+ result = 0xff;
+ });
+ return result;
+}
// Access the error type.
-ErrorType Status::GetType() const { return m_type; }
+ErrorType Status::GetType() const {
+ ErrorType result = eErrorTypeInvalid;
+ llvm::visitErrors(m_error, [&](const llvm::ErrorInfoBase &error) {
+ 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() == generic_category() ||
+ error.convertToErrorCode() == llvm::inconvertibleErrorCode())
+ result = eErrorTypeGeneric;
+ else
+ result = eErrorTypeInvalid;
+ });
+ return result;
+}
-// 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; }
+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() {
// Update the error value to be "errno" and update the type to be "POSIX".
- return Status(errno, eErrorTypePOSIX);
+ return Status(llvm::errorCodeToError(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,
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
