llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) <details> <summary>Changes</summary> Depends on: * https://github.com/llvm/llvm-project/pull/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 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. --- Patch is 25.86 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/166940.diff 13 Files Affected: - (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp (+5-3) - (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp (+5-4) - (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp (+56-46) - (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h (+4-14) - (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp (+9-15) - (added) lldb/test/Shell/Expr/TestClangModuleLoadError_FromExpression.test (+54) - (added) lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidNestedSubmodule.test (+59) - (added) lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSearchPath.test (+46) - (added) lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSubmodule.test (+51) - (added) lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidTopLevelModule.test (+49) - (added) lldb/test/Shell/Expr/TestClangModuleLoadError_ModulemapParsing.test (+45) - (added) lldb/test/Shell/Expr/TestClangModuleLoadError_NoModule.test (+46) - (added) lldb/test/Shell/Expr/TestClangModuleLoadError_NoModuleMap.test (+41) ``````````diff 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..7635e49051216 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(); + + llvm::Error errors = llvm::Error::success(); + + for (auto &imported_module : cu.GetImportedModules()) + if (auto err = AddModule(imported_module, &exported_modules)) + errors = llvm::joinErrors(std::move(errors), std::move(err)); + + return errors; } // 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..9a5d49a7ea363 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp @@ -371,26 +371,20 @@ 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. + llvm::handleAllErrors( + std::move(err), [&diagnostic_manager](const llvm::StringError &e) { + diagnostic_manager.PutString(lldb::eSeverityInfo, e.getMessage()); + }); } 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..a5822ae4a75e7 --- /dev/null +++ b/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidNestedSubmodule.test @@ -0,0 +1,59 @@ +## 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' diff --git a/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSearchPath.test b/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSearchPath.test new file mode 100644 index 0000000000000..e5a6334d2ee22 --- /dev/null +++ b/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSearchPath.test @@ -0,0 +1,46 @@ +## 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 +# CHECK: note: couldn't find module search path directory {{.*}}sources diff --git a/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSubmodule.test b/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSubmodule.test new file mode 100644 index 0000000000000..2552cdb4b61db --- /dev/null +++ b/lldb/test/Shell/Expr/TestClangModuleLoadError_InvalidSubmodule.test @@ -0,0 +1,51 @@ +## 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' diff --git a/lldb/test/Shell/Expr... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/166940 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
