https://github.com/adrian-prantl updated https://github.com/llvm/llvm-project/pull/106442
>From d6d55a4d6f806f260e875dc29fdf7481d44fa1b1 Mon Sep 17 00:00:00 2001 From: Adrian Prantl <apra...@apple.com> Date: Wed, 4 Sep 2024 12:50:37 -0700 Subject: [PATCH 1/2] [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 | 5 +- lldb/source/Utility/Status.cpp | 244 ++++++++++++------ 3 files changed, 242 insertions(+), 90 deletions(-) diff --git a/lldb/include/lldb/Utility/Status.h b/lldb/include/lldb/Utility/Status.h index 795c830b965173..517724aa94fae4 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..79e1db9b54e590 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp @@ -973,6 +973,9 @@ PythonException::PythonException(const char *caller) { PyErr_Clear(); } Py_XDECREF(repr); + Py_INCREF(m_exception); + if (m_traceback) + Py_INCREF(m_traceback); } else { PyErr_Clear(); } @@ -993,8 +996,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); } diff --git a/lldb/source/Utility/Status.cpp b/lldb/source/Utility/Status.cpp index 4af3af5fba0185..733f7cfeef6765 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,74 @@ 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 GenericCategory : public std::error_category { + const char *name() const override { return "LLDBGenericCategory"; } + std::string message(int __ev) const override { return "generic LLDB error"; }; +}; +GenericCategory &generic_category() { + static GenericCategory 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 eErrorTypePOSIX: + return llvm::errorCodeToError( + std::error_code(err, std::generic_category())); + case eErrorTypeWin32: + return llvm::make_error<Win32Error>( + std::error_code(err, std::system_category())); + default: + return llvm::createStringError(std::move(msg), + std::error_code(err, generic_category())); + } +} Status::Status(ValueType err, ErrorType type, std::string msg) - : m_code(err), m_type(type), m_string(std::move(msg)) {} + : m_error(ErrorFromEnums(err, type, msg)) {} -// This logic is confusing because c++ calls the traditional (posix) errno codes +// This logic is confusing because C++ calls the traditional (posix) errno codes // "generic errors", while we use the term "generic" to mean completely // arbitrary (text-based) errors. Status::Status(std::error_code EC) - : m_code(EC.value()), - m_type(EC.category() == std::generic_category() ? eErrorTypePOSIX - : eErrorTypeGeneric), - m_string(EC.message()) {} + : m_error(!EC ? llvm::Error::success() : llvm::errorCodeToError(EC)) {} Status::Status(std::string err_str) - : m_code(LLDB_GENERIC_ERROR), m_type(eErrorTypeGeneric), - m_string(std::move(err_str)) {} - -Status::Status(llvm::Error error) { - if (!error) { - Clear(); - return; - } + : m_error( + llvm::createStringError(llvm::inconvertibleErrorCode(), err_str)) {} - // if the error happens to be a errno error, preserve the error code - error = llvm::handleErrors( - std::move(error), [&](std::unique_ptr<llvm::ECError> e) -> llvm::Error { - std::error_code ec = e->convertToErrorCode(); - if (ec.category() == std::generic_category()) { - m_code = ec.value(); - m_type = ErrorType::eErrorTypePOSIX; - return llvm::Error::success(); - } - return llvm::Error(std::move(e)); - }); - - // Otherwise, just preserve the message - if (error) { - m_code = LLDB_GENERIC_ERROR; - m_type = eErrorTypeGeneric; - m_string = llvm::toString(std::move(error)); - } +const Status &Status::operator=(Status &&other) { + Clear(); + m_error = std::move(other.m_error); + return *this; } Status Status::FromErrorStringWithFormat(const char *format, ...) { @@ -94,25 +122,36 @@ 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()) +/// 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) { + std::vector<std::unique_ptr<llvm::ErrorInfoBase>> info; + llvm::visitErrors(error, [&](const llvm::ErrorInfoBase &error) { + if (error.isA<CloneableError>()) + info.push_back(static_cast<const CloneableError *>(&error)->Clone()); + else + info.push_back(std::make_unique<llvm::StringError>( + error.message(), error.convertToErrorCode(), true)); + }); + if (info.size() == 0) return llvm::Error::success(); - if (m_type == ErrorType::eErrorTypePOSIX) - return llvm::errorCodeToError( - std::error_code(m_code, std::generic_category())); - return llvm::createStringError(AsCString()); + llvm::Error e(std::move(info.front())); + for (auto it = std::next(info.begin()); it != info.end(); ++it) + e = llvm::joinErrors(std::move(e), llvm::Error(std::move(*it))); + return e; } -Status::~Status() = default; +Status Status::FromError(llvm::Error error) { return Status(std::move(error)); } + +llvm::Error Status::ToError() const { return CloneError(m_error); } -const Status &Status::operator=(Status &&other) { - m_code = other.m_code; - m_type = other.m_type; - m_string = std::move(other.m_string); - return *this; -} +Status::~Status() { llvm::consumeError(std::move(m_error)); } #ifdef _WIN32 static std::string RetrieveWin32ErrorString(uint32_t error_code) { @@ -140,6 +179,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 +213,8 @@ 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); - default: - break; - } - } if (m_string.empty()) { if (default_error_str) m_string.assign(default_error_str); @@ -181,29 +226,68 @@ 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(); + llvm::consumeError(std::move(m_error)); } -// 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(); + if (ec.category() == std::generic_category() || + ec.category() == generic_category() || + ec.category() == expression_category()) + result = ec.value(); + else + result = 0xff; + }); + 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() == 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, >From 542229ecd139e47e535a83f5e91ca63570087961 Mon Sep 17 00:00:00 2001 From: Adrian Prantl <apra...@apple.com> Date: Wed, 28 Aug 2024 10:04:33 -0700 Subject: [PATCH 2/2] [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 | 88 ++++++++++++++----- lldb/include/lldb/Utility/Status.h | 5 +- lldb/source/Breakpoint/BreakpointLocation.cpp | 10 +-- lldb/source/Expression/DiagnosticManager.cpp | 83 +++++++++++++---- 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 | 5 ++ .../TestModulesCompileError.py | 2 +- .../Expression/DiagnosticManagerTest.cpp | 12 +-- 15 files changed, 227 insertions(+), 111 deletions(-) diff --git a/lldb/include/lldb/Expression/DiagnosticManager.h b/lldb/include/lldb/Expression/DiagnosticManager.h index d49b7c99b114fb..bcf108a4d9d398 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,13 +162,21 @@ 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 // warning: warning text\n // remark text\n" std::string GetString(char separator = '\n'); - + void Dump(Log *log); const std::string &GetFixedExpression() { return m_fixed_expression; } diff --git a/lldb/include/lldb/Utility/Status.h b/lldb/include/lldb/Utility/Status.h index 517724aa94fae4..1255af0ab3fb16 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..1fff224fd912e9 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,51 @@ 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 +120,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 90f26de939a478..16c708626dbdcd 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)); @@ -1505,13 +1538,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..f0609596ded731 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 f1c378b968d2ba..775a9eb2ab2988 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 733f7cfeef6765..83b2587c0b0da5 100644 --- a/lldb/source/Utility/Status.cpp +++ b/lldb/source/Utility/Status.cpp @@ -214,6 +214,11 @@ const char *Status::AsCString(const char *default_error_str) const { return nullptr; m_string = llvm::toStringWithoutConsuming(m_error); + // FIXME: Workaround for ErrorList[ExpressionError, ...]. + 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) 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(); _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits