[Lldb-commits] [lldb] 32baf5c - [lldb][expr] Propagate ClangDynamicCheckerFunctions::Install() errors to caller

[Stefan Gränitz via lldb-commits]

Author: Stefan Gränitz
Date: 2023-03-21T19:05:23+01:00
New Revision: 32baf5c1c29b6b2f282354c9f5919865bc1ff958

URL: https://github.com/llvm/llvm-project/commit/32baf5c1c29b6b2f282354c9f5919865bc1ff958
DIFF: https://github.com/llvm/llvm-project/commit/32baf5c1c29b6b2f282354c9f5919865bc1ff958.diff

LOG: [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

Added: 
    

Modified: 
    lldb/include/lldb/Expression/DynamicCheckerFunctions.h
    lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
    lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.cpp
    lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.h

Removed: 
    


################################################################################
diff  --git a/lldb/include/lldb/Expression/DynamicCheckerFunctions.h b/lldb/include/lldb/Expression/DynamicCheckerFunctions.h
index 02bce5abdf4cb..57a93ca30586a 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 @@ class DynamicCheckerFunctions {
   ///     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 0b40df141f098..9852bbc62aa43 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 0549868274685..cd7d1ff6148b3 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<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)
+    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,

diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.h b/lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.h
index 4abd16c5c3261..ff20c1f08be0c 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/IRDynamicChecks.h
@@ -46,10 +46,10 @@ class ClangDynamicCheckerFunctions
   ///     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;