[Lldb-commits] [lldb] [lldb] Inline expression evaluator error visualization (PR #106470)
Adrian Prantl via lldb-commits
lldb-commits at lists.llvm.org
Mon Sep 16 16:52:53 PDT 2024
https://github.com/adrian-prantl updated https://github.com/llvm/llvm-project/pull/106470
>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/4] [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/4] [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/4] [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) {
>From 81eba0a82ef0c65b5fca5ca9db38d35495ffaac5 Mon Sep 17 00:00:00 2001
From: Adrian Prantl <aprantl at apple.com>
Date: Wed, 28 Aug 2024 16:19:21 -0700
Subject: [PATCH 4/4] [lldb] Inline expression evaluator error visualization
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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
Before:
```
$ lldb -o "expr a+b"
(lldb) expr a+b
error: <user expression 0>:1:1: use of undeclared identifier 'a'
a+b
^
error: <user expression 0>:1:3: use of undeclared identifier 'b'
a+b
^
```
After:
```
(lldb) expr a+b
^ ^
│ ╰─ error: use of undeclared identifier 'b'
╰─ error: use of undeclared identifier 'a'
```
This eliminates the confusing `<user expression 0>:1:3` source
location and avoids echoing the expression to the console again, which
results in a cleaner presentation that makes it easier to grasp what's
going on.
---
.../lldb/Expression/DiagnosticManager.h | 9 +-
lldb/include/lldb/Interpreter/CommandObject.h | 8 +
.../Commands/CommandObjectExpression.cpp | 141 ++++++++++++++++--
lldb/source/Expression/DiagnosticManager.cpp | 12 ++
.../source/Interpreter/CommandInterpreter.cpp | 4 +-
.../ctf/CommandObjectThreadTraceExportCTF.h | 2 +-
lldb/source/Utility/Status.cpp | 3 +-
.../TestPersistentVariables.py | 4 +-
.../TestStaticInitializers.py | 2 +-
.../TestTemplateFunctions.py | 2 +-
.../test/API/lang/mixed/TestMixedLanguages.py | 2 +-
lldb/test/Shell/Expr/TestObjCIDCast.test | 2 +-
.../test/Shell/Expr/TestObjCInCXXContext.test | 2 +-
.../NativePDB/incomplete-tag-type.cpp | 4 +-
.../Expression/DiagnosticManagerTest.cpp | 35 +++--
lldb/unittests/Interpreter/CMakeLists.txt | 1 +
.../TestCommandObjectExpression.cpp | 29 ++++
17 files changed, 220 insertions(+), 42 deletions(-)
create mode 100644 lldb/unittests/Interpreter/TestCommandObjectExpression.cpp
diff --git a/lldb/include/lldb/Expression/DiagnosticManager.h b/lldb/include/lldb/Expression/DiagnosticManager.h
index fb9502f4b3ffba..1c0763eea92f05 100644
--- a/lldb/include/lldb/Expression/DiagnosticManager.h
+++ b/lldb/include/lldb/Expression/DiagnosticManager.h
@@ -54,13 +54,18 @@ struct DiagnosticDetail {
/// 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> {
+ : public llvm::ErrorInfo<DetailedExpressionError, CloneableError> {
DiagnosticDetail m_detail;
public:
- using llvm::ErrorInfo<DetailedExpressionError, llvm::ECError>::ErrorInfo;
+ using llvm::ErrorInfo<DetailedExpressionError, CloneableError>::ErrorInfo;
DetailedExpressionError(DiagnosticDetail detail) : m_detail(detail) {}
std::string message() const override;
+ DiagnosticDetail GetDetail() const { return m_detail; }
+ std::error_code convertToErrorCode() const override;
+ void log(llvm::raw_ostream &OS) const override;
+ std::unique_ptr<CloneableError> Clone() const override;
+
static char ID;
};
diff --git a/lldb/include/lldb/Interpreter/CommandObject.h b/lldb/include/lldb/Interpreter/CommandObject.h
index 20c4769af90338..c5167e5e0ecb6a 100644
--- a/lldb/include/lldb/Interpreter/CommandObject.h
+++ b/lldb/include/lldb/Interpreter/CommandObject.h
@@ -340,6 +340,13 @@ class CommandObject : public std::enable_shared_from_this<CommandObject> {
return false;
}
+ /// Set the command input as it appeared in the terminal. This
+ /// is used to have errors refer directly to the original command.
+ void SetOriginalCommandString(std::string s) { m_original_command = s; }
+
+ /// \param offset_in_command is on what column \c args_string
+ /// appears, if applicable. This enables diagnostics that refer back
+ /// to the user input.
virtual void Execute(const char *args_string,
CommandReturnObject &result) = 0;
@@ -404,6 +411,7 @@ class CommandObject : public std::enable_shared_from_this<CommandObject> {
std::string m_cmd_help_short;
std::string m_cmd_help_long;
std::string m_cmd_syntax;
+ std::string m_original_command;
Flags m_flags;
std::vector<CommandArgumentEntry> m_arguments;
lldb::CommandOverrideCallback m_deprecated_command_override_callback;
diff --git a/lldb/source/Commands/CommandObjectExpression.cpp b/lldb/source/Commands/CommandObjectExpression.cpp
index 771194638e1b65..116dda809f2a80 100644
--- a/lldb/source/Commands/CommandObjectExpression.cpp
+++ b/lldb/source/Commands/CommandObjectExpression.cpp
@@ -10,6 +10,7 @@
#include "CommandObjectExpression.h"
#include "lldb/Core/Debugger.h"
+#include "lldb/Expression/DiagnosticManager.h"
#include "lldb/Expression/ExpressionVariable.h"
#include "lldb/Expression/REPL.h"
#include "lldb/Expression/UserExpression.h"
@@ -398,6 +399,104 @@ CanBeUsedForElementCountPrinting(ValueObject &valobj) {
return Status();
}
+static llvm::raw_ostream &PrintSeverity(Stream &stream,
+ lldb::Severity severity) {
+ llvm::HighlightColor color;
+ llvm::StringRef text;
+ switch (severity) {
+ case eSeverityError:
+ color = llvm::HighlightColor::Error;
+ text = "error: ";
+ break;
+ case eSeverityWarning:
+ color = llvm::HighlightColor::Warning;
+ text = "warning: ";
+ break;
+ case eSeverityInfo:
+ color = llvm::HighlightColor::Remark;
+ text = "note: ";
+ break;
+ }
+ return llvm::WithColor(stream.AsRawOstream(), color, llvm::ColorMode::Enable)
+ << text;
+}
+
+namespace lldb_private {
+// Public for unittesting.
+std::string RenderDiagnosticDetails(std::optional<uint16_t> offset_in_command,
+ llvm::ArrayRef<DiagnosticDetail> details) {
+ if (details.empty())
+ return {};
+
+ StreamString stream(true);
+ if (!offset_in_command) {
+ for (const DiagnosticDetail &detail : details) {
+ PrintSeverity(stream, detail.severity);
+ stream << detail.rendered << '\n';
+ }
+ return stream.GetData();
+ }
+
+ // Print a line with caret indicator(s) below the lldb prompt + command.
+ const size_t padding = *offset_in_command;
+ stream << std::string(padding, ' ');
+
+ size_t offset = 1;
+ std::vector<DiagnosticDetail> remaining_details, other_details;
+ for (const DiagnosticDetail &detail : details) {
+ if (!detail.source_location) {
+ other_details.push_back(detail);
+ continue;
+ }
+
+ auto &loc = *detail.source_location;
+ remaining_details.push_back(detail);
+ if (offset > loc.column)
+ continue;
+ stream << std::string(loc.column - offset, ' ') << '^';
+ if (loc.length > 1)
+ stream << std::string(loc.length - 1, '~');
+ offset = loc.column + 1;
+ }
+ stream << '\n';
+
+ // Work through each detail in reverse order using the vector/stack.
+ for (auto detail = remaining_details.rbegin();
+ detail != remaining_details.rend();
+ ++detail, remaining_details.pop_back()) {
+ // Get the information to print this detail and remove it from the stack.
+ // Print all the lines for all the other messages first.
+ stream << std::string(padding, ' ');
+ size_t offset = 1;
+ for (auto &remaining_detail :
+ llvm::ArrayRef(remaining_details).drop_back(1)) {
+ uint16_t column = remaining_detail.source_location->column;
+ stream << std::string(column - offset, ' ') << "│";
+ offset = column + 1;
+ }
+
+ // Print the line connecting the ^ with the error message.
+ uint16_t column = detail->source_location->column;
+ if (offset <= column)
+ stream << std::string(column - offset, ' ') << "╰─ ";
+
+ // Print a colorized string based on the message's severity type.
+ PrintSeverity(stream, detail->severity);
+
+ // Finally, print the message and start a new line.
+ stream << detail->message << '\n';
+ }
+
+ // Print the non-located details.
+ for (const DiagnosticDetail &detail : other_details) {
+ PrintSeverity(stream, detail.severity);
+ stream << detail.message << '\n';
+ }
+
+ return stream.GetData();
+}
+} // namespace lldb_private
+
bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
Stream &output_stream,
Stream &error_stream,
@@ -486,19 +585,37 @@ bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
result.SetStatus(eReturnStatusSuccessFinishResult);
} else {
- const char *error_cstr = result_valobj_sp->GetError().AsCString();
- if (error_cstr && error_cstr[0]) {
- const size_t error_cstr_len = strlen(error_cstr);
- const bool ends_with_newline = error_cstr[error_cstr_len - 1] == '\n';
- if (strstr(error_cstr, "error:") != error_cstr)
- error_stream.PutCString("error: ");
- error_stream.Write(error_cstr, error_cstr_len);
- if (!ends_with_newline)
- error_stream.EOL();
- } else {
- error_stream.PutCString("error: unknown error\n");
+ // Retrieve the diagnostics.
+ std::vector<DiagnosticDetail> details;
+ llvm::consumeError(
+ llvm::handleErrors(result_valobj_sp->GetError().ToError(),
+ [&](DetailedExpressionError &error) {
+ details.push_back(error.GetDetail());
+ }));
+ // Find the position of the expression in the command.
+ std::optional<uint16_t> expr_pos;
+ size_t nchar = m_original_command.find(expr);
+ if (nchar != std::string::npos)
+ expr_pos = nchar + GetDebugger().GetPrompt().size();
+
+ std::string error = RenderDiagnosticDetails(expr_pos, details);
+ if (!error.empty())
+ error_stream.PutCString(error.c_str());
+ else {
+ const char *error_cstr = result_valobj_sp->GetError().AsCString();
+ if (error_cstr && error_cstr[0]) {
+ const size_t error_cstr_len = strlen(error_cstr);
+ const bool ends_with_newline =
+ error_cstr[error_cstr_len - 1] == '\n';
+ if (strstr(error_cstr, "error:") != error_cstr)
+ error_stream.PutCString("error: ");
+ error_stream.Write(error_cstr, error_cstr_len);
+ if (!ends_with_newline)
+ error_stream.EOL();
+ } else {
+ error_stream.PutCString("error: unknown error\n");
+ }
}
-
result.SetStatus(eReturnStatusFailed);
}
}
diff --git a/lldb/source/Expression/DiagnosticManager.cpp b/lldb/source/Expression/DiagnosticManager.cpp
index f05782c5528387..99533aefb685b1 100644
--- a/lldb/source/Expression/DiagnosticManager.cpp
+++ b/lldb/source/Expression/DiagnosticManager.cpp
@@ -36,6 +36,18 @@ std::string DetailedExpressionError::message() const {
return str;
}
+std::error_code DetailedExpressionError::convertToErrorCode() const {
+ return llvm::inconvertibleErrorCode();
+}
+
+void DetailedExpressionError::log(llvm::raw_ostream &OS) const {
+ OS << message();
+}
+
+std::unique_ptr<CloneableError> DetailedExpressionError::Clone() const {
+ return std::make_unique<DetailedExpressionError>(m_detail);
+}
+
std::string DiagnosticManager::GetString(char separator) {
std::string str;
llvm::raw_string_ostream stream(str);
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index b93f47a8a8d5ec..49681f06802443 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -1887,7 +1887,8 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
CommandReturnObject &result,
bool force_repeat_command) {
std::string command_string(command_line);
- std::string original_command_string(command_line);
+ std::string original_command_string(command_string);
+ std::string real_original_command_string(command_string);
Log *log = GetLog(LLDBLog::Commands);
llvm::PrettyStackTraceFormat stack_trace("HandleCommand(command = \"%s\")",
@@ -2076,6 +2077,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
}
ElapsedTime elapsed(execute_time);
+ cmd_obj->SetOriginalCommandString(real_original_command_string);
cmd_obj->Execute(remainder.c_str(), result);
}
diff --git a/lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h b/lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h
index 1a034e87cfb65b..06834edf14ea16 100644
--- a/lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h
+++ b/lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h
@@ -48,7 +48,7 @@ class CommandObjectThreadTraceExportCTF : public CommandObjectParsed {
Options *GetOptions() override { return &m_options; }
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override;
+ void DoExecute(Args &args, CommandReturnObject &result) override;
CommandOptions m_options;
};
diff --git a/lldb/source/Utility/Status.cpp b/lldb/source/Utility/Status.cpp
index 03a919e0a9da2c..298cf538578966 100644
--- a/lldb/source/Utility/Status.cpp
+++ b/lldb/source/Utility/Status.cpp
@@ -8,6 +8,7 @@
#include "lldb/Utility/Status.h"
+#include "lldb/Expression/DiagnosticManager.h"
#include "lldb/Utility/LLDBLog.h"
#include "lldb/Utility/Log.h"
#include "lldb/Utility/VASPrintf.h"
@@ -275,8 +276,6 @@ ErrorType Status::GetType() const {
else if (error.convertToErrorCode().category() == lldb_generic_category() ||
error.convertToErrorCode() == llvm::inconvertibleErrorCode())
result = eErrorTypeGeneric;
- else
- result = eErrorTypeInvalid;
});
return result;
}
diff --git a/lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py b/lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py
index 6a7995ff2a837e..d0d9358589a719 100644
--- a/lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py
+++ b/lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py
@@ -56,7 +56,7 @@ def test_persistent_variables(self):
self.expect(
"expr int $i = 123",
error=True,
- substrs=["error: redefinition of persistent variable '$i'"],
+ substrs=["redefinition of persistent variable '$i'"],
)
self.expect_expr("$i", result_type="int", result_value="5")
@@ -65,7 +65,7 @@ def test_persistent_variables(self):
self.expect(
"expr long $i = 123",
error=True,
- substrs=["error: redefinition of persistent variable '$i'"],
+ substrs=["redefinition of persistent variable '$i'"],
)
self.expect_expr("$i", result_type="int", result_value="5")
diff --git a/lldb/test/API/commands/expression/static-initializers/TestStaticInitializers.py b/lldb/test/API/commands/expression/static-initializers/TestStaticInitializers.py
index ea3aa6a4608c41..c734033bd6c028 100644
--- a/lldb/test/API/commands/expression/static-initializers/TestStaticInitializers.py
+++ b/lldb/test/API/commands/expression/static-initializers/TestStaticInitializers.py
@@ -35,7 +35,7 @@ def test_failing_init(self):
self.expect(
"expr -p -- struct Foo2 { Foo2() { do_abort(); } }; Foo2 f;",
error=True,
- substrs=["error: couldn't run static initializer:"],
+ substrs=["couldn't run static initializer:"],
)
def test_without_process(self):
diff --git a/lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py b/lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py
index 71df90e6a6d161..8d4200b1076d03 100644
--- a/lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py
+++ b/lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py
@@ -21,7 +21,7 @@ def do_test_template_function(self, add_cast):
"expr b1 <=> b2",
error=True,
substrs=[
- "warning: <user expression 0>:1:4: '<=>' is a single token in C++20; add a space to avoid a change in behavior"
+ "warning:", "'<=>' is a single token in C++20; add a space to avoid a change in behavior"
],
)
diff --git a/lldb/test/API/lang/mixed/TestMixedLanguages.py b/lldb/test/API/lang/mixed/TestMixedLanguages.py
index 8b73254cce4a93..1637d59a5edcba 100644
--- a/lldb/test/API/lang/mixed/TestMixedLanguages.py
+++ b/lldb/test/API/lang/mixed/TestMixedLanguages.py
@@ -40,7 +40,7 @@ def cleanup():
self.runCmd("run")
self.expect("thread backtrace", substrs=["`main", "lang=c"])
# Make sure evaluation of C++11 fails.
- self.expect("expr foo != nullptr", error=True, startstr="error")
+ self.expect("expr foo != nullptr", error=True, substrs=["error"])
# Run to BP at foo (in foo.cpp) and test that the language is C++.
self.runCmd("breakpoint set -n foo")
diff --git a/lldb/test/Shell/Expr/TestObjCIDCast.test b/lldb/test/Shell/Expr/TestObjCIDCast.test
index 0611171da09e2e..19ca404643c1d9 100644
--- a/lldb/test/Shell/Expr/TestObjCIDCast.test
+++ b/lldb/test/Shell/Expr/TestObjCIDCast.test
@@ -6,4 +6,4 @@
// RUN: 2>&1 | FileCheck %s
// CHECK: (lldb) expression --language objc -- *(id)0x1
-// CHECK: error: Couldn't apply expression side effects : Couldn't dematerialize a result variable: couldn't read its memory
+// CHECK: error:{{.*}}Couldn't apply expression side effects : Couldn't dematerialize a result variable: couldn't read its memory
diff --git a/lldb/test/Shell/Expr/TestObjCInCXXContext.test b/lldb/test/Shell/Expr/TestObjCInCXXContext.test
index 8537799bdeb674..f8cad5b58a1e53 100644
--- a/lldb/test/Shell/Expr/TestObjCInCXXContext.test
+++ b/lldb/test/Shell/Expr/TestObjCInCXXContext.test
@@ -18,4 +18,4 @@
// CHECK-NEXT: (NSString *){{.*}}= nil
// CHECK: (lldb) expression NSString
-// CHECK-NEXT: error:{{.*}} use of undeclared identifier 'NSString'
+// CHECK: error:{{.*}}use of undeclared identifier 'NSString'
diff --git a/lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp b/lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp
index a249057282d890..8c168286903014 100644
--- a/lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp
+++ b/lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp
@@ -13,9 +13,7 @@
// CHECK: (lldb) expression d
// CHECK: (D) $1 = {}
// CHECK: (lldb) expression static_e_ref
-// CHECK: error: {{.*}}incomplete type 'E' where a complete type is required
-// CHECK: static_e_ref
-// CHECK: ^
+// CHECK: error:{{.*}}incomplete type 'E' where a complete type is required
// Complete base class.
struct A { int x; A(); };
diff --git a/lldb/unittests/Expression/DiagnosticManagerTest.cpp b/lldb/unittests/Expression/DiagnosticManagerTest.cpp
index 596baedd728ffa..c1980090740603 100644
--- a/lldb/unittests/Expression/DiagnosticManagerTest.cpp
+++ b/lldb/unittests/Expression/DiagnosticManagerTest.cpp
@@ -31,7 +31,7 @@ class TextDiag : public Diagnostic {
public:
TextDiag(llvm::StringRef msg, lldb::Severity severity)
: Diagnostic(DiagnosticOrigin::eDiagnosticOriginLLDB, custom_diag_id,
- DiagnosticDetail{{}, severity, msg.str(), {}}) {}
+ DiagnosticDetail{{}, severity, msg.str(), msg.str()}) {}
};
} // namespace
@@ -72,18 +72,25 @@ TEST(DiagnosticManagerTest, HasFixits) {
EXPECT_TRUE(mgr.HasFixIts());
}
+static std::string toString(DiagnosticManager &mgr) {
+ std::string s = llvm::toString(mgr.GetAsError(""));
+ if (s.empty())
+ return s;
+ return s.substr(1) + "\n";
+}
+
TEST(DiagnosticManagerTest, GetStringNoDiags) {
DiagnosticManager mgr;
- EXPECT_EQ("", mgr.GetString());
+ EXPECT_EQ("", toString(mgr));
std::unique_ptr<Diagnostic> empty;
mgr.AddDiagnostic(std::move(empty));
- EXPECT_EQ("", mgr.GetString());
+ EXPECT_EQ("", toString(mgr));
}
TEST(DiagnosticManagerTest, GetStringBasic) {
DiagnosticManager mgr;
mgr.AddDiagnostic(std::make_unique<TextDiag>("abc", lldb::eSeverityError));
- EXPECT_EQ("error: abc\n", mgr.GetString());
+ EXPECT_EQ("error: abc\n", toString(mgr));
}
TEST(DiagnosticManagerTest, GetStringMultiline) {
@@ -91,15 +98,15 @@ TEST(DiagnosticManagerTest, GetStringMultiline) {
// Multiline diagnostics should only get one severity label.
mgr.AddDiagnostic(std::make_unique<TextDiag>("b\nc", lldb::eSeverityError));
- EXPECT_EQ("error: b\nc\n", mgr.GetString());
+ EXPECT_EQ("error: b\nc\n", toString(mgr));
}
TEST(DiagnosticManagerTest, GetStringMultipleDiags) {
DiagnosticManager mgr;
mgr.AddDiagnostic(std::make_unique<TextDiag>("abc", lldb::eSeverityError));
- EXPECT_EQ("error: abc\n", mgr.GetString());
+ EXPECT_EQ("error: abc\n", toString(mgr));
mgr.AddDiagnostic(std::make_unique<TextDiag>("def", lldb::eSeverityError));
- EXPECT_EQ("error: abc\nerror: def\n", mgr.GetString());
+ EXPECT_EQ("error: abc\nerror: def\n", toString(mgr));
}
TEST(DiagnosticManagerTest, GetStringSeverityLabels) {
@@ -110,7 +117,7 @@ TEST(DiagnosticManagerTest, GetStringSeverityLabels) {
mgr.AddDiagnostic(std::make_unique<TextDiag>("bar", lldb::eSeverityWarning));
// Remarks have no labels.
mgr.AddDiagnostic(std::make_unique<TextDiag>("baz", lldb::eSeverityInfo));
- EXPECT_EQ("error: foo\nwarning: bar\nbaz\n", mgr.GetString());
+ EXPECT_EQ("error: foo\nwarning: bar\nbaz\n", toString(mgr));
}
TEST(DiagnosticManagerTest, GetStringPreserveOrder) {
@@ -120,7 +127,7 @@ TEST(DiagnosticManagerTest, GetStringPreserveOrder) {
mgr.AddDiagnostic(std::make_unique<TextDiag>("baz", lldb::eSeverityInfo));
mgr.AddDiagnostic(std::make_unique<TextDiag>("bar", lldb::eSeverityWarning));
mgr.AddDiagnostic(std::make_unique<TextDiag>("foo", lldb::eSeverityError));
- EXPECT_EQ("baz\nwarning: bar\nerror: foo\n", mgr.GetString());
+ EXPECT_EQ("baz\nwarning: bar\nerror: foo\n", toString(mgr));
}
TEST(DiagnosticManagerTest, AppendMessageNoDiag) {
@@ -139,7 +146,7 @@ TEST(DiagnosticManagerTest, AppendMessageAttachToLastDiag) {
// This should append to 'bar' and not to 'foo'.
mgr.AppendMessageToDiagnostic("message text");
- EXPECT_EQ("error: foo\nerror: bar\nmessage text\n", mgr.GetString());
+ EXPECT_EQ("error: foo\nerror: bar\nmessage text\n", toString(mgr));
}
TEST(DiagnosticManagerTest, AppendMessageSubsequentDiags) {
@@ -150,7 +157,7 @@ TEST(DiagnosticManagerTest, AppendMessageSubsequentDiags) {
// Pushing another diag after the message should work fine.
mgr.AddDiagnostic(std::make_unique<TextDiag>("foo", lldb::eSeverityError));
- EXPECT_EQ("error: bar\nmessage text\nerror: foo\n", mgr.GetString());
+ EXPECT_EQ("error: bar\nmessage text\nerror: foo\n", toString(mgr));
}
TEST(DiagnosticManagerTest, PutString) {
@@ -159,7 +166,7 @@ TEST(DiagnosticManagerTest, PutString) {
mgr.PutString(lldb::eSeverityError, "foo");
EXPECT_EQ(1U, mgr.Diagnostics().size());
EXPECT_EQ(eDiagnosticOriginLLDB, mgr.Diagnostics().front()->getKind());
- EXPECT_EQ("error: foo\n", mgr.GetString());
+ EXPECT_EQ("error: foo\n", toString(mgr));
}
TEST(DiagnosticManagerTest, PutStringMultiple) {
@@ -169,7 +176,7 @@ TEST(DiagnosticManagerTest, PutStringMultiple) {
mgr.PutString(lldb::eSeverityError, "foo");
mgr.PutString(lldb::eSeverityError, "bar");
EXPECT_EQ(2U, mgr.Diagnostics().size());
- EXPECT_EQ("error: foo\nerror: bar\n", mgr.GetString());
+ EXPECT_EQ("error: foo\nerror: bar\n", toString(mgr));
}
TEST(DiagnosticManagerTest, PutStringSeverities) {
@@ -180,7 +187,7 @@ TEST(DiagnosticManagerTest, PutStringSeverities) {
mgr.PutString(lldb::eSeverityError, "foo");
mgr.PutString(lldb::eSeverityWarning, "bar");
EXPECT_EQ(2U, mgr.Diagnostics().size());
- EXPECT_EQ("error: foo\nwarning: bar\n", mgr.GetString());
+ EXPECT_EQ("error: foo\nwarning: bar\n", toString(mgr));
}
TEST(DiagnosticManagerTest, FixedExpression) {
diff --git a/lldb/unittests/Interpreter/CMakeLists.txt b/lldb/unittests/Interpreter/CMakeLists.txt
index 54cea995084d3d..14a7d6c5610388 100644
--- a/lldb/unittests/Interpreter/CMakeLists.txt
+++ b/lldb/unittests/Interpreter/CMakeLists.txt
@@ -1,5 +1,6 @@
add_lldb_unittest(InterpreterTests
TestCommandPaths.cpp
+ TestCommandObjectExpression.cpp
TestCompletion.cpp
TestOptionArgParser.cpp
TestOptions.cpp
diff --git a/lldb/unittests/Interpreter/TestCommandObjectExpression.cpp b/lldb/unittests/Interpreter/TestCommandObjectExpression.cpp
new file mode 100644
index 00000000000000..60c4d658cdc753
--- /dev/null
+++ b/lldb/unittests/Interpreter/TestCommandObjectExpression.cpp
@@ -0,0 +1,29 @@
+#include "lldb/Expression/DiagnosticManager.h"
+#include "gtest/gtest.h"
+
+namespace lldb_private {
+std::string RenderDiagnosticDetails(std::optional<uint16_t> offset_in_command,
+ llvm::ArrayRef<DiagnosticDetail> details);
+}
+
+using namespace lldb_private;
+using namespace lldb;
+using llvm::StringRef;
+namespace {
+class ErrorDisplayTest : public ::testing::Test {};
+} // namespace
+
+static std::string Render(std::vector<DiagnosticDetail> details) {
+ return RenderDiagnosticDetails(0, details);
+}
+
+TEST_F(ErrorDisplayTest, RenderStatus) {
+ DiagnosticDetail::SourceLocation inline_loc;
+ inline_loc.in_user_input = true;
+ {
+ std::string result =
+ Render({DiagnosticDetail{inline_loc, eSeverityError, "foo", ""}});
+ ASSERT_TRUE(StringRef(result).contains("error:"));
+ ASSERT_TRUE(StringRef(result).contains("foo"));
+ }
+}
More information about the lldb-commits
mailing list