On 4 December 2016 at 20:44, Richard Smith <rich...@metafoo.co.uk> wrote:
> On 4 Dec 2016 2:44 pm, "Daniel Jasper via cfe-commits" < > cfe-commits@lists.llvm.org> wrote: > > Author: djasper > Date: Sun Dec 4 16:34:37 2016 > New Revision: 288626 > > URL: http://llvm.org/viewvc/llvm-project?rev=288626&view=rev > Log: > Revert "Recover better from an incompatible .pcm file being provided by > -fmodule-file=. We try to include the headers of the module textually in > this case, still enforcing the modules semantic rules. In order to make > that work, we need to still track that we're entering and leaving the > module. Also, if the module was also marked as unavailable (perhaps because > it was missing a file), we shouldn't mark the module unavailable -- we > don't need the module to be complete if we're going to enter it textually." > > This reverts commit r288449. > > I believe that this is currently faulty wrt. modules being imported > inside namespaces. Adding these lines to the new test: > > namespace n { > #include "foo.h" > } > > Makes it break with > > fatal error: import of module 'M' appears within namespace 'n' > > However, I believe it should fail with > > error: redundant #include of module 'M' appears within namespace 'n' > > > Is that really worse than the broken internal state and asserts we get in > the same situation without this patch? The ability to continue like this > after a config mismatch is best-effort as is (and we're working towards > removing it...); I don't think we should be all that picky about which > error diagnostic we get here. > > Mind if I revert your revert? > Wrong diagnostic fixed in r288737, revert reverted in revert r288741 (with the above addition to the test). > I have tracked this down to us now inserting a tok::annot_module_begin > instead of a tok::annot_module_include in > Preprocessor::HandleIncludeDirective() and then later in > Parser::parseMisplacedModuleImport(), we hit the code path for > tok::annot_module_begin, which doesn't set FromInclude of > checkModuleImportContext to true (thus leading to the "wrong" > diagnostic). > > Removed: > cfe/trunk/test/Modules/config-mismatch.cpp > Modified: > cfe/trunk/include/clang/Lex/ModuleLoader.h > cfe/trunk/lib/Frontend/CompilerInstance.cpp > cfe/trunk/lib/Lex/PPDirectives.cpp > > Modified: cfe/trunk/include/clang/Lex/ModuleLoader.h > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ > Lex/ModuleLoader.h?rev=288626&r1=288625&r2=288626&view=diff > ============================================================ > ================== > --- cfe/trunk/include/clang/Lex/ModuleLoader.h (original) > +++ cfe/trunk/include/clang/Lex/ModuleLoader.h Sun Dec 4 16:34:37 2016 > @@ -31,22 +31,13 @@ typedef ArrayRef<std::pair<IdentifierInf > > /// \brief Describes the result of attempting to load a module. > class ModuleLoadResult { > -public: > - enum LoadResultKind { > - // We either succeeded or failed to load the named module. > - Normal, > - // The module exists, but does not actually contain the named > submodule. > - // This should only happen if the named submodule was inferred from an > - // umbrella directory, but not actually part of the umbrella header. > - MissingExpected, > - // The module exists but cannot be imported due to a configuration > mismatch. > - ConfigMismatch > - }; > - llvm::PointerIntPair<Module *, 2, LoadResultKind> Storage; > + llvm::PointerIntPair<Module *, 1, bool> Storage; > > +public: > ModuleLoadResult() : Storage() { } > - ModuleLoadResult(Module *M) : Storage(M, Normal) {} > - ModuleLoadResult(LoadResultKind Kind) : Storage(nullptr, Kind) {} > + > + ModuleLoadResult(Module *module, bool missingExpected) > + : Storage(module, missingExpected) { } > > operator Module *() const { return Storage.getPointer(); } > > @@ -54,11 +45,7 @@ public: > /// 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() == > MissingExpected; } > - > - /// \brief Determines whether the module failed to load due to a > configuration > - /// mismatch with an explicitly-named .pcm file from the command line. > - bool isConfigMismatch() const { return Storage.getInt() == > ConfigMismatch; } > + bool isMissingExpected() const { return Storage.getInt(); } > }; > > /// \brief Abstract interface for a module loader. > > Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ > CompilerInstance.cpp?rev=288626&r1=288625&r2=288626&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original) > +++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Sun Dec 4 16:34:37 2016 > @@ -1393,21 +1393,8 @@ bool CompilerInstance::loadModuleFile(St > if (Module *M = CI.getPreprocessor() > .getHeaderSearchInfo() > .getModuleMap() > - .findModule(II->getName())) { > + .findModule(II->getName())) > M->HasIncompatibleModuleFile = true; > - > - // Mark module as available if the only reason it was > unavailable > - // was missing headers. > - SmallVector<Module *, 2> Stack; > - Stack.push_back(M); > - while (!Stack.empty()) { > - Module *Current = Stack.pop_back_val(); > - if (Current->IsMissingRequirement) continue; > - Current->IsAvailable = true; > - Stack.insert(Stack.end(), > - Current->submodule_begin(), > Current->submodule_end()); > - } > - } > } > LoadedModules.clear(); > } > @@ -1511,7 +1498,7 @@ CompilerInstance::loadModule(SourceLocat > if (Module && Module->HasIncompatibleModuleFile) { > // We tried and failed to load a module file for this module. Fall > // back to textual inclusion for its headers. > - return ModuleLoadResult::ConfigMismatch; > + return ModuleLoadResult(nullptr, /*missingExpected*/true); > } > > getDiagnostics().Report(ModuleNameLoc, > diag::err_module_build_disabled) > @@ -1718,7 +1705,7 @@ CompilerInstance::loadModule(SourceLocat > << Module->getFullModuleName() > << SourceRange(Path.front().second, Path.back().second); > > - return ModuleLoadResult::MissingExpected; > + return ModuleLoadResult(nullptr, true); > } > > // Check whether this module is available. > @@ -1752,7 +1739,7 @@ CompilerInstance::loadModule(SourceLocat > } > > LastModuleImportLoc = ImportLoc; > - LastModuleImportResult = ModuleLoadResult(Module); > + LastModuleImportResult = ModuleLoadResult(Module, false); > return LastModuleImportResult; > } > > > Modified: cfe/trunk/lib/Lex/PPDirectives.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPDire > ctives.cpp?rev=288626&r1=288625&r2=288626&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/Lex/PPDirectives.cpp (original) > +++ cfe/trunk/lib/Lex/PPDirectives.cpp Sun Dec 4 16:34:37 2016 > @@ -1866,7 +1866,10 @@ void Preprocessor::HandleIncludeDirectiv > // unavailable, diagnose the situation and bail out. > // FIXME: Remove this; loadModule does the same check (but produces > // slightly worse diagnostics). > - if (!SuggestedModule.getModule()->isAvailable()) { > + if (!SuggestedModule.getModule()->isAvailable() && > + !SuggestedModule.getModule() > + ->getTopLevelModule() > + ->HasIncompatibleModuleFile) { > Module::Requirement Requirement; > Module::UnresolvedHeaderDirective MissingHeader; > Module *M = SuggestedModule.getModule(); > @@ -1915,12 +1918,9 @@ void Preprocessor::HandleIncludeDirectiv > else if (Imported.isMissingExpected()) { > // We failed to find a submodule that we assumed would exist > (because it > // was in the directory of an umbrella header, for instance), but no > - // actual module containing it exists (because the umbrella header > is > + // actual module exists for it (because the umbrella header is > // incomplete). Treat this as a textual inclusion. > SuggestedModule = ModuleMap::KnownHeader(); > - } else if (Imported.isConfigMismatch()) { > - // On a configuration mismatch, enter the header textually. We > still know > - // that it's part of the corresponding module. > } else { > // We hit an error processing the import. Bail out. > if (hadModuleLoaderFatalFailure()) { > > Removed: cfe/trunk/test/Modules/config-mismatch.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/ > config-mismatch.cpp?rev=288625&view=auto > ============================================================ > ================== > --- cfe/trunk/test/Modules/config-mismatch.cpp (original) > +++ cfe/trunk/test/Modules/config-mismatch.cpp (removed) > @@ -1,10 +0,0 @@ > -// RUN: rm -rf %t > -// RUN: mkdir %t > -// RUN: echo 'module M { header "foo.h" header "bar.h" }' > %t/map > -// RUN: echo 'template<typename T> void f(T t) { int n; t.f(n); }' > > %t/foo.h > -// RUN: touch %t/bar.h > -// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility -x c++ > %t/map -emit-module -fmodule-name=M -o %t/pcm > -// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility > -fmodule-map-file=%t/map -fmodule-file=%t/pcm -I%t %s -fsyntax-only > -fexceptions -Wno-module-file-config-mismatch > -// RUN: rm %t/bar.h > -// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility > -fmodule-map-file=%t/map -fmodule-file=%t/pcm -I%t %s -fsyntax-only > -fexceptions -Wno-module-file-config-mismatch > -#include "foo.h" > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
