[Lldb-commits] [lldb] 104b249 - [lldb] Change the implementation of Status to store an llvm::Error (NFC) (#106774)
Adrian Prantl via lldb-commits
lldb-commits at lists.llvm.org
Thu Sep 19 10:07:27 PDT 2024
Author: Adrian Prantl
Date: 2024-09-19T10:07:13-07:00
New Revision: 104b249c236578d298384416c495ff7310b97f4d
URL: https://github.com/llvm/llvm-project/commit/104b249c236578d298384416c495ff7310b97f4d
DIFF: https://github.com/llvm/llvm-project/commit/104b249c236578d298384416c495ff7310b97f4d.diff
LOG: [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.
Added:
Modified:
lldb/include/lldb/Utility/Status.h
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
lldb/source/Utility/Status.cpp
lldb/unittests/Utility/StatusTest.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Utility/Status.h b/lldb/include/lldb/Utility/Status.h
index 795c830b965173..084ce4afb8cefd 100644
--- a/lldb/include/lldb/Utility/Status.h
+++ b/lldb/include/lldb/Utility/Status.h
@@ -28,6 +28,67 @@ 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
@@ -100,9 +161,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 +174,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
@@ -149,12 +209,20 @@ class Status {
/// 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;
@@ -170,12 +238,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 calling toString(m_error).
mutable std::string m_string;
};
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
index 24cf3430006329..90ccd1055199a0 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
@@ -1108,9 +1108,10 @@ template <typename Base> class OwnedPythonFile : public Base {
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 @@ class PythonIOFile : public OwnedPythonFile<File> {
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 @@ class PythonIOFile : public OwnedPythonFile<File> {
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 @@ class BinaryPythonFile : public PythonIOFile {
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 @@ class BinaryPythonFile : public PythonIOFile {
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 @@ class BinaryPythonFile : public PythonIOFile {
}
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 @@ class TextPythonFile : public PythonIOFile {
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 @@ class TextPythonFile : public PythonIOFile {
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 4af3af5fba0185..ba08353c7b24d2 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,80 @@ 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:
+ 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 +128,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)); }
-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;
-}
+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) {
@@ -140,6 +184,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 +218,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 +235,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,
diff --git a/lldb/unittests/Utility/StatusTest.cpp b/lldb/unittests/Utility/StatusTest.cpp
index be4f2beebcdb52..e37c94ac17f2d0 100644
--- a/lldb/unittests/Utility/StatusTest.cpp
+++ b/lldb/unittests/Utility/StatusTest.cpp
@@ -70,6 +70,14 @@ 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
More information about the lldb-commits
mailing list