[lldb][ClangModulesDeclVendor] Revamp error handling when loading Clang modules #166917
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ng modules 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
Member
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesInstead of propagating the errors as a See the new tests for what the errors look like. rdar://164002569 Patch is 25.73 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/166917.diff 13 Files Affected:
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 6bab880b4d521..75ed87baf636a 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/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);
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
index ff9ed9c27f70f..b4e81aa21c138 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ b/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}");
}
}
}
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
index b77e2690deb06..6c5243b853ddf 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ b/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.parseAndLoadModuleMapFile(*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
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h
index debf4761175b8..043632007b7d3 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h
@@ -45,17 +45,12 @@ class ClangModulesDeclVendor : public DeclVendor {
/// 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.
@@ -67,18 +62,13 @@ class ClangModulesDeclVendor : public DeclVendor {
/// 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.
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
index e8d5ec3c7fd96..13d32b3bbc4f3 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ b/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 {
diff --git a/lldb/test/Shell/Expr/TestClangModuleLoadError_FromExpression.test b/lldb/test/Shell/Expr/TestClangModuleLoadError_FromExpression.test
new file mode 100644
index 0000000000000..b964e9b27e914
--- /dev/null
+++ b/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
diff --git a/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidNestedSubmodule.test b/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidNestedSubmodule.test
new file mode 100644
index 0000000000000..7bd0bea8b0b9e
--- /dev/null
+++ b/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidNestedSubmodule.test
@@ -0,0 +1,61 @@
+## Tests the case where we fail to load a submodule of a submodule. We force this
+## by removing the submodule 'module qux' of 'module baz' from the modulemap.
+#
+# 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/module qux/module quz/' %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.baz.qux;
+@import bar;
+
+int main() { __builtin_debugtrap(); }
+
+#--- foo.h
+struct foo {};
+
+#--- bar.h
+struct bar {};
+
+#--- baz.h
+struct baz {};
+
+#--- qux.h
+struct qux {};
+
+#--- module.modulemap
+module foo {
+ header "foo.h"
+ export *
+
+ module baz {
+ header "baz.h"
+ export *
+
+ module qux {
+ header "qux.h"
+ export *
+ }
+ }
+}
+
+module bar {
+ header "bar.h"
+ export *
+}
+
+#--- commands.input
+run
+## Make sure expression fails so the 'note' diagnostics get printed.
+expr blah
+
+# CHECK: note: couldn't load submodule 'qux' of module 'foo.baz'
+## FIXME: We never attempted to load bar.
+# CHECK-NOT: bar
diff --git a/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSearchPath.test b/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSearchPath.test
new file mode 100644
index 0000000000000..818b64f345806
--- /dev/null
+++ b/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSearchPath.test
@@ -0,0 +1,47 @@
+## Tests the case where the DW_AT_LLVM_include_path of the module is invalid.
+## We forces this by just removing that directory (which in our case is 'sources').
+#
+# 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: cp %t/sources/commands.input %t/commands.input
+# RUN: rm -r %t/sources
+#
+# RUN: %lldb -x -o "settings set interpreter.stop-command-source-on-error false" \
+# RUN: -s %t/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 blah
+
+# CHECK: note: couldn't find module search path directory {{.*}}sources
+## FIXME: We never attempted to load bar.
+# CHECK-NOT: bar
diff --git a/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSubmodule.test b/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSubmodule.test
new file mode 100644
index 0000000000000..7432c841ff2ee
--- /dev/null
+++ b/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSubmodule.test
@@ -0,0 +1,53 @@
+## Tests the case where we fail to load a submodule. We force this by removing
+## the submodule 'module baz' from the modulemap.
+#
+# 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/module baz/module qux/' %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.baz;
+@import bar;
+
+int main() { __builtin_debugtrap(); }
+
+#--- foo.h
+struct foo {};
+
+#--- bar.h
+struct bar {};
+
+#--- baz.h
+struct baz {};
+
+#--- module.modulemap
+module foo {
+ header "foo.h"
+ export *
+
+ module baz {
+ header "baz.h"
+ export *
+ }
+}
+
+module bar {
+ header "bar.h"
+ export *
+}
+
+#--- commands.input
+run
+## Make sure expression fails so the 'note' diagnostics get printed.
+expr blah
+
+# CHECK: note: couldn't load submodule 'baz' of module 'foo' (top-level modu...
[truncated]
|
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Nov 7, 2025
… individual import failed When loading all Clang modules for a CU, we stop on first error. This means benign module loading errors may stop us from importing actually useful modules. There's no good reason to bail on the first one. The pathological would be if we try to load a large number of Clang modules but all fail to load for the same reason. That could happen, but in practice I've always seen only a handful of modules failing to load out of a large number. Particularly system modules are useful and usually don't fail to load. Whereas project-specific Clang modules are more likely to fail because the build system moves the modulemap/sources around. This patch accumulates all module loading errors and doesn't stop when an error is encountered. Depends on: * llvm#166917
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Nov 7, 2025
… individual import failed When loading all Clang modules for a CU, we stop on first error. This means benign module loading errors may stop us from importing actually useful modules. There's no good reason to bail on the first one. The pathological would be if we try to load a large number of Clang modules but all fail to load for the same reason. That could happen, but in practice I've always seen only a handful of modules failing to load out of a large number. Particularly system modules are useful and usually don't fail to load. Whereas project-specific Clang modules are more likely to fail because the build system moves the modulemap/sources around. This patch accumulates all module loading errors and doesn't stop when an error is encountered. Depends on: * llvm#166917
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Nov 7, 2025
… individual import failed When loading all Clang modules for a CU, we stop on first error. This means benign module loading errors may stop us from importing actually useful modules. There's no good reason to bail on the first one. The pathological would be if we try to load a large number of Clang modules but all fail to load for the same reason. That could happen, but in practice I've always seen only a handful of modules failing to load out of a large number. Particularly system modules are useful and usually don't fail to load. Whereas project-specific Clang modules are more likely to fail because the build system moves the modulemap/sources around. This patch accumulates all module loading errors and doesn't stop when an error is encountered. Depends on: * llvm#166917
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Nov 7, 2025
…xpression log instead of console Depends on: * llvm#166917 * llvm#166940 While these errors can contribute to an expression failing, they are never *the* reason the expression failed. I.e., they are always just 'note:' diagnostics that we hand-emit. Because they are quite noisy (and we potentially have many of them if we auto-load all modules in a CU), this patch logs the errors to the `expr` log, instead of the console. Previously these errors would only get omitted when the expression itself failed. Meaning if the expression failed, we'd dump these 'note' module load errors in next to the actual expression error, obscuring the output. Moreover, if the expression succeeded, any module load errors would be dropped. Now we always log all module loading errors to the expression log, regardless of whether the expression fails or not.
adrian-prantl
approved these changes
Nov 7, 2025
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Nov 7, 2025
… individual import failed When loading all Clang modules for a CU, we stop on first error. This means benign module loading errors may stop us from importing actually useful modules. There's no good reason to bail on the first one. The pathological would be if we try to load a large number of Clang modules but all fail to load for the same reason. That could happen, but in practice I've always seen only a handful of modules failing to load out of a large number. Particularly system modules are useful and usually don't fail to load. Whereas project-specific Clang modules are more likely to fail because the build system moves the modulemap/sources around. This patch accumulates all module loading errors and doesn't stop when an error is encountered. Depends on: * llvm#166917
Michael137
added a commit
that referenced
this pull request
Nov 7, 2025
… individual import failed (#166940) Depends on: * #166917 When loading all Clang modules for a CU, we stop on first error. This means benign module loading errors may stop us from importing actually useful modules. There's no good reason to bail on the first one. The pathological case would be if we try to load a large number of Clang modules but all fail to load for the same reason. That could happen, but in practice I've always seen only a handful of modules failing to load out of a large number. Particularly system modules are useful and usually don't fail to load. Whereas project-specific Clang modules are more likely to fail because the build system moves the modulemap/sources around. This patch accumulates all module loading errors and doesn't stop when an error is encountered.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Nov 7, 2025
…dules if an individual import failed (#166940) Depends on: * llvm/llvm-project#166917 When loading all Clang modules for a CU, we stop on first error. This means benign module loading errors may stop us from importing actually useful modules. There's no good reason to bail on the first one. The pathological case would be if we try to load a large number of Clang modules but all fail to load for the same reason. That could happen, but in practice I've always seen only a handful of modules failing to load out of a large number. Particularly system modules are useful and usually don't fail to load. Whereas project-specific Clang modules are more likely to fail because the build system moves the modulemap/sources around. This patch accumulates all module loading errors and doesn't stop when an error is encountered.
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Nov 8, 2025
…xpression log instead of console Depends on: * llvm#166917 * llvm#166940 While these errors can contribute to an expression failing, they are never *the* reason the expression failed. I.e., they are always just 'note:' diagnostics that we hand-emit. Because they are quite noisy (and we potentially have many of them if we auto-load all modules in a CU), this patch logs the errors to the `expr` log, instead of the console. Previously these errors would only get omitted when the expression itself failed. Meaning if the expression failed, we'd dump these 'note' module load errors in next to the actual expression error, obscuring the output. Moreover, if the expression succeeded, any module load errors would be dropped. Now we always log all module loading errors to the expression log, regardless of whether the expression fails or not.
Michael137
added a commit
that referenced
this pull request
Nov 8, 2025
…xpression log instead of console (#166964) Depends on: * #166917 * #166940 While these errors can contribute to an expression failing, they are never *the* reason the expression failed. I.e., they are always just 'note:' diagnostics that we hand-emit. Because they are quite noisy (and we potentially have many of them if we auto-load all modules in a CU), this patch logs the errors to the `expr` log, instead of the console. Previously these errors would only get omitted when the expression itself failed. Meaning if the expression failed, we'd dump these 'note' module load errors in next to the actual expression error, obscuring the output. Moreover, if the expression succeeded, any module load errors would be dropped. Now we always log all module loading errors to the expression log, regardless of whether the expression fails or not.
Michael137
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Nov 8, 2025
…ng 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)
Michael137
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Nov 8, 2025
… individual import failed (llvm#166940) Depends on: * llvm#166917 When loading all Clang modules for a CU, we stop on first error. This means benign module loading errors may stop us from importing actually useful modules. There's no good reason to bail on the first one. The pathological case would be if we try to load a large number of Clang modules but all fail to load for the same reason. That could happen, but in practice I've always seen only a handful of modules failing to load out of a large number. Particularly system modules are useful and usually don't fail to load. Whereas project-specific Clang modules are more likely to fail because the build system moves the modulemap/sources around. This patch accumulates all module loading errors and doesn't stop when an error is encountered. (cherry picked from commit 74e34ef)
Michael137
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Nov 8, 2025
…xpression log instead of console (llvm#166964) Depends on: * llvm#166917 * llvm#166940 While these errors can contribute to an expression failing, they are never *the* reason the expression failed. I.e., they are always just 'note:' diagnostics that we hand-emit. Because they are quite noisy (and we potentially have many of them if we auto-load all modules in a CU), this patch logs the errors to the `expr` log, instead of the console. Previously these errors would only get omitted when the expression itself failed. Meaning if the expression failed, we'd dump these 'note' module load errors in next to the actual expression error, obscuring the output. Moreover, if the expression succeeded, any module load errors would be dropped. Now we always log all module loading errors to the expression log, regardless of whether the expression fails or not. (cherry picked from commit d5b62fa)
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Nov 8, 2025
…errors to expression log instead of console (#166964) Depends on: * llvm/llvm-project#166917 * llvm/llvm-project#166940 While these errors can contribute to an expression failing, they are never *the* reason the expression failed. I.e., they are always just 'note:' diagnostics that we hand-emit. Because they are quite noisy (and we potentially have many of them if we auto-load all modules in a CU), this patch logs the errors to the `expr` log, instead of the console. Previously these errors would only get omitted when the expression itself failed. Meaning if the expression failed, we'd dump these 'note' module load errors in next to the actual expression error, obscuring the output. Moreover, if the expression succeeded, any module load errors would be dropped. Now we always log all module loading errors to the expression log, regardless of whether the expression fails or not.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of propagating the errors as a
bool+Streamwe change theClangModulesDeclVendormodule loading APIs to usellvm::Error. We also reword some of the diagnostics (notably removing the hardcodederror: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