https://github.com/adrian-prantl created https://github.com/llvm/llvm-project/pull/106442
…NFC] This patch is the first patch in a series reworking of Pete Lawrence's (@PortalPete) amazing 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 Status::Detail vector to Status that is used when the error type is eErrorTypeExpression. I tried my best to minimize the overhead this adds to Status in all other use-cases. Right now the extra information is not used by the CommandInterpreter, this will be added in a follow-up patch! >From 9f014cf845712914a68419a18ace0aecbc13c30b Mon Sep 17 00:00:00 2001 From: Adrian Prantl <apra...@apple.com> Date: Wed, 28 Aug 2024 10:04:33 -0700 Subject: [PATCH] [lldb] Add Status::Detail to store expression evaluator diagnostics [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 Status::Detail vector to Status that is used when the error type is eErrorTypeExpression. I tried my best to minimize the overhead this adds to Status in all other use-cases. Right now the extra information is not used by the CommandInterpreter, this will be added in a follow-up patch! --- .../lldb/Expression/DiagnosticManager.h | 50 ++++++++----------- lldb/include/lldb/Utility/Status.h | 50 ++++++++++++++++++- lldb/source/Expression/DiagnosticManager.cpp | 27 ++++++++++ lldb/source/Expression/UserExpression.cpp | 26 +++++----- .../ExpressionParser/Clang/ClangDiagnostic.h | 5 +- .../Clang/ClangExpressionParser.cpp | 47 ++++++++++++++--- lldb/source/Utility/Status.cpp | 25 ++++++++-- .../TestModulesCompileError.py | 2 +- .../Expression/DiagnosticManagerTest.cpp | 12 ++--- 9 files changed, 180 insertions(+), 64 deletions(-) diff --git a/lldb/include/lldb/Expression/DiagnosticManager.h b/lldb/include/lldb/Expression/DiagnosticManager.h index d49b7c99b114fb..252492ce8f776a 100644 --- a/lldb/include/lldb/Expression/DiagnosticManager.h +++ b/lldb/include/lldb/Expression/DiagnosticManager.h @@ -12,6 +12,8 @@ #include "lldb/lldb-defines.h" #include "lldb/lldb-types.h" +#include "lldb/Utility/Status.h" + #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringRef.h" @@ -49,37 +51,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, + Status::Detail 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; } + Status::Detail GetDetail() const { return m_detail; } - 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; + Status::Detail m_detail; }; typedef std::vector<std::unique_ptr<Diagnostic>> DiagnosticList; @@ -102,10 +95,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 +120,17 @@ class DiagnosticManager { m_diagnostics.back()->AppendMessage(str); } - // Returns a string containing errors in this format: - // - // "error: error text\n - // warning: warning text\n - // remark text\n" + /// Returns a string containing errors in this format: + /// + /// "error: error text\n + /// warning: warning text\n + /// remark text\n" + LLVM_DEPRECATED("Use GetAsStatus instead", "GetAsStatus()") std::string GetString(char separator = '\n'); + /// Copies the diagnostics into an lldb::Status. + Status GetAsStatus(lldb::ExpressionResults result); + 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 b304291ffae00e..5a9683e09c6e53 100644 --- a/lldb/include/lldb/Utility/Status.h +++ b/lldb/include/lldb/Utility/Status.h @@ -9,6 +9,7 @@ #ifndef LLDB_UTILITY_STATUS_H #define LLDB_UTILITY_STATUS_H +#include "lldb/Utility/FileSpec.h" #include "lldb/lldb-defines.h" #include "lldb/lldb-enumerations.h" #include "llvm/ADT/StringRef.h" @@ -47,7 +48,34 @@ class Status { /// into ValueType. typedef uint32_t ValueType; - Status(); + /// A customizable "detail" for an error. For example, expression + /// evaluation failures often have more than one diagnostic that the + /// UI layer might want to render differently. + /// + /// Running example: + /// (lldb) expr 1+x + /// error: <user expression 0>:1:3: use of undeclared identifier 'foo' + /// 1+foo + /// ^ + struct Detail { + 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; + }; + + Status() = default; /// Initialize the error object with a generic success value. /// @@ -57,7 +85,8 @@ class Status { /// \param[in] type /// The type for \a err. explicit Status(ValueType err, lldb::ErrorType type = lldb::eErrorTypeGeneric, - std::string msg = {}); + std::string msg = {}) + : m_code(err), m_type(type), m_string(std::move(msg)) {} Status(std::error_code EC); @@ -83,6 +112,12 @@ class Status { return Status(result, lldb::eErrorTypeExpression, msg); } + static Status + FromExpressionErrorDetails(lldb::ExpressionResults result, + llvm::SmallVectorImpl<Detail> &&details) { + return Status(result, lldb::eErrorTypeExpression, std::move(details)); + } + /// Set the current error to errno. /// /// Update the error value to be \c errno and update the type to be \c @@ -144,13 +179,24 @@ class Status { /// success (non-erro), \b false otherwise. bool Success() const; + /// Get the list of details for this error. If this is non-empty, + /// clients can use this to render more appealing error messages + /// from the details. If you just want a pre-rendered string, use + /// AsCString() instead. Currently details are only used for + /// eErrorTypeExpression. + llvm::ArrayRef<Detail> GetDetails() const; + protected: + /// Separate so the main constructor doesn't need to deal with the array. + Status(ValueType err, lldb::ErrorType type, + llvm::SmallVectorImpl<Detail> &&details); /// Status code as an integer value. ValueType m_code = 0; /// The type of the above error code. lldb::ErrorType m_type = lldb::eErrorTypeInvalid; /// A string representation of the error code. mutable std::string m_string; + llvm::SmallVector<Detail, 0> m_details; }; } // namespace lldb_private diff --git a/lldb/source/Expression/DiagnosticManager.cpp b/lldb/source/Expression/DiagnosticManager.cpp index a8330138f3d53b..5c20eba1dbd07e 100644 --- a/lldb/source/Expression/DiagnosticManager.cpp +++ b/lldb/source/Expression/DiagnosticManager.cpp @@ -65,6 +65,23 @@ std::string DiagnosticManager::GetString(char separator) { return ret; } +Status DiagnosticManager::GetAsStatus(lldb::ExpressionResults result) { + llvm::SmallVector<Status::Detail, 0> details; + details.reserve(m_diagnostics.size()); + for (const auto &diagnostic : m_diagnostics) + details.push_back(diagnostic->GetDetail()); + return Status::FromExpressionErrorDetails(result, std::move(details)); +} + +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, + Status::Detail{{}, severity, message.str(), message.str()})); +} + size_t DiagnosticManager::Printf(lldb::Severity severity, const char *format, ...) { StreamString ss; @@ -85,3 +102,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/UserExpression.cpp b/lldb/source/Expression/UserExpression.cpp index c2889e4c986bfe..159714489ad111 100644 --- a/lldb/source/Expression/UserExpression.cpp +++ b/lldb/source/Expression/UserExpression.cpp @@ -328,18 +328,19 @@ 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 = diagnostic_manager.GetAsStatus(execution_results); } } @@ -384,8 +385,7 @@ 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 = diagnostic_manager.GetAsStatus(execution_results); } else { if (expr_result) { result_valobj_sp = expr_result->GetValueObject(); diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h index 21abd71cc34eeb..f6e635afbf68ca 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(Status::Detail 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..6b524250bbf4de 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; + Status::Detail 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()) { + Status::Detail::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)); diff --git a/lldb/source/Utility/Status.cpp b/lldb/source/Utility/Status.cpp index 7260b7b3e0a03e..3b552e5540df61 100644 --- a/lldb/source/Utility/Status.cpp +++ b/lldb/source/Utility/Status.cpp @@ -37,10 +37,9 @@ class raw_ostream; using namespace lldb; using namespace lldb_private; -Status::Status() {} - -Status::Status(ValueType err, ErrorType type, std::string msg) - : m_code(err), m_type(type), m_string(std::move(msg)) {} +Status::Status(ValueType err, ErrorType type, + llvm::SmallVectorImpl<Status::Detail> &&details) + : m_code(err), m_type(type), m_details(std::move(details)) {} // This logic is confusing because c++ calls the traditional (posix) errno codes // "generic errors", while we use the term "generic" to mean completely @@ -133,6 +132,19 @@ static std::string RetrieveWin32ErrorString(uint32_t error_code) { } #endif +static const char *StringForSeverity(lldb::Severity severity) { + switch (severity) { + // this should be exhaustive + case lldb::eSeverityError: + return "error: "; + case lldb::eSeverityWarning: + return "warning: "; + case lldb::eSeverityInfo: + return ""; + } + llvm_unreachable("switch needs another case for lldb::Severity enum"); +} + // 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. @@ -159,6 +171,11 @@ const char *Status::AsCString(const char *default_error_str) const { #endif break; + case eErrorTypeExpression: { + llvm::raw_string_ostream stream(m_string); + for (const auto &detail : m_details) + stream << StringForSeverity(detail.severity) << detail.rendered << '\n'; + } break; default: break; } 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..bf554020ad8924 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, + Status::Detail{{}, 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, + Status::Detail{{}, 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, Status::Detail{{}, 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
