Wed Oct 3 11:16:06 PDT 2012

On Oct 2, 2012, at 2:09 AM, Axel Naumann <Axel.Naumann at cern.ch> wrote:

> Author: axel
> Date: Tue Oct  2 04:09:43 2012
> New Revision: 164993
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=164993&view=rev
> Log:
> Merge pending instantiations instead of overwriting existing ones.
> Check whether a pending instantiation needs to be instantiated (or whether an instantiation already exists).
> Verify the size of the PendingInstantiations record (was only checking size of existing PendingInstantiations).

Okay. Some comments below.

> Migrate Obj-C++ part of redecl-merge into separate test, now that this is growing.
> templates.mm: test that CodeGen has seen exactly one definition of template instantiations.
> redecl-merge.m: use "@" specifier for expected-diagnostics.

Thanks for migrating this.

> Added:
>    cfe/trunk/test/Modules/Inputs/templates-left.h   (with props)
>    cfe/trunk/test/Modules/Inputs/templates-right.h   (with props)
>    cfe/trunk/test/Modules/Inputs/templates-top.h   (with props)
>    cfe/trunk/test/Modules/templates.mm
> Modified:
>    cfe/trunk/include/clang/Serialization/ASTReader.h
>    cfe/trunk/lib/Serialization/ASTReader.cpp
>    cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>    cfe/trunk/test/Modules/Inputs/module.map
>    cfe/trunk/test/Modules/Inputs/redecl-merge-bottom.h
>    cfe/trunk/test/Modules/Inputs/redecl-merge-left.h
>    cfe/trunk/test/Modules/Inputs/redecl-merge-right.h
>    cfe/trunk/test/Modules/Inputs/redecl-merge-top.h
>    cfe/trunk/test/Modules/redecl-merge.m
> 
> Modified: cfe/trunk/include/clang/Serialization/ASTReader.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTReader.h?rev=164993&r1=164992&r2=164993&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Serialization/ASTReader.h (original)
> +++ cfe/trunk/include/clang/Serialization/ASTReader.h Tue Oct  2 04:09:43 2012
> @@ -687,7 +687,7 @@
>   /// Objective-C protocols.
>   std::deque<Decl *> InterestingDecls;
> 
> -  /// \brief The set of redeclarable declaraations that have been deserialized
> +  /// \brief The set of redeclarable declarations that have been deserialized
>   /// since the last time the declaration chains were linked.
>   llvm::SmallPtrSet<Decl *, 16> RedeclsDeserialized;
> 
> @@ -854,6 +854,10 @@
> 
>   void finishPendingActions();
> 
> +  /// \brief Whether D needs to be instantiated, i.e. whether an instantiation
> +  /// for D does not exist yet.
> +  bool needPendingInstantiation(ValueDecl* D) const;
> +
>   /// \brief Produce an error diagnostic and return true.
>   ///
>   /// This routine should only be used for fatal errors that have to
> 
> Modified: cfe/trunk/lib/Serialization/ASTReader.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=164993&r1=164992&r2=164993&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Serialization/ASTReader.cpp (original)
> +++ cfe/trunk/lib/Serialization/ASTReader.cpp Tue Oct  2 04:09:43 2012
> @@ -2223,13 +2223,15 @@
> 
>     case PENDING_IMPLICIT_INSTANTIATIONS:
>       if (PendingInstantiations.size() % 2 != 0) {
> +        Error("Invalid existing PendingInstantiations");
> +        return Failure;
> +      }
> +
> +      if (Record.size() % 2 != 0) {
>         Error("Invalid PENDING_IMPLICIT_INSTANTIATIONS block");
>         return Failure;
>       }
> -        
> -      // Later lists of pending instantiations overwrite earlier ones.
> -      // FIXME: This is most certainly wrong for modules.
> -      PendingInstantiations.clear();
> +
>       for (unsigned I = 0, N = Record.size(); I != N; /* in loop */) {
>         PendingInstantiations.push_back(getGlobalDeclID(F, Record[I++]));
>         PendingInstantiations.push_back(
> @@ -5592,7 +5594,11 @@
>     ValueDecl *D = cast<ValueDecl>(GetDecl(PendingInstantiations[Idx++]));
>     SourceLocation Loc
>       = SourceLocation::getFromRawEncoding(PendingInstantiations[Idx++]);
> -    Pending.push_back(std::make_pair(D, Loc));
> +
> +    // For modules, find out whether an instantiation already exists
> +    if (!getContext().getLangOpts().Modules
> +        || needPendingInstantiation(D))
> +      Pending.push_back(std::make_pair(D, Loc));
>   }  
>   PendingInstantiations.clear();
> }

Why do we even bother with needPendingInstantiation()? Extra pending instantiations don't actually do any harm, because the instantiation routines that Sema::PerformPendingInstantiations delegates to---InstantiateFunctionDefinition and InstantiateStaticDataMemberDefinition---already know how to return early if the thing they are supposed to instantiate already has a definition. So we're doing a bunch of work up front in the ASTReader to save us from a cheap check in Sema.

FWIW, all tests pass if needPendingInstantiation(D) here always returns true. If you have other code that the call to needPendingInstantiation() fixes, it's a symptom of a different problem.

> Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=164993&r1=164992&r2=164993&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)
> +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Tue Oct  2 04:09:43 2012
> @@ -2156,7 +2156,7 @@
>   // loading, and some declarations may still be initializing.
>   if (isConsumerInterestedIn(D))
>     InterestingDecls.push_back(D);
> -  
> +
>   return D;
> }
> 
> @@ -2504,3 +2504,60 @@
>     }
>   }
> }
> +
> +/// \brief Return a template specialization of ND (should be a TemplateDecl)
> +///  that matches FD or TD.
> +static NamedDecl* findMatchingSpecialization(FunctionDecl* FD,
> +                                             ClassTemplateSpecializationDecl*TD,
> +                                             NamedDecl* ND) {
> +  TemplateDecl* Templt = dyn_cast<TemplateDecl>(ND);
> +  if (!Templt) return 0;
> +  if (FD) {
> +    FunctionTemplateDecl* FTD = dyn_cast<FunctionTemplateDecl>(Templt);
> +    if (!FTD) return 0;
> +    const TemplateArgumentList* TmpltArgs = FD->getTemplateSpecializationArgs();
> +    assert(TmpltArgs || "Template without arguments");
> +    void* InsertionPoint;
> +    return FTD->findSpecialization(TmpltArgs->data(), TmpltArgs->size(),
> +                                   InsertionPoint);

Function templates can be overloaded, and you're not checking that FD is actually a specialization of FTD, so it looks like this can confuse instantiations of different templates that happen to have the same template arguments, e.g., f<int> for

	template<typename T> void f(T);
	template<typename T> void f(T*);

> +  } else {
> +    ClassTemplateDecl* CTD = dyn_cast<ClassTemplateDecl>(Templt);
> +    if (!CTD) return 0;
> +    const TemplateArgumentList& TmpltArgs = TD->getTemplateArgs();
> +    void* InsertionPoint;
> +    return CTD->findSpecialization(TmpltArgs.data(), TmpltArgs.size(),
> +                                   InsertionPoint);
> +  }
> +  return 0;
> +}
> +
> +/// \brief Find out whether an instantiation (outside the module) already exists
> +bool ASTReader::needPendingInstantiation(ValueDecl* D) const {
> +  DeclContext *DC = D->getDeclContext()->getRedeclContext();
> +  DeclarationName Name = D->getDeclName();
> +  assert(Name && "unnamed template");
> +
> +  FunctionDecl* FD = dyn_cast<FunctionDecl>(D);
> +  ClassTemplateSpecializationDecl* CD
> +    = FD ? 0 : dyn_cast<ClassTemplateSpecializationDecl>(D);
> +
> +  NamedDecl* FoundSpecialization = 0;
> +  if (DC->isTranslationUnit() && SemaObj) {
> +    IdentifierResolver &IdResolver = SemaObj->IdResolver;
> +    for (IdentifierResolver::iterator I = IdResolver.begin(Name), 
> +           IEnd = IdResolver.end();
> +         I != IEnd && !FoundSpecialization; ++I)
> +      FoundSpecialization = findMatchingSpecialization(FD, CD, *I);
> +  } else {
> +    // templates are redeclarables, i.e. they must have been merged into
> +    // the primary context. Use localUncachedLookup to not pick up template
> +    // decls from modules again.
> +    llvm::SmallVector<NamedDecl*, 6> Results;
> +    DC->getPrimaryContext()->localUncachedLookup(Name, Results);
> +    for (llvm::SmallVector<NamedDecl *, 6>::const_iterator
> +           I = Results.begin(), E = Results.end();
> +         I != E && FoundSpecialization; ++I)
> +      FoundSpecialization = findMatchingSpecialization(FD, CD, *I);
> +  }
> +  return FoundSpecialization && isSameEntity(FoundSpecialization, D);
> +}

Why this this walking through trying to match up names? If D is fact an instantiation, we can find the template (or member of a template) from which it was instantiated directly, rather than searching for it.

	- Doug