[modules] Fix some of the confusion when computing the override set for a macro

introduced by finalization. This is still not entirely correct; more fixes to
follow.

llvm-svn: 213498

clang/lib/Serialization/ASTReader.cpp

@@ -3323,8 +3323,7 @@ static void moveMethodToBackOfGlobalList(Sema &S, ObjCMethodDecl *Method) {
 void ASTReader::makeNamesVisible(const HiddenNames &Names, Module *Owner,
                                  bool FromFinalization) {
   // FIXME: Only do this if Owner->NameVisibility == AllVisible.
-  for (unsigned I = 0, N = Names.HiddenDecls.size(); I != N; ++I) {
-    Decl *D = Names.HiddenDecls[I];
+  for (Decl *D : Names.HiddenDecls) {
     bool wasHidden = D->Hidden;
     D->Hidden = false;
 
@@ -3337,10 +3336,8 @@ void ASTReader::makeNamesVisible(const HiddenNames &Names, Module *Owner,
 
   assert((FromFinalization || Owner->NameVisibility >= Module::MacrosVisible) &&
          "nothing to make visible?");
-  for (HiddenMacrosMap::const_iterator I = Names.HiddenMacros.begin(),
-                                       E = Names.HiddenMacros.end();
-       I != E; ++I)
-    installImportedMacro(I->first, I->second, Owner, FromFinalization);
+  for (const auto &Macro : Names.HiddenMacros)
+    installImportedMacro(Macro.first, Macro.second, Owner, FromFinalization);
 }
 
 void ASTReader::makeModuleVisible(Module *Mod,
@@ -3374,9 +3371,12 @@ void ASTReader::makeModuleVisible(Module *Mod,
     // mark them as visible.
     HiddenNamesMapType::iterator Hidden = HiddenNamesMap.find(Mod);
     if (Hidden != HiddenNamesMap.end()) {
-      makeNamesVisible(Hidden->second, Hidden->first,
-                       /*FromFinalization*/false);
+      auto HiddenNames = std::move(*Hidden);
       HiddenNamesMap.erase(Hidden);
+      makeNamesVisible(HiddenNames.second, HiddenNames.first,
+                       /*FromFinalization*/false);
+      assert(HiddenNamesMap.find(Mod) == HiddenNamesMap.end() &&
+             "making names visible added hidden names");
     }
 
     // Push any exported modules onto the stack to be marked as visible.
@@ -3899,12 +3899,12 @@ void ASTReader::InitializeContext() {
 }
 
 void ASTReader::finalizeForWriting() {
-  for (HiddenNamesMapType::iterator Hidden = HiddenNamesMap.begin(),
-                                 HiddenEnd = HiddenNamesMap.end();
-       Hidden != HiddenEnd; ++Hidden) {
-    makeNamesVisible(Hidden->second, Hidden->first, /*FromFinalization*/true);
+  while (!HiddenNamesMap.empty()) {
+    auto HiddenNames = std::move(*HiddenNamesMap.begin());
+    HiddenNamesMap.erase(HiddenNamesMap.begin());
+    makeNamesVisible(HiddenNames.second, HiddenNames.first,
+                     /*FromFinalization*/true);
   }
-  HiddenNamesMap.clear();
 }
 
 /// \brief Given a cursor at the start of an AST file, scan ahead and drop the

clang/lib/Serialization/ASTWriter.cpp

@@ -3034,16 +3034,25 @@ class ASTIdentifierTableTrait {
   MacroDirective *getPublicSubmoduleMacro(MacroDirective *MD,
                                           SubmoduleID &ModID,
                                           OverriddenList &Overridden) {
+    Overridden.clear();
     if (!MD)
       return nullptr;
 
-    Overridden.clear();
     SubmoduleID OrigModID = ModID;
     Optional<bool> IsPublic;
     for (; MD; MD = MD->getPrevious()) {
       SubmoduleID ThisModID = getSubmoduleID(MD);
       if (ThisModID == 0) {
         IsPublic = Optional<bool>();
+
+        // If we have no directive location, this macro was installed when
+        // finalizing the ASTReader.
+        if (DefMacroDirective *DefMD = dyn_cast<DefMacroDirective>(MD))
+          if (DefMD->getInfo()->getOwningModuleID())
+            return MD;
+        // Skip imports that only produce #undefs for now.
+        // FIXME: We should still re-export them!
+
         continue;
       }
       if (ThisModID != ModID) {
@@ -3068,7 +3077,7 @@ class ASTIdentifierTableTrait {
           else
             SourceID = Writer.inferSubmoduleIDFromLocation(DefLoc);
         }
-        if (SourceID != OrigModID)
+        if (OrigModID && SourceID != OrigModID)
           Overridden.push_back(SourceID);
       }
 
@@ -3095,17 +3104,7 @@ class ASTIdentifierTableTrait {
   }
 
   SubmoduleID getSubmoduleID(MacroDirective *MD) {
-    if (MD->getLocation().isValid())
-      return Writer.inferSubmoduleIDFromLocation(MD->getLocation());
-
-    // If we have no directive location, this macro was installed when
-    // finalizing the ASTReader.
-    if (DefMacroDirective *DefMD = dyn_cast<DefMacroDirective>(MD))
-      return DefMD->getInfo()->getOwningModuleID();
-
-    // Skip imports that only produce #undefs for now.
-    // FIXME: We should still re-export them!
-    return 0;
+    return Writer.inferSubmoduleIDFromLocation(MD->getLocation());
   }
 
 public:

clang/test/Modules/Inputs/macro-hiding/e1.h

@@ -0,0 +1 @@
+#include "a1.h"

clang/test/Modules/Inputs/macro-hiding/e2.h

@@ -0,0 +1,2 @@
+#include "a1.h"
+inline void e1() { assert(true); }

clang/test/Modules/Inputs/macro-hiding/module.modulemap

@@ -12,3 +12,7 @@ module c {
 module d {
   module d1 { header "d1.h" export * }
 }
+module e {
+  module e1 { header "e1.h" export * }
+  module e2 { header "e2.h" export * }
+}

clang/test/Modules/macro-hiding.cpp

@@ -62,6 +62,8 @@
 // RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DB1 -DB2 -DD1
 // RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DB1 -DB2 -DC1
 // RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DB1 -DB2 -DC1 -DD1
+//
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DE1
 
 #ifdef A1
 #include "a1.h"
@@ -87,7 +89,15 @@
 #include "d1.h"
 #endif
 
-#if defined(A1) || defined(B2) || defined(C1) || defined(D1)
+#ifdef E1
+#include "e1.h"
+#endif
+
+#ifdef E2
+#include "e2.h"
+#endif
+
+#if defined(A1) || defined(B2) || defined(C1) || defined(D1) || defined(E1) || defined(E2)
 void h() { assert(true); }
 #else
 void assert() {}

clang/test/Modules/macro-reexport/a1.h

@@ -0,0 +1 @@
+#define assert(x) a

clang/test/Modules/macro-reexport/b2.h

@@ -0,0 +1,2 @@
+#include "a2.h"
+#define assert(x) b

clang/test/Modules/macro-reexport/c1.h

@@ -0,0 +1,2 @@
+#include "b1.h"
+#define assert(x) c

clang/test/Modules/macro-reexport/d1.h

@@ -0,0 +1,2 @@
+#include "c1.h"
+#define assert(x) d

clang/test/Modules/macro-reexport/d2.h

@@ -0,0 +1 @@
+#include "b2.h"

clang/test/Modules/macro-reexport/macro-reexport.cpp

@@ -0,0 +1,13 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -DD2 -I. %s -fmodules-cache-path=%t -verify
+// RUN: %clang_cc1 -fsyntax-only -DD2 -I. -fmodules %s -fmodules-cache-path=%t -verify
+// RUN: %clang_cc1 -fsyntax-only -DC1 -I. %s -fmodules-cache-path=%t -verify
+// RUN: %clang_cc1 -fsyntax-only -DC1 -I. -fmodules %s -fmodules-cache-path=%t -verify
+
+#ifdef D2
+#include "d2.h"
+void f() { return assert(true); } // expected-error {{undeclared identifier 'b'}}
+#else
+#include "c1.h"
+void f() { return assert(true); } // expected-error {{undeclared identifier 'c'}}
+#endif

clang/test/Modules/macro-reexport/module.modulemap

@@ -0,0 +1,15 @@
+module b {
+  module b2 { header "b2.h" export * }
+  module b1 { header "b1.h" export * }
+}
+module a {
+  module a1 { header "a1.h" export * }
+  module a2 { header "a2.h" export * }
+}
+module c {
+  module c1 { header "c1.h" export * }
+}
+module d {
+  module d1 { header "d1.h" export * }
+  module d2 { header "d2.h" export * }
+}