https://github.com/adrian-prantl updated https://github.com/llvm/llvm-project/pull/106442
>From ab88865f07bcb711ed911ca16e8310f2e13220f6 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] 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 | 28 ++--- lldb/source/Breakpoint/BreakpointLocation.cpp | 11 +- lldb/source/Expression/DiagnosticManager.cpp | 103 +++++++++++++++--- lldb/source/Expression/ExpressionParser.cpp | 5 +- lldb/source/Expression/UserExpression.cpp | 49 +++++---- lldb/source/Expression/UtilityFunction.cpp | 18 +-- .../ExpressionParser/Clang/ClangDiagnostic.h | 5 +- .../Clang/ClangExpressionParser.cpp | 69 ++++++++---- .../Clang/ClangUserExpression.h | 2 + .../Plugins/Platform/POSIX/PlatformPOSIX.cpp | 12 +- .../Platform/Windows/PlatformWindows.cpp | 12 +- lldb/source/Target/Target.cpp | 3 +- lldb/source/Utility/Status.cpp | 64 ++++------- .../TestModulesCompileError.py | 2 +- .../Expression/DiagnosticManagerTest.cpp | 22 +++- 16 files changed, 318 insertions(+), 175 deletions(-) diff --git a/lldb/include/lldb/Expression/DiagnosticManager.h b/lldb/include/lldb/Expression/DiagnosticManager.h index d49b7c99b114fb..62c6bcefe54110 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,53 @@ 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 ExpressionError + : public llvm::ErrorInfo<ExpressionError, ExpressionErrorBase> { + std::string m_message; + std::vector<DiagnosticDetail> m_details; + +public: + static char ID; + using llvm::ErrorInfo<ExpressionError, ExpressionErrorBase>::ErrorInfo; + ExpressionError(lldb::ExpressionResults result, std::string msg, + std::vector<DiagnosticDetail> details = {}); + std::string message() const override; + llvm::ArrayRef<DiagnosticDetail> GetDetails() const { return m_details; } + std::error_code convertToErrorCode() const override; + void log(llvm::raw_ostream &OS) const override; + std::unique_ptr<CloneableError> Clone() const override; +}; + enum DiagnosticOrigin { eDiagnosticOriginUnknown = 0, eDiagnosticOriginLLDB, @@ -49,37 +99,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; } + const DiagnosticDetail &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; + DiagnosticDetail m_detail; }; typedef std::vector<std::unique_ptr<Diagnostic>> DiagnosticList; @@ -102,10 +143,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 +168,10 @@ class DiagnosticManager { m_diagnostics.back()->AppendMessage(str); } + /// Returns an \ref ExpressionError with \c arg as error code. + llvm::Error GetAsError(lldb::ExpressionResults result, + llvm::Twine message = {}) 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 084ce4afb8cefd..3910c26d115a09 100644 --- a/lldb/include/lldb/Utility/Status.h +++ b/lldb/include/lldb/Utility/Status.h @@ -38,6 +38,7 @@ class CloneableError using llvm::ErrorInfo<CloneableError, llvm::ErrorInfoBase>::ErrorInfo; CloneableError() : ErrorInfo() {} virtual std::unique_ptr<CloneableError> Clone() const = 0; + virtual lldb::ErrorType GetErrorType() const = 0; static char ID; }; @@ -48,6 +49,7 @@ class CloneableECError using llvm::ErrorInfo<CloneableECError, CloneableError>::ErrorInfo; std::error_code convertToErrorCode() const override { return EC; } void log(llvm::raw_ostream &OS) const override { OS << EC.message(); } + lldb::ErrorType GetErrorType() const override; static char ID; protected: @@ -63,6 +65,7 @@ class MachKernelError MachKernelError(std::error_code ec) : ErrorInfo(ec) {} std::string message() const override; std::unique_ptr<CloneableError> Clone() const override; + lldb::ErrorType GetErrorType() const override; static char ID; }; @@ -72,21 +75,18 @@ class Win32Error : public llvm::ErrorInfo<Win32Error, CloneableECError> { Win32Error(std::error_code ec, const llvm::Twine &msg = {}) : ErrorInfo(ec) {} std::string message() const override; std::unique_ptr<CloneableError> Clone() const override; + lldb::ErrorType GetErrorType() const override; static char ID; }; -class ExpressionError - : public llvm::ErrorInfo<ExpressionError, CloneableECError> { +class ExpressionErrorBase + : public llvm::ErrorInfo<ExpressionErrorBase, 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; } + using llvm::ErrorInfo<ExpressionErrorBase, CloneableECError>::ErrorInfo; + ExpressionErrorBase(std::error_code ec, std::string msg = {}) + : ErrorInfo(ec) {} + lldb::ErrorType GetErrorType() const override; static char ID; - -protected: - std::string m_string; }; /// \class Status Status.h "lldb/Utility/Status.h" An error handling class. @@ -160,9 +160,6 @@ class Status { return Status(llvm::formatv(format, std::forward<Args>(args)...)); } - static Status FromExpressionError(lldb::ExpressionResults result, - std::string msg); - /// Set the current error to errno. /// /// Update the error value to be \c errno and update the type to be \c @@ -175,8 +172,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..35058a713aef86 100644 --- a/lldb/source/Breakpoint/BreakpointLocation.cpp +++ b/lldb/source/Breakpoint/BreakpointLocation.cpp @@ -264,9 +264,10 @@ 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(lldb::eExpressionParseError, + "Couldn't parse conditional expression:")); + m_user_expression_sp.reset(); return true; } @@ -324,8 +325,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( + lldb::eExpressionParseError, "Couldn't execute expression:")); } return ret; diff --git a/lldb/source/Expression/DiagnosticManager.cpp b/lldb/source/Expression/DiagnosticManager.cpp index a8330138f3d53b..3731a200c5e2f7 100644 --- a/lldb/source/Expression/DiagnosticManager.cpp +++ b/lldb/source/Expression/DiagnosticManager.cpp @@ -14,23 +14,29 @@ #include "lldb/Utility/StreamString.h" using namespace lldb_private; +char ExpressionError::ID; -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(); +/// A std::error_code category for eErrorTypeExpression. +class ExpressionCategory : public std::error_category { + const char *name() const noexcept override { + return "LLDBExpressionCategory"; } - - log->PutCString(str.c_str()); + 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; } +ExpressionError::ExpressionError(lldb::ExpressionResults result, + std::string msg, + std::vector<DiagnosticDetail> details) + : ErrorInfo(std::error_code(result, expression_category())), m_message(msg), + m_details(details) {} + static const char *StringForSeverity(lldb::Severity severity) { switch (severity) { // this should be exhaustive @@ -44,9 +50,33 @@ static const char *StringForSeverity(lldb::Severity severity) { llvm_unreachable("switch needs another case for lldb::Severity enum"); } +std::string ExpressionError::message() const { + std::string str; + { + llvm::raw_string_ostream os(str); + if (!m_message.empty()) + os << m_message << '\n'; + for (const auto &detail : m_details) + os << StringForSeverity(detail.severity) << detail.rendered << '\n'; + } + return str; +} + +std::error_code ExpressionError::convertToErrorCode() const { + return llvm::inconvertibleErrorCode(); +} + +void ExpressionError::log(llvm::raw_ostream &OS) const { OS << message(); } + +std::unique_ptr<CloneableError> ExpressionError::Clone() const { + return std::make_unique<ExpressionError>( + (lldb::ExpressionResults)convertToErrorCode().value(), m_message, + m_details); +} + 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 +91,39 @@ std::string DiagnosticManager::GetString(char separator) { stream << message.drop_front(severity_pos + severity.size()); stream << separator; } + return str; +} + +void DiagnosticManager::Dump(Log *log) { + if (!log) + return; - return ret; + std::string str = GetString(); + + // We want to remove the last '\n' because log->PutCString will add + // one for us. + + if (!str.empty() && str.back() == '\n') + str.pop_back(); + + log->PutString(str); +} + +llvm::Error DiagnosticManager::GetAsError(lldb::ExpressionResults result, + llvm::Twine message) const { + std::vector<DiagnosticDetail> details; + for (const auto &diag : m_diagnostics) + details.push_back(diag->GetDetail()); + return llvm::make_error<ExpressionError>(result, message.str(), 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, + DiagnosticDetail{{}, severity, message.str(), message.str()})); } size_t DiagnosticManager::Printf(lldb::Severity severity, const char *format, @@ -85,3 +146,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..1ba5e10d65d058 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( + lldb::eExpressionSetupError, "couldn't run static initializer:")); return err; } } diff --git a/lldb/source/Expression/UserExpression.cpp b/lldb/source/Expression/UserExpression.cpp index 872f6304f91baa..b3c81af24893d0 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().empty()) + error = Status::FromError(llvm::make_error<ExpressionError>( + execution_results, + "expression failed to parse (no further compiler diagnostics)")); + else + error = + Status::FromError(diagnostic_manager.GetAsError(execution_results)); } } @@ -351,18 +353,18 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx, LLDB_LOG(log, "== [UserExpression::Evaluate] Expression may not run, but " "is not constant =="); - if (!diagnostic_manager.Diagnostics().size()) - error = Status::FromExpressionError( + if (diagnostic_manager.Diagnostics().empty()) + error = Status::FromError(llvm::make_error<ExpressionError>( lldb::eExpressionSetupError, - "expression needed to run but couldn't"); + "expression needed to run but couldn't")); } else if (execution_policy == eExecutionPolicyTopLevel) { error = Status(UserExpression::kNoResult, lldb::eErrorTypeGeneric); return lldb::eExpressionCompleted; } else { if (options.InvokeCancelCallback(lldb::eExpressionEvaluationExecution)) { - error = Status::FromExpressionError( + error = Status::FromError(llvm::make_error<ExpressionError>( lldb::eExpressionInterrupted, - "expression interrupted by callback before execution"); + "expression interrupted by callback before execution")); result_valobj_sp = ValueObjectConstResult::Create( exe_ctx.GetBestExecutionContextScope(), std::move(error)); return lldb::eExpressionInterrupted; @@ -380,12 +382,13 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx, LLDB_LOG(log, "== [UserExpression::Evaluate] Execution completed " "abnormally =="); - if (!diagnostic_manager.Diagnostics().size()) - error = Status::FromExpressionError( - execution_results, "expression failed to execute, unknown error"); + if (diagnostic_manager.Diagnostics().empty()) + error = Status::FromError(llvm::make_error<ExpressionError>( + 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(); @@ -407,9 +410,9 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx, } if (options.InvokeCancelCallback(lldb::eExpressionEvaluationComplete)) { - error = Status::FromExpressionError( + error = Status::FromError(llvm::make_error<ExpressionError>( lldb::eExpressionInterrupted, - "expression interrupted by callback after complete"); + "expression interrupted by callback after complete")); return lldb::eExpressionInterrupted; } diff --git a/lldb/source/Expression/UtilityFunction.cpp b/lldb/source/Expression/UtilityFunction.cpp index 55ebfb8ef342e8..97c226ae1c5f95 100644 --- a/lldb/source/Expression/UtilityFunction.cpp +++ b/lldb/source/Expression/UtilityFunction.cpp @@ -83,19 +83,19 @@ 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( + lldb::eExpressionParseError, + "Error compiling " + m_function_name + " caller function:")); + m_caller_up.reset(); return nullptr; } @@ -104,9 +104,9 @@ 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( + lldb::eExpressionSetupError, + "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 4eeac372a2e657..687962c700a30b 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" @@ -161,7 +162,8 @@ static void AddAllFixIts(ClangDiagnostic *diag, const clang::Diagnostic &Info) { class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer { public: - ClangDiagnosticManagerAdapter(DiagnosticOptions &opts) { + ClangDiagnosticManagerAdapter(DiagnosticOptions &opts, StringRef filename) + : m_filename(filename) { DiagnosticOptions *options = new DiagnosticOptions(opts); options->ShowPresumedLoc = true; options->ShowLevel = false; @@ -211,20 +213,20 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer { m_output.clear(); m_passthrough->HandleDiagnostic(DiagLevel, Info); - 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); @@ -253,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(); + loc.in_user_input = filename == m_filename; + + // 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)); @@ -280,6 +314,7 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer { std::shared_ptr<llvm::raw_string_ostream> m_os; /// Output string filled by m_os. std::string m_output; + StringRef m_filename; }; /// Returns true if the SDK for the specified triple supports @@ -710,8 +745,8 @@ ClangExpressionParser::ClangExpressionParser( // 4. Set language options. SetupLangOpts(*m_compiler, *exe_scope, expr); - if (auto *clang_expr = dyn_cast<ClangUserExpression>(&m_expr); - clang_expr && clang_expr->DidImportCxxModules()) { + auto *clang_expr = dyn_cast<ClangUserExpression>(&m_expr); + if (clang_expr && clang_expr->DidImportCxxModules()) { LLDB_LOG(log, "Adding lang options for importing C++ modules"); SetupImportStdModuleLangOpts(*m_compiler, *target_sp); SetupModuleHeaderPaths(m_compiler.get(), m_include_directories, target_sp); @@ -738,9 +773,9 @@ ClangExpressionParser::ClangExpressionParser( m_compiler->getLangOpts()); // 5. Set up the diagnostic buffer for reporting errors - auto diag_mgr = new ClangDiagnosticManagerAdapter( - m_compiler->getDiagnostics().getDiagnosticOptions()); + m_compiler->getDiagnostics().getDiagnosticOptions(), + clang_expr ? clang_expr->GetFilename() : StringRef()); m_compiler->getDiagnostics().setClient(diag_mgr); // 6. Set up the source management objects inside the compiler @@ -1502,13 +1537,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( + lldb::eExpressionSetupError, "couldn't install checkers:")); process->SetDynamicCheckers(dynamic_checkers); diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h index 09604feea5dec4..7c0c6a0147e2a0 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h @@ -177,6 +177,8 @@ class ClangUserExpression : public LLVMUserExpression { /// Returns true iff this expression is using any imported C++ modules. bool DidImportCxxModules() const { return !m_imported_cpp_modules.empty(); } + llvm::StringRef GetFilename() const { return m_filename; } + private: /// Populate m_in_cplusplus_method and m_in_objectivec_method based on the /// environment. diff --git a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp index 31315e46ca168d..e9830c9f8722b2 100644 --- a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp +++ b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp @@ -863,9 +863,9 @@ 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( + lldb::eExpressionSetupError, + "dlopen error: could not write function arguments:")); return LLDB_INVALID_IMAGE_TOKEN; } @@ -906,9 +906,9 @@ 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( + lldb::eExpressionSetupError, + "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..3936b8367fb83b 100644 --- a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp +++ b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp @@ -341,9 +341,9 @@ 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( + eExpressionSetupError, + "LoadLibrary error: unable to write function parameters:")); return LLDB_INVALID_IMAGE_TOKEN; } @@ -384,9 +384,9 @@ 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( + eExpressionSetupError, + "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 6123e5b9c20902..04395e37f0425d 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -2680,7 +2680,8 @@ 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(lldb::eExpressionSetupError, + "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 7f73962c7fc9aa..cfdca75c0d05ec 100644 --- a/lldb/source/Utility/Status.cpp +++ b/lldb/source/Utility/Status.cpp @@ -43,7 +43,7 @@ char CloneableError::ID; char CloneableECError::ID; char MachKernelError::ID; char Win32Error::ID; -char ExpressionError::ID; +char ExpressionErrorBase::ID; namespace { /// A std::error_code category for eErrorTypeGeneric. @@ -55,21 +55,6 @@ LLDBGenericCategory &lldb_generic_category() { static LLDBGenericCategory g_generic_category; return g_generic_category; } - -/// A std::error_code category for eErrorTypeExpression. -class ExpressionCategory : public std::error_category { - const char *name() const noexcept override { - return "LLDBExpressionCategory"; - } - std::string message(int __ev) const override { - return ExpressionResultAsCString( - static_cast<lldb::ExpressionResults>(__ev)); - }; -}; -ExpressionCategory &expression_category() { - static ExpressionCategory g_expression_category; - return g_expression_category; -} } // namespace Status::Status() : m_error(llvm::Error::success()) {} @@ -132,12 +117,6 @@ Status Status::FromErrorStringWithFormat(const char *format, ...) { return Status(string); } -Status Status::FromExpressionError(lldb::ExpressionResults result, - std::string msg) { - return Status(llvm::make_error<ExpressionError>( - std::error_code(result, expression_category()), msg)); -} - /// 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) { @@ -211,10 +190,6 @@ 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. @@ -257,26 +232,35 @@ Status::ValueType Status::GetError() const { return result; } -// Access the error type. +ErrorType CloneableECError::GetErrorType() const { + if (EC.category() == std::generic_category()) + return eErrorTypePOSIX; + if (EC.category() == lldb_generic_category() || + EC == llvm::inconvertibleErrorCode()) + return eErrorTypeGeneric; + return eErrorTypeInvalid; +} + +lldb::ErrorType MachKernelError::GetErrorType() const { + return lldb::eErrorTypeMachKernel; +} + +lldb::ErrorType Win32Error::GetErrorType() const { + return lldb::eErrorTypeWin32; +} + +lldb::ErrorType ExpressionErrorBase::GetErrorType() const { + return lldb::eErrorTypeExpression; +} + 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; + if (error.isA<CloneableError>()) + result = static_cast<const CloneableError &>(error).GetErrorType(); }); return result; } 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..4608b779f79a96 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(), 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(); @@ -197,3 +197,13 @@ TEST(DiagnosticManagerTest, FixedExpression) { mgr.SetFixedExpression("bar"); EXPECT_EQ("bar", mgr.GetFixedExpression()); } + +TEST(DiagnosticManagerTest, StatusConversion) { + DiagnosticManager mgr; + mgr.AddDiagnostic(std::make_unique<TextDiag>("abc", lldb::eSeverityError)); + mgr.AddDiagnostic(std::make_unique<TextDiag>("def", lldb::eSeverityWarning)); + Status status = + Status::FromError(mgr.GetAsError(lldb::eExpressionParseError)); + EXPECT_EQ(std::string("error: abc\nwarning: def\n"), + std::string(status.AsCString())); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
