[Lldb-commits] [lldb] [lldb] Change the implementation of Status to store an llvm::Error (NFC) (PR #106774)
Adrian Prantl via lldb-commits
lldb-commits at lists.llvm.org
Mon Sep 16 16:15:06 PDT 2024
https://github.com/adrian-prantl updated https://github.com/llvm/llvm-project/pull/106774
>From 34a856d47322cd35a7b568a335adbcd804f2302d Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Wed, 11 Sep 2024 10:35:37 +0200
Subject: [PATCH 1/3] [lldb] Only send "posix" error codes through the
gdb-remote protocol
The other side has no way of telling which namespace do these codes
belong to, so mashing them all together is not very helpful.
I'm mainly doing this to simplify some code in a pending patch
<https://github.com/llvm/llvm-project/pull/106774/files#r1752628604>,
and I've picked the posix error category semi-randomly. If we wanted to
be serious about assigning meaning to these error codes, we should
create a special error category for "gdb errors".
---
.../Process/gdb-remote/GDBRemoteCommunicationServer.cpp | 7 ++++---
.../gdb-remote/GDBRemoteCommunicationServerTest.cpp | 3 ++-
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
index 9b72cb00352821..d4aa90b2c7731a 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
@@ -103,13 +103,14 @@ GDBRemoteCommunicationServer::SendErrorResponse(uint8_t err) {
GDBRemoteCommunication::PacketResult
GDBRemoteCommunicationServer::SendErrorResponse(const Status &error) {
+ uint8_t code = error.GetType() == eErrorTypePOSIX ? error.GetError() : 0xff;
if (m_send_error_strings) {
lldb_private::StreamString packet;
- packet.Printf("E%2.2x;", static_cast<uint8_t>(error.GetError()));
+ packet.Printf("E%2.2x;", code);
packet.PutStringAsRawHex8(error.AsCString());
return SendPacketNoLock(packet.GetString());
- } else
- return SendErrorResponse(error.GetError());
+ }
+ return SendErrorResponse(code);
}
GDBRemoteCommunication::PacketResult
diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
index 69ca1720c04fc9..ba9ca6ea73e3be 100644
--- a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
+++ b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
@@ -12,6 +12,7 @@
#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
#include "lldb/Utility/Connection.h"
#include "lldb/Utility/UnimplementedError.h"
+#include "lldb/lldb-enumerations.h"
namespace lldb_private {
namespace process_gdb_remote {
@@ -25,7 +26,7 @@ TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_ErrorNumber) {
TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_Status) {
MockServerWithMockConnection server;
- Status status(0x42, lldb::eErrorTypeGeneric, "Test error message");
+ Status status(0x42, lldb::eErrorTypePOSIX, "Test error message");
server.SendErrorResponse(status);
EXPECT_THAT(
>From f89d0b6311d3c5c3a659d5ff6a5f733c32e57ad0 Mon Sep 17 00:00:00 2001
From: Adrian Prantl <aprantl at apple.com>
Date: Wed, 4 Sep 2024 12:50:37 -0700
Subject: [PATCH 2/3] [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 | 83 +++++-
.../Python/PythonDataObjects.cpp | 31 ++-
lldb/source/Utility/Status.cpp | 245 ++++++++++++------
lldb/unittests/Utility/StatusTest.cpp | 8 +
4 files changed, 266 insertions(+), 101 deletions(-)
diff --git a/lldb/include/lldb/Utility/Status.h b/lldb/include/lldb/Utility/Status.h
index 795c830b965173..64e193f8d84206 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;
+ 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;
+ 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 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..0daef67b13d8d1 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)) {}
+ : 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 +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,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 +214,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 +231,62 @@ 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);
+ 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,
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
>From 770bfebe2929434803c53cecde47e68e6a8e8ee7 Mon Sep 17 00:00:00 2001
From: Adrian Prantl <aprantl at apple.com>
Date: Wed, 28 Aug 2024 10:04:33 -0700
Subject: [PATCH 3/3] [lldb] Store expression evaluator diagnostics in an
llvm::Error (NFC)
This patch is a reworking of Pete Lawrence's (@PortalPete) proposal
for better expression evaluator error messages:
https://github.com/llvm/llvm-project/pull/80938
This patch is preparatory patch for improving the rendering of
expression evaluator diagnostics. Currently diagnostics are rendered
into a string and the command interpreter layer then textually parses
words like "error:" to (sometimes) color the output accordingly. In
order to enable user interfaces to do better with diagnostics, we need
to store them in a machine-readable fromat. This patch does this by
adding a new llvm::Error kind wrapping a DiagnosticDetail struct that
is used when the error type is eErrorTypeExpression. Multiple
diagnostics are modeled using llvm::ErrorList.
Right now the extra information is not used by the CommandInterpreter,
this will be added in a follow-up patch!
---
.../lldb/Expression/DiagnosticManager.h | 86 ++++++++++++++-----
lldb/include/lldb/Utility/Status.h | 5 +-
lldb/source/Breakpoint/BreakpointLocation.cpp | 10 +--
lldb/source/Expression/DiagnosticManager.cpp | 82 ++++++++++++++----
lldb/source/Expression/ExpressionParser.cpp | 5 +-
lldb/source/Expression/UserExpression.cpp | 28 +++---
lldb/source/Expression/UtilityFunction.cpp | 16 ++--
.../ExpressionParser/Clang/ClangDiagnostic.h | 5 +-
.../Clang/ClangExpressionParser.cpp | 57 +++++++++---
.../Plugins/Platform/POSIX/PlatformPOSIX.cpp | 10 +--
.../Platform/Windows/PlatformWindows.cpp | 10 +--
lldb/source/Target/Target.cpp | 2 +-
lldb/source/Utility/Status.cpp | 8 ++
.../TestModulesCompileError.py | 2 +-
.../Expression/DiagnosticManagerTest.cpp | 12 +--
lldb/unittests/Utility/StatusTest.cpp | 2 +-
16 files changed, 229 insertions(+), 111 deletions(-)
diff --git a/lldb/include/lldb/Expression/DiagnosticManager.h b/lldb/include/lldb/Expression/DiagnosticManager.h
index d49b7c99b114fb..fb9502f4b3ffba 100644
--- a/lldb/include/lldb/Expression/DiagnosticManager.h
+++ b/lldb/include/lldb/Expression/DiagnosticManager.h
@@ -12,6 +12,9 @@
#include "lldb/lldb-defines.h"
#include "lldb/lldb-types.h"
+#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Status.h"
+
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringRef.h"
@@ -20,6 +23,47 @@
namespace lldb_private {
+/// A compiler-independent representation of a Diagnostic. Expression
+/// evaluation failures often have more than one diagnostic that a UI
+/// layer might want to render differently, for example to colorize
+/// it.
+///
+/// Running example:
+/// (lldb) expr 1+foo
+/// error: <user expression 0>:1:3: use of undeclared identifier 'foo'
+/// 1+foo
+/// ^
+struct DiagnosticDetail {
+ struct SourceLocation {
+ FileSpec file;
+ unsigned line = 0;
+ uint16_t column = 0;
+ uint16_t length = 0;
+ bool in_user_input = false;
+ };
+ /// Contains {{}, 1, 3, 3, true} in the example above.
+ std::optional<SourceLocation> source_location;
+ /// Contains eSeverityError in the example above.
+ lldb::Severity severity = lldb::eSeverityInfo;
+ /// Contains "use of undeclared identifier 'x'" in the example above.
+ std::string message;
+ /// Contains the fully rendered error message.
+ std::string rendered;
+};
+
+/// An llvm::Error used to communicate diagnostics in Status. Multiple
+/// diagnostics may be chained in an llvm::ErrorList.
+class DetailedExpressionError
+ : public llvm::ErrorInfo<DetailedExpressionError, llvm::ECError> {
+ DiagnosticDetail m_detail;
+
+public:
+ using llvm::ErrorInfo<DetailedExpressionError, llvm::ECError>::ErrorInfo;
+ DetailedExpressionError(DiagnosticDetail detail) : m_detail(detail) {}
+ std::string message() const override;
+ static char ID;
+};
+
enum DiagnosticOrigin {
eDiagnosticOriginUnknown = 0,
eDiagnosticOriginLLDB,
@@ -49,37 +93,28 @@ class Diagnostic {
}
}
- Diagnostic(llvm::StringRef message, lldb::Severity severity,
- DiagnosticOrigin origin, uint32_t compiler_id)
- : m_message(message), m_severity(severity), m_origin(origin),
- m_compiler_id(compiler_id) {}
-
- Diagnostic(const Diagnostic &rhs)
- : m_message(rhs.m_message), m_severity(rhs.m_severity),
- m_origin(rhs.m_origin), m_compiler_id(rhs.m_compiler_id) {}
+ Diagnostic(DiagnosticOrigin origin, uint32_t compiler_id,
+ DiagnosticDetail detail)
+ : m_origin(origin), m_compiler_id(compiler_id), m_detail(detail) {}
virtual ~Diagnostic() = default;
virtual bool HasFixIts() const { return false; }
- lldb::Severity GetSeverity() const { return m_severity; }
+ lldb::Severity GetSeverity() const { return m_detail.severity; }
uint32_t GetCompilerID() const { return m_compiler_id; }
- llvm::StringRef GetMessage() const { return m_message; }
+ llvm::StringRef GetMessage() const { return m_detail.message; }
+ llvm::Error GetAsError() const;
- void AppendMessage(llvm::StringRef message,
- bool precede_with_newline = true) {
- if (precede_with_newline)
- m_message.push_back('\n');
- m_message += message;
- }
+ void AppendMessage(llvm::StringRef message, bool precede_with_newline = true);
protected:
- std::string m_message;
- lldb::Severity m_severity;
DiagnosticOrigin m_origin;
- uint32_t m_compiler_id; // Compiler-specific diagnostic ID
+ /// Compiler-specific diagnostic ID.
+ uint32_t m_compiler_id;
+ DiagnosticDetail m_detail;
};
typedef std::vector<std::unique_ptr<Diagnostic>> DiagnosticList;
@@ -102,10 +137,7 @@ class DiagnosticManager {
void AddDiagnostic(llvm::StringRef message, lldb::Severity severity,
DiagnosticOrigin origin,
- uint32_t compiler_id = LLDB_INVALID_COMPILER_ID) {
- m_diagnostics.emplace_back(
- std::make_unique<Diagnostic>(message, severity, origin, compiler_id));
- }
+ uint32_t compiler_id = LLDB_INVALID_COMPILER_ID);
void AddDiagnostic(std::unique_ptr<Diagnostic> diagnostic) {
if (diagnostic)
@@ -130,6 +162,14 @@ class DiagnosticManager {
m_diagnostics.back()->AppendMessage(str);
}
+ /// Copies the diagnostics into an llvm::Error{List}.
+ /// The first diagnostic wraps \c result.
+ llvm::Error GetAsError(lldb::ExpressionResults result) const;
+
+ /// Copies the diagnostics into an llvm::Error, the first diagnostic
+ /// being an llvm::StringError.
+ llvm::Error GetAsError(llvm::Twine msg) const;
+
// Returns a string containing errors in this format:
//
// "error: error text\n
diff --git a/lldb/include/lldb/Utility/Status.h b/lldb/include/lldb/Utility/Status.h
index 64e193f8d84206..ff100441986e66 100644
--- a/lldb/include/lldb/Utility/Status.h
+++ b/lldb/include/lldb/Utility/Status.h
@@ -175,8 +175,11 @@ class 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.
+ /// FIXME: Replace all uses with takeError() instead.
llvm::Error ToError() const;
+
+ llvm::Error takeError() { return std::move(m_error); }
+
/// Don't call this function in new code. Instead, redesign the API
/// to use llvm::Expected instead of Status.
Status Clone() const { return Status(ToError()); }
diff --git a/lldb/source/Breakpoint/BreakpointLocation.cpp b/lldb/source/Breakpoint/BreakpointLocation.cpp
index 8d7364052a006a..fd9248eb0677c5 100644
--- a/lldb/source/Breakpoint/BreakpointLocation.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocation.cpp
@@ -264,9 +264,9 @@ bool BreakpointLocation::ConditionSaysStop(ExecutionContext &exe_ctx,
if (!m_user_expression_sp->Parse(diagnostics, exe_ctx,
eExecutionPolicyOnlyWhenNeeded, true,
false)) {
- error = Status::FromErrorStringWithFormat(
- "Couldn't parse conditional expression:\n%s",
- diagnostics.GetString().c_str());
+ error = Status::FromError(
+ diagnostics.GetAsError("Couldn't parse conditional expression:"));
+
m_user_expression_sp.reset();
return true;
}
@@ -324,8 +324,8 @@ bool BreakpointLocation::ConditionSaysStop(ExecutionContext &exe_ctx,
}
} else {
ret = false;
- error = Status::FromErrorStringWithFormat(
- "Couldn't execute expression:\n%s", diagnostics.GetString().c_str());
+ error = Status::FromError(
+ diagnostics.GetAsError("Couldn't execute expression:"));
}
return ret;
diff --git a/lldb/source/Expression/DiagnosticManager.cpp b/lldb/source/Expression/DiagnosticManager.cpp
index a8330138f3d53b..f05782c5528387 100644
--- a/lldb/source/Expression/DiagnosticManager.cpp
+++ b/lldb/source/Expression/DiagnosticManager.cpp
@@ -14,22 +14,7 @@
#include "lldb/Utility/StreamString.h"
using namespace lldb_private;
-
-void DiagnosticManager::Dump(Log *log) {
- if (!log)
- return;
-
- std::string str = GetString();
-
- // GetString() puts a separator after each diagnostic. We want to remove the
- // last '\n' because log->PutCString will add one for us.
-
- if (str.size() && str.back() == '\n') {
- str.pop_back();
- }
-
- log->PutCString(str.c_str());
-}
+char DetailedExpressionError::ID;
static const char *StringForSeverity(lldb::Severity severity) {
switch (severity) {
@@ -44,9 +29,16 @@ static const char *StringForSeverity(lldb::Severity severity) {
llvm_unreachable("switch needs another case for lldb::Severity enum");
}
+std::string DetailedExpressionError::message() const {
+ std::string str;
+ llvm::raw_string_ostream(str)
+ << StringForSeverity(m_detail.severity) << m_detail.rendered;
+ return str;
+}
+
std::string DiagnosticManager::GetString(char separator) {
- std::string ret;
- llvm::raw_string_ostream stream(ret);
+ std::string str;
+ llvm::raw_string_ostream stream(str);
for (const auto &diagnostic : Diagnostics()) {
llvm::StringRef severity = StringForSeverity(diagnostic->GetSeverity());
@@ -61,8 +53,50 @@ std::string DiagnosticManager::GetString(char separator) {
stream << message.drop_front(severity_pos + severity.size());
stream << separator;
}
+ return str;
+}
- return ret;
+void DiagnosticManager::Dump(Log *log) {
+ if (!log)
+ return;
+
+ std::string str = GetString();
+
+ // We want to remove the last '\n' because log->PutCString will add
+ // one for us.
+
+ if (str.size() && str.back() == '\n')
+ str.pop_back();
+
+ log->PutString(str);
+}
+
+llvm::Error Diagnostic::GetAsError() const {
+ return llvm::make_error<DetailedExpressionError>(m_detail);
+}
+
+llvm::Error
+DiagnosticManager::GetAsError(lldb::ExpressionResults result) const {
+ llvm::Error diags = Status::FromExpressionError(result, "").takeError();
+ for (const auto &diagnostic : m_diagnostics)
+ diags = llvm::joinErrors(std::move(diags), diagnostic->GetAsError());
+ return diags;
+}
+
+llvm::Error DiagnosticManager::GetAsError(llvm::Twine msg) const {
+ llvm::Error diags = llvm::createStringError(msg);
+ for (const auto &diagnostic : m_diagnostics)
+ diags = llvm::joinErrors(std::move(diags), diagnostic->GetAsError());
+ return diags;
+}
+
+void DiagnosticManager::AddDiagnostic(llvm::StringRef message,
+ lldb::Severity severity,
+ DiagnosticOrigin origin,
+ uint32_t compiler_id) {
+ m_diagnostics.emplace_back(std::make_unique<Diagnostic>(
+ origin, compiler_id,
+ DiagnosticDetail{{}, severity, message.str(), message.str()}));
}
size_t DiagnosticManager::Printf(lldb::Severity severity, const char *format,
@@ -85,3 +119,13 @@ void DiagnosticManager::PutString(lldb::Severity severity,
return;
AddDiagnostic(str, severity, eDiagnosticOriginLLDB);
}
+
+void Diagnostic::AppendMessage(llvm::StringRef message,
+ bool precede_with_newline) {
+ if (precede_with_newline) {
+ m_detail.message.push_back('\n');
+ m_detail.rendered.push_back('\n');
+ }
+ m_detail.message += message;
+ m_detail.rendered += message;
+}
diff --git a/lldb/source/Expression/ExpressionParser.cpp b/lldb/source/Expression/ExpressionParser.cpp
index 868556c1c58a57..26bc19874c53cc 100644
--- a/lldb/source/Expression/ExpressionParser.cpp
+++ b/lldb/source/Expression/ExpressionParser.cpp
@@ -63,9 +63,8 @@ ExpressionParser::RunStaticInitializers(IRExecutionUnitSP &execution_unit_sp,
exe_ctx, call_static_initializer, options, execution_errors);
if (results != eExpressionCompleted) {
- err = Status::FromErrorStringWithFormat(
- "couldn't run static initializer: %s",
- execution_errors.GetString().c_str());
+ err = Status::FromError(
+ execution_errors.GetAsError("couldn't run static initializer:"));
return err;
}
}
diff --git a/lldb/source/Expression/UserExpression.cpp b/lldb/source/Expression/UserExpression.cpp
index 872f6304f91baa..3e035cfbff567b 100644
--- a/lldb/source/Expression/UserExpression.cpp
+++ b/lldb/source/Expression/UserExpression.cpp
@@ -328,18 +328,20 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
}
if (!parse_success) {
- std::string msg;
- {
- llvm::raw_string_ostream os(msg);
- if (!diagnostic_manager.Diagnostics().empty())
- os << diagnostic_manager.GetString();
- else
- os << "expression failed to parse (no further compiler diagnostics)";
- if (target->GetEnableNotifyAboutFixIts() && fixed_expression &&
- !fixed_expression->empty())
- os << "\nfixed expression suggested:\n " << *fixed_expression;
+ if (target->GetEnableNotifyAboutFixIts() && fixed_expression &&
+ !fixed_expression->empty()) {
+ std::string fixit =
+ "fixed expression suggested:\n " + *fixed_expression;
+ diagnostic_manager.AddDiagnostic(fixit, lldb::eSeverityInfo,
+ eDiagnosticOriginLLDB);
}
- error = Status::FromExpressionError(execution_results, msg);
+ if (!diagnostic_manager.Diagnostics().size())
+ error = Status::FromExpressionError(
+ execution_results,
+ "expression failed to parse (no further compiler diagnostics)");
+ else
+ error =
+ Status::FromError(diagnostic_manager.GetAsError(execution_results));
}
}
@@ -384,8 +386,8 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
error = Status::FromExpressionError(
execution_results, "expression failed to execute, unknown error");
else
- error = Status::FromExpressionError(execution_results,
- diagnostic_manager.GetString());
+ error = Status::FromError(
+ diagnostic_manager.GetAsError(execution_results));
} else {
if (expr_result) {
result_valobj_sp = expr_result->GetValueObject();
diff --git a/lldb/source/Expression/UtilityFunction.cpp b/lldb/source/Expression/UtilityFunction.cpp
index 55ebfb8ef342e8..e35c0774815298 100644
--- a/lldb/source/Expression/UtilityFunction.cpp
+++ b/lldb/source/Expression/UtilityFunction.cpp
@@ -83,19 +83,18 @@ FunctionCaller *UtilityFunction::MakeFunctionCaller(
m_caller_up.reset(process_sp->GetTarget().GetFunctionCallerForLanguage(
Language().AsLanguageType(), return_type, impl_code_address,
arg_value_list, name.c_str(), error));
- if (error.Fail()) {
-
+ if (error.Fail())
return nullptr;
- }
+
if (m_caller_up) {
DiagnosticManager diagnostics;
unsigned num_errors =
m_caller_up->CompileFunction(thread_to_use_sp, diagnostics);
if (num_errors) {
- error = Status::FromErrorStringWithFormat(
- "Error compiling %s caller function: \"%s\".",
- m_function_name.c_str(), diagnostics.GetString().c_str());
+ error = Status::FromError(diagnostics.GetAsError(
+ "Error compiling " + m_function_name + " caller function:"));
+
m_caller_up.reset();
return nullptr;
}
@@ -104,9 +103,8 @@ FunctionCaller *UtilityFunction::MakeFunctionCaller(
ExecutionContext exe_ctx(process_sp);
if (!m_caller_up->WriteFunctionWrapper(exe_ctx, diagnostics)) {
- error = Status::FromErrorStringWithFormat(
- "Error inserting caller function for %s: \"%s\".",
- m_function_name.c_str(), diagnostics.GetString().c_str());
+ error = Status::FromError(diagnostics.GetAsError(
+ "Error inserting " + m_function_name + " caller function:"));
m_caller_up.reset();
return nullptr;
}
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
index 21abd71cc34eeb..c473df808ee8d0 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
@@ -29,9 +29,8 @@ class ClangDiagnostic : public Diagnostic {
return diag->getKind() == eDiagnosticOriginClang;
}
- ClangDiagnostic(llvm::StringRef message, lldb::Severity severity,
- uint32_t compiler_id)
- : Diagnostic(message, severity, eDiagnosticOriginClang, compiler_id) {}
+ ClangDiagnostic(DiagnosticDetail detail, uint32_t compiler_id)
+ : Diagnostic(eDiagnosticOriginClang, compiler_id, detail) {}
~ClangDiagnostic() override = default;
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 2fe3c0460aa7f8..07b1660e203870 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -26,6 +26,7 @@
#include "clang/Frontend/FrontendActions.h"
#include "clang/Frontend/FrontendDiagnostic.h"
#include "clang/Frontend/FrontendPluginRegistry.h"
+#include "clang/Frontend/TextDiagnostic.h"
#include "clang/Frontend/TextDiagnosticBuffer.h"
#include "clang/Frontend/TextDiagnosticPrinter.h"
#include "clang/Lex/Preprocessor.h"
@@ -212,20 +213,20 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
m_passthrough->HandleDiagnostic(DiagLevel, Info);
m_os->flush();
- lldb::Severity severity;
+ DiagnosticDetail detail;
bool make_new_diagnostic = true;
switch (DiagLevel) {
case DiagnosticsEngine::Level::Fatal:
case DiagnosticsEngine::Level::Error:
- severity = lldb::eSeverityError;
+ detail.severity = lldb::eSeverityError;
break;
case DiagnosticsEngine::Level::Warning:
- severity = lldb::eSeverityWarning;
+ detail.severity = lldb::eSeverityWarning;
break;
case DiagnosticsEngine::Level::Remark:
case DiagnosticsEngine::Level::Ignored:
- severity = lldb::eSeverityInfo;
+ detail.severity = lldb::eSeverityInfo;
break;
case DiagnosticsEngine::Level::Note:
m_manager->AppendMessageToDiagnostic(m_output);
@@ -254,14 +255,46 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
std::string stripped_output =
std::string(llvm::StringRef(m_output).trim());
- auto new_diagnostic = std::make_unique<ClangDiagnostic>(
- stripped_output, severity, Info.getID());
+ // Translate the source location.
+ if (Info.hasSourceManager()) {
+ DiagnosticDetail::SourceLocation loc;
+ clang::SourceManager &sm = Info.getSourceManager();
+ const clang::SourceLocation sloc = Info.getLocation();
+ if (sloc.isValid()) {
+ const clang::FullSourceLoc fsloc(sloc, sm);
+ clang::PresumedLoc PLoc = fsloc.getPresumedLoc(true);
+ StringRef filename =
+ PLoc.isValid() ? PLoc.getFilename() : StringRef{};
+ loc.file = FileSpec(filename);
+ loc.line = fsloc.getSpellingLineNumber();
+ loc.column = fsloc.getSpellingColumnNumber();
+ // A heuristic detecting the #line 1 "<user expression 1>".
+ loc.in_user_input = filename.starts_with("<user expression ");
+ // Find the range of the primary location.
+ for (const auto &range : Info.getRanges()) {
+ if (range.getBegin() == sloc) {
+ // FIXME: This is probably not handling wide characters correctly.
+ unsigned end_col = sm.getSpellingColumnNumber(range.getEnd());
+ if (end_col > loc.column)
+ loc.length = end_col - loc.column;
+ break;
+ }
+ }
+ detail.source_location = loc;
+ }
+ }
+ llvm::SmallString<0> msg;
+ Info.FormatDiagnostic(msg);
+ detail.message = msg.str();
+ detail.rendered = stripped_output;
+ auto new_diagnostic =
+ std::make_unique<ClangDiagnostic>(detail, Info.getID());
// Don't store away warning fixits, since the compiler doesn't have
// enough context in an expression for the warning to be useful.
// FIXME: Should we try to filter out FixIts that apply to our generated
// code, and not the user's expression?
- if (severity == lldb::eSeverityError)
+ if (detail.severity == lldb::eSeverityError)
AddAllFixIts(new_diagnostic.get(), Info);
m_manager->AddDiagnostic(std::move(new_diagnostic));
@@ -1503,13 +1536,9 @@ lldb_private::Status ClangExpressionParser::DoPrepareForExecution(
new ClangDynamicCheckerFunctions();
DiagnosticManager install_diags;
- if (Error Err = dynamic_checkers->Install(install_diags, exe_ctx)) {
- std::string ErrMsg = "couldn't install checkers: " + toString(std::move(Err));
- if (install_diags.Diagnostics().size())
- ErrMsg = ErrMsg + "\n" + install_diags.GetString().c_str();
- err = Status(ErrMsg);
- return err;
- }
+ if (Error Err = dynamic_checkers->Install(install_diags, exe_ctx))
+ return Status::FromError(
+ install_diags.GetAsError("couldn't install checkers:"));
process->SetDynamicCheckers(dynamic_checkers);
diff --git a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
index 31315e46ca168d..5d873bf73da260 100644
--- a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
+++ b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
@@ -863,9 +863,8 @@ uint32_t PlatformPOSIX::DoLoadImage(lldb_private::Process *process,
func_args_addr,
arguments,
diagnostics)) {
- error = Status::FromErrorStringWithFormat(
- "dlopen error: could not write function arguments: %s",
- diagnostics.GetString().c_str());
+ error = Status::FromError(diagnostics.GetAsError(
+ "dlopen error: could not write function arguments:"));
return LLDB_INVALID_IMAGE_TOKEN;
}
@@ -906,9 +905,8 @@ uint32_t PlatformPOSIX::DoLoadImage(lldb_private::Process *process,
ExpressionResults results = do_dlopen_function->ExecuteFunction(
exe_ctx, &func_args_addr, options, diagnostics, return_value);
if (results != eExpressionCompleted) {
- error = Status::FromErrorStringWithFormat(
- "dlopen error: failed executing dlopen wrapper function: %s",
- diagnostics.GetString().c_str());
+ error = Status::FromError(diagnostics.GetAsError(
+ "dlopen error: failed executing dlopen wrapper function:"));
return LLDB_INVALID_IMAGE_TOKEN;
}
diff --git a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
index 7352d6f33f2174..fa39d93a91297c 100644
--- a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -341,9 +341,8 @@ uint32_t PlatformWindows::DoLoadImage(Process *process,
diagnostics.Clear();
if (!invocation->WriteFunctionArguments(context, injected_parameters,
parameters, diagnostics)) {
- error = Status::FromErrorStringWithFormat(
- "LoadLibrary error: unable to write function parameters: %s",
- diagnostics.GetString().c_str());
+ error = Status::FromError(diagnostics.GetAsError(
+ "LoadLibrary error: unable to write function parameters:"));
return LLDB_INVALID_IMAGE_TOKEN;
}
@@ -384,9 +383,8 @@ uint32_t PlatformWindows::DoLoadImage(Process *process,
invocation->ExecuteFunction(context, &injected_parameters, options,
diagnostics, value);
if (result != eExpressionCompleted) {
- error = Status::FromErrorStringWithFormat(
- "LoadLibrary error: failed to execute LoadLibrary helper: %s",
- diagnostics.GetString().c_str());
+ error = Status::FromError(diagnostics.GetAsError(
+ "LoadLibrary error: failed to execute LoadLibrary helper:"));
return LLDB_INVALID_IMAGE_TOKEN;
}
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index f1659aae0800db..57061ecd5bd445 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -2605,7 +2605,7 @@ Target::CreateUtilityFunction(std::string expression, std::string name,
DiagnosticManager diagnostics;
if (!utility_fn->Install(diagnostics, exe_ctx))
- return llvm::createStringError(diagnostics.GetString());
+ return diagnostics.GetAsError("Could not install utility function:");
return std::move(utility_fn);
}
diff --git a/lldb/source/Utility/Status.cpp b/lldb/source/Utility/Status.cpp
index 0daef67b13d8d1..03a919e0a9da2c 100644
--- a/lldb/source/Utility/Status.cpp
+++ b/lldb/source/Utility/Status.cpp
@@ -220,6 +220,14 @@ const char *Status::AsCString(const char *default_error_str) const {
if (!m_string.empty() && m_string[m_string.size() - 1] == '\n')
m_string.pop_back();
+ // FIXME: Workaround for ErrorList[ExpressionError, ...].
+ if (m_error.isA<llvm::ErrorList>()) {
+ while (!m_string.empty() && m_string[0] == '\n')
+ m_string = std::string(m_string.data() + 1, m_string.size() - 1);
+ if (!m_string.empty() && m_string[m_string.size() - 1] != '\n')
+ m_string += '\n';
+ }
+
if (m_string.empty()) {
if (default_error_str)
m_string.assign(default_error_str);
diff --git a/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py b/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py
index 620b6e44fc852a..36e302be2525b5 100644
--- a/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py
+++ b/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py
@@ -21,7 +21,7 @@ def test(self):
"expr @import LLDBTestModule",
error=True,
substrs=[
- "module.h:4:1: use of undeclared identifier 'syntax_error_for_lldb_to_find'",
+ "module.h:4:1: error: use of undeclared identifier 'syntax_error_for_lldb_to_find'",
"syntax_error_for_lldb_to_find // comment that tests source printing",
"could not build module 'LLDBTestModule'",
],
diff --git a/lldb/unittests/Expression/DiagnosticManagerTest.cpp b/lldb/unittests/Expression/DiagnosticManagerTest.cpp
index 05fe7c164d6818..596baedd728ffa 100644
--- a/lldb/unittests/Expression/DiagnosticManagerTest.cpp
+++ b/lldb/unittests/Expression/DiagnosticManagerTest.cpp
@@ -19,8 +19,8 @@ class FixItDiag : public Diagnostic {
public:
FixItDiag(llvm::StringRef msg, bool has_fixits)
- : Diagnostic(msg, lldb::eSeverityError,
- DiagnosticOrigin::eDiagnosticOriginLLDB, custom_diag_id),
+ : Diagnostic(DiagnosticOrigin::eDiagnosticOriginLLDB, custom_diag_id,
+ DiagnosticDetail{{}, lldb::eSeverityError, msg.str(), {}}),
m_has_fixits(has_fixits) {}
bool HasFixIts() const override { return m_has_fixits; }
};
@@ -30,8 +30,8 @@ namespace {
class TextDiag : public Diagnostic {
public:
TextDiag(llvm::StringRef msg, lldb::Severity severity)
- : Diagnostic(msg, severity, DiagnosticOrigin::eDiagnosticOriginLLDB,
- custom_diag_id) {}
+ : Diagnostic(DiagnosticOrigin::eDiagnosticOriginLLDB, custom_diag_id,
+ DiagnosticDetail{{}, severity, msg.str(), {}}) {}
};
} // namespace
@@ -42,8 +42,8 @@ TEST(DiagnosticManagerTest, AddDiagnostic) {
std::string msg = "foo bar has happened";
lldb::Severity severity = lldb::eSeverityError;
DiagnosticOrigin origin = DiagnosticOrigin::eDiagnosticOriginLLDB;
- auto diag =
- std::make_unique<Diagnostic>(msg, severity, origin, custom_diag_id);
+ auto diag = std::make_unique<Diagnostic>(
+ origin, custom_diag_id, DiagnosticDetail{{}, severity, msg, {}});
mgr.AddDiagnostic(std::move(diag));
EXPECT_EQ(1U, mgr.Diagnostics().size());
const Diagnostic *got = mgr.Diagnostics().front().get();
diff --git a/lldb/unittests/Utility/StatusTest.cpp b/lldb/unittests/Utility/StatusTest.cpp
index e37c94ac17f2d0..48110b160be93c 100644
--- a/lldb/unittests/Utility/StatusTest.cpp
+++ b/lldb/unittests/Utility/StatusTest.cpp
@@ -55,7 +55,7 @@ TEST(StatusTest, ErrorCodeConstructor) {
llvm::Error list = llvm::joinErrors(llvm::createStringError("foo"),
llvm::createStringError("bar"));
Status foobar = Status::FromError(std::move(list));
- EXPECT_EQ(std::string("foo\nbar"), std::string(foobar.AsCString()));
+ EXPECT_EQ(std::string("foo\nbar\n"), std::string(foobar.AsCString()));
}
TEST(StatusTest, ErrorConversion) {
More information about the lldb-commits
mailing list