[lldb][ClangModulesDeclVendor] Revamp error handling when loading Clang modules (llvm#166917)

Instead of propagating the errors as a `bool`+`Stream` we change the
`ClangModulesDeclVendor` module loading APIs to use `llvm::Error`. We
also reword some of the diagnostics (notably removing the hardcoded
`error:` prefix). A follow-up patch will further make the module loading
errors less noisy.

See the new tests for what the errors look like.

rdar://164002569
(cherry picked from commit 7f55f26)

Author: Michael137

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

@@ -115,6 +115,7 @@ class ClangExpressionParser::LLDBPreprocessorCallbacks : public PPCallbacks {
   ClangModulesDeclVendor &m_decl_vendor;
   ClangPersistentVariables &m_persistent_vars;
   clang::SourceManager &m_source_mgr;
+  /// Accumulates error messages across all moduleImport calls.
   StreamString m_error_stream;
   bool m_has_errors = false;
 
@@ -140,11 +141,12 @@ class ClangExpressionParser::LLDBPreprocessorCallbacks : public PPCallbacks {
       module.path.push_back(
           ConstString(component.getIdentifierInfo()->getName()));
 
-    StreamString error_stream;
-
     ClangModulesDeclVendor::ModuleVector exported_modules;
-    if (!m_decl_vendor.AddModule(module, &exported_modules, m_error_stream))
+    if (auto err = m_decl_vendor.AddModule(module, &exported_modules)) {
       m_has_errors = true;
+      m_error_stream.PutCString(llvm::toString(std::move(err)));
+      m_error_stream.PutChar('\n');
+    }
 
     for (ClangModulesDeclVendor::ModuleID module : exported_modules)
       m_persistent_vars.AddHandLoadedClangModule(module);

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

@@ -383,10 +383,11 @@ bool ClangExpressionSourceCode::GetText(
             block->CalculateSymbolContext(&sc);
 
             if (sc.comp_unit) {
-              StreamString error_stream;
-
-              decl_vendor->AddModulesForCompileUnit(
-                  *sc.comp_unit, modules_for_macros, error_stream);
+              if (auto err = decl_vendor->AddModulesForCompileUnit(
+                      *sc.comp_unit, modules_for_macros))
+                LLDB_LOG_ERROR(
+                    GetLog(LLDBLog::Expressions), std::move(err),
+                    "Error while loading hand-imported modules: {0}");
             }
           }
         }

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

@@ -92,11 +92,11 @@ class ClangModulesDeclVendorImpl : public ClangModulesDeclVendor {
 
   ~ClangModulesDeclVendorImpl() override = default;
 
-  bool AddModule(const SourceModule &module, ModuleVector *exported_modules,
-                 Stream &error_stream) override;
+  llvm::Error AddModule(const SourceModule &module,
+                        ModuleVector *exported_modules) override;
 
-  bool AddModulesForCompileUnit(CompileUnit &cu, ModuleVector &exported_modules,
-                                Stream &error_stream) override;
+  llvm::Error AddModulesForCompileUnit(CompileUnit &cu,
+                                       ModuleVector &exported_modules) override;
 
   uint32_t FindDecls(ConstString name, bool append, uint32_t max_matches,
                      std::vector<CompilerDecl> &decls) override;
@@ -273,16 +273,14 @@ void ClangModulesDeclVendorImpl::ReportModuleExports(
     exports.push_back(module);
 }
 
-bool ClangModulesDeclVendorImpl::AddModule(const SourceModule &module,
-                                           ModuleVector *exported_modules,
-                                           Stream &error_stream) {
+llvm::Error
+ClangModulesDeclVendorImpl::AddModule(const SourceModule &module,
+                                      ModuleVector *exported_modules) {
   // Fail early.
 
-  if (m_compiler_instance->hadModuleLoaderFatalFailure()) {
-    error_stream.PutCString("error: Couldn't load a module because the module "
-                            "loader is in a fatal state.\n");
-    return false;
-  }
+  if (m_compiler_instance->hadModuleLoaderFatalFailure())
+    return llvm::createStringError(
+        "couldn't load a module because the module loader is in a fatal state");
 
   // Check if we've already imported this module.
 
@@ -297,7 +295,7 @@ bool ClangModulesDeclVendorImpl::AddModule(const SourceModule &module,
     if (mi != m_imported_modules.end()) {
       if (exported_modules)
         ReportModuleExports(*exported_modules, mi->second);
-      return true;
+      return llvm::Error::success();
     }
   }
 
@@ -315,30 +313,30 @@ bool ClangModulesDeclVendorImpl::AddModule(const SourceModule &module,
                             std::equal(sysroot_begin, sysroot_end, path_begin);
     // No need to inject search paths to modules in the sysroot.
     if (!is_system_module) {
-      auto error = [&]() {
-        error_stream.Printf("error: No module map file in %s\n",
-                            module.search_path.AsCString());
-        return false;
-      };
-
       bool is_system = true;
       bool is_framework = false;
       auto dir = HS.getFileMgr().getOptionalDirectoryRef(
           module.search_path.GetStringRef());
       if (!dir)
-        return error();
+        return llvm::createStringError(
+            "couldn't find module search path directory %s",
+            module.search_path.GetCString());
+
       auto file = HS.lookupModuleMapFile(*dir, is_framework);
       if (!file)
-        return error();
+        return llvm::createStringError("couldn't find modulemap file in %s",
+                                       module.search_path.GetCString());
+
       if (HS.loadModuleMapFile(*file, is_system))
-        return error();
+        return llvm::createStringError(
+            "failed to parse and load modulemap file in %s",
+            module.search_path.GetCString());
     }
   }
-  if (!HS.lookupModule(module.path.front().GetStringRef())) {
-    error_stream.Printf("error: Header search couldn't locate module '%s'\n",
-                        module.path.front().AsCString());
-    return false;
-  }
+
+  if (!HS.lookupModule(module.path.front().GetStringRef()))
+    return llvm::createStringError("header search couldn't locate module '%s'",
+                                   module.path.front().AsCString());
 
   llvm::SmallVector<clang::IdentifierLoc, 4> clang_path;
 
@@ -364,22 +362,29 @@ bool ClangModulesDeclVendorImpl::AddModule(const SourceModule &module,
   clang::Module *top_level_module = DoGetModule(clang_path.front(), false);
 
   if (!top_level_module) {
+    lldb_private::StreamString error_stream;
     diagnostic_consumer->DumpDiagnostics(error_stream);
-    error_stream.Printf("error: Couldn't load top-level module %s\n",
-                        module.path.front().AsCString());
-    return false;
+
+    return llvm::createStringError(llvm::formatv(
+        "couldn't load top-level module {0}:\n{1}",
+        module.path.front().GetStringRef(), error_stream.GetString()));
   }
 
   clang::Module *submodule = top_level_module;
 
   for (auto &component : llvm::ArrayRef<ConstString>(module.path).drop_front()) {
-    submodule = submodule->findSubmodule(component.GetStringRef());
-    if (!submodule) {
+    clang::Module *found = submodule->findSubmodule(component.GetStringRef());
+    if (!found) {
+      lldb_private::StreamString error_stream;
       diagnostic_consumer->DumpDiagnostics(error_stream);
-      error_stream.Printf("error: Couldn't load submodule %s\n",
-                          component.GetCString());
-      return false;
+
+      return llvm::createStringError(llvm::formatv(
+          "couldn't load submodule '{0}' of module '{1}':\n{2}",
+          component.GetStringRef(), submodule->getFullModuleName(),
+          error_stream.GetString()));
     }
+
+    submodule = found;
   }
 
   // If we didn't make the submodule visible here, Clang wouldn't allow LLDB to
@@ -399,10 +404,12 @@ bool ClangModulesDeclVendorImpl::AddModule(const SourceModule &module,
 
     m_enabled = true;
 
-    return true;
+    return llvm::Error::success();
   }
 
-  return false;
+  return llvm::createStringError(
+      llvm::formatv("unknown error while loading module {0}\n",
+                    module.path.front().GetStringRef()));
 }
 
 bool ClangModulesDeclVendor::LanguageSupportsClangModules(
@@ -424,15 +431,18 @@ bool ClangModulesDeclVendor::LanguageSupportsClangModules(
   }
 }
 
-bool ClangModulesDeclVendorImpl::AddModulesForCompileUnit(
-    CompileUnit &cu, ClangModulesDeclVendor::ModuleVector &exported_modules,
-    Stream &error_stream) {
-  if (LanguageSupportsClangModules(cu.GetLanguage())) {
-    for (auto &imported_module : cu.GetImportedModules())
-      if (!AddModule(imported_module, &exported_modules, error_stream))
-        return false;
-  }
-  return true;
+llvm::Error ClangModulesDeclVendorImpl::AddModulesForCompileUnit(
+    CompileUnit &cu, ClangModulesDeclVendor::ModuleVector &exported_modules) {
+  if (!LanguageSupportsClangModules(cu.GetLanguage()))
+    return llvm::Error::success();
+
+  for (auto &imported_module : cu.GetImportedModules())
+    // TODO: don't short-circuit. Continue loading modules even if one of them
+    // fails. Concatenate all the errors.
+    if (auto err = AddModule(imported_module, &exported_modules))
+      return err;
+
+  return llvm::Error::success();
 }
 
 // ClangImporter::lookupValue

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

@@ -46,17 +46,12 @@ class ClangModulesDeclVendor : public ClangDeclVendor {
   ///     If non-NULL, a pointer to a vector to populate with the ID of every
   ///     module that is re-exported by the specified module.
   ///
-  /// \param[out] error_stream
-  ///     A stream to populate with the output of the Clang parser when
-  ///     it tries to load the module.
-  ///
   /// \return
   ///     True if the module could be loaded; false if not.  If the
   ///     compiler encountered a fatal error during a previous module
   ///     load, then this will always return false for this ModuleImporter.
-  virtual bool AddModule(const SourceModule &module,
-                         ModuleVector *exported_modules,
-                         Stream &error_stream) = 0;
+  virtual llvm::Error AddModule(const SourceModule &module,
+                                ModuleVector *exported_modules) = 0;
 
   /// Add all modules referred to in a given compilation unit to the list
   /// of modules to search.
@@ -68,18 +63,13 @@ class ClangModulesDeclVendor : public ClangDeclVendor {
   ///     A vector to populate with the ID of each module loaded (directly
   ///     and via re-exports) in this way.
   ///
-  /// \param[out] error_stream
-  ///     A stream to populate with the output of the Clang parser when
-  ///     it tries to load the modules.
-  ///
   /// \return
   ///     True if all modules referred to by the compilation unit could be
   ///     loaded; false if one could not be loaded.  If the compiler
   ///     encountered a fatal error during a previous module
   ///     load, then this will always return false for this ModuleImporter.
-  virtual bool AddModulesForCompileUnit(CompileUnit &cu,
-                                        ModuleVector &exported_modules,
-                                        Stream &error_stream) = 0;
+  virtual llvm::Error
+  AddModulesForCompileUnit(CompileUnit &cu, ModuleVector &exported_modules) = 0;
 
   /// Enumerate all the macros that are defined by a given set of modules
   /// that are already imported.

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

@@ -371,26 +371,18 @@ static void SetupDeclVendor(ExecutionContext &exe_ctx, Target *target,
 
   if (!sc.comp_unit)
     return;
-  StreamString error_stream;
-
   ClangModulesDeclVendor::ModuleVector modules_for_macros =
       persistent_state->GetHandLoadedClangModules();
-  if (decl_vendor->AddModulesForCompileUnit(*sc.comp_unit, modules_for_macros,
-                                            error_stream))
-    return;
 
-  // Failed to load some modules, so emit the error stream as a diagnostic.
-  if (!error_stream.Empty()) {
-    // The error stream already contains several Clang diagnostics that might
-    // be either errors or warnings, so just print them all as one remark
-    // diagnostic to prevent that the message starts with "error: error:".
-    diagnostic_manager.PutString(lldb::eSeverityInfo, error_stream.GetString());
+  auto err =
+      decl_vendor->AddModulesForCompileUnit(*sc.comp_unit, modules_for_macros);
+  if (!err)
     return;
-  }
 
-  diagnostic_manager.PutString(lldb::eSeverityError,
-                               "Unknown error while loading modules needed for "
-                               "current compilation unit.");
+  // FIXME: should we be dumping these to the error log instead of as
+  // diagnostics? They are non-fatal and are usually quite noisy.
+  diagnostic_manager.PutString(lldb::eSeverityInfo,
+                               llvm::toString(std::move(err)));
 }
 
 ClangExpressionSourceCode::WrapKind ClangUserExpression::GetWrapKind() const {

lldb/test/Shell/Expr/TestClangModuleLoadError_FromExpression.test

@@ -0,0 +1,54 @@
+## Tests the case where we fail to import modules from @import
+## statements that are part of the expression being run.
+#
+# REQUIRES: system-darwin
+#
+# RUN: split-file %s %t/sources
+# RUN: %clang_host -g %t/sources/main.m -fmodules -fcxx-modules \
+# RUN:             -fmodule-map-file=%t/sources/module.modulemap \
+# RUN:             -fmodules-cache-path=%t/ModuleCache -o %t.out
+#
+# RUN: sed -i '' -e 's/foo\.h/baz\.h/' %t/sources/module.modulemap
+#
+# RUN: %lldb -x -o "settings set interpreter.stop-command-source-on-error false" \
+# RUN:       -s %t/sources/commands.input %t.out -o exit 2>&1 | FileCheck %s
+
+#--- main.m
+@import foo;
+@import bar;
+
+int main() { __builtin_debugtrap(); }
+
+#--- foo.h
+struct foo {};
+
+#--- bar.h
+struct bar {};
+
+#--- module.modulemap
+module foo {
+    header "foo.h"
+    export *
+}
+
+module bar {
+    header "bar.h"
+    export *
+}
+
+#--- commands.input
+run
+## Make sure expression fails so the 'note' diagnostics get printed.
+expr @import Foo; @import Bar
+expr @import foo
+
+# CHECK: error: while importing modules:
+# CHECK-NEXT: header search couldn't locate module 'Foo'
+# CHECK-NEXT: header search couldn't locate module 'Bar'
+#
+# CHECK: expr @import foo
+# CHECK: error: while importing modules:
+# CHECK-NEXT: couldn't load top-level module foo
+## No mention of the previous import errors.
+# CHECK-NOT: Foo
+# CHECK-NOT: Bar