[SemaTemplate] Stop passing insertion position around during VarTemplate instantiation

[bruno]

Summary

They can get stale at use time because of updates from other recursive specializations. Instead, rely on the existence of previous declarations to add the specialization.

[hoy]

The bug is exposed by instantiating a large variadic template. I haven't managed to get down to a decent size of regression test.

[rsmith]

Thanks, this is pretty subtle. Here's a small C++20 testcase that invalidates InsertPos while matching partial specializations:

template<typename T, int N> constexpr bool v = true;
template<int N> requires v<int, N-1> constexpr bool v<int, N> = true;
template<> constexpr bool v<int, 0> = true;
int k = v<int, 500>;

clang/lib/Sema/SemaTemplate.cpp

4485

All the handling of InsertPos in BuildVarTemplateInstantiation and below looks wrong to me, with the same bug that this code had. We end up in VisitVarTemplateSpecializationDecl which does this:

// Do substitution on the type of the declaration
TypeSourceInfo *DI =
    SemaRef.SubstType(D->getTypeSourceInfo(), TemplateArgs,
                      D->getTypeSpecStartLoc(), D->getDeclName());
if (!DI)
  return nullptr;

if (DI->getType()->isFunctionType()) {
  SemaRef.Diag(D->getLocation(), diag::err_variable_instantiates_to_function)
      << D->isStaticDataMember() << DI->getType();
  return nullptr;
}

// Build the instantiated declaration
VarTemplateSpecializationDecl *Var = VarTemplateSpecializationDecl::Create(
    SemaRef.Context, Owner, D->getInnerLocStart(), D->getLocation(),
    VarTemplate, DI->getType(), DI, D->getStorageClass(), Converted);
Var->setTemplateArgsInfo(TemplateArgsInfo);
if (InsertPos)
  VarTemplate->AddSpecialization(Var, InsertPos);

But the call to SubstType can absolutely invalidate InsertPos; here's a test case for a crash caused by that:

template<typename T, int N> T v;
template<int N> decltype(v<int, N-1>) v<int, N>;
template<> int v<int, 0>;
int k = v<int, 500>;

So I think we should remove the InsertPos parameter from this entire cluster of functions. No use of it is correct.

With InsertPos removed, we still need to know whether to call AddSpecialization or not. We should in principle do that if and only if !PrevDecl, but the call to VisitVarTemplateSpecializationDecl in InstantiateVariableDefinition doesn't pass in a PrevDecl even though it has one (OldVar). I wonder if we can change InstantiateVariableDefinition to pass OldVar in as the PrevDecl and remove its call to MergeVarDecl and probably also its call to InstantiateVariableInitializer -- since BuildVariableInstantiation should then do all of that for us.

[bruno]

I'm taking this over from Hongtao (w/ his blessing :)

[bruno]

Hi Richard. Thanks for the quick feedback and for the testcase!

The updated patch implements your suggestions. BuildVariableInstantiation seems to cover for MergeVarDecl however it doesn't seem to be enough for InstantiateVariableInitializer (breaks several tests) most likely because InstantiatingSpecFromTemplate prevents it to be called in BuildVariableInstantiation.

[bruno]

Add context

[bruno]

Ping!

[bruno]

Ping!

[rsmith]

Thanks!

clang/include/clang/Sema/Sema.h

       const TemplateArgumentList &TemplateArgList,
       const TemplateArgumentListInfo &TemplateArgsInfo,
       SmallVectorImpl<TemplateArgument> &Converted,
-      SourceLocation PointOfInstantiation, void *InsertPos,
+      SourceLocation PointOfInstantiation,
       LateInstantiatedAttrVec *LateAttrs = nullptr,
       LocalInstantiationScope *StartingScope = nullptr);
   VarTemplateSpecializationDecl *CompleteVarTemplateSpecializationDecl(

clang/include/clang/Sema/Template.h

                         TagDecl *NewDecl);
 
     Decl *VisitVarTemplateSpecializationDecl(
-        VarTemplateDecl *VarTemplate, VarDecl *FromVar, void *InsertPos,
+        VarTemplateDecl *VarTemplate, VarDecl *FromVar,
         const TemplateArgumentListInfo &TemplateArgsInfo,
         ArrayRef<TemplateArgument> Converted,
         VarTemplateSpecializationDecl *PrevDecl = nullptr);

clang/lib/Sema/SemaTemplate.cpp

   // FIXME: LateAttrs et al.?
   VarTemplateSpecializationDecl *Decl = BuildVarTemplateInstantiation(
       Template, InstantiationPattern, *InstantiationArgs, TemplateArgs,
-      Converted, TemplateNameLoc, InsertPos /*, LateAttrs, StartingScope*/);
+      Converted, TemplateNameLoc /*, LateAttrs, StartingScope*/);
   if (!Decl)
     return true;
 

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

     return nullptr;
 
   return VisitVarTemplateSpecializationDecl(
-      InstVarTemplate, D, InsertPos, VarTemplateArgsInfo, Converted, PrevDecl);
+      InstVarTemplate, D, VarTemplateArgsInfo, Converted, PrevDecl);
 }
 
 Decl *TemplateDeclInstantiator::VisitVarTemplateSpecializationDecl(
-    VarTemplateDecl *VarTemplate, VarDecl *D, void *InsertPos,
+    VarTemplateDecl *VarTemplate, VarDecl *D,
     const TemplateArgumentListInfo &TemplateArgsInfo,
     ArrayRef<TemplateArgument> Converted,
     VarTemplateSpecializationDecl *PrevDecl) {

       SemaRef.Context, Owner, D->getInnerLocStart(), D->getLocation(),
       VarTemplate, DI->getType(), DI, D->getStorageClass(), Converted);
   Var->setTemplateArgsInfo(TemplateArgsInfo);
-  if (InsertPos)
+  if (!PrevDecl) {
+    void *InsertPos = nullptr;
+    VarTemplate->findSpecialization(Converted, InsertPos);
     VarTemplate->AddSpecialization(Var, InsertPos);
+  }
 
   if (SemaRef.getLangOpts().OpenCL)
     SemaRef.deduceOpenCLAddressSpace(Var);

     const TemplateArgumentList &TemplateArgList,
     const TemplateArgumentListInfo &TemplateArgsInfo,
     SmallVectorImpl<TemplateArgument> &Converted,
-    SourceLocation PointOfInstantiation, void *InsertPos,
+    SourceLocation PointOfInstantiation,
     LateInstantiatedAttrVec *LateAttrs,
     LocalInstantiationScope *StartingScope) {
   if (FromVar->isInvalidDecl())

 
   return cast_or_null<VarTemplateSpecializationDecl>(
       Instantiator.VisitVarTemplateSpecializationDecl(
-          VarTemplate, FromVar, InsertPos, TemplateArgsInfo, Converted));
+          VarTemplate, FromVar, TemplateArgsInfo, Converted));
 }
 
 /// Instantiates a variable template specialization by completing it

     TemplateDeclInstantiator Instantiator(*this, Var->getDeclContext(),
                                           TemplateArgs);
     Var = cast_or_null<VarDecl>(Instantiator.VisitVarTemplateSpecializationDecl(
-        VarSpec->getSpecializedTemplate(), Def, nullptr,
-        VarSpec->getTemplateArgsInfo(), VarSpec->getTemplateArgs().asArray()));
+        VarSpec->getSpecializedTemplate(), Def, VarSpec->getTemplateArgsInfo(),
+        VarSpec->getTemplateArgs().asArray(), VarSpec));
     if (Var) {
       llvm::PointerUnion<VarTemplateDecl *,
                          VarTemplatePartialSpecializationDecl *> PatternPtr =

         cast<VarTemplateSpecializationDecl>(Var)->setInstantiationOf(
             Partial, &VarSpec->getTemplateInstantiationArgs());
 
-      // Merge the definition with the declaration.
-      LookupResult R(*this, Var->getDeclName(), Var->getLocation(),
-                     LookupOrdinaryName, forRedeclarationInCurContext());
-      R.addDecl(OldVar);
-      MergeVarDecl(Var, R);
-
       // Attach the initializer.
       InstantiateVariableInitializer(Var, Def, TemplateArgs);
     }

clang/test/SemaTemplate/instantiate-var-template.cpp

   template<typename... T> A<T...> models;
   template<> struct B models<>; // expected-error {{incomplete type 'struct B'}} expected-note {{forward declaration}}
 }
+
+namespace InvalidInsertPos {
+  template<typename T, int N> T v;
+  template<int N> decltype(v<int, N-1>) v<int, N>;
+  template<> int v<int, 0>;
+  int k = v<int, 500>;
+}