[PCH/Modules] Change how macro [re]definitions are de/serialized.

Previously we would serialize the macro redefinitions as a list, part of
the identifier, and try to chain them together across modules individually
without having the info that they were already chained at definition time.

Change this by serializing the macro redefinition chain and then try
to synthesize the chain parts across modules. This allows us to correctly
pinpoint when 2 different definitions are ambiguous because they came from
unrelated modules.

Fixes bogus "ambiguous expansion of macro" warning when a macro in a PCH
is redefined without undef'ing it first.

rdar://13016031

llvm-svn: 172620

clang/include/clang/Lex/MacroInfo.h

@@ -169,6 +169,14 @@ public:
   /// \brief Get previous definition of the macro with the same name.
   MacroInfo *getPreviousDefinition() { return PreviousDefinition; }
 
+  /// \brief Get the first definition in the chain.
+  MacroInfo *getFirstDefinition() {
+    MacroInfo *MI = this;
+    while (MI->PreviousDefinition)
+      MI = MI->PreviousDefinition;
+    return MI;
+  }
+
   /// \brief Find macro definition active in the specified source location. If
   /// this macro was not defined there, return NULL.
   const MacroInfo *findDefinitionAtLoc(SourceLocation L,

clang/include/clang/Lex/Preprocessor.h

@@ -525,8 +525,7 @@ public:
   /// \brief Specify a macro for this identifier.
   void setMacroInfo(IdentifierInfo *II, MacroInfo *MI);
   /// \brief Add a MacroInfo that was loaded from an AST file.
-  void addLoadedMacroInfo(IdentifierInfo *II, MacroInfo *MI,
-                          MacroInfo *Hint = 0);
+  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);

clang/include/clang/Serialization/ASTReader.h

@@ -1535,7 +1535,7 @@ public:
                                                     unsigned LocalID);
 
   /// \brief Retrieve the macro with the given ID.
-  MacroInfo *getMacro(serialization::MacroID ID, MacroInfo *Hint = 0);
+  MacroInfo *getMacro(serialization::MacroID ID);
 
   /// \brief Retrieve the global macro ID corresponding to the given local
   /// ID within the given module file.
@@ -1693,20 +1693,19 @@ public:
   Expr *ReadSubExpr();
 
   /// \brief Reads the macro record located at the given offset.
-  void ReadMacroRecord(ModuleFile &F, uint64_t Offset, MacroInfo *Hint = 0);
+  void ReadMacroRecord(ModuleFile &F, uint64_t Offset);
 
   /// \brief Determine the global preprocessed entity ID that corresponds to
   /// the given local ID within the given module.
   serialization::PreprocessedEntityID
   getGlobalPreprocessedEntityID(ModuleFile &M, unsigned LocalID) const;
 
-  /// \brief Note that the identifier has a macro history.
+  /// \brief Add the macro ID assosiated with \c II for deserialization.
   ///
   /// \param II The name of the macro.
-  ///
-  /// \param IDs The global macro IDs that are associated with this identifier.
-  void setIdentifierIsMacro(IdentifierInfo *II,
-                            ArrayRef<serialization::MacroID> IDs);
+  /// \param ID The global macro ID that is associated with this identifier.
+  void addMacroIDForDeserialization(IdentifierInfo *II,
+                                    serialization::MacroID ID);
 
   /// \brief Read the set of macros defined by this external macro source.
   virtual void ReadDefinedMacros();

clang/lib/Lex/PPMacroExpansion.cpp

@@ -55,11 +55,9 @@ void Preprocessor::setMacroInfo(IdentifierInfo *II, MacroInfo *MI) {
     II->setChangedSinceDeserialization();
 }
 
-void Preprocessor::addLoadedMacroInfo(IdentifierInfo *II, MacroInfo *MI,
-                                      MacroInfo *Hint) {
+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];
 
@@ -79,7 +77,7 @@ void Preprocessor::addLoadedMacroInfo(IdentifierInfo *II, MacroInfo *MI,
     // Simple case: if this is the first actual definition, just put it at
     // th beginning.
     if (!StoredMI->isDefined()) {
-      MI->setPreviousDefinition(StoredMI);
+      MI->getFirstDefinition()->setPreviousDefinition(StoredMI);
       StoredMI = MI;
 
       II->setHasMacroDefinition(true);
@@ -112,16 +110,14 @@ void Preprocessor::addLoadedMacroInfo(IdentifierInfo *II, MacroInfo *MI,
       MI->setAmbiguous(true);
 
     // Wire this macro information into the chain.
-    MI->setPreviousDefinition(Prev->getPreviousDefinition());
+    MI->getFirstDefinition()->setPreviousDefinition(
+                                                 Prev->getPreviousDefinition());
     Prev->setPreviousDefinition(MI);
     return;
   }
 
   // The macro is not a definition; put it at the end of the list.
-  MacroInfo *Prev = Hint? Hint : StoredMI;
-  while (Prev->getPreviousDefinition())
-    Prev = Prev->getPreviousDefinition();
-  Prev->setPreviousDefinition(MI);
+  StoredMI->getFirstDefinition()->setPreviousDefinition(MI);
 }
 
 void Preprocessor::makeLoadedMacroInfoVisible(IdentifierInfo *II,

clang/lib/Serialization/ASTReader.cpp

@@ -525,13 +525,9 @@ IdentifierInfo *ASTIdentifierLookupTrait::ReadData(const internal_key_type& k,
   // If this identifier is a macro, deserialize the macro
   // definition.
   if (hadMacroDefinition) {
-    SmallVector<MacroID, 4> MacroIDs;
-    while (uint32_t LocalID = ReadUnalignedLE32(d)) {
-      MacroIDs.push_back(Reader.getGlobalMacroID(F, LocalID));
-      DataLen -= 4;
-    }
     DataLen -= 4;
-    Reader.setIdentifierIsMacro(II, MacroIDs);
+    uint32_t LocalID = ReadUnalignedLE32(d);
+    Reader.addMacroIDForDeserialization(II, Reader.getGlobalMacroID(F,LocalID));
   }
 
   Reader.SetIdentifierInfo(ID, II);
@@ -1061,8 +1057,7 @@ bool ASTReader::ReadBlockAbbrevs(llvm::BitstreamCursor &Cursor,
   }
 }
 
-void ASTReader::ReadMacroRecord(ModuleFile &F, uint64_t Offset,
-                                MacroInfo *Hint) {
+void ASTReader::ReadMacroRecord(ModuleFile &F, uint64_t Offset) {
   llvm::BitstreamCursor &Stream = F.MacroCursor;
 
   // Keep track of where we are in the stream, then jump back there
@@ -1074,24 +1069,6 @@ void ASTReader::ReadMacroRecord(ModuleFile &F, uint64_t Offset,
   SmallVector<IdentifierInfo*, 16> MacroArgs;
   MacroInfo *Macro = 0;
 
-  // RAII object to add the loaded macro information once we're done
-  // adding tokens.
-  struct AddLoadedMacroInfoRAII {
-    Preprocessor &PP;
-    MacroInfo *Hint;
-    MacroInfo *MI;
-    IdentifierInfo *II;
-
-    AddLoadedMacroInfoRAII(Preprocessor &PP, MacroInfo *Hint)
-      : PP(PP), Hint(Hint), MI(), II() { }
-    ~AddLoadedMacroInfoRAII( ) {
-      if (MI) {
-        // Finally, install the macro.
-        PP.addLoadedMacroInfo(II, MI, Hint);
-      }
-    }
-  } AddLoadedMacroInfo(PP, Hint);
-
   while (true) {
     unsigned Code = Stream.ReadCode();
     switch (Code) {
@@ -1146,6 +1123,8 @@ void ASTReader::ReadMacroRecord(ModuleFile &F, uint64_t Offset,
       SourceLocation Loc = ReadSourceLocation(F, Record, NextIndex);
       MacroInfo *MI = PP.AllocateMacroInfo(Loc);
       MI->setDefinitionEndLoc(ReadSourceLocation(F, Record, NextIndex));
+      MacroInfo *PrevMI = getMacro(getGlobalMacroID(F, Record[NextIndex++]));
+      MI->setPreviousDefinition(PrevMI);
 
       // Record this macro.
       MacrosLoaded[GlobalID - NUM_PREDEF_MACRO_IDS] = MI;
@@ -1230,10 +1209,6 @@ void ASTReader::ReadMacroRecord(ModuleFile &F, uint64_t Offset,
       }
       MI->setHidden(Hidden);
 
-      // Make sure we install the macro once we're done.
-      AddLoadedMacroInfo.MI = MI;
-      AddLoadedMacroInfo.II = II;
-
       // Remember that we saw this macro last so that we add the tokens that
       // form its body to it.
       Macro = MI;
@@ -1341,10 +1316,13 @@ HeaderFileInfoTrait::ReadData(const internal_key_type, const unsigned char *d,
   return HFI;
 }
 
-void ASTReader::setIdentifierIsMacro(IdentifierInfo *II, ArrayRef<MacroID> IDs){
+void ASTReader::addMacroIDForDeserialization(IdentifierInfo *II, MacroID ID){
   II->setHadMacroDefinition(true);
   assert(NumCurrentElementsDeserializing > 0 &&"Missing deserialization guard");
-  PendingMacroIDs[II].append(IDs.begin(), IDs.end());
+  SmallVector<serialization::MacroID, 2> &MacroIDs = PendingMacroIDs[II];
+  assert(std::find(MacroIDs.begin(), MacroIDs.end(), ID) == MacroIDs.end() &&
+         "Already added the macro ID for deserialization");
+  MacroIDs.push_back(ID);
 }
 
 void ASTReader::ReadDefinedMacros() {
@@ -6158,7 +6136,7 @@ IdentifierID ASTReader::getGlobalIdentifierID(ModuleFile &M, unsigned LocalID) {
   return LocalID + I->second;
 }
 
-MacroInfo *ASTReader::getMacro(MacroID ID, MacroInfo *Hint) {
+MacroInfo *ASTReader::getMacro(MacroID ID) {
   if (ID == 0)
     return 0;
 
@@ -6174,7 +6152,7 @@ MacroInfo *ASTReader::getMacro(MacroID ID, MacroInfo *Hint) {
     assert(I != GlobalMacroMap.end() && "Corrupted global macro map");
     ModuleFile *M = I->second;
     unsigned Index = ID - M->BaseMacroID;
-    ReadMacroRecord(*M, M->MacroOffsets[Index], Hint);
+    ReadMacroRecord(*M, M->MacroOffsets[Index]);
   }
 
   return MacrosLoaded[ID];
@@ -6873,13 +6851,14 @@ void ASTReader::finishPendingActions() {
     PendingDeclChains.clear();
 
     // Load any pending macro definitions.
+    // Note that new macros may be added while deserializing a macro.
     for (unsigned I = 0; I != PendingMacroIDs.size(); ++I) {
-      // FIXME: std::move here
-      SmallVector<MacroID, 2> GlobalIDs = PendingMacroIDs.begin()[I].second;
-      MacroInfo *Hint = 0;
-      for (unsigned IDIdx = 0, NumIDs = GlobalIDs.size(); IDIdx !=  NumIDs;
-           ++IDIdx) {
-        Hint = getMacro(GlobalIDs[IDIdx], Hint);
+      PendingMacroIDsMap::iterator PMIt = PendingMacroIDs.begin() + I;
+      SmallVector<serialization::MacroID, 2> &MacroIDs = PMIt->second;
+      for (SmallVectorImpl<serialization::MacroID>::iterator
+             MIt = MacroIDs.begin(), ME = MacroIDs.end(); MIt != ME; ++MIt) {
+        MacroInfo *MI = getMacro(*MIt);
+        PP.addLoadedMacroInfo(PMIt->first, MI);
       }
     }
     PendingMacroIDs.clear();

clang/lib/Serialization/ASTWriter.cpp

@@ -1796,12 +1796,10 @@ void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule) {
   // Construct the list of macro definitions that need to be serialized.
   SmallVector<std::pair<const IdentifierInfo *, MacroInfo *>, 2> 
     MacrosToEmit;
-  llvm::SmallPtrSet<const IdentifierInfo*, 4> MacroDefinitionsSeen;
   for (Preprocessor::macro_iterator I = PP.macro_begin(Chain == 0),
                                     E = PP.macro_end(Chain == 0);
        I != E; ++I) {
     if (!IsModule || I->second->isPublic()) {
-      MacroDefinitionsSeen.insert(I->first);
       MacrosToEmit.push_back(std::make_pair(I->first, I->second));
     }
   }
@@ -1854,6 +1852,12 @@ void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule) {
       Record.push_back(inferSubmoduleIDFromLocation(MI->getDefinitionLoc()));
       AddSourceLocation(MI->getDefinitionLoc(), Record);
       AddSourceLocation(MI->getDefinitionEndLoc(), Record);
+      MacroInfo *PrevMI = MI->getPreviousDefinition();
+      // Serialize only the part of the definition chain that is local.
+      // The chain will be synthesized across modules by the ASTReader.
+      if (Chain && PrevMI && PrevMI->isFromAST())
+        PrevMI = 0;
+      addMacroRef(PrevMI, Record);
       AddSourceLocation(MI->getUndefLoc(), Record);
       Record.push_back(MI->isUsed());
       Record.push_back(MI->isPublic());
@@ -2735,14 +2739,8 @@ public:
     if (isInterestingIdentifier(II, Macro)) {
       DataLen += 2; // 2 bytes for builtin ID
       DataLen += 2; // 2 bytes for flags
-      if (hadMacroDefinition(II, Macro)) {
-        for (MacroInfo *M = Macro; M; M = M->getPreviousDefinition()) {
-          if (Writer.getMacroRef(M) != 0)
-            DataLen += 4;
-        }
-
+      if (hadMacroDefinition(II, Macro))
         DataLen += 4;
-      }
 
       for (IdentifierResolver::iterator D = IdResolver.begin(II),
                                      DEnd = IdResolver.end();
@@ -2787,13 +2785,8 @@ public:
     clang::io::Emit16(Out, Bits);
 
     if (HadMacroDefinition) {
-      // 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);
+      // Write the macro ID associated with this identifier.
+      clang::io::Emit32(Out, Writer.getMacroRef(Macro));
     }
 
     // Emit the declaration IDs in reverse order, because the

clang/test/PCH/macro-redef.c

@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 %s -emit-pch -o %t1.pch -verify
+// RUN: %clang_cc1 %s -emit-pch -o %t2.pch -include-pch %t1.pch -verify
+// RUN: %clang_cc1 -fsyntax-only %s -include-pch %t2.pch -verify
+
+// Test that a redefinition inside the PCH won't manifest as an ambiguous macro.
+// rdar://13016031
+
+#ifndef HEADER1
+#define HEADER1
+
+#define M1 0 // expected-note {{previous}}
+#define M1 1 // expected-warning {{redefined}}
+
+#define M2 3
+
+#elif !defined(HEADER2)
+#define HEADER2
+
+#define M2 4 // expected-warning {{redefined}}
+ // expected-note@-6 {{previous}}
+
+#else
+
+// Use the error to verify it was parsed.
+int x = M1; // expected-note {{previous}}
+int x = M2; // expected-error {{redefinition}}
+
+#endif