[NFC] Make MultiplexExternalSemaSource own sources

[beanz]

Summary

This change refactors the MuiltiplexExternalSemaSource to take ownership
of the underlying sources. As a result it makes a larger cleanup of
external source ownership in Sema and the ChainedIncludesSource.

[aprantl]

clang/include/clang/Sema/MultiplexExternalSemaSource.h

53

why this change? Does & imply ownership in our coding style?

[beanz]

clang/include/clang/Sema/MultiplexExternalSemaSource.h

53

The old implementation took the address of the references to store. It seems more clear to me to accept a pointer when you need a pointer.

The old interface using references also allowed for passing in stack-allocated objects, which causes problems since the change here calls retain and release on the underlying ref count object. Taking pointers puts the onus on the user of the API to think through where the address passed in comes from.

[aprantl]

This looks plausible to me — did you build clang-tools-extra & lldb to make sure nothing else needs to be updated?

clang/include/clang/Sema/MultiplexExternalSemaSource.h

53

nit: clang-format :-)

[aaron.ballman]

LGTM aside from some minor nits (also, please run clang-format over the patch before landing).

clang/lib/Sema/MultiplexExternalSemaSource.cpp

32

clang/lib/Sema/Sema.cpp

548

[beanz]

Updating with changes based on review feedback, and fixups for clang-tools-extra

[beanz]

I built LLDB and ran its tests. I see no additional failures after applying my change, but LLDB's tests do not execute successfully on my local system (22 failures).

clang/include/clang/Sema/MultiplexExternalSemaSource.h

 /// An abstract interface that should be implemented by
 /// external AST sources that also provide information for semantic
 /// analysis.
 class MultiplexExternalSemaSource : public ExternalSemaSource {
   /// LLVM-style RTTI.
   static char ID;
 
 private:
-  SmallVector<ExternalSemaSource *, 2> Sources; // doesn't own them.
+  SmallVector<ExternalSemaSource *, 2> Sources;
 
 public:
 
   ///Constructs a new multiplexing external sema source and appends the
   /// given element to it.
   ///
-  ///\param[in] s1 - A non-null (old) ExternalSemaSource.
-  ///\param[in] s2 - A non-null (new) ExternalSemaSource.
+  ///\param[in] S1 - A non-null (old) ExternalSemaSource.
+  ///\param[in] S2 - A non-null (new) ExternalSemaSource.
   ///
-  MultiplexExternalSemaSource(ExternalSemaSource& s1, ExternalSemaSource& s2);
+  MultiplexExternalSemaSource(ExternalSemaSource* S1, ExternalSemaSource* S2);
 
   ~MultiplexExternalSemaSource() override;
 
   ///Appends new source to the source list.
   ///
   ///\param[in] source - An ExternalSemaSource.
   ///
-  void addSource(ExternalSemaSource &source);
+  void AddSource(ExternalSemaSource *Source);
 
   //===--------------------------------------------------------------------===//
   // ExternalASTSource.
   //===--------------------------------------------------------------------===//
 
   /// Resolve a declaration ID into a declaration, potentially
   /// building a new declaration.
   Decl *GetExternalDecl(uint32_t ID) override;

clang/include/clang/Sema/Sema.h

 };
 
 /// Sema - This implements semantic analysis and AST building for C.
 class Sema final {
   Sema(const Sema &) = delete;
   void operator=(const Sema &) = delete;
 
   ///Source of additional semantic information.
-  ExternalSemaSource *ExternalSource;
-
-  ///Whether Sema has generated a multiplexer and has to delete it.
-  bool isMultiplexExternalSource;
+  IntrusiveRefCntPtr<ExternalSemaSource> ExternalSource;
 
   static bool mightHaveNonExternalLinkage(const DeclaratorDecl *FD);
 
   /// Determine whether two declarations should be linked together, given that
   /// the old declaration might not be visible and the new declaration might
   /// not have external linkage.
   bool shouldLinkPossiblyHiddenDecl(const NamedDecl *Old,
                                     const NamedDecl *New) {
public:
   FPOptions     &getCurFPFeatures() { return CurFPFeatures; }
 
   DiagnosticsEngine &getDiagnostics() const { return Diags; }
   SourceManager &getSourceManager() const { return SourceMgr; }
   Preprocessor &getPreprocessor() const { return PP; }
   ASTContext &getASTContext() const { return Context; }
   ASTConsumer &getASTConsumer() const { return Consumer; }
   ASTMutationListener *getASTMutationListener() const;
-  ExternalSemaSource* getExternalSource() const { return ExternalSource; }
+  ExternalSemaSource* getExternalSource() const { return ExternalSource.get(); }
 
   DarwinSDKInfo *getDarwinSDKInfoForAvailabilityChecking(SourceLocation Loc,
                                                          StringRef Platform);
   DarwinSDKInfo *getDarwinSDKInfoForAvailabilityChecking();
 
   ///Registers an external source. If an external source already exists,
   /// creates a multiplex external source and appends to it.
   ///

clang/lib/Frontend/ChainedIncludesSource.cpp

 #include "clang/Sema/MultiplexExternalSemaSource.h"
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/ASTWriter.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 using namespace clang;
 
 namespace {
-class ChainedIncludesSourceImpl : public ExternalSemaSource {
+class ChainedIncludesSource : public ExternalSemaSource {
 public:
-  ChainedIncludesSourceImpl(std::vector<std::unique_ptr<CompilerInstance>> CIs)
+  ChainedIncludesSource(std::vector<std::unique_ptr<CompilerInstance>> CIs)
       : CIs(std::move(CIs)) {}
 
 protected:
-  //===----------------------------------------------------------------------===//
+  //===--------------------------------------------------------------------===//
   // ExternalASTSource interface.
-  //===----------------------------------------------------------------------===//
+  //===--------------------------------------------------------------------===//
 
   /// Return the amount of memory used by memory buffers, breaking down
   /// by heap-backed versus mmap'ed memory.
   void getMemoryBufferSizes(MemoryBufferSizes &sizes) const override {
     for (unsigned i = 0, e = CIs.size(); i != e; ++i) {
       if (const ExternalASTSource *eSrc =
           CIs[i]->getASTContext().getExternalSource()) {
         eSrc->getMemoryBufferSizes(sizes);
       }
     }
   }
 
 private:
   std::vector<std::unique_ptr<CompilerInstance>> CIs;
 };
-
-/// Members of ChainedIncludesSource, factored out so we can initialize
-/// them before we initialize the ExternalSemaSource base class.
-struct ChainedIncludesSourceMembers {
-  ChainedIncludesSourceMembers(
-      std::vector<std::unique_ptr<CompilerInstance>> CIs,
-      IntrusiveRefCntPtr<ExternalSemaSource> FinalReader)
-      : Impl(std::move(CIs)), FinalReader(std::move(FinalReader)) {}
-  ChainedIncludesSourceImpl Impl;
-  IntrusiveRefCntPtr<ExternalSemaSource> FinalReader;
-};
-
-/// Use MultiplexExternalSemaSource to dispatch all ExternalSemaSource
-/// calls to the final reader.
-class ChainedIncludesSource
-    : private ChainedIncludesSourceMembers,
-      public MultiplexExternalSemaSource {
-public:
-  ChainedIncludesSource(std::vector<std::unique_ptr<CompilerInstance>> CIs,
-                        IntrusiveRefCntPtr<ExternalSemaSource> FinalReader)
-      : ChainedIncludesSourceMembers(std::move(CIs), std::move(FinalReader)),
-        MultiplexExternalSemaSource(Impl, *this->FinalReader) {}
-};
-}
+} // end anonymous namespace
 
 static ASTReader *
 createASTReader(CompilerInstance &CI, StringRef pchFile,
                 SmallVectorImpl<std::unique_ptr<llvm::MemoryBuffer>> &MemBufs,
                 SmallVectorImpl<std::string> &bufNames,
                 ASTDeserializationListener *deserialListener = nullptr) {
   Preprocessor &PP = CI.getPreprocessor();
   std::unique_ptr<ASTReader> Reader;
IntrusiveRefCntPtr<ExternalSemaSource> clang::createChainedIncludesSource(
 
   assert(!SerialBufs.empty());
   std::string pchName = includes.back() + ".pch-final";
   serialBufNames.push_back(pchName);
   Reader = createASTReader(CI, pchName, SerialBufs, serialBufNames);
   if (!Reader)
     return nullptr;
 
-  return IntrusiveRefCntPtr<ChainedIncludesSource>(
-      new ChainedIncludesSource(std::move(CIs), Reader));
+  auto ChainedSrc =
+      llvm::makeIntrusiveRefCnt<ChainedIncludesSource>(std::move(CIs));
+  return llvm::makeIntrusiveRefCnt<MultiplexExternalSemaSource>(
+      ChainedSrc.get(), Reader.get());
 }

clang/lib/Sema/MultiplexExternalSemaSource.cpp

 
 using namespace clang;
 
 char MultiplexExternalSemaSource::ID;
 
 ///Constructs a new multiplexing external sema source and appends the
 /// given element to it.
 ///
-MultiplexExternalSemaSource::MultiplexExternalSemaSource(ExternalSemaSource &s1,
-                                                        ExternalSemaSource &s2){
-  Sources.push_back(&s1);
-  Sources.push_back(&s2);
+MultiplexExternalSemaSource::MultiplexExternalSemaSource(ExternalSemaSource *S1,
+                                                        ExternalSemaSource *S2){
+  S1->Retain();
+  S2->Retain();
+  Sources.push_back(S1);
+  Sources.push_back(S2);
 }
 
 // pin the vtable here.
-MultiplexExternalSemaSource::~MultiplexExternalSemaSource() {}
+MultiplexExternalSemaSource::~MultiplexExternalSemaSource() {
+  for (auto &S : Sources)
+    S->Release();
+}
 
 ///Appends new source to the source list.
 ///
 ///\param[in] source - An ExternalSemaSource.
 ///
-void MultiplexExternalSemaSource::addSource(ExternalSemaSource &source) {
-  Sources.push_back(&source);
+void MultiplexExternalSemaSource::AddSource(ExternalSemaSource *Source) {
+  Source->Retain();
+  Sources.push_back(Source);
 }
 
 //===----------------------------------------------------------------------===//
 // ExternalASTSource.
 //===----------------------------------------------------------------------===//
 
 Decl *MultiplexExternalSemaSource::GetExternalDecl(uint32_t ID) {
   for(size_t i = 0; i < Sources.size(); ++i)

clang/lib/Sema/Sema.cpp

 } // end namespace sema
 } // end namespace clang
 
 const unsigned Sema::MaxAlignmentExponent;
 const uint64_t Sema::MaximumAlignment;
 
 Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
            TranslationUnitKind TUKind, CodeCompleteConsumer *CodeCompleter)
-    : ExternalSource(nullptr), isMultiplexExternalSource(false),
-      CurFPFeatures(pp.getLangOpts()), LangOpts(pp.getLangOpts()), PP(pp),
-      Context(ctxt), Consumer(consumer), Diags(PP.getDiagnostics()),
-      SourceMgr(PP.getSourceManager()), CollectStats(false),
-      CodeCompleter(CodeCompleter), CurContext(nullptr),
+    : ExternalSource(nullptr), CurFPFeatures(pp.getLangOpts()),
+      LangOpts(pp.getLangOpts()), PP(pp), Context(ctxt), Consumer(consumer),
+      Diags(PP.getDiagnostics()), SourceMgr(PP.getSourceManager()),
+      CollectStats(false), CodeCompleter(CodeCompleter), CurContext(nullptr),
       OriginalLexicalContext(nullptr), MSStructPragmaOn(false),
       MSPointerToMemberRepresentationMethod(
           LangOpts.getMSPointerToMemberRepresentationMethod()),
       VtorDispStack(LangOpts.getVtorDispMode()),
       AlignPackStack(AlignPackInfo(getLangOpts().XLPragmaPack)),
       DataSegStack(nullptr), BSSSegStack(nullptr), ConstSegStack(nullptr),
       CodeSegStack(nullptr), FpPragmaStack(FPOptionsOverride()),
       CurInitSeg(nullptr), VisContext(nullptr),
Sema::~Sema() {
   if (SemaConsumer *SC = dyn_cast<SemaConsumer>(&Consumer))
     SC->ForgetSema();
 
   // Detach from the external Sema source.
   if (ExternalSemaSource *ExternalSema
         = dyn_cast_or_null<ExternalSemaSource>(Context.getExternalSource()))
     ExternalSema->ForgetSema();
 
-  // If Sema's ExternalSource is the multiplexer - we own it.
-  if (isMultiplexExternalSource)
-    delete ExternalSource;
-
   // Delete cached satisfactions.
   std::vector<ConstraintSatisfaction *> Satisfactions;
   Satisfactions.reserve(Satisfactions.size());
   for (auto &Node : SatisfactionCache)
     Satisfactions.push_back(&Node);
   for (auto *Node : Satisfactions)
     delete Node;
 

 void Sema::addExternalSource(ExternalSemaSource *E) {
   assert(E && "Cannot use with NULL ptr");
 
   if (!ExternalSource) {
     ExternalSource = E;
     return;
   }
 
-  if (isMultiplexExternalSource)
-    static_cast<MultiplexExternalSemaSource*>(ExternalSource)->addSource(*E);
-  else {
-    ExternalSource = new MultiplexExternalSemaSource(*ExternalSource, *E);
-    isMultiplexExternalSource = true;
-  }
+  if (auto Ex = dyn_cast<MultiplexExternalSemaSource>(ExternalSource))
+    Ex->AddSource(E);
+  else
+    ExternalSource = new MultiplexExternalSemaSource(ExternalSource.get(), E);
 }
 
 /// Print out statistics about the semantic analysis.
 void Sema::PrintStats() const {
   llvm::errs() << "\n*** Semantic Analysis Stats:\n";
   llvm::errs() << NumSFINAEErrors << " SFINAE diagnostics trapped.\n";
 
   BumpAlloc.PrintStats();
void Sema::ActOnEndOfTranslationUnit() {
   //   contains one or more tentative definitions for an identifier,
   //   and the translation unit contains no external definition for
   //   that identifier, then the behavior is exactly as if the
   //   translation unit contains a file scope declaration of that
   //   identifier, with the composite type as of the end of the
   //   translation unit, with an initializer equal to 0.
   llvm::SmallSet<VarDecl *, 32> Seen;
   for (TentativeDefinitionsType::iterator
-            T = TentativeDefinitions.begin(ExternalSource),
+            T = TentativeDefinitions.begin(ExternalSource.get()),
          TEnd = TentativeDefinitions.end();
        T != TEnd; ++T) {
     VarDecl *VD = (*T)->getActingDefinition();
 
     // If the tentative definition was completed, getActingDefinition() returns
     // null. If we've already seen this variable before, insert()'s second
     // return value is false.
     if (!VD || VD->isInvalidDecl() || !Seen.insert(VD).second)
void Sema::ActOnEndOfTranslationUnit() {
 
   // If there were errors, disable 'unused' warnings since they will mostly be
   // noise. Don't warn for a use from a module: either we should warn on all
   // file-scope declarations in modules or not at all, but whether the
   // declaration is used is immaterial.
   if (!Diags.hasErrorOccurred() && TUKind != TU_Module) {
     // Output warning for unused file scoped decls.
     for (UnusedFileScopedDeclsType::iterator
-           I = UnusedFileScopedDecls.begin(ExternalSource),
+           I = UnusedFileScopedDecls.begin(ExternalSource.get()),
            E = UnusedFileScopedDecls.end(); I != E; ++I) {
       if (ShouldRemoveFromUnused(this, *I))
         continue;
 
       if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(*I)) {
         const FunctionDecl *DiagD;
         if (!FD->hasBody(DiagD))
           DiagD = FD;

clang/lib/Sema/SemaDeclCXX.cpp

void DelegatingCycleHelper(CXXConstructorDecl* Ctor,
   }
 }
 
 
 void Sema::CheckDelegatingCtorCycles() {
   llvm::SmallPtrSet<CXXConstructorDecl*, 4> Valid, Invalid, Current;
 
   for (DelegatingCtorDeclsType::iterator
-         I = DelegatingCtorDecls.begin(ExternalSource),
+         I = DelegatingCtorDecls.begin(ExternalSource.get()),
          E = DelegatingCtorDecls.end();
        I != E; ++I)
     DelegatingCycleHelper(*I, Valid, Invalid, Current, *this);
 
   for (auto CI = Invalid.begin(), CE = Invalid.end(); CI != CE; ++CI)
     (*CI)->setInvalidDecl();
 }
 

clang/unittests/Sema/ExternalSemaSourceTest.cpp

 public:
   void PushSource(clang::ExternalSemaSource *Source) {
     Sources.push_back(Source);
   }
 
   void PushWatcher(DiagnosticWatcher *Watcher) { Watchers.push_back(Watcher); }
 };
 
+using llvm::makeIntrusiveRefCnt;
+
 // Make sure that the DiagnosticWatcher is not miscounting.
 TEST(ExternalSemaSource, DiagCheck) {
   auto Installer = std::make_unique<ExternalSemaSourceInstaller>();
   DiagnosticWatcher Watcher("AAB", "BBB");
   Installer->PushWatcher(&Watcher);
   std::vector<std::string> Args(1, "-std=c++11");
   ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
       std::move(Installer), "namespace AAA { } using namespace AAB;", Args));
   ASSERT_EQ(0, Watcher.SeenCount);
 }
 
 // Check that when we add a NamespaceTypeProvider, we use that suggestion
 // instead of the usual suggestion we would use above.
 TEST(ExternalSemaSource, ExternalTypoCorrectionPrioritized) {
   auto Installer = std::make_unique<ExternalSemaSourceInstaller>();
-  NamespaceTypoProvider Provider("AAB", "BBB");
+  auto Provider = makeIntrusiveRefCnt<NamespaceTypoProvider>("AAB", "BBB");
   DiagnosticWatcher Watcher("AAB", "BBB");
-  Installer->PushSource(&Provider);
+  Installer->PushSource(Provider.get());
   Installer->PushWatcher(&Watcher);
   std::vector<std::string> Args(1, "-std=c++11");
   ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
       std::move(Installer), "namespace AAA { } using namespace AAB;", Args));
-  ASSERT_LE(0, Provider.CallCount);
+  ASSERT_LE(0, Provider->CallCount);
   ASSERT_EQ(1, Watcher.SeenCount);
 }
 
 // Check that we use the first successful TypoCorrection returned from an
 // ExternalSemaSource.
 TEST(ExternalSemaSource, ExternalTypoCorrectionOrdering) {
   auto Installer = std::make_unique<ExternalSemaSourceInstaller>();
-  NamespaceTypoProvider First("XXX", "BBB");
-  NamespaceTypoProvider Second("AAB", "CCC");
-  NamespaceTypoProvider Third("AAB", "DDD");
+  auto First = makeIntrusiveRefCnt<NamespaceTypoProvider>("XXX", "BBB");
+  auto Second = makeIntrusiveRefCnt<NamespaceTypoProvider>("AAB", "CCC");
+  auto Third = makeIntrusiveRefCnt<NamespaceTypoProvider>("AAB", "DDD");
   DiagnosticWatcher Watcher("AAB", "CCC");
-  Installer->PushSource(&First);
-  Installer->PushSource(&Second);
-  Installer->PushSource(&Third);
+  Installer->PushSource(First.get());
+  Installer->PushSource(Second.get());
+  Installer->PushSource(Third.get());
   Installer->PushWatcher(&Watcher);
   std::vector<std::string> Args(1, "-std=c++11");
   ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
       std::move(Installer), "namespace AAA { } using namespace AAB;", Args));
-  ASSERT_LE(1, First.CallCount);
-  ASSERT_LE(1, Second.CallCount);
-  ASSERT_EQ(0, Third.CallCount);
+  ASSERT_LE(1, First->CallCount);
+  ASSERT_LE(1, Second->CallCount);
+  ASSERT_EQ(0, Third->CallCount);
   ASSERT_EQ(1, Watcher.SeenCount);
 }
 
 TEST(ExternalSemaSource, ExternalDelayedTypoCorrection) {
   auto Installer = std::make_unique<ExternalSemaSourceInstaller>();
-  FunctionTypoProvider Provider("aaa", "bbb");
+  auto Provider = makeIntrusiveRefCnt<FunctionTypoProvider>("aaa", "bbb");
   DiagnosticWatcher Watcher("aaa", "bbb");
-  Installer->PushSource(&Provider);
+  Installer->PushSource(Provider.get());
   Installer->PushWatcher(&Watcher);
   std::vector<std::string> Args(1, "-std=c++11");
   ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
       std::move(Installer), "namespace AAA { } void foo() { AAA::aaa(); }",
       Args));
-  ASSERT_LE(0, Provider.CallCount);
+  ASSERT_LE(0, Provider->CallCount);
   ASSERT_EQ(1, Watcher.SeenCount);
 }
 
 // We should only try MaybeDiagnoseMissingCompleteType if we can't otherwise
 // solve the problem.
 TEST(ExternalSemaSource, TryOtherTacticsBeforeDiagnosing) {
   auto Installer = std::make_unique<ExternalSemaSourceInstaller>();
-  CompleteTypeDiagnoser Diagnoser(false);
-  Installer->PushSource(&Diagnoser);
+  auto Diagnoser = makeIntrusiveRefCnt<CompleteTypeDiagnoser>(false);
+  Installer->PushSource(Diagnoser.get());
   std::vector<std::string> Args(1, "-std=c++11");
   // This code hits the class template specialization/class member of a class
   // template specialization checks in Sema::RequireCompleteTypeImpl.
   ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
       std::move(Installer),
       "template <typename T> struct S { class C { }; }; S<char>::C SCInst;",
       Args));
-  ASSERT_EQ(0, Diagnoser.CallCount);
+  ASSERT_EQ(0, Diagnoser->CallCount);
 }
 
 // The first ExternalSemaSource where MaybeDiagnoseMissingCompleteType returns
 // true should be the last one called.
 TEST(ExternalSemaSource, FirstDiagnoserTaken) {
   auto Installer = std::make_unique<ExternalSemaSourceInstaller>();
-  CompleteTypeDiagnoser First(false);
-  CompleteTypeDiagnoser Second(true);
-  CompleteTypeDiagnoser Third(true);
-  Installer->PushSource(&First);
-  Installer->PushSource(&Second);
-  Installer->PushSource(&Third);
+  auto First = makeIntrusiveRefCnt<CompleteTypeDiagnoser>(false);
+  auto Second = makeIntrusiveRefCnt<CompleteTypeDiagnoser>(true);
+  auto Third = makeIntrusiveRefCnt<CompleteTypeDiagnoser>(true);
+  Installer->PushSource(First.get());
+  Installer->PushSource(Second.get());
+  Installer->PushSource(Third.get());
   std::vector<std::string> Args(1, "-std=c++11");
   ASSERT_FALSE(clang::tooling::runToolOnCodeWithArgs(
       std::move(Installer), "class Incomplete; Incomplete IncompleteInstance;",
       Args));
-  ASSERT_EQ(1, First.CallCount);
-  ASSERT_EQ(1, Second.CallCount);
-  ASSERT_EQ(0, Third.CallCount);
+  ASSERT_EQ(1, First->CallCount);
+  ASSERT_EQ(1, Second->CallCount);
+  ASSERT_EQ(0, Third->CallCount);
 }
 
 } // anonymous namespace