Argh, I forgot to update my commit message after I changed this patch. Instead of updating the PP’s module set in typo-correction, which broke a submodule visibility test, I fixed this by not updating the PP’s module set in the astreader if the source location is invalid.
> On Feb 11, 2016, at 9:04 AM, Ben Langmuir via cfe-commits > <cfe-commits@lists.llvm.org> wrote: > > Author: benlangmuir > Date: Thu Feb 11 11:04:42 2016 > New Revision: 260543 > > URL: http://llvm.org/viewvc/llvm-project?rev=260543&view=rev > Log: > [Modules] Don't infinite recurse on implicit import of circular modules in > preamble > > Update the Preprocessor's VisibleModuleSet when typo-correction creates > an implicit module import so that we won't accidentally write an invalid > SourceLocation into the preamble AST. This would later lead to infinite > recursion when loading the preamble AST because we use the value in > ImportLocs to prevent visiting a module twice. > > rdar://problem/24440990 > > Added: > cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-A.h > cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-B.h > cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-C.h > cfe/trunk/test/Index/Inputs/preamble-with-implicit-import.h > cfe/trunk/test/Index/preamble-with-implicit-import.m > Modified: > cfe/trunk/lib/Basic/Module.cpp > cfe/trunk/lib/Serialization/ASTReader.cpp > cfe/trunk/test/Index/Inputs/module.map > > Modified: cfe/trunk/lib/Basic/Module.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Module.cpp?rev=260543&r1=260542&r2=260543&view=diff > ============================================================================== > --- cfe/trunk/lib/Basic/Module.cpp (original) > +++ cfe/trunk/lib/Basic/Module.cpp Thu Feb 11 11:04:42 2016 > @@ -492,6 +492,7 @@ LLVM_DUMP_METHOD void Module::dump() con > > void VisibleModuleSet::setVisible(Module *M, SourceLocation Loc, > VisibleCallback Vis, ConflictCallback Cb) { > + assert(Loc.isValid() && "setVisible expects a valid import location"); > if (isVisible(M)) > return; > > > Modified: cfe/trunk/lib/Serialization/ASTReader.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=260543&r1=260542&r2=260543&view=diff > ============================================================================== > --- cfe/trunk/lib/Serialization/ASTReader.cpp (original) > +++ cfe/trunk/lib/Serialization/ASTReader.cpp Thu Feb 11 11:04:42 2016 > @@ -4062,7 +4062,9 @@ void ASTReader::InitializeContext() { > if (Module *Imported = getSubmodule(Import.ID)) { > makeModuleVisible(Imported, Module::AllVisible, > /*ImportLoc=*/Import.ImportLoc); > - PP.makeModuleVisible(Imported, Import.ImportLoc); > + if (Import.ImportLoc.isValid()) > + PP.makeModuleVisible(Imported, Import.ImportLoc); > + // FIXME: should we tell Sema to make the module visible too? > } > } > ImportedModules.clear(); > > Modified: cfe/trunk/test/Index/Inputs/module.map > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/Inputs/module.map?rev=260543&r1=260542&r2=260543&view=diff > ============================================================================== > --- cfe/trunk/test/Index/Inputs/module.map (original) > +++ cfe/trunk/test/Index/Inputs/module.map Thu Feb 11 11:04:42 2016 > @@ -6,3 +6,17 @@ module ModuleNeedsVFS { > framework module * { } > > module ModuleUndef { header "module-undef.h" } > + > +module PreambleWithImplicitImport { > + module A { > + header "preamble-with-implicit-import-A.h" > + } > + module B { > + header "preamble-with-implicit-import-B.h" > + export * > + } > + module C { > + header "preamble-with-implicit-import-C.h" > + export * > + } > +} > > Added: cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-A.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-A.h?rev=260543&view=auto > ============================================================================== > --- cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-A.h (added) > +++ cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-A.h Thu Feb 11 > 11:04:42 2016 > @@ -0,0 +1 @@ > +// preamble-with-implicit-import-A > > Added: cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-B.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-B.h?rev=260543&view=auto > ============================================================================== > --- cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-B.h (added) > +++ cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-B.h Thu Feb 11 > 11:04:42 2016 > @@ -0,0 +1,3 @@ > +#pragma once > +#include "preamble-with-implicit-import-C.h" // Circular > +typedef struct { char x; } Typo; > > Added: cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-C.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-C.h?rev=260543&view=auto > ============================================================================== > --- cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-C.h (added) > +++ cfe/trunk/test/Index/Inputs/preamble-with-implicit-import-C.h Thu Feb 11 > 11:04:42 2016 > @@ -0,0 +1,2 @@ > +#pragma once > +#include "preamble-with-implicit-import-B.h" // Circular > > Added: cfe/trunk/test/Index/Inputs/preamble-with-implicit-import.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/Inputs/preamble-with-implicit-import.h?rev=260543&view=auto > ============================================================================== > --- cfe/trunk/test/Index/Inputs/preamble-with-implicit-import.h (added) > +++ cfe/trunk/test/Index/Inputs/preamble-with-implicit-import.h Thu Feb 11 > 11:04:42 2016 > @@ -0,0 +1,4 @@ > +#include "preamble-with-implicit-import-A.h" > + > +// Typo is defined in B, which is not imported. > +void useTypeFromB(Typo *); > > Added: cfe/trunk/test/Index/preamble-with-implicit-import.m > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/preamble-with-implicit-import.m?rev=260543&view=auto > ============================================================================== > --- cfe/trunk/test/Index/preamble-with-implicit-import.m (added) > +++ cfe/trunk/test/Index/preamble-with-implicit-import.m Thu Feb 11 11:04:42 > 2016 > @@ -0,0 +1,6 @@ > +// RUN: rm -rf %t > +// RUN: env CINDEXTEST_EDITING=1 c-index-test -test-load-source-reparse 2 > none %s -I %S/Inputs -fmodules -fmodules-cache-path=%t -fspell-checking 2>&1 > | FileCheck %s > +// CHECK: error: declaration of 'Typo' must be imported > +// CHECK: error: declaration of 'Typo' must be imported > + > +#include "preamble-with-implicit-import.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
