Skip to content

Commit 7f55f26

Browse files
Michael137Michael137
authored
[lldb][ClangModulesDeclVendor] Revamp error handling when loading Clang modules (#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
1 parent 397701f commit 7f55f26

13 files changed

+471
-82
lines changed

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,7 @@ class ClangExpressionParser::LLDBPreprocessorCallbacks : public PPCallbacks {
115115
ClangModulesDeclVendor &m_decl_vendor;
116116
ClangPersistentVariables &m_persistent_vars;
117117
clang::SourceManager &m_source_mgr;
118+
/// Accumulates error messages across all moduleImport calls.
118119
StreamString m_error_stream;
119120
bool m_has_errors = false;
120121

@@ -140,11 +141,12 @@ class ClangExpressionParser::LLDBPreprocessorCallbacks : public PPCallbacks {
140141
module.path.push_back(
141142
ConstString(component.getIdentifierInfo()->getName()));
142143

143-
StreamString error_stream;
144-
145144
ClangModulesDeclVendor::ModuleVector exported_modules;
146-
if (!m_decl_vendor.AddModule(module, &exported_modules, m_error_stream))
145+
if (auto err = m_decl_vendor.AddModule(module, &exported_modules)) {
147146
m_has_errors = true;
147+
m_error_stream.PutCString(llvm::toString(std::move(err)));
148+
m_error_stream.PutChar('\n');
149+
}
148150

149151
for (ClangModulesDeclVendor::ModuleID module : exported_modules)
150152
m_persistent_vars.AddHandLoadedClangModule(module);

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -383,10 +383,11 @@ bool ClangExpressionSourceCode::GetText(
383383
block->CalculateSymbolContext(&sc);
384384

385385
if (sc.comp_unit) {
386-
StreamString error_stream;
387-
388-
decl_vendor->AddModulesForCompileUnit(
389-
*sc.comp_unit, modules_for_macros, error_stream);
386+
if (auto err = decl_vendor->AddModulesForCompileUnit(
387+
*sc.comp_unit, modules_for_macros))
388+
LLDB_LOG_ERROR(
389+
GetLog(LLDBLog::Expressions), std::move(err),
390+
"Error while loading hand-imported modules: {0}");
390391
}
391392
}
392393
}

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

Lines changed: 56 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -92,11 +92,11 @@ class ClangModulesDeclVendorImpl : public ClangModulesDeclVendor {
9292

9393
~ClangModulesDeclVendorImpl() override = default;
9494

95-
bool AddModule(const SourceModule &module, ModuleVector *exported_modules,
96-
Stream &error_stream) override;
95+
llvm::Error AddModule(const SourceModule &module,
96+
ModuleVector *exported_modules) override;
9797

98-
bool AddModulesForCompileUnit(CompileUnit &cu, ModuleVector &exported_modules,
99-
Stream &error_stream) override;
98+
llvm::Error AddModulesForCompileUnit(CompileUnit &cu,
99+
ModuleVector &exported_modules) override;
100100

101101
uint32_t FindDecls(ConstString name, bool append, uint32_t max_matches,
102102
std::vector<CompilerDecl> &decls) override;
@@ -273,16 +273,14 @@ void ClangModulesDeclVendorImpl::ReportModuleExports(
273273
exports.push_back(module);
274274
}
275275

276-
bool ClangModulesDeclVendorImpl::AddModule(const SourceModule &module,
277-
ModuleVector *exported_modules,
278-
Stream &error_stream) {
276+
llvm::Error
277+
ClangModulesDeclVendorImpl::AddModule(const SourceModule &module,
278+
ModuleVector *exported_modules) {
279279
// Fail early.
280280

281-
if (m_compiler_instance->hadModuleLoaderFatalFailure()) {
282-
error_stream.PutCString("error: Couldn't load a module because the module "
283-
"loader is in a fatal state.\n");
284-
return false;
285-
}
281+
if (m_compiler_instance->hadModuleLoaderFatalFailure())
282+
return llvm::createStringError(
283+
"couldn't load a module because the module loader is in a fatal state");
286284

287285
// Check if we've already imported this module.
288286

@@ -297,7 +295,7 @@ bool ClangModulesDeclVendorImpl::AddModule(const SourceModule &module,
297295
if (mi != m_imported_modules.end()) {
298296
if (exported_modules)
299297
ReportModuleExports(*exported_modules, mi->second);
300-
return true;
298+
return llvm::Error::success();
301299
}
302300
}
303301

@@ -315,30 +313,30 @@ bool ClangModulesDeclVendorImpl::AddModule(const SourceModule &module,
315313
std::equal(sysroot_begin, sysroot_end, path_begin);
316314
// No need to inject search paths to modules in the sysroot.
317315
if (!is_system_module) {
318-
auto error = [&]() {
319-
error_stream.Printf("error: No module map file in %s\n",
320-
module.search_path.AsCString());
321-
return false;
322-
};
323-
324316
bool is_system = true;
325317
bool is_framework = false;
326318
auto dir = HS.getFileMgr().getOptionalDirectoryRef(
327319
module.search_path.GetStringRef());
328320
if (!dir)
329-
return error();
321+
return llvm::createStringError(
322+
"couldn't find module search path directory %s",
323+
module.search_path.GetCString());
324+
330325
auto file = HS.lookupModuleMapFile(*dir, is_framework);
331326
if (!file)
332-
return error();
327+
return llvm::createStringError("couldn't find modulemap file in %s",
328+
module.search_path.GetCString());
329+
333330
if (HS.parseAndLoadModuleMapFile(*file, is_system))
334-
return error();
331+
return llvm::createStringError(
332+
"failed to parse and load modulemap file in %s",
333+
module.search_path.GetCString());
335334
}
336335
}
337-
if (!HS.lookupModule(module.path.front().GetStringRef())) {
338-
error_stream.Printf("error: Header search couldn't locate module '%s'\n",
339-
module.path.front().AsCString());
340-
return false;
341-
}
336+
337+
if (!HS.lookupModule(module.path.front().GetStringRef()))
338+
return llvm::createStringError("header search couldn't locate module '%s'",
339+
module.path.front().AsCString());
342340

343341
llvm::SmallVector<clang::IdentifierLoc, 4> clang_path;
344342

@@ -364,22 +362,29 @@ bool ClangModulesDeclVendorImpl::AddModule(const SourceModule &module,
364362
clang::Module *top_level_module = DoGetModule(clang_path.front(), false);
365363

366364
if (!top_level_module) {
365+
lldb_private::StreamString error_stream;
367366
diagnostic_consumer->DumpDiagnostics(error_stream);
368-
error_stream.Printf("error: Couldn't load top-level module %s\n",
369-
module.path.front().AsCString());
370-
return false;
367+
368+
return llvm::createStringError(llvm::formatv(
369+
"couldn't load top-level module {0}:\n{1}",
370+
module.path.front().GetStringRef(), error_stream.GetString()));
371371
}
372372

373373
clang::Module *submodule = top_level_module;
374374

375375
for (auto &component : llvm::ArrayRef<ConstString>(module.path).drop_front()) {
376-
submodule = submodule->findSubmodule(component.GetStringRef());
377-
if (!submodule) {
376+
clang::Module *found = submodule->findSubmodule(component.GetStringRef());
377+
if (!found) {
378+
lldb_private::StreamString error_stream;
378379
diagnostic_consumer->DumpDiagnostics(error_stream);
379-
error_stream.Printf("error: Couldn't load submodule %s\n",
380-
component.GetCString());
381-
return false;
380+
381+
return llvm::createStringError(llvm::formatv(
382+
"couldn't load submodule '{0}' of module '{1}':\n{2}",
383+
component.GetStringRef(), submodule->getFullModuleName(),
384+
error_stream.GetString()));
382385
}
386+
387+
submodule = found;
383388
}
384389

385390
// 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,
399404

400405
m_enabled = true;
401406

402-
return true;
407+
return llvm::Error::success();
403408
}
404409

405-
return false;
410+
return llvm::createStringError(
411+
llvm::formatv("unknown error while loading module {0}\n",
412+
module.path.front().GetStringRef()));
406413
}
407414

408415
bool ClangModulesDeclVendor::LanguageSupportsClangModules(
@@ -424,15 +431,18 @@ bool ClangModulesDeclVendor::LanguageSupportsClangModules(
424431
}
425432
}
426433

427-
bool ClangModulesDeclVendorImpl::AddModulesForCompileUnit(
428-
CompileUnit &cu, ClangModulesDeclVendor::ModuleVector &exported_modules,
429-
Stream &error_stream) {
430-
if (LanguageSupportsClangModules(cu.GetLanguage())) {
431-
for (auto &imported_module : cu.GetImportedModules())
432-
if (!AddModule(imported_module, &exported_modules, error_stream))
433-
return false;
434-
}
435-
return true;
434+
llvm::Error ClangModulesDeclVendorImpl::AddModulesForCompileUnit(
435+
CompileUnit &cu, ClangModulesDeclVendor::ModuleVector &exported_modules) {
436+
if (!LanguageSupportsClangModules(cu.GetLanguage()))
437+
return llvm::Error::success();
438+
439+
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.
442+
if (auto err = AddModule(imported_module, &exported_modules))
443+
return err;
444+
445+
return llvm::Error::success();
436446
}
437447

438448
// ClangImporter::lookupValue

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

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -45,17 +45,12 @@ class ClangModulesDeclVendor : public DeclVendor {
4545
/// If non-NULL, a pointer to a vector to populate with the ID of every
4646
/// module that is re-exported by the specified module.
4747
///
48-
/// \param[out] error_stream
49-
/// A stream to populate with the output of the Clang parser when
50-
/// it tries to load the module.
51-
///
5248
/// \return
5349
/// True if the module could be loaded; false if not. If the
5450
/// compiler encountered a fatal error during a previous module
5551
/// load, then this will always return false for this ModuleImporter.
56-
virtual bool AddModule(const SourceModule &module,
57-
ModuleVector *exported_modules,
58-
Stream &error_stream) = 0;
52+
virtual llvm::Error AddModule(const SourceModule &module,
53+
ModuleVector *exported_modules) = 0;
5954

6055
/// Add all modules referred to in a given compilation unit to the list
6156
/// of modules to search.
@@ -67,18 +62,13 @@ class ClangModulesDeclVendor : public DeclVendor {
6762
/// A vector to populate with the ID of each module loaded (directly
6863
/// and via re-exports) in this way.
6964
///
70-
/// \param[out] error_stream
71-
/// A stream to populate with the output of the Clang parser when
72-
/// it tries to load the modules.
73-
///
7465
/// \return
7566
/// True if all modules referred to by the compilation unit could be
7667
/// loaded; false if one could not be loaded. If the compiler
7768
/// encountered a fatal error during a previous module
7869
/// load, then this will always return false for this ModuleImporter.
79-
virtual bool AddModulesForCompileUnit(CompileUnit &cu,
80-
ModuleVector &exported_modules,
81-
Stream &error_stream) = 0;
70+
virtual llvm::Error
71+
AddModulesForCompileUnit(CompileUnit &cu, ModuleVector &exported_modules) = 0;
8272

8373
/// Enumerate all the macros that are defined by a given set of modules
8474
/// that are already imported.

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

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -371,26 +371,18 @@ static void SetupDeclVendor(ExecutionContext &exe_ctx, Target *target,
371371

372372
if (!sc.comp_unit)
373373
return;
374-
StreamString error_stream;
375-
376374
ClangModulesDeclVendor::ModuleVector modules_for_macros =
377375
persistent_state->GetHandLoadedClangModules();
378-
if (decl_vendor->AddModulesForCompileUnit(*sc.comp_unit, modules_for_macros,
379-
error_stream))
380-
return;
381376

382-
// Failed to load some modules, so emit the error stream as a diagnostic.
383-
if (!error_stream.Empty()) {
384-
// The error stream already contains several Clang diagnostics that might
385-
// be either errors or warnings, so just print them all as one remark
386-
// diagnostic to prevent that the message starts with "error: error:".
387-
diagnostic_manager.PutString(lldb::eSeverityInfo, error_stream.GetString());
377+
auto err =
378+
decl_vendor->AddModulesForCompileUnit(*sc.comp_unit, modules_for_macros);
379+
if (!err)
388380
return;
389-
}
390381

391-
diagnostic_manager.PutString(lldb::eSeverityError,
392-
"Unknown error while loading modules needed for "
393-
"current compilation unit.");
382+
// FIXME: should we be dumping these to the error log instead of as
383+
// diagnostics? They are non-fatal and are usually quite noisy.
384+
diagnostic_manager.PutString(lldb::eSeverityInfo,
385+
llvm::toString(std::move(err)));
394386
}
395387

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

‎lldb/test/Shell/Expr/TestClangModuleLoadError_FromExpression.test‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
## Tests the case where we fail to import modules from @import
2+
## statements that are part of the expression being run.
3+
#
4+
# REQUIRES: system-darwin
5+
#
6+
# RUN: split-file %s %t/sources
7+
# RUN: %clang_host -g %t/sources/main.m -fmodules -fcxx-modules \
8+
# RUN: -fmodule-map-file=%t/sources/module.modulemap \
9+
# RUN: -fmodules-cache-path=%t/ModuleCache -o %t.out
10+
#
11+
# RUN: sed -i '' -e 's/foo\.h/baz\.h/' %t/sources/module.modulemap
12+
#
13+
# RUN: %lldb -x -o "settings set interpreter.stop-command-source-on-error false" \
14+
# RUN: -s %t/sources/commands.input %t.out -o exit 2>&1 | FileCheck %s
15+
16+
#--- main.m
17+
@import foo;
18+
@import bar;
19+
20+
int main() { __builtin_debugtrap(); }
21+
22+
#--- foo.h
23+
struct foo {};
24+
25+
#--- bar.h
26+
struct bar {};
27+
28+
#--- module.modulemap
29+
module foo {
30+
header "foo.h"
31+
export *
32+
}
33+
34+
module bar {
35+
header "bar.h"
36+
export *
37+
}
38+
39+
#--- commands.input
40+
run
41+
## Make sure expression fails so the 'note' diagnostics get printed.
42+
expr @import Foo; @import Bar
43+
expr @import foo
44+
45+
# CHECK: error: while importing modules:
46+
# CHECK-NEXT: header search couldn't locate module 'Foo'
47+
# CHECK-NEXT: header search couldn't locate module 'Bar'
48+
#
49+
# CHECK: expr @import foo
50+
# CHECK: error: while importing modules:
51+
# CHECK-NEXT: couldn't load top-level module foo
52+
## No mention of the previous import errors.
53+
# CHECK-NOT: Foo
54+
# CHECK-NOT: Bar

0 commit comments

Comments
 (0)