Deserialize macro history when we deserialize an identifier that has

macro history.

When deserializing macro history, we arrange history such that the
macros that have definitions (that haven't been #undef'd) and are
visible come at the beginning of the list, which is what the
preprocessor and other clients of Preprocessor::getMacroInfo()
expect. If additional macro definitions become visible later, they'll
be moved toward the front of the list. Note that it's possible to have
ambiguities, but we don't diagnose them yet.

There is a partially-implemented design decision here that, if a
particular identifier has been defined or #undef'd within the
translation unit, that definition (or #undef) hides any macro
definitions that come from imported modules. There's still a little
work to do to ensure that the right #undef'ing happens.

Additionally, we'll need to scope the update records for #undefs, so
they only kick in when the submodule containing that update record
becomes visible.



git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@165682 91177308-0d34-0410-b5e6-96231b3b80d8

12 files changed, 353 insertions, 116 deletions

diff --git a/include/clang/Lex/ExternalPreprocessorSource.h b/include/clang/Lex/ExternalPreprocessorSource.h
index f172b5c8d2..d2e2412192 100644
--- a/include/clang/Lex/ExternalPreprocessorSource.h
+++ b/include/clang/Lex/ExternalPreprocessorSource.h
@@ -28,9 +28,6 @@ public:
   /// \brief Read the set of macros defined by this external macro source.
   virtual void ReadDefinedMacros() = 0;
   
-  /// \brief Read the definition for the given macro.
-  virtual void LoadMacroDefinition(IdentifierInfo *II) = 0;
-  
   /// \brief Update an out-of-date identifier.
   virtual void updateOutOfDateIdentifier(IdentifierInfo &II) = 0;
 };
diff --git a/include/clang/Lex/MacroInfo.h b/include/clang/Lex/MacroInfo.h
index db6cdef290..a7bcfb9db1 100644
--- a/include/clang/Lex/MacroInfo.h
+++ b/include/clang/Lex/MacroInfo.h
@@ -105,7 +105,12 @@ private:
    
   /// \brief Whether the macro has public (when described in a module).
   bool IsPublic : 1;
-  
+
+  /// \brief Whether the macro definition is currently "hidden".
+  /// Note that this is transient state that is never serialized to the AST
+  /// file.
+  bool IsHidden : 1;
+
    ~MacroInfo() {
     assert(ArgumentList == 0 && "Didn't call destroy before dtor!");
   }
@@ -149,10 +154,8 @@ public:
   /// \brief Get the location where macro was undefined.
   SourceLocation getUndefLoc() const { return UndefLocation; }
 
-  /// \brief Set previous definition of the macro with the same name. Can only
-  /// be set once.
+  /// \brief Set previous definition of the macro with the same name.
   void setPreviousDefinition(MacroInfo *PreviousDef) {
-    assert(!PreviousDefinition && "PreviousDefiniton is already set!");
     PreviousDefinition = PreviousDef;
   }
 
@@ -326,7 +329,17 @@ public:
   /// \brief Determine the location where this macro was explicitly made
   /// public or private within its module.
   SourceLocation getVisibilityLocation() { return VisibilityLocation; }
-  
+
+  /// \brief Determine whether this macro is currently defined (and has not
+  /// been #undef'd) or has been hidden.
+  bool isDefined() const { return UndefLocation.isInvalid() && !IsHidden; }
+
+  /// \brief Determine whether this macro definition is hidden.
+  bool isHidden() const { return IsHidden; }
+
+  /// \brief Set whether this macro definition is hidden.
+  void setHidden(bool Val) { IsHidden = Val; }
+
 private:
   unsigned getDefinitionLengthSlow(SourceManager &SM) const;
 };
diff --git a/include/clang/Lex/Preprocessor.h b/include/clang/Lex/Preprocessor.h
index 12b99af6e0..2c0814c910 100644
--- a/include/clang/Lex/Preprocessor.h
+++ b/include/clang/Lex/Preprocessor.h
@@ -517,8 +517,12 @@ public:
   MacroInfo *getMacroInfoHistory(IdentifierInfo *II) const;
 
   /// \brief Specify a macro for this identifier.
-  void setMacroInfo(IdentifierInfo *II, MacroInfo *MI,
-                    bool LoadedFromAST = false);
+  void setMacroInfo(IdentifierInfo *II, MacroInfo *MI);
+  /// \brief Add a MacroInfo that was loaded from an AST file.
+  void addLoadedMacroInfo(IdentifierInfo *II, MacroInfo *MI);
+  /// \brief Make the given MacroInfo, that was loaded from an AST file and
+  /// previously hidden, visible.
+  void makeLoadedMacroInfoVisible(IdentifierInfo *II, MacroInfo *MI);
   /// \brief Undefine a macro for this identifier.
   void clearMacroInfo(IdentifierInfo *II);
 
diff --git a/include/clang/Serialization/ASTReader.h b/include/clang/Serialization/ASTReader.h
index c7e3bb8ba9..3a938a24c7 100644
--- a/include/clang/Serialization/ASTReader.h
+++ b/include/clang/Serialization/ASTReader.h
@@ -415,8 +415,35 @@ private:
   /// global submodule ID to produce a local ID.
   GlobalSubmoduleMapType GlobalSubmoduleMap;
 
+  /// \brief An entity that has been hidden.
+  class HiddenName {
+    /// \brief The hidden declaration or macro.
+    llvm::PointerUnion<Decl *, MacroInfo *> DeclOrMacro;
+
+    /// \brief The name being defined to a macro, for the macro case.
+    IdentifierInfo *Identifier;
+
+  public:
+    HiddenName(Decl *D) : DeclOrMacro(D), Identifier() { }
+    HiddenName(IdentifierInfo *II, MacroInfo *MI)
+      : DeclOrMacro(MI), Identifier(II) { }
+
+    bool isDecl() const { return DeclOrMacro.is<Decl*>(); }
+    bool isMacro() const { return !isDecl(); }
+
+    Decl *getDecl() const {
+      assert(isDecl() && "Hidden name is not a declaration");
+      return DeclOrMacro.get<Decl *>();
+    }
+
+    std::pair<IdentifierInfo *, MacroInfo *> getMacro() const {
+      assert(isMacro() && "Hidden name is not a macro!");
+      return std::make_pair(Identifier, DeclOrMacro.get<MacroInfo *>());
+    }
+  };
+
   /// \brief A set of hidden declarations.
-  typedef llvm::SmallVector<llvm::PointerUnion<Decl *, IdentifierInfo *>, 2>
+  typedef llvm::SmallVector<HiddenName, 2>
     HiddenNames;
   
   typedef llvm::DenseMap<Module *, HiddenNames> HiddenNamesMapType;
@@ -468,9 +495,13 @@ private:
   /// global method pool for this selector.
   llvm::DenseMap<Selector, unsigned> SelectorGeneration;
 
-  /// \brief Mapping from identifiers that represent macros whose definitions
-  /// have not yet been deserialized to the global ID of the macro.
-  llvm::DenseMap<IdentifierInfo *, serialization::MacroID> UnreadMacroIDs;
+  typedef llvm::DenseMap<IdentifierInfo *,
+                         llvm::SmallVector<serialization::MacroID, 2> >
+    PendingMacroIDsMap;
+
+  /// \brief Mapping from identifiers that have a macro history to the global
+  /// IDs have not yet been deserialized to the global IDs of those macros.
+  PendingMacroIDsMap PendingMacroIDs;
 
   typedef ContinuousRangeMap<unsigned, ModuleFile *, 4>
     GlobalPreprocessedEntityMapType;
@@ -1390,6 +1421,9 @@ public:
   }
 
   virtual IdentifierInfo *GetIdentifier(serialization::IdentifierID ID) {
+    // Note that we are loading an identifier.
+    Deserializing AnIdentifier(this);
+
     return DecodeIdentifierInfo(ID);
   }
 
@@ -1555,23 +1589,17 @@ public:
   serialization::PreprocessedEntityID
   getGlobalPreprocessedEntityID(ModuleFile &M, unsigned LocalID) const;
 
-  /// \brief Note that the identifier is a macro whose record will be loaded
-  /// from the given AST file at the given (file-local) offset.
+  /// \brief Note that the identifier has a macro history.
   ///
   /// \param II The name of the macro.
   ///
-  /// \param ID The global macro ID.
-  ///
-  /// \param Visible Whether the macro should be made visible.
-  void setIdentifierIsMacro(IdentifierInfo *II, serialization::MacroID ID,
-                            bool Visible);
+  /// \param IDs The global macro IDs that are associated with this identifier.
+  void setIdentifierIsMacro(IdentifierInfo *II,
+                            ArrayRef<serialization::MacroID> IDs);
 
   /// \brief Read the set of macros defined by this external macro source.
   virtual void ReadDefinedMacros();
 
-  /// \brief Read the macro definition for this identifier.
-  virtual void LoadMacroDefinition(IdentifierInfo *II);
-
   /// \brief Update an out-of-date identifier.
   virtual void updateOutOfDateIdentifier(IdentifierInfo &II);
 
@@ -1580,8 +1608,7 @@ public:
 
   /// \brief Read the macro definition corresponding to this iterator
   /// into the unread macro record offsets table.
-  void LoadMacroDefinition(
-         llvm::DenseMap<IdentifierInfo *,serialization::MacroID>::iterator Pos);
+  void LoadMacroDefinition(PendingMacroIDsMap::iterator Pos);
 
   /// \brief Load all external visible decls in the given DeclContext.
   void completeVisibleDeclsMap(const DeclContext *DC);
diff --git a/lib/Lex/MacroInfo.cpp b/lib/Lex/MacroInfo.cpp
index ffe31f24d3..7e538fabf4 100644
--- a/lib/Lex/MacroInfo.cpp
+++ b/lib/Lex/MacroInfo.cpp
@@ -31,7 +31,8 @@ MacroInfo::MacroInfo(SourceLocation DefLoc)
     IsUsed(false),
     IsAllowRedefinitionsWithoutWarning(false),
     IsWarnIfUnused(false),
-    IsPublic(true) {
+    IsPublic(true),
+    IsHidden(false) {
 }
 
 MacroInfo::MacroInfo(const MacroInfo &MI, llvm::BumpPtrAllocator &PPAllocator)
@@ -54,7 +55,8 @@ MacroInfo::MacroInfo(const MacroInfo &MI, llvm::BumpPtrAllocator &PPAllocator)
     IsUsed(MI.IsUsed),
     IsAllowRedefinitionsWithoutWarning(MI.IsAllowRedefinitionsWithoutWarning),
     IsWarnIfUnused(MI.IsWarnIfUnused),
-    IsPublic(MI.IsPublic) {
+    IsPublic(MI.IsPublic),
+    IsHidden(MI.IsHidden) {
   setArgumentList(MI.ArgumentList, MI.NumArguments, PPAllocator);
 }
 
diff --git a/lib/Lex/PPMacroExpansion.cpp b/lib/Lex/PPMacroExpansion.cpp
index 1ef534daa7..013178c555 100644
--- a/lib/Lex/PPMacroExpansion.cpp
+++ b/lib/Lex/PPMacroExpansion.cpp
@@ -36,29 +36,106 @@ MacroInfo *Preprocessor::getMacroInfoHistory(IdentifierInfo *II) const {
   assert(II->hadMacroDefinition() && "Identifier has not been not a macro!");
 
   macro_iterator Pos = Macros.find(II);
-  if (Pos == Macros.end()) {
-    // Load this macro from the external source.
-    getExternalSource()->LoadMacroDefinition(II);
-    Pos = Macros.find(II);
-  }
   assert(Pos != Macros.end() && "Identifier macro info is missing!");
   return Pos->second;
 }
 
 /// setMacroInfo - Specify a macro for this identifier.
 ///
-void Preprocessor::setMacroInfo(IdentifierInfo *II, MacroInfo *MI,
-                                bool LoadedFromAST) {
+void Preprocessor::setMacroInfo(IdentifierInfo *II, MacroInfo *MI) {
   assert(MI && "MacroInfo should be non-zero!");
-  assert((LoadedFromAST || MI->getUndefLoc().isInvalid()) &&
-         "Undefined macros can only be registered when just LoadedFromAST");
-  MI->setPreviousDefinition(Macros[II]);
-  Macros[II] = MI;
+  assert(MI->getUndefLoc().isInvalid() &&
+         "Undefined macros cannot be registered");
+
+  MacroInfo *&StoredMI = Macros[II];
+  MI->setPreviousDefinition(StoredMI);
+  StoredMI = MI;
   II->setHasMacroDefinition(MI->getUndefLoc().isInvalid());
-  if (II->isFromAST() && !LoadedFromAST)
+  if (II->isFromAST())
     II->setChangedSinceDeserialization();
 }
 
+void Preprocessor::addLoadedMacroInfo(IdentifierInfo *II, MacroInfo *MI) {
+  assert(MI && "Missing macro?");
+  assert(MI->isFromAST() && "Macro is not from an AST?");
+  assert(!MI->getPreviousDefinition() && "Macro already in chain?");
+  
+  MacroInfo *&StoredMI = Macros[II];
+
+  // Easy case: this is the first macro definition for this macro.
+  if (!StoredMI) {
+    StoredMI = MI;
+
+    if (MI->isDefined())
+      II->setHasMacroDefinition(true);
+    return;
+  }
+
+  // If this macro is a definition and this identifier has been neither
+  // defined nor undef'd in the current translation unit, add this macro
+  // to the end of the chain of definitions.
+  if (MI->isDefined() && StoredMI->isFromAST()) {
+    // Simple case: if this is the first actual definition, just put it at
+    // th beginning.
+    if (!StoredMI->isDefined()) {
+      MI->setPreviousDefinition(StoredMI);
+      StoredMI = MI;
+
+      II->setHasMacroDefinition(true);
+      return;
+    }
+
+    // Find the end of the definition chain.
+    MacroInfo *Prev = StoredMI;
+    MacroInfo *PrevPrev;
+    bool Ambiguous = false;
+    do {
+      // If the macros are not identical, we have an ambiguity.
+      if (!Prev->isIdenticalTo(*MI, *this))
+        Ambiguous = true;
+    } while ((PrevPrev = Prev->getPreviousDefinition()) &&
+             PrevPrev->isDefined());
+
+    // FIXME: Actually use the ambiguity information for something.
+    
+    // Wire this macro information into the chain.
+    MI->setPreviousDefinition(Prev->getPreviousDefinition());
+    Prev->setPreviousDefinition(MI);
+    return;
+  }
+
+  // The macro is not a definition; put it at the end of the list.
+  // FIXME: Adding macro history is quadratic, but a hint could fix this.
+  MacroInfo *Prev = StoredMI;
+  while (Prev->getPreviousDefinition())
+    Prev = Prev->getPreviousDefinition();
+  Prev->setPreviousDefinition(MI);
+}
+
+void Preprocessor::makeLoadedMacroInfoVisible(IdentifierInfo *II,
+                                              MacroInfo *MI) {
+  assert(MI->isFromAST() && "Macro must be from the AST");
+  assert(MI->isDefined() && "Macro is not visible");
+
+  MacroInfo *&StoredMI = Macros[II];
+  if (StoredMI == MI) {
+    // Easy case: this is the first macro anyway.
+    II->setHasMacroDefinition(true);
+    return;
+  }
+
+  // Go find the macro and pull it out of the list.
+  // FIXME: Yes, this is O(N), and making a pile of macros visible would be
+  // quadratic.
+  MacroInfo *Prev = StoredMI;
+  while (Prev->getPreviousDefinition() != MI)
+    Prev = Prev->getPreviousDefinition();
+  Prev->setPreviousDefinition(MI->getPreviousDefinition());
+
+  // Add the macro back to the list.
+  addLoadedMacroInfo(II, MI);
+}
+
 /// \brief Undefine a macro for this identifier.
 void Preprocessor::clearMacroInfo(IdentifierInfo *II) {
   assert(II->hasMacroDefinition() && "Macro is not defined!");
diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp
index 4e6673bedd..6e3eee9533 100644
--- a/lib/Serialization/ASTReader.cpp
+++ b/lib/Serialization/ASTReader.cpp
@@ -573,31 +573,13 @@ IdentifierInfo *ASTIdentifierLookupTrait::ReadData(const internal_key_type& k,
   // If this identifier is a macro, deserialize the macro
   // definition.
   if (hadMacroDefinition) {
-    // FIXME: Check for conflicts?
-    uint32_t LocalID = ReadUnalignedLE32(d);
-    unsigned LocalSubmoduleID = ReadUnalignedLE32(d);
-    
-    // Determine whether this macro definition should be visible now, or
-    // whether it is in a hidden submodule.
-    bool Visible = true;
-    if (SubmoduleID GlobalSubmoduleID
-          = Reader.getGlobalSubmoduleID(F, LocalSubmoduleID)) {
-      if (Module *Owner = Reader.getSubmodule(GlobalSubmoduleID)) {
-        if (Owner->NameVisibility == Module::Hidden) {
-          // The owning module is not visible, and this macro definition should
-          // not be, either.
-          Visible = false;
-          
-          // Note that this macro definition was hidden because its owning 
-          // module is not yet visible.
-          Reader.HiddenNamesMap[Owner].push_back(II);
-        }
-      }
+    SmallVector<MacroID, 4> MacroIDs;
+    while (uint32_t LocalID = ReadUnalignedLE32(d)) {
+      MacroIDs.push_back(Reader.getGlobalMacroID(F, LocalID));
+      DataLen -= 4;
     }
-
-    Reader.setIdentifierIsMacro(II, Reader.getGlobalMacroID(F, LocalID),
-                                Visible && hasMacroDefinition);
-    DataLen -= 8;
+    DataLen -= 4;
+    Reader.setIdentifierIsMacro(II, MacroIDs);
   }
 
   Reader.SetIdentifierInfo(ID, II);
@@ -1322,7 +1304,8 @@ void ASTReader::ReadMacroRecord(ModuleFile &F, uint64_t Offset) {
       if (MacrosLoaded[GlobalID - NUM_PREDEF_MACRO_IDS])
         return;
 
-      unsigned NextIndex = 2;
+      SubmoduleID GlobalSubmoduleID = getGlobalSubmoduleID(F, Record[2]);
+      unsigned NextIndex = 3;
       SourceLocation Loc = ReadSourceLocation(F, Record, NextIndex);
       MacroInfo *MI = PP.AllocateMacroInfo(Loc);
 
@@ -1360,6 +1343,7 @@ void ASTReader::ReadMacroRecord(ModuleFile &F, uint64_t Offset) {
         DeserializationListener->MacroRead(GlobalID, MI);
 
       // If an update record marked this as undefined, do so now.
+      // FIXME: Only if the submodule this update came from is visible?
       MacroUpdatesMap::iterator Update = MacroUpdates.find(GlobalID);
       if (Update != MacroUpdates.end()) {
         if (MI->getUndefLoc().isInvalid()) {
@@ -1370,8 +1354,25 @@ void ASTReader::ReadMacroRecord(ModuleFile &F, uint64_t Offset) {
         MacroUpdates.erase(Update);
       }
 
+      // Determine whether this macro definition is visible.
+      bool Hidden = !MI->isPublic();
+      if (!Hidden && GlobalSubmoduleID) {
+        if (Module *Owner = getSubmodule(GlobalSubmoduleID)) {
+          if (Owner->NameVisibility == Module::Hidden) {
+            // The owning module is not visible, and this macro definition
+            // should not be, either.
+            Hidden = true;
+
+            // Note that this macro definition was hidden because its owning
+            // module is not yet visible.
+            HiddenNamesMap[Owner].push_back(HiddenName(II, MI));
+          }
+        }
+      }
+      MI->setHidden(Hidden);
+
       // Finally, install the macro.
-      PP.setMacroInfo(II, MI, /*LoadedFromAST=*/true);
+      PP.addLoadedMacroInfo(II, MI);
 
       // Remember that we saw this macro last so that we add the tokens that
       // form its body to it.
@@ -1480,21 +1481,16 @@ HeaderFileInfoTrait::ReadData(const internal_key_type, const unsigned char *d,
   return HFI;
 }
 
-void ASTReader::setIdentifierIsMacro(IdentifierInfo *II, MacroID ID,
-                                     bool Visible) {
-  if (Visible) {
-    // Note that this identifier has a macro definition.
-    II->setHasMacroDefinition(true);
-  } else {
-    II->setHadMacroDefinition(true);
-  }
-
-  // FIXME: This could end up overwriting a previously recording macro
-  // definition here, which is not cool at all.
-  UnreadMacroIDs[II] = ID;
+void ASTReader::setIdentifierIsMacro(IdentifierInfo *II, ArrayRef<MacroID> IDs){
+  II->setHadMacroDefinition(true);
+  assert(NumCurrentElementsDeserializing > 0 &&"Missing deserialization guard");
+  PendingMacroIDs[II].append(IDs.begin(), IDs.end());
 }
 
 void ASTReader::ReadDefinedMacros() {
+  // Note that we are loading defined macros.
+  Deserializing Macros(this);
+
   for (ModuleReverseIterator I = ModuleMgr.rbegin(),
       E = ModuleMgr.rend(); I != E; ++I) {
     llvm::BitstreamCursor &MacroCursor = (*I)->MacroCursor;
@@ -1546,24 +1542,14 @@ void ASTReader::ReadDefinedMacros() {
       }
     }
   }
-  
-  // Drain the unread macro-record offsets map.
-  while (!UnreadMacroIDs.empty())
-    LoadMacroDefinition(UnreadMacroIDs.begin());
 }
 
-void ASTReader::LoadMacroDefinition(
-                    llvm::DenseMap<IdentifierInfo *, MacroID>::iterator Pos) {
-  assert(Pos != UnreadMacroIDs.end() && "Unknown macro definition");
-  uint64_t GlobalID = Pos->second;
-  UnreadMacroIDs.erase(Pos);
-  getMacro(GlobalID);
-}
-
-void ASTReader::LoadMacroDefinition(IdentifierInfo *II) {
-  llvm::DenseMap<IdentifierInfo *, MacroID>::iterator Pos
-    = UnreadMacroIDs.find(II);
-  LoadMacroDefinition(Pos);
+void ASTReader::LoadMacroDefinition(PendingMacroIDsMap::iterator Pos) {
+  assert(Pos != PendingMacroIDs.end() && "Unknown macro definition");
+  SmallVector<MacroID, 2> GlobalIDs = Pos->second;
+  PendingMacroIDs.erase(Pos);
+  for (unsigned I = 0, N = GlobalIDs.size(); I != N; ++I)
+    getMacro(GlobalIDs[I]);
 }
 
 namespace {
@@ -1612,6 +1598,9 @@ namespace {
 }
 
 void ASTReader::updateOutOfDateIdentifier(IdentifierInfo &II) {
+  // Note that we are loading an identifier.
+  Deserializing AnIdentifier(this);
+
   unsigned PriorGeneration = 0;
   if (getContext().getLangOpts().Modules)
     PriorGeneration = IdentifierGeneration[&II];
@@ -2620,22 +2609,17 @@ ASTReader::ASTReadResult ASTReader::validateFileEntries(ModuleFile &M) {
 
 void ASTReader::makeNamesVisible(const HiddenNames &Names) {
   for (unsigned I = 0, N = Names.size(); I != N; ++I) {
-    if (Decl *D = Names[I].dyn_cast<Decl *>())
-      D->Hidden = false;
-    else {
-      IdentifierInfo *II = Names[I].get<IdentifierInfo *>();
-      // FIXME: Check if this works correctly with macro history.
-      if (!II->hasMacroDefinition()) {
-        // Make sure that this macro hasn't been #undef'd in the mean-time.
-        llvm::DenseMap<IdentifierInfo*, MacroInfo*>::iterator Known
-          = PP.Macros.find(II);
-        if (Known == PP.Macros.end() ||
-            Known->second->getUndefLoc().isInvalid()) {
-          II->setHasMacroDefinition(true);
-          if (DeserializationListener)
-            DeserializationListener->MacroVisible(II);
-        }
-      }
+    if (Names[I].isDecl()) {
+      Names[I].getDecl()->Hidden = false;
+      continue;
+    }
+
+    std::pair<IdentifierInfo *, MacroInfo *> Macro = Names[I].getMacro();
+    Macro.second->setHidden(!Macro.second->isPublic());
+    if (Macro.second->isDefined()) {
+      PP.makeLoadedMacroInfoVisible(Macro.first, Macro.second);
+      if (DeserializationListener)
+        DeserializationListener->MacroVisible(Macro.first);
     }
   }
 }
@@ -5418,6 +5402,9 @@ void ASTReader::InitializeSema(Sema &S) {
 }
 
 IdentifierInfo* ASTReader::get(const char *NameStart, const char *NameEnd) {
+  // Note that we are loading an identifier.
+  Deserializing AnIdentifier(this);
+  
   IdentifierLookupVisitor Visitor(StringRef(NameStart, NameEnd - NameStart),
                                   /*PriorGeneration=*/0);
   ModuleMgr.visit(IdentifierLookupVisitor::visit, &Visitor);
@@ -6516,7 +6503,8 @@ void ASTReader::ReadComments() {
 }
 
 void ASTReader::finishPendingActions() {
-  while (!PendingIdentifierInfos.empty() || !PendingDeclChains.empty()) {
+  while (!PendingIdentifierInfos.empty() || !PendingDeclChains.empty() ||
+         !PendingMacroIDs.empty()) {
     // If any identifiers with corresponding top-level declarations have
     // been loaded, load those declarations now.
     while (!PendingIdentifierInfos.empty()) {
@@ -6531,6 +6519,11 @@ void ASTReader::finishPendingActions() {
       PendingDeclChainsKnown.erase(PendingDeclChains[I]);
     }
     PendingDeclChains.clear();
+
+    // Load any pending macro definitions.
+    // FIXME: Non-determinism here.
+    while (!PendingMacroIDs.empty())
+      LoadMacroDefinition(PendingMacroIDs.begin());
   }
   
   // If we deserialized any C++ or Objective-C class definitions, any
diff --git a/lib/Serialization/ASTWriter.cpp b/lib/Serialization/ASTWriter.cpp
index 76eb1a1a8a..98e841cb2b 100644
--- a/lib/Serialization/ASTWriter.cpp
+++ b/lib/Serialization/ASTWriter.cpp
@@ -1744,6 +1744,7 @@ void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule) {
 
       AddIdentifierRef(Name, Record);
       addMacroRef(MI, Record);
+      Record.push_back(inferSubmoduleIDFromLocation(MI->getDefinitionLoc()));
       AddSourceLocation(MI->getDefinitionLoc(), Record);
       AddSourceLocation(MI->getUndefLoc(), Record);
       Record.push_back(MI->isUsed());
@@ -2587,8 +2588,14 @@ public:
     if (isInterestingIdentifier(II, Macro)) {
       DataLen += 2; // 2 bytes for builtin ID
       DataLen += 2; // 2 bytes for flags
-      if (hadMacroDefinition(II, Macro))
-        DataLen += 8;
+      if (hadMacroDefinition(II, Macro)) {
+        for (MacroInfo *M = Macro; M; M = M->getPreviousDefinition()) {
+          if (Writer.getMacroRef(M) != 0)
+            DataLen += 4;
+        }
+
+        DataLen += 4;
+      }
 
       for (IdentifierResolver::iterator D = IdResolver.begin(II),
                                      DEnd = IdResolver.end();
@@ -2635,9 +2642,13 @@ public:
     clang::io::Emit16(Out, Bits);
 
     if (HadMacroDefinition) {
-      clang::io::Emit32(Out, Writer.getMacroRef(Macro));
-      clang::io::Emit32(Out,
-        Writer.inferSubmoduleIDFromLocation(Macro->getDefinitionLoc()));
+      // Write all of the macro IDs associated with this identifier.
+      for (MacroInfo *M = Macro; M; M = M->getPreviousDefinition()) {
+        if (MacroID ID = Writer.getMacroRef(M))
+          clang::io::Emit32(Out, ID);
+      }
+
+      clang::io::Emit32(Out, 0);
     }
 
     // Emit the declaration IDs in reverse order, because the
diff --git a/test/Modules/Inputs/macros_left.h b/test/Modules/Inputs/macros_left.h
index dfe56052bf..919dc20ebc 100644
--- a/test/Modules/Inputs/macros_left.h
+++ b/test/Modules/Inputs/macros_left.h
@@ -3,3 +3,10 @@
 
 #undef TOP_LEFT_UNDEF
 
+
+
+
+#define LEFT_RIGHT_IDENTICAL int
+#define LEFT_RIGHT_DIFFERENT float
+#define LEFT_RIGHT_DIFFERENT2 float
+#define LEFT_RIGHT_DIFFERENT3 float
diff --git a/test/Modules/Inputs/macros_right.h b/test/Modules/Inputs/macros_right.h
index 3351e9796c..8e9688223b 100644
--- a/test/Modules/Inputs/macros_right.h
+++ b/test/Modules/Inputs/macros_right.h
@@ -4,3 +4,12 @@
 #undef TOP_RIGHT_REDEF
 #define TOP_RIGHT_REDEF float
 
+
+
+
+
+#define LEFT_RIGHT_IDENTICAL int
+#define LEFT_RIGHT_DIFFERENT int
+#define LEFT_RIGHT_DIFFERENT2 int
+#define LEFT_RIGHT_DIFFERENT3 int
+
diff --git a/test/Modules/Inputs/module.map b/test/Modules/Inputs/module.map
index 9044922799..b26234382b 100644
--- a/test/Modules/Inputs/module.map
+++ b/test/Modules/Inputs/module.map
@@ -18,6 +18,17 @@ module lookup_left_cxx { header "lookup_left.hpp" }
 module lookup_right_cxx { header "lookup_right.hpp" }
 module module_private_left { header "module_private_left.h" }
 module module_private_right { header "module_private_right.h" }
+module macros_top { 
+  header "macros_top.h" 
+}
+module macros_left { 
+  header "macros_left.h" 
+  export *
+}
+module macros_right { 
+  header "macros_right.h" 
+  export *
+}
 module macros { header "macros.h" }
 module category_top { header "category_top.h" }
 module category_left { 
diff --git a/test/Modules/macros.c b/test/Modules/macros.c
index d93ce35a03..168e2c6fce 100644
--- a/test/Modules/macros.c
+++ b/test/Modules/macros.c
@@ -1,8 +1,14 @@
 // RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -x objective-c -emit-module -fmodule-cache-path %t -fmodule-name=macros_top %S/Inputs/module.map
+// RUN: %clang_cc1 -fmodules -x objective-c -emit-module -fmodule-cache-path %t -fmodule-name=macros_left %S/Inputs/module.map
+// RUN: %clang_cc1 -fmodules -x objective-c -emit-module -fmodule-cache-path %t -fmodule-name=macros_right %S/Inputs/module.map
 // RUN: %clang_cc1 -fmodules -x objective-c -emit-module -fmodule-cache-path %t -fmodule-name=macros %S/Inputs/module.map
 // RUN: %clang_cc1 -fmodules -x objective-c -verify -fmodule-cache-path %t %s
 // RUN: %clang_cc1 -E -fmodules -x objective-c -fmodule-cache-path %t %s | FileCheck -check-prefix CHECK-PREPROCESSED %s
 // FIXME: When we have a syntax for modules in C, use that.
+// These notes come from headers in modules, and are bogus.
+// FIXME: expected-note{{previous definition is here}}
+// FIXME: expected-note{{previous definition is here}}
 
 @__experimental_modules_import macros;
 
@@ -37,3 +43,83 @@ void f() {
 #if __building_module(macros)
 # error Not building a module
 #endif
+
+// None of the modules we depend on have been imported, and therefore
+// their macros should not be visible.
+#ifdef LEFT
+#  error LEFT should not be visible
+#endif
+
+#ifdef RIGHT
+#  error RIGHT should not be visible
+#endif
+
+#ifdef TOP
+#  error TOP should not be visible
+#endif
+
+// Import left module (which also imports top)
+@__experimental_modules_import macros_left;
+
+#ifndef LEFT
+#  error LEFT should be visible
+#endif
+
+#ifdef RIGHT
+#  error RIGHT should not be visible
+#endif
+
+#ifndef TOP
+#  error TOP should be visible
+#endif
+
+#ifdef TOP_LEFT_UNDEF
+#  error TOP_LEFT_UNDEF should not be visible
+#endif
+
+void test1() {
+  int i;
+  TOP_RIGHT_REDEF *ip = &i;
+}
+
+#define LEFT_RIGHT_DIFFERENT2 double // FIXME: expected-warning{{'LEFT_RIGHT_DIFFERENT2' macro redefined}}
+
+// Import right module (which also imports top)
+@__experimental_modules_import macros_right;
+
+#undef LEFT_RIGHT_DIFFERENT3
+
+#ifndef LEFT
+#  error LEFT should be visible
+#endif
+
+#ifndef RIGHT
+#  error RIGHT should be visible
+#endif
+
+#ifndef TOP
+#  error TOP should be visible
+#endif
+
+#ifndef TOP_LEFT_UNDEF
+#  error TOP_LEFT_UNDEF should be visible
+#endif
+
+void test2() {
+  int i;
+  float f;
+  double d;
+  TOP_RIGHT_REDEF *ip = &i; // FIXME: warning
+  
+  LEFT_RIGHT_IDENTICAL *ip2 = &i;
+  LEFT_RIGHT_DIFFERENT *fp = &f; // FIXME: warning
+  LEFT_RIGHT_DIFFERENT2 *dp = &d; // okay
+  int LEFT_RIGHT_DIFFERENT3;
+}
+
+#define LEFT_RIGHT_DIFFERENT double // FIXME: expected-warning{{'LEFT_RIGHT_DIFFERENT' macro redefined}}
+
+void test3() {
+  double d;
+  LEFT_RIGHT_DIFFERENT *dp = &d; // okay
+}