Author: Adrian Prantl Date: 2024-09-18T14:54:49-07:00 New Revision: 06939fa2e140a171132275ec0ea1857d20c5dbdd
URL: https://github.com/llvm/llvm-project/commit/06939fa2e140a171132275ec0ea1857d20c5dbdd DIFF: https://github.com/llvm/llvm-project/commit/06939fa2e140a171132275ec0ea1857d20c5dbdd.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 ``` Added: Modified: lldb/include/lldb/Utility/Status.h lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp lldb/source/Utility/Status.cpp lldb/unittests/TestingSupport/Host/SocketTestUtilities.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..4a09c38ce62f1b 100644 --- a/lldb/include/lldb/Utility/Status.h +++ b/lldb/include/lldb/Utility/Status.h @@ -28,6 +28,69 @@ 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() = delete; + CloneableECError(std::error_code ec) : ErrorInfo(), EC(ec) {} + std::error_code convertToErrorCode() const override { return EC; } + void log(llvm::raw_ostream &OS) const override { OS << EC.message(); } + std::unique_ptr<CloneableError> Clone() const override; + 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; + 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 +163,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 +176,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 +211,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 +240,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/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp index 24cf3430006329..6ddd00df3a2180 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp @@ -993,8 +993,8 @@ void PythonException::Restore() { } PythonException::~PythonException() { - Py_XDECREF(m_exception_type); Py_XDECREF(m_exception); + Py_XDECREF(m_exception_type); Py_XDECREF(m_traceback); Py_XDECREF(m_repr_bytes); } @@ -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..faa8d3a83c7ed1 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,78 @@ 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 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 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)) {} - -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(); + llvm::consumeError(std::move(m_error)); + m_error = std::move(other.m_error); + return *this; } Status Status::FromErrorStringWithFormat(const char *format, ...) { @@ -94,25 +126,33 @@ 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()); + return llvm::make_error<llvm::StringError>(e.message(), + e.convertToErrorCode(), true); + }; + 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 +180,37 @@ 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> CloneableECError::Clone() const { + return std::make_unique<CloneableECError>(convertToErrorCode()); +} + +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,64 @@ 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; } +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; +} -// 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); + std::error_code ec = llvm::errnoAsErrorCode(); + if (ec) + return Status::FromError(llvm::make_error<CloneableECError>(ec)); + return Status(); } // 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/TestingSupport/Host/SocketTestUtilities.cpp b/lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp index 2455a4f6f5d490..86fb5b05ea8009 100644 --- a/lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp +++ b/lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp @@ -102,12 +102,16 @@ static bool CheckIPSupport(llvm::StringRef Proto, llvm::StringRef Addr) { Proto, Err) .str(); bool HasProtocolError = false; - handleAllErrors(std::move(Err), [&](std::unique_ptr<llvm::ECError> ECErr) { - std::error_code ec = ECErr->convertToErrorCode(); - if (ec == std::make_error_code(std::errc::address_family_not_supported) || - ec == std::make_error_code(std::errc::address_not_available)) - HasProtocolError = true; - }); + handleAllErrors( + std::move(Err), + [&](std::unique_ptr<llvm::ECError> ECErr) { + std::error_code ec = ECErr->convertToErrorCode(); + if (ec == + std::make_error_code(std::errc::address_family_not_supported) || + ec == std::make_error_code(std::errc::address_not_available)) + HasProtocolError = true; + }, + [](const llvm::ErrorInfoBase &) {}); if (HasProtocolError) { GTEST_LOG_(WARNING) << llvm::formatv( 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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
