[Lldb-commits] [lldb] 1553714 - Revert "[lldb] Change the implementation of Status to store an llvm::Error (NFC) (#106774)"
David Spickett via lldb-commits
lldb-commits at lists.llvm.org
Fri Sep 20 07:20:22 PDT 2024
Author: David Spickett
Date: 2024-09-20T15:18:45+01:00
New Revision: 1553714b0093a8ef907faf3b3145c224caa7364a
URL: https://github.com/llvm/llvm-project/commit/1553714b0093a8ef907faf3b3145c224caa7364a
DIFF: https://github.com/llvm/llvm-project/commit/1553714b0093a8ef907faf3b3145c224caa7364a.diff
LOG: Revert "[lldb] Change the implementation of Status to store an llvm::Error (NFC) (#106774)"
This reverts commit 104b249c236578d298384416c495ff7310b97f4d because
it has caused 2 test failures on Windows:
https://lab.llvm.org/buildbot/#/builders/141/builds/2544
Failed Tests (2):
lldb-api :: functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py
lldb-unit :: Utility/./UtilityTests.exe/StatusTest/ErrorWin32
I reckon the cause is the same, that we construct an error with the Win32
NO_ERROR value which means there was no error but we're assuming anything
with an error code is a failure.
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 084ce4afb8cefd..795c830b965173 100644
--- a/lldb/include/lldb/Utility/Status.h
+++ b/lldb/include/lldb/Utility/Status.h
@@ -28,67 +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;
- 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
@@ -161,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.
///
@@ -174,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
@@ -209,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;
@@ -238,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 calling 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 90ccd1055199a0..24cf3430006329 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
@@ -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 ba08353c7b24d2..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,80 +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 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() {}
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, ...) {
@@ -128,35 +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());
- 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;
+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) {
@@ -184,33 +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> 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,59 +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() { return Status(llvm::errnoAsErrorCode()); }
+Status Status::FromErrno() {
+ // 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/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
More information about the lldb-commits
mailing list