Skip to content

Commit 74e34ef

Browse files
authored
[lldb][ClangModulesDeclVendor] Don't stop loading Clang modules if an 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.
1 parent bbc4a45 commit 74e34ef

8 files changed

+16
-17
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -387,7 +387,7 @@ bool ClangExpressionSourceCode::GetText(
387387
*sc.comp_unit, modules_for_macros))
388388
LLDB_LOG_ERROR(
389389
GetLog(LLDBLog::Expressions), std::move(err),
390-
"Error while loading hand-imported modules: {0}");
390+
"Error while loading hand-imported modules:\n{0}");
391391
}
392392
}
393393
}

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -436,13 +436,13 @@ llvm::Error ClangModulesDeclVendorImpl::AddModulesForCompileUnit(
436436
if (!LanguageSupportsClangModules(cu.GetLanguage()))
437437
return llvm::Error::success();
438438

439+
llvm::Error errors = llvm::Error::success();
440+
439441
for (auto &imported_module : cu.GetImportedModules())
440-
// TODO: don't short-circuit. Continue loading modules even if one of them
441-
// fails. Concatenate all the errors.
442442
if (auto err = AddModule(imported_module, &exported_modules))
443-
return err;
443+
errors = llvm::joinErrors(std::move(errors), std::move(err));
444444

445-
return llvm::Error::success();
445+
return errors;
446446
}
447447

448448
// ClangImporter::lookupValue

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -381,8 +381,10 @@ static void SetupDeclVendor(ExecutionContext &exe_ctx, Target *target,
381381

382382
// FIXME: should we be dumping these to the error log instead of as
383383
// diagnostics? They are non-fatal and are usually quite noisy.
384-
diagnostic_manager.PutString(lldb::eSeverityInfo,
385-
llvm::toString(std::move(err)));
384+
llvm::handleAllErrors(
385+
std::move(err), [&diagnostic_manager](const llvm::StringError &e) {
386+
diagnostic_manager.PutString(lldb::eSeverityInfo, e.getMessage());
387+
});
386388
}
387389

388390
ClangExpressionSourceCode::WrapKind ClangUserExpression::GetWrapKind() const {

lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSearchPath.test

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,5 +43,4 @@ run
4343
expr blah
4444

4545
# CHECK: note: couldn't find module search path directory {{.*}}sources
46-
## FIXME: We never attempted to load bar.
47-
# CHECK-NOT: couldn't find module search path
46+
# CHECK: note: couldn't find module search path directory {{.*}}sources

lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidTopLevelModule.test

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,5 +44,6 @@ expr blah
4444
# CHECK: note: couldn't load top-level module foo
4545
## FIXME: clang error diagnostic shouldn't be dumped to the console.
4646
# CHECK: error:
47-
## FIXME: We never attempted to load bar.
48-
# CHECK-NOT: bar
47+
# CHECK: note: couldn't load top-level module bar
48+
## FIXME: clang error diagnostic shouldn't be dumped to the console.
49+
# CHECK: error:

lldb/test/Shell/Expr/TestClangModuleLoadError_ModulemapParsing.test

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,5 +42,4 @@ run
4242
expr blah
4343

4444
# CHECK: note: failed to parse and load modulemap file in {{.*}}sources
45-
## FIXME: We never attempted to load bar.
46-
# CHECK-NOT: failed to parse and load modulemap
45+
# CHECK: note: failed to parse and load modulemap file in {{.*}}sources

lldb/test/Shell/Expr/TestClangModuleLoadError_NoModule.test

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,5 +43,4 @@ run
4343
expr blah
4444

4545
# CHECK: note: header search couldn't locate module 'foo'
46-
## FIXME: We never attempted to load bar.
47-
# CHECK-NOT: bar
46+
# CHECK: note: header search couldn't locate module 'bar'

lldb/test/Shell/Expr/TestClangModuleLoadError_NoModuleMap.test

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,5 +38,4 @@ run
3838
expr blah
3939

4040
# CHECK: note: couldn't find modulemap file in {{.*}}sources
41-
## FIXME: We never attempted to load bar.
42-
# CHECK-NOT: couldn't find modulemap
41+
# CHECK: note: couldn't find modulemap file in {{.*}}sources

0 commit comments

Comments
 (0)