[lldb][expr] Propagate ClangDynamicCheckerFunctions::Install() errors to caller

[sgraenitz]

Summary

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).

[bulbazord]

Thanks for improving the error message here!

[sgraenitz]

Thanks for the quick replies!

lldb/include/lldb/Expression/DynamicCheckerFunctions.h

 //===-- DynamicCheckerFunctions.h -------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_EXPRESSION_DYNAMICCHECKERFUNCTIONS_H
 #define LLDB_EXPRESSION_DYNAMICCHECKERFUNCTIONS_H
 
 #include "lldb/lldb-types.h"
 
+#include "llvm/Support/Error.h"
+
 namespace lldb_private {
 
 class DiagnosticManager;
 class ExecutionContext;
 
 /// Encapsulates dynamic check functions used by expressions.
 ///
 /// Each of the utility functions encapsulated in this class is responsible
public:
   ///
   /// \param[in] diagnostic_manager
   ///     A diagnostic manager to report errors to.
   ///
   /// \param[in] exe_ctx
   ///     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,
+  ///     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; }
 
 private:
   const DynamicCheckerFunctionsKind m_kind;
 };
 } // namespace lldb_private
 
 #endif // LLDB_EXPRESSION_DYNAMICCHECKERFUNCTIONS_H

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

if (decl_map) {
 
     if (execution_policy == eExecutionPolicyAlways ||
         (execution_policy != eExecutionPolicyTopLevel && !can_interpret)) {
       if (m_expr.NeedsValidation() && process) {
         if (!process->GetDynamicCheckers()) {
           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;
           }
 
           process->SetDynamicCheckers(dynamic_checkers);
 
           LLDB_LOGF(log, "== [ClangExpressionParser::PrepareForExecution] "
                          "Finished installing dynamic checkers ==");
         }

lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.h

public:
   ///
   /// \param[in] diagnostic_manager
   ///     A diagnostic manager to report errors to.
   ///
   /// \param[in] exe_ctx
   ///     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,
+  ///     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;
 
   std::shared_ptr<UtilityFunction> m_valid_pointer_check;
   std::shared_ptr<UtilityFunction> m_objc_object_check;
 };
 
 /// \class IRDynamicChecks IRDynamicChecks.h

lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.cpp

static const char g_valid_pointer_check_text[] =
     "    unsigned char $__lldb_local_val = *$__lldb_arg_ptr;\n"
     "}";
 
 ClangDynamicCheckerFunctions::ClangDynamicCheckerFunctions()
     : DynamicCheckerFunctions(DCF_Clang) {}
 
 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(
+  Expected<std::unique_ptr<UtilityFunction>> utility_fn =
+      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);
+  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<std::unique_ptr<UtilityFunction>> 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,
                                                          Stream &message) {
   // FIXME: We have to get the checkers to know why they scotched the call in
   // more detail,
   // so we can print a better message here.
   if (m_valid_pointer_check && m_valid_pointer_check->ContainsAddress(addr)) {