[modules] If we re-enter a submodule from within itself (when submodule

visibility is enabled) or leave and re-enter it, restore the macro and module
visibility state from last time we were in that submodule.

This allows mutually-#including header files to stand a chance at being
modularized with local visibility enabled.

llvm-svn: 237871

clang/include/clang/Lex/Preprocessor.h

@@ -391,7 +391,7 @@ class Preprocessor : public RefCountedBase<Preprocessor> {
       // FIXME: Find a spare bit on IdentifierInfo and store a
       //        HasModuleMacros flag.
       if (!II->hasMacroDefinition() || !PP.getLangOpts().Modules ||
-          !PP.VisibleModules.getGeneration())
+          !PP.CurSubmoduleState->VisibleModules.getGeneration())
         return nullptr;
 
       auto *Info = State.dyn_cast<ModuleMacroInfo*>();
@@ -401,7 +401,7 @@ class Preprocessor : public RefCountedBase<Preprocessor> {
         State = Info;
       }
 
-      if (PP.VisibleModules.getGeneration() !=
+      if (PP.CurSubmoduleState->VisibleModules.getGeneration() !=
           Info->ActiveModuleMacrosGeneration)
         PP.updateModuleMacroInfo(II, *Info);
       return Info;
@@ -483,33 +483,50 @@ class Preprocessor : public RefCountedBase<Preprocessor> {
     }
   };
 
-  typedef llvm::DenseMap<const IdentifierInfo *, MacroState> MacroMap;
-
   /// For each IdentifierInfo that was associated with a macro, we
   /// keep a mapping to the history of all macro definitions and #undefs in
   /// the reverse order (the latest one is in the head of the list).
-  MacroMap Macros;
+  ///
+  /// This mapping lives within the \p CurSubmoduleState.
+  typedef llvm::DenseMap<const IdentifierInfo *, MacroState> MacroMap;
 
   friend class ASTReader;
 
+  struct SubmoduleState;
+
   /// \brief Information about a submodule that we're currently building.
   struct BuildingSubmoduleInfo {
-    BuildingSubmoduleInfo(Module *M, SourceLocation ImportLoc)
+    BuildingSubmoduleInfo(Module *M, SourceLocation ImportLoc,
-        : M(M), ImportLoc(ImportLoc) {}
+                          SubmoduleState *OuterSubmoduleState)
+        : M(M), ImportLoc(ImportLoc), OuterSubmoduleState(OuterSubmoduleState) {
+    }
 
     /// The module that we are building.
     Module *M;
     /// The location at which the module was included.
     SourceLocation ImportLoc;
-    /// The macros that were visible before we entered the module.
+    /// The previous SubmoduleState.
-    MacroMap Macros;
+    SubmoduleState *OuterSubmoduleState;
-    /// The set of modules that was visible in the surrounding submodule.
+  };
-    VisibleModuleSet VisibleModules;
+  SmallVector<BuildingSubmoduleInfo, 8> BuildingSubmoduleStack;
 
+  /// \brief Information about a submodule's preprocessor state.
+  struct SubmoduleState {
+    /// The macros for the submodule.
+    MacroMap Macros;
+    /// The set of modules that are visible within the submodule.
+    VisibleModuleSet VisibleModules;
     // FIXME: CounterValue?
     // FIXME: PragmaPushMacroInfo?
   };
-  SmallVector<BuildingSubmoduleInfo, 8> BuildingSubmoduleStack;
+  std::map<Module*, SubmoduleState> Submodules;
+
+  /// The preprocessor state for preprocessing outside of any submodule.
+  SubmoduleState NullSubmoduleState;
+
+  /// The current submodule state. Will be \p NullSubmoduleState if we're not
+  /// in a submodule.
+  SubmoduleState *CurSubmoduleState;
 
   /// The set of known macros exported from modules.
   llvm::FoldingSet<ModuleMacro> ModuleMacros;
@@ -519,9 +536,6 @@ class Preprocessor : public RefCountedBase<Preprocessor> {
   llvm::DenseMap<const IdentifierInfo *, llvm::TinyPtrVector<ModuleMacro*>>
       LeafModuleMacros;
 
-  /// The current set of visible modules.
-  VisibleModuleSet VisibleModules;
-
   /// \brief Macros that we want to warn because they are not used at the end
   /// of the translation unit.
   ///
@@ -767,7 +781,7 @@ public:
     if (!II->hasMacroDefinition())
       return MacroDefinition();
 
-    MacroState &S = Macros[II];
+    MacroState &S = CurSubmoduleState->Macros[II];
     auto *MD = S.getLatest();
     while (MD && isa<VisibilityMacroDirective>(MD))
       MD = MD->getPrevious();
@@ -781,7 +795,7 @@ public:
     if (!II->hadMacroDefinition())
       return MacroDefinition();
 
-    MacroState &S = Macros[II];
+    MacroState &S = CurSubmoduleState->Macros[II];
     MacroDirective::DefInfo DI;
     if (auto *MD = S.getLatest())
       DI = MD->findDirectiveAtLoc(Loc, getSourceManager());
@@ -1018,7 +1032,7 @@ public:
   void makeModuleVisible(Module *M, SourceLocation Loc);
 
   SourceLocation getModuleImportLoc(Module *M) const {
-    return VisibleModules.getImportLoc(M);
+    return CurSubmoduleState->VisibleModules.getImportLoc(M);
   }
 
   /// \brief Lex a string literal, which may be the concatenation of multiple

clang/lib/Lex/PPLexerChange.cpp

@@ -610,39 +610,51 @@ void Preprocessor::HandleMicrosoftCommentPaste(Token &Tok) {
 }
 
 void Preprocessor::EnterSubmodule(Module *M, SourceLocation ImportLoc) {
-  // Save the current state for future imports.
+  if (!getLangOpts().ModulesLocalVisibility) {
-  BuildingSubmoduleStack.push_back(BuildingSubmoduleInfo(M, ImportLoc));
+    // Just track that we entered this submodule.
-  auto &Info = BuildingSubmoduleStack.back();
+    BuildingSubmoduleStack.push_back(
-  Info.Macros.swap(Macros);
+        BuildingSubmoduleInfo(M, ImportLoc, CurSubmoduleState));
-  // Save our visible modules set. This is guaranteed to clear the set.
+    return;
-  if (getLangOpts().ModulesLocalVisibility) {
+  }
-    Info.VisibleModules = std::move(VisibleModules);
 
-    // Resolve as much of the module definition as we can now, before we enter
+  // Resolve as much of the module definition as we can now, before we enter
-    // one if its headers.
+  // one of its headers.
-    // FIXME: Can we enable Complain here?
+  // FIXME: Can we enable Complain here?
-    ModuleMap &ModMap = getHeaderSearchInfo().getModuleMap();
+  // FIXME: Can we do this when local visibility is disabled?
-    ModMap.resolveExports(M, /*Complain=*/false);
+  ModuleMap &ModMap = getHeaderSearchInfo().getModuleMap();
-    ModMap.resolveUses(M, /*Complain=*/false);
+  ModMap.resolveExports(M, /*Complain=*/false);
-    ModMap.resolveConflicts(M, /*Complain=*/false);
+  ModMap.resolveUses(M, /*Complain=*/false);
+  ModMap.resolveConflicts(M, /*Complain=*/false);
 
-    // This module is visible to itself.
+  // If this is the first time we've entered this module, set up its state.
+  auto R = Submodules.emplace(std::piecewise_construct, std::make_tuple(M),
+                              std::make_tuple());
+  auto &State = R.first->second;
+  bool FirstTime = R.second;
+  if (FirstTime) {
+    // Determine the set of starting macros for this submodule; take these
+    // from the "null" module (the predefines buffer).
+    auto &StartingMacros = NullSubmoduleState.Macros;
+
+    // Restore to the starting state.
+    // FIXME: Do this lazily, when each macro name is first referenced.
+    for (auto &Macro : StartingMacros) {
+      MacroState MS(Macro.second.getLatest());
+      MS.setOverriddenMacros(*this, Macro.second.getOverriddenMacros());
+      State.Macros.insert(std::make_pair(Macro.first, std::move(MS)));
+    }
+  }
+
+  // Track that we entered this module.
+  BuildingSubmoduleStack.push_back(
+      BuildingSubmoduleInfo(M, ImportLoc, CurSubmoduleState));
+
+  // Switch to this submodule as the current submodule.
+  CurSubmoduleState = &State;
+
+  // This module is visible to itself.
+  if (FirstTime)
     makeModuleVisible(M, ImportLoc);
-  }
-
-  // Determine the set of starting macros for this submodule.
-  // FIXME: If we re-enter a submodule, should we restore its MacroDirectives?
-  auto &StartingMacros = getLangOpts().ModulesLocalVisibility
-                             ? BuildingSubmoduleStack[0].Macros
-                             : Info.Macros;
-
-  // Restore to the starting state.
-  // FIXME: Do this lazily, when each macro name is first referenced.
-  for (auto &Macro : StartingMacros) {
-    MacroState MS(Macro.second.getLatest());
-    MS.setOverriddenMacros(*this, Macro.second.getOverriddenMacros());
-    Macros.insert(std::make_pair(Macro.first, std::move(MS)));
-  }
 }
 
 void Preprocessor::LeaveSubmodule() {
@@ -652,15 +664,15 @@ void Preprocessor::LeaveSubmodule() {
   SourceLocation ImportLoc = Info.ImportLoc;
 
   // Create ModuleMacros for any macros defined in this submodule.
-  for (auto &Macro : Macros) {
+  for (auto &Macro : CurSubmoduleState->Macros) {
     auto *II = const_cast<IdentifierInfo*>(Macro.first);
-    auto &OuterInfo = Info.Macros[II];
 
     // Find the starting point for the MacroDirective chain in this submodule.
-    auto *OldMD = OuterInfo.getLatest();
+    MacroDirective *OldMD = nullptr;
-    if (getLangOpts().ModulesLocalVisibility &&
+    if (getLangOpts().ModulesLocalVisibility) {
-        BuildingSubmoduleStack.size() > 1) {
+      // FIXME: It'd be better to start at the state from when we most recently
-      auto &PredefMacros = BuildingSubmoduleStack[0].Macros;
+      // entered this submodule, but it doesn't really matter.
+      auto &PredefMacros = NullSubmoduleState.Macros;
       auto PredefMacroIt = PredefMacros.find(Macro.first);
       if (PredefMacroIt == PredefMacros.end())
         OldMD = nullptr;
@@ -708,23 +720,15 @@ void Preprocessor::LeaveSubmodule() {
         break;
       }
     }
-
-    // Maintain a single macro directive chain if we're not tracking
-    // per-submodule macro visibility.
-    if (!getLangOpts().ModulesLocalVisibility)
-      OuterInfo.setLatest(Macro.second.getLatest());
   }
 
-  // Put back the old macros.
+  // Put back the outer module's state, if we're tracking it.
-  std::swap(Info.Macros, Macros);
-
   if (getLangOpts().ModulesLocalVisibility)
-    VisibleModules = std::move(Info.VisibleModules);
+    CurSubmoduleState = Info.OuterSubmoduleState;
 
   BuildingSubmoduleStack.pop_back();
 
   // A nested #include makes the included submodule visible.
-  if (!BuildingSubmoduleStack.empty() ||
+  if (!BuildingSubmoduleStack.empty() || !getLangOpts().ModulesLocalVisibility)
-      !getLangOpts().ModulesLocalVisibility)
     makeModuleVisible(LeavingMod, ImportLoc);
 }

clang/lib/Lex/PPMacroExpansion.cpp

@@ -37,15 +37,16 @@ MacroDirective *
 Preprocessor::getLocalMacroDirectiveHistory(const IdentifierInfo *II) const {
   if (!II->hadMacroDefinition())
     return nullptr;
-  auto Pos = Macros.find(II);
+  auto Pos = CurSubmoduleState->Macros.find(II);
-  return Pos == Macros.end() ? nullptr : Pos->second.getLatest();
+  return Pos == CurSubmoduleState->Macros.end() ? nullptr
+                                                : Pos->second.getLatest();
 }
 
 void Preprocessor::appendMacroDirective(IdentifierInfo *II, MacroDirective *MD){
   assert(MD && "MacroDirective should be non-zero!");
   assert(!MD->getPrevious() && "Already attached to a MacroDirective history.");
 
-  MacroState &StoredMD = Macros[II];
+  MacroState &StoredMD = CurSubmoduleState->Macros[II];
   auto *OldMD = StoredMD.getLatest();
   MD->setPrevious(OldMD);
   StoredMD.setLatest(MD);
@@ -62,7 +63,7 @@ void Preprocessor::appendMacroDirective(IdentifierInfo *II, MacroDirective *MD){
 void Preprocessor::setLoadedMacroDirective(IdentifierInfo *II,
                                            MacroDirective *MD) {
   assert(II && MD);
-  MacroState &StoredMD = Macros[II];
+  MacroState &StoredMD = CurSubmoduleState->Macros[II];
   assert(!StoredMD.getLatest() &&
          "the macro history was modified before initializing it from a pch");
   StoredMD = MD;
@@ -124,9 +125,11 @@ ModuleMacro *Preprocessor::getModuleMacro(Module *Mod, IdentifierInfo *II) {
 
 void Preprocessor::updateModuleMacroInfo(const IdentifierInfo *II,
                                          ModuleMacroInfo &Info) {
-  assert(Info.ActiveModuleMacrosGeneration != VisibleModules.getGeneration() &&
+  assert(Info.ActiveModuleMacrosGeneration !=
+             CurSubmoduleState->VisibleModules.getGeneration() &&
          "don't need to update this macro name info");
-  Info.ActiveModuleMacrosGeneration = VisibleModules.getGeneration();
+  Info.ActiveModuleMacrosGeneration =
+      CurSubmoduleState->VisibleModules.getGeneration();
 
   auto Leaf = LeafModuleMacros.find(II);
   if (Leaf == LeafModuleMacros.end()) {
@@ -146,7 +149,7 @@ void Preprocessor::updateModuleMacroInfo(const IdentifierInfo *II,
                                                 Leaf->second.end());
   while (!Worklist.empty()) {
     auto *MM = Worklist.pop_back_val();
-    if (VisibleModules.isVisible(MM->getOwningModule())) {
+    if (CurSubmoduleState->VisibleModules.isVisible(MM->getOwningModule())) {
       // We only care about collecting definitions; undefinitions only act
       // to override other definitions.
       if (MM->getMacroInfo())
@@ -200,8 +203,8 @@ void Preprocessor::dumpMacroInfo(const IdentifierInfo *II) {
   if (LeafIt != LeafModuleMacros.end())
     Leaf = LeafIt->second;
   const MacroState *State = nullptr;
-  auto Pos = Macros.find(II);
+  auto Pos = CurSubmoduleState->Macros.find(II);
-  if (Pos != Macros.end())
+  if (Pos != CurSubmoduleState->Macros.end())
     State = &Pos->second;
 
   llvm::errs() << "MacroState " << State << " " << II->getNameStart();
@@ -236,7 +239,8 @@ void Preprocessor::dumpMacroInfo(const IdentifierInfo *II) {
 
     if (Active.count(MM))
       llvm::errs() << " active";
-    else if (!VisibleModules.isVisible(MM->getOwningModule()))
+    else if (!CurSubmoduleState->VisibleModules.isVisible(
+                 MM->getOwningModule()))
       llvm::errs() << " hidden";
     else if (MM->getMacroInfo())
       llvm::errs() << " overridden";

clang/lib/Lex/Preprocessor.cpp

@@ -73,7 +73,8 @@ Preprocessor::Preprocessor(IntrusiveRefCntPtr<PreprocessorOptions> PPOpts,
       ModuleImportExpectsIdentifier(false), CodeCompletionReached(0),
       MainFileDir(nullptr), SkipMainFilePreamble(0, true), CurPPLexer(nullptr),
       CurDirLookup(nullptr), CurLexerKind(CLK_Lexer), CurSubmodule(nullptr),
-      Callbacks(nullptr), MacroArgCache(nullptr), Record(nullptr),
+      Callbacks(nullptr), CurSubmoduleState(&NullSubmoduleState),
+      MacroArgCache(nullptr), Record(nullptr),
       MIChainHead(nullptr), DeserialMIChainHead(nullptr) {
   OwnsHeaderSearch = OwnsHeaders;
   
@@ -266,7 +267,9 @@ void Preprocessor::PrintStats() {
   llvm::errs() << "\n  Macro Expanded Tokens: "
                << llvm::capacity_in_bytes(MacroExpandedTokens);
   llvm::errs() << "\n  Predefines Buffer: " << Predefines.capacity();
-  llvm::errs() << "\n  Macros: " << llvm::capacity_in_bytes(Macros);
+  // FIXME: List information for all submodules.
+  llvm::errs() << "\n  Macros: "
+               << llvm::capacity_in_bytes(CurSubmoduleState->Macros);
   llvm::errs() << "\n  #pragma push_macro Info: "
                << llvm::capacity_in_bytes(PragmaPushMacroInfo);
   llvm::errs() << "\n  Poison Reasons: "
@@ -283,14 +286,16 @@ Preprocessor::macro_begin(bool IncludeExternalMacros) const {
     ExternalSource->ReadDefinedMacros();
   }
 
-  return Macros.begin();
+  return CurSubmoduleState->Macros.begin();
 }
 
 size_t Preprocessor::getTotalMemory() const {
   return BP.getTotalMemory()
     + llvm::capacity_in_bytes(MacroExpandedTokens)
     + Predefines.capacity() /* Predefines buffer. */
-    + llvm::capacity_in_bytes(Macros)
+    // FIXME: Include sizes from all submodules, and include MacroInfo sizes,
+    // and ModuleMacros.
+    + llvm::capacity_in_bytes(CurSubmoduleState->Macros)
     + llvm::capacity_in_bytes(PragmaPushMacroInfo)
     + llvm::capacity_in_bytes(PoisonReasons)
     + llvm::capacity_in_bytes(CommentHandlers);
@@ -304,7 +309,7 @@ Preprocessor::macro_end(bool IncludeExternalMacros) const {
     ExternalSource->ReadDefinedMacros();
   }
 
-  return Macros.end();
+  return CurSubmoduleState->Macros.end();
 }
 
 /// \brief Compares macro tokens with a specified token value sequence.
@@ -781,7 +786,7 @@ void Preprocessor::LexAfterModuleImport(Token &Result) {
 }
 
 void Preprocessor::makeModuleVisible(Module *M, SourceLocation Loc) {
-  VisibleModules.setVisible(
+  CurSubmoduleState->VisibleModules.setVisible(
       M, Loc, [](Module *) {},
       [&](ArrayRef<Module *> Path, Module *Conflict, StringRef Message) {
         // FIXME: Include the path in the diagnostic.

clang/test/Modules/Inputs/submodule-visibility/cycle1.h

@@ -0,0 +1,8 @@
+#ifndef CYCLE1
+#define CYCLE1
+
+#include "cycle2.h"
+
+struct C1 {};
+
+#endif

clang/test/Modules/Inputs/submodule-visibility/cycle2.h

@@ -0,0 +1,8 @@
+#ifndef CYCLE2
+#define CYCLE2
+
+#include "cycle1.h"
+
+struct C2 {};
+
+#endif

clang/test/Modules/Inputs/submodule-visibility/module.modulemap

@@ -1 +1,6 @@
 module x { module a { header "a.h" } module b { header "b.h" } }
+
+module cycles {
+  module cycle1 { header "cycle1.h" }
+  module cycle2 { header "cycle2.h" }
+}

clang/test/Modules/submodule-visibility-cycles.cpp

@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fno-modules-error-recovery -fmodules-local-submodule-visibility -fmodules-cache-path=%t -I%S/Inputs/submodule-visibility -verify %s
+
+#include "cycle1.h"
+C1 c1;
+C2 c2; // expected-error {{must be imported}} expected-error {{}}
+// expected-note@cycle2.h:6 {{here}}
+
+#include "cycle2.h"
+C2 c3;