[modules] Move generation counter from ExternalASTSource to ASTContext

[teemperor]

Summary

The generation counter mechanism of the AST is currently implemented inside the ExternalASTSource. These counters
work as intended with clang's module setup, but causes problems with cling and ROOT's setup:

In cling we attach multiple ExternalASTSources which will cause the ASTReader to fail. The ASTReader assumes it's the
only attached external source. So it will increase it's own generation counter and then reads the top-level generation
counter of the ASTContext to get back it's own generation counter.

In ROOT we also attach after cling's startup another ExternalASTSource which will further disrupt existing generation
counters. The new multiplexer that is added as the new top level source will essentially reset all existing generation
counters back to zero.

This patch fixes those problems by merging all generation counters into a single one in the ASTContext. This
prevents that sometimes we are incrementing and then reading the different counters. It also prevents
that attaching a new counter or having multiple counters suddenly forces us to keep all those counters
in a consistent state.

[rsmith]

include/clang/AST/ExternalASTSource.h

461–464

Is there a reason to ask the ExternalASTSource for the generation here rather than directly asking the ASTContext? (Is there any circumstance in which we would expect or even allow them to return different numbers?)

[teemperor]

include/clang/AST/ExternalASTSource.h

461–464

ASTContext and ExternalASTSource reference each other and this header only got the fwd decl of ASTContext. Calling it via the ExtASTSource (which works around the fact that we only have a fwd decl) and following the existing workarounds was just less invasive than trying to cleanup this cyclic dependency :)

[rsmith]

include/clang/AST/ExternalASTSource.h

461–464

Hm, I'd prefer an approach that we can inline away without IPO. At least we should avoid calling getGeneration twice here.

[teemperor]

(Setting this to "Plan changes" so that I know this is waiting on me).

include/clang/AST/ASTContext.h

 public:
   IdentifierTable &Idents;
   SelectorTable &Selectors;
   Builtin::Context &BuiltinInfo;
   mutable DeclarationNameTable DeclarationNames;
   IntrusiveRefCntPtr<ExternalASTSource> ExternalSource;
   ASTMutationListener *Listener = nullptr;
 
+  /// Generation counter for this AST, used by attached external AST sources to
+  /// signal and detect modifications to the AST.
+  ///
+  /// Counter must be increased when an external source added new redeclaration
+  /// for an existing decl.
+  uint32_t CurrentGeneration = 0;
+
   /// \brief Contains parents of a node.
   using ParentVector = llvm::SmallVector<ast_type_traits::DynTypedNode, 2>;
 
   /// \brief Maps from a node to its parents. This is used for nodes that have
   /// pointer identity only, which are more common and we can save space by
   /// only storing a unique pointer to them.
   using ParentMapPointers =
       llvm::DenseMap<const void *,
#include "clang/Basic/OpenCLImageTypes.def"
   void setASTMutationListener(ASTMutationListener *Listener) {
     this->Listener = Listener;
   }
 
   /// \brief Retrieve a pointer to the AST mutation listener associated
   /// with this AST context, if any.
   ASTMutationListener *getASTMutationListener() const { return Listener; }
 
+  uint32_t getGeneration() const { return CurrentGeneration; }
+  uint32_t incrementGeneration() {
+    uint32_t OldGeneration = CurrentGeneration;
+    CurrentGeneration++;
+    assert(CurrentGeneration > OldGeneration &&
+           "Overflowed generation counter");
+    return OldGeneration;
+  }
+
   void PrintStats() const;
   const SmallVectorImpl<Type *>& getTypes() const { return Types; }
 
   BuiltinTemplateDecl *buildBuiltinTemplateDecl(BuiltinTemplateKind BTK,
                                                 const IdentifierInfo *II) const;
 
   /// \brief Create a new implicit TU-level CXXRecordDecl or RecordDecl
   /// declaration.

 /// This operator is just a companion to the new[] above. There is no way of
 /// invoking it directly; see the new[] operator for more details. This operator
 /// is called implicitly by the compiler if a placement new[] expression using
 /// the ASTContext throws in the object constructor.
 inline void operator delete[](void *Ptr, const clang::ASTContext &C, size_t) {
   C.Deallocate(Ptr);
 }
 
+// Note, this is implemented here so that ExternalASTSource.h doesn't need to
+// include ASTContext.h. We explicitly instantiate it for all relevant types
+// in ASTContext.cpp.
+
 /// \brief Create the representation of a LazyGenerationalUpdatePtr.
 template <typename Owner, typename T,
           void (clang::ExternalASTSource::*Update)(Owner)>
 typename clang::LazyGenerationalUpdatePtr<Owner, T, Update>::ValueType
     clang::LazyGenerationalUpdatePtr<Owner, T, Update>::makeValue(
         const clang::ASTContext &Ctx, T Value) {
-  // Note, this is implemented here so that ExternalASTSource.h doesn't need to
-  // include ASTContext.h. We explicitly instantiate it for all relevant types
-  // in ASTContext.cpp.
-  if (auto *Source = Ctx.getExternalSource())
-    return new (Ctx) LazyData(Source, Value);
+  if (Ctx.getExternalSource())
+    return new (Ctx) LazyData(&Ctx, Value);
   return Value;
 }
 
+template <typename Owner, typename T,
+          void (clang::ExternalASTSource::*Update)(Owner)>
+typename clang::ExternalASTSource *
+clang::LazyGenerationalUpdatePtr<Owner, T, Update>::getExternalSource(
+    const clang::ASTContext &Ctx) {
+  return Ctx.getExternalSource();
+}
+
 #endif // LLVM_CLANG_AST_ASTCONTEXT_H

include/clang/AST/ExternalASTSource.h

 /// External AST sources provide AST nodes constructed from some
 /// external source, such as a precompiled header. External AST
 /// sources can resolve types and declarations from abstract IDs into
 /// actual type and declaration nodes, and read parts of declaration
 /// contexts.
 class ExternalASTSource : public RefCountedBase<ExternalASTSource> {
   friend class ExternalSemaSource;
 
-  /// Generation number for this external AST source. Must be increased
-  /// whenever we might have added new redeclarations for existing decls.
-  uint32_t CurrentGeneration = 0;
-
   /// \brief Whether this AST source also provides information for
   /// semantic analysis.
   bool SemaSource = false;
 
 public:
   ExternalASTSource() = default;
   virtual ~ExternalASTSource();
 
public:
     ~Deserializing() {
       Source->FinishedDeserializing();
     }
   };
 
   /// \brief Get the current generation of this AST source. This number
   /// is incremented each time the AST source lazily extends an existing
   /// entity.
-  uint32_t getGeneration() const { return CurrentGeneration; }
+  uint32_t getGeneration(const ASTContext &C) const;
 
   /// \brief Resolve a declaration ID into a declaration, potentially
   /// building a new declaration.
   ///
   /// This method only needs to be implemented if the AST source ever
   /// passes back decl sets as VisibleDeclaration objects.
   ///
   /// The default implementation of this method is a no-op.

 /// \brief A lazy value (of type T) that is within an AST node of type Owner,
 /// where the value might change in later generations of the external AST
 /// source.
 template<typename Owner, typename T, void (ExternalASTSource::*Update)(Owner)>
 struct LazyGenerationalUpdatePtr {
   /// A cache of the value of this pointer, in the most recent generation in
   /// which we queried it.
   struct LazyData {
-    ExternalASTSource *ExternalSource;
+    const ASTContext *Context;
     uint32_t LastGeneration = 0;
     T LastValue;
 
-    LazyData(ExternalASTSource *Source, T Value)
-        : ExternalSource(Source), LastValue(Value) {}
+    LazyData(const ASTContext *Context, T Value)
+        : Context(Context), LastValue(Value) {}
   };
 
   // Our value is represented as simply T if there is no external AST source.
   using ValueType = llvm::PointerUnion<T, LazyData*>;
   ValueType Value;
 
   LazyGenerationalUpdatePtr(ValueType V) : Value(V) {}
 
   // Defined in ASTContext.h
   static ValueType makeValue(const ASTContext &Ctx, T Value);
+  static ExternalASTSource *getExternalSource(const ASTContext &Ctx);
 
 public:
   explicit LazyGenerationalUpdatePtr(const ASTContext &Ctx, T Value = T())
       : Value(makeValue(Ctx, Value)) {}
 
   /// Create a pointer that is not potentially updated by later generations of
   /// the external AST source.
   enum NotUpdatedTag { NotUpdated };
public:
   }
 
   /// Set the value of this pointer, for this and all future generations.
   void setNotUpdated(T NewValue) { Value = NewValue; }
 
   /// Get the value of this pointer, updating its owner if necessary.
   T get(Owner O) {
     if (LazyData *LazyVal = Value.template dyn_cast<LazyData*>()) {
-      if (LazyVal->LastGeneration != LazyVal->ExternalSource->getGeneration()) {
-        LazyVal->LastGeneration = LazyVal->ExternalSource->getGeneration();
-        (LazyVal->ExternalSource->*Update)(O);
+      ExternalASTSource *ExternalSource = getExternalSource(*LazyVal->Context);
+      if (LazyVal->LastGeneration !=
+          ExternalSource->getGeneration(*LazyVal->Context)) {
+        LazyVal->LastGeneration =
+            ExternalSource->getGeneration(*LazyVal->Context);
+        (ExternalSource->*Update)(O);
       }
       return LazyVal->LastValue;
     }
     return Value.template get<T>();
   }
 
   /// Get the most recently computed value of this pointer without updating it.
   T getNotUpdated() const {

include/clang/Serialization/ASTReader.h

struct ImportedModule {
     SourceLocation ImportLoc;
 
     ImportedModule(ModuleFile *Mod,
                    ModuleFile *ImportedBy,
                    SourceLocation ImportLoc)
       : Mod(Mod), ImportedBy(ImportedBy), ImportLoc(ImportLoc) {}
   };
 
+  using ExternalSemaSource::getGeneration;
+  /// Get the generation of the ASTContext we read into.
+  /// Returns 0 if we don't read into an ASTContext and have no generations.
+  uint32_t getGeneration() const {
+    if (ContextObj)
+      return getGeneration(*ContextObj);
+    return 0u;
+  }
+
   ASTReadResult ReadASTCore(StringRef FileName, ModuleKind Type,
                             SourceLocation ImportLoc, ModuleFile *ImportedBy,
                             SmallVectorImpl<ImportedModule> &Loaded,
                             off_t ExpectedSize, time_t ExpectedModTime,
                             ASTFileSignature ExpectedSignature,
                             unsigned ClientLoadCapabilities);
   ASTReadResult ReadControlBlock(ModuleFile &F,
                                  SmallVectorImpl<ImportedModule> &Loaded,

lib/AST/ASTContext.cpp

 // Explicitly instantiate this in case a Redeclarable<T> is used from a TU that
 // doesn't include ASTContext.h
 template
 clang::LazyGenerationalUpdatePtr<
     const Decl *, Decl *, &ExternalASTSource::CompleteRedeclChain>::ValueType
 clang::LazyGenerationalUpdatePtr<
     const Decl *, Decl *, &ExternalASTSource::CompleteRedeclChain>::makeValue(
         const clang::ASTContext &Ctx, Decl *Value);
+template clang::ExternalASTSource *
+clang::LazyGenerationalUpdatePtr<const Decl *, Decl *,
+                                 &ExternalASTSource::CompleteRedeclChain>::
+    getExternalSource(const clang::ASTContext &Ctx);

lib/AST/ExternalASTSource.cpp

 #include "llvm/ADT/None.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <cstdint>
 
 using namespace clang;
 
 ExternalASTSource::~ExternalASTSource() = default;
 
+uint32_t ExternalASTSource::getGeneration(const ASTContext &C) const {
+  return C.getGeneration();
+}
+
 llvm::Optional<ExternalASTSource::ASTSourceDescriptor>
 ExternalASTSource::getSourceDescriptor(unsigned ID) {
   return None;
 }
 
 ExternalASTSource::ExtKind
 ExternalASTSource::hasExternalDefinitions(const Decl *D) {
   return EK_ReplyHazy;

 
 void ExternalASTSource::FindExternalLexicalDecls(
     const DeclContext *DC, llvm::function_ref<bool(Decl::Kind)> IsKindWeWant,
     SmallVectorImpl<Decl *> &Result) {}
 
 void ExternalASTSource::getMemoryBufferSizes(MemoryBufferSizes &sizes) const {}
 
 uint32_t ExternalASTSource::incrementGeneration(ASTContext &C) {
-  uint32_t OldGeneration = CurrentGeneration;
-
-  // Make sure the generation of the topmost external source for the context is
-  // incremented. That might not be us.
-  auto *P = C.getExternalSource();
-  if (P && P != this)
-    CurrentGeneration = P->incrementGeneration(C);
-  else {
-    // FIXME: Only bump the generation counter if the current generation number
-    // has been observed?
-    if (!++CurrentGeneration)
-      llvm::report_fatal_error("generation counter overflowed", false);
-  }
-
-  return OldGeneration;
+  return C.incrementGeneration();
 }

unittests/CMakeLists.txt

 add_subdirectory(Sema)
 add_subdirectory(CodeGen)
 # FIXME: libclang unit tests are disabled on Windows due
 # to failures, mostly in libclang.VirtualFileOverlay_*.
 if(NOT WIN32 AND CLANG_TOOL_LIBCLANG_BUILD) 
   add_subdirectory(libclang)
 endif()
 add_subdirectory(Rename)
+add_subdirectory(Serialization)

unittests/Serialization/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS
+  Support
+  )
+
+add_clang_unittest(SerializationTests
+  GenerationCounterTest.cpp
+  )
+
+target_link_libraries(SerializationTests
+  PRIVATE
+  clangAST
+  clangSerialization
+  clangBasic
+  clangFrontend
+  clangTooling
+  )

unittests/Serialization/GenerationCounterTest.cpp

+//===- unittest/Serialization/GenerationCounterTest.cpp -------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// Tests the generation counter in the ExternalASTSource.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/ASTContext.h"
+#include "clang/Sema/MultiplexExternalSemaSource.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace {
+// Just allows us to easier increment the generation without actually having
+// to modify the AST in some way.
+class ASTSourceTester : public clang::ExternalSemaSource {
+public:
+  void testIncrementGeneration(clang::ASTContext &C) { incrementGeneration(C); }
+};
+} // namespace
+
+namespace clang {
+
+TEST(GenerationCounter, MultipleConsumers) {
+  // Three sources which always should see the same generation counter value
+  // once they have been attached to the ASTContext.
+  ASTSourceTester Source1, Source2, NewSource;
+
+  // Create a real ASTContext.
+  std::unique_ptr<ASTUnit> ASTUnit = tooling::buildASTFromCode("int main() {}");
+  clang::ASTContext &C = ASTUnit->getASTContext();
+
+  // Attach the first two sources with a multiplexer.
+  MultiplexExternalSemaSource *Multiplexer =
+      new MultiplexExternalSemaSource(Source1, Source2);
+  C.setExternalSource(Multiplexer);
+
+  auto OldGeneration = Source1.getGeneration(C);
+
+  // Pretend each source modifies the AST and increments the generation counter.
+  // After each step the generation counter needs to be identical for each
+  // source (but different than the previous counter value).
+  Source1.testIncrementGeneration(C);
+  ASSERT_EQ(Source1.getGeneration(C), Source2.getGeneration(C));
+  ASSERT_EQ(Source1.getGeneration(C), Multiplexer->getGeneration(C));
+  ASSERT_NE(Source1.getGeneration(C), OldGeneration);
+  OldGeneration = Source1.getGeneration(C);
+
+  Source2.testIncrementGeneration(C);
+  ASSERT_EQ(Source1.getGeneration(C), Source2.getGeneration(C));
+  ASSERT_EQ(Source1.getGeneration(C), Multiplexer->getGeneration(C));
+  ASSERT_NE(Source1.getGeneration(C), OldGeneration);
+
+  // Just add the last source which should also directly inherit the correct
+  // generation counter value.
+  Multiplexer->addSource(NewSource);
+  ASSERT_EQ(NewSource.getGeneration(C), Source1.getGeneration(C));
+  ASSERT_EQ(NewSource.getGeneration(C), Source2.getGeneration(C));
+  ASSERT_EQ(NewSource.getGeneration(C), Multiplexer->getGeneration(C));
+}
+
+} // namespace clang