Author: Adrian Prantl Date: 2024-09-18T17:28:10-07:00 New Revision: cb6d53198e39838ba6f9d2974c4f4317057d1556
URL: https://github.com/llvm/llvm-project/commit/cb6d53198e39838ba6f9d2974c4f4317057d1556 DIFF: https://github.com/llvm/llvm-project/commit/cb6d53198e39838ba6f9d2974c4f4317057d1556.diff LOG: Revert "[lldb] Change the implementation of Status to store an llvm::Error (NFC) (#106774)" This reverts commit 06939fa2e140a171132275ec0ea1857d20c5dbdd. 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 4a09c38ce62f1b..795c830b965173 100644 --- a/lldb/include/lldb/Utility/Status.h +++ b/lldb/include/lldb/Utility/Status.h @@ -28,69 +28,6 @@ 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 @@ -163,7 +100,9 @@ class Status { } static Status FromExpressionError(lldb::ExpressionResults result, - std::string msg); + std::string msg) { + return Status(result, lldb::eErrorTypeExpression, msg); + } /// Set the current error to errno. /// @@ -176,7 +115,6 @@ 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 @@ -211,20 +149,12 @@ 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; @@ -240,9 +170,12 @@ class Status { bool Success() const; protected: - Status(llvm::Error error) : m_error(std::move(error)) {} - llvm::Error m_error; - /// TODO: Replace this with just callling toString(m_error). + 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. mutable std::string m_string; }; diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp index 6ddd00df3a2180..24cf3430006329 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); Py_XDECREF(m_exception_type); + Py_XDECREF(m_exception); Py_XDECREF(m_traceback); Py_XDECREF(m_repr_bytes); } @@ -1108,10 +1108,9 @@ 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.Clone(); - return base_error.Clone(); + return py_error; + return base_error; }; PyObject *GetPythonObject() const { @@ -1197,8 +1196,7 @@ class PythonIOFile : public OwnedPythonFile<File> { return Flush(); auto r = m_py_obj.CallMethod("close"); if (!r) - // Cloning since the wrapped exception may still reference the PyThread. - return Status::FromError(r.takeError()).Clone(); + return Status::FromError(r.takeError()); return Status(); } @@ -1206,8 +1204,7 @@ class PythonIOFile : public OwnedPythonFile<File> { GIL takeGIL; auto r = m_py_obj.CallMethod("flush"); if (!r) - // Cloning since the wrapped exception may still reference the PyThread. - return Status::FromError(r.takeError()).Clone(); + return Status::FromError(r.takeError()); return Status(); } @@ -1243,8 +1240,7 @@ class BinaryPythonFile : public PythonIOFile { PyObject *pybuffer_p = PyMemoryView_FromMemory( const_cast<char *>((const char *)buf), num_bytes, PyBUF_READ); if (!pybuffer_p) - // Cloning since the wrapped exception may still reference the PyThread. - return Status::FromError(llvm::make_error<PythonException>()).Clone(); + return Status::FromError(llvm::make_error<PythonException>()); auto pybuffer = Take<PythonObject>(pybuffer_p); num_bytes = 0; auto bytes_written = As<long long>(m_py_obj.CallMethod("write", pybuffer)); @@ -1264,8 +1260,7 @@ class BinaryPythonFile : public PythonIOFile { auto pybuffer_obj = m_py_obj.CallMethod("read", (unsigned long long)num_bytes); if (!pybuffer_obj) - // Cloning since the wrapped exception may still reference the PyThread. - return Status::FromError(pybuffer_obj.takeError()).Clone(); + return Status::FromError(pybuffer_obj.takeError()); num_bytes = 0; if (pybuffer_obj.get().IsNone()) { // EOF @@ -1274,8 +1269,7 @@ class BinaryPythonFile : public PythonIOFile { } auto pybuffer = PythonBuffer::Create(pybuffer_obj.get()); if (!pybuffer) - // Cloning since the wrapped exception may still reference the PyThread. - return Status::FromError(pybuffer.takeError()).Clone(); + return Status::FromError(pybuffer.takeError()); memcpy(buf, pybuffer.get().get().buf, pybuffer.get().get().len); num_bytes = pybuffer.get().get().len; return Status(); @@ -1306,8 +1300,7 @@ class TextPythonFile : public PythonIOFile { auto bytes_written = As<long long>(m_py_obj.CallMethod("write", pystring.get())); if (!bytes_written) - // Cloning since the wrapped exception may still reference the PyThread. - return Status::FromError(bytes_written.takeError()).Clone(); + return Status::FromError(bytes_written.takeError()); if (bytes_written.get() < 0) return Status::FromErrorString( ".write() method returned a negative number!"); @@ -1328,16 +1321,14 @@ class TextPythonFile : public PythonIOFile { auto pystring = As<PythonString>( m_py_obj.CallMethod("read", (unsigned long long)num_chars)); if (!pystring) - // Cloning since the wrapped exception may still reference the PyThread. - return Status::FromError(pystring.takeError()).Clone(); + return Status::FromError(pystring.takeError()); if (pystring.get().IsNone()) { // EOF return Status(); } auto stringref = pystring.get().AsUTF8(); if (!stringref) - // Cloning since the wrapped exception may still reference the PyThread. - return Status::FromError(stringref.takeError()).Clone(); + return Status::FromError(stringref.takeError()); 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 faa8d3a83c7ed1..4af3af5fba0185 100644 --- a/lldb/source/Utility/Status.cpp +++ b/lldb/source/Utility/Status.cpp @@ -8,8 +8,6 @@ #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" @@ -39,78 +37,48 @@ class raw_ostream; using namespace lldb; using namespace lldb_private; -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() {} Status::Status(ValueType err, ErrorType type, std::string msg) - : m_error(ErrorFromEnums(err, type, msg)) {} + : m_code(err), m_type(type), m_string(std::move(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_error(!EC ? llvm::Error::success() : llvm::errorCodeToError(EC)) {} + : m_code(EC.value()), + m_type(EC.category() == std::generic_category() ? eErrorTypePOSIX + : eErrorTypeGeneric), + m_string(EC.message()) {} Status::Status(std::string err_str) - : m_error( - llvm::createStringError(llvm::inconvertibleErrorCode(), err_str)) {} + : m_code(LLDB_GENERIC_ERROR), m_type(eErrorTypeGeneric), + m_string(std::move(err_str)) {} -const Status &Status::operator=(Status &&other) { - Clear(); - llvm::consumeError(std::move(m_error)); - m_error = std::move(other.m_error); - return *this; +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)); + } } Status Status::FromErrorStringWithFormat(const char *format, ...) { @@ -126,33 +94,25 @@ Status Status::FromErrorStringWithFormat(const char *format, ...) { return Status(string); } -Status Status::FromExpressionError(lldb::ExpressionResults result, - std::string msg) { - return Status(llvm::make_error<ExpressionError>( - std::error_code(result, expression_category()), msg)); -} +Status Status::FromError(llvm::Error error) { return Status(std::move(error)); } -/// 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; +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::FromError(llvm::Error error) { return Status(std::move(error)); } - -llvm::Error Status::ToError() const { return CloneError(m_error); } +Status::~Status() = default; -Status::~Status() { llvm::consumeError(std::move(m_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; +} #ifdef _WIN32 static std::string RetrieveWin32ErrorString(uint32_t error_code) { @@ -180,37 +140,6 @@ 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. @@ -218,12 +147,29 @@ const char *Status::AsCString(const char *default_error_str) const { if (Success()) return nullptr; - 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(); + 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; + default: + break; + } + } if (m_string.empty()) { if (default_error_str) m_string.assign(default_error_str); @@ -235,64 +181,29 @@ 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() { - if (m_error) - LLDB_LOG_ERRORV(GetLog(LLDBLog::API), std::move(m_error), - "dropping error {0}"); - m_error = llvm::Error::success(); + m_code = 0; + m_type = eErrorTypeInvalid; + m_string.clear(); } -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 value. +Status::ValueType Status::GetError() const { return m_code; } // Access the error 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; -} +ErrorType Status::GetType() const { return m_type; } -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>(); -} +// 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; } Status Status::FromErrno() { - std::error_code ec = llvm::errnoAsErrorCode(); - if (ec) - return Status::FromError(llvm::make_error<CloneableECError>(ec)); - return Status(); + // Update the error value to be "errno" and update the type to be "POSIX". + return Status(errno, eErrorTypePOSIX); } // Returns true if the error code in this object is considered a successful // return value. -bool Status::Success() const { return !Fail(); } +bool Status::Success() const { return m_code == 0; } 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 86fb5b05ea8009..2455a4f6f5d490 100644 --- a/lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp +++ b/lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp @@ -102,16 +102,12 @@ 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; - }, - [](const llvm::ErrorInfoBase &) {}); + 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; + }); if (HasProtocolError) { GTEST_LOG_(WARNING) << llvm::formatv( diff --git a/lldb/unittests/Utility/StatusTest.cpp b/lldb/unittests/Utility/StatusTest.cpp index e37c94ac17f2d0..be4f2beebcdb52 100644 --- a/lldb/unittests/Utility/StatusTest.cpp +++ b/lldb/unittests/Utility/StatusTest.cpp @@ -70,14 +70,6 @@ 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
