[Sema] Fix PR14211 Crash for explicit instantiation of overloaded template function within class template

[hintonda]

Summary

Fix PR14211

Only add FunctionDecl's that are actual template methods to the set of
matches passed to getMostSpecializied() -- otherwise it will
assert/crash. If a match to a non-template method is found, save the
pointer and don't call getMostSpecialized().

[rjmccall]

lib/Sema/SemaTemplate.cpp

7829

Please name this variable NonTemplateMatch instead of Specialization, then initialize the later Specialization variable to it if it's non-null. That's more self-documenting, and it avoids readability problems with the fact that this loop actually declares its own Specialization variable that would shadow this one.

Please also rename Matches to TemplateMatches.

7841–7847

This cast shouldn't be necessary. CXXMethodDecl is a subclass of FunctionDecl.

[hintonda]

• Renamed variables to make code self-documenting

[hintonda]

Btw, I don't have commit access, so if accepted, could you please commit for me.

[rjmccall]

lib/Sema/SemaTemplate.cpp

7848

Could you add an assertion here that NonTemplateMatch is still null? That should definitely never trip.

Hmm, actually, it might trip in invalid code; you should include a test case like

template <class T, class U> class A { void foo(T) {} void foo(U) {} };
template void A<int, int>::foo(int);

[hintonda]

lib/Sema/SemaTemplate.cpp

7848

I'll add the new test, but we do check for null below, i.e., we assign NonTemplateMatch to the original Specialization pointer, and then check the matches. If we don't find one, we true on line 7883.

[rjmccall]

lib/Sema/SemaTemplate.cpp

7848

No, I mean an assertion that we're not re-assigning NonTemplateMatch, i.e. that we didn't find two different non-template functions like this.

[hintonda]

• Added assert and additional test

[hintonda]

lib/Sema/SemaTemplate.cpp

7848

Got it...

[hintonda]

Fixes PR14211.

Since getMostSpecialized only handles FunctionTemplateDecl matches,
keep track of non-FunctionTemplateDecl matches and only call
getMostSpecialized if no non-FunctionTemplateDecl matches are found.

Added tests.

[rjmccall]

lib/Sema/SemaTemplate.cpp

7850

These continues that you're adding are already at the end of the loop body, so this code is really confusing.

I think the template-specialization check here is trying to do the same thing you're doing: treat non-template matches specially. But your way of handling it is better, so I would just remove the check and all the breaks and continues so that we add the match to either NonTemplateMatch or TemplateMatches. (Please do rename Matches to TemplateMatches, though.)

[hintonda]

• Addressed comments.

[rjmccall]

Thanks, looks great. Do you still not have commit access?

[hintonda]

Great, thanks John.

Yes, that's correct -- I do not have commit access.

[hintonda]

Since I don't have commit access, could you commit this for me?

thanks...
don

[rjmccall]

Sure thing, r304951.

lib/Sema/SemaTemplate.cpp

if (D.getName().getKind() == UnqualifiedId::IK_TemplateId) {
     HasExplicitTemplateArgs = true;
   }
 
   // C++ [temp.explicit]p1:
   //   A [...] function [...] can be explicitly instantiated from its template.
   //   A member function [...] of a class template can be explicitly
   //  instantiated from the member definition associated with its class
   //  template.
-  UnresolvedSet<8> Matches;
+  UnresolvedSet<8> TemplateMatches;
+  FunctionDecl *NonTemplateMatch = nullptr;
   TemplateSpecCandidateSet FailedCandidates(D.getIdentifierLoc());
   for (LookupResult::iterator P = Previous.begin(), PEnd = Previous.end();
        P != PEnd; ++P) {
     NamedDecl *Prev = *P;
     if (!HasExplicitTemplateArgs) {
       if (CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(Prev)) {
         QualType Adjusted = adjustCCAndNoReturn(R, Method->getType());
         if (Context.hasSameUnqualifiedType(Method->getType(), Adjusted)) {
-          Matches.clear();
+          if (Method->getPrimaryTemplate()){
+            TemplateMatches.clear();
 
-          Matches.addDecl(Method, P.getAccess());
+            TemplateMatches.addDecl(Method, P.getAccess());
             if (Method->getTemplateSpecializationKind() == TSK_Undeclared)
               break;
+          } else {
+            assert(!NonTemplateMatch && "Found more than one non-template match");
+            NonTemplateMatch = Method;
+          }
         }
       }
     }
 
     FunctionTemplateDecl *FunTmpl = dyn_cast<FunctionTemplateDecl>(Prev);
     if (!FunTmpl)
       continue;
 
     TemplateDeductionInfo Info(FailedCandidates.getLocation());
     FunctionDecl *Specialization = nullptr;
     if (TemplateDeductionResult TDK
           = DeduceTemplateArguments(FunTmpl,
                                (HasExplicitTemplateArgs ? &TemplateArgs
                                                         : nullptr),
                                     R, Specialization, Info)) {
       // Keep track of almost-matches.
       FailedCandidates.addCandidate()
           .set(FunTmpl->getTemplatedDecl(),
                MakeDeductionFailureInfo(Context, TDK, Info));
       (void)TDK;
       continue;
     }
 
-    Matches.addDecl(Specialization, P.getAccess());
+    TemplateMatches.addDecl(Specialization, P.getAccess());
   }
 
+  FunctionDecl *Specialization = NonTemplateMatch;
   // Find the most specialized function template specialization.
+  if (!Specialization) {
     UnresolvedSetIterator Result = getMostSpecialized(
-      Matches.begin(), Matches.end(), FailedCandidates,
+        TemplateMatches.begin(), TemplateMatches.end(), FailedCandidates,
         D.getIdentifierLoc(),
         PDiag(diag::err_explicit_instantiation_not_known) << Name,
         PDiag(diag::err_explicit_instantiation_ambiguous) << Name,
         PDiag(diag::note_explicit_instantiation_candidate));
 
-  if (Result == Matches.end())
+    if (Result == TemplateMatches.end())
       return true;
 
     // Ignore access control bits, we don't need them for redeclaration checking.
-  FunctionDecl *Specialization = cast<FunctionDecl>(*Result);
+    Specialization = cast<FunctionDecl>(*Result);
+  }
 
   // C++11 [except.spec]p4
   // In an explicit instantiation an exception-specification may be specified,
   // but is not required.
   // If an exception-specification is specified in an explicit instantiation
   // directive, it shall be compatible with the exception-specifications of
   // other declarations of that function.
   if (auto *FPT = R->getAs<FunctionProtoType>())

test/SemaCXX/pr14211-crash-on-invalid.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// Don't crash (PR14211).
+
+template<class T>
+struct X {
+  void bar(T t) {}
+  void foo(T t, int i) {}
+  template<class U>
+  void foo(T t, U i) {}
+};
+
+template class X<double>; // expected-note {{previous explicit instantiation is here}}
+template void X<double>::foo(double t, int i); // expected-error {{duplicate explicit instantiation of 'foo'}}
+template void X<double>::foo<int>(double t, int i);
+
+
+
+template <class T, class U>
+class A {
+  void foo(T) {} // expected-note {{previous declaration is here}}
+  void foo(U) {} // expected-error {{multiple overloads of 'foo' instantiate to the same signature 'void (int)'}}
+};
+template void A<int, int>::foo(int); // expected-note {{in instantiation of template class 'A<int, int>' requested here}}