Reinstate r213348, reverted in r213395, with an additional bug fix and more

thorough tests.

Original commit message:

[modules] Fix macro hiding bug exposed if:

 * A submodule of module A is imported into module B
 * Another submodule of module A that is not imported into B exports a macro
 * Some submodule of module B also exports a definition of the macro, and
   happens to be the first submodule of B that imports module A.

In this case, we would incorrectly determine that A's macro redefines B's
macro, and so we don't need to re-export B's macro at all.

This happens with the 'assert' macro in an LLVM self-host. =(

llvm-svn: 213416

clang/include/clang/Serialization/ASTReader.h

@@ -1366,7 +1366,8 @@ public:
                          bool Complain);
 
   /// \brief Make the names within this set of hidden names visible.
-  void makeNamesVisible(const HiddenNames &Names, Module *Owner);
+  void makeNamesVisible(const HiddenNames &Names, Module *Owner,
+                        bool FromFinalization);
 
   /// \brief Set the AST callbacks listener.
   void setListener(ASTReaderListener *listener) {
@@ -1831,7 +1832,7 @@ public:
                                  ModuleFile &M, uint64_t Offset);
 
   void installImportedMacro(IdentifierInfo *II, ModuleMacroInfo *MMI,
-                            Module *Owner);
+                            Module *Owner, bool FromFinalization);
 
   typedef llvm::TinyPtrVector<DefMacroDirective *> AmbiguousMacros;
   llvm::DenseMap<IdentifierInfo*, AmbiguousMacros> AmbiguousMacroDefs;

clang/lib/Lex/MacroInfo.cpp

@@ -232,4 +232,5 @@ void MacroDirective::dump() const {
       Info->dump();
     }
   }
+  Out << "\n";
 }

clang/lib/Serialization/ASTReader.cpp

@@ -1790,7 +1790,7 @@ void ASTReader::resolvePendingMacro(IdentifierInfo *II,
     // install if we make this module visible.
     HiddenNamesMap[Owner].HiddenMacros.insert(std::make_pair(II, MMI));
   } else {
-    installImportedMacro(II, MMI, Owner);
+    installImportedMacro(II, MMI, Owner, /*FromFinalization*/false);
   }
 }
 
@@ -1941,11 +1941,11 @@ ASTReader::removeOverriddenMacros(IdentifierInfo *II,
 }
 
 void ASTReader::installImportedMacro(IdentifierInfo *II, ModuleMacroInfo *MMI,
-                                     Module *Owner) {
+                                     Module *Owner, bool FromFinalization) {
   assert(II && Owner);
 
   SourceLocation ImportLoc = Owner->MacroVisibilityLoc;
-  if (ImportLoc.isInvalid()) {
+  if (ImportLoc.isInvalid() && !FromFinalization) {
     // FIXME: If we made macros from this module visible but didn't provide a
     // source location for the import, we don't have a location for the macro.
     // Use the location at which the containing module file was first imported
@@ -3320,7 +3320,9 @@ static void moveMethodToBackOfGlobalList(Sema &S, ObjCMethodDecl *Method) {
   }
 }
 
-void ASTReader::makeNamesVisible(const HiddenNames &Names, Module *Owner) {
+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];
     bool wasHidden = D->Hidden;
@@ -3333,10 +3335,12 @@ 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);
+    installImportedMacro(I->first, I->second, Owner, FromFinalization);
 }
 
 void ASTReader::makeModuleVisible(Module *Mod,
@@ -3370,7 +3374,8 @@ void ASTReader::makeModuleVisible(Module *Mod,
     // mark them as visible.
     HiddenNamesMapType::iterator Hidden = HiddenNamesMap.find(Mod);
     if (Hidden != HiddenNamesMap.end()) {
-      makeNamesVisible(Hidden->second, Hidden->first);
+      makeNamesVisible(Hidden->second, Hidden->first,
+                       /*FromFinalization*/false);
       HiddenNamesMap.erase(Hidden);
     }
 
@@ -3897,7 +3902,7 @@ void ASTReader::finalizeForWriting() {
   for (HiddenNamesMapType::iterator Hidden = HiddenNamesMap.begin(),
                                  HiddenEnd = HiddenNamesMap.end();
        Hidden != HiddenEnd; ++Hidden) {
-    makeNamesVisible(Hidden->second, Hidden->first);
+    makeNamesVisible(Hidden->second, Hidden->first, /*FromFinalization*/true);
   }
   HiddenNamesMap.clear();
 }

clang/lib/Serialization/ASTWriter.cpp

@@ -1900,6 +1900,12 @@ static bool shouldIgnoreMacro(MacroDirective *MD, bool IsModule,
       return true;
 
   if (IsModule) {
+    // Re-export any imported directives.
+    // FIXME: Also ensure we re-export imported #undef directives.
+    if (auto *DMD = dyn_cast<DefMacroDirective>(MD))
+      if (DMD->isImported())
+        return false;
+
     SourceLocation Loc = MD->getLocation();
     if (Loc.isInvalid())
       return true;
@@ -3089,7 +3095,17 @@ class ASTIdentifierTableTrait {
   }
 
   SubmoduleID getSubmoduleID(MacroDirective *MD) {
-    return Writer.inferSubmoduleIDFromLocation(MD->getLocation());
+    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;
   }
 
 public:

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

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

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

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

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

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

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

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

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

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

clang/test/Modules/macro-hiding.cpp

@@ -0,0 +1,94 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DB2
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DB2 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DB2 -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DB2 -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DB1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DB1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DB1 -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DB1 -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DB1 -DB2
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DB1 -DB2 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DB1 -DB2 -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DB1 -DB2 -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DB2
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DB2 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DB2 -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DB2 -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DB1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DB1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DB1 -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DB1 -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DB1 -DB2
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DB1 -DB2 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DB1 -DB2 -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA2 -DB1 -DB2 -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DB2
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DB2 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DB2 -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DB2 -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DB1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DB1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DB1 -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DB1 -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DB1 -DB2
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DB1 -DB2 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DB1 -DB2 -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DB1 -DB2 -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DB2
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DB2 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DB2 -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DB2 -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DB1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DB1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DB1 -DC1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DB1 -DC1 -DD1
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s -DA1 -DA2 -DB1 -DB2
+// 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
+
+#ifdef A1
+#include "a1.h"
+#endif
+
+#ifdef A2
+#include "a2.h"
+#endif
+
+#ifdef B1
+#include "b1.h"
+#endif
+
+#ifdef B2
+#include "b2.h"
+#endif
+
+#ifdef C1
+#include "c1.h"
+#endif
+
+#ifdef D1
+#include "d1.h"
+#endif
+
+#if defined(A1) || defined(B2) || defined(C1) || defined(D1)
+void h() { assert(true); }
+#else
+void assert() {}
+#endif