Method pool in modules: sync up out of date selectors before writing the module

[manmanren]

Summary

Method Pool in modules: we make sure that if a module contains an entry for a selector, the entry should be complete, containing
everything introduced by that module and all modules it imports.

Before writing out the method pool of a module, we sync up the out of date selectors by pulling in methods for the selectors from all modules it imports.

In ReadMethodPool, after pulling in the method pool entry for module A, this lets us skip the modules that module A imports.

[aprantl]

include/clang/Serialization/ASTReader.h

657

///

test/Modules/method_pool_write.m

4

Is there anything meaningful that could be CHECKed in the output?

[manmanren]

Adrian,

Thanks for reviewing the patch!

Manman

include/clang/Serialization/ASTReader.h

657

Yes.

test/Modules/method_pool_write.m

4

This commit makes sure that we don't emit an error message for not being able to find the method. That is why it checks for no diagnostics. Maybe I should use -fsyntax-only?

[aprantl]

test/Modules/method_pool_write.m

4

Using -fsyntax-only and expected-no-diagnostics is fine, too. I was just wondering whether it might make sense (or is possible at all) to check that the method was indeed found in the emitted IR on top of that.

[doug.gregor]

Mostly looks good, but see my comment about MapVector invalidation.

lib/Serialization/ASTWriter.cpp

4394

Are we certain that SelectorIDs cannot get updated during this iteration under any circumstances? If the underlying vector were to get reallocated, we'd end up with a horrible-to-reproduce use-after-free here. It might be worth copying SelectorIDs or indexing by an integer value rather than using the for-each loop.

[manmanren]

Addressing review comments from Doug and Adrian.

Manman

[doug.gregor]

LGTM!

[manmanren]

lib/Serialization/ASTWriter.cpp

4394

Nice catch. Thanks,

[manmanren]

In r268091.

include/clang/Sema/ExternalSemaSource.h

public:
 
   /// \brief Inform the semantic consumer that Sema is no longer available.
   virtual void ForgetSema() {}
 
   /// \brief Load the contents of the global method pool for a given
   /// selector.
   virtual void ReadMethodPool(Selector Sel);
 
+  /// Load the contents of the global method pool for a given
+  /// selector if necessary.
+  virtual void updateOutOfDateSelector(Selector Sel);
+
   /// \brief Load the set of namespaces that are known to the external source,
   /// which will be used during typo correction.
   virtual void ReadKnownNamespaces(
                            SmallVectorImpl<NamespaceDecl *> &Namespaces);
 
   /// \brief Load the set of used but not defined functions or variables with
   /// internal linkage, or used but not defined internal functions.
   virtual void

include/clang/Sema/MultiplexExternalSemaSource.h

public:
 
   /// \brief Inform the semantic consumer that Sema is no longer available.
   void ForgetSema() override;
 
   /// \brief Load the contents of the global method pool for a given
   /// selector.
   void ReadMethodPool(Selector Sel) override;
 
+  /// Load the contents of the global method pool for a given
+  /// selector if necessary.
+  void updateOutOfDateSelector(Selector Sel) override;
+
   /// \brief Load the set of namespaces that are known to the external source,
   /// which will be used during typo correction.
   void
   ReadKnownNamespaces(SmallVectorImpl<NamespaceDecl*> &Namespaces) override;
 
   /// \brief Load the set of used but not defined functions or variables with
   /// internal linkage, or used but not defined inline functions.
   void ReadUndefinedButUsed(

include/clang/Sema/Sema.h

public:
   typedef std::pair<CXXRecordDecl*, CXXSpecialMember> SpecialMemberDecl;
 
   /// The C++ special members which we are currently in the process of
   /// declaring. If this process recursively triggers the declaration of the
   /// same special member, we should act as if it is not yet declared.
   llvm::SmallSet<SpecialMemberDecl, 4> SpecialMembersBeingDeclared;
 
   void ReadMethodPool(Selector Sel);
+  void updateOutOfDateSelector(Selector Sel);
 
   /// Private Helper predicate to check for 'self'.
   bool isSelfExpr(Expr *RExpr);
   bool isSelfExpr(Expr *RExpr, const ObjCMethodDecl *Method);
 
   /// \brief Cause the active diagnostic on the DiagosticsEngine to be
   /// emitted. This is closely coupled to the SemaDiagnosticBuilder class and
   /// should not be used elsewhere.

include/clang/Serialization/ASTReader.h

private:
 
   /// global selector ID to produce a local ID.
   GlobalSelectorMapType GlobalSelectorMap;
 
   /// \brief The generation number of the last time we loaded data from the
   /// global method pool for this selector.
   llvm::DenseMap<Selector, unsigned> SelectorGeneration;
 
+  /// Whether a selector is out of date. We mark a selector as out of date
+  /// if we load another module after the method pool entry was pulled in.
+  llvm::DenseMap<Selector, bool> SelectorOutOfDate;
+
   struct PendingMacroInfo {
     ModuleFile *M;
     uint64_t MacroDirectivesOffset;
 
     PendingMacroInfo(ModuleFile *M, uint64_t MacroDirectivesOffset)
         : M(M), MacroDirectivesOffset(MacroDirectivesOffset) {}
   };
 
public:
   /// \brief Retrieve an iterator into the set of all identifiers
   /// in all loaded AST files.
   IdentifierIterator *getIdentifiers() override;
 
   /// \brief Load the contents of the global method pool for a given
   /// selector.
   void ReadMethodPool(Selector Sel) override;
 
+  /// Load the contents of the global method pool for a given
+  /// selector if necessary.
+  void updateOutOfDateSelector(Selector Sel) override;
+
   /// \brief Load the set of namespaces that are known to the external source,
   /// which will be used during typo correction.
   void ReadKnownNamespaces(
                          SmallVectorImpl<NamespaceDecl *> &Namespaces) override;
 
   void ReadUndefinedButUsed(
       llvm::MapVector<NamedDecl *, SourceLocation> &Undefined) override;
 

lib/Sema/MultiplexExternalSemaSource.cpp

for(size_t i = 0; i < Sources.size(); ++i)
     Sources[i]->ForgetSema();
 }
 
 void MultiplexExternalSemaSource::ReadMethodPool(Selector Sel) {
   for(size_t i = 0; i < Sources.size(); ++i)
     Sources[i]->ReadMethodPool(Sel);
 }
 
+void MultiplexExternalSemaSource::updateOutOfDateSelector(Selector Sel) {
+  for(size_t i = 0; i < Sources.size(); ++i)
+    Sources[i]->updateOutOfDateSelector(Sel);
+}
+
 void MultiplexExternalSemaSource::ReadKnownNamespaces(
                                    SmallVectorImpl<NamespaceDecl*> &Namespaces){
   for(size_t i = 0; i < Sources.size(); ++i)
     Sources[i]->ReadKnownNamespaces(Namespaces);
 }
 
 void MultiplexExternalSemaSource::ReadUndefinedButUsed(
     llvm::MapVector<NamedDecl *, SourceLocation> &Undefined) {

lib/Sema/Sema.cpp

void Sema::ActOnComment(SourceRange Comment) {
   }
   Context.addComment(RC);
 }
 
 // Pin this vtable to this file.
 ExternalSemaSource::~ExternalSemaSource() {}
 
 void ExternalSemaSource::ReadMethodPool(Selector Sel) { }
+void ExternalSemaSource::updateOutOfDateSelector(Selector Sel) { }
 
 void ExternalSemaSource::ReadKnownNamespaces(
                            SmallVectorImpl<NamespaceDecl *> &Namespaces) {
 }
 
 void ExternalSemaSource::ReadUndefinedButUsed(
     llvm::MapVector<NamedDecl *, SourceLocation> &Undefined) {}
 

lib/Sema/SemaDeclObjC.cpp

 
 /// \brief Read the contents of the method pool for a given selector from
 /// external storage.
 void Sema::ReadMethodPool(Selector Sel) {
   assert(ExternalSource && "We need an external AST source");
   ExternalSource->ReadMethodPool(Sel);
 }
 
+void Sema::updateOutOfDateSelector(Selector Sel) {
+  if (!ExternalSource)
+    return;
+  ExternalSource->updateOutOfDateSelector(Sel);
+}
+
 void Sema::AddMethodToGlobalPool(ObjCMethodDecl *Method, bool impl,
                                  bool instance) {
   // Ignore methods of invalid containers.
   if (cast<Decl>(Method->getDeclContext())->isInvalidDecl())
     return;
 
   if (ExternalSource)
     ReadMethodPool(Method->getSelector());

lib/Serialization/ASTReader.cpp

if (!Context.getLangOpts().CPlusPlus ||
     // For C++ modules, we don't need information on many identifiers (just
     // those that provide macros or are poisoned), so we mark all of
     // the interesting ones via PreloadIdentifierOffsets.
     for (IdentifierTable::iterator Id = PP.getIdentifierTable().begin(),
                                 IdEnd = PP.getIdentifierTable().end();
          Id != IdEnd; ++Id)
       Id->second->setOutOfDate(true);
   }
+  // Mark selectors as out of date.
+  for (auto Sel : SelectorGeneration)
+    SelectorOutOfDate[Sel.first] = true;
   
   // Resolve any unresolved module exports.
   for (unsigned I = 0, N = UnresolvedModuleRefs.size(); I != N; ++I) {
     UnresolvedModuleRef &Unresolved = UnresolvedModuleRefs[I];
     SubmoduleID GlobalID = getGlobalSubmoduleID(*Unresolved.File,Unresolved.ID);
     Module *ResolvedMod = getSubmodule(GlobalID);
 
     switch (Unresolved.Kind) {
static void addMethodsToPool(Sema &S, ArrayRef<ObjCMethodDecl *> Methods,
   }
 }
                              
 void ASTReader::ReadMethodPool(Selector Sel) {
   // Get the selector generation and update it to the current generation.
   unsigned &Generation = SelectorGeneration[Sel];
   unsigned PriorGeneration = Generation;
   Generation = getGeneration();
+  SelectorOutOfDate[Sel] = false;
   
   // Search for methods defined with this selector.
   ++NumMethodPoolLookups;
   ReadMethodPoolVisitor Visitor(*this, Sel, PriorGeneration);
   ModuleMgr.visit(Visitor);
 
   if (Visitor.getInstanceMethods().empty() &&
       Visitor.getFactoryMethods().empty())
void ASTReader::ReadMethodPool(Selector Sel) {
 
   // Add methods to the global pool *after* setting hasMoreThanOneDecl, since
   // when building a module we keep every method individually and may need to
   // update hasMoreThanOneDecl as we add the methods.
   addMethodsToPool(S, Visitor.getInstanceMethods(), Pos->second.first);
   addMethodsToPool(S, Visitor.getFactoryMethods(), Pos->second.second);
 }
 
+void ASTReader::updateOutOfDateSelector(Selector Sel) {
+  if (SelectorOutOfDate[Sel])
+    ReadMethodPool(Sel);
+}
+
 void ASTReader::ReadKnownNamespaces(
                           SmallVectorImpl<NamespaceDecl *> &Namespaces) {
   Namespaces.clear();
   
   for (unsigned I = 0, N = KnownNamespaces.size(); I != N; ++I) {
     if (NamespaceDecl *Namespace 
                 = dyn_cast_or_null<NamespaceDecl>(GetDecl(KnownNamespaces[I])))
       Namespaces.push_back(Namespace);

lib/Serialization/ASTWriter.cpp

for (const IdentifierInfo *II : IIs) {
       for (IdentifierResolver::iterator D = SemaRef.IdResolver.begin(II),
                                      DEnd = SemaRef.IdResolver.end();
            D != DEnd; ++D) {
         GetDeclRef(*D);
       }
     }
   }
 
+  // For method pool in the module, if it contains an entry for a selector,
+  // the entry should be complete, containing everything introduced by that
+  // module and all modules it imports. It's possible that the entry is out of
+  // date, so we need to pull in the new content here.
+
+  // It's possible that updateOutOfDateSelector can update SelectorIDs. To be
+  // safe, we copy all selectors out.
+  llvm::SmallVector<Selector, 256> AllSelectors;
+  for (auto &SelectorAndID : SelectorIDs)
+    AllSelectors.push_back(SelectorAndID.first);
+  for (auto &Selector : AllSelectors)
+    SemaRef.updateOutOfDateSelector(Selector);
+
   // Form the record of special types.
   RecordData SpecialTypes;
   AddTypeRef(Context.getRawCFConstantStringType(), SpecialTypes);
   AddTypeRef(Context.getFILEType(), SpecialTypes);
   AddTypeRef(Context.getjmp_bufType(), SpecialTypes);
   AddTypeRef(Context.getsigjmp_bufType(), SpecialTypes);
   AddTypeRef(Context.ObjCIdRedefinitionType, SpecialTypes);
   AddTypeRef(Context.ObjCClassRedefinitionType, SpecialTypes);

test/Modules/Inputs/MethodPoolCombined1.h

+
+@import MethodPoolString1;
+@interface A
+- (int)stringValue;
+@end
+

test/Modules/Inputs/MethodPoolCombined2.h

+@import MethodPoolString2;

test/Modules/Inputs/MethodPoolString1.h

+
+@interface S1
+- (int)stringValue;
+@end

test/Modules/Inputs/MethodPoolString2.h

+
+@interface S2
+- (int)stringValue;
+@end

test/Modules/Inputs/module.map

 }
 
 module ElaboratedTypeStructs {
   module Empty {}
   module Structs {
     header "elaborated-type-structs.h"
   }
 }
+
+// We import a module, then declare a method with selector stringValue in
+// MethodPoolCombined1.h. In MethodPoolCombined2.h, we import another module
+// that also contains a method for selector stringValue. We make sure that
+// the method pool entry for stringValue in this module is complete.
+module MethodPoolCombined {
+  header "MethodPoolCombined1.h"
+  header "MethodPoolCombined2.h"
+}
+
+module MethodPoolString1 {
+  header "MethodPoolString1.h"
+}
+
+module MethodPoolString2 {
+  header "MethodPoolString2.h"
+}

test/Modules/method_pool_write.m

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -fsyntax-only -I %S/Inputs %s -verify
+// expected-no-diagnostics
+
+@import MethodPoolCombined;
+@import MethodPoolString2;
+
+void message_kindof_object(__kindof S2 *kindof_S2) {
+  [kindof_S2 stringValue];
+}
+