Revert r314955: "Remove PendingBody mechanism for function and ObjC method deserialization."

This is breaking a build of https://github.com/abseil/abseil-cpp and so
likely not really NFC. Also reverted subsequent r314956/7.

I'll forward reproduction instructions to Richard.

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

include/clang/Serialization/ASTReader.h

@@ -559,9 +559,13 @@ private:
   /// declarations that have not yet been linked to their definitions.
   llvm::SmallPtrSet<Decl *, 4> PendingDefinitions;
 
-  /// \brief Functions or methods that are known to already have a definition
-  /// (that might not yet be merged into the redeclaration chain).
-  llvm::SmallDenseMap<FunctionDecl *, FunctionDecl*, 4> FunctionDefinitions;
+  typedef llvm::MapVector<Decl *, uint64_t,
+                          llvm::SmallDenseMap<Decl *, unsigned, 4>,
+                          SmallVector<std::pair<Decl *, uint64_t>, 4> >
+    PendingBodiesMap;
+
+  /// \brief Functions or methods that have bodies that will be attached.
+  PendingBodiesMap PendingBodies;
 
   /// \brief Definitions for which we have added merged definitions but not yet
   /// performed deduplication.
@@ -987,13 +991,25 @@ private:
   /// the last time we loaded information about this identifier.
   llvm::DenseMap<IdentifierInfo *, unsigned> IdentifierGeneration;
 
+  class InterestingDecl {
+    Decl *D;
+    bool DeclHasPendingBody;
+
+  public:
+    InterestingDecl(Decl *D, bool HasBody)
+        : D(D), DeclHasPendingBody(HasBody) {}
+    Decl *getDecl() { return D; }
+    /// Whether the declaration has a pending body.
+    bool hasPendingBody() { return DeclHasPendingBody; }
+  };
+
   /// \brief Contains declarations and definitions that could be
   /// "interesting" to the ASTConsumer, when we get that AST consumer.
   ///
   /// "Interesting" declarations are those that have data that may
   /// need to be emitted, such as inline function definitions or
   /// Objective-C protocols.
-  std::deque<Decl*> PotentiallyInterestingDecls;
+  std::deque<InterestingDecl> PotentiallyInterestingDecls;
 
   /// \brief The list of redeclaration chains that still need to be 
   /// reconstructed, and the local offset to the corresponding list

lib/Serialization/ASTCommon.cpp

@@ -344,8 +344,8 @@ bool serialization::needsAnonymousDeclarationNumber(const NamedDecl *D) {
     return true;
   }
 
-  // Otherwise, we only care about anonymous class members / block-scope decls.
-  if (D->getDeclName() || D->getLexicalDeclContext()->isFileContext())
+  // Otherwise, we only care about anonymous class members.
+  if (D->getDeclName() || !isa<CXXRecordDecl>(D->getLexicalDeclContext()))
     return false;
   return isa<TagDecl>(D) || isa<FieldDecl>(D);
 }

lib/Serialization/ASTReader.cpp

@@ -9169,6 +9169,30 @@ void ASTReader::finishPendingActions() {
   }
   PendingDefinitions.clear();
 
+  // Load the bodies of any functions or methods we've encountered. We do
+  // this now (delayed) so that we can be sure that the declaration chains
+  // have been fully wired up (hasBody relies on this).
+  // FIXME: We shouldn't require complete redeclaration chains here.
+  for (PendingBodiesMap::iterator PB = PendingBodies.begin(),
+                               PBEnd = PendingBodies.end();
+       PB != PBEnd; ++PB) {
+    if (FunctionDecl *FD = dyn_cast<FunctionDecl>(PB->first)) {
+      // FIXME: Check for =delete/=default?
+      // FIXME: Complain about ODR violations here?
+      const FunctionDecl *Defn = nullptr;
+      if (!getContext().getLangOpts().Modules || !FD->hasBody(Defn)) {
+        FD->setLazyBody(PB->second);
+      } else
+        mergeDefinitionVisibility(const_cast<FunctionDecl*>(Defn), FD);
+      continue;
+    }
+
+    ObjCMethodDecl *MD = cast<ObjCMethodDecl>(PB->first);
+    if (!getContext().getLangOpts().Modules || !MD->hasBody())
+      MD->setLazyBody(PB->second);
+  }
+  PendingBodies.clear();
+
   // Do some cleanup.
   for (auto *ND : PendingMergedDefinitionsToDeduplicate)
     getContext().deduplicateMergedDefinitonsFor(ND);

lib/Serialization/ASTReaderDecl.cpp

@@ -45,6 +45,8 @@ namespace clang {
     GlobalDeclID NamedDeclForTagDecl;
     IdentifierInfo *TypedefNameForLinkage;
 
+    bool HasPendingBody;
+
     ///\brief A flag to carry the information for a decl from the entity is
     /// used. We use it to delay the marking of the canonical decl as used until
     /// the entire declaration is deserialized and merged.
@@ -214,7 +216,8 @@ namespace clang {
         : Reader(Reader), Record(Record), Loc(Loc),
           ThisDeclID(thisDeclID), ThisDeclLoc(ThisDeclLoc),
           TypeIDForTypeDecl(0), NamedDeclForTagDecl(0),
-          TypedefNameForLinkage(nullptr), IsDeclMarkedUsed(false) {}
+          TypedefNameForLinkage(nullptr), HasPendingBody(false),
+          IsDeclMarkedUsed(false) {}
 
     template <typename T> static
     void AddLazySpecializations(T *D,
@@ -262,7 +265,9 @@ namespace clang {
     static void markIncompleteDeclChainImpl(Redeclarable<DeclT> *D);
     static void markIncompleteDeclChainImpl(...);
 
-    FunctionDecl *TryRegisterAsFunctionDefinition(FunctionDecl *FD);
+    /// \brief Determine whether this declaration has a pending body.
+    bool hasPendingBody() const { return HasPendingBody; }
+
     void ReadFunctionDefinition(FunctionDecl *FD);
     void Visit(Decl *D);
 
@@ -415,8 +420,7 @@ public:
   MergedRedeclIterator &operator++() {
     if (Current->isFirstDecl()) {
       Canonical = Current;
-      Decl *MostRecent = ASTDeclReader::getMostRecentDecl(Canonical);
-      Current = MostRecent ? cast<DeclT>(MostRecent) : nullptr;
+      Current = Current->getMostRecentDecl();
     } else
       Current = Current->getPreviousDecl();
 
@@ -447,70 +451,17 @@ uint64_t ASTDeclReader::GetCurrentCursorOffset() {
   return Loc.F->DeclsCursor.GetCurrentBitNo() + Loc.F->GlobalBitOffset;
 }
 
-FunctionDecl *ASTDeclReader::TryRegisterAsFunctionDefinition(FunctionDecl *D) {
-  FunctionDecl *&Definition = Reader.FunctionDefinitions[D->getCanonicalDecl()];
-
-  if (!Definition) {
-    // No imported definition, but we might have a local definition.
-    for (auto *Redecl : merged_redecls(D)) {
-      // FIXME: If an existing declaration is a definition with no body
-      // (via =delete etc), we shouldn't permit another definition here.
-      if (Redecl->Body.isValid()) {
-        Definition = Redecl;
-        break;
-      }
-    }
-  }
-
-  if (Definition) {
-    // We might have multiple update records adding definitions to the same
-    // declaration.
-    if (Definition != D) {
-      // Already have a different definition, merge this one into it.
-      Reader.MergedDeclContexts.insert(std::make_pair(D, Definition));
-      Reader.mergeDefinitionVisibility(Definition, D);
-    }
-    return Definition;
-  }
-
-  Definition = D;
-  return nullptr;
-}
-
 void ASTDeclReader::ReadFunctionDefinition(FunctionDecl *FD) {
-  // Is this definition scheduled for modular codegen? (That is, emitted to a
-  // separate object file for the module itself, rather than with module users.)
-  bool ModularCodegen = Record.readInt();
-
-  // Don't load this definition if we already have one.
-  if (auto *Definition = TryRegisterAsFunctionDefinition(FD)) {
-    if (ModularCodegen) {
-      // Request a strong definition be emitted if any merged definition does.
-      // FIXME: Do we need to ensure that Definition is handed to the AST
-      // consumer in this case?
-      Reader.DefinitionSource[Definition] |=
-          Loc.F->Kind == ModuleKind::MK_MainFile;
-    }
-    // Skip the ctor-initializers, if any.
-    if (isa<CXXConstructorDecl>(FD))
-      if (Record.readInt())
-        ReadGlobalOffset();
-    // FIXME: Optionally register the duplicate definitions somewhere so we can
-    // check for ODR violations.
-    return;
-  }
-
-  if (ModularCodegen)
+  if (Record.readInt())
     Reader.DefinitionSource[FD] = Loc.F->Kind == ModuleKind::MK_MainFile;
-
   if (auto *CD = dyn_cast<CXXConstructorDecl>(FD)) {
     CD->NumCtorInitializers = Record.readInt();
     if (CD->NumCtorInitializers)
       CD->CtorInitializers = ReadGlobalOffset();
   }
-
   // Store the offset of the body so we can lazily load it later.
-  FD->setLazyBody(GetCurrentCursorOffset());
+  Reader.PendingBodies[FD] = GetCurrentCursorOffset();
+  HasPendingBody = true;
 }
 
 void ASTDeclReader::Visit(Decl *D) {
@@ -962,10 +913,10 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
 void ASTDeclReader::VisitObjCMethodDecl(ObjCMethodDecl *MD) {
   VisitNamedDecl(MD);
   if (Record.readInt()) {
-    // Load the body on-demand (if we don't already have a definition). Most
-    // clients won't care, because method definitions rarely show up in
-    // headers.
-    MD->setLazyBody(GetCurrentCursorOffset());
+    // Load the body on-demand. Most clients won't care, because method
+    // definitions rarely show up in headers.
+    Reader.PendingBodies[MD] = GetCurrentCursorOffset();
+    HasPendingBody = true;
     MD->setSelfDecl(ReadDeclAs<ImplicitParamDecl>());
     MD->setCmdDecl(ReadDeclAs<ImplicitParamDecl>());
   }
@@ -2616,7 +2567,7 @@ inline void ASTReader::LoadedDecl(unsigned Index, Decl *D) {
 /// This routine should return true for anything that might affect
 /// code generation, e.g., inline function definitions, Objective-C
 /// declarations with metadata, etc.
-static bool isConsumerInterestedIn(ASTContext &Ctx, Decl *D) {
+static bool isConsumerInterestedIn(ASTContext &Ctx, Decl *D, bool HasBody) {
   // An ObjCMethodDecl is never considered as "interesting" because its
   // implementation container always is.
 
@@ -2642,7 +2593,7 @@ static bool isConsumerInterestedIn(ASTContext &Ctx, Decl *D) {
     return Var->isFileVarDecl() &&
            Var->isThisDeclarationADefinition() == VarDecl::Definition;
   if (FunctionDecl *Func = dyn_cast<FunctionDecl>(D))
-    return Func->doesThisDeclarationHaveABody();
+    return Func->doesThisDeclarationHaveABody() || HasBody;
 
   if (auto *ES = D->getASTContext().getExternalSource())
     if (ES->hasExternalDefinitions(D) == ExternalASTSource::EK_Never)
@@ -3707,7 +3658,8 @@ Decl *ASTReader::ReadDeclRecord(DeclID ID) {
   // AST consumer might need to know about, queue it.
   // We don't pass it to the consumer immediately because we may be in recursive
   // loading, and some declarations may still be initializing.
-  PotentiallyInterestingDecls.push_back(D);
+  PotentiallyInterestingDecls.push_back(
+      InterestingDecl(D, Reader.hasPendingBody()));
 
   return D;
 }
@@ -3730,10 +3682,10 @@ void ASTReader::PassInterestingDeclsToConsumer() {
   EagerlyDeserializedDecls.clear();
 
   while (!PotentiallyInterestingDecls.empty()) {
-    Decl *D = PotentiallyInterestingDecls.front();
+    InterestingDecl D = PotentiallyInterestingDecls.front();
     PotentiallyInterestingDecls.pop_front();
-    if (isConsumerInterestedIn(getContext(), D))
-      PassInterestingDeclToConsumer(D);
+    if (isConsumerInterestedIn(getContext(), D.getDecl(), D.hasPendingBody()))
+      PassInterestingDeclToConsumer(D.getDecl());
   }
 }
 
@@ -3757,7 +3709,7 @@ void ASTReader::loadDeclUpdateRecords(PendingUpdateRecord &Record) {
     // to isConsumerInterestedIn because it is unsafe to call in the
     // current ASTReader state.
     bool WasInteresting =
-        Record.JustLoaded || isConsumerInterestedIn(getContext(), D);
+        Record.JustLoaded || isConsumerInterestedIn(getContext(), D, false);
     for (auto &FileAndOffset : UpdateOffsets) {
       ModuleFile *F = FileAndOffset.first;
       uint64_t Offset = FileAndOffset.second;
@@ -3776,8 +3728,10 @@ void ASTReader::loadDeclUpdateRecords(PendingUpdateRecord &Record) {
 
       // We might have made this declaration interesting. If so, remember that
       // we need to hand it off to the consumer.
-      if (!WasInteresting && isConsumerInterestedIn(getContext(), D)) {
-        PotentiallyInterestingDecls.push_back(D);
+      if (!WasInteresting &&
+          isConsumerInterestedIn(getContext(), D, Reader.hasPendingBody())) {
+        PotentiallyInterestingDecls.push_back(
+            InterestingDecl(D, Reader.hasPendingBody()));
         WasInteresting = true;
       }
     }
@@ -4075,6 +4029,12 @@ void ASTDeclReader::UpdateDecl(Decl *D,
 
     case UPD_CXX_ADDED_FUNCTION_DEFINITION: {
       FunctionDecl *FD = cast<FunctionDecl>(D);
+      if (Reader.PendingBodies[FD]) {
+        // FIXME: Maybe check for ODR violations.
+        // It's safe to stop now because this update record is always last.
+        return;
+      }
+
       if (Record.readInt()) {
         // Maintain AST consistency: any later redeclarations of this function
         // are inline if this one is. (We might have merged another declaration

test/Modules/merge-lambdas.cpp

@@ -1,43 +0,0 @@
-// RUN: %clang_cc1 -std=c++11 -emit-llvm-only -fmodules %s
-
-// PR33924: ensure that we merge together local lambas in multiple definitions
-// of the same function.
-
-#pragma clang module build format
-module format {}
-#pragma clang module contents
-#pragma clang module begin format
-struct A { template<typename T> void doFormat(T &&out) {} };
-template<typename T> void format(T t) { A().doFormat([]{}); }
-#pragma clang module end
-#pragma clang module endbuild
-
-#pragma clang module build foo1
-module foo1 { export * }
-#pragma clang module contents
-#pragma clang module begin foo1
-#pragma clang module import format
-inline void foo1() {
-  format(0);
-}
-#pragma clang module end
-#pragma clang module endbuild
-
-#pragma clang module build foo2
-module foo2 { export * }
-#pragma clang module contents
-#pragma clang module begin foo2
-#pragma clang module import format
-inline void foo2() {
-  format(0);
-}
-#pragma clang module end
-#pragma clang module endbuild
-
-#pragma clang module import foo1
-#pragma clang module import foo2
-
-int main() {
-  foo1();
-  foo2();
-}