[Sema] Fix an error-on-valid with friends and templates

[erik.pilkington]

Summary

Clang used to error out on the attached testcase, due to multiple definitions of foo<int>. The problem is that multiple FunctionTemplateDecl::Common pointers are created for the two FunctionTemplateDeclarations foo in the redeclaration chain, each with their own copy of the instantiation of f<int>.

This patch fixes the problem by using the previous function template in a call to FuntionTemplateDecl::getInjectedTemplateArgs() (this was the call that caused the Common pointer to be allocated in the redeclaration). Most of this patch is just boilerplate to get the function template declaration down the stack to where it's needed.

Fixes rdar://44810129

Thanks for taking a look!
Erik

[erik.pilkington]

clang/lib/Sema/SemaDecl.cpp

10015 ↗

(On Diff #168876)

The problem is here, MergeFunctionDecl() needs the injected template args, but...

10026 ↗

(On Diff #168876)

... we don't link the two declarations until here!

[rjmccall]

The linking does actually happen in this test case, right? Can we just do something when linking them to unify their Common structures?

[erik.pilkington]

In D53046#1259933, @rjmccall wrote:

The linking does actually happen in this test case, right? Can we just do something when linking them to unify their Common structures?

Yep, that would work too I think. We can't properly merge in the general case, because we could have to decide between more than one identical instantiations of different template redecls, but in this case the only ordering problem is with the injected template arguments, which I believe we can merge (by just discarding the new arguments, if a previous redeclaration already has some). It would definitely be less intrusive, but its a more complicated invariant. Do you think that is a better solution? I could go either way.

[rjmccall]

In D53046#1259945, @erik.pilkington wrote:

In D53046#1259933, @rjmccall wrote:

The linking does actually happen in this test case, right? Can we just do something when linking them to unify their Common structures?

Yep, that would work too I think. We can't properly merge in the general case, because we could have to decide between more than one identical instantiations of different template redecls, but in this case the only ordering problem is with the injected template arguments, which I believe we can merge (by just discarding the new arguments, if a previous redeclaration already has some). It would definitely be less intrusive, but its a more complicated invariant. Do you think that is a better solution? I could go either way.

Well, we're really still in the middle of creating the new declaration, so I'm not too worried about the invariants. And yeah, we can safely discard the new arguments because IIUC this is just a cache.

[erik.pilkington]

Merge the common pointers rather than trying to use the previous one. Thanks!

[rjmccall]

LGTM.

cfe/trunk/include/clang/AST/DeclTemplate.h

public:
   /// template parameters of this function template.
   ///
   /// Although the C++ standard has no notion of the "injected" template
   /// arguments for a function template, the notion is convenient when
   /// we need to perform substitutions inside the definition of a function
   /// template.
   ArrayRef<TemplateArgument> getInjectedTemplateArgs();
 
+  /// Merge our RedeclarableTemplateDecl::Common with \param Prev.
+  void mergePrevDecl(FunctionTemplateDecl *Prev);
+
   /// Create a function template node.
   static FunctionTemplateDecl *Create(ASTContext &C, DeclContext *DC,
                                       SourceLocation L,
                                       DeclarationName Name,
                                       TemplateParameterList *Params,
                                       NamedDecl *Decl);
 
   /// Create an empty function template node.

cfe/trunk/lib/AST/DeclTemplate.cpp

CommonPtr->InjectedArgs =
         new (Context) TemplateArgument[TemplateArgs.size()];
     std::copy(TemplateArgs.begin(), TemplateArgs.end(),
               CommonPtr->InjectedArgs);
   }
 
   return llvm::makeArrayRef(CommonPtr->InjectedArgs, Params->size());
 }
 
+void FunctionTemplateDecl::mergePrevDecl(FunctionTemplateDecl *Prev) {
+  using Base = RedeclarableTemplateDecl;
+
+  // If we haven't created a common pointer yet, then it can just be created
+  // with the usual method.
+  if (!Base::Common)
+    return;
+
+  Common *ThisCommon = static_cast<Common *>(Base::Common);
+  Common *PrevCommon = nullptr;
+  SmallVector<FunctionTemplateDecl *, 8> PreviousDecls;
+  for (; Prev; Prev = Prev->getPreviousDecl()) {
+    if (Prev->Base::Common) {
+      PrevCommon = static_cast<Common *>(Prev->Base::Common);
+      break;
+    }
+    PreviousDecls.push_back(Prev);
+  }
+
+  // If the previous redecl chain hasn't created a common pointer yet, then just
+  // use this common pointer.
+  if (!PrevCommon) {
+    for (auto *D : PreviousDecls)
+      D->Base::Common = ThisCommon;
+    return;
+  }
+
+  // Ensure we don't leak any important state.
+  assert(ThisCommon->Specializations.size() == 0 &&
+         !ThisCommon->InstantiatedFromMember.getPointer() &&
+         !ThisCommon->InstantiatedFromMember.getInt() &&
+         "Can't merge incompatible declarations!");
+
+  Base::Common = PrevCommon;
+}
+
 //===----------------------------------------------------------------------===//
 // ClassTemplateDecl Implementation
 //===----------------------------------------------------------------------===//
 
 ClassTemplateDecl *ClassTemplateDecl::Create(ASTContext &C,
                                              DeclContext *DC,
                                              SourceLocation L,
                                              DeclarationName Name,

cfe/trunk/lib/Sema/SemaDecl.cpp

if (Redeclaration) {
     }
 
     Previous.clear();
     Previous.addDecl(OldDecl);
 
     if (FunctionTemplateDecl *OldTemplateDecl =
             dyn_cast<FunctionTemplateDecl>(OldDecl)) {
       auto *OldFD = OldTemplateDecl->getTemplatedDecl();
-      NewFD->setPreviousDeclaration(OldFD);
-      adjustDeclContextForDeclaratorDecl(NewFD, OldFD);
       FunctionTemplateDecl *NewTemplateDecl
         = NewFD->getDescribedFunctionTemplate();
       assert(NewTemplateDecl && "Template/non-template mismatch");
+
+      // The call to MergeFunctionDecl above may have created some state in
+      // NewTemplateDecl that needs to be merged with OldTemplateDecl before we
+      // can add it as a redeclaration.
+      NewTemplateDecl->mergePrevDecl(OldTemplateDecl);
+
+      NewFD->setPreviousDeclaration(OldFD);
+      adjustDeclContextForDeclaratorDecl(NewFD, OldFD);
       if (NewFD->isCXXClassMember()) {
         NewFD->setAccess(OldTemplateDecl->getAccess());
         NewTemplateDecl->setAccess(OldTemplateDecl->getAccess());
       }
 
       // If this is an explicit specialization of a member that is a function
       // template, mark it as a member specialization.
       if (IsMemberSpecialization &&

cfe/trunk/test/SemaCXX/friend-template-redecl.cpp

+// RUN: %clang_cc1 -std=c++17 -verify -emit-llvm-only %s
+
+// expected-no-diagnostics
+
+template <class T> void bar(const T &t) { foo(t); }
+
+template <class>
+struct HasFriend {
+  template <class T>
+  friend void foo(const HasFriend<T> &m) noexcept(false);
+};
+
+template <class T>
+void foo(const HasFriend<T> &m) noexcept(false) {}
+
+void f() {
+  HasFriend<int> x;
+  foo(x);
+  bar(x);
+}