https://github.com/adrian-prantl updated https://github.com/llvm/llvm-project/pull/106442
>From 40d336f5ffcbf36346eb017f65c0c150fc7190d2 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 | 5 +- lldb/source/Breakpoint/BreakpointLocation.cpp | 10 +-- lldb/source/Expression/DiagnosticManager.cpp | 83 +++++++++++++---- lldb/source/Expression/ExpressionParser.cpp | 5 +- lldb/source/Expression/UserExpression.cpp | 28 +++--- lldb/source/Expression/UtilityFunction.cpp | 16 ++-- .../ExpressionParser/Clang/ClangDiagnostic.h | 5 +- .../Clang/ClangExpressionParser.cpp | 57 +++++++++--- .../Plugins/Platform/POSIX/PlatformPOSIX.cpp | 10 +-- .../Platform/Windows/PlatformWindows.cpp | 10 +-- lldb/source/Target/Target.cpp | 2 +- lldb/source/Utility/Status.cpp | 7 ++ .../TestModulesCompileError.py | 2 +- .../Expression/DiagnosticManagerTest.cpp | 12 +-- 15 files changed, 229 insertions(+), 111 deletions(-) diff --git a/lldb/include/lldb/Expression/DiagnosticManager.h b/lldb/include/lldb/Expression/DiagnosticManager.h index d49b7c99b114fb..bcf108a4d9d398 100644 --- a/lldb/include/lldb/Expression/DiagnosticManager.h +++ b/lldb/include/lldb/Expression/DiagnosticManager.h @@ -12,6 +12,9 @@ #include "lldb/lldb-defines.h" #include "lldb/lldb-types.h" +#include "lldb/Utility/FileSpec.h" +#include "lldb/Utility/Status.h" + #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringRef.h" @@ -20,6 +23,47 @@ namespace lldb_private { +/// A compiler-independent representation of a Diagnostic. Expression +/// evaluation failures often have more than one diagnostic that a UI +/// layer might want to render differently, for example to colorize +/// it. +/// +/// Running example: +/// (lldb) expr 1+foo +/// error: <user expression 0>:1:3: use of undeclared identifier 'foo' +/// 1+foo +/// ^ +struct DiagnosticDetail { + struct SourceLocation { + FileSpec file; + unsigned line = 0; + uint16_t column = 0; + uint16_t length = 0; + bool in_user_input = false; + }; + /// Contains {{}, 1, 3, 3, true} in the example above. + std::optional<SourceLocation> source_location; + /// Contains eSeverityError in the example above. + lldb::Severity severity = lldb::eSeverityInfo; + /// Contains "use of undeclared identifier 'x'" in the example above. + std::string message; + /// Contains the fully rendered error message. + std::string rendered; +}; + +/// An llvm::Error used to communicate diagnostics in Status. Multiple +/// diagnostics may be chained in an llvm::ErrorList. +class DetailedExpressionError + : public llvm::ErrorInfo<DetailedExpressionError, llvm::ECError> { + DiagnosticDetail m_detail; + +public: + using llvm::ErrorInfo<DetailedExpressionError, llvm::ECError>::ErrorInfo; + DetailedExpressionError(DiagnosticDetail detail) : m_detail(detail) {} + std::string message() const override; + static char ID; +}; + enum DiagnosticOrigin { eDiagnosticOriginUnknown = 0, eDiagnosticOriginLLDB, @@ -49,37 +93,28 @@ class Diagnostic { } } - Diagnostic(llvm::StringRef message, lldb::Severity severity, - DiagnosticOrigin origin, uint32_t compiler_id) - : m_message(message), m_severity(severity), m_origin(origin), - m_compiler_id(compiler_id) {} - - Diagnostic(const Diagnostic &rhs) - : m_message(rhs.m_message), m_severity(rhs.m_severity), - m_origin(rhs.m_origin), m_compiler_id(rhs.m_compiler_id) {} + Diagnostic(DiagnosticOrigin origin, uint32_t compiler_id, + DiagnosticDetail detail) + : m_origin(origin), m_compiler_id(compiler_id), m_detail(detail) {} virtual ~Diagnostic() = default; virtual bool HasFixIts() const { return false; } - lldb::Severity GetSeverity() const { return m_severity; } + lldb::Severity GetSeverity() const { return m_detail.severity; } uint32_t GetCompilerID() const { return m_compiler_id; } - llvm::StringRef GetMessage() const { return m_message; } + llvm::StringRef GetMessage() const { return m_detail.message; } + llvm::Error GetAsError() const; - void AppendMessage(llvm::StringRef message, - bool precede_with_newline = true) { - if (precede_with_newline) - m_message.push_back('\n'); - m_message += message; - } + void AppendMessage(llvm::StringRef message, bool precede_with_newline = true); protected: - std::string m_message; - lldb::Severity m_severity; DiagnosticOrigin m_origin; - uint32_t m_compiler_id; // Compiler-specific diagnostic ID + /// Compiler-specific diagnostic ID. + uint32_t m_compiler_id; + DiagnosticDetail m_detail; }; typedef std::vector<std::unique_ptr<Diagnostic>> DiagnosticList; @@ -102,10 +137,7 @@ class DiagnosticManager { void AddDiagnostic(llvm::StringRef message, lldb::Severity severity, DiagnosticOrigin origin, - uint32_t compiler_id = LLDB_INVALID_COMPILER_ID) { - m_diagnostics.emplace_back( - std::make_unique<Diagnostic>(message, severity, origin, compiler_id)); - } + uint32_t compiler_id = LLDB_INVALID_COMPILER_ID); void AddDiagnostic(std::unique_ptr<Diagnostic> diagnostic) { if (diagnostic) @@ -130,13 +162,21 @@ class DiagnosticManager { m_diagnostics.back()->AppendMessage(str); } + /// Copies the diagnostics into an llvm::Error{List}. + /// The first diagnostic wraps \c result. + llvm::Error GetAsError(lldb::ExpressionResults result) const; + + /// Copies the diagnostics into an llvm::Error, the first diagnostic + /// being an llvm::StringError. + llvm::Error GetAsError(llvm::Twine msg) const; + // Returns a string containing errors in this format: // // "error: error text\n // warning: warning text\n // remark text\n" std::string GetString(char separator = '\n'); - + void Dump(Log *log); const std::string &GetFixedExpression() { return m_fixed_expression; } diff --git a/lldb/include/lldb/Utility/Status.h b/lldb/include/lldb/Utility/Status.h index 4a09c38ce62f1b..360f7ec08bcecc 100644 --- a/lldb/include/lldb/Utility/Status.h +++ b/lldb/include/lldb/Utility/Status.h @@ -177,8 +177,11 @@ class Status { /// Avoid using this in new code. Migrate APIs to llvm::Expected instead. static Status FromError(llvm::Error error); - /// FIXME: Replace this with a takeError() method. + /// FIXME: Replace all uses with takeError() instead. llvm::Error ToError() const; + + llvm::Error takeError() { return std::move(m_error); } + /// Don't call this function in new code. Instead, redesign the API /// to use llvm::Expected instead of Status. Status Clone() const { return Status(ToError()); } diff --git a/lldb/source/Breakpoint/BreakpointLocation.cpp b/lldb/source/Breakpoint/BreakpointLocation.cpp index 8d7364052a006a..fd9248eb0677c5 100644 --- a/lldb/source/Breakpoint/BreakpointLocation.cpp +++ b/lldb/source/Breakpoint/BreakpointLocation.cpp @@ -264,9 +264,9 @@ bool BreakpointLocation::ConditionSaysStop(ExecutionContext &exe_ctx, if (!m_user_expression_sp->Parse(diagnostics, exe_ctx, eExecutionPolicyOnlyWhenNeeded, true, false)) { - error = Status::FromErrorStringWithFormat( - "Couldn't parse conditional expression:\n%s", - diagnostics.GetString().c_str()); + error = Status::FromError( + diagnostics.GetAsError("Couldn't parse conditional expression:")); + m_user_expression_sp.reset(); return true; } @@ -324,8 +324,8 @@ bool BreakpointLocation::ConditionSaysStop(ExecutionContext &exe_ctx, } } else { ret = false; - error = Status::FromErrorStringWithFormat( - "Couldn't execute expression:\n%s", diagnostics.GetString().c_str()); + error = Status::FromError( + diagnostics.GetAsError("Couldn't execute expression:")); } return ret; diff --git a/lldb/source/Expression/DiagnosticManager.cpp b/lldb/source/Expression/DiagnosticManager.cpp index a8330138f3d53b..1fff224fd912e9 100644 --- a/lldb/source/Expression/DiagnosticManager.cpp +++ b/lldb/source/Expression/DiagnosticManager.cpp @@ -14,22 +14,7 @@ #include "lldb/Utility/StreamString.h" using namespace lldb_private; - -void DiagnosticManager::Dump(Log *log) { - if (!log) - return; - - std::string str = GetString(); - - // GetString() puts a separator after each diagnostic. We want to remove the - // last '\n' because log->PutCString will add one for us. - - if (str.size() && str.back() == '\n') { - str.pop_back(); - } - - log->PutCString(str.c_str()); -} +char DetailedExpressionError::ID; static const char *StringForSeverity(lldb::Severity severity) { switch (severity) { @@ -44,9 +29,16 @@ static const char *StringForSeverity(lldb::Severity severity) { llvm_unreachable("switch needs another case for lldb::Severity enum"); } +std::string DetailedExpressionError::message() const { + std::string str; + llvm::raw_string_ostream(str) + << StringForSeverity(m_detail.severity) << m_detail.rendered; + return str; +} + std::string DiagnosticManager::GetString(char separator) { - std::string ret; - llvm::raw_string_ostream stream(ret); + std::string str; + llvm::raw_string_ostream stream(str); for (const auto &diagnostic : Diagnostics()) { llvm::StringRef severity = StringForSeverity(diagnostic->GetSeverity()); @@ -61,8 +53,51 @@ std::string DiagnosticManager::GetString(char separator) { stream << message.drop_front(severity_pos + severity.size()); stream << separator; } + return str; +} - return ret; +void DiagnosticManager::Dump(Log *log) { + if (!log) + return; + + std::string str = GetString(); + + // We want to remove the last '\n' because log->PutCString will add + // one for us. + + if (str.size() && str.back() == '\n') + str.pop_back(); + + log->PutString(str); +} + +llvm::Error Diagnostic::GetAsError() const { + return llvm::make_error<DetailedExpressionError>(m_detail); +} + +llvm::Error +DiagnosticManager::GetAsError(lldb::ExpressionResults result) const { + llvm::Error diags = Status::FromExpressionError(result, "").takeError(); + for (const auto &diagnostic : m_diagnostics) + diags = llvm::joinErrors(std::move(diags), diagnostic->GetAsError()); + return diags; +} + +llvm::Error +DiagnosticManager::GetAsError(llvm::Twine msg) const { + llvm::Error diags = llvm::createStringError(msg); + for (const auto &diagnostic : m_diagnostics) + diags = llvm::joinErrors(std::move(diags), diagnostic->GetAsError()); + return diags; +} + +void DiagnosticManager::AddDiagnostic(llvm::StringRef message, + lldb::Severity severity, + DiagnosticOrigin origin, + uint32_t compiler_id) { + m_diagnostics.emplace_back(std::make_unique<Diagnostic>( + origin, compiler_id, + DiagnosticDetail{{}, severity, message.str(), message.str()})); } size_t DiagnosticManager::Printf(lldb::Severity severity, const char *format, @@ -85,3 +120,13 @@ void DiagnosticManager::PutString(lldb::Severity severity, return; AddDiagnostic(str, severity, eDiagnosticOriginLLDB); } + +void Diagnostic::AppendMessage(llvm::StringRef message, + bool precede_with_newline) { + if (precede_with_newline) { + m_detail.message.push_back('\n'); + m_detail.rendered.push_back('\n'); + } + m_detail.message += message; + m_detail.rendered += message; +} diff --git a/lldb/source/Expression/ExpressionParser.cpp b/lldb/source/Expression/ExpressionParser.cpp index 868556c1c58a57..26bc19874c53cc 100644 --- a/lldb/source/Expression/ExpressionParser.cpp +++ b/lldb/source/Expression/ExpressionParser.cpp @@ -63,9 +63,8 @@ ExpressionParser::RunStaticInitializers(IRExecutionUnitSP &execution_unit_sp, exe_ctx, call_static_initializer, options, execution_errors); if (results != eExpressionCompleted) { - err = Status::FromErrorStringWithFormat( - "couldn't run static initializer: %s", - execution_errors.GetString().c_str()); + err = Status::FromError( + execution_errors.GetAsError("couldn't run static initializer:")); return err; } } diff --git a/lldb/source/Expression/UserExpression.cpp b/lldb/source/Expression/UserExpression.cpp index 872f6304f91baa..3e035cfbff567b 100644 --- a/lldb/source/Expression/UserExpression.cpp +++ b/lldb/source/Expression/UserExpression.cpp @@ -328,18 +328,20 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx, } if (!parse_success) { - std::string msg; - { - llvm::raw_string_ostream os(msg); - if (!diagnostic_manager.Diagnostics().empty()) - os << diagnostic_manager.GetString(); - else - os << "expression failed to parse (no further compiler diagnostics)"; - if (target->GetEnableNotifyAboutFixIts() && fixed_expression && - !fixed_expression->empty()) - os << "\nfixed expression suggested:\n " << *fixed_expression; + if (target->GetEnableNotifyAboutFixIts() && fixed_expression && + !fixed_expression->empty()) { + std::string fixit = + "fixed expression suggested:\n " + *fixed_expression; + diagnostic_manager.AddDiagnostic(fixit, lldb::eSeverityInfo, + eDiagnosticOriginLLDB); } - error = Status::FromExpressionError(execution_results, msg); + if (!diagnostic_manager.Diagnostics().size()) + error = Status::FromExpressionError( + execution_results, + "expression failed to parse (no further compiler diagnostics)"); + else + error = + Status::FromError(diagnostic_manager.GetAsError(execution_results)); } } @@ -384,8 +386,8 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx, error = Status::FromExpressionError( execution_results, "expression failed to execute, unknown error"); else - error = Status::FromExpressionError(execution_results, - diagnostic_manager.GetString()); + error = Status::FromError( + diagnostic_manager.GetAsError(execution_results)); } else { if (expr_result) { result_valobj_sp = expr_result->GetValueObject(); diff --git a/lldb/source/Expression/UtilityFunction.cpp b/lldb/source/Expression/UtilityFunction.cpp index 55ebfb8ef342e8..e35c0774815298 100644 --- a/lldb/source/Expression/UtilityFunction.cpp +++ b/lldb/source/Expression/UtilityFunction.cpp @@ -83,19 +83,18 @@ FunctionCaller *UtilityFunction::MakeFunctionCaller( m_caller_up.reset(process_sp->GetTarget().GetFunctionCallerForLanguage( Language().AsLanguageType(), return_type, impl_code_address, arg_value_list, name.c_str(), error)); - if (error.Fail()) { - + if (error.Fail()) return nullptr; - } + if (m_caller_up) { DiagnosticManager diagnostics; unsigned num_errors = m_caller_up->CompileFunction(thread_to_use_sp, diagnostics); if (num_errors) { - error = Status::FromErrorStringWithFormat( - "Error compiling %s caller function: \"%s\".", - m_function_name.c_str(), diagnostics.GetString().c_str()); + error = Status::FromError(diagnostics.GetAsError( + "Error compiling " + m_function_name + " caller function:")); + m_caller_up.reset(); return nullptr; } @@ -104,9 +103,8 @@ FunctionCaller *UtilityFunction::MakeFunctionCaller( ExecutionContext exe_ctx(process_sp); if (!m_caller_up->WriteFunctionWrapper(exe_ctx, diagnostics)) { - error = Status::FromErrorStringWithFormat( - "Error inserting caller function for %s: \"%s\".", - m_function_name.c_str(), diagnostics.GetString().c_str()); + error = Status::FromError(diagnostics.GetAsError( + "Error inserting " + m_function_name + " caller function:")); m_caller_up.reset(); return nullptr; } diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h index 21abd71cc34eeb..c473df808ee8d0 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h @@ -29,9 +29,8 @@ class ClangDiagnostic : public Diagnostic { return diag->getKind() == eDiagnosticOriginClang; } - ClangDiagnostic(llvm::StringRef message, lldb::Severity severity, - uint32_t compiler_id) - : Diagnostic(message, severity, eDiagnosticOriginClang, compiler_id) {} + ClangDiagnostic(DiagnosticDetail detail, uint32_t compiler_id) + : Diagnostic(eDiagnosticOriginClang, compiler_id, detail) {} ~ClangDiagnostic() override = default; diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp index 2fe3c0460aa7f8..07b1660e203870 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp @@ -26,6 +26,7 @@ #include "clang/Frontend/FrontendActions.h" #include "clang/Frontend/FrontendDiagnostic.h" #include "clang/Frontend/FrontendPluginRegistry.h" +#include "clang/Frontend/TextDiagnostic.h" #include "clang/Frontend/TextDiagnosticBuffer.h" #include "clang/Frontend/TextDiagnosticPrinter.h" #include "clang/Lex/Preprocessor.h" @@ -212,20 +213,20 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer { m_passthrough->HandleDiagnostic(DiagLevel, Info); m_os->flush(); - lldb::Severity severity; + DiagnosticDetail detail; bool make_new_diagnostic = true; switch (DiagLevel) { case DiagnosticsEngine::Level::Fatal: case DiagnosticsEngine::Level::Error: - severity = lldb::eSeverityError; + detail.severity = lldb::eSeverityError; break; case DiagnosticsEngine::Level::Warning: - severity = lldb::eSeverityWarning; + detail.severity = lldb::eSeverityWarning; break; case DiagnosticsEngine::Level::Remark: case DiagnosticsEngine::Level::Ignored: - severity = lldb::eSeverityInfo; + detail.severity = lldb::eSeverityInfo; break; case DiagnosticsEngine::Level::Note: m_manager->AppendMessageToDiagnostic(m_output); @@ -254,14 +255,46 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer { std::string stripped_output = std::string(llvm::StringRef(m_output).trim()); - auto new_diagnostic = std::make_unique<ClangDiagnostic>( - stripped_output, severity, Info.getID()); + // Translate the source location. + if (Info.hasSourceManager()) { + DiagnosticDetail::SourceLocation loc; + clang::SourceManager &sm = Info.getSourceManager(); + const clang::SourceLocation sloc = Info.getLocation(); + if (sloc.isValid()) { + const clang::FullSourceLoc fsloc(sloc, sm); + clang::PresumedLoc PLoc = fsloc.getPresumedLoc(true); + StringRef filename = + PLoc.isValid() ? PLoc.getFilename() : StringRef{}; + loc.file = FileSpec(filename); + loc.line = fsloc.getSpellingLineNumber(); + loc.column = fsloc.getSpellingColumnNumber(); + // A heuristic detecting the #line 1 "<user expression 1>". + loc.in_user_input = filename.starts_with("<user expression "); + // Find the range of the primary location. + for (const auto &range : Info.getRanges()) { + if (range.getBegin() == sloc) { + // FIXME: This is probably not handling wide characters correctly. + unsigned end_col = sm.getSpellingColumnNumber(range.getEnd()); + if (end_col > loc.column) + loc.length = end_col - loc.column; + break; + } + } + detail.source_location = loc; + } + } + llvm::SmallString<0> msg; + Info.FormatDiagnostic(msg); + detail.message = msg.str(); + detail.rendered = stripped_output; + auto new_diagnostic = + std::make_unique<ClangDiagnostic>(detail, Info.getID()); // Don't store away warning fixits, since the compiler doesn't have // enough context in an expression for the warning to be useful. // FIXME: Should we try to filter out FixIts that apply to our generated // code, and not the user's expression? - if (severity == lldb::eSeverityError) + if (detail.severity == lldb::eSeverityError) AddAllFixIts(new_diagnostic.get(), Info); m_manager->AddDiagnostic(std::move(new_diagnostic)); @@ -1503,13 +1536,9 @@ lldb_private::Status ClangExpressionParser::DoPrepareForExecution( new ClangDynamicCheckerFunctions(); DiagnosticManager install_diags; - if (Error Err = dynamic_checkers->Install(install_diags, exe_ctx)) { - std::string ErrMsg = "couldn't install checkers: " + toString(std::move(Err)); - if (install_diags.Diagnostics().size()) - ErrMsg = ErrMsg + "\n" + install_diags.GetString().c_str(); - err = Status(ErrMsg); - return err; - } + if (Error Err = dynamic_checkers->Install(install_diags, exe_ctx)) + return Status::FromError( + install_diags.GetAsError("couldn't install checkers:")); process->SetDynamicCheckers(dynamic_checkers); diff --git a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp index 31315e46ca168d..f0609596ded731 100644 --- a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp +++ b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp @@ -863,9 +863,8 @@ uint32_t PlatformPOSIX::DoLoadImage(lldb_private::Process *process, func_args_addr, arguments, diagnostics)) { - error = Status::FromErrorStringWithFormat( - "dlopen error: could not write function arguments: %s", - diagnostics.GetString().c_str()); + error = Status::FromError(diagnostics.GetAsError( + "dlopen error: could not write function arguments:" )); return LLDB_INVALID_IMAGE_TOKEN; } @@ -906,9 +905,8 @@ uint32_t PlatformPOSIX::DoLoadImage(lldb_private::Process *process, ExpressionResults results = do_dlopen_function->ExecuteFunction( exe_ctx, &func_args_addr, options, diagnostics, return_value); if (results != eExpressionCompleted) { - error = Status::FromErrorStringWithFormat( - "dlopen error: failed executing dlopen wrapper function: %s", - diagnostics.GetString().c_str()); + error = Status::FromError(diagnostics.GetAsError( + "dlopen error: failed executing dlopen wrapper function:")); return LLDB_INVALID_IMAGE_TOKEN; } diff --git a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp index 7352d6f33f2174..fa39d93a91297c 100644 --- a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp +++ b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp @@ -341,9 +341,8 @@ uint32_t PlatformWindows::DoLoadImage(Process *process, diagnostics.Clear(); if (!invocation->WriteFunctionArguments(context, injected_parameters, parameters, diagnostics)) { - error = Status::FromErrorStringWithFormat( - "LoadLibrary error: unable to write function parameters: %s", - diagnostics.GetString().c_str()); + error = Status::FromError(diagnostics.GetAsError( + "LoadLibrary error: unable to write function parameters:")); return LLDB_INVALID_IMAGE_TOKEN; } @@ -384,9 +383,8 @@ uint32_t PlatformWindows::DoLoadImage(Process *process, invocation->ExecuteFunction(context, &injected_parameters, options, diagnostics, value); if (result != eExpressionCompleted) { - error = Status::FromErrorStringWithFormat( - "LoadLibrary error: failed to execute LoadLibrary helper: %s", - diagnostics.GetString().c_str()); + error = Status::FromError(diagnostics.GetAsError( + "LoadLibrary error: failed to execute LoadLibrary helper:")); return LLDB_INVALID_IMAGE_TOKEN; } diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index 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 f557cb540b5655..5826c3aa0bcd0b 100644 --- a/lldb/source/Utility/Status.cpp +++ b/lldb/source/Utility/Status.cpp @@ -226,6 +226,13 @@ 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, ...]. + 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'; +>>>>>>> 542229ecd139 ([lldb] Store expression evaluator diagnostics in an llvm::Error (NFC)) + if (m_string.empty()) { if (default_error_str) m_string.assign(default_error_str); diff --git a/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py b/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py index 620b6e44fc852a..36e302be2525b5 100644 --- a/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py +++ b/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py @@ -21,7 +21,7 @@ def test(self): "expr @import LLDBTestModule", error=True, substrs=[ - "module.h:4:1: use of undeclared identifier 'syntax_error_for_lldb_to_find'", + "module.h:4:1: error: use of undeclared identifier 'syntax_error_for_lldb_to_find'", "syntax_error_for_lldb_to_find // comment that tests source printing", "could not build module 'LLDBTestModule'", ], diff --git a/lldb/unittests/Expression/DiagnosticManagerTest.cpp b/lldb/unittests/Expression/DiagnosticManagerTest.cpp index 05fe7c164d6818..596baedd728ffa 100644 --- a/lldb/unittests/Expression/DiagnosticManagerTest.cpp +++ b/lldb/unittests/Expression/DiagnosticManagerTest.cpp @@ -19,8 +19,8 @@ class FixItDiag : public Diagnostic { public: FixItDiag(llvm::StringRef msg, bool has_fixits) - : Diagnostic(msg, lldb::eSeverityError, - DiagnosticOrigin::eDiagnosticOriginLLDB, custom_diag_id), + : Diagnostic(DiagnosticOrigin::eDiagnosticOriginLLDB, custom_diag_id, + DiagnosticDetail{{}, lldb::eSeverityError, msg.str(), {}}), m_has_fixits(has_fixits) {} bool HasFixIts() const override { return m_has_fixits; } }; @@ -30,8 +30,8 @@ namespace { class TextDiag : public Diagnostic { public: TextDiag(llvm::StringRef msg, lldb::Severity severity) - : Diagnostic(msg, severity, DiagnosticOrigin::eDiagnosticOriginLLDB, - custom_diag_id) {} + : Diagnostic(DiagnosticOrigin::eDiagnosticOriginLLDB, custom_diag_id, + DiagnosticDetail{{}, severity, msg.str(), {}}) {} }; } // namespace @@ -42,8 +42,8 @@ TEST(DiagnosticManagerTest, AddDiagnostic) { std::string msg = "foo bar has happened"; lldb::Severity severity = lldb::eSeverityError; DiagnosticOrigin origin = DiagnosticOrigin::eDiagnosticOriginLLDB; - auto diag = - std::make_unique<Diagnostic>(msg, severity, origin, custom_diag_id); + auto diag = std::make_unique<Diagnostic>( + origin, custom_diag_id, DiagnosticDetail{{}, severity, msg, {}}); mgr.AddDiagnostic(std::move(diag)); EXPECT_EQ(1U, mgr.Diagnostics().size()); const Diagnostic *got = mgr.Diagnostics().front().get(); _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
