https://github.com/adrian-prantl updated https://github.com/llvm/llvm-project/pull/106470
>From 8c682fc70bff5c6dff43c4601c14e5e61d11a08f Mon Sep 17 00:00:00 2001 From: Adrian Prantl <apra...@apple.com> Date: Wed, 28 Aug 2024 10:04:33 -0700 Subject: [PATCH 1/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 | 91 +++++++++++++----- lldb/include/lldb/Utility/Status.h | 5 +- lldb/source/Breakpoint/BreakpointLocation.cpp | 10 +- lldb/source/Expression/DiagnosticManager.cpp | 94 +++++++++++++++---- lldb/source/Expression/ExpressionParser.cpp | 5 +- lldb/source/Expression/UserExpression.cpp | 32 ++++--- lldb/source/Expression/UtilityFunction.cpp | 16 ++-- .../ExpressionParser/Clang/ClangDiagnostic.h | 5 +- .../Clang/ClangExpressionParser.cpp | 69 ++++++++++---- .../Clang/ClangUserExpression.h | 2 + .../Plugins/Platform/POSIX/PlatformPOSIX.cpp | 10 +- .../Platform/Windows/PlatformWindows.cpp | 10 +- lldb/source/Target/Target.cpp | 2 +- lldb/source/Utility/Status.cpp | 8 ++ .../TestModulesCompileError.py | 2 +- .../Expression/DiagnosticManagerTest.cpp | 22 +++-- lldb/unittests/Utility/StatusTest.cpp | 2 +- 17 files changed, 267 insertions(+), 118 deletions(-) diff --git a/lldb/include/lldb/Expression/DiagnosticManager.h b/lldb/include/lldb/Expression/DiagnosticManager.h index d49b7c99b114fb..1c0763eea92f05 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,52 @@ 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, CloneableError> { + DiagnosticDetail m_detail; + +public: + using llvm::ErrorInfo<DetailedExpressionError, CloneableError>::ErrorInfo; + DetailedExpressionError(DiagnosticDetail detail) : m_detail(detail) {} + std::string message() const override; + DiagnosticDetail GetDetail() const { return m_detail; } + std::error_code convertToErrorCode() const override; + void log(llvm::raw_ostream &OS) const override; + std::unique_ptr<CloneableError> Clone() const override; + + static char ID; +}; + enum DiagnosticOrigin { eDiagnosticOriginUnknown = 0, eDiagnosticOriginLLDB, @@ -49,37 +98,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 +142,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 +167,14 @@ class DiagnosticManager { m_diagnostics.back()->AppendMessage(str); } + /// Copies the diagnostics into an llvm::Error{List}. + /// The first diagnostic wraps \c result. + llvm::Error GetAsError(lldb::ExpressionResults result) const; + + /// Copies the diagnostics into an llvm::Error, the first diagnostic + /// being an llvm::StringError. + llvm::Error GetAsError(llvm::Twine msg) const; + // Returns a string containing errors in this format: // // "error: error text\n diff --git a/lldb/include/lldb/Utility/Status.h b/lldb/include/lldb/Utility/Status.h index 084ce4afb8cefd..cac0fad4c3b309 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..99533aefb685b1 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,28 @@ 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::error_code DetailedExpressionError::convertToErrorCode() const { + return llvm::inconvertibleErrorCode(); +} + +void DetailedExpressionError::log(llvm::raw_ostream &OS) const { + OS << message(); +} + +std::unique_ptr<CloneableError> DetailedExpressionError::Clone() const { + return std::make_unique<DetailedExpressionError>(m_detail); +} + std::string DiagnosticManager::GetString(char separator) { - std::string 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 +65,50 @@ std::string DiagnosticManager::GetString(char separator) { stream << message.drop_front(severity_pos + severity.size()); stream << separator; } + return str; +} - return ret; +void DiagnosticManager::Dump(Log *log) { + if (!log) + return; + + std::string str = GetString(); + + // We want to remove the last '\n' because log->PutCString will add + // one for us. + + if (str.size() && str.back() == '\n') + str.pop_back(); + + log->PutString(str); +} + +llvm::Error Diagnostic::GetAsError() const { + return llvm::make_error<DetailedExpressionError>(m_detail); +} + +llvm::Error +DiagnosticManager::GetAsError(lldb::ExpressionResults result) const { + llvm::Error diags = Status::FromExpressionError(result, "").takeError(); + for (const auto &diagnostic : m_diagnostics) + diags = llvm::joinErrors(std::move(diags), diagnostic->GetAsError()); + return diags; +} + +llvm::Error DiagnosticManager::GetAsError(llvm::Twine msg) const { + llvm::Error diags = llvm::createStringError(msg); + for (const auto &diagnostic : m_diagnostics) + diags = llvm::joinErrors(std::move(diags), diagnostic->GetAsError()); + return diags; +} + +void DiagnosticManager::AddDiagnostic(llvm::StringRef message, + lldb::Severity severity, + DiagnosticOrigin origin, + uint32_t compiler_id) { + m_diagnostics.emplace_back(std::make_unique<Diagnostic>( + origin, compiler_id, + DiagnosticDetail{{}, severity, message.str(), message.str()})); } size_t DiagnosticManager::Printf(lldb::Severity severity, const char *format, @@ -85,3 +131,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..55ba857db88db6 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::FromExpressionError( + execution_results, + "expression failed to parse (no further compiler diagnostics)"); + else + error = + Status::FromError(diagnostic_manager.GetAsError(execution_results)); } } @@ -351,7 +353,7 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx, LLDB_LOG(log, "== [UserExpression::Evaluate] Expression may not run, but " "is not constant =="); - if (!diagnostic_manager.Diagnostics().size()) + if (diagnostic_manager.Diagnostics().empty()) error = Status::FromExpressionError( lldb::eExpressionSetupError, "expression needed to run but couldn't"); @@ -380,12 +382,12 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx, LLDB_LOG(log, "== [UserExpression::Evaluate] Execution completed " "abnormally =="); - if (!diagnostic_manager.Diagnostics().size()) + if (diagnostic_manager.Diagnostics().empty()) error = Status::FromExpressionError( execution_results, "expression failed to execute, unknown error"); else - error = Status::FromExpressionError(execution_results, - diagnostic_manager.GetString()); + error = Status::FromError( + diagnostic_manager.GetAsError(execution_results)); } else { if (expr_result) { result_valobj_sp = expr_result->GetValueObject(); diff --git a/lldb/source/Expression/UtilityFunction.cpp b/lldb/source/Expression/UtilityFunction.cpp index 55ebfb8ef342e8..e35c0774815298 100644 --- a/lldb/source/Expression/UtilityFunction.cpp +++ b/lldb/source/Expression/UtilityFunction.cpp @@ -83,19 +83,18 @@ FunctionCaller *UtilityFunction::MakeFunctionCaller( m_caller_up.reset(process_sp->GetTarget().GetFunctionCallerForLanguage( Language().AsLanguageType(), return_type, impl_code_address, arg_value_list, name.c_str(), error)); - if (error.Fail()) { - + if (error.Fail()) return nullptr; - } + if (m_caller_up) { DiagnosticManager diagnostics; unsigned num_errors = m_caller_up->CompileFunction(thread_to_use_sp, diagnostics); if (num_errors) { - error = Status::FromErrorStringWithFormat( - "Error compiling %s caller function: \"%s\".", - m_function_name.c_str(), diagnostics.GetString().c_str()); + error = Status::FromError(diagnostics.GetAsError( + "Error compiling " + m_function_name + " caller function:")); + m_caller_up.reset(); return nullptr; } @@ -104,9 +103,8 @@ FunctionCaller *UtilityFunction::MakeFunctionCaller( ExecutionContext exe_ctx(process_sp); if (!m_caller_up->WriteFunctionWrapper(exe_ctx, diagnostics)) { - error = Status::FromErrorStringWithFormat( - "Error inserting caller function for %s: \"%s\".", - m_function_name.c_str(), diagnostics.GetString().c_str()); + error = Status::FromError(diagnostics.GetAsError( + "Error inserting " + m_function_name + " caller function:")); m_caller_up.reset(); return nullptr; } diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h index 21abd71cc34eeb..c473df808ee8d0 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h @@ -29,9 +29,8 @@ class ClangDiagnostic : public Diagnostic { return diag->getKind() == eDiagnosticOriginClang; } - ClangDiagnostic(llvm::StringRef message, lldb::Severity severity, - uint32_t compiler_id) - : Diagnostic(message, severity, eDiagnosticOriginClang, compiler_id) {} + ClangDiagnostic(DiagnosticDetail detail, uint32_t compiler_id) + : Diagnostic(eDiagnosticOriginClang, compiler_id, detail) {} ~ClangDiagnostic() override = default; diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp index 2fe3c0460aa7f8..92bd554767dae2 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; @@ -212,20 +214,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 +256,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)); @@ -281,6 +315,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 @@ -711,8 +746,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); @@ -739,9 +774,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 @@ -1503,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/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..5d873bf73da260 100644 --- a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp +++ b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp @@ -863,9 +863,8 @@ uint32_t PlatformPOSIX::DoLoadImage(lldb_private::Process *process, func_args_addr, arguments, diagnostics)) { - error = Status::FromErrorStringWithFormat( - "dlopen error: could not write function arguments: %s", - diagnostics.GetString().c_str()); + error = Status::FromError(diagnostics.GetAsError( + "dlopen error: could not write function arguments:")); return LLDB_INVALID_IMAGE_TOKEN; } @@ -906,9 +905,8 @@ uint32_t PlatformPOSIX::DoLoadImage(lldb_private::Process *process, ExpressionResults results = do_dlopen_function->ExecuteFunction( exe_ctx, &func_args_addr, options, diagnostics, return_value); if (results != eExpressionCompleted) { - error = Status::FromErrorStringWithFormat( - "dlopen error: failed executing dlopen wrapper function: %s", - diagnostics.GetString().c_str()); + error = Status::FromError(diagnostics.GetAsError( + "dlopen error: failed executing dlopen wrapper function:")); return LLDB_INVALID_IMAGE_TOKEN; } diff --git a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp index 7352d6f33f2174..fa39d93a91297c 100644 --- a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp +++ b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp @@ -341,9 +341,8 @@ uint32_t PlatformWindows::DoLoadImage(Process *process, diagnostics.Clear(); if (!invocation->WriteFunctionArguments(context, injected_parameters, parameters, diagnostics)) { - error = Status::FromErrorStringWithFormat( - "LoadLibrary error: unable to write function parameters: %s", - diagnostics.GetString().c_str()); + error = Status::FromError(diagnostics.GetAsError( + "LoadLibrary error: unable to write function parameters:")); return LLDB_INVALID_IMAGE_TOKEN; } @@ -384,9 +383,8 @@ uint32_t PlatformWindows::DoLoadImage(Process *process, invocation->ExecuteFunction(context, &injected_parameters, options, diagnostics, value); if (result != eExpressionCompleted) { - error = Status::FromErrorStringWithFormat( - "LoadLibrary error: failed to execute LoadLibrary helper: %s", - diagnostics.GetString().c_str()); + error = Status::FromError(diagnostics.GetAsError( + "LoadLibrary error: failed to execute LoadLibrary helper:")); return LLDB_INVALID_IMAGE_TOKEN; } diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index f1659aae0800db..57061ecd5bd445 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -2605,7 +2605,7 @@ Target::CreateUtilityFunction(std::string expression, std::string name, DiagnosticManager diagnostics; if (!utility_fn->Install(diagnostics, exe_ctx)) - return llvm::createStringError(diagnostics.GetString()); + return diagnostics.GetAsError("Could not install utility function:"); return std::move(utility_fn); } diff --git a/lldb/source/Utility/Status.cpp b/lldb/source/Utility/Status.cpp index ba08353c7b24d2..3099e54af483ff 100644 --- a/lldb/source/Utility/Status.cpp +++ b/lldb/source/Utility/Status.cpp @@ -224,6 +224,14 @@ const char *Status::AsCString(const char *default_error_str) const { if (!m_string.empty() && m_string[m_string.size() - 1] == '\n') m_string.pop_back(); + // FIXME: Workaround for ErrorList[ExpressionError, ...]. + if (m_error.isA<llvm::ErrorList>()) { + while (!m_string.empty() && m_string[0] == '\n') + m_string = std::string(m_string.data() + 1, m_string.size() - 1); + if (!m_string.empty() && m_string[m_string.size() - 1] != '\n') + m_string += '\n'; + } + if (m_string.empty()) { if (default_error_str) m_string.assign(default_error_str); diff --git a/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py b/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py index 620b6e44fc852a..36e302be2525b5 100644 --- a/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py +++ b/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py @@ -21,7 +21,7 @@ def test(self): "expr @import LLDBTestModule", error=True, substrs=[ - "module.h:4:1: use of undeclared identifier 'syntax_error_for_lldb_to_find'", + "module.h:4:1: error: use of undeclared identifier 'syntax_error_for_lldb_to_find'", "syntax_error_for_lldb_to_find // comment that tests source printing", "could not build module 'LLDBTestModule'", ], diff --git a/lldb/unittests/Expression/DiagnosticManagerTest.cpp b/lldb/unittests/Expression/DiagnosticManagerTest.cpp index 05fe7c164d6818..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())); +} diff --git a/lldb/unittests/Utility/StatusTest.cpp b/lldb/unittests/Utility/StatusTest.cpp index e37c94ac17f2d0..48110b160be93c 100644 --- a/lldb/unittests/Utility/StatusTest.cpp +++ b/lldb/unittests/Utility/StatusTest.cpp @@ -55,7 +55,7 @@ TEST(StatusTest, ErrorCodeConstructor) { llvm::Error list = llvm::joinErrors(llvm::createStringError("foo"), llvm::createStringError("bar")); Status foobar = Status::FromError(std::move(list)); - EXPECT_EQ(std::string("foo\nbar"), std::string(foobar.AsCString())); + EXPECT_EQ(std::string("foo\nbar\n"), std::string(foobar.AsCString())); } TEST(StatusTest, ErrorConversion) { >From f84f26610f1bed9ed6647f09fa6f27cde3126653 Mon Sep 17 00:00:00 2001 From: Adrian Prantl <apra...@apple.com> Date: Wed, 28 Aug 2024 16:19:21 -0700 Subject: [PATCH 2/2] [lldb] Inline expression evaluator error visualization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch is a reworking of Pete Lawrence's (@PortalPete) proposal for better expression evaluator error messages: https://github.com/llvm/llvm-project/pull/80938 Before: ``` $ lldb -o "expr a+b" (lldb) expr a+b error: <user expression 0>:1:1: use of undeclared identifier 'a' a+b ^ error: <user expression 0>:1:3: use of undeclared identifier 'b' a+b ^ ``` After: ``` (lldb) expr a+b ^ ^ │ ╰─ error: use of undeclared identifier 'b' ╰─ error: use of undeclared identifier 'a' ``` This eliminates the confusing `<user expression 0>:1:3` source location and avoids echoing the expression to the console again, which results in a cleaner presentation that makes it easier to grasp what's going on. --- .../lldb/Expression/DiagnosticManager.h | 1 + lldb/include/lldb/Interpreter/CommandObject.h | 8 + .../Commands/CommandObjectExpression.cpp | 157 ++++++++++++++++-- .../source/Interpreter/CommandInterpreter.cpp | 4 +- .../Clang/ClangExpressionParser.cpp | 8 +- .../ctf/CommandObjectThreadTraceExportCTF.h | 2 +- lldb/source/Utility/Status.cpp | 3 +- .../diagnostics/TestExprDiagnostics.py | 50 ++++++ .../TestPersistentVariables.py | 4 +- .../TestStaticInitializers.py | 2 +- .../TestTemplateFunctions.py | 2 +- .../test/API/lang/mixed/TestMixedLanguages.py | 2 +- lldb/test/Shell/Expr/TestObjCIDCast.test | 2 +- .../test/Shell/Expr/TestObjCInCXXContext.test | 2 +- .../NativePDB/incomplete-tag-type.cpp | 4 +- .../Expression/DiagnosticManagerTest.cpp | 33 ++-- lldb/unittests/Interpreter/CMakeLists.txt | 1 + .../TestCommandObjectExpression.cpp | 34 ++++ 18 files changed, 274 insertions(+), 45 deletions(-) create mode 100644 lldb/unittests/Interpreter/TestCommandObjectExpression.cpp diff --git a/lldb/include/lldb/Expression/DiagnosticManager.h b/lldb/include/lldb/Expression/DiagnosticManager.h index 1c0763eea92f05..67e5082dff208e 100644 --- a/lldb/include/lldb/Expression/DiagnosticManager.h +++ b/lldb/include/lldb/Expression/DiagnosticManager.h @@ -39,6 +39,7 @@ struct DiagnosticDetail { unsigned line = 0; uint16_t column = 0; uint16_t length = 0; + bool hidden = false; bool in_user_input = false; }; /// Contains {{}, 1, 3, 3, true} in the example above. diff --git a/lldb/include/lldb/Interpreter/CommandObject.h b/lldb/include/lldb/Interpreter/CommandObject.h index 20c4769af90338..c5167e5e0ecb6a 100644 --- a/lldb/include/lldb/Interpreter/CommandObject.h +++ b/lldb/include/lldb/Interpreter/CommandObject.h @@ -340,6 +340,13 @@ class CommandObject : public std::enable_shared_from_this<CommandObject> { return false; } + /// Set the command input as it appeared in the terminal. This + /// is used to have errors refer directly to the original command. + void SetOriginalCommandString(std::string s) { m_original_command = s; } + + /// \param offset_in_command is on what column \c args_string + /// appears, if applicable. This enables diagnostics that refer back + /// to the user input. virtual void Execute(const char *args_string, CommandReturnObject &result) = 0; @@ -404,6 +411,7 @@ class CommandObject : public std::enable_shared_from_this<CommandObject> { std::string m_cmd_help_short; std::string m_cmd_help_long; std::string m_cmd_syntax; + std::string m_original_command; Flags m_flags; std::vector<CommandArgumentEntry> m_arguments; lldb::CommandOverrideCallback m_deprecated_command_override_callback; diff --git a/lldb/source/Commands/CommandObjectExpression.cpp b/lldb/source/Commands/CommandObjectExpression.cpp index 771194638e1b65..b5c6cd143eb06b 100644 --- a/lldb/source/Commands/CommandObjectExpression.cpp +++ b/lldb/source/Commands/CommandObjectExpression.cpp @@ -10,6 +10,7 @@ #include "CommandObjectExpression.h" #include "lldb/Core/Debugger.h" +#include "lldb/Expression/DiagnosticManager.h" #include "lldb/Expression/ExpressionVariable.h" #include "lldb/Expression/REPL.h" #include "lldb/Expression/UserExpression.h" @@ -398,6 +399,122 @@ CanBeUsedForElementCountPrinting(ValueObject &valobj) { return Status(); } +static llvm::raw_ostream &PrintSeverity(Stream &stream, + lldb::Severity severity) { + llvm::HighlightColor color; + llvm::StringRef text; + switch (severity) { + case eSeverityError: + color = llvm::HighlightColor::Error; + text = "error: "; + break; + case eSeverityWarning: + color = llvm::HighlightColor::Warning; + text = "warning: "; + break; + case eSeverityInfo: + color = llvm::HighlightColor::Remark; + text = "note: "; + break; + } + return llvm::WithColor(stream.AsRawOstream(), color, llvm::ColorMode::Enable) + << text; +} + +namespace lldb_private { +// Public for unittesting. +void RenderDiagnosticDetails(Stream &stream, + std::optional<uint16_t> offset_in_command, + bool multiline, + llvm::ArrayRef<DiagnosticDetail> details) { + if (details.empty()) + return; + + if (!offset_in_command) { + for (const DiagnosticDetail &detail : details) { + PrintSeverity(stream, detail.severity); + stream << detail.rendered << '\n'; + } + return; + } + + // Print a line with caret indicator(s) below the lldb prompt + command. + const size_t padding = *offset_in_command; + stream << std::string(padding, ' '); + + size_t offset = 1; + std::vector<DiagnosticDetail> remaining_details, other_details, + hidden_details; + for (const DiagnosticDetail &detail : details) { + if (multiline || !detail.source_location) { + other_details.push_back(detail); + continue; + } + if (detail.source_location->hidden) { + hidden_details.push_back(detail); + continue; + } + if (!detail.source_location->in_user_input) { + other_details.push_back(detail); + continue; + } + + auto &loc = *detail.source_location; + remaining_details.push_back(detail); + if (offset > loc.column) + continue; + stream << std::string(loc.column - offset, ' ') << '^'; + if (loc.length > 1) + stream << std::string(loc.length - 1, '~'); + offset = loc.column + 1; + } + stream << '\n'; + + // Work through each detail in reverse order using the vector/stack. + bool did_print = false; + for (auto detail = remaining_details.rbegin(); + detail != remaining_details.rend(); + ++detail, remaining_details.pop_back()) { + // Get the information to print this detail and remove it from the stack. + // Print all the lines for all the other messages first. + stream << std::string(padding, ' '); + size_t offset = 1; + for (auto &remaining_detail : + llvm::ArrayRef(remaining_details).drop_back(1)) { + uint16_t column = remaining_detail.source_location->column; + stream << std::string(column - offset, ' ') << "│"; + offset = column + 1; + } + + // Print the line connecting the ^ with the error message. + uint16_t column = detail->source_location->column; + if (offset <= column) + stream << std::string(column - offset, ' ') << "╰─ "; + + // Print a colorized string based on the message's severity type. + PrintSeverity(stream, detail->severity); + + // Finally, print the message and start a new line. + stream << detail->message << '\n'; + did_print = true; + } + + // Print the non-located details. + for (const DiagnosticDetail &detail : other_details) { + PrintSeverity(stream, detail.severity); + stream << detail.rendered << '\n'; + did_print = true; + } + + // Print the hidden details as a last resort. + if (!did_print) + for (const DiagnosticDetail &detail : hidden_details) { + PrintSeverity(stream, detail.severity); + stream << detail.rendered << '\n'; + } +} +} // namespace lldb_private + bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr, Stream &output_stream, Stream &error_stream, @@ -486,19 +603,37 @@ bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr, result.SetStatus(eReturnStatusSuccessFinishResult); } else { - const char *error_cstr = result_valobj_sp->GetError().AsCString(); - if (error_cstr && error_cstr[0]) { - const size_t error_cstr_len = strlen(error_cstr); - const bool ends_with_newline = error_cstr[error_cstr_len - 1] == '\n'; - if (strstr(error_cstr, "error:") != error_cstr) - error_stream.PutCString("error: "); - error_stream.Write(error_cstr, error_cstr_len); - if (!ends_with_newline) - error_stream.EOL(); + // Retrieve the diagnostics. + std::vector<DiagnosticDetail> details; + llvm::consumeError( + llvm::handleErrors(result_valobj_sp->GetError().ToError(), + [&](DetailedExpressionError &error) { + details.push_back(error.GetDetail()); + })); + // Find the position of the expression in the command. + std::optional<uint16_t> expr_pos; + size_t nchar = m_original_command.find(expr); + if (nchar != std::string::npos) + expr_pos = nchar + GetDebugger().GetPrompt().size(); + + if (!details.empty()) { + bool multiline = expr.contains('\n'); + RenderDiagnosticDetails(error_stream, expr_pos, multiline, details); } else { - error_stream.PutCString("error: unknown error\n"); + const char *error_cstr = result_valobj_sp->GetError().AsCString(); + if (error_cstr && error_cstr[0]) { + const size_t error_cstr_len = strlen(error_cstr); + const bool ends_with_newline = + error_cstr[error_cstr_len - 1] == '\n'; + if (strstr(error_cstr, "error:") != error_cstr) + error_stream.PutCString("error: "); + error_stream.Write(error_cstr, error_cstr_len); + if (!ends_with_newline) + error_stream.EOL(); + } else { + error_stream.PutCString("error: unknown error\n"); + } } - result.SetStatus(eReturnStatusFailed); } } diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index b93f47a8a8d5ec..49681f06802443 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -1887,7 +1887,8 @@ bool CommandInterpreter::HandleCommand(const char *command_line, CommandReturnObject &result, bool force_repeat_command) { std::string command_string(command_line); - std::string original_command_string(command_line); + std::string original_command_string(command_string); + std::string real_original_command_string(command_string); Log *log = GetLog(LLDBLog::Commands); llvm::PrettyStackTraceFormat stack_trace("HandleCommand(command = \"%s\")", @@ -2076,6 +2077,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line, } ElapsedTime elapsed(execute_time); + cmd_obj->SetOriginalCommandString(real_original_command_string); cmd_obj->Execute(remainder.c_str(), result); } diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp index 92bd554767dae2..2aac25543a3a0f 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp @@ -215,8 +215,6 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer { m_os->flush(); DiagnosticDetail detail; - bool make_new_diagnostic = true; - switch (DiagLevel) { case DiagnosticsEngine::Level::Fatal: case DiagnosticsEngine::Level::Error: @@ -230,9 +228,6 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer { detail.severity = lldb::eSeverityInfo; break; case DiagnosticsEngine::Level::Note: - m_manager->AppendMessageToDiagnostic(m_output); - make_new_diagnostic = false; - // 'note:' diagnostics for errors and warnings can also contain Fix-Its. // We add these Fix-Its to the last error diagnostic to make sure // that we later have all Fix-Its related to an 'error' diagnostic when @@ -250,7 +245,6 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer { AddAllFixIts(clang_diag, Info); break; } - if (make_new_diagnostic) { // ClangDiagnostic messages are expected to have no whitespace/newlines // around them. std::string stripped_output = @@ -270,6 +264,7 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer { loc.line = fsloc.getSpellingLineNumber(); loc.column = fsloc.getSpellingColumnNumber(); loc.in_user_input = filename == m_filename; + loc.hidden = filename.starts_with("<lldb wrapper "); // Find the range of the primary location. for (const auto &range : Info.getRanges()) { @@ -299,7 +294,6 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer { AddAllFixIts(new_diagnostic.get(), Info); m_manager->AddDiagnostic(std::move(new_diagnostic)); - } } void BeginSourceFile(const LangOptions &LO, const Preprocessor *PP) override { diff --git a/lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h b/lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h index 1a034e87cfb65b..06834edf14ea16 100644 --- a/lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h +++ b/lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h @@ -48,7 +48,7 @@ class CommandObjectThreadTraceExportCTF : public CommandObjectParsed { Options *GetOptions() override { return &m_options; } protected: - void DoExecute(Args &command, CommandReturnObject &result) override; + void DoExecute(Args &args, CommandReturnObject &result) override; CommandOptions m_options; }; diff --git a/lldb/source/Utility/Status.cpp b/lldb/source/Utility/Status.cpp index 3099e54af483ff..e2c3029244735e 100644 --- a/lldb/source/Utility/Status.cpp +++ b/lldb/source/Utility/Status.cpp @@ -8,6 +8,7 @@ #include "lldb/Utility/Status.h" +#include "lldb/Expression/DiagnosticManager.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" #include "lldb/Utility/VASPrintf.h" @@ -279,8 +280,6 @@ ErrorType Status::GetType() const { else if (error.convertToErrorCode().category() == lldb_generic_category() || error.convertToErrorCode() == llvm::inconvertibleErrorCode()) result = eErrorTypeGeneric; - else - result = eErrorTypeInvalid; }); return result; } diff --git a/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py b/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py index ddc1c3598480cf..f81d3852946b9a 100644 --- a/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py +++ b/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py @@ -183,3 +183,53 @@ def test_source_locations_from_objc_modules(self): # The NSLog definition source line should be printed. Return value and # the first argument are probably stable enough that this test can check for them. self.assertIn("void NSLog(NSString *format", value.GetError().GetCString()) + + def test_command_expr_formatting(self): + """Test that the source and caret positions LLDB prints are correct""" + self.build() + + (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint( + self, "// Break here", self.main_source_spec + ) + frame = thread.GetFrameAtIndex(0) + + def check(input_ref): + self.expect(input_ref[0], error=True, substrs=input_ref[1:]) + + check( + [ + "expression -- a+b", + " ^ ^", + " │ ╰─ error: use of undeclared identifier 'b'", + " ╰─ error: use of undeclared identifier 'a'", + ] + ) + + check( + [ + "expr -- a", + " ^", + " ╰─ error: use of undeclared identifier 'a'", + ] + ) + check( + [ + "expr -i 0 -o 0 -- a", + " ^", + " ╰─ error: use of undeclared identifier 'a'", + ] + ) + + self.expect( + "expression --top-level -- template<typename T> T FOO(T x) { return x/2;}" + ) + check( + [ + 'expression -- FOO("")', + " ^", + " ╰─ note: in instantiation of function template specialization 'FOO<const char *>' requested here", + "error: <user expression", + "invalid operands to binary expression", + ] + ) + check(["expression --\na\n+\nb", "error: <user", "a", "error: <user", "b"]) diff --git a/lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py b/lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py index 6a7995ff2a837e..d0d9358589a719 100644 --- a/lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py +++ b/lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py @@ -56,7 +56,7 @@ def test_persistent_variables(self): self.expect( "expr int $i = 123", error=True, - substrs=["error: redefinition of persistent variable '$i'"], + substrs=["redefinition of persistent variable '$i'"], ) self.expect_expr("$i", result_type="int", result_value="5") @@ -65,7 +65,7 @@ def test_persistent_variables(self): self.expect( "expr long $i = 123", error=True, - substrs=["error: redefinition of persistent variable '$i'"], + substrs=["redefinition of persistent variable '$i'"], ) self.expect_expr("$i", result_type="int", result_value="5") diff --git a/lldb/test/API/commands/expression/static-initializers/TestStaticInitializers.py b/lldb/test/API/commands/expression/static-initializers/TestStaticInitializers.py index ea3aa6a4608c41..c734033bd6c028 100644 --- a/lldb/test/API/commands/expression/static-initializers/TestStaticInitializers.py +++ b/lldb/test/API/commands/expression/static-initializers/TestStaticInitializers.py @@ -35,7 +35,7 @@ def test_failing_init(self): self.expect( "expr -p -- struct Foo2 { Foo2() { do_abort(); } }; Foo2 f;", error=True, - substrs=["error: couldn't run static initializer:"], + substrs=["couldn't run static initializer:"], ) def test_without_process(self): diff --git a/lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py b/lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py index 71df90e6a6d161..8d4200b1076d03 100644 --- a/lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py +++ b/lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py @@ -21,7 +21,7 @@ def do_test_template_function(self, add_cast): "expr b1 <=> b2", error=True, substrs=[ - "warning: <user expression 0>:1:4: '<=>' is a single token in C++20; add a space to avoid a change in behavior" + "warning:", "'<=>' is a single token in C++20; add a space to avoid a change in behavior" ], ) diff --git a/lldb/test/API/lang/mixed/TestMixedLanguages.py b/lldb/test/API/lang/mixed/TestMixedLanguages.py index 8b73254cce4a93..1637d59a5edcba 100644 --- a/lldb/test/API/lang/mixed/TestMixedLanguages.py +++ b/lldb/test/API/lang/mixed/TestMixedLanguages.py @@ -40,7 +40,7 @@ def cleanup(): self.runCmd("run") self.expect("thread backtrace", substrs=["`main", "lang=c"]) # Make sure evaluation of C++11 fails. - self.expect("expr foo != nullptr", error=True, startstr="error") + self.expect("expr foo != nullptr", error=True, substrs=["error"]) # Run to BP at foo (in foo.cpp) and test that the language is C++. self.runCmd("breakpoint set -n foo") diff --git a/lldb/test/Shell/Expr/TestObjCIDCast.test b/lldb/test/Shell/Expr/TestObjCIDCast.test index 0611171da09e2e..19ca404643c1d9 100644 --- a/lldb/test/Shell/Expr/TestObjCIDCast.test +++ b/lldb/test/Shell/Expr/TestObjCIDCast.test @@ -6,4 +6,4 @@ // RUN: 2>&1 | FileCheck %s // CHECK: (lldb) expression --language objc -- *(id)0x1 -// CHECK: error: Couldn't apply expression side effects : Couldn't dematerialize a result variable: couldn't read its memory +// CHECK: error:{{.*}}Couldn't apply expression side effects : Couldn't dematerialize a result variable: couldn't read its memory diff --git a/lldb/test/Shell/Expr/TestObjCInCXXContext.test b/lldb/test/Shell/Expr/TestObjCInCXXContext.test index 8537799bdeb674..f8cad5b58a1e53 100644 --- a/lldb/test/Shell/Expr/TestObjCInCXXContext.test +++ b/lldb/test/Shell/Expr/TestObjCInCXXContext.test @@ -18,4 +18,4 @@ // CHECK-NEXT: (NSString *){{.*}}= nil // CHECK: (lldb) expression NSString -// CHECK-NEXT: error:{{.*}} use of undeclared identifier 'NSString' +// CHECK: error:{{.*}}use of undeclared identifier 'NSString' diff --git a/lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp b/lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp index a249057282d890..8c168286903014 100644 --- a/lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp +++ b/lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp @@ -13,9 +13,7 @@ // CHECK: (lldb) expression d // CHECK: (D) $1 = {} // CHECK: (lldb) expression static_e_ref -// CHECK: error: {{.*}}incomplete type 'E' where a complete type is required -// CHECK: static_e_ref -// CHECK: ^ +// CHECK: error:{{.*}}incomplete type 'E' where a complete type is required // Complete base class. struct A { int x; A(); }; diff --git a/lldb/unittests/Expression/DiagnosticManagerTest.cpp b/lldb/unittests/Expression/DiagnosticManagerTest.cpp index 4608b779f79a96..2373b8101bf4e2 100644 --- a/lldb/unittests/Expression/DiagnosticManagerTest.cpp +++ b/lldb/unittests/Expression/DiagnosticManagerTest.cpp @@ -72,18 +72,25 @@ TEST(DiagnosticManagerTest, HasFixits) { EXPECT_TRUE(mgr.HasFixIts()); } +static std::string toString(DiagnosticManager &mgr) { + std::string s = llvm::toString(mgr.GetAsError("")); + if (s.empty()) + return s; + return s.substr(1) + "\n"; +} + TEST(DiagnosticManagerTest, GetStringNoDiags) { DiagnosticManager mgr; - EXPECT_EQ("", mgr.GetString()); + EXPECT_EQ("", toString(mgr)); std::unique_ptr<Diagnostic> empty; mgr.AddDiagnostic(std::move(empty)); - EXPECT_EQ("", mgr.GetString()); + EXPECT_EQ("", toString(mgr)); } TEST(DiagnosticManagerTest, GetStringBasic) { DiagnosticManager mgr; mgr.AddDiagnostic(std::make_unique<TextDiag>("abc", lldb::eSeverityError)); - EXPECT_EQ("error: abc\n", mgr.GetString()); + EXPECT_EQ("error: abc\n", toString(mgr)); } TEST(DiagnosticManagerTest, GetStringMultiline) { @@ -91,15 +98,15 @@ TEST(DiagnosticManagerTest, GetStringMultiline) { // Multiline diagnostics should only get one severity label. mgr.AddDiagnostic(std::make_unique<TextDiag>("b\nc", lldb::eSeverityError)); - EXPECT_EQ("error: b\nc\n", mgr.GetString()); + EXPECT_EQ("error: b\nc\n", toString(mgr)); } TEST(DiagnosticManagerTest, GetStringMultipleDiags) { DiagnosticManager mgr; mgr.AddDiagnostic(std::make_unique<TextDiag>("abc", lldb::eSeverityError)); - EXPECT_EQ("error: abc\n", mgr.GetString()); + EXPECT_EQ("error: abc\n", toString(mgr)); mgr.AddDiagnostic(std::make_unique<TextDiag>("def", lldb::eSeverityError)); - EXPECT_EQ("error: abc\nerror: def\n", mgr.GetString()); + EXPECT_EQ("error: abc\nerror: def\n", toString(mgr)); } TEST(DiagnosticManagerTest, GetStringSeverityLabels) { @@ -110,7 +117,7 @@ TEST(DiagnosticManagerTest, GetStringSeverityLabels) { mgr.AddDiagnostic(std::make_unique<TextDiag>("bar", lldb::eSeverityWarning)); // Remarks have no labels. mgr.AddDiagnostic(std::make_unique<TextDiag>("baz", lldb::eSeverityInfo)); - EXPECT_EQ("error: foo\nwarning: bar\nbaz\n", mgr.GetString()); + EXPECT_EQ("error: foo\nwarning: bar\nbaz\n", toString(mgr)); } TEST(DiagnosticManagerTest, GetStringPreserveOrder) { @@ -120,7 +127,7 @@ TEST(DiagnosticManagerTest, GetStringPreserveOrder) { mgr.AddDiagnostic(std::make_unique<TextDiag>("baz", lldb::eSeverityInfo)); mgr.AddDiagnostic(std::make_unique<TextDiag>("bar", lldb::eSeverityWarning)); mgr.AddDiagnostic(std::make_unique<TextDiag>("foo", lldb::eSeverityError)); - EXPECT_EQ("baz\nwarning: bar\nerror: foo\n", mgr.GetString()); + EXPECT_EQ("baz\nwarning: bar\nerror: foo\n", toString(mgr)); } TEST(DiagnosticManagerTest, AppendMessageNoDiag) { @@ -139,7 +146,7 @@ TEST(DiagnosticManagerTest, AppendMessageAttachToLastDiag) { // This should append to 'bar' and not to 'foo'. mgr.AppendMessageToDiagnostic("message text"); - EXPECT_EQ("error: foo\nerror: bar\nmessage text\n", mgr.GetString()); + EXPECT_EQ("error: foo\nerror: bar\nmessage text\n", toString(mgr)); } TEST(DiagnosticManagerTest, AppendMessageSubsequentDiags) { @@ -150,7 +157,7 @@ TEST(DiagnosticManagerTest, AppendMessageSubsequentDiags) { // Pushing another diag after the message should work fine. mgr.AddDiagnostic(std::make_unique<TextDiag>("foo", lldb::eSeverityError)); - EXPECT_EQ("error: bar\nmessage text\nerror: foo\n", mgr.GetString()); + EXPECT_EQ("error: bar\nmessage text\nerror: foo\n", toString(mgr)); } TEST(DiagnosticManagerTest, PutString) { @@ -159,7 +166,7 @@ TEST(DiagnosticManagerTest, PutString) { mgr.PutString(lldb::eSeverityError, "foo"); EXPECT_EQ(1U, mgr.Diagnostics().size()); EXPECT_EQ(eDiagnosticOriginLLDB, mgr.Diagnostics().front()->getKind()); - EXPECT_EQ("error: foo\n", mgr.GetString()); + EXPECT_EQ("error: foo\n", toString(mgr)); } TEST(DiagnosticManagerTest, PutStringMultiple) { @@ -169,7 +176,7 @@ TEST(DiagnosticManagerTest, PutStringMultiple) { mgr.PutString(lldb::eSeverityError, "foo"); mgr.PutString(lldb::eSeverityError, "bar"); EXPECT_EQ(2U, mgr.Diagnostics().size()); - EXPECT_EQ("error: foo\nerror: bar\n", mgr.GetString()); + EXPECT_EQ("error: foo\nerror: bar\n", toString(mgr)); } TEST(DiagnosticManagerTest, PutStringSeverities) { @@ -180,7 +187,7 @@ TEST(DiagnosticManagerTest, PutStringSeverities) { mgr.PutString(lldb::eSeverityError, "foo"); mgr.PutString(lldb::eSeverityWarning, "bar"); EXPECT_EQ(2U, mgr.Diagnostics().size()); - EXPECT_EQ("error: foo\nwarning: bar\n", mgr.GetString()); + EXPECT_EQ("error: foo\nwarning: bar\n", toString(mgr)); } TEST(DiagnosticManagerTest, FixedExpression) { diff --git a/lldb/unittests/Interpreter/CMakeLists.txt b/lldb/unittests/Interpreter/CMakeLists.txt index 54cea995084d3d..14a7d6c5610388 100644 --- a/lldb/unittests/Interpreter/CMakeLists.txt +++ b/lldb/unittests/Interpreter/CMakeLists.txt @@ -1,5 +1,6 @@ add_lldb_unittest(InterpreterTests TestCommandPaths.cpp + TestCommandObjectExpression.cpp TestCompletion.cpp TestOptionArgParser.cpp TestOptions.cpp diff --git a/lldb/unittests/Interpreter/TestCommandObjectExpression.cpp b/lldb/unittests/Interpreter/TestCommandObjectExpression.cpp new file mode 100644 index 00000000000000..9fe0432fd22653 --- /dev/null +++ b/lldb/unittests/Interpreter/TestCommandObjectExpression.cpp @@ -0,0 +1,34 @@ +#include "lldb/Expression/DiagnosticManager.h" +#include "lldb/Utility/StreamString.h" +#include "gtest/gtest.h" + +namespace lldb_private { +std::string RenderDiagnosticDetails(Stream &stream, + std::optional<uint16_t> offset_in_command, + bool multiline, + llvm::ArrayRef<DiagnosticDetail> details); +} + +using namespace lldb_private; +using namespace lldb; +using llvm::StringRef; +namespace { +class ErrorDisplayTest : public ::testing::Test {}; +} // namespace + +static std::string Render(std::vector<DiagnosticDetail> details) { + StreamString stream; + RenderDiagnosticDetails(stream, 0, details); + return stream.GetData(); +} + +TEST_F(ErrorDisplayTest, RenderStatus) { + DiagnosticDetail::SourceLocation inline_loc; + inline_loc.in_user_input = true; + { + std::string result = + Render({DiagnosticDetail{inline_loc, eSeverityError, "foo", ""}}); + ASSERT_TRUE(StringRef(result).contains("error:")); + ASSERT_TRUE(StringRef(result).contains("foo")); + } +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
