From 463d90986ec54c62bf8fe31193ef5db701db48a5 Mon Sep 17 00:00:00 2001 From: Douglas Gregor Date: Thu, 29 Nov 2012 23:55:25 +0000 Subject: [PATCH] Keep track of modules that have failed to build. If we encounter an import of that module elsewhere, don't try to build the module again: it won't work, and the experience is quite dreadful. We track this information somewhat globally, shared among all of the related CompilerInvocations used to build modules on-the-fly, so that a particular Clang instance will only try to build a given module once. Fixes . git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@168961 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Frontend/ASTUnit.h | 9 +-- include/clang/Frontend/CompilerInstance.h | 11 ++-- include/clang/Lex/ModuleLoader.h | 32 ++++++++-- include/clang/Lex/PreprocessorOptions.h | 23 +++++++ lib/Frontend/CompilerInstance.cpp | 73 +++++++++++++++++------ lib/Lex/PPDirectives.cpp | 9 ++- test/Modules/epic-fail.m | 11 ++++ 7 files changed, 135 insertions(+), 33 deletions(-) create mode 100644 test/Modules/epic-fail.m diff --git a/include/clang/Frontend/ASTUnit.h b/include/clang/Frontend/ASTUnit.h index 5e409bd7ed..ac9a0461b8 100644 --- a/include/clang/Frontend/ASTUnit.h +++ b/include/clang/Frontend/ASTUnit.h @@ -830,11 +830,12 @@ public: /// \returns True if an error occurred, false otherwise. bool serialize(raw_ostream &OS); - virtual Module *loadModule(SourceLocation ImportLoc, ModuleIdPath Path, - Module::NameVisibilityKind Visibility, - bool IsInclusionDirective) { + virtual ModuleLoadResult loadModule(SourceLocation ImportLoc, + ModuleIdPath Path, + Module::NameVisibilityKind Visibility, + bool IsInclusionDirective) { // ASTUnit doesn't know how to load modules (not that this matters). - return 0; + return ModuleLoadResult(); } }; diff --git a/include/clang/Frontend/CompilerInstance.h b/include/clang/Frontend/CompilerInstance.h index 2f3dc3f808..2861511dd1 100644 --- a/include/clang/Frontend/CompilerInstance.h +++ b/include/clang/Frontend/CompilerInstance.h @@ -94,7 +94,7 @@ class CompilerInstance : public ModuleLoader { /// \brief The semantic analysis object. OwningPtr TheSema; - + /// \brief The frontend timer OwningPtr FrontendTimer; @@ -111,7 +111,7 @@ class CompilerInstance : public ModuleLoader { /// \brief The result of the last module import. /// - Module *LastModuleImportResult; + ModuleLoadResult LastModuleImportResult; /// \brief Holds information about the output file. /// @@ -645,9 +645,10 @@ public: /// } - virtual Module *loadModule(SourceLocation ImportLoc, ModuleIdPath Path, - Module::NameVisibilityKind Visibility, - bool IsInclusionDirective); + virtual ModuleLoadResult loadModule(SourceLocation ImportLoc, + ModuleIdPath Path, + Module::NameVisibilityKind Visibility, + bool IsInclusionDirective); }; } // end namespace clang diff --git a/include/clang/Lex/ModuleLoader.h b/include/clang/Lex/ModuleLoader.h index 36d03c0aa2..3a5f41d510 100644 --- a/include/clang/Lex/ModuleLoader.h +++ b/include/clang/Lex/ModuleLoader.h @@ -17,16 +17,37 @@ #include "clang/Basic/Module.h" #include "clang/Basic/SourceLocation.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/PointerIntPair.h" namespace clang { class IdentifierInfo; - +class Module; + /// \brief A sequence of identifier/location pairs used to describe a particular /// module or submodule, e.g., std.vector. typedef llvm::ArrayRef > ModuleIdPath; - + +/// \brief Describes the result of attempting to load a module. +class ModuleLoadResult { + llvm::PointerIntPair Storage; + +public: + ModuleLoadResult() : Storage() { } + + ModuleLoadResult(Module *module, bool missingExpected) + : Storage(module, missingExpected) { } + + operator Module *() const { return Storage.getPointer(); } + + /// \brief Determines whether the module, which failed to load, was + /// actually a submodule that we expected to see (based on implying the + /// submodule from header structure), but didn't materialize in the actual + /// module. + bool isMissingExpected() const { return Storage.getInt(); } +}; + /// \brief Abstract interface for a module loader. /// /// This abstract interface describes a module loader, which is responsible @@ -55,9 +76,10 @@ public: /// /// \returns If successful, returns the loaded module. Otherwise, returns /// NULL to indicate that the module could not be loaded. - virtual Module *loadModule(SourceLocation ImportLoc, ModuleIdPath Path, - Module::NameVisibilityKind Visibility, - bool IsInclusionDirective) = 0; + virtual ModuleLoadResult loadModule(SourceLocation ImportLoc, + ModuleIdPath Path, + Module::NameVisibilityKind Visibility, + bool IsInclusionDirective) = 0; }; } diff --git a/include/clang/Lex/PreprocessorOptions.h b/include/clang/Lex/PreprocessorOptions.h index e5fe373793..857161ddc0 100644 --- a/include/clang/Lex/PreprocessorOptions.h +++ b/include/clang/Lex/PreprocessorOptions.h @@ -13,6 +13,7 @@ #include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" +#include "llvm/ADT/StringSet.h" #include #include #include @@ -126,7 +127,29 @@ public: /// to do so (e.g., if on-demand module construction moves out-of-process), /// we can add a cc1-level option to do so. SmallVector ModuleBuildPath; + + /// \brief Records the set of modules + class FailedModulesSet : public llvm::RefCountedBase { + llvm::StringSet<> Failed; + + public: + bool hasAlreadyFailed(StringRef module) { + return Failed.count(module) > 0; + } + + void addFailed(StringRef module) { + Failed.insert(module); + } + }; + /// \brief The set of modules that failed to build. + /// + /// This pointer will be shared among all of the compiler instances created + /// to (re)build modules, so that once a module fails to build anywhere, + /// other instances will see that the module has failed and won't try to + /// build it again. + llvm::IntrusiveRefCntPtr FailedModules; + typedef std::vector >::iterator remapped_file_iterator; typedef std::vector >::const_iterator diff --git a/lib/Frontend/CompilerInstance.cpp b/lib/Frontend/CompilerInstance.cpp index b01a3f6e1a..f87a635420 100644 --- a/lib/Frontend/CompilerInstance.cpp +++ b/lib/Frontend/CompilerInstance.cpp @@ -801,6 +801,15 @@ static void compileModule(CompilerInstance &ImportingInstance, // can detect cycles in the module graph. PPOpts.ModuleBuildPath.push_back(Module->getTopLevelModuleName()); + // Make sure that the failed-module structure has been allocated in + // the importing instance, and propagate the pointer to the newly-created + // instance. + PreprocessorOptions &ImportingPPOpts + = ImportingInstance.getInvocation().getPreprocessorOpts(); + if (!ImportingPPOpts.FailedModules) + ImportingPPOpts.FailedModules = new PreprocessorOptions::FailedModulesSet; + PPOpts.FailedModules = ImportingPPOpts.FailedModules; + // If there is a module map file, build the module using the module map. // Set up the inputs/outputs so that we build the module from its umbrella // header. @@ -872,10 +881,11 @@ static void compileModule(CompilerInstance &ImportingInstance, llvm::sys::Path(TempModuleMapFileName).eraseFromDisk(); } -Module *CompilerInstance::loadModule(SourceLocation ImportLoc, - ModuleIdPath Path, - Module::NameVisibilityKind Visibility, - bool IsInclusionDirective) { +ModuleLoadResult +CompilerInstance::loadModule(SourceLocation ImportLoc, + ModuleIdPath Path, + Module::NameVisibilityKind Visibility, + bool IsInclusionDirective) { // If we've already handled this import, just return the cached result. // This one-element cache is important to eliminate redundant diagnostics // when both the preprocessor and parser see the same import declaration. @@ -910,14 +920,14 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc, ModuleFileName = PP->getHeaderSearchInfo().getModuleFileName(Module); else ModuleFileName = PP->getHeaderSearchInfo().getModuleFileName(ModuleName); - + if (ModuleFileName.empty()) { getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_found) << ModuleName << SourceRange(ImportLoc, ModuleNameLoc); LastModuleImportLoc = ImportLoc; - LastModuleImportResult = 0; - return 0; + LastModuleImportResult = ModuleLoadResult(); + return LastModuleImportResult; } const FileEntry *ModuleFile @@ -943,7 +953,18 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc, getDiagnostics().Report(ModuleNameLoc, diag::err_module_cycle) << ModuleName << CyclePath; - return 0; + return ModuleLoadResult(); + } + + // Check whether we have already attempted to build this module (but + // failed). + if (getPreprocessorOpts().FailedModules && + getPreprocessorOpts().FailedModules->hasAlreadyFailed(ModuleName)) { + getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_built) + << ModuleName + << SourceRange(ImportLoc, ModuleNameLoc); + + return ModuleLoadResult(); } getDiagnostics().Report(ModuleNameLoc, diag::warn_module_build) @@ -951,6 +972,9 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc, BuildingModule = true; compileModule(*this, Module, ModuleFileName); ModuleFile = FileMgr->getFile(ModuleFileName); + + if (!ModuleFile) + getPreprocessorOpts().FailedModules->addFailed(ModuleName); } if (!ModuleFile) { @@ -959,7 +983,7 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc, : diag::err_module_not_found) << ModuleName << SourceRange(ImportLoc, ModuleNameLoc); - return 0; + return ModuleLoadResult(); } // If we don't already have an ASTReader, create one now. @@ -1004,6 +1028,18 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc, getFileManager().invalidateCache(ModuleFile); bool Existed; llvm::sys::fs::remove(ModuleFileName, Existed); + + // Check whether we have already attempted to build this module (but + // failed). + if (getPreprocessorOpts().FailedModules && + getPreprocessorOpts().FailedModules->hasAlreadyFailed(ModuleName)) { + getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_built) + << ModuleName + << SourceRange(ImportLoc, ModuleNameLoc); + + return ModuleLoadResult(); + } + compileModule(*this, Module, ModuleFileName); // Try loading the module again. @@ -1012,8 +1048,9 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc, ModuleManager->ReadAST(ModuleFileName, serialization::MK_Module, ImportLoc, ASTReader::ARR_None) != ASTReader::Success) { + getPreprocessorOpts().FailedModules->addFailed(ModuleName); KnownModules[Path[0].first] = 0; - return 0; + return ModuleLoadResult(); } // Okay, we've rebuilt and now loaded the module. @@ -1026,12 +1063,12 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc, // FIXME: The ASTReader will already have complained, but can we showhorn // that diagnostic information into a more useful form? KnownModules[Path[0].first] = 0; - return 0; + return ModuleLoadResult(); case ASTReader::Failure: // Already complained, but note now that we failed. KnownModules[Path[0].first] = 0; - return 0; + return ModuleLoadResult(); } if (!Module) { @@ -1050,7 +1087,7 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc, // If we never found the module, fail. if (!Module) - return 0; + return ModuleLoadResult(); // Verify that the rest of the module path actually corresponds to // a submodule. @@ -1120,7 +1157,7 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc, << Module->getFullModuleName() << SourceRange(Path.front().second, Path.back().second); - return 0; + return ModuleLoadResult(0, true); } // Check whether this module is available. @@ -1131,8 +1168,8 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc, << Feature << SourceRange(Path.front().second, Path.back().second); LastModuleImportLoc = ImportLoc; - LastModuleImportResult = 0; - return 0; + LastModuleImportResult = ModuleLoadResult(); + return ModuleLoadResult(); } ModuleManager->makeModuleVisible(Module, Visibility); @@ -1151,6 +1188,6 @@ Module *CompilerInstance::loadModule(SourceLocation ImportLoc, } LastModuleImportLoc = ImportLoc; - LastModuleImportResult = Module; - return Module; + LastModuleImportResult = ModuleLoadResult(Module, false); + return LastModuleImportResult; } diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp index 50b161a172..89490a3f23 100644 --- a/lib/Lex/PPDirectives.cpp +++ b/lib/Lex/PPDirectives.cpp @@ -1483,7 +1483,7 @@ void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc, // If this was an #__include_macros directive, only make macros visible. Module::NameVisibilityKind Visibility = (IncludeKind == 3)? Module::MacrosVisible : Module::AllVisible; - Module *Imported + ModuleLoadResult Imported = TheModuleLoader.loadModule(IncludeTok.getLocation(), Path, Visibility, /*IsIncludeDirective=*/true); assert((Imported == 0 || Imported == SuggestedModule) && @@ -1498,6 +1498,13 @@ void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc, } return; } + + // If we failed to find a submodule that we expected to find, we can + // continue. Otherwise, there's an error in the included file, so we + // don't want to include it. + if (!BuildingImportedModule && !Imported.isMissingExpected()) { + return; + } } if (Callbacks && SuggestedModule) { diff --git a/test/Modules/epic-fail.m b/test/Modules/epic-fail.m new file mode 100644 index 0000000000..7d2cdcf602 --- /dev/null +++ b/test/Modules/epic-fail.m @@ -0,0 +1,11 @@ +// RUN: rm -rf %t +// RUN: %clang_cc1 -fmodule-cache-path %t -fmodules -F %S/Inputs -DgetModuleVersion="epic fail" %s 2>&1 | FileCheck %s + +@__experimental_modules_import Module; +@__experimental_modules_import DependsOnModule; + +// CHECK: error: expected ';' after top level declarator +// CHECK: note: expanded from macro 'getModuleVersion' +// CHECK: fatal error: could not build module 'Module' +// CHECK: fatal error: could not build module 'Module' +// CHECK-NOT: error: -- 2.40.0