From 32baf5c1c29b6b2f282354c9f5919865bc1ff958 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Stefan=20Gr=C3=A4nitz?= Date: Tue, 21 Mar 2023 19:01:54 +0100 Subject: [PATCH] [lldb][expr] Propagate ClangDynamicCheckerFunctions::Install() errors to caller I came accross this, because a lot of regression tests were saying: ``` (lldb) p argc error: expression failed to parse: error: couldn't install checkers, unknown error ``` With this change, error messages provide more detail: ``` (lldb) p argc error: expression failed to parse: error: couldn't install checkers: error: Couldn't lookup symbols: __objc_load ``` I didn't find a case where `Diagnostics()` is not empty. Also it looks like this isn't covered in any test (yet). Reviewed By: bulbazord, Michael137 Differential Revision: https://reviews.llvm.org/D146541 --- .../lldb/Expression/DynamicCheckerFunctions.h | 10 ++++--- .../Clang/ClangExpressionParser.cpp | 14 +++++----- .../ExpressionParser/Clang/IRDynamicChecks.cpp | 31 ++++++++++------------ .../ExpressionParser/Clang/IRDynamicChecks.h | 8 +++--- 4 files changed, 30 insertions(+), 33 deletions(-) diff --git a/lldb/include/lldb/Expression/DynamicCheckerFunctions.h b/lldb/include/lldb/Expression/DynamicCheckerFunctions.h index 02bce5a..57a93ca 100644 --- a/lldb/include/lldb/Expression/DynamicCheckerFunctions.h +++ b/lldb/include/lldb/Expression/DynamicCheckerFunctions.h @@ -11,6 +11,8 @@ #include "lldb/lldb-types.h" +#include "llvm/Support/Error.h" + namespace lldb_private { class DiagnosticManager; @@ -46,10 +48,10 @@ public: /// The execution context to install the functions into. /// /// \return - /// True on success; false on failure, or if the functions have - /// already been installed. - virtual bool Install(DiagnosticManager &diagnostic_manager, - ExecutionContext &exe_ctx) = 0; + /// Either llvm::ErrorSuccess or Error with llvm::ErrorInfo + /// + virtual llvm::Error Install(DiagnosticManager &diagnostic_manager, + ExecutionContext &exe_ctx) = 0; virtual bool DoCheckersExplainStop(lldb::addr_t addr, Stream &message) = 0; DynamicCheckerFunctionsKind GetKind() const { return m_kind; } diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp index 0b40df1..9852bbc 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp @@ -1403,14 +1403,12 @@ lldb_private::Status ClangExpressionParser::PrepareForExecution( ClangDynamicCheckerFunctions *dynamic_checkers = new ClangDynamicCheckerFunctions(); - DiagnosticManager install_diagnostics; - - if (!dynamic_checkers->Install(install_diagnostics, exe_ctx)) { - if (install_diagnostics.Diagnostics().size()) - err.SetErrorString(install_diagnostics.GetString().c_str()); - else - err.SetErrorString("couldn't install checkers, unknown error"); - + 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.SetErrorString(ErrMsg); return err; } diff --git a/lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.cpp b/lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.cpp index 0549868..cd7d1ff 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.cpp @@ -47,33 +47,30 @@ ClangDynamicCheckerFunctions::ClangDynamicCheckerFunctions() ClangDynamicCheckerFunctions::~ClangDynamicCheckerFunctions() = default; -bool ClangDynamicCheckerFunctions::Install( +llvm::Error ClangDynamicCheckerFunctions::Install( DiagnosticManager &diagnostic_manager, ExecutionContext &exe_ctx) { - auto utility_fn_or_error = exe_ctx.GetTargetRef().CreateUtilityFunction( - g_valid_pointer_check_text, VALID_POINTER_CHECK_NAME, - lldb::eLanguageTypeC, exe_ctx); - if (!utility_fn_or_error) { - llvm::consumeError(utility_fn_or_error.takeError()); - return false; - } - m_valid_pointer_check = std::move(*utility_fn_or_error); + Expected> utility_fn = + exe_ctx.GetTargetRef().CreateUtilityFunction( + g_valid_pointer_check_text, VALID_POINTER_CHECK_NAME, + lldb::eLanguageTypeC, exe_ctx); + if (!utility_fn) + return utility_fn.takeError(); + m_valid_pointer_check = std::move(*utility_fn); if (Process *process = exe_ctx.GetProcessPtr()) { ObjCLanguageRuntime *objc_language_runtime = ObjCLanguageRuntime::Get(*process); if (objc_language_runtime) { - auto utility_fn_or_error = objc_language_runtime->CreateObjectChecker( - VALID_OBJC_OBJECT_CHECK_NAME, exe_ctx); - if (!utility_fn_or_error) { - llvm::consumeError(utility_fn_or_error.takeError()); - return false; - } - m_objc_object_check = std::move(*utility_fn_or_error); + Expected> checker_fn = + objc_language_runtime->CreateObjectChecker(VALID_OBJC_OBJECT_CHECK_NAME, exe_ctx); + if (!checker_fn) + return checker_fn.takeError(); + m_objc_object_check = std::move(*checker_fn); } } - return true; + return Error::success(); } bool ClangDynamicCheckerFunctions::DoCheckersExplainStop(lldb::addr_t addr, diff --git a/lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.h b/lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.h index 4abd16c..ff20c1f 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.h +++ b/lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.h @@ -46,10 +46,10 @@ public: /// The execution context to install the functions into. /// /// \return - /// True on success; false on failure, or if the functions have - /// already been installed. - bool Install(DiagnosticManager &diagnostic_manager, - ExecutionContext &exe_ctx) override; + /// Either llvm::ErrorSuccess or Error with llvm::ErrorInfo + /// + llvm::Error Install(DiagnosticManager &diagnostic_manager, + ExecutionContext &exe_ctx) override; bool DoCheckersExplainStop(lldb::addr_t addr, Stream &message) override; -- 2.7.4